test(document-viewer): name the cause when render-ahead is never scheduled, and record the five unreproducible Windows failures - #2252
Conversation
…duce on Linux A full `npm run test` on a clean Windows workstation at cdfcbac exited 1 with four files and five tests failed. None of them reproduce here. On Linux at HEAD all five pass individually, and a full `GATE_RECEIPTS=off npm run test` is green: 696 files / 7770 tests passed, 4 skipped, exit 0. The document-viewer pair also passed three consecutive runs under 3x CPU oversubscription, so ordinary load does not reproduce it either. None of the four test files, nor their subjects, changed between cdfcbac and HEAD, so the Linux green applies to the reported SHA and the three intervening commits did not quietly fix these. The record keeps the classification open rather than closing it. Items 3-5 are timing shapes and are plausibly load-induced. Items 1 and 2 are deterministic state assertions, so a Windows-only failure there is a real Windows-only defect and must not be written off as environmental; closing them out needs the actual assertion text from that machine. It also captures the leads found by reading: the migration-role guard grades untracked files and hashes on-disk bytes, and the document-viewer `flushIdle()` helper is structurally fragile because the render-ahead effect never schedules an idle callback when the canvas budget resolves to one. `tests/flake-ledger.json` is explicitly Playwright-only, so none of these five Vitest failures are eligible for quarantine there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEzkroXdtL9kYo8rW5eZh2
…duled Both reported document-viewer failures land on the same line: `flushIdle()` waiting for `idleCallbacks.length` to exceed zero. All `waitFor` can say there is "expected 0 to be greater than 0", which reads as a slow machine and is usually not one. The viewer schedules a render-ahead idle callback only while `liveCanvasLimit > 1`, and `resolveLiveCanvasWindow` collapses that to 1 whenever `perCanvasPixels` is 0 — which is what jsdom's zero-width layout measurement yields before `contentWidth` lands. The test's `cancelIdleCallback` stub is a no-op, so `idleCallbacks` only ever accumulates callbacks from earlier effect runs. If the first run measures a zero-width canvas, none is ever pushed and no amount of waiting produces one. So the timeout now throws with the budget explanation and the observable state at that moment: slot count, rendered count, observer count. A run that mounted and rastered the reader's page but scheduled nothing is now distinguishable from one that was merely slow. Assertions are unchanged; this only replaces an opaque timeout with a named failure. Verified both ways: the file still passes 9/9, and forcing the stub to drop callbacks produces "8 page slot(s), 1 rendered, 1 intersection observer(s)". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEzkroXdtL9kYo8rW5eZh2
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:48 minutes Limit details: You’ve used the included review currently available. Your 88 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Uh oh!
There was an error while loading. Please reload this page.
… was wrong Four merge-safe inbox requests; none touches the canonical ledger. Corrects my own earlier record. The request filed in #2252 reasoned that failures 1 and 2 were deterministic state assertions and therefore real Windows-only defects rather than load noise. That was wrong for failure 1: the assertions are fixed, but the runtime is not, because the test spawns Git Bash twice. It is cancelled and replaced by one record carrying the resolved status of all five — two fixed (#2255, #2256), two unreproducible with a legibility fix (#2252), and hosted-migration-role-guard still genuinely unexplained, with the untracked-file hypothesis and the diagnostic to capture if it recurs. Updates #CCZ4HB with the second measurement of the review-bot budget: every PR opened this session was skipped, the countdown shrank across it, and the binding constraint has moved from the rate limit to the organisation spending cap — a billing decision no repo gate can substitute for. It also names the interaction worth writing down: the draft-by-default rule guarantees no bot review until someone undrafts, and undrafting mid-CI cancels the run and reds the required aggregate. Draft default, review budget and cancel-in-progress churn are one problem seen from three sides. Updates #6GW95D with a wider scope than it records. A stray `npx vitest` from the home directory enumerated 253 checkouts of this repository across six distinct roots — .codex, .gemini, .copilot, opencode, Documents/Codex, and a pnpm store with its own .worktrees. A sweep scoped to .claude/worktrees covers none of them. The #XCAX01 safety preconditions still apply before any pruning. Both updates restate the prior text verbatim rather than replacing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEzkroXdtL9kYo8rW5eZh2
Summary
npm run teston a clean Windows workstation atcdfcbaccdexited 1 with four files / five tests failed. None reproduce on Linux. A new immutable outstanding-issues inbox request captures the evidence, the leads found by reading, and — deliberately — leaves the classification open rather than closing it as environmental.flushIdle()intests/document-viewer-page-virtualization.dom.test.tsxis the shared failure point of two of the five, and all it could report wasexpected 0 to be greater than 0. It now throws with the budget explanation and the observable state at timeout. Assertions are unchanged.What the investigation established
On Linux at HEAD, all five pass individually, and a full
GATE_RECEIPTS=off npm run testis green: 696 files / 7770 tests passed, 4 skipped, exit 0 (196 s). The document-viewer pair also passed 3/3 consecutive runs under 3× CPU oversubscription, so ordinary load does not reproduce it.git diff cdfcbaccd..HEADshows none of the four test files, nor their subjects (scripts/check-hosted-migration-role.mjs,scripts/setup-codex-cloud.sh,supabase/**,src/components/document-viewer/**), changed between the reported SHA and HEAD. So the Linux green applies tocdfcbaccditself, and the three intervening commits did not quietly fix these.Two things worth not glossing over:
hosted-migration-role-guard› "accepts the current repository state" andcodex-cloud-setup› "writes managed shell policy behaviorally" are deterministic state assertions. A Windows-only failure there is a real Windows-only defect, not load noise, and is not dismissed as environmental here. Closing them out needs the assertion text from that machine.tests/flake-ledger.jsonis explicitly Playwright-only (@quarantinetitle, exact spec match). All five failures are Vitest, so none are eligible for quarantine and none were added.The
flushIdle()changeThe viewer schedules a render-ahead idle callback only while
liveCanvasLimit > 1, andresolveLiveCanvasWindowcollapses that to 1 wheneverperCanvasPixelsis 0 — which is what jsdom's zero-width layout measurement yields beforecontentWidthlands. The test'scancelIdleCallbackstub is a no-op, soidleCallbacksonly ever accumulates callbacks from earlier effect runs; if the first run measures a zero-width canvas, none is ever pushed and no amount of waiting produces one. That failure is indistinguishable from a slow machine, which is how it would be misfiled.Verified both directions: the file still passes 9/9, and forcing the stub to drop callbacks produces the new message —
8 page slot(s), 1 rendered, 1 intersection observer(s)— which shows the viewer mounted and rastered the reader's page but scheduled nothing.Verification
npm run test— full offline suite,696 passed (696)files,7770 passed | 4 skipped (7774)tests, exit 0npm run typecheck— passed (4371 input files)npm run lint— passed (4371 input files)npm run format— clean; affected file reports(unchanged)npm run check:outstanding-issues—Ledger inbox check passed: 8 pending request(s), 485 applied; guard passed, 423 rowsnpx vitest run tests/document-viewer-page-virtualization.dom.test.tsx—Tests 9 passed (9), before and after the changerequestIdleCallbackstub to drop callbacksVerification not run: npm run verify:pr-local— its constituent gates for this scope (lint, typecheck, full unit suite, format) were each run directly and are listed above; the wrapper would only repeat them on unchanged content.UI verification not run:the diff contains no production UI, routing, styling, or browser behaviour — the only source-tree change is a test helper's failure message.Risk and rollout
git reverteither commit independently; they are separate and unrelated in effect.Notes
The five failures remain open and unclassified as to root cause. This PR does not claim to have fixed them — it records what is now known and makes two of them fail legibly if they recur. The decisive missing evidence is the assertion text from the Windows workstation:
One lead worth checking first on that machine:
validateRepository()grades untracked, non-ignored files, becauserepositoryEntries()runsgit ls-files --cached --others --exclude-standard. Any stray file underscripts/,supabase/migrations/,.github/workflows/, or the guarded exact paths whose name or content matches the reserved role turns that test red whilegit statusstill looks clean enough to call the tree empty — as would a nested git worktree directory under the repo root, sincereadFileSyncon a directory entry throwsEISDIR. It also SHA-256s the on-disk bytes of the pinned immutable migration, so CRLF in the working tree would break it, though.gitattributes* text=auto eol=lfshould prevent that and the bytes hash correctly on Linux.Also worth correcting in any local notes: the previously known Windows-local failures (
tests/session-start-hook.test.ts, worker-observability) passed in the reported run, so that "known environmental failures" note is stale.Run
npm run issues:reconcilefrom a serialized ledger branch after this lands.Generated by Claude Code