fix(plugin-chart-table): re-measure sticky layout when the table gains a box - #44430
Conversation
…s a box A table chart that mounts inside a `display: none` subtree -- an inactive dashboard tab, for instance -- measures its header against zero-sized boxes, so `StickyWrap`'s layout effect bails out at its `if (!theadHeight) return;` guard, computes no sticky layout, and renders only its `visibility: hidden` sizer. The chart is blank even though its query response arrived fine. None of that effect's dependencies (`maxWidth`, `maxHeight`, `setStickyState`, the scrollbar size) change when the tab becomes visible -- the width and height are static grid-layout math, not measurements -- so the measurement never runs again and the chart stays blank until it is force-refreshed or the window is resized. Observe the wrapper with a ResizeObserver while no sticky layout has been computed, and measure again once it gains a box. Fixes #44415 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code Review Agent Run #45e6dbActionable Suggestions - 0Additional Suggestions - 2
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 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Fixes table charts rendering blank when initially mounted under display: none (e.g., inactive dashboard tab) by re-triggering sticky layout measurement once the table gains a real box.
Changes:
- Add a
ResizeObserverfallback inuseStickyto re-measure when the wrapper becomes visible. - Add a regression test that simulates
display: nonemounting and verifies the table paints after a resize notification.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx | Observes wrapper size until sticky layout is computed, then re-measures to avoid blank render after hidden mount. |
| superset-frontend/plugins/plugin-chart-table/test/DataTable/hooks/useSticky.test.tsx | Adds a regression test and mocks to simulate hidden subtree measurements and ResizeObserver notifications. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| useEffect(() => { | ||
| const wrap = wrapRef.current; | ||
| if (columnWidths || !wrap || typeof ResizeObserver === 'undefined') { | ||
| return undefined; | ||
| } | ||
| const observer = new ResizeObserver(() => { | ||
| measure(); | ||
| }); | ||
| observer.observe(wrap); | ||
| return () => observer.disconnect(); | ||
| }, [columnWidths, measure]); |
| host.style.display = 'none'; | ||
| document.body.append(host); | ||
|
|
||
| render(<StickyTableHarness />, { container: host }); | ||
|
|
||
| expect(visibleDataCellCount(host)).toBe(0); | ||
|
|
||
| // Switching to the tab gives the chart a real box. Neither `maxWidth`, | ||
| // `maxHeight`, `setStickyState` nor the scrollbar size changes, so without | ||
| // the ResizeObserver the sticky layout is never recomputed and the chart | ||
| // stays blank until it is force-refreshed or the window is resized. | ||
| host.style.display = ''; | ||
| act(() => { | ||
| resizeObserver.trigger(); | ||
| }); | ||
|
|
||
| expect(visibleDataCellCount(host)).toBe(data.length * columns.length); | ||
|
|
||
| host.remove(); | ||
| resizeObserver.restore(); | ||
| jest.restoreAllMocks(); |
…sertion failure Move host.remove()/resizeObserver.restore()/jest.restoreAllMocks() into a finally block so a failing assertion mid-test can't leak the mocked ResizeObserver or the hidden host node into later tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM, thanks for tracking this down to the display:none mount race and adding a regression test for it.
Code Review Agent Run #58c810Actionable 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 |
SUMMARY
A table chart that mounts inside a
display: nonesubtree — an inactive dashboard tab — measures its header against zero-sized boxes, soStickyWrap's layout effect bails out at itsif (!theadHeight) return;guard, computes no sticky layout, and renders only itsvisibility: hiddensizer. The chart is blank even though its query response arrived successfully.Nothing brings it back. The measurement effect's dependencies are
maxWidth,maxHeight,setStickyStateand the scrollbar size; the width and height are static grid-layout math fromChartHolder, not measurements, so none of them change when the tab becomes visible. The measurement never runs again and the chart stays blank until it is force-refreshed (a remount, this time while visible) or the window is resized (a real width change) — exactly the two workarounds in #44415.This is why the issue reproduces only when the filter is applied from another tab: applying it on the chart's own tab does the one and only measurement while the chart has a box. It is also why only some charts blank — ECharts-based charts pass
width/heightexplicitly intoinit()(plugins/plugin-chart-echarts/src/components/Echart.tsx, with a comment saying it exists to avoid depending on DOM size) and are immune.No introducing commit exists in this repository.
git log -Sshows both theif (!theadHeight) return;guard and the[maxWidth, maxHeight, setStickyState, scrollBarSize]dependency list arriving with the monorepo import, 3c41ff6 (#17552), so they came in already-formed from the externalsuperset-uirepo. For history: #10432 (2020) fixed the same class of bug in the then-separate@superset-ui/plugin-chart-tablevia a version bump, and #22009 (2022) fixed the dashboard-side resize trigger in the old class-basedChart.jsx; that dashboard-side guard has no equivalent in today's hooks-basedChart.tsx, but it would not help here either, since the chart'swidth/heightprops genuinely do not change on a tab switch.The fix observes the sticky wrapper with a
ResizeObserverand measures again once it gains a box. The observer is attached only while no sticky layout has been computed and disconnects as soon as one exists, so charts that measured normally are untouched;measurere-reads the DOM itself, so a still-boxless notification is a no-op. A stable wrapper ref is observed rather than the existingtheadRef, because that ref is only attached while the sizer is rendered and its identity changes across renders without signalling the effect.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No screenshots: no running Superset instance was available in the environment where this was developed, so a genuine before/after of the blank chart could not be captured. The failure and the fix are demonstrated instead by the added regression test, which fails on
masterand passes with this change.TESTING INSTRUCTIONS
masterit is blank, and only a per-chart Force refresh or a browser-window resize brings it back.ADDITIONAL INFORMATION
🤖 Generated with Claude Code