fix(bench): wait for grouping to paint before opening the group-expand window - #483
Merged
Merged
Conversation
…d window `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 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Vercel preview readyPreview: https://pretable-b93mjaxah-cacheplane.vercel.app Updated automatically by the |
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.
Why
testhas been failing onmainsince Aug 17, which skipsDeploy → Vercel (production)and the preview smoke — grey checks, no red X — so production has been serving5f59f86whilemainisa438efb. Tracked by #482. A CI re-run failed identically, so it is not a transient flake.The failing test is
groups the grid BEFORE the group-expand measurement window opens.The bug
waitForGroupedRowModelgated 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 the paint, so the window opened early and silently folded grouping cost into the group-expand number.Worth naming: the pre-existing
countGroupRowsalso reads the model — nothing in the app consulted the DOM before this change.The fix
countPaintedGroupRows).partialinstead of measuring. A run that says it fell short beats a completed run that quietly measured the wrong thing.Verification
Every number below is from this branch, not a cache.
Tests 1 failed | 17 passed (18)— only the new test fails, so the teardown change breaks nothing.Tests 18 passed (18).Test Files 15 passed (15)/Tests 159 passed (159).typecheckclean,prettier --checkclean.The new test withholds paint forever and asserts the run declines to measure. An earlier version of it was vacuous — it mocked the DOM query, but the old code never queries the DOM at all, so the mock changed nothing and it passed on broken code. This version discriminates.
A second, separate defect found on the way
The new test passed alone and failed in the suite.
window[__PRETABLE_BENCH_RESULT__]is a global that nothing reset between tests, so a test waiting for a published result could match the previous test's result and assert against a run that never happened — it failed in 200 ms, before anything could have run. Cleared in the suite teardown, which closes the class for every test in the file.Note this only makes the measurement honest; it does not by itself prove the group-expand number was wrong in past runs.
🤖 Generated with Claude Code