fix(bench): make S2 measure wrapped text in every grid, not just pretable - #415
Merged
Conversation
…able S2 is the project's self-described primary wedge benchmark — `wrapped_columns: 3`, `row_height_mode: "variable"`. Only pretable was wrapping. The three comparator adapters imported scenario TYPES only and never read `column.wrap`, so their code path on S1 and S2 was byte-identical: fixed 48px rows, `nowrap`, `overflow: hidden`. Every S2 comparison has measured pretable doing variable-height text layout against three grids not doing it. Closes #400. The flag was already on the wire. It was simply unread. - ag-grid: `wrapText` + `autoHeight` per column. Both are needed and neither implies the other — `autoHeight` alone gives tall single-line rows, `wrapText` alone gives clipped wrapped text. `AllCommunityModule` already pulls in `RowAutoHeightModule`; `rowHeight` becomes a floor via `Math.max(cellHeight, rowHeight)`. - mui: `getRowHeight: () => "auto"`. Sufficient on v9 without an `sx` override — `row--dynamicHeight > .cell` sets `whiteSpace: initial` with two class selectors, outranking the single-class `nowrap` default. `rowHeight` is KEPT: it is not inert, it is the base estimate for unmeasured rows. - tanstack: `measureElement` + `indexAttribute: "data-row-index"`, and the wrapped columns get pretable's own text model (`anywhere`/`pre-wrap`) so the two lay the same string out under the same rules. Every change is gated on `column.wrap`, and every test asserts BOTH directions. That is not ceremony: enabling wrapping unconditionally passes a positive-only test while silently moving every other scenario. Each was proven by mutation — including the negative arm, which is the one that catches it. Two things found while doing this that the issue did not have: `indexAttribute` was a trap. TanStack's `measureElement` defaults to `data-index`; this adapter emits `data-row-index`. A missing attribute yields `-1`, `resizeItem` early-returns, and the row silently keeps its estimate — console warning only. Measurement would have looked enabled and done nothing. The scroll script sampled `scrollHeight` ONCE and derived all 36 targets from it. AG Grid does not estimate auto-height rows, so content grows underneath the pass as cells are measured: the run would have aimed at a mostly-unmeasured model and covered a shrinking fraction of the dataset, while a grid that sizes rows up front covered all of it. A like-for-like break introduced BY this change, in exactly the scenario it exists to measure. Targets are now fractions resolved against the live extent — arithmetically identical for a fixed-height grid, so it changes only the case the old form got wrong. Expect S2 numbers to move, and expect pretable's apparent lead to shrink. `dom_nodes_peak` is `querySelectorAll("*").length`, proportional to rendered row count, so most of the reported 2.4-3.4x DOM advantage was the row-count gap the asymmetry created. Per rendered row it is 1.13x over MUI and 1.40x over AG Grid. S2 figures from before this commit are not comparable to figures after it. 138 bench tests, typecheck, lint, format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This was referenced Aug 15, 2026
…g in pixels Two findings from the first like-for-like S2 run. **AG Grid's `autoHeight` was working.** At rest its rows measure 236/236/119/314 px with a 2px error. The 264px error the benchmark reported is the UNMEASURED state: AG Grid's auto height is a post-paint correction, not a layout mode. The row carries an explicit `style.height` from the row model, each cell measures itself on mount, and the apply pass is debounced behind `_debounce(..., 1)` -> `setRowHeight` -> redraw. A newly rendered row therefore always paints at the 48px fallback and is corrected at least a frame later, and the measurement is DELETED when the cell is destroyed, so a row that scrolls out and back pays the two-pass cost again. Reproduced independently: the harness's 36-jump loop replicated standalone gives p95 264, peak rows 27, rows sampled at 48px. Four and ten settle frames only reach 261 and 217; it needs ~500ms per jump to converge. That is not fixable from the adapter and is now recorded in-source so nobody re-chases it. It is also the honest form of the wedge claim: not "AG Grid is slow" but "its auto height is a two-pass correction, so it paints rows at the wrong height during scroll". **Half the error was ours.** AG Grid's CSS derives cell `line-height` from ROW height — correct for a single-line cell, a category error for a wrapped one, giving every line of a paragraph the full row height as leading. AG Grid was wrapping at 2.79x leading against pretable's and TanStack's 1.50 and MUI's 1.43, so S2 was still not comparing the same layout. A theme parameter cannot fix it: `--ag-line-height` is combined under `min()`, so it can only make leading smaller than the row height, never release it. An explicit `lineHeight: 1.5` does. Error drops 264 -> 120. That line height is a per-adapter parity choice, not a neutral default, and it must be disclosed wherever these numbers are published. **The pixel proof.** The jsdom test asserted `ag-cell-wrap-text` and `ag-cell-auto-height` were present. Those classes are toggled straight off the colDef, so it passed whether or not a pixel moved — and it cannot do better, because jsdom has no layout engine. Wrapping is a layout fact, so the proof now runs in real Chromium: rows taller than the 48px floor, heights that vary, each row agreeing with its tallest cell, and text genuinely on more than one line so the leading ratio cannot pass vacuously. Three mutations each redden it (`autoHeight` removed -> 48; `cellStyle` removed -> ratio 2.79; flags applied unconditionally -> S1 rows move). The jsdom test stays as a cheap colDef guard, retitled to say what it cannot see. `rendered_rows_peak` staying at 27 is the expected result, not a miss: it is a 320px viewport over a 48px fallback plus AG Grid's default 10-row buffer each side — the peak frame IS the unmeasured state. 138 bench tests, typecheck, lint, format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`prepareText` segmented the whole string into graphemes, then `tokenizeText` re-segmented every token via `countGraphemes` purely to fill each token's `length`. Every character was segmented twice. A CPU profile of an S2 wrapped scroll put `segmentGraphemes` at 193ms of self time in a 761ms window — 25.4%, roughly 3x TanStack's entire library-side JS cost for the same scroll. Token lengths now come from the grapheme array `prepareText` already built. The mapping is not the obvious one, and the obvious one is wrong. A grapheme cluster can straddle a token boundary: in "a ␣́b" the space and the following combining acute are ONE cluster, while the tokenizer's regex splits inside it. The old code called `countGraphemes` per token and so charged that cluster to BOTH sides. Mapping each grapheme to the token containing its start offset gives different answers and would have silently changed wrapping for combining marks. A grapheme is therefore counted against every token its span overlaps, with a rewind when a cluster runs past a token's end. This is behaviour-preserving on purpose. The safety net asserts, for a 31-entry corpus of ZWJ families, regional-indicator runs where pair parity matters, skin-tone modifiers, combining marks, straddles, CRLF and a lone CR, that every token's new `length` equals what `countGraphemes(token.value)` returned before. Counting code units instead reddens 10 tests including two pre-existing ones; dropping the straddle rewind reddens exactly the straddle cases. A corpus-fitness test fails if the corpus stops exercising the traps, and a perf pin stubs `Intl.Segmenter` and asserts one `segment()` call for a 15-token input, where there were 16. Measured, 3 repeats each side, same session: segmentGraphemes self 185ms -> 37ms (4 call paths -> 2) sampled scroll window 758ms -> 496ms scroll_frame_p95_ms 33.3 -> 17.3 rendered_rows_peak 11 -> 12 Nothing about what gets drawn moved: rendered cells, blank gap frames, long tasks and the 4px row-height error are identical either side, which is the check that this removed waste rather than work. The recovered row is worth naming. #388 recorded #367 dropping `rendered_rows_peak` 12 -> 11 and asked whether that frame was a fair price for 48/48 line counts. The price was not the segment measurement; it was the duplicated segmentation alongside it. The row comes back without giving up the accuracy. Against TanStack on S2 scroll, 5 repeats: 17.3ms vs 17.3ms, a 0.0ms gap against 0.7ms of noise — statistically tied, from a 16.1ms deficit. pretable's own spread fell from 7.4ms to 0.3ms; the variance WAS this work landing unevenly across frames. 1133 react tests, 117 renderer-dom, 69 grid-core, 60 text-core, typecheck, lint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Vercel preview readyPreview: https://pretable-25y7tj6go-cacheplane.vercel.app Updated automatically by the |
blove
added a commit
that referenced
this pull request
Aug 15, 2026
) Owed by #388, and by the 2026-08-11 attempt whose timing half was abandoned as machine-contaminated. Both of its preconditions now hold: F1 is fixed and the TanStack 9 migration (#277) has landed. This is the FIRST valid S2 comparison. #415 made the three comparator adapters read `column.wrap`; before it, only pretable wrapped, so every previous S2 figure measured pretable doing variable-height text layout against three grids doing fixed 48px nowrap rows. #415 also changed the scroll script's targeting, which is why pretable's own `rendered_rows_peak` reads 12 here and read 11 on the commit before — the script moved, not the library. Medians over 7 repeats, all 28 runs completed: scroll_frame_p95_ms pretable 17.7 tanstack 17.6 mui 32.6 ag-grid 48.3 Fitness is argued from the spread WITHIN the runset rather than an absolute threshold: two adapters returned sd 0.40 and 0.39 ms and every structural count sd 0.00, which a machine distorting tail statistics cannot do. The wide spreads on ag-grid and mui are therefore those libraries, not the machine. The earlier criterion — beat the committed May 9.7ms — is deliberately not used, because the same ~17.5ms reproduces on a machine with zero swap and a third of the load, so it measures the current code and script rather than machine health. Three findings worth more than the headline: - pretable and TanStack are indistinguishable on frame time. No lead should be claimed. But TanStack gets there with a 592px scroll-anchor shift where the other three are 0 — that is where the tie stops being a tie. - pretable's DOM advantage is column virtualization, not per-cell leanness. It draws 72 rendered cells to TanStack's 640; per CELL, TanStack is leanest at 1.10 nodes against pretable's 1.88. A nodes-per-ROW reading inverts this. - No grid holds 60Hz here, pretable included (17.7 against a 16.7 budget). This file makes no 60Hz claim and discharges no ROADMAP gate. Also recorded unreconciled: #415's per-rendered-row DOM ratios do not reproduce against this runset. Neither figure is asserted over the other. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
blove
added a commit
that referenced
this pull request
Aug 15, 2026
… jsdom (#436) `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>
blove
added a commit
that referenced
this pull request
Aug 15, 2026
…ous zero (#438) * fix(bench): row_height_error reports not applicable instead of a vacuous zero `row_height_error_p95_px` compares a row's rendered box against its tallest cell's content height, and `cell.scrollHeight` is floored at `cell.clientHeight` — so on a grid rendering `white-space: nowrap` no row height, however wrong, can move it. What the comparison reported there was the constant offset between the row's border box and the cell's padding box: identical on every row of every frame, and read by the published page as a score. Every comparator adapter rendered exactly that way until #415, which is why their committed 0/1/2 was never an achievement. Part of #414. Detection is per row, from the live computed `white-space`, not from the scenario's `wrapped_columns` and not from a per-adapter flag. The scenario only says what the adapter was ASKED to do, and an adapter that ignores the flag is the defect #400 found; a per-adapter constant is wrong again, because the same adapter wraps on S2 and does not on S1. Membership in the non-wrapping set is a positive, exact match on `nowrap` or `pre`, so an unfamiliar value keeps the metric REPORTING rather than sliding into a status that also cannot fail. Only wrapping-capable cells now set the expectation. A `nowrap` sibling's `scrollHeight` is its own `clientHeight` — the row's geometry echoed back — and letting it into the `max` lets box-model noise stand in for content. Measured on S2/dev this changes nothing (the wrapped cell is the tallest either way). The p95 is emitted only when something was measurable, alongside a new `*_measurable_rows` count that is emitted always. `packages/bench-runner` requires the COUNT where it used to require the p95, and requires the p95 back the moment the count says a row could have failed — strictly stronger than before, and the reason "not applicable" cannot become a cheaper way to satisfy the gate than measuring. Mutation-proven in a real browser (`row-height-error-applicability.spec.ts`); jsdom cannot host any of it — `getBoundingClientRect()` returns zeros, `scrollHeight` is 0, and `getComputedStyle().whiteSpace` is undefined, so all three inputs to the rule are absent there. pretable/S2/dev/scroll: control measurable_rows 116, p95 4 rows forced to height: 34px measurable_rows 87, p95 150 S1 (nowrap) measurable_rows 0, p95 absent S1 with the check deleted measurable_rows 272, p95 1 <- the defect 138 bench tests, 16 bench-runner, 23 playwright, typecheck, lint, format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(bench): H1 cannot claim uniqueness over an unmeasured competitor H1's uniqueness clause is "no measured full-grid competitor achieves the same combined quality". `row_height_error_p95_px` is now absent exactly when the grid rendered nothing that could overflow a wrong row, and `undefined <= 1` is false — so an unmeasurable competitor would have read as one that FAILED the row-height sub-criterion and pushed H1 toward `satisfied` on no evidence at all. That is the same defect #414 is about, one layer up: a missing measurement scoring in pretable's favour. H1 now returns `insufficient` and names the adapter and the reason. Unreachable on today's data — since #415 every comparator wraps on S2 — which is exactly why it needed pinning rather than leaving to accident. The test asserts both directions: the same fixture with the competitor measured must still reach a verdict, or it would pass against an H1 that had simply been broken. Proven by mutation: replacing the guard's condition with `false` fails it and nothing else. 88 script tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(bench): withdraw the AG Grid row-height inference, and print n/a as n/a The published page read AG Grid's 2px as "a sign that wrapped-cell layout doesn't round-trip through its line-height pipeline as cleanly as pretable's text-core does" — an inference about wrapped-cell layout, drawn about a grid that was not wrapping. In that runset only pretable wrapped (#400); the other three drew fixed 48px `nowrap` rows, so their figures are a box-model constant and can support no claim about layout at all. Closes #414. The inference is deleted, not replaced. Re-deriving it would need a post-#415 runset and none exists — #415 shipped the wrapping comparators and recorded that the re-measure is still owed and needs a quiet machine. There is nothing honest to put in its space, so nothing is put there. Two other sentences rested on the same numbers and are corrected rather than kept: "≤ 1px row-height drift" as a joint pretable/MUI claim, and "MUI matches pretable on quality" — MUI matches on the axes both were measured on. pretable's own 1px stands and is now stated as what it is, a fact about pretable rather than a comparison. The milestone JSON gains a `metricApplicability` block recording which values it holds that cannot support what they look like, in the same spirit as the `superseded` block already there. Nothing measured was changed: the run happened and the record should say what it was. The table prints `n/a` for the three, the number for pretable, and a paragraph saying why — because rendering "no opinion" the same way as "scored zero" is the whole defect. Proven by mutation: dropping the applicability lookup prints AG Grid's 2 again and fails the test. 553 website tests, typecheck, lint, format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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>
blove
added a commit
that referenced
this pull request
Aug 15, 2026
…439) * fix(bench): honour pinned_left in the ag-grid and tanstack adapters S2, S3 and S7 set `pinned_left`, and every dataset column carries the resulting `pinned` alongside `wrap`. pretable has always honoured it; the comparators read neither until #415 took the wrapping half. So S2 was still comparing one grid maintaining a sticky zone on every scroll frame against three that were not. - ag-grid: `pinned: 'left'` on the colDef. Plain COMMUNITY field, no module. - tanstack: `columnPinningFeature` + `columnSizingFeature`, with `columnPinning` in `state` (not `initialState`, so it follows a dataset swap) and `size` on each columnDef so `getStart()` agrees with the drawn `gridTemplateColumns`. TanStack is headless, so the sticky CSS is the app's to write — that is what a TanStack user pinning a column does, and it is TanStack's own bookkeeping the benchmark then pays for. Headers pin too, or they scroll away from the cells they name. **MUI is deliberately unchanged.** Column pinning is an MUI X Pro feature and the matrix runs Community. Verified against the installed `@mui/x-data-grid` 9.11.0 rather than from the docs: no `pinnedColumns` prop on `DataGridProps`, no `pinned` on `GridColDef`, no `columnPinning` under `hooks/features/`. The issue assumed otherwise. Hand-rolling sticky cells there would measure this repo's CSS instead of MUI, so the gap is documented on `toColDef` and reported with the numbers instead. Every change is gated on `column.pinned`, and every test asserts both directions. Mutation-checked at both layers: removing the pinning reddens the unit test AND the real-Chromium test; pinning unconditionally reddens the negative arms that protect the `pinned_left: 0` scenarios (S1, S4, S5, S6). The browser spec is the one that matters. jsdom has no layout engine, so the unit tests can only see that a colDef was emitted or an inline style written — both passed throughout while nothing was pinned at all. The new spec scrolls a real grid and asserts the pinned cell holds its viewport x, carries the same text (a virtualizing grid can recycle a node into another column), and that the scroller actually moved, so the assertion cannot pass vacuously. Expect S2 numbers to move again: ag-grid and tanstack now maintain a pinned zone they previously did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * bench(status): supersede the S2 re-baseline on pinning The 2026-08-15 runset was measured with S2's `pinned_left: 1` honoured by pretable only. The file called itself "the first like-for-like wrapped-text measurement" — accurate about wrapping, silent about pinning, which #413 had already predicted would happen. #413 changes that for ag-grid and tanstack, so their numbers and every comparison drawn against them are superseded. MUI stays asymmetric permanently: column pinning is an MUI X Pro feature and the matrix runs Community, so any future S2 run compares three grids with a pinned zone against one without and has to say so. No fresh runset here. The machine was at load 29 on 10 cores from concurrent agent work, and measuring through that is the exact mistake the 2026-08-11 attempt made. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
blove
added a commit
that referenced
this pull request
Aug 15, 2026
…tered (#442) * test(bench): mount the comparator surface tests at smoke, and re-time MUI's ceiling Three tests in this file mounted a comparator adapter on S2 — a scenario with `wrapped_columns: 3` — at the default `dev` scale of 750 rows x 40 columns. Since #415 (f22cf92) taught the comparator adapters to honour `column.wrap`, that switches on their wrapped-cell measurement, which jsdom has no layout engine to converge. The ag-grid one is the case worth naming, because its own reported duration said it was fine. Measured paired, both scales in one process, with a drain after mount so the DEFERRED correction is counted rather than just `render()`: adapter scale event loop blocked DOM nodes after drain ag-grid dev 23235-25282ms 19803 ag-grid smoke 91-2515ms 1603-4053 tanstack dev/smoke ~2ms 135 mui dev/smoke ~2ms 5094 At ~100-230ms the ag-grid test looked cheapest of the three. It never awaits, so the 23s it queues at mount drained into whichever tests ran next in the file — it externalised its cost rather than avoiding it. That is the same stall that timed the B2 #5b dispatch test out in #434 and cost two Releases and two skipped production deploys. All three assert WHICH surface renders, which is scale-independent, so smoke asserts exactly the same thing. MUI is a different finding and is recorded as one: it materialises 5094 nodes and blocks ~2ms at BOTH 120 and 750 rows, because its virtualizer caps the rendered window either way. `scale=smoke` does not speed it up — the ~1-2s is MUI X DataGrid's inherent jsdom mount cost. Its ceiling is kept, but re-timed 30_000 -> 15_000: the wrapped-scale trap that justified the wider ceiling is gone, and a ceiling that generous stops reporting regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(bench): fence the jsdom wrapped-scale rule so #434 cannot be re-entered The previous commit fixed three tests that had walked into the trap #415 opened. This makes walking back into it fail loudly. The rule: a jsdom test may not mount a COMPARATOR adapter on a scenario with `wrapped_columns > 0` at any scale other than `smoke`. pretable is exempt — the cost is the comparators' wrapped-cell measurement, not pretable's. It fails rather than silently capping the scale. Forcing `smoke` behind the author's back would let a test that counts rows pass while measuring a dataset it never asked for, which is the same class of defect — a check that passes without testing the thing — this repo has spent a lot of effort removing. Mechanism is a runtime assertion at the top of each comparator adapter, gated on jsdom's user agent, not a scan of test sources. Three reasons: - It fires on the mount itself, so it catches a violation reached through a computed search string, a helper, or a direct adapter mount — anything a source scan would have to re-implement a module graph to see. - It cannot false-positive on a query string that is never rendered. query-state.test.ts parses `?adapter=ag-grid&scenario=S2&scale=hypothesis` and never mounts it; a grep-based rule would have reddened it. - It fires BEFORE the cost is paid — 58ms, against the 23s the mount would have queued. The wrapped-scenario set is derived from scenario-data (`wrapped_columns > 0`), not hardcoded to S2/S7: S4 and S5 also wrap columns and the list has drifted. Two layers, because either alone has a hole. Unit tests cover the assertion in both directions — it fires on wrapped-above-smoke, and stays silent on smoke, on an unwrapped scenario at dev, and on a hand-rolled dataset. A behavioural fitness test then derives the comparator list from `adapterRegistry` and mounts each one, so an adapter added to bench-app.tsx WITHOUT the guard call reddens rather than quietly re-opening the trap. Both layers were proven by mutation, not assumed: - A deliberate `?adapter=mui&scenario=S7` (dev) test failed in 58ms with a message naming the fix; removing it went green. - Deleting the guard call from tanstack-adapter.tsx reddened exactly "the tanstack adapter refuses to mount a wrapped scenario above smoke". - Applied to the four real violations on main, it reddened those four and nothing else. What it cannot catch is stated in the test file: a hand-rolled dataset with no `scenario` field, cost that is not wrapped-cell measurement, and anything outside jsdom — the last deliberately, since dev-scale wrapped comparator runs are the whole point of the benchmark in a real browser. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(bench): give the adapter registry its own module, so the fence stays cheap The wrapped-scale fitness test derives its comparator list from `adapterRegistry`, which is what makes the rule survive a new adapter. Reading it from bench-app.tsx meant a third vitest worker pulling in bench-app's whole graph — bench-runtime, bench-runner, @pretable/react, the interaction and update planners — none of which that test uses. That import is not free, and a fence that costs more than the trap it guards is a bad trade. Measured on the fitness file alone: import 25.44s -> 16.67s total 29.42s -> 18.70s `comparatorAdapterIds` moves here with it, so "which adapters are comparators" is stated once rather than re-derived at each call site. Also drops the last `react-refresh` lint warning in bench-app.tsx: the file no longer exports a non-component alongside `BenchApp`. 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.
S2 is the project's self-described primary wedge benchmark —
wrapped_columns: 3,row_height_mode: "variable". Only pretable was wrapping. The threecomparator adapters imported scenario TYPES only and never read
column.wrap,so their code path on S1 and S2 was byte-identical: fixed 48px rows,
nowrap,overflow: hidden. Every S2 comparison has measured pretable doingvariable-height text layout against three grids not doing it. Closes #400.
The flag was already on the wire. It was simply unread.
wrapText+autoHeightper column. Both are needed and neitherimplies the other —
autoHeightalone gives tall single-line rows,wrapTextalone gives clipped wrapped text.AllCommunityModulealreadypulls in
RowAutoHeightModule;rowHeightbecomes a floor viaMath.max(cellHeight, rowHeight).getRowHeight: () => "auto". Sufficient on v9 without ansxoverride —row--dynamicHeight > .cellsetswhiteSpace: initialwith two classselectors, outranking the single-class
nowrapdefault.rowHeightis KEPT:it is not inert, it is the base estimate for unmeasured rows.
measureElement+indexAttribute: "data-row-index", and thewrapped columns get pretable's own text model (
anywhere/pre-wrap) so thetwo lay the same string out under the same rules.
Every change is gated on
column.wrap, and every test asserts BOTH directions.That is not ceremony: enabling wrapping unconditionally passes a positive-only
test while silently moving every other scenario. Each was proven by mutation —
including the negative arm, which is the one that catches it.
Two things found while doing this that the issue did not have:
indexAttributewas a trap. TanStack'smeasureElementdefaults todata-index; this adapter emitsdata-row-index. A missing attribute yields-1,resizeItemearly-returns, and the row silently keeps its estimate —console warning only. Measurement would have looked enabled and done nothing.
The scroll script sampled
scrollHeightONCE and derived all 36 targets fromit. AG Grid does not estimate auto-height rows, so content grows underneath the
pass as cells are measured: the run would have aimed at a mostly-unmeasured
model and covered a shrinking fraction of the dataset, while a grid that sizes
rows up front covered all of it. A like-for-like break introduced BY this
change, in exactly the scenario it exists to measure. Targets are now fractions
resolved against the live extent — arithmetically identical for a fixed-height
grid, so it changes only the case the old form got wrong.
Expect S2 numbers to move, and expect pretable's apparent lead to shrink.
dom_nodes_peakisquerySelectorAll("*").length, proportional to rendered rowcount, so most of the reported 2.4-3.4x DOM advantage was the row-count gap the
asymmetry created. Per rendered row it is 1.13x over MUI and 1.40x over AG Grid.
S2 figures from before this commit are not comparable to figures after it.
138 bench tests, typecheck, lint, format.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Follow-ups filed rather than folded in, so this change's effect on the numbers stays attributable: #413 (comparators ignore
pinned_left), #414 (row_height_error_p95_pxcannot fail for a nowrap grid, and the bench page reads meaning into it).Not yet done: re-measuring S2 and updating the published page. That is the next step and needs a quiet machine.
🤖 Generated with Claude Code