fix(matrixify): lazy-load grid cells with dashboard virtualization - #43053
fix(matrixify): lazy-load grid cells with dashboard virtualization#43053jenwitteng wants to merge 2 commits into
Conversation
Defer mounting each Matrixify cell's StatefulChart (and therefore its data query) until the cell scrolls into view, using an IntersectionObserver. This prevents a large matrix from firing every cell's query at once. Gated behind the existing DASHBOARD_VIRTUALIZATION feature flag, matching the lazy-mount/unmount pattern dashboard Row.tsx already uses. A one-way latch keeps a cell mounted once seen, avoiding refetch churn while scrolling. Two paths force cells to render eagerly regardless of scroll position, so exports still capture the full matrix: - Server-side screenshot/report/thumbnail workers set navigator.webdriver; cells render immediately in that case. - Client-side "Download as Image/PDF" dispatches FORCE_IN_VIEW_EVENT (see downloadUtils.ts), optionally scoped to a batch of dashboard row ids for large dashboards. MatrixifyGridCell mirrors Row.tsx's own scoping (via the nearest ancestor [data-row-id]) so it mounts in step with its own row's batch instead of dumping every deferred cell's query into whichever batch happens to fire first - which would otherwise defeat the very batching downloadUtils.ts uses to avoid a thundering herd of chart queries.
dosubot
Bot
added
dashboard:performance
![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 #637bb7
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridCell.test.tsx - 1
- data-testid attribute mismatch · Line 309-309
Additional Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/virtualizationEvents.ts - 1
-
Missing unit tests for isForceInViewActiveForRow · Line 78-78The exported `isForceInViewActiveForRow` is called in MatrixifyGridCell.tsx at line 181 but has no direct unit test assertions. It needs tests that cover all three return paths (undefined, null, Set) and the null rowId case per BITO.md rule 6262.
-
Review Details
-
Files reviewed - 3 · Commit Range:
bc3c529..bc3c529- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridCell.test.tsx
- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridCell.tsx
- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/virtualizationEvents.ts
-
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
|
|
||
| expect(screen.getByText('SuperChart Mock')).toBeInTheDocument(); | ||
| expect( | ||
| screen.queryByTestId('matrixify-cell-placeholder'), |
There was a problem hiding this comment.
The placeholder element at line 265 uses data-test="matrixify-cell-placeholder" but all new tests use screen.getByTestId('matrixify-cell-placeholder') which looks for data-testid. This mismatch will cause every virtualization-related test to fail with: TestingLibraryElementError: Unable to find an accessible element with the test-id: 'matrixify-cell-placeholder'. Fix: change data-test to data-testid on the ChartPlaceholder component in MatrixifyGridCell.tsx:265.
Code Review Run #637bb7
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
This is a false positive: this repo globally remaps RTL's testIdAttribute to 'data-test'
(spec/helpers/setup.ts), which is exactly why StatefulChart.test.tsx uses the same data-test convention elsewhere.
There was a problem hiding this comment.
If the project's testing setup globally maps data-test to the testIdAttribute used by screen.getByTestId, then this suggestion is indeed a false positive. You can safely ignore this comment, as the existing test convention is consistent with the project's global configuration.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43053 +/- ##
=======================================
Coverage 66.55% 66.55%
=======================================
Files 2864 2865 +1
Lines 161891 161945 +54
Branches 37304 37324 +20
=======================================
+ Hits 107743 107781 +38
- Misses 52102 52118 +16
Partials 2046 2046
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:
|
Every existing test installs a mock IntersectionObserver in beforeEach, so the eager-render fallback for browsers without IntersectionObserver support was never exercised.
Code Review Agent Run #3ddc9eActionable 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 |
1 participant
SUMMARY
Matrixify grid cells did not participate in the existing
DASHBOARD_VIRTUALIZATIONmechanism that other dashboard content (Row.tsx) already uses to avoid firing every chart's query at once. As a result, a large Matrixify matrix (e.g. 10x10 = 100 cells) fires 100 chart queries in the same tick regardless of how many cells are actually visible, even with the flag enabled.This fixes that gap: each Matrixify grid cell now defers mounting its
StatefulChart(and therefore its data query) until the cell scrolls into view, using anIntersectionObserver, mirroring the lazy mount/unmount patternRow.tsxalready uses for the same flag - applied at the individual-cell level instead of the row level.A one-way latch keeps a cell mounted once it has entered the viewport, so scrolling back and forth doesn't cause refetch churn.
Two existing export paths are respected so captures still include the full matrix regardless of scroll position:
navigator.webdriver; cells render immediately in that case.FORCE_IN_VIEW_EVENT(src/utils/downloadUtils.ts), optionally scoped to a batch of dashboard row ids for large dashboards (forceLoadAllCharts's row-batching).MatrixifyGridCellmirrors the same scopingRow.tsxapplies (via the nearest ancestor[data-row-id]), so it only force-mounts when its own row's batch fires (or the dispatch is unscoped). Without this, a Matrixify chart in an already-mounted row would dump every deferred cell's query into whichever batch happens to fire first, defeating the purpose ofdownloadUtils.ts's row batching.The event-name constants are duplicated (not imported) from
src/dashboard/constants.tsinto a newpackages/superset-ui-core/.../Matrixify/virtualizationEvents.ts, sincepackages/superset-ui-corecannot depend on app code undersrc/dashboard.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - this changes query timing/count, not visual appearance. Off-screen cells render a same-sized empty placeholder instead of the chart until they scroll into view; there is no visible layout change.
TESTING INSTRUCTIONS
DASHBOARD_VIRTUALIZATIONfeature flag.Unit tests added in
MatrixifyGridCell.test.tsxcover: eager rendering with the flag off, deferred mount + IntersectionObserver-triggered mount, the mount latch, headless-capture eager rendering, unscoped and row-scopedFORCE_IN_VIEW_EVENThandling (including a cell mounting for the first time after its row's batch has already fired), and re-arming afterRESTORE_VIRTUALIZATION_EVENT.ADDITIONAL INFORMATION
DASHBOARD_VIRTUALIZATION(existing flag; this fixes Matrixify's per-cell rendering to respect it, it does not add a new flag)