From a14a8b99a1780793945ae91e697b9a97e1043d7c Mon Sep 17 00:00:00 2001 From: Brian Love Date: Sat, 22 Aug 2026 17:16:07 -0700 Subject: [PATCH] fix(bench): wait for grouping to PAINT before opening the group-expand window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `waitForGroupedRowModel` gated on the row model, but the invariant it exists to hold is about the screen: if the group rows render inside the measurement window, that render is what gets measured instead of the toggle. Under CI load the model settles several frames before React commits, so the window opened early and silently folded grouping cost into the group-expand number — which is what made `groups the grid BEFORE the group-expand measurement window opens` fail on main, skipping the production deploy (#482). Gate on group rows actually committed to the DOM. When the frame budget is exhausted without a painted grouping, report `partial` rather than measuring the wrong thing — a run that says it fell short beats a completed run that quietly measured something else. Note the pre-existing `countGroupRows` reads the model too; nothing in the app consulted the DOM before this. The new regression test withholds paint forever and asserts the run declines to measure. Proven red against unpatched code, green against patched. Also clear `window[__PRETABLE_BENCH_RESULT__]` in the suite's teardown: it is a global that nothing reset, so a test waiting for a published result could match the PREVIOUS test's and assert against a run that never happened. That is exactly how the new test passed alone and failed in the suite. Co-Authored-By: Claude Opus 5 --- apps/bench/src/__tests__/bench-app.test.tsx | 77 +++++++++++++++++++++ apps/bench/src/bench-app.tsx | 25 ++++++- 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/apps/bench/src/__tests__/bench-app.test.tsx b/apps/bench/src/__tests__/bench-app.test.tsx index 950568eeb..426a8adcd 100644 --- a/apps/bench/src/__tests__/bench-app.test.tsx +++ b/apps/bench/src/__tests__/bench-app.test.tsx @@ -15,6 +15,10 @@ describe("BenchApp", () => { afterEach(() => { cleanup(); vi.restoreAllMocks(); + // The published result is a global and nothing else clears it, so a test + // that waits for one can match the PREVIOUS test's result and assert + // against it — passing or failing on a run that never happened. + delete window[BENCH_RESULT_KEY]; }); test("renders selected scenario metadata and publishes a terminal result", async () => { @@ -443,6 +447,79 @@ describe("BenchApp", () => { expect(groupRowsAtCallTime).toBeGreaterThan(0); }, 20_000); + test("declines to measure group-expand when the grouping never paints", async () => { + // The CI failure this pins: the row MODEL settles several frames before + // React commits the paint, so gating the wait on the model alone opened the + // measurement window with zero group rows on screen — folding the grouping + // render into a number that is supposed to measure only the toggle. + // + // Withholding the paint FOREVER is what makes this discriminate. Gating on + // the model (the old behaviour) ignores the DOM entirely and still reports + // `completed`; gating on the paint runs out its frame budget and reports + // `partial`, which is the honest answer when the precondition never held. + const realQuery = Element.prototype.querySelectorAll; + // Both prototypes: the app queries `viewportRef.current ?? document`, and + // Document does not inherit Element's method — stubbing only Element left + // the fallback path live, which is why this first passed alone and failed + // in the suite. + const realDocumentQuery = Document.prototype.querySelectorAll; + const hide = (selector: string) => + selector === "[data-pretable-group-row]" + ? "[data-nonexistent-so-this-is-empty]" + : selector; + + vi.spyOn(Element.prototype, "querySelectorAll").mockImplementation( + function (this: Element, selector: string) { + return realQuery.call(this, hide(selector)); + } as typeof Element.prototype.querySelectorAll, + ); + vi.spyOn(Document.prototype, "querySelectorAll").mockImplementation( + function (this: Document, selector: string) { + return realDocumentQuery.call(this, hide(selector)); + } as typeof Document.prototype.querySelectorAll, + ); + + const measureSpy = vi + .spyOn(benchRuntime, "measureBenchInteractionRun") + .mockResolvedValue({ + status: "completed", + notes: ["interaction mode: group-expand"], + metrics: { + interaction_latency_ms: 9, + settle_duration_ms: 8, + post_interaction_blank_gap_frames: 0, + post_interaction_anchor_shift_px: 0, + post_interaction_row_height_error_p95_px: 0, + post_interaction_row_height_error_measurable_rows: 11, + result_row_count: 40, + selected_row_preserved: 1, + focused_row_preserved: 1, + dom_nodes_peak: 400, + rendered_rows_peak: 11, + rendered_cells_peak: 440, + }, + }); + + render( + , + ); + + await waitFor( + () => { + expect(window[BENCH_RESULT_KEY]).toMatchObject({ + adapterId: "pretable", + }); + }, + { timeout: 20_000 }, + ); + + expect(window[BENCH_RESULT_KEY]).toMatchObject({ status: "partial" }); + expect(measureSpy).not.toHaveBeenCalled(); + }, 30_000); + test("runs the group script through the interaction probe with the grouping applied by the trigger", async () => { const interactionSpy = vi .spyOn(benchRuntime, "measureBenchInteractionRun") diff --git a/apps/bench/src/bench-app.tsx b/apps/bench/src/bench-app.tsx index 420225c4c..d1fa644a0 100644 --- a/apps/bench/src/bench-app.tsx +++ b/apps/bench/src/bench-app.tsx @@ -231,6 +231,19 @@ export function BenchApp({ search, browserVersion }: BenchAppProps) { continue; } + // The model is grouped, but React may not have committed the paint yet, + // and the invariant this wait exists to hold is about the SCREEN: if the + // group rows render inside the measurement window, that render is what + // gets measured instead of the toggle. Under CI load the model settles + // several frames before the commit lands, so gating on the model alone + // opens the window early and silently folds grouping cost into the + // group-expand number. + if (countPaintedGroupRows() === 0) { + previousRowCount = -1; + stableFrames = 0; + continue; + } + if (snapshot.visibleRowCount === previousRowCount) { stableFrames += 1; @@ -243,8 +256,11 @@ export function BenchApp({ search, browserVersion }: BenchAppProps) { } } + // Budget exhausted. Only report a grouped model if it actually reached the + // screen — an unpainted one would reopen the hole above, and a `partial` + // run that says so beats a completed run measuring the wrong thing. const snapshot = pretableGridRef.current?.rowModel.getState().snapshot; - if (!snapshot) return null; + if (!snapshot || countPaintedGroupRows() === 0) return null; for (let index = 0; index < snapshot.visibleRowCount; index += 1) { const row = snapshot.rowAt(index); if (row?.kind === "group") return row; @@ -252,6 +268,13 @@ export function BenchApp({ search, browserVersion }: BenchAppProps) { return null; } + /** Group rows actually committed to the DOM, which is what the measurement + * window's precondition is about — `countGroupRows` reads the model. */ + function countPaintedGroupRows() { + const scope: ParentNode = viewportRef.current ?? document; + return scope.querySelectorAll("[data-pretable-group-row]").length; + } + /** * Setup barrier for the row-set change scripts: hand over only once the resident * window is mounted and the surface has stopped moving.