fix: chart rendering race condition and homepage connection reset#40065
Conversation
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| promise = Promise.all( | ||
| keys.map(key => Promise.resolve(options.loader[key]())), | ||
| ).then( |
There was a problem hiding this comment.
Suggestion: Synchronous exceptions thrown by a loader are not converted into a rejected promise, so load() can throw during render effect setup and bypass the component's error state handling path. Wrap each loader invocation in a try/catch (or use a promise constructor) so sync throws are normalized to rejected promises and routed through setState({ error }). [logic error]
Severity Level: Major ⚠️
- ❌ Chart renderer can crash on sync loader exceptions.
- ⚠️ onRenderFailure callback never fires for sync failures.Steps of Reproduction ✅
1. In
`superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts`
lines 54–74, the `load()` helper constructs its shared `promise` as
`Promise.all(keys.map(key => Promise.resolve(options.loader[key]())))` (lines 57–59)
without wrapping the `options.loader[key]()` call in a try/catch or a deferred `then`.
2. A consumer creates a renderer via `createLoadableRenderer` (same file, lines 47–49) and
supplies a `loader` map where at least one loader function throws synchronously before
returning (for example, a misconfigured loader that immediately does `throw new Error("bad
config")`).
3. When the `Renderer` component mounts, its effect at lines 82–96 calls
`load().then(...)`; because the loader throws synchronously during evaluation of
`options.loader[key]()`, the exception is thrown before `Promise.all(...)` is created, so
`load()` itself throws instead of returning a promise.
4. React executes the effect function and sees this uncaught exception, causing the render
tree to error/crash; the component state `{ loaded, error }` is never updated, and the
error-handling path `options.loading({ error })` / `onRenderFailure` (lines 105–111) is
never invoked because `setState({ loaded: null, error })` is only reached when `load()`
returns a rejecting promise.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts
**Line:** 57:59
**Comment:**
*Logic Error: Synchronous exceptions thrown by a loader are not converted into a rejected promise, so `load()` can throw during render effect setup and bypass the component's error state handling path. Wrap each loader invocation in a try/catch (or use a promise constructor) so sync throws are normalized to rejected promises and routed through `setState({ error })`.
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
| if (key != null) { | ||
| const currentController = | ||
| getState().charts?.[key]?.queryController; | ||
| if (currentController && currentController !== controller) { | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The stale-response guard only drops when currentController is truthy, so it does not drop responses after the chart entry has been removed (or the controller was cleared). In that case, this stale success still dispatches and can target a non-existent chart key, which can crash the chart reducer when it tries to spread an undefined chart state. Treat any controller mismatch (including missing current controller) as stale and return early. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ Removing a loading chart can crash chartReducer.
- ❌ Dashboard render can fail due to reducer exception.Steps of Reproduction ✅
1. Load a dashboard so that charts are hydrated into Redux `charts` state via
`chartReducer` (see `chartReducer.ts:49-52` where `chartReducer` manages the `charts`
map).
2. Trigger a chart query for some slice id `id` (e.g. via `postChartFormData()` which
calls `exploreJSON` in `chartAction.ts:330-378`), causing `exploreJSON` to dispatch
`CHART_UPDATE_STARTED` and store an `AbortController` in
`state.charts[id].queryController` (see `chartAction.ts:308-320` and
`chartReducer.ts:69-78`).
3. While that request is still in flight, remove the same chart from the dashboard, which
calls `removeChart(id)` from `dashboardState.ts:911-917` (imported from `chartAction.ts`
as shown in `dashboardState.ts:31-37`), and `chartReducer` handles `REMOVE_CHART` by
omitting `charts[id]` entirely (`chartReducer.ts:190-193`), leaving no entry for that key.
4. When the original async request for that chart later resolves successfully, the `.then`
handler in `exploreJSON` at `chartAction.ts:379-420` runs and executes the stale-response
guard at `chartAction.ts:388-393`:
```ts
if (key != null) {
const currentController = getState().charts?.[key]?.queryController;
if (currentController && currentController !== controller) {
return undefined;
}
}
Since charts[id] was deleted in step 3, currentController is undefined, the if (currentController && ...) condition is false, and the guard does not drop the stale
response.
- The same handler then dispatches
CHART_UPDATE_SUCCEEDEDwithkey = id
(chartAction.ts:419-420), andchartReducerprocesses it by calling the handler with
charts[action.key](chartReducer.ts:225-229); becausecharts[id]is nowundefined,
the handler atchartReducer.ts:60-67executesreturn { ...state, ... }withstate === undefined, which throws a runtime error (TypeError: Cannot convert undefined or null to object) and can crash chart/dashboards rendering.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FchartAction.ts%0A%2A%2ALine%3A%2A%2A%20787%3A793%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20The%20stale-response%20guard%20only%20drops%20when%20%60currentController%60%20is%20truthy%2C%20so%20it%20does%20not%20drop%20responses%20after%20the%20chart%20entry%20has%20been%20removed%20%28or%20the%20controller%20was%20cleared%29.%20In%20that%20case%2C%20this%20stale%20success%20still%20dispatches%20and%20can%20target%20a%20non-existent%20chart%20key%2C%20which%20can%20crash%20the%20chart%20reducer%20when%20it%20tries%20to%20spread%20an%20undefined%20chart%20state.%20Treat%20any%20controller%20mismatch%20%28including%20missing%20current%20controller%29%20as%20stale%20and%20return%20early.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FchartAction.ts%0A%2A%2ALine%3A%2A%2A%20787%3A793%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20The%20stale-response%20guard%20only%20drops%20when%20%60currentController%60%20is%20truthy%2C%20so%20it%20does%20not%20drop%20responses%20after%20the%20chart%20entry%20has%20been%20removed%20%28or%20the%20controller%20was%20cleared%29.%20In%20that%20case%2C%20this%20stale%20success%20still%20dispatches%20and%20can%20target%20a%20non-existent%20chart%20key%2C%20which%20can%20crash%20the%20chart%20reducer%20when%20it%20tries%20to%20spread%20an%20undefined%20chart%20state.%20Treat%20any%20controller%20mismatch%20%28including%20missing%20current%20controller%29%20as%20stale%20and%20return%20early.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/components/Chart/chartAction.ts
**Line:** 787:793
**Comment:**
*Incorrect Condition Logic: The stale-response guard only drops when `currentController` is truthy, so it does not drop responses after the chart entry has been removed (or the controller was cleared). In that case, this stale success still dispatches and can target a non-existent chart key, which can crash the chart reducer when it tries to spread an undefined chart state. Treat any controller mismatch (including missing current controller) as stale and return early.
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
| if (key != null) { | ||
| const currentController = | ||
| getState().charts?.[key]?.queryController; | ||
| if (currentController && currentController !== controller) { | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The stale-error guard has the same truthy check bug as the success path: if the chart state was removed or its controller is cleared, stale failures are not dropped and still dispatch CHART_UPDATE_FAILED for an invalid chart key. This can propagate a stale failure into reducer logic that expects an existing chart state. The check should also reject when currentController is absent. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ Late failing requests for removed charts can crash reducer.
- ⚠️ Dashboard may mis-handle errors after chart deletion.Steps of Reproduction ✅
1. Load a dashboard so that the `charts` reducer is active for each chart
(`chartReducer.ts:49-52`) and start a chart query for slice id `id` via
`postChartFormData` → `exploreJSON` (`chartAction.ts:330-378`), which records an
`AbortController` in `state.charts[id].queryController` (`chartReducer.ts:69-78`).
2. Remove that chart from the dashboard while the request is still in flight;
`dashboardState.ts:911-917` dispatches `removeChart(id)` (imported from `chartAction.ts`
in `dashboardState.ts:31-37`), and `chartReducer` handles `REMOVE_CHART` by deleting
`charts[id]` (`chartReducer.ts:190-193`).
3. The original request for that chart subsequently fails (e.g. server error, timeout, or
any non-abort error), causing the `.catch` handler in `exploreJSON` at
`chartAction.ts:819-874` to run; it first filters abort errors, then executes the
stale-error guard at `chartAction.ts:821-828`:
```ts
if (key != null) {
const currentController = getState().charts?.[key]?.queryController;
if (currentController && currentController !== controller) {
return undefined;
}
}
Because charts[id] was removed in step 2, currentController is undefined, the if (currentController && ...) condition is false, and the stale failing response is not
dropped.
-
For a non-abort failure, the catch block dispatches
CHART_UPDATE_FAILEDwithkey = id(seechartAction.ts:100-107andchartAction.ts:128-140), which thechartReducer
processes via its generic handler table atchartReducer.ts:225-229using
charts[action.key]asstate. -
Since
charts[id]was deleted and is nowundefined, theCHART_UPDATE_FAILED
handler atchartReducer.ts:110-122attemptsreturn { ...state, ... }withstate === undefined, causing aTypeErrorand potentially breaking dashboard error handling and
chart rendering when late failing responses arrive for charts that have already been
removed.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FchartAction.ts%0A%2A%2ALine%3A%2A%2A%20841%3A847%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20The%20stale-error%20guard%20has%20the%20same%20truthy%20check%20bug%20as%20the%20success%20path%3A%20if%20the%20chart%20state%20was%20removed%20or%20its%20controller%20is%20cleared%2C%20stale%20failures%20are%20not%20dropped%20and%20still%20dispatch%20%60CHART_UPDATE_FAILED%60%20for%20an%20invalid%20chart%20key.%20This%20can%20propagate%20a%20stale%20failure%20into%20reducer%20logic%20that%20expects%20an%20existing%20chart%20state.%20The%20check%20should%20also%20reject%20when%20%60currentController%60%20is%20absent.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FchartAction.ts%0A%2A%2ALine%3A%2A%2A%20841%3A847%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20The%20stale-error%20guard%20has%20the%20same%20truthy%20check%20bug%20as%20the%20success%20path%3A%20if%20the%20chart%20state%20was%20removed%20or%20its%20controller%20is%20cleared%2C%20stale%20failures%20are%20not%20dropped%20and%20still%20dispatch%20%60CHART_UPDATE_FAILED%60%20for%20an%20invalid%20chart%20key.%20This%20can%20propagate%20a%20stale%20failure%20into%20reducer%20logic%20that%20expects%20an%20existing%20chart%20state.%20The%20check%20should%20also%20reject%20when%20%60currentController%60%20is%20absent.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/components/Chart/chartAction.ts
**Line:** 841:847
**Comment:**
*Incorrect Condition Logic: The stale-error guard has the same truthy check bug as the success path: if the chart state was removed or its controller is cleared, stale failures are not dropped and still dispatch `CHART_UPDATE_FAILED` for an invalid chart key. This can propagate a stale failure into reducer logic that expects an existing chart state. The check should also reject when `currentController` is absent.
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
|
Yes, the suggestion is valid — synchronous exceptions from loaders would cause the load function to throw instead of returning a rejected promise, bypassing the component's error state handling. To fix this, wrap each loader call in a try/catch within a Promise constructor to normalize sync throws to rejections. Here's the concise fix for lines 73-74 in createLoadableRenderer.ts: promise = Promise.all(
keys.map(key => new Promise((resolve, reject) => {
try {
resolve(options.loader[key]());
} catch (e) {
reject(e);
}
})),There are no other comments on this PR. superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40065 +/- ##
==========================================
- Coverage 64.13% 64.10% -0.04%
==========================================
Files 2590 2590
Lines 138037 138000 -37
Branches 32021 32021
==========================================
- Hits 88534 88460 -74
- Misses 47984 48022 +38
+ Partials 1519 1518 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 #f51e9c
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts - 1
- useEffect deps cause re-runs · Line 82-96
Additional Suggestions - 1
-
superset-frontend/src/components/Chart/chartAction.ts - 1
-
Duplicate stale controller check logic · Line 787-792The logic to check if the query controller is stale is duplicated between the success and error handling paths (lines 787-792 and 841-846). This creates maintenance risk if the logic needs to change. Consider extracting this into a helper function to eliminate duplication and improve code maintainability.
-
Review Details
-
Files reviewed - 6 · Commit Range:
48461f0..806fad9- superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts
- superset-frontend/src/components/Chart/chartAction.ts
- superset-frontend/src/components/Chart/chartActions.test.ts
- superset-frontend/src/embedded/index.tsx
- superset-frontend/src/views/index.tsx
- superset-frontend/src/views/menu.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ 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
| useEffect(() => { | ||
| if (state.loaded || state.error) return undefined; | ||
| let cancelled = false; | ||
| load().then( | ||
| loaded => { | ||
| if (!cancelled) setState({ loaded, error: null }); | ||
| }, | ||
| err => { | ||
| if (!cancelled) setState({ loaded: null, error: err }); | ||
| }, | ||
| ); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [state.loaded, state.error]); |
There was a problem hiding this comment.
The useEffect hook has dependencies on state values it updates, causing unnecessary re-executions after the initial load completes. This leads to performance overhead and potential issues. Use an empty dependency array with a ref to track loading state instead.
Code suggestion
Check the AI-generated fix before applying
- }>(() => ({ loaded: cachedResult, error: cachedError }));
-
- useEffect(() => {
- if (state.loaded || state.error) return undefined;
- let cancelled = false;
- load().then(
- loaded => {
- if (!cancelled) setState({ loaded, error: null });
- },
- err => {
- if (!cancelled) setState({ loaded: null, error: err });
- },
- );
- return () => {
- cancelled = true;
- };
- }, [state.loaded, state.error]);
+ }>(() => ({ loaded: cachedResult, error: cachedError }));
+
+ const loadingStartedRef = useRef(false);
+
+ useEffect(() => {
+ if (loadingStartedRef.current) return;
+ loadingStartedRef.current = true;
+ let cancelled = false;
+ load().then(
- loaded => {
- if (!cancelled) setState({ loaded, error: null });
- },
- err => {
- if (!cancelled) setState({ loaded: null, error: err });
- },
- );
- return () => {
- cancelled = true;
- };
- }, []);
Code Review Run #f51e9c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #3dbf40Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
|
Thanks for fixing this! |
|
Bito Automatic Review Skipped – PR Already Merged |
4 participants
SUMMARY
#35998 introduced a race condition that was amplified by #39893 and resulted in charts not rendering properly most of the times with blank pages instead. This pr does the following:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION