fix: quit handler cleanup ordering and timeout test coverage#689
fix: quit handler cleanup ordering and timeout test coverage#689pedramamini merged 1 commit intomainfrom
Conversation
…rt whitespace Follow-up to PR #677 (quit confirmation safety timeout): - Move powerManager.clearAllReasons() after processManager.killAll() to prevent late process output from re-arming the sleep blocker - Add tests for the 5s safety timeout (force-quit, clear on confirm, clear on cancel) - Assert killAll/clearAllReasons call ordering - Revert whitespace-only changes to docs/releases.md
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe quit-handler implementation reorders power management cleanup to occur after process termination instead of before. The test suite is updated to enforce call sequencing and adds three new test cases validating safety timeout behavior when quit confirmation is delayed, confirmed, or cancelled by the renderer. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR makes two targeted improvements to the quit-handler subsystem as a follow-up to #677. Source change ( Test change (
The logic is sound and the new tests are well-structured. Two minor style suggestions are noted:
Confidence Score: 5/5Safe to merge — the source change is a one-line reorder with clear correctness rationale, and the new tests solidify coverage of the timeout path. Both findings are P2 style/maintenance suggestions (fake-timer cleanup guard and exporting the timeout constant). Neither represents a correctness defect or data-integrity risk. All remaining concerns are non-blocking. No files require special attention; the minor suggestions are in the test file only. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (Cmd+Q)
participant E as Electron app
participant R as Renderer
participant PM as ProcessManager
participant PW as PowerManager
U->>E: quit attempt
E->>E: before-quit fired
E->>R: app:requestQuitConfirmation
E->>E: arm 5s safety timeout
alt Renderer confirms (< 5 s)
R->>E: app:quitConfirmed
E->>E: clearConfirmationTimeout()
E->>E: app.quit() → before-quit (confirmed)
E->>PM: killAll()
E->>PW: clearAllReasons()
else Renderer cancels (< 5 s)
R->>E: app:quitCancelled
E->>E: clearConfirmationTimeout()
else Renderer never responds (≥ 5 s)
E-->>E: timeout fires → force-quit
E->>E: app.quit() → before-quit (confirmed)
E->>PM: killAll()
E->>PW: clearAllReasons()
end
Reviews (1): Last reviewed commit: "fix: quit handler followup — reorder cle..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| vi.useFakeTimers(); | ||
|
|
||
| const { createQuitHandler } = await import('../../../main/app-lifecycle/quit-handler'); | ||
|
|
||
| const quitHandler = createQuitHandler(deps as Parameters<typeof createQuitHandler>[0]); | ||
| quitHandler.setup(); | ||
|
|
||
| const mockEvent = { preventDefault: vi.fn() }; | ||
| beforeQuitHandler!(mockEvent); | ||
|
|
||
| // Renderer was asked for confirmation | ||
| expect(mockMainWindow.webContents.send).toHaveBeenCalledWith('app:requestQuitConfirmation'); | ||
| expect(mockQuit).not.toHaveBeenCalled(); | ||
|
|
||
| // Advance past the 5s timeout without renderer responding | ||
| vi.advanceTimersByTime(5000); | ||
|
|
||
| expect(mockQuit).toHaveBeenCalled(); | ||
| expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('timed out'), 'Window'); | ||
|
|
||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
vi.useRealTimers() is placed at the end of each fake-timer test, but if any expect between vi.useFakeTimers() and vi.useRealTimers() throws, the process-wide timer state is left in fake mode for all subsequent tests. This can cause mysterious hangs or failures in otherwise unrelated tests.
The same pattern appears in all three new tests (lines 374–395, 398–421, 423–443).
Consider centralising the cleanup in an afterEach:
afterEach(() => {
vi.useRealTimers(); // idempotent when timers are already real
vi.restoreAllMocks();
});Then the individual vi.useRealTimers() calls at the end of each test can be removed.
| expect(mockQuit).not.toHaveBeenCalled(); | ||
|
|
||
| // Advance past the 5s timeout without renderer responding | ||
| vi.advanceTimersByTime(5000); |
There was a problem hiding this comment.
5000 is duplicated in all three new tests (lines 390, 417, 439) but QUIT_CONFIRMATION_TIMEOUT_MS is not exported from quit-handler.ts. If the timeout is ever adjusted, the tests will silently diverge.
Exporting the constant makes the coupling explicit:
// quit-handler.ts
export const QUIT_CONFIRMATION_TIMEOUT_MS = 5000;// quit-handler.test.ts
import { QUIT_CONFIRMATION_TIMEOUT_MS } from '../../../main/app-lifecycle/quit-handler';
// ...
vi.advanceTimersByTime(QUIT_CONFIRMATION_TIMEOUT_MS);Bring quit handler safety timeout (#677, #689) and power manager into RC. Resolve merge conflicts: - quit-handler.ts: keep both deleteCliServerInfo (RC) and powerManager + QUIT_CONFIRMATION_TIMEOUT_MS (main) - index.ts: keep Cue engine stop (RC) and powerManager + stopSessionCleanup (main) - releases.md: take main's version (more complete changelog)
1 participant
Summary
powerManager.clearAllReasons()afterprocessManager.killAll()in shutdown cleanup to prevent late process output from re-arming the sleep blockerkillAllis called beforeclearAllReasonsFollow-up to #677.
Test plan
Summary by CodeRabbit
Bug Fixes
Tests