Stop two CI flakes blocking the merge queue - #369
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ceiling | 383d3b1 | Commit Preview URL Branch Preview URL |
Aug 22 2026, 06:31 PM |
Both have failed PRs today that had nothing to do with them. TrayPanel: afterEach calls vi.restoreAllMocks(), which resets every mock to the implementation it was created with - and for a bare vi.fn() that is "return undefined". LocaleProvider does listen(...).then(...), so a call landing outside the beforeEach-to-test window threw TypeError: Cannot read properties of undefined (reading 'then') and failed whichever test was on screen. It was always the star-ask test because that is the only one that renders the prompt. eventMocks.listen is now declared with a resolved-promise default, so a restore leaves it harmless; beforeEach still installs the listener-capturing version. secure_file: the delete-pending retry added for the Windows release race also sat on a directory planted at the sibling path, which Windows reports as access-denied too but which never clears. That spent the whole grace period before failing and broke the sub-second bound in an_unenforceable_lock_fails_closed_instead_of_writing_unserialized. The retry now skips a directory, the grace is 250ms rather than a second, and the test asserts against that constant instead of a bare literal.
tsc infers the mock's call signature from the implementation it is created with, so a zero-argument default rejected the two-argument mockImplementation in beforeEach. The build typechecks the test files, so this failed CI even though vitest was green.
182b0d8 to
383d3b1
Compare
1 participant
Two unrelated flakes have each failed PRs today that did not touch them.
1.
TrayPanel > keeps a counted star ask visible when the tray is emptyFailed on #367 and #355 with:
afterEachcallsvi.restoreAllMocks(), which resets every mock to the implementation it was created with — and for a barevi.fn()that is "return undefined".LocaleProviderdoeslisten(...).then(...), so any call landing outside thebeforeEach→test window throws.It is always this test because it is the only one that sets
starPromptMocks.reason, so it is the only one that renders the prompt and takes the slower path.eventMocks.listenis now declared asvi.fn(() => Promise.resolve(() => {})), so a restore leaves it harmless.beforeEachstill installs the listener-capturing version the tests drive.Proof — same file, forcing
eventMocks.listen.mockReset()before the render:listendeclared asvi.fn()(before)1 failed | 24 passed— the exact CI errorvi.fn(() => Promise.resolve(…))(after)25 passed2.
an_unenforceable_lock_fails_closed_instead_of_writing_unserializedFailed on #355. This one is my regression from #368. That PR added a retry for Windows' delete-pending state, which reports access-denied. A directory planted at the sibling path — which is exactly what this test does — is also access-denied on Windows, but it never clears. The retry sat on it for the full second and broke the test's sub-second bound.
The retry now skips a directory, the grace drops from 1s to 250ms (still orders of magnitude longer than delete-pending needs), and the test asserts against
DELETE_PENDING_GRACErather than a bare literal so the two cannot drift.Verified
Frontend: 88 files, 707 tests passing. Rust: 26
secure_filetests, clippy-D warningsclean.Note
Low Risk
Test-only mock change plus a narrower Windows lock retry that fails faster on an unfixable directory sibling. Does not change the successful lock or write path.
Overview
Stops two unrelated CI flakes: a Vitest mock restore that made
listen()returnundefined, and a Windows exclusive-create retry that sat on a directory for the full grace period.eventMocks.listenis now created as a function that resolves to an unsubscribe, sovi.restoreAllMocks()inafterEachno longer breaksLocaleProvider'slisten(...).then(...)outside thebeforeEachwindow.On the lock path,
try_exclusive_createno longer retriesPermissionDeniedwhen the sibling is a directory (that never clears on Windows).DELETE_PENDING_GRACEdrops from 1s to 250ms, and the fail-closed test asserts against that constant so it cannot drift.Reviewed by Cursor Bugbot for commit 383d3b1. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix CI flakes in
TrayPaneltest mock andsecure_filelock retry logiceventMocks.listenmock in TrayPanel.test.tsx to return a resolved Promise to prevent errors when mocks are restored.DELETE_PENDING_GRACEfrom 1 second to 250 milliseconds in secure_file.rs.StateWriteLock.try_exclusive_createto fail fast when the lock path is a directory instead of retrying until the deadline.DELETE_PENDING_GRACEreduction lowers the maximum wait time for exclusive-create retries on Windows.Macroscope summarized 383d3b1.