Repository navigation
fix(dashboard): force-render virtualized charts before client-side export - #42561
Conversation
Code Review Agent Run #766331Actionable 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 |
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| export function restoreVirtualization(): void { | ||
| window.dispatchEvent(new Event(RESTORE_VIRTUALIZATION_EVENT)); | ||
| } |
There was a problem hiding this comment.
Suggestion: The restore event is global and has no ownership or nesting protection. If two exports overlap, the first export to finish dispatches RESTORE_VIRTUALIZATION_EVENT while the second is still capturing, causing rows to reconnect their observers and potentially unmount off-screen charts during the second export. Coordinate exports with a shared lock or reference count so virtualization is restored only after the final active export completes. [race condition]
Severity Level: Major ⚠️
- ❌ Concurrent exports can capture loading spinners.
- ❌ Dashboard or chart export output may be incomplete.
- ⚠️ Global row state is restored before all captures finish.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/downloadUtils.ts
**Line:** 82:84
**Comment:**
*Race Condition: The restore event is global and has no ownership or nesting protection. If two exports overlap, the first export to finish dispatches `RESTORE_VIRTUALIZATION_EVENT` while the second is still capturing, causing rows to reconnect their observers and potentially unmount off-screen charts during the second export. Coordinate exports with a shared lock or reference count so virtualization is restored only after the final active export completes.
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.
REAL, but genuinely MINOR
Confirmed reachable: restoreVirtualization() fires a global event with no ref-count, and the menu handlers (DownloadMenuItems/index.tsx:93-111) fire the exports without an isExporting guard. If you deliberately re-trigger a second export during the
first's multi-second capture, RESTORE(A) can revert far-off rows to spinners mid-capture of export B.
But it's bounded: requires DASHBOARD_VIRTUALIZATION + a tall (>4-viewport) dashboard + a deliberate second export in the capture window; only the second export degrades, the user gets an explicit "export may be incomplete" warning, and it self-heals
(every exit path restores). Not a merge blocker.
that disables/early-returns the Export menu items while an export runs.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
For export concurrency, prevent overlapping exports with an in-flight guard that disables or early-returns duplicate export actions; do not use a shared virtualization reference count that could remain stuck after a missed decrement.
Applied to:
superset-frontend/src/utils/downloadUtils.ts
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
The flagged race condition is correct. Because To resolve this, you can implement a simple reference counter in Proposed FixUpdate let activeExportCount = 0;
export async function forceLoadAllCharts(container: Element): Promise<boolean> {
const useVirtualization = isFeatureEnabled(FeatureFlag.DashboardVirtualization);
if (useVirtualization) {
if (activeExportCount === 0) {
window.dispatchEvent(new Event(FORCE_IN_VIEW_EVENT));
}
activeExportCount++;
const allLoaded = await waitForChartsToLoad(container);
// ... (rest of logic)
}
return useVirtualization;
}
export function restoreVirtualization(): void {
activeExportCount = Math.max(0, activeExportCount - 1);
if (activeExportCount === 0) {
window.dispatchEvent(new Event(RESTORE_VIRTUALIZATION_EVENT));
}
}Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/src/utils/downloadUtils.ts |
| const handleForceInView = () => { | ||
| observerEnabler?.disconnect(); | ||
| observerDisabler?.disconnect(); | ||
| setIsInView(true); | ||
| }; |
There was a problem hiding this comment.
Suggestion: The force handler unconditionally sets isInView to true without checking isComponentVisibleRef.current. For rows belonging to hidden tabs or otherwise invisible dashboard components, this bypasses the visibility guard used by the normal observer callbacks; Chart then treats the row as renderable and can trigger queries for charts that should remain inactive during export. Preserve the visibility check when forcing rows into view, or explicitly exclude invisible components from the force-render path. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Inactive dashboard tabs may issue chart queries during export.
- ⚠️ Exports can trigger unnecessary hidden-chart rendering.
- ⚠️ Large tabbed dashboards may incur extra load.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx
**Line:** 219:223
**Comment:**
*Incorrect Condition Logic: The force handler unconditionally sets `isInView` to `true` without checking `isComponentVisibleRef.current`. For rows belonging to hidden tabs or otherwise invisible dashboard components, this bypasses the visibility guard used by the normal observer callbacks; `Chart` then treats the row as renderable and can trigger queries for charts that should remain inactive during export. Preserve the visibility check when forcing rows into view, or explicitly exclude invisible components from the force-render path.
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.
- The dashboard grid Chart is wrapped in React.memo with a comparator (gridComponents/Chart/Chart.tsx:838-839) that short-circuits whenever !isComponentVisible. A hidden-tab row flipping isInView is swallowed at the memo boundary — the inner chart
never re-renders, shouldRenderChart/runQuery are never re-evaluated, no mount, no query. - Inactive antd tab panes are display:none/unmounted and aren't in the .dashboard capture anyway.
Investigator + both skeptics unanimous: not real, no fix needed. Adding the guard would be a behavioral no-op.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42561 +/- ##
==========================================
+ Coverage 66.41% 66.42% +0.01%
==========================================
Files 2858 2859 +1
Lines 161446 161540 +94
Branches 37190 37206 +16
==========================================
+ Hits 107222 107308 +86
- Misses 52187 52195 +8
Partials 2037 2037
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:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @jenwitteng, checked the CodeAnt hidden-tab thread myself: Chart's memo comparator returns true whenever !isComponentVisible regardless of isInView (Chart.tsx around 804-823), so a hidden-tab row flipping isInView never reaches shouldRenderChart. Confirms your read that it's a false positive.
Agree on the restore-event race too, bounded enough not to block.
Wouldn't mind a quick test for Row's force/restore handlers before this merges, if you have time (or don't mind me adding one) but not blocking either. LGTM!
|
This is a solid improvement over what folks are doing today (globally disabling
Also, I'll open a follow-up PR for both, plus maybe some way to signal a large export is in progress. Thanks for chasing this down, @jenwitteng! |
|
Opened #42786 for this, batches the force-render in groups of 5 instead of all at once, plus the missing |
|
ok Thanks @rusackas |
|
@jenwitteng do you want to merge that PR into this one, then merge them together? Curious your thoughts on the approach of it. |
yes merge them together is better i will do it. |
…ard export apache#42561 fixes the client-side Download as Image/PDF path exporting virtualized rows as loading spinners, but forces every row into view in one instant `window` event with no batching. On a dashboard with hundreds of charts, that reintroduces the exact thundering-herd load DASHBOARD_VIRTUALIZATION exists to prevent, just moved from page-load time to export time, with no concurrency limit and no signal to the user that anything expensive is happening. This adds: - Row-level batching. Rows now carry a `data-row-id` attribute; the export path force-renders them in groups of 5 rather than all at once, waiting (with a bounded per-batch timeout) for each batch before moving to the next. A chart stuck in one batch doesn't stall every later batch, since the per-batch wait is scoped to that batch's own row elements rather than the whole container. Dashboards small enough to fit in one batch keep the original single-event behavior unchanged. - An upfront info toast ("Preparing N charts for export...") when a multi-batch export starts, so the user knows a large dashboard export is underway rather than wondering if the click did nothing. A live per-batch progress bar felt like a bigger UI commitment than this follow-up warranted; `onProgress` is exposed as an extensibility hook if that's wanted later. - Test coverage for Row.tsx's force-in-view/restore-virtualization event handling, which had none before this PR despite being the component that does the actual work. - Test coverage for the batching logic itself: grouping, per-batch timeout isolation, and the existing single-pass/flag-off paths (confirmed unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #21420bActionable 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 |
|
Once CI is passing here, I think we'll be good to go (though I'll give it a final pass). Holler if you want help with that. |
…port When DASHBOARD_VIRTUALIZATION is enabled, dashboard rows more than a viewport away are unmounted and replaced with a loading spinner. The client-side "Download as Image/PDF" path captures the live DOM, so those off-screen charts are exported as loading spinners instead of the actual charts. The isCurrentUserBot() bypass only covers server-side (headless) screenshots, so this affects every real-user client-side export. Add a small force-render contract used only during export: - downloadUtils.ts: forceLoadAllCharts() dispatches a superset-force-all-in-view window event (gated on the feature flag), polls until the container's .loading spinners clear (with a timeout and a warning toast), and returns whether virtualization was active. restoreVirtualization() dispatches superset-restore-virtualization. - Row.tsx: on force-in-view, disconnect the IntersectionObservers and render; on restore, re-observe. - downloadAsPdf / downloadAsImage: force-load before capture and restore on every exit path (including the ag-grid "still loading" early return and error paths) so a failed export never leaves virtualization disabled for the rest of the session. Fixes: apache#29719 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Jenwit Amonpongitsara <jenwit.amonpongitsara@agoda.com>
…ard export apache#42561 fixes the client-side Download as Image/PDF path exporting virtualized rows as loading spinners, but forces every row into view in one instant `window` event with no batching. On a dashboard with hundreds of charts, that reintroduces the exact thundering-herd load DASHBOARD_VIRTUALIZATION exists to prevent, just moved from page-load time to export time, with no concurrency limit and no signal to the user that anything expensive is happening. This adds: - Row-level batching. Rows now carry a `data-row-id` attribute; the export path force-renders them in groups of 5 rather than all at once, waiting (with a bounded per-batch timeout) for each batch before moving to the next. A chart stuck in one batch doesn't stall every later batch, since the per-batch wait is scoped to that batch's own row elements rather than the whole container. Dashboards small enough to fit in one batch keep the original single-event behavior unchanged. - An upfront info toast ("Preparing N charts for export...") when a multi-batch export starts, so the user knows a large dashboard export is underway rather than wondering if the click did nothing. A live per-batch progress bar felt like a bigger UI commitment than this follow-up warranted; `onProgress` is exposed as an extensibility hook if that's wanted later. - Test coverage for Row.tsx's force-in-view/restore-virtualization event handling, which had none before this PR despite being the component that does the actual work. - Test coverage for the batching logic itself: grouping, per-batch timeout isolation, and the existing single-pass/flag-off paths (confirmed unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bound the combined per-batch and final whole-container waits to a single overall budget so a dashboard with many stalled batches can't burn a full per-batch timeout on each one plus another full timeout on the final check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8569629 to
47e9bad
Compare
SUMMARY
When
DASHBOARD_VIRTUALIZATIONis enabled, dashboard rows more than a viewport away are unmounted and replaced with a<Loading>spinner (Row.tsx→Chart.tsxshouldRenderChart). The client-side Download as Image / Download as PDF path captures the live DOM, so those off-screen charts are exported as loading spinners instead of the actual charts.The existing
isCurrentUserBot()(window.navigator.webdriver) bypass only disables virtualization for server-side headless capture (scheduled reports, thumbnails, the webdriver screenshot endpoints). A real user's browser haswebdriver === false, so the client-sidedownloadAsImage('.dashboard')/downloadAsPdf('.dashboard')handlers — used by default whenENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS/ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOTare off — capture unmounted rows.This adds a small "force a full render before capturing" contract used only during export:
src/utils/downloadUtils.ts(new):forceLoadAllCharts()dispatches asuperset-force-all-in-viewwindow event (gated onDASHBOARD_VIRTUALIZATION), polls until the container's.loadingspinners clear (60s timeout + a warning toast if it times out), and returns whether virtualization was active.restoreVirtualization()dispatchessuperset-restore-virtualization.Row.tsx: on force-in-view, disconnect theIntersectionObservers and render; on restore, re-observe. No change to normal (non-export) virtualization behavior.downloadAsPdf/downloadAsImage: force-load before capture and restore on every exit path — including the ag-grid "still loading" early return and the error paths — so a failed export never leaves virtualization disabled for the rest of the session.Behavior is unchanged when
DASHBOARD_VIRTUALIZATIONis off, and unchanged for server-side/headless capture.Context and maintainer discussion: #29719 (comment)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Exporting a tall dashboard (more than ~1 viewport of charts) as Image/PDF renders the on-screen charts followed by loading spinners for every off-screen chart.
After: All charts render in the exported image/PDF regardless of scroll position.
TESTING INSTRUCTIONS
DASHBOARD_VIRTUALIZATIONis enabled (default on).DASHBOARD_VIRTUALIZATIONdisabled — behavior should be unchanged.Unit tests:
src/utils/downloadUtils.test.ts(force/restore event dispatch, timeout warning, flag-off no-op). ExistingdownloadAsImage.test.tsandRow.test.tsxsuites pass unchanged.ADDITIONAL INFORMATION