fix(react): read every honesty input from one commit, and run the engine-sort rule - #435
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
…ine-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>
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
force-pushed
the
blove/data-scope-honesty-fixes
branch
from
August 15, 2026 17:15
a24ef7e to
1372843
Compare
Contributor
Vercel preview readyPreview: https://pretable-4blu2fxqy-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.
Two defects in
data-scope.ts, which turned out to be one problem and its consequence.1. Every narrowing query fired a false warning — and disarmed the real one
Filtering a server-backed grid printed:
on correct code.
aria-rowcountthen settled correctly, so it read as noise. It wasn't:warnOncelatches, so the spurious warning permanently suppressed the genuine one for the rest of the session. The bug cost the check, not just the console.Cause:
dataHonesty.loadedRowCountcame fromrowModelSnapshot.sourceRowCount, butmatchingTotalcame from theresultMeta.totalprop.useLocalRowModelingests rows in auseLayoutEffect, after the render that already read the prop — so for one render the check compared a new total against the previous query's row count.Fix: read both from the same commit. In rows mode that is
rows.length; in explicit-model mode there is no such prop and no skew, sosourceRowCountstays. Keyed offmodel === undefined, the discriminator the surface already uses — not offrows.length, which isEMPTY_ROWSin model mode and would have silently reportedloadedRowCount: 0for every model-mode grid, flippingresolveDataScopeto"loaded"for grids that hold everything. There is a test for that.The same skew existed in the fallback total (
resultMeta.totalabsent), pointing the other way — it would have fired on widening queries. Fixed with it, and separately tested.2.
warnOnEngineSortOverPartialWindowwas never wired upFully written, fully unit-tested, never called.
git log -Sshows it has never appeared in the surface in any commit — the 2026-08-09 plan specified it alongsideresolveAriaRowCountandresolveDataScope, and the step shipped two-thirds done.It is now called. The hazard is real: engine sort over a partial window presents "top N of a server-selected sample" under a truthful-looking
aria-sort.These are not independent. Wiring #2 without #1 false-positives on the lifecycle example —
filter: "external"with engine sort, whererows: [] → 12against a stalesourceRowCountof 0 makes 12 rows look like a partial window. Verified in a browser on a reverted build. #1 is a precondition for #2.Docs
/docs/server-data/totalscarried a section headed "One warning you may see, and cannot prevent." It existed only because of #1, and it is gone. The paragraph above it stays — the check is still a real floor — with two corrections the fix makes true./docs/server-data/query-ownershipdescribed the partial-window hazard while deliberately not promising a warning; it now says the grid warns, scoped to where that is provable.Verification
server-authority-aria.test.tsx./docs/server-datapages clean, and the original reproduction (Region → North) now settles ataria-rowcount121 with an empty console. The collector was validated by reverting the fix and confirming it does catch both warnings — a collector that has never caught anything proves nothing.pnpm build→api:check(no report changes),lint,typecheck,test: react 1161, website 549, docs guards 161.🤖 Generated with Claude Code