Copilot commented on code in PR #44430:
URL: https://github.com/apache/superset/pull/44430#discussion_r4064674061
##########
superset-frontend/plugins/plugin-chart-table/test/DataTable/hooks/useSticky.test.tsx:
##########
@@ -203,3 +214,73 @@ test('sticky header/footer width matches the body,
independent of the scrollbar-
jest.restoreAllMocks();
});
+
+function installResizeObserverMock() {
+ const callbacks: ResizeObserverCallback[] = [];
+ const original = globalThis.ResizeObserver;
+ globalThis.ResizeObserver = class {
+ constructor(callback: ResizeObserverCallback) {
+ callbacks.push(callback);
+ }
+
+ observe() {}
+
+ unobserve() {}
+
+ disconnect() {}
+ } as unknown as typeof ResizeObserver;
+ return {
+ trigger: () =>
+ callbacks.forEach(callback => callback([], {} as ResizeObserver)),
+ restore: () => {
+ globalThis.ResizeObserver = original;
+ },
+ };
+}
+
+// When useSticky cannot measure, `StickyWrap` renders only its hidden
+// "sizer" table (`visibility: hidden`), which still contains a full tbody.
+// So count only the data cells a user could actually see.
+function visibleDataCellCount(container: HTMLElement) {
+ return Array.from(container.querySelectorAll('tbody td')).filter(cell => {
+ for (
+ let node: HTMLElement | null = cell as HTMLElement;
+ node;
+ node = node.parentElement
+ ) {
+ if (node.style?.visibility === 'hidden') return false;
+ }
+ return true;
+ }).length;
+}
+
+test('sticky table paints once it gains a box, when first rendered inside a
hidden dashboard tab', () => {
+ mockMeasurements();
+ const resizeObserver = installResizeObserverMock();
+
+ // An inactive dashboard tab keeps its charts mounted under `display: none`,
+ // so a chart that (re)mounts while its tab is hidden does its one and only
+ // layout measurement against zero-sized boxes.
+ const host = document.createElement('div');
+ 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();
Review Comment:
This test mutates globals (`globalThis.ResizeObserver`) and installs
prototype spies, but cleanup only happens at the end of the test body. If an
assertion throws earlier, globals/mocks can leak into subsequent tests. To make
the suite resilient, move cleanup to an `afterEach`/`afterAll`, or wrap the
test body in `try/finally` to ensure `host.remove()`,
`resizeObserver.restore()`, and `jest.restoreAllMocks()` always run.
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx:
##########
@@ -224,6 +225,30 @@ function StickyWrap({
});
}, [maxWidth, maxHeight, setStickyState, scrollBarSize]);
+ // update scrollable area and header column sizes when mounted
+ useLayoutEffect(() => {
+ measure();
+ }, [measure]);
+
+ // A `display: none` ancestor -- an inactive dashboard tab, for instance --
+ // gives the table no box, so the measurement above bails out and no sticky
+ // layout is computed, leaving the chart blank. None of that measurement's
+ // dependencies change when the table later gains a box, so watch for it
+ // directly and measure again. `measure` re-reads the DOM itself, so a
+ // still-boxless notification is a no-op, and the observer is only attached
+ // while there is no layout to show.
+ 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]);
Review Comment:
`columnWidths` is used as a truthiness sentinel for “sticky layout is
computed”. If `columnWidths` is initialized to an empty array/object (truthy)
before measurement succeeds, this effect will never attach the
`ResizeObserver`, and the hidden-mount blank table regression could persist.
Consider switching this guard to an explicit “computed” condition (e.g.,
`columnWidths?.length > 0`, or a dedicated boolean/field that indicates layout
has been successfully measured).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
