fix(explore): pre-populate SaveModal dashboard from chart metadata#41181
fix(explore): pre-populate SaveModal dashboard from chart metadata#41181yousoph wants to merge 3 commits into
Conversation
When a user opens the save modal for a chart that is already on one or more dashboards, the Dashboard field was blank unless the user had arrived from that dashboard (URL param) or had saved there before (sessionStorage). The Explore API already returns metadata.dashboards listing every dashboard the chart belongs to, but SaveModal never consulted it. The fix adds a fallback in componentDidMount: if no dashboardId is found from the URL prop or sessionStorage, iterate metadata.dashboards, load each one, and pre-select the first dashboard the user can edit. This makes "Save & go to dashboard" work correctly when navigating directly to a chart's Explore page. Also wires explore.metadata into SaveModal via mapStateToProps so the component does not need any new parent-level prop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
dashboard
Code Review Agent Run #a51ef2Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| component.loadDashboard = jest.fn().mockResolvedValue(mockFull); | ||
| component.loadTabs = jest.fn().mockResolvedValue([]); | ||
|
|
||
| const stateUpdates: any[] = []; |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this 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.
| component.loadTabs = jest.fn().mockResolvedValue([]); | ||
|
|
||
| const stateUpdates: any[] = []; | ||
| component.setState = jest.fn((update: any) => { |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Declining — same reason as the adjacent comment: line 602 uses the identical any pattern for setState mocking. Consistent with existing style.
| const loaded = await Promise.all( | ||
| metadataDashboards.map(({ id }) => | ||
| this.loadDashboard(id).catch(() => null), | ||
| ), | ||
| ); |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Explore SaveModal fires many /api/v1/dashboard requests.
- ⚠️ Backend load grows with dashboards per chart.
- ⚠️ Save modal initialization slower for multi-dashboard charts.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41181 +/- ##
=======================================
Coverage 63.73% 63.73%
=======================================
Files 2651 2651
Lines 144737 144750 +13
Branches 33402 33406 +4
=======================================
+ Hits 92242 92259 +17
+ Misses 50827 50823 -4
Partials 1668 1668
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…in SaveModal Promise.all fired N concurrent dashboard API requests even though only the first editable dashboard is needed. Switch to a sequential for-of loop that breaks as soon as it finds an editable dashboard, bounding requests to the minimum needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| ); | ||
| component.loadTabs = jest.fn().mockResolvedValue([]); | ||
|
|
||
| const stateUpdates: any[] = []; |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| component.loadTabs = jest.fn().mockResolvedValue([]); | ||
|
|
||
| const stateUpdates: any[] = []; | ||
| component.setState = jest.fn((update: any) => { |
There was a problem hiding this 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
Why it matters? 🤔
The modified test code uses any in the mocked setState callback parameter, which is explicitly disallowed by the rule.
This is a genuine violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| component.loadDashboard = jest.fn().mockResolvedValue(mockFull); | ||
| component.loadTabs = jest.fn().mockResolvedValue([]); | ||
|
|
||
| const stateUpdates: any[] = []; |
There was a problem hiding this comment.
Suggestion: Replace this any[] usage with a concrete typed structure that reflects the expected dashboard selection state updates. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
| 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; | ||
| } | ||
| } |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Explore Save chart modal slow when chart on many dashboards.
- ⚠️ "Save & go to dashboard" button stays disabled unnecessarily.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #c85537
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/SaveModal.tsx - 1
- Sequential loading performance regression · Line 177-185
Additional Suggestions - 1
-
superset-frontend/src/explore/components/SaveModal.tsx - 1
-
Suppressed lint rule for await-in-loop · Line 179-179The eslint-disable comment acknowledges the no-await-in-loop anti-pattern. This is a known issue being suppressed rather than addressed. If the sequential loading behavior is intentional, consider adding context to explain why this anti-pattern is acceptable here.
-
Review Details
-
Files reviewed - 2 · Commit Range:
1f46360..8b9ed69- superset-frontend/src/explore/components/SaveModal.test.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
Code Review Run #c85537
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
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.componentDidMountonly tried to pre-populate the Dashboard dropdown from two sources — thedashboardIdURL param (only present when navigating from a dashboard view) orsessionStorage. But the/api/v1/explore/response already includesmetadata.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
metadatatomapStateToPropsand add a fallback incomponentDidMount: when nodashboardIdis found from URL/sessionStorage, load each dashboard frommetadata.dashboardsin 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
/explore/?slice_id=<id>directly (nodashboard_idURL param).Unit tests added:
pre-populates dashboard from metadata.dashboards when dashboardId prop is absentskips non-editable dashboards and picks the first editable one from metadatadoes not use metadata fallback when dashboardId prop is setADDITIONAL INFORMATION