Conversation
Replaces the native Menu::quit() role with a labeled menu item so Cmd+Q fires MenuItemClicked, then routes the press through a Livewire overlay that requires a sustained 1.5s hold before calling App::quit(). Avoids the macOS chain-quit footgun by waiting for keyup after the threshold, matching Chrome and OrbStack's pattern. https://claude.ai/code/session_01WddJfYskQCENcCNTu6KVpf
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds quit confirmation flow for a NativePHP desktop app by introducing a centralized ChangesQuit Confirmation Flow
Sequence DiagramsequenceDiagram
participant User
participant NativeMenu as Native Menu
participant Livewire as Livewire<br/>keepalive
participant QuitPrompt as quit-confirmation.js<br/>(Client)
participant QuitAction as QuitAppAction
participant NativeApp as NativePHP App
User->>NativeMenu: Click Quit (quit-rfa)
NativeMenu->>Livewire: MenuItemClicked event
Livewire->>QuitPrompt: Dispatch quit-prompt-show
QuitPrompt->>User: Show "Hold Cmd+Q to Quit" overlay
User->>QuitPrompt: Hold Cmd+Q
QuitPrompt->>QuitPrompt: Start hold timer
Note over QuitPrompt: If released before thresholdMs,<br/>cancel (no dispatch)
User->>QuitPrompt: Hold past thresholdMs + keyup
QuitPrompt->>Livewire: Dispatch quit-now event
Livewire->>QuitAction: invoke handle()
QuitAction->>NativeApp: App::quit()
NativeApp->>User: Exit app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Livewire/QuitConfirmationTest.php (1)
23-29: 💤 Low valueSame facade mocking note as
QuitAppActionTest.The
app()->instance(NativeApp::class, $mock)binding works if the NativePHP facade resolves to that class name. Otherwise, considerApp::shouldReceive('quit')->once()for a more idiomatic approach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Livewire/QuitConfirmationTest.php` around lines 23 - 29, The test uses app()->instance(NativeApp::class, $native) which may not intercept the NativePHP facade; replace the instance binding with a facade mock on the App facade (e.g., use App::shouldReceive('quit')->once() in the test) or alternatively ensure the NativePHP facade resolves to NativeApp::class before binding; update the test in QuitConfirmationTest to mock App::shouldReceive('quit')->once() instead of app()->instance(NativeApp::class, $native) so Livewire::test('quit-confirmation')->call('quit') exercises the facade as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/Livewire/QuitConfirmationTest.php`:
- Around line 23-29: The test uses app()->instance(NativeApp::class, $native)
which may not intercept the NativePHP facade; replace the instance binding with
a facade mock on the App facade (e.g., use App::shouldReceive('quit')->once() in
the test) or alternatively ensure the NativePHP facade resolves to
NativeApp::class before binding; update the test in QuitConfirmationTest to mock
App::shouldReceive('quit')->once() instead of app()->instance(NativeApp::class,
$native) so Livewire::test('quit-confirmation')->call('quit') exercises the
facade as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03c0aa05-7033-4b0f-89e0-d616c02665a8
📒 Files selected for processing (7)
app/Actions/QuitAppAction.phpapp/Providers/NativeAppServiceProvider.phpresources/views/layouts/app.blade.phpresources/views/livewire/quit-confirmation.blade.phptests/Unit/Actions/QuitAppActionTest.phptests/Unit/Livewire/QuitConfirmationTest.phptests/Unit/Providers/NativeAppServiceProviderTest.php
The standalone quit-confirmation Livewire SFC mounted a second child component on every page render, which added ~170ms (+130%) to the review-page-100-files benchmark and tripped the perf gate. Move the menu-id routing and the App::quit() call into the existing keepalive component (already in the layout), and convert the overlay to a pure Blade x- component. The Alpine state machine confirms quit by dispatching a `quit-now` Livewire event that keepalive listens for, so the layout no longer pays a per-page Livewire mount. https://claude.ai/code/session_01WddJfYskQCENcCNTu6KVpf
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Unit/Livewire/KeepaliveTest.php (1)
11-21: 💤 Low valueUse arrow-function syntax for the two simple one-liner tests.
The coding guidelines specify
test('lowercase description', fn() => ...). Tests 1 and 2 are single-chain expressions and fit the pattern; test 3 genuinely needs a closure (multi-statement Mockery setup).♻️ Proposed refactor
-test('the menu broadcast for quit-rfa fires the quit-prompt-show browser event', function () { - Livewire::test('keepalive') - ->dispatch('native:Native\\Desktop\\Events\\Menu\\MenuItemClicked', item: ['id' => 'quit-rfa']) - ->assertDispatched('quit-prompt-show'); -}); - -test('an unrelated menu broadcast does not fire the quit prompt', function () { - Livewire::test('keepalive') - ->dispatch('native:Native\\Desktop\\Events\\Menu\\MenuItemClicked', item: ['id' => 'open-repo']) - ->assertNotDispatched('quit-prompt-show'); -}); +test('dispatches quit-prompt-show on quit-rfa menu click', fn () => Livewire::test('keepalive') + ->dispatch('native:Native\\Desktop\\Events\\Menu\\MenuItemClicked', item: ['id' => 'quit-rfa']) + ->assertDispatched('quit-prompt-show')); + +test('does not dispatch quit-prompt-show for unrelated menu click', fn () => Livewire::test('keepalive') + ->dispatch('native:Native\\Desktop\\Events\\Menu\\MenuItemClicked', item: ['id' => 'open-repo']) + ->assertNotDispatched('quit-prompt-show'));As per coding guidelines, "Use arrow function style for test declarations" and "Use action-first test naming."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Livewire/KeepaliveTest.php` around lines 11 - 21, Replace the two one-liner tests that use full function closures with arrow-function syntax: change the first test declaration for "the menu broadcast for quit-rfa fires the quit-prompt-show browser event" and the second test declaration for "an unrelated menu broadcast does not fire the quit prompt" to use fn() => Livewire::test('keepalive')->dispatch(...)->assertDispatched('quit-prompt-show') and fn() => Livewire::test('keepalive')->dispatch(...)->assertNotDispatched('quit-prompt-show') respectively, keeping the same Livewire::test('keepalive')->dispatch('native:Native\\Desktop\\Events\\Menu\\MenuItemClicked', item: [...]) chains and assertions (assertDispatched / assertNotDispatched) but replacing the function() { ... } closures with arrow functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/views/components/quit-confirmation.blade.php`:
- Around line 26-36: The overlay currently starts holdTimer (which sets armed =
true) inside armTimers()/show(), so dismissTimer never cancels mouse-opened
overlays; change armTimers()/show() to only install the auto-dismiss
dismissTimer and remove the unconditional start of holdTimer there, and instead
start the holdTimer from the keyboard path (onKeydown when Meta+Q or Cmd+Q is
detected) so repeated macOS keydown events will arm the overlay; ensure the
keydown path uses the same holdTimer identifier (holdTimer) and clears it on
cancel/commit/onKeyup, and keep cancel() invoked by dismissTimer when armed is
still false to auto-close mouse-opened overlays.
---
Nitpick comments:
In `@tests/Unit/Livewire/KeepaliveTest.php`:
- Around line 11-21: Replace the two one-liner tests that use full function
closures with arrow-function syntax: change the first test declaration for "the
menu broadcast for quit-rfa fires the quit-prompt-show browser event" and the
second test declaration for "an unrelated menu broadcast does not fire the quit
prompt" to use fn() =>
Livewire::test('keepalive')->dispatch(...)->assertDispatched('quit-prompt-show')
and fn() =>
Livewire::test('keepalive')->dispatch(...)->assertNotDispatched('quit-prompt-show')
respectively, keeping the same
Livewire::test('keepalive')->dispatch('native:Native\\Desktop\\Events\\Menu\\MenuItemClicked',
item: [...]) chains and assertions (assertDispatched / assertNotDispatched) but
replacing the function() { ... } closures with arrow functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc740995-0e5d-4075-8b8d-7d73062708ad
📒 Files selected for processing (4)
resources/views/components/quit-confirmation.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/keepalive.blade.phptests/Unit/Livewire/KeepaliveTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/views/layouts/app.blade.php
| this.holdTimer = setTimeout(() => { | ||
| this.armed = true; | ||
| }, this.thresholdMs); | ||
|
|
||
| // Auto-dismiss when the user clicks Quit in the menu with a | ||
| // mouse and never follows up with Cmd+Q. | ||
| this.dismissTimer = setTimeout(() => { | ||
| if (! this.armed) { | ||
| this.cancel(); | ||
| } | ||
| }, this.autoDismissMs); |
There was a problem hiding this comment.
holdTimer arms the overlay before dismissTimer can dismiss it — mouse-click case gets stuck open and becomes a quit trap.
holdTimer fires at 1500 ms and sets armed = true. dismissTimer fires at 4000 ms and checks !this.armed — but by that point armed is already true, so cancel() is never called.
Practical consequence: when the overlay is opened via a mouse click on the menu item (not Cmd+Q):
- The overlay arms silently at t = 1.5 s with no user input.
dismissTimerfires at t = 4 s, evaluates!this.armed→false, and does nothing.- The overlay stays open indefinitely.
- Any subsequent Cmd+shortcut (e.g. Cmd+Z, Cmd+C) will release
Meta,onKeyupwill seearmed === true, andcommit()is invoked → unintended quit.
The simplest correct fix is to start holdTimer only from a keyboard event, keeping armTimers() for the auto-dismiss only:
🐛 Proposed fix
- show() {
- this.visible = true;
- this.armed = false;
- this.armTimers();
- },
- armTimers() {
- this.clearTimers();
-
- // Assume Cmd+Q is held the moment the menu accelerator fired
- // (it just did, that's how we got here). If the user releases
- // before the threshold, keyup cancels.
- this.holdTimer = setTimeout(() => {
- this.armed = true;
- }, this.thresholdMs);
-
- // Auto-dismiss when the user clicks Quit in the menu with a
- // mouse and never follows up with Cmd+Q.
- this.dismissTimer = setTimeout(() => {
- if (! this.armed) {
- this.cancel();
- }
- }, this.autoDismissMs);
- },
+ show() {
+ this.visible = true;
+ this.armed = false;
+ this.clearTimers();
+ // Auto-dismiss if no keyboard follow-up (mouse-click path).
+ this.dismissTimer = setTimeout(() => this.cancel(), this.autoDismissMs);
+ },Then start holdTimer from the first Cmd/Q keydown while visible (macOS key-repeat delivers repeated keydown events while the key is held, so the event arrives even when the overlay appears after the initial press):
onKeydown(event) {
if (event.key === 'Escape' && this.visible) {
event.preventDefault();
this.cancel();
+ return;
}
+ if (this.visible && ! this.holdTimer &&
+ (event.key === 'Meta' || event.key === 'Control' || event.key.toLowerCase() === 'q')) {
+ this.holdTimer = setTimeout(() => { this.armed = true; }, this.thresholdMs);
+ }
},This keeps the unconditional dismiss in show() so mouse-click overlays auto-close, while the keyboard hold path still arms naturally from the repeated keydown events macOS delivers while a key is held.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@resources/views/components/quit-confirmation.blade.php` around lines 26 - 36,
The overlay currently starts holdTimer (which sets armed = true) inside
armTimers()/show(), so dismissTimer never cancels mouse-opened overlays; change
armTimers()/show() to only install the auto-dismiss dismissTimer and remove the
unconditional start of holdTimer there, and instead start the holdTimer from the
keyboard path (onKeydown when Meta+Q or Cmd+Q is detected) so repeated macOS
keydown events will arm the overlay; ensure the keydown path uses the same
holdTimer identifier (holdTimer) and clears it on cancel/commit/onKeyup, and
keep cancel() invoked by dismissTimer when armed is still false to auto-close
mouse-opened overlays.
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Js/quit-confirmation.test.js`:
- Around line 48-62: The two tests are duplicates and the "unarmed" test is
wrong because the hold threshold fires before autoDismiss; fix by creating two
distinct scenarios: 1) For the "unarmed prompt" test, instantiate or configure
the controller with thresholdMs greater than autoDismissMs (so the hold timer
never arms) and then call controller.show() followed by
vi.advanceTimersByTime(autoDismissMs) to assert it hides and livewire.dispatch
not called; 2) For the "armed" test, use the default/previous thresholdMs, call
controller.show(), vi.advanceTimersByTime(thresholdMs + smallDelta) to arm it,
then advance to autoDismissMs and assert it hides and dispatch behavior; update
test descriptions accordingly and remove the duplicate body.
- Around line 40-62: Replace the synchronous timer advances with the async
variant so tests flush microtasks: in the tests using controller.show() and
vi.advanceTimersByTime(...) (three occurrences in quit-confirmation.test.js),
change each call to vi.advanceTimersByTimeAsync(...) and await the call so the
timers and promises are processed (i.e., use await
vi.advanceTimersByTimeAsync(1500) / await vi.advanceTimersByTimeAsync(4000)
where used near controller.show and the KeyboardEvent dispatch).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6dec39dc-ee76-4e4a-9c1f-1743ad80be35
📒 Files selected for processing (6)
app/Actions/QuitAppAction.phpapp/Providers/NativeAppServiceProvider.phppublic/js/quit-confirmation.jsresources/views/layouts/app.blade.phpresources/views/livewire/keepalive.blade.phptests/Js/quit-confirmation.test.js
✅ Files skipped from review due to trivial changes (1)
- app/Actions/QuitAppAction.php
| vi.advanceTimersByTime(1500); | ||
|
|
||
| window.dispatchEvent(new KeyboardEvent('keyup', { key: 'Meta' })); | ||
|
|
||
| expect(controller.isVisible()).toBe(false); | ||
| expect(livewire.dispatch).toHaveBeenCalledWith('quit-now'); | ||
| }); | ||
|
|
||
| it('dismisses an unarmed prompt after the auto-dismiss timeout', () => { | ||
| controller.show(); | ||
| vi.advanceTimersByTime(4000); | ||
|
|
||
| expect(controller.isVisible()).toBe(false); | ||
| expect(livewire.dispatch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('auto-dismisses even after the prompt is armed', () => { | ||
| controller.show(); | ||
| vi.advanceTimersByTime(4000); | ||
|
|
||
| expect(controller.isVisible()).toBe(false); | ||
| expect(livewire.dispatch).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Use vi.advanceTimersByTimeAsync() per the project's Vitest timer guideline
All three timer advances use the synchronous vi.advanceTimersByTime(). The async variant flushes microtasks between timer executions, preventing the promise/timer deadlock that causes hanging tests. The guideline for this repo explicitly requires vi.advanceTimersByTimeAsync() to avoid flakiness.
🔧 Proposed fix
- it('dispatches quit after the hold threshold and key release', () => {
+ it('dispatches quit after the hold threshold and key release', async () => {
controller.show();
- vi.advanceTimersByTime(1500);
+ await vi.advanceTimersByTimeAsync(1500);
window.dispatchEvent(new KeyboardEvent('keyup', { key: 'Meta' }));
expect(controller.isVisible()).toBe(false);
expect(livewire.dispatch).toHaveBeenCalledWith('quit-now');
});
- it('dismisses an unarmed prompt after the auto-dismiss timeout', () => {
+ it('dismisses an unarmed prompt after the auto-dismiss timeout', async () => {
controller.show();
- vi.advanceTimersByTime(4000);
+ await vi.advanceTimersByTimeAsync(4000);
expect(controller.isVisible()).toBe(false);
expect(livewire.dispatch).not.toHaveBeenCalled();
});
- it('auto-dismisses even after the prompt is armed', () => {
+ it('auto-dismisses even after the prompt is armed', async () => {
controller.show();
- vi.advanceTimersByTime(4000);
+ await vi.advanceTimersByTimeAsync(4000);As per coding guidelines: "Always use fake timers for setTimeout/setInterval logic in DOM tests using vi.useFakeTimers() and vi.advanceTimersByTimeAsync() to avoid flakiness."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Js/quit-confirmation.test.js` around lines 40 - 62, Replace the
synchronous timer advances with the async variant so tests flush microtasks: in
the tests using controller.show() and vi.advanceTimersByTime(...) (three
occurrences in quit-confirmation.test.js), change each call to
vi.advanceTimersByTimeAsync(...) and await the call so the timers and promises
are processed (i.e., use await vi.advanceTimersByTimeAsync(1500) / await
vi.advanceTimersByTimeAsync(4000) where used near controller.show and the
KeyboardEvent dispatch).
| it('dismisses an unarmed prompt after the auto-dismiss timeout', () => { | ||
| controller.show(); | ||
| vi.advanceTimersByTime(4000); | ||
|
|
||
| expect(controller.isVisible()).toBe(false); | ||
| expect(livewire.dispatch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('auto-dismisses even after the prompt is armed', () => { | ||
| controller.show(); | ||
| vi.advanceTimersByTime(4000); | ||
|
|
||
| expect(controller.isVisible()).toBe(false); | ||
| expect(livewire.dispatch).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Duplicate test bodies — first test's "unarmed" label is also incorrect
Both tests (lines 48–54 and 56–62) have identical bodies: controller.show() then advance 4000ms. With thresholdMs: 1500 and autoDismissMs: 4000, advancing 4000ms fires the hold timer at 1500ms (setting armed = true) and then fires the dismiss timer at 4000ms — so the prompt is armed in both cases. The "unarmed prompt" label on the first test is factually wrong, and neither test adds coverage the other doesn't already provide.
To actually cover the "unarmed dismiss" scenario, thresholdMs must exceed autoDismissMs so the hold timer never fires before the dismiss:
🔧 Proposed fix
- it('dismisses an unarmed prompt after the auto-dismiss timeout', () => {
- controller.show();
- vi.advanceTimersByTime(4000);
-
- expect(controller.isVisible()).toBe(false);
- expect(livewire.dispatch).not.toHaveBeenCalled();
- });
-
- it('auto-dismisses even after the prompt is armed', () => {
- controller.show();
- vi.advanceTimersByTime(4000);
-
- expect(controller.isVisible()).toBe(false);
- expect(livewire.dispatch).not.toHaveBeenCalled();
- });
+ it('dismisses without quit when auto-dismissed before the hold threshold', async () => {
+ // Use a controller where the dismiss fires before arming is possible
+ const fastDismiss = createQuitConfirmation({
+ window, document, livewire,
+ thresholdMs: 9000, // never arms within the dismiss window
+ autoDismissMs: 4000,
+ });
+ fastDismiss.attach();
+ fastDismiss.show();
+ await vi.advanceTimersByTimeAsync(4000);
+
+ expect(fastDismiss.isArmed()).toBe(false);
+ expect(fastDismiss.isVisible()).toBe(false);
+ expect(livewire.dispatch).not.toHaveBeenCalled();
+ fastDismiss.detach();
+ });
+
+ it('auto-dismisses without quit even after the prompt is armed', async () => {
+ controller.show();
+ await vi.advanceTimersByTimeAsync(1500); // arm it
+ expect(controller.isArmed()).toBe(true);
+ await vi.advanceTimersByTimeAsync(2500); // reach autoDismiss at 4000ms total
+
+ expect(controller.isVisible()).toBe(false);
+ expect(livewire.dispatch).not.toHaveBeenCalled();
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Js/quit-confirmation.test.js` around lines 48 - 62, The two tests are
duplicates and the "unarmed" test is wrong because the hold threshold fires
before autoDismiss; fix by creating two distinct scenarios: 1) For the "unarmed
prompt" test, instantiate or configure the controller with thresholdMs greater
than autoDismissMs (so the hold timer never arms) and then call
controller.show() followed by vi.advanceTimersByTime(autoDismissMs) to assert it
hides and livewire.dispatch not called; 2) For the "armed" test, use the
default/previous thresholdMs, call controller.show(),
vi.advanceTimersByTime(thresholdMs + smallDelta) to arm it, then advance to
autoDismissMs and assert it hides and dispatch behavior; update test
descriptions accordingly and remove the duplicate body.
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
Replaces the native Menu::quit() role with a labeled menu item so Cmd+Q
fires MenuItemClicked, then routes the press through a Livewire overlay
that requires a sustained 1.5s hold before calling App::quit(). Avoids
the macOS chain-quit footgun by waiting for keyup after the threshold,
matching Chrome and OrbStack's pattern.
https://claude.ai/code/session_01WddJfYskQCENcCNTu6KVpf
Summary by CodeRabbit