Repository navigation
test(ai): hold the no-grant reads open for a late request - #2546
Conversation
Three flows-ai negative cases read the free-grant request count once, as the page settled, so a regression that sent the grant request late (behind idle work, a timer, or a later effect) passed them. Each now reads the count after LATE_GRANT_REQUEST_WINDOW_MS through expectNoGrantRequest. Negative control: a temporary injection in followEligibility's not-ready branch that sends one grant request once the page goes quiet passed all three original cases and fails each fixed case. Refs #2529 Refs #2500 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
KyleMit
left a comment
There was a problem hiding this comment.
Independent review by the codex rival agent, round 1, of pull request 2546.
No merge-blocking defects found in the full diff. Reviewed all three call sites, grant dispatch, credential hydration, the shared mock, repository guidance, and the PR’s timing/control evidence. Type checking and lint passed; the complete targeted Playwright spec passed 10/10 through the handler.
Independently reproduced both temporary controls: A failed all three cases with Expected: 0 / Received: 1; B failed the access-code generation case while the two drawer-only cases passed, matching the disclosed limitation. Splitting the controls fairly demonstrates additional coverage in each case. All injections were reverted and the worktree is clean.
The negative-path state and storage writes precede the possible awaited fetch; neither supplies a deterministic signal after a would-be request. The 750 ms window is a reasonable empirical observation budget, with no code-defined deadline supporting a uniquely better value. Its coverage remains bounded: credential hydration continues in the background, so the comment’s claim that on-time requests are already counted describes measurements rather than a universal ordering guarantee. The final access-code assertion retains requests counted throughout the preceding flow.
The strongest alternative is a harness-level fail-on-request stub: it would report forbidden traffic immediately, improve diagnostics, and prevent callers from forgetting a final count assertion. It would still need an observation window before teardown. The local helper is acceptable for these three cases under the documented bounded-negative pattern.
Drafted follow-ups (not filed)Drafts for the user to review. None has been opened. 1. Let
|
Campaign unit F8 (lane C) of the
ship-campaign parallel=3run. Refs #2529 (tracking), Refs #2500 (source: its leftovers comment, follow-up 8, from audit 53).Spec
web/tests/flows-ai.spec.tshas three negative cases that readgrant.requests()once, as the page settles:a fresh installation does not fetch an AI allowance or show the canvas actiona migrated BYO key reveals the AI button on the next launchthe AI button posts the drawing and reveals the generated resultA regression that sent the free-grant request late would pass them. Done when: a late grant request fails each of the three cases, a negative control proves it (injection present: each fixed case fails; injection absent: each passes), and the spec is stable across repeated targeted runs.
What changed
Only
web/tests/flows-ai.spec.ts. No shared helper was touched.web/tests/flows-ai.spec.ts:30: addsLATE_GRANT_REQUEST_WINDOW_MS = 750, with its WHY comment, andexpectNoGrantRequest(page, grant), which waits out the window and then reads the count. Thegrantparameter is typed frommockFreeGrant's real return type.:69,:205and:249: each case's single readexpect(grant.requests()).toBe(0)becomesawait expectNoGrantRequest(page, grant).type Pageinline, andenableAiInSettingsuses it in place ofimport('@playwright/test').Page.Approach, and why the window is bounded rather than signal-driven
The brief preferred a deterministic signal that the send window has closed. None exists here.
followEligibilityinweb/src/lib/state/freeGenerations.svelte.ts, which callsrequestGrant(). That fetch goes out oneawait installationId()after the decision.inactivegrant state, and theunavailablestate together with itsfreeGenerationBadgeHintstorage write. All of it lands before the point where an awaited fetch would go. So no UI or storage state is ordered after a request the path might send.docs/TESTING.md, "Writing flake-resistant specs", case (b). That means a named, bounded window after which the count is read, ascoloring-pack-download-gate.spec.ts(IDLE_WORK_OBSERVATION_MS) andflows-settings-report.spec.ts(LATE_REPORT_SETTLE_MS) already do. A slower worker only lengthens the real wait, so the window cannot false-red.expect.poll(...).toBe(0): it passes on its first sample.Sizing N from the path's own timing. Measured with temporary
performance.now()logging (never committed) over three instrumented runs of the whole file:fetchdispatchA grant request sent on time is therefore already counted by the old read, hundreds of milliseconds early. The window exists only for a late one. 750 ms is more than three times the whole navigation-to-request span, and it matches the repo's existing idle-work negative window. The cost is +0.75 s for each of the three tests.
Negative control
Two temporary injections went into the code path all three cases guard:
followEligibility's not-ready branch, after hydration. Each sends one grant request once the page has "settled". Neither is committed; the diffs are below. Every cell is--repeat-each=3at--workers=1on port 5333, limited by--grepto the three cases.The access-code generation case needs B. Its flow has a reveal wait of more than 600 ms with no input, so A fires mid-flow, and even the original read catches that.
Each failure is
Expected: 0 / Received: 1at the case'sexpectNoGrantRequestread.Result: every case has a late request that the original single read misses and the fixed spec catches.
Injection absent, fixed spec: the whole file with
--repeat-each=3passed 30/30 (59.0 s). Baseline on freshmain077c34f before any change: 10/10. That also shows the just-merged generate-image and safety-classifier changes did not disturb this spec.Injection A (input-quiet), never committed
Injection B (input- and DOM-quiet), never committed
Commands run
SPLOTCH_E2E_PORT=5333 npm run test:e2e -- web/tests/flows-ai.spec.ts --workers=1: the baseline onmain(10/10), the fixed spec--repeat-each=3(30/30), and the control matrix above.npm run check: exit 0.npm run lint: exit 0.npx prettier --check web/tests/flows-ai.spec.ts: clean.SMOKE_PORT=5334 npm run test:browserless: all 5 checks passed.Review and merge gate
unrelated. Fetchingmainand running thereconcile-with-mainsurvey found 0 incoming commits, so there was nothing to merge. Gatedmainis the merge base 077c34f.Caveats
LATE_GRANT_REQUEST_WINDOW_MSafter the read still passes, as injection B shows for the two drawer-only cases.🤖 Generated with Claude Code