You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "yousoph (via GitHub)" <gi...@apache.org> on 2026/06/18 01:32:37 UTC
[PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
yousoph opened a new pull request, #41181:
URL: https://github.com/apache/superset/pull/41181
### SUMMARY
Fixes the blank **Dashboard** field in the chart save modal (SaveModal) when the chart is already on one or more dashboards.
**Root cause:** `SaveModal.componentDidMount` only tried to pre-populate the Dashboard dropdown from two sources — the `dashboardId` URL param (only present when navigating from a dashboard view) or `sessionStorage`. But the `/api/v1/explore/` response already includes `metadata.dashboards` — the full list of dashboards the chart belongs to — and this data was available in Redux state (`explore.metadata`) but never read by SaveModal.
**Fix:** Add `metadata` to `mapStateToProps` and add a fallback in `componentDidMount`: when no `dashboardId` is found from URL/sessionStorage, load each dashboard from `metadata.dashboards` in parallel and pre-select the first one the user can edit.
Relates to the "Save & go to dashboard" UX bug reported internally as sc-104553.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — behavioral fix; no visual UI changes.
### TESTING INSTRUCTIONS
1. Save a chart to a dashboard.
2. Navigate to the chart at `/explore/?slice_id=<id>` directly (no `dashboard_id` URL param).
3. Open the Save modal.
4. **Before:** Dashboard field is blank. **After:** Dashboard field is pre-populated with the first dashboard the chart belongs to that you can edit.
5. "Save & go to dashboard" should now work from a direct chart URL.
Unit tests added:
- `pre-populates dashboard from metadata.dashboards when dashboardId prop is absent`
- `skips non-editable dashboards and picks the first editable one from metadata`
- `does not use metadata fallback when dashboardId prop is set`
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-104553 (internal)
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph merged PR #41181:
URL: https://github.com/apache/superset/pull/41181
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3434589130
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
Review Comment:
Declining — line 602 in this same file already uses the identical `setState = function (this: any, stateUpdate: any)` pattern. Adding specific types only to the new test helper would be inconsistent.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
Review Comment:
Declining — same reason as the adjacent comment: line 602 uses the identical `any` pattern for `setState` mocking. Consistent with existing style.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -162,6 +163,34 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
t('An error occurred while loading dashboard information.'),
);
}
+ } else {
+ const metadataDashboards = this.props.metadata?.dashboards;
+ if (metadataDashboards?.length) {
+ // Fallback: the chart is already on one or more dashboards (from Explore API
+ // metadata). Pre-populate with the first dashboard the user can edit so the
+ // "Save & go to dashboard" button works out of the box.
+ try {
+ const loaded = await Promise.all(
+ metadataDashboards.map(({ id }) =>
+ this.loadDashboard(id).catch(() => null),
+ ),
+ );
Review Comment:
Valid concern — fixing in a follow-up commit. Will switch to a sequential early-exit loop so we stop fetching once the first editable dashboard is found, instead of firing N concurrent requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3432482581
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -162,6 +163,34 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
t('An error occurred while loading dashboard information.'),
);
}
+ } else {
+ const metadataDashboards = this.props.metadata?.dashboards;
+ if (metadataDashboards?.length) {
+ // Fallback: the chart is already on one or more dashboards (from Explore API
+ // metadata). Pre-populate with the first dashboard the user can edit so the
+ // "Save & go to dashboard" button works out of the box.
+ try {
+ const loaded = await Promise.all(
+ metadataDashboards.map(({ id }) =>
+ this.loadDashboard(id).catch(() => null),
+ ),
+ );
Review Comment:
**Suggestion:** The metadata fallback fires one dashboard API request per entry with `Promise.all`, which can create an unbounded burst of concurrent network calls when a chart belongs to many dashboards. This can noticeably slow modal initialization and spike backend load; switch to a bounded-concurrency or early-exit approach (stop after finding the first editable dashboard) to avoid request storms. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Explore SaveModal fires many /api/v1/dashboard requests.
- ⚠️ Backend load grows with dashboards per chart.
- ⚠️ Save modal initialization slower for multi-dashboard charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the Explore view, clicking the Save button in the chart header dispatches
`setSaveChartModalVisibility(true)` in `ExploreChartHeader`
(superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:12-15), which sets
`saveModal.isVisible` in Redux via `saveModalActions`
(superset-frontend/src/explore/actions/saveModalActions.ts:51-55).
2. `ExploreViewContainer` reads `saveModal.isVisible` and, when `isSaveModalVisible` is
true, renders `<SaveModal>`
(superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:20-27), passing
`dashboardId` (derived from `explore.form_data.dashboardId` in `mapStateToProps` at lines
63-66 and returned at 1199-1215) and wiring the component to the `explore` slice of state.
3. On mount, `SaveModal.componentDidMount`
(superset-frontend/src/explore/components/SaveModal.tsx:61-115) first tries to determine a
`dashboardId` from props or `sessionStorage`; if none is found it falls into the `else`
branch at lines 87-115, obtains `metadataDashboards` from
`this.props.metadata?.dashboards`, where `metadata` is wired from `explore.metadata` in
`mapStateToProps` (superset-frontend/src/explore/components/SaveModal.tsx:125-139; type
defined in ExplorePageInitialData.metadata at
superset-frontend/src/explore/types.ts:83-99).
4. When a chart belongs to many dashboards (so `metadataDashboards` is a long array of `{
id, dashboard_title }` objects), `componentDidMount` executes `const loaded = await
Promise.all(metadataDashboards.map(({ id }) => this.loadDashboard(id).catch(() => null)))`
(lines 94-98 in SaveModal.tsx / lines 173-177 in the PR hunk), which calls `loadDashboard`
(SupersetClient GET to `/api/v1/dashboard/${id}` at 219-223) once for every dashboard
concurrently; there is no early exit or concurrency cap, so a chart added to N dashboards
causes N parallel HTTP requests and their corresponding backend work even though only the
first editable dashboard found by `.find(...)` at lines 99-101 is actually used.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=076c1b2c0914436fa5fdcace749da739&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=076c1b2c0914436fa5fdcace749da739&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.tsx
**Line:** 173:177
**Comment:**
*Performance: The metadata fallback fires one dashboard API request per entry with `Promise.all`, which can create an unbounded burst of concurrent network calls when a chart belongs to many dashboards. This can noticeably slow modal initialization and spike backend load; switch to a bounded-concurrency or early-exit approach (stop after finding the first editable dashboard) to avoid request storms.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=361c2fead3b5f6f554f547025f638d3997af743befeef81b264931a61a37ce1b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=361c2fead3b5f6f554f547025f638d3997af743befeef81b264931a61a37ce1b&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41181:
URL: https://github.com/apache/superset/pull/41181#issuecomment-4737081981
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41181?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.66667%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.73%. Comparing base ([`883b7a2`](https://app.codecov.io/gh/apache/superset/commit/883b7a286db420d64eb721b53ab02fbb98c2b85a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1f46360`](https://app.codecov.io/gh/apache/superset/commit/1f46360d75e0673b79b38b7a13508e3acbc60e3e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41181?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rset-frontend/src/explore/components/SaveModal.tsx](https://app.codecov.io/gh/apache/superset/pull/41181?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2FSaveModal.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | 91.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41181 +/- ##
=======================================
Coverage 63.73% 63.73%
=======================================
Files 2651 2651
Lines 144737 144749 +12
Branches 33402 33406 +4
=======================================
+ Hits 92242 92258 +16
+ Misses 50827 50823 -4
Partials 1668 1668
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41181/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/41181/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.44% <91.66%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41181?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3432466721
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
Review Comment:
**Suggestion:** Change the `update` callback parameter from `any` to a precise union/type that matches the `setState` updater contract used in this test. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is modified TypeScript code using `any` in a callback parameter, which matches the custom rule prohibiting `any` in new or changed TS/TSX code. The suggestion is therefore addressing a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f2cd9198af9943419ee49b436a6eb834&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f2cd9198af9943419ee49b436a6eb834&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 672:672
**Comment:**
*Custom Rule: Change the `update` callback parameter from `any` to a precise union/type that matches the `setState` updater contract used in this test.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=b31d4b13d9aa2cdcbca283717d8a07afe99c954d02de6169e36b865233f1e729&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=b31d4b13d9aa2cdcbca283717d8a07afe99c954d02de6169e36b865233f1e729&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
Review Comment:
**Suggestion:** Replace `any[]` with a specific state update collection type (for example, a typed partial state shape) so the test assertions stay type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file is TypeScript (`.tsx`), and this line uses `any[]`, which directly violates the rule against using `any` in modified TypeScript/TSX code. A concrete type or a narrower reusable type should be used instead.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f2830e2bf6c4612be334c963ff54142&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9f2830e2bf6c4612be334c963ff54142&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 671:671
**Comment:**
*Custom Rule: Replace `any[]` with a specific state update collection type (for example, a typed partial state shape) so the test assertions stay type-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=783934e5c0c6710d8042e175e93782cce86627d83506682c5c8b337cc18ddb08&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=783934e5c0c6710d8042e175e93782cce86627d83506682c5c8b337cc18ddb08&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41181:
URL: https://github.com/apache/superset/pull/41181#issuecomment-4741674817
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41181:
URL: https://github.com/apache/superset/pull/41181#issuecomment-4737040616
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3435596944
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -162,6 +167,35 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
t('An error occurred while loading dashboard information.'),
);
}
+ } else {
+ const metadataDashboards = this.props.metadata?.dashboards;
+ if (metadataDashboards?.length) {
+ // Fallback: the chart is already on one or more dashboards (from Explore API
+ // metadata). Pre-populate with the first dashboard the user can edit so the
+ // "Save & go to dashboard" button works out of the box.
+ try {
+ let editable: Dashboard | undefined;
+ for (const { id } of metadataDashboards) {
+ // eslint-disable-next-line no-await-in-loop
+ const result = await this.loadDashboard(id).catch(() => null);
+ if (result && canUserEditDashboard(result, this.props.user)) {
+ editable = result as Dashboard;
+ break;
+ }
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Sequential loading performance regression</b></div>
<div id="fix">
This refactor changes from parallel (Promise.all) to sequential (for...await) loading. The original loads all dashboards concurrently and finds the first editable one; the new code loads one-by-one and stops at the first editable. This is slower when an editable dashboard exists early (n-1 sequential delays) or when none exist (full sequential chain vs parallel). If intentional for resource conservation, add a comment documenting this trade-off.
</div>
</div>
<small><i>Code Review Run #c85537</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3434624703
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,172 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
+ stateUpdates.push(update);
+ });
+
+ try {
+ sessionStorage.clear();
+ } catch (_) {
+ // ignore
+ }
+
+ await component.componentDidMount();
+
+ expect(component.loadDashboard).toHaveBeenCalledWith(dashboardId);
+ expect(stateUpdates).toContainEqual({
+ dashboard: { label: dashboardTitle, value: dashboardId },
+ });
+ expect(component.loadTabs).toHaveBeenCalledWith(dashboardId);
+});
+
+test('skips non-editable dashboards and picks the first editable one from metadata', async () => {
+ const editableId = 7;
+ const editableTitle = 'Editable Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [
+ { id: 6, dashboard_title: 'Not Mine' },
+ { id: editableId, dashboard_title: editableTitle },
+ ],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+
+ const notMine = makeMetadataDashboard(6, 'Not Mine');
+ notMine.owners = [{ id: 99, first_name: 'Other', last_name: 'Owner' }];
+ const editable = makeMetadataDashboard(editableId, editableTitle);
+
+ component.loadDashboard = jest
+ .fn()
+ .mockImplementation((id: number) =>
+ Promise.resolve(id === 6 ? notMine : editable),
+ );
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
Review Comment:
**Suggestion:** Replace this `any[]` declaration with a reusable typed alias for state updates to keep test typing strict and consistent. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another occurrence of `any[]` in the added test code, so it clearly matches the custom rule to flag TypeScript usage of `any`.
The suggestion is directly applicable and accurate.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f7d34392e874484ad73a5466065ecef&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3f7d34392e874484ad73a5466065ecef&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 726:726
**Comment:**
*Custom Rule: Replace this `any[]` declaration with a reusable typed alias for state updates to keep test typing strict and consistent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=c6ed9565a68a30eff3c1a6614ef610ac693e44d9a7952e33b3928c75a7278cf5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=c6ed9565a68a30eff3c1a6614ef610ac693e44d9a7952e33b3928c75a7278cf5&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,172 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
+ stateUpdates.push(update);
+ });
+
+ try {
+ sessionStorage.clear();
+ } catch (_) {
+ // ignore
+ }
+
+ await component.componentDidMount();
+
+ expect(component.loadDashboard).toHaveBeenCalledWith(dashboardId);
+ expect(stateUpdates).toContainEqual({
+ dashboard: { label: dashboardTitle, value: dashboardId },
+ });
+ expect(component.loadTabs).toHaveBeenCalledWith(dashboardId);
+});
+
+test('skips non-editable dashboards and picks the first editable one from metadata', async () => {
+ const editableId = 7;
+ const editableTitle = 'Editable Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [
+ { id: 6, dashboard_title: 'Not Mine' },
+ { id: editableId, dashboard_title: editableTitle },
+ ],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+
+ const notMine = makeMetadataDashboard(6, 'Not Mine');
+ notMine.owners = [{ id: 99, first_name: 'Other', last_name: 'Owner' }];
+ const editable = makeMetadataDashboard(editableId, editableTitle);
+
+ component.loadDashboard = jest
+ .fn()
+ .mockImplementation((id: number) =>
+ Promise.resolve(id === 6 ? notMine : editable),
+ );
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
Review Comment:
**Suggestion:** Type the mocked `setState` callback argument with a specific updater type (or `Partial` state type) instead of `any`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The modified test code uses `any` in the mocked `setState` callback parameter, which is explicitly disallowed by the rule.
This is a genuine violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf09c90e66f94fc69262e91dbde996ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bf09c90e66f94fc69262e91dbde996ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 727:727
**Comment:**
*Custom Rule: Type the mocked `setState` callback argument with a specific updater type (or `Partial` state type) instead of `any`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=21fae5a9a503c42ccde96ffef93f77ae17f5dec5b08e8209c6f2ae43165cee4a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=21fae5a9a503c42ccde96ffef93f77ae17f5dec5b08e8209c6f2ae43165cee4a&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,172 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
+ stateUpdates.push(update);
+ });
+
+ try {
+ sessionStorage.clear();
+ } catch (_) {
+ // ignore
+ }
+
+ await component.componentDidMount();
+
+ expect(component.loadDashboard).toHaveBeenCalledWith(dashboardId);
+ expect(stateUpdates).toContainEqual({
+ dashboard: { label: dashboardTitle, value: dashboardId },
+ });
+ expect(component.loadTabs).toHaveBeenCalledWith(dashboardId);
+});
+
+test('skips non-editable dashboards and picks the first editable one from metadata', async () => {
+ const editableId = 7;
+ const editableTitle = 'Editable Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [
+ { id: 6, dashboard_title: 'Not Mine' },
+ { id: editableId, dashboard_title: editableTitle },
+ ],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+
+ const notMine = makeMetadataDashboard(6, 'Not Mine');
+ notMine.owners = [{ id: 99, first_name: 'Other', last_name: 'Owner' }];
+ const editable = makeMetadataDashboard(editableId, editableTitle);
+
+ component.loadDashboard = jest
+ .fn()
+ .mockImplementation((id: number) =>
+ Promise.resolve(id === 6 ? notMine : editable),
+ );
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
+ stateUpdates.push(update);
+ });
+
+ try {
+ sessionStorage.clear();
+ } catch (_) {
+ // ignore
+ }
+
+ await component.componentDidMount();
+
+ expect(stateUpdates).toContainEqual({
+ dashboard: { label: editableTitle, value: editableId },
+ });
+ expect(component.loadTabs).toHaveBeenCalledWith(editableId);
+});
+
+test('does not use metadata fallback when dashboardId prop is set', async () => {
+ const propDashboardId = 3;
+ const propDashboardTitle = 'Prop Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: propDashboardId,
+ metadata: {
+ dashboards: [{ id: 99, dashboard_title: 'Should Not Be Used' }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(propDashboardId, propDashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
Review Comment:
**Suggestion:** Replace this `any[]` usage with a concrete typed structure that reflects the expected dashboard selection state updates. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The final PR state includes this `any[]` declaration in newly added TypeScript test code, so it violates the no-`any` custom rule.
The suggestion correctly identifies the issue.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=222b94054b0c4d36b286a784edb724b1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=222b94054b0c4d36b286a784edb724b1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 770:770
**Comment:**
*Custom Rule: Replace this `any[]` usage with a concrete typed structure that reflects the expected dashboard selection state updates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=f9795199f18c7a451d3162ccf0b2d59fc8c0a64530062ed5bc675221276b20de&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=f9795199f18c7a451d3162ccf0b2d59fc8c0a64530062ed5bc675221276b20de&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): pre-populate SaveModal dashboard from chart metadata [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3434664487
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -162,6 +167,35 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
t('An error occurred while loading dashboard information.'),
);
}
+ } else {
+ const metadataDashboards = this.props.metadata?.dashboards;
+ if (metadataDashboards?.length) {
+ // Fallback: the chart is already on one or more dashboards (from Explore API
+ // metadata). Pre-populate with the first dashboard the user can edit so the
+ // "Save & go to dashboard" button works out of the box.
+ try {
+ let editable: Dashboard | undefined;
+ for (const { id } of metadataDashboards) {
+ // eslint-disable-next-line no-await-in-loop
+ const result = await this.loadDashboard(id).catch(() => null);
+ if (result && canUserEditDashboard(result, this.props.user)) {
+ editable = result as Dashboard;
+ break;
+ }
+ }
Review Comment:
**Suggestion:** The metadata fallback fetches dashboards sequentially using `await` inside a loop, which creates one network round-trip per dashboard and can make the modal noticeably slow for charts attached to many dashboards. This also contradicts the intended behavior to load metadata dashboards in parallel. Fetch all candidate dashboards concurrently (e.g., `Promise.all`) and then pick the first editable one by original metadata order. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Explore Save chart modal slow when chart on many dashboards.
- ⚠️ "Save & go to dashboard" button stays disabled unnecessarily.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an existing chart's Explore page at `/explore/?slice_id=<id>` without a
`dashboard_id` URL param so `dashboardId` is undefined in `hydrateExplore` and hence in
`ExploreViewContainer` (see `src/explore/actions/hydrateExplore.ts:16-21` and
`src/explore/components/ExploreViewContainer/index.tsx:1180-1186` where `dashboardId` is
derived from `explore.form_data.dashboardId`).
2. Ensure the chart belongs to many dashboards so the bootstrap payload includes a
populated `metadata.dashboards` array (shape defined in `src/explore/types.ts:4-20`,
especially `metadata.dashboards?: { id: number; dashboard_title: string }[]`), which
`hydrateExplore` stores on `explore.metadata` and `ExploreViewContainer` passes through as
`metadata` (mapStateToProps around
`src/explore/components/ExploreViewContainer/index.tsx:1188-1204`).
3. In the Explore toolbar, click the Save button so `ExploreChartHeader` dispatches
`setSaveChartModalVisibility(true)` via `useUnsavedChangesPrompt` (see
`src/explore/components/ExploreChartHeader/index.tsx:22-28` where `onSave` calls
`dispatch(setSaveChartModalVisibility(true))`). This sets `saveModal.isVisible` in Redux,
causing `ExploreViewContainer` to render `<SaveModal>` conditionally when
`props.isSaveModalVisible` is true
(`src/explore/components/ExploreViewContainer/index.tsx:1191-1199`).
4. When `<SaveModal>` mounts, its `componentDidMount` runs
(`src/explore/components/SaveModal.tsx:5-61` in the 140–219 block). Because there is no
`dashboardId` and no `SK_DASHBOARD_ID` in `sessionStorage`, execution follows the `else`
branch that reads `const metadataDashboards = this.props.metadata?.dashboards;` (lines
31-33 of that block). The fallback then iterates `for (const { id } of
metadataDashboards)` and performs `await this.loadDashboard(id).catch(() => null);` inside
the loop (`src/explore/components/SaveModal.tsx` PR hunk lines 178-185 / snippet lines
39-46), issuing one `/api/v1/dashboard/<id>` request at a time. With many dashboards,
modal initialization (pre-populating `this.state.dashboard` and loading tabs via
`loadTabs`) is delayed by roughly N×per-request-latency instead of the max of concurrent
latencies, making the Save modal noticeably slower whenever the chart is attached to many
dashboards.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83f5af6956ae4b7eb92f704833e580ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=83f5af6956ae4b7eb92f704833e580ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.tsx
**Line:** 178:185
**Comment:**
*Performance: The metadata fallback fetches dashboards sequentially using `await` inside a loop, which creates one network round-trip per dashboard and can make the modal noticeably slow for charts attached to many dashboards. This also contradicts the intended behavior to load metadata dashboards in parallel. Fetch all candidate dashboards concurrently (e.g., `Promise.all`) and then pick the first editable one by original metadata order.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=98ce30de55b432693e942369f7303882e9ea5d3107b43a445575e639c4239cd4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41181&comment_hash=98ce30de55b432693e942369f7303882e9ea5d3107b43a445575e639c4239cd4&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org