test(app-shell): close the approvals teardown race that trips the network-escape guard - #7764
Merged
Merged
Conversation
…file `RecordDetailView.approvalDeclaredActions.test.tsx` installed its `fetch` double inside `stubApprovalsApi` and tore it down in an unconditional `afterEach`. A decision dispatched through `DeclaredActionsBar` carries `refreshAfter: true`, so on success the record page re-reads the approval state — `handleApprovalActionDone` calls `void approvals.refresh()` and the `notifyDataChanged` beside it runs the same read again through the record-invalidation effect. Neither is awaited by the console or by the test, which asserts on the POST and returns, so a `GET /api/v1/approvals/requests?object=…` was still in flight at teardown. Vitest runs `afterEach` in reverse registration order, so this file's teardown ran first — before RTL `cleanup()` and before the network-escape guard's assertion — and restored the real `fetch` while that read was pending. Whether the read landed before or after the restore was pure timing. Install one double at module scope instead and never remove it: the per-test router still swaps, so call counts keep meaning "this test's reads", but the window where a late probe can reach a real socket no longer exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
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-sam
marked this pull request as ready for review
September 5, 2026 16:27
This was referenced Sep 5, 2026
os-sam
pushed a commit
that referenced
this pull request
Sep 5, 2026
…teardown goes
The guard's failure message prescribed `vi.stubGlobal('fetch', router) +
vi.unstubAllGlobals()` with no word about WHERE the unstub goes. That pair is
safe only when nothing the component started is still in flight at teardown,
and objectui#7439 is the measured counter-example: a file with exactly the
prescribed shape still escaped intermittently, because a decision carrying
`refreshAfter: true` makes the record page re-read approval state after the
test body returns and the prescribed teardown put the real `fetch` back while
that read was pending.
Vitest runs `afterEach` in reverse registration order, so a teardown written in
the test file runs before the root setup's RTL `cleanup()` and before the
guard's own assertion. The message now says so, and gives the two remedies in
order: unmount before unstubbing (which the referenced DatasetReportRenderer
`afterEach` already does, with its own measured note), and — when the component
can issue a read after the test body returns — install one double at module
scope and never tear it down, following the worked example PR #7764 landed.
The shrink-only ledger pin printed a second copy of the same prescription. It
now points at the guard's text instead of restating it: one ruling written
twice, with one copy rotting, is the defect this card is about.
No change to what the guard catches: `KNOWN_ESCAPES` and the verdict logic are
untouched, and only the thrown message's template literal moved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
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 #7439
Everything below was measured in this run, on
8f9a2406c.Which of the two candidate causes it is
The card offered two. Measured: it is (1), the teardown window in the test file — not an un-awaited effect in
RecordDetailView.tsx.The late probe was captured with a stack trace at the moment the network-escape guard records an escape:
RecordDetailView.tsx:1096ishandleApprovalActionDone, reached from the api handler'srefreshAfterbranch (useConsoleActionRuntime.tsx:438). It is the record page's declared, intentional post-decision re-read: the call isvoid-ed on purpose, with a comment saying so, and nothing in the console is in a position to await it. A second escape in the same run came fromRecordDetailView.tsx:1037, the record-invalidation effect thatnotifyDataChangeddrives — a normal React passive effect. Neither is a dropped promise; no runtime source is at fault, so the file surface stays inside the test file and Clause-2 staysno.What ends early is the test: the decision cases await only
authFetchSpybeing called, i.e. the moment the POST is dispatched, and then return with the follow-up read still in flight.Second measured fact — the hook order that makes the window real. Vitest runs
afterEachin reverse registration order, so this file's own teardown runs first, ahead of the root setup's RTLcleanup()and ahead of the guard's assertion. A probe registered immediately after that teardown reportedfetch is still the double? falseon all 13 tests: the realfetchis back while the tree is still mounted andcleanup()has not run yet.The repair
Install the
fetchdouble once at module scope and never tear it down. The per-test router still swaps, so a call count still means "this test's reads"; there is simply no point in the file's lifetime at which the realfetchis installed. A block comment states why it must outliveafterEach, so the next cleanup does not put the teardown back.Not done, deliberately: no
KNOWN_ESCAPESentry, no skip, no quarantine, no widened timeout.Evidence — deterministic first, repetition only as a supplement
A single green run is worthless here, so the ordering was forced rather than looped. The forcing holds the decision POST's response until teardown (the call is still recorded synchronously, so the test body's
waitForstill resolves and the test still returns), then releases it from a hook positioned to run after this file's teardown. Same forcing, same guard instrumentation, three legs:e546222b3)L1's two red tests are
POSTs to the REQUEST, not to the business record it is opened onandfolds a declared decision output into the nested outputs body— the second is the one the card reports. L1 reproduced the card verbatim: same URLhttp://localhost:3000/api/v1/approvals/requests?object=qif_report&recordId=QIF202607310002, and the escape attributed to the same test,folds a declared decision output into the nested outputs body. L1 vs L2 is the clean comparison — identical forcing, identical instrumentation, the diff is the only variable.L3 is supplementary and is not "pre-fix behaviour": with the double hoisted, re-adding
vi.unstubAllGlobals()destroys it permanently rather than re-opening a window, which is why it is far redder than L1. It does establish that the removed line is load-bearing.Repetition, reported honestly as a supplement and not as proof: the file ran to completion 4 times on the fixed tree across this work. Three of those were file-scoped and gave 13 of 13 each (two plain runs and the L2 leg); the fourth covered it inside the 27-file, 188-test neighbouring-suite run, also green. That says nothing about the next run; the L1/L2 pair is the argument.
The instrument fired. The same guard instrumentation that reported 0 escapes in L2 reported 4 in L1 and 33 in L3, in the same file with the same command — so the zero is a reading, not a silent instrument.
The repaired test can still fail when the behaviour is genuinely wrong. The decision-output fold in
useConsoleActionRuntime.tsxwas mutated so its prefix test never matches. Result: exactly 1 of 13 red —folds a declared decision output into the nested outputs body— reporting the nestedoutputsobject missing from the body, with the other 12 green. Restored afterwards;git hash-objectequals the HEAD blob andgit diff HEADis empty.Every mutation in this run ran under a
trap ... EXIT INT TERM, was proven to have reached disk before the run (injected-text and removed-text counts, both directions), and was proven restored afterwards by object hash against the HEAD blob plus an emptygit diff HEAD.Verification, all on
8f9a2406cpnpm exec turbo run lint --concurrency=2— 47 successful, 47 total, 0 errors (repo-wide; not narrowed).pnpm --filter @object-ui/app-shell run type-check— green. It is a real reading, not a vacuous one: an earlier iteration failed inside this very test file atTS2348, so the edited file is inside the compiled set.pnpm exec vitest run scripts/__tests__/network-escape-ledger.test.ts packages/app-shell/src/views/RecordDetailView packages/app-shell/src/hooks/useRecordApprovals— 27 files, 188 tests, all pass. The ledger pin is included because it is the reconcile gate forKNOWN_ESCAPES; this change adds no line to that list.package.jsonand.github/workflows/(the dispatch-gate deriver lives in the sibling repo and answers only about that tree):check-changeset-presence,check-changeset-no-major,check-changeset-fixed,check-changeset-overwrite,check-control-bytes,check-vi-mock-inherit,check-vi-mock-specifiers,check-lint-coverage,check-type-check-coverage,check-shell-escape-residue— all exit 0.check-changeset-presencenames it and calls that "a complete answer to this gate".Exit codes were captured before any pipe throughout, and each verdict above quotes the gate's own line rather than a bare status.
Serial constraints, re-measured at implementation time
Across all 11 open PRs (225 files):
RecordDetailView0,useRecordApprovals0,vitest.setup.network-escape-guard.ts0. Controls that had to fire and did: PRs touchingpackages/9, PRs touching.changeset/7. One PR (#7685) touchesapp-shell/src/views/but only undermetadata-admin/. Nothing contended.Generated by Claude Code