fix(bench): stop AG Grid autoHeight starving the jsdom event loop - #436
Merged
Conversation
… jsdom `main` went red on "dispatches comparator interaction scripts through measureBenchInteractionRun (B2 #5b)": Test timed out in 5000ms. Cause is #415 (f22cf92), which set `wrapText` + `autoHeight` on S2's wrapped columns. It is not a hang — the run completes. Instrumented, the mocked `measureBenchInteractionRun` is entered and the result is published on `window` within ~200ms; what fails is that `waitFor` gets only FOUR poll opportunities in five seconds, because AG Grid blocks the event loop for ~5.3s first. `autoHeight` is the whole of it, isolated by A/B on the two flags: both flags 375 rows / 15 000 cells in the DOM, run 5.3s wrapText 11 rows / 440 cells, run 0.28s autoHeight 375 rows / 15 000 cells, run 5.4s neither 11 rows / 440 cells, run 0.29s AG Grid auto height is a post-paint correction — the cell reports its size on mount and the apply pass sits behind a 1ms debounce (see the comment on ROW_HEIGHT in ag-grid-adapter.tsx). jsdom has no layout engine, so the correction never converges and the row renderer materialises half the dataset instead of a viewport's worth. The fix points the test at TanStack. Its subject is bench-app's DISPATCH — that a non-pretable adapter reaches measureBenchInteractionRun and is handed `undefined` for the telemetry override — and that branch keys on `adapterId === "pretable"`, so any comparator proves it. The measurement is mocked and never invokes the apply callback, so no adapter's native sort API was being exercised either way. Switching SCENARIO instead is not available: interaction scripts are gated to S2/S7 and both wrap three columns, so every legal scenario here mounts the autoHeight colDef. Raising the timeout would only make a five-second stall a slower green. The wrapped AG Grid path keeps its real coverage where layout exists: apps/bench/tests/ag-grid-wrap-auto-height.spec.ts (Chromium), which asserts rows grow past the 48px floor, vary in height, fit their content and use the matrix's 1.5 leading — 18 Playwright tests pass. A comment on the test records all of this so the adapter is not "restored" to ag-grid later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Vercel preview readyPreview: https://pretable-jyosc454m-cacheplane.vercel.app Updated automatically by the |
blove
added a commit
that referenced
this pull request
Aug 15, 2026
The comment above `warnOnEngineSortOverPartialWindow` was read as a performance claim and sent someone hunting a 6.5x bench regression inside this file. It is not a performance claim: this package is built by tsup with no babel-plugin-react-compiler, so nothing here is ever compiled and no runtime benchmark can observe the ordering either way. What the ordering really is, is a lint gate. `preserve-manual-memoization` is an ERROR in eslint.config.js, and moving the call below the `windowSpacers` memo fails the required `lint` job with "Compilation Skipped: Existing memoization could not be preserved", pointing at the memo's `ariaRowCount` dependency. Verified by mutation. The comment now says so, so the next reader reaches for `pnpm lint` instead of a benchmark. The B2 #5b regression this was blamed for is #415's AG Grid `autoHeight` colDef under jsdom, fixed on main by #436; the rebase picks it up. Measured here with both honesty changes intact and this package rebuilt: 57, 64, 70, 73ms, against 406-444ms before the rebase and 408-442ms with pretable-surface.tsx reverted to its pre-honesty state — the react source is not in that path at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blove
added a commit
that referenced
this pull request
Aug 15, 2026
…ine-sort rule (#435) * fix(react): read every honesty input from one commit, and run the engine-sort rule `rows` and `resultMeta.total` arrive on the same commit, but the row model ingests rows in a layout effect — after the render that already read the new total. `dataHonesty.loadedRowCount` came from the model while `matchingTotal` came from the prop, so the contiguous-window check compared a query's new total against the previous query's row count: narrowing 480 rows to 120 warned that the rows "cannot be a contiguous window", then settled at aria-rowcount 121 a render later. The console noise was the smaller half. `warnOnce` latches per page load, so that spurious first warning permanently disarmed the check that exists to catch a genuinely inconsistent `resultMeta` — the grid shipped with its honesty assertion switched off after the first filter. In rows mode the loaded count now comes from the `rows` prop the consumer just handed over, and the "no total supplied" fallback counts the same records. Explicit-model mode keeps reading the model: `rows` is `EMPTY_ROWS` there — `[]`, not `undefined` — so a `rows.length` read would report zero loaded records and flip `resolveDataScope` to "loaded" for grids that demonstrably hold everything. The discriminator is `model === undefined`, the one the surface already uses. `warnOnEngineSortOverPartialWindow` was fully written, fully unit-tested and never called from a render — absent from the API report, and never present in the surface in any commit. It is wired now, and it depended on the fix above: on the reverted build it fires on /docs/server-data/lifecycle at mount, because the same one-render skew makes an ordinary widening query look like a partial window. Verified in a real browser against a production build: the reproduction warns before and is silent after, and all four /docs/server-data pages load with an empty console. The docs section that documented the false positive as unavoidable is deleted; query-ownership.mdx now says the engine-sort hazard warns, and that silence is not a clearance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(react): name the gate that actually enforces the warning's position The comment above `warnOnEngineSortOverPartialWindow` was read as a performance claim and sent someone hunting a 6.5x bench regression inside this file. It is not a performance claim: this package is built by tsup with no babel-plugin-react-compiler, so nothing here is ever compiled and no runtime benchmark can observe the ordering either way. What the ordering really is, is a lint gate. `preserve-manual-memoization` is an ERROR in eslint.config.js, and moving the call below the `windowSpacers` memo fails the required `lint` job with "Compilation Skipped: Existing memoization could not be preserved", pointing at the memo's `ariaRowCount` dependency. Verified by mutation. The comment now says so, so the next reader reaches for `pnpm lint` instead of a benchmark. The B2 #5b regression this was blamed for is #415's AG Grid `autoHeight` colDef under jsdom, fixed on main by #436; the rebase picks it up. Measured here with both honesty changes intact and this package rebuilt: 57, 64, 70, 73ms, against 406-444ms before the rebase and 408-442ms with pretable-surface.tsx reverted to its pre-honesty state — the react source is not in that path at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mainis red — this fixes itbench-app.test.tsx > dispatches comparator interaction scripts through measureBenchInteractionRuntimes out at 5000ms. It failed theReleaseworkflow onf22cf928(#415) and reproduces 3/3 locally at load ~6.It is not a hang — it is event-loop starvation
Instrumented:
createBenchInteractionPlan, the mockedmeasureBenchInteractionRunandpublishBenchResultall fire within 200ms of the click, andwindow[BENCH_RESULT_KEY]is populated withstatus: "completed". Replaying with a 120s timeout,waitForresolved after 5085ms having run its callback only 4 times — RTL polls every 50ms, so 4 polls in 5s means the loop was blocked, not that anything failed to settle.The blocking work is inside AG Grid. A/B on #415's two flags:
wrapText+autoHeight(main)wrapTextonlyautoHeightonlyautoHeightis the entire cost. It matches the adapter's own comment added in #415: auto height is a post-paint correction behind a 1ms debounce, jsdom has no layout engine, the correction never converges, and the row renderer materialises half the dataset (375 of 750 rows × 40 cols) rather than a viewport's worth. Row inline heights stay pinned at the 48px floor throughout. Controls: TanStack on the same scenario is 32ms.At only ~6% over the limit this is load-sensitive, which is why
main's CI passed whileReleasefailed on the same commit.The fix
That one test now uses
adapter=tanstack, with a comment recording the measurements and forbidding a "restore".Its subject is bench-app's dispatch — a non-pretable adapter reaches
measureBenchInteractionRunand receivesundefinedfor the telemetry override — and that branch keys onadapterId === "pretable", so any comparator proves it. The measurement is mocked and never invokes the apply callback, so no adapter's native sort API was being exercised in the first place. Nothing AG Grid-specific is lost.The scenario could not move instead. Interaction scripts are gated to S2/S7 by
validateSupportedP0aRequest(packages/bench-runner/src/index.ts:426) and both havewrapped_columns: 3. Verified:?adapter=ag-grid&scenario=S1&script=sortpublishesstatus: "unsupported"and never callsmeasureBenchInteractionRun, so that test would have passed vacuously.The timeout was not raised, and no jsdom-detecting branch was added to
ag-grid-adapter.tsx— that is bench subject code, and special-casing it would make the jsdom tests measure something the browser does not do.Wrapped AG Grid coverage is retained
apps/bench/tests/ag-grid-wrap-auto-height.spec.ts(added by #415, real Chromium) is genuine rather than a selector match: rows must exceed the 48px floor, one must exceed 2×, heights must be distinct (which kills a "raise the fixed row height" fix), each row must equal its tallest cell's content ±3px, and leading must be 1.5 with more than one line box — plus the negative half on S1. Both pass.Verification
@pretable/app-bench test— 138 passed / 14 files (baseline at HEAD: 137 passed, 1 failed)playwright test— 18 passedexpected [Function] to be undefined; revertedFollow-up, not fixed here
bench-app.test.tsx:101still mountsag-grid+ S2 in jsdom. It passes only because it never awaits — theautoHeightstorm is queued and drains during later tests. Any new jsdom test that mounts AG Grid on a wrapped scenario and then awaits will hit the same stall. The durable answer is a jsdom-side cap or an explicit "AG Grid + wrapped scenario is browser-only" rule.🤖 Generated with Claude Code