test(permissions): wait on the array the assertion reads (objectui#8688) - #8689
Merged
Merged
Conversation
`providerCtxIdentity.discarded.test.tsx` had two waits keyed on the request
`log` followed by a read of `ctxSeen`, a different recorder. `log` is pushed
when a request is ISSUED; `ctxSeen` only fills once the response has landed,
because `MePermissionsProvider` renders `loadingFallback` — not `children` —
while `loading && !data`. So the wait did not gate the read, and on an
unfavourable ordering the read ran against an empty array.
Both sites are repaired by folding the assertion into the wait, so the two
cannot drift apart again. The `!` is gone with them: it converted "this can be
empty" into a compile-time promise that it cannot be, which is why nothing at
authoring time could flag the gap (objectui#8688).
Measured under a forced unfavourable ordering (the fetch double's Response
deferred 50ms past RTL's one-macrotask asyncWrapper drain):
old waits: 2 failed / 11 passed / 0 suite deaths
- `still refetches exactly once when the fetcher itself is
swapped`: TypeError: Cannot read properties of undefined
(reading 'userId') — the CI failure, reproduced verbatim
- `costs no redundant round trip …`: expected [] to have a
length of 1 — and its identity pin passed VACUOUSLY,
comparing undefined to undefined
new waits: 13 passed / 0 failed / 0 suite deaths, same ordering
13 passed under the ordinary ordering
Index form rather than `.at(-1)`: this package's tests inherit the root
`lib: ["ES2020"]`, under which `.at` is TS2550.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
This was referenced Sep 8, 2026
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-justin
marked this pull request as ready for review
September 8, 2026 22:50
os-justin
enabled auto-merge
September 8, 2026 22:50
This was referenced Sep 8, 2026
This was referenced Sep 9, 2026
Contributor
|
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
2 participants
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 #8688
providerCtxIdentity.discarded.test.tsxhad two waits keyed on the requestlogfollowed by a read ofctxSeen— a different recorder.logis pushed when a request is issued;ctxSeenonly fills once the response has landed, becauseMePermissionsProviderrendersloadingFallback(notchildren) whileloading && !data. So the wait did not gate the read, and on an unfavourable ordering the read ran against an empty array.Both sites are repaired by folding the assertion into the wait, so the wait and the read cannot drift apart again. The
!goes with them: it converted "this can be empty" — the fact the test needed to encode — into a compile-time promise that it cannot be, which is why nothing at authoring time could flag the gap.Not a flake — the defect was forced and observed
The file is green on most runs, so a green run proves nothing. The unfavourable ordering was forced: the fetch double still pushes to
logwhen the request is issued, but itsResponseis deferred 50ms, past RTL's one-macrotaskasyncWrapperdrain. (AsetTimeout(..., 0)sits inside that drain window and does not reproduce the race — measured on objectui#8664, and the reason its first repro leg landed on disk without reddening.)Every leg below is classified per test from vitest's JSON reporter (
--reporter=json --outputFile), never the text reporter, with the death strings (Element type is invalid,Failed Suites,No test suite found in file,Tests no tests,Transform failed) counted separately. All five legs: 0 death strings — every red below is a failing assertion, not a suite death.Leg 1's two reds, verbatim:
Line 620 of the mutated file is line 619 of the unmutated one, column 41 — the CI failure on PR #8656's
Test (shard 2/4), reproduced character for character.The second site is the worse one
The sweep found a second instance nobody had observed, in
costs no redundant round trip ...: the samelogwait, thenloaded = ctxSeen[ctxSeen.length - 1]!. Its failure mode is quieter:loadedbecomesundefined, and the identity pin below it,expect(ctxSeen[ctxSeen.length - 1]).toBe(loaded), then comparesundefinedtoundefinedand passes. Under leg 1 that test failed only oneffectRuns, with the identity pin passing vacuously.Asked of that pin — would an implementation strictly worse than the bug pass this? — the answer was yes, and it is measured rather than argued (ablation control, below).
Ablations — read sites only, never the pin
Each mutates production code (
MePermissionsProvider.tsx) from a committed tree, under atrap ... EXIT INT TERMwith absolute paths, proving the mutation on disk in both directions (anchor counts andgit hash-objectagainst theHEADblob) plus a line-total gate, and restoring by state (git checkout HEAD -- ABSOLUTE_PATH, thengit diff HEADproven empty).fetcherdropped from the fetch effect's dependency liststill refetches exactly once when the fetcher itself is swapped:expected 'A' to be 'B'userId: data?.userId ?? nullfrozen to a constant — the provider never publishes the fetched idexpected 'ablated-8688' to be 'A'HEAD~1costs no redundant round trip ...PASSEDThe control is the point: under a provider that never publishes the fetched payload, the old pin at the second site went green and the new one goes red. The repair there is a strengthening, not a relocation. And with
?.in place, the failure is a clean assertion diff instead of aTypeErrorthat reads like a broken component.The sweep
Population: all 9 test files under
packages/permissions/src/__tests__/(the card's file plus every sibling in the package), 22await waitForsites across the three files that have any.The detector discovers recorders mechanically per file (every
X.push(target), reads the identifiers named inside eachawait waitFor(...), then scans forward to the nextawaitand flags any recorder read that the wait did not name.Lit control: run against the pre-fix tree it flags 2 sites — line 619 (the one CI observed) and line 546 (the one nobody had) — while leaving the other 8
waitForsites in that file unflagged, including the two that legitimately wait onctxSeenand readctxSeen. So the sweep discriminates, and it did find a second instance rather than merely confirming the reported one. Against the fixed tree: 0 flags.Empirically lit as well: the leg-1 forced ordering reddened both flagged sites and none of the other 11 tests.
Out of the detector's reach, audited by hand instead:
MePermissionsProvider.test.tsx(5 waits) andMePermissionsProvider.retry.test.tsx(7 waits) have nopush-style recorders at all — they gate on a DOM node (loaded/errtest ids) and then read other test ids from the same render commit, plus mock call counts that are causally upstream of that commit. The family cannot occur in that shape.usePermissions.discardedIdentity.test.tsxhas recorders but nowaitFor(it is entirely synchronous).Scope
MePermissionsProvideritself is unchanged. The sweep found the test at fault, not the component; the provider was mutated only inside the ablation legs, and restored by state each time./me/permissionsrequest being setState-suppressed rather than aborted — is untouched. Nothing in this diff reaches the fetch or its cancellation token.ctxSeen[ctxSeen.length - 1]?.userId) rather than the card's suggestedctxSeen.at(-1)?.userId: this package's tests inherit the rootlib: ["ES2020"], under which.atisTS2550. The suggested shape runs green under vitest and failstsc -p tsconfig.test.json, which is exactly the vitest/tsc divergence class. (packages/app-shell/tsconfig.test.jsonsetslib: ["ES2022", ...], which is why.at(-1)is fine in 75 other test files and not here.)Verification
pnpm exec vitest run packages/permissions/— 9 files, 123 tests, 123 passed, 0 death strings.pnpm exec tsc --noEmitandpnpm exec tsc -p tsconfig.test.jsoninpackages/permissions— both exit 0, run afterpnpm --filter '@object-ui/permissions^...' build(on an unbuilt tree these reportTS2307for every workspace import, which is a precondition failure and not a measurement).pnpm exec eslint .inpackages/permissions— 0 errors (27 pre-existingno-explicit-anywarnings, none introduced here).node scripts/check-changeset-presence.mjs— red before the changeset, green after;node scripts/check-changeset-no-major.mjsgreen. The changeset has empty frontmatter: test-only, releases nothing. (skip-changesetis a phantom label in this repo and was not applied.)node scripts/check-governed-queue-guard.mjs --test ...— NOT GOVERNED; both paths checked against all 5 governed surfaces, none matched.🤖 Generated with Claude Code
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Generated by Claude Code