Skip to content

fix(dashboard): batch virtualized chart force-render for large-dashboard export - #42786

Open
rusackas wants to merge 2 commits into
upstream-fix-29719-force-render-exportfrom
fix/29719-force-render-batching
Open

fix(dashboard): batch virtualized chart force-render for large-dashboard export#42786
rusackas wants to merge 2 commits into
upstream-fix-29719-force-render-exportfrom
fix/29719-force-render-batching

Conversation

@rusackas

@rusackas rusackas commented Aug 5, 2026

Copy link
Copy Markdown
Member

SUMMARY

Follow-up to #42561, stacked on top of it (base branch is upstream-fix-29719-force-render-export, not master, so this diff only shows what's new here).

#42561 fixes the client-side Download as Image/PDF path exporting virtualized dashboard rows as loading spinners, by forcing every row into view before capture. The fix is correct, but it forces every row into view in a single 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 in the first place, just moved from page-load time to export time, with no concurrency limit and no signal to the user that anything expensive is about to happen.

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 10s per-batch timeout) for each batch before moving to the next. The per-batch wait is scoped to just that batch's own row elements, not the whole container, so a chart stuck in one batch doesn't force every later batch to also burn its full timeout re-checking that same stale spinner (caught this via a failing test, not by inspection, see commit for details). Dashboards small enough to fit in one batch keep the original single-event behavior unchanged. The existing whole-container 60s check after all batches still runs as a final safety net.
  • An upfront info toast ("Preparing N charts for export. This may take a moment.") when a multi-batch export starts, so the user gets a signal that a large 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; forceLoadAllCharts now accepts an optional onProgress callback as an extensibility hook if that's wanted later.
  • Test coverage for Row.tsx's event handling. fix(dashboard): force-render virtualized charts before client-side export #42561 added the force-in-view/restore-virtualization listeners to Row.tsx but never tested them directly, only the dispatching side in downloadUtils.test.ts. This adds coverage for the actual consumer: scoped batch targeting (responds only when its own row id is in the event, or when no rowIds filter is present at all) and observer re-attachment on restore.
  • Test coverage for the batching logic: grouping into batches of 5, progress callback sequencing, per-batch timeout isolation (the bug mentioned above), and confirming the existing single-pass/flag-off paths from fix(dashboard): force-render virtualized charts before client-side export #42561 are unchanged.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — this changes the pacing/signaling of an existing export flow, not its visual output.

TESTING INSTRUCTIONS

npx jest src/utils/downloadUtils.test.ts src/dashboard/components/gridComponents/Row/Row.test.tsx

Manually: with DASHBOARD_VIRTUALIZATION enabled, open a dashboard with more than 5 charts in one tab (enough to span multiple batches), and use Download → Export to Image/PDF. You should see a "Preparing N charts for export" toast, and the export should still contain every chart rendered (not loading spinners), same end result as #42561, just staggered.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

…ard export

#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>
@dosubot dosubot Bot added change:frontend Requires changing the frontend dashboard:export Related to exporting dashboards labels Aug 5, 2026
@bito-code-review

bito-code-review Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped - Branch Excluded

Bito didn't auto-review because the source or target branch is excluded from automatic reviews.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change the branch exclusion settings here, or contact your Bito workspace admin at evan@preset.io.

@netlify

netlify Bot commented Aug 5, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 0c02353
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a72c0d4fc0e950008cafeb5
😎 Deploy Preview https://deploy-preview-42786--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment thread superset-frontend/src/utils/downloadUtils.ts Outdated
@bito-code-review

Copy link
Copy Markdown
Contributor

The flagged issue is correct. The current implementation of forceLoadAllCharts in superset-frontend/src/utils/downloadUtils.ts already addresses this by introducing batching and a per-batch timeout (BATCH_LOAD_TIMEOUT_MS), followed by a final waitForChartsToLoad call that acts as the overall safety net. The implementation ensures that a slow chart in one batch does not block subsequent batches, and the final check ensures that any stragglers are accounted for before the export proceeds.

superset-frontend/src/utils/downloadUtils.ts

// eslint-disable-next-line no-await-in-loop -- see above
        await waitForRowsToLoad(batch, BATCH_LOAD_TIMEOUT_MS);
        onProgress?.({
          loadedBatches: index + 1,
          totalBatches: rowBatches.length,
        });
      }
    }
    const allLoaded = await waitForChartsToLoad(container);

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.28%. Comparing base (9b31f3b) to head (8569629).

Additional details and impacted files
@@                           Coverage Diff                           @@
##           upstream-fix-29719-force-render-export   #42786   +/-   ##
=======================================================================
  Coverage                                   65.27%   65.28%           
=======================================================================
  Files                                        2798     2798           
  Lines                                      158048   158088   +40     
  Branches                                    36114    36120    +6     
=======================================================================
+ Hits                                       103169   103205   +36     
- Misses                                      52884    52888    +4     
  Partials                                     1995     1995           
Flag Coverage Δ
javascript 71.41% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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>
@rusackas rusackas added the 🎪 ⚡ showtime-trigger Trigger showtime deployment label Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend dashboard:export Related to exporting dashboards preset-io 🎪 ⚡ showtime-trigger Trigger showtime deployment size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants