react-supergrain: drop tbody remount so isKeyed detects keyed rows - #2
Merged
Merged
Conversation
`npm run isKeyed keyed/react-supergrain` reported the implementation as non-keyed for the 'run benchmark' with 0 added and 0 removed TRs. The <tbody> was keyed by an epoch counter that was bumped on run and clear, so a rebuild replaced the whole tbody instead of its rows. The keyed detector's MutationObserver watches `table.table` and counts TRs in each mutation's addedNodes/removedNodes, but filterTRInNodeList only recurses into a node when that node is itself a TR. A replaced TBODY therefore hides all 1000 TRs from the count, and the run reads as non-keyed even though the rows are keyed by item id. Remove the epoch and let the rows mount and unmount within a stable tbody. Verified locally: the epoch build reproduces the reported error exactly, and without it isKeyed reports keyed for run, remove and swap. The nine CPU benchmarks (01_ through 09_) all complete successfully. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EjGtKNCwotZVC23paQe4i6
There was a problem hiding this comment.
Pull request overview
This PR fixes a benchmarking classification issue for the react-supergrain keyed implementation by preventing <tbody> remounts during run()/clear(), so webdriver-ts’s isKeyed logic can observe row reuse correctly.
Changes:
- Removed the
tbodyEpochcounter and its increments onrun()andclear(). - Removed the
<tbody key={tbodyEpoch}>that forced<tbody>to remount on rebuilds, preserving the existing<tr>reconciliation visibility forisKeyed.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Fixes the
isKeyedfailure reported on upstream krausest/js-framework-benchmark#2068.Base is
add-supergrain— the branch krausest#2068 points at — so merging this updates that PR.Cause
The
<tbody>was keyed by an epoch counter bumped inrun()andclear(), so a rebuild replaced the tbody instead of its rows.filterTRInNodeListinwebdriver-ts/src/isKeyed.tsopens a node only when that node is itself aTR. A replacedTBODYtherefore hides its 1000 TRs, and both counters stay at 0.The rows were keyed by
item.idthroughout — 0 of 1000 TR elements are reused across a re-run, with or without the epoch — but the test needs the mutations to be visible.Change
src/main.tsx: removedtbodyEpoch, its two bumps, andkey={tbodyEpoch}. One file, +1 / −9.Verification
npm run isKeyed -- --runner playwright --headless true keyed/react-supergrainnow passes:No
ERROR: ... is not correctly categorized. The epoch build reproduces the reported failure exactly.Benchmarks
01_through09_all pass. No measurable perf change — medians within noise over 10 runs.🤖 Generated with Claude Code
https://claude.ai/code/session_01EjGtKNCwotZVC23paQe4i6