Fix zoom shortcuts on macOS by routing through renderer keymap#75
Merged
Fix zoom shortcuts on macOS by routing through renderer keymap#75
Conversation
⌘- silently failed because Electron 38 registers the zoomOut role's keyEquivalent on macOS but never fires the role's webContentsMethod from the keystroke (electron/electron#19559, #15496). The previous fix tried overriding the accelerator on a normal-typed item, but Electron still treats the item as role-based once `role` is set, so the override doesn't help — the keystroke is consumed and dropped. Drop both `role` and `accelerator` from the View-menu zoom items so the keystrokes flow through to the renderer, where a new public/js handler dispatches `rfa-zoom` to a Livewire component (keepalive). The component delegates to ZoomWindowAction, which clamps, persists the factor, and calls Window::zoomFactor — restoring the cached factor on next launch. Mouse clicks on the menu items reach the same handler via the existing native MenuItemClicked bridge.
Spamming ⌘- at MIN (or ⌘+ at MAX, or ⌘0 at 1.0) wrote the same factor
to cache and posted to Electron's HTTP server every keystroke. Read the
current factor once, return early when the clamped target equals it.
Drop a few redundancies the cleanup turned up:
- In-renderer keepalive doesn't need to re-validate the direction; the
action already throws on unknown values.
- createWindow now reads the cached factor through ZoomWindowAction
instead of touching the cache key directly.
- Pest test names dropped the handle("in") syntax in favour of
behaviour-first phrasing.
- tests/Js/zoom-shortcuts removed unused vitest imports.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR refactors zoom handling from role-based menu items to an event-driven architecture with cached zoom state. Global shortcuts are registered/unregistered based on window focus, triggering events that invoke a unified Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GlobalShortcut as GlobalShortcut Facade
participant Event as ZoomShortcutPressed Event
participant Listener as HandleZoomShortcutPressed
participant Action as ZoomWindowAction
participant Cache as Cache
participant Window as Window (Electron)
User->>GlobalShortcut: Press Cmd+Plus (zoom-in)
GlobalShortcut->>Event: Emit ZoomShortcutPressed
Event->>Listener: handle(event)
Listener->>Listener: event.direction() → "in"
Listener->>Action: handle("in")
Action->>Cache: Get current zoom factor
Action->>Action: Compute next = current + STEP<br/>Clamp to MIN/MAX
alt Clamped ≠ Current
Action->>Cache: Set new factor (30-day TTL)
Action->>Window: zoomFactor(clamped)
else Clamped = Current
Action->>Action: No-op, return current
end
Action-->>Listener: Return updated zoom
Listener-->>User: Zoom applied
sequenceDiagram
participant OS as Operating System
participant Window as Main Window
participant EventBus as Event Bus
participant Register as RegisterZoomGlobalShortcuts
participant Unregister as UnregisterZoomGlobalShortcuts
participant GlobalShortcut as GlobalShortcut Facade
Window->>EventBus: WindowFocused('main')
EventBus->>Register: handle(event)
Register->>Register: Check window id == 'main'
Register->>GlobalShortcut: Register all zoom keys<br/>(Cmd+Plus, Cmd+Minus, Cmd+0)
GlobalShortcut->>OS: Listen for shortcuts
OS->>GlobalShortcut: Shortcut pressed
GlobalShortcut->>EventBus: Emit ZoomShortcutPressed
Window->>EventBus: WindowBlurred('main')
EventBus->>Unregister: handle(event)
Unregister->>Unregister: Check window id == 'main'
Unregister->>GlobalShortcut: Unregister all zoom keys
GlobalShortcut->>OS: Stop listening for shortcuts
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. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c12683db84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+41
to
+43
| keymap.register('⌘=', dispatch(root, 'in')); | ||
| keymap.register('⌘-', dispatch(root, 'out')); | ||
| keymap.register('⌘0', dispatch(root, 'reset')); |
There was a problem hiding this comment.
These registrations omit allowInEditable, so keymap-store keeps its default (false) and skips the handlers when focus is inside <input>, <textarea>, or contenteditable elements. Since NativeAppServiceProvider::createMenu now removes role/accelerator from the zoom menu items, there is no menu-level keyboard fallback, so pressing zoom shortcuts while typing will not route through ZoomWindowAction (and the persisted zoom path is bypassed).
Useful? React with 👍 / 👎.
| const keymap = root.Alpine?.store?.('keymap'); | ||
| if (!keymap) return false; | ||
|
|
||
| keymap.register('⌘=', dispatch(root, 'in')); |
There was a problem hiding this comment.
The zoom-in shortcut is registered as ⌘=, but the standard zoom-in combo is ⌘+. With this keymap matcher, ⌘+ is a shifted keypress (shiftKey=true, key='+') and will not match the unshifted ⌘= binding, so users who press the expected ⌘+ combination will not trigger zoom-in.
Useful? React with 👍 / 👎.
Two gaps from the renderer-keymap zoom path:
- Without `allowInEditable`, the keymap-store skips the handler when
focus is in an <input>/<textarea>/contenteditable. There is no
menu-level fallback now that the View-menu items don't carry an
accelerator, so pressing ⌘- while typing in a comment was a dead
keystroke.
- The matcher checks shift state strictly. `⌘+` on US layouts is
Cmd+Shift+= (`shiftKey=true, key='+'`), so `⌘=` alone never caught
the canonical "Cmd plus" combo. Bind both `⌘=` and `⌘⇧+` to zoom-in,
matching Chrome/Safari.
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the broken Electron 38 role-based zoom accelerators with a custom action and renderer-side keymap bindings. On macOS, Electron 38's
zoomIn/zoomOut/resetZoomroles register menu accelerators but fail to fire thewebContentsMethodwhen the keystroke is pressed (only mouse clicks work). This change bypasses the role system entirely by routing zoom adjustments through a Livewire component that owns the persisted zoom state.Key Changes
ZoomWindowAction: Single source of truth for zoom adjustments with caching, clamping (0.5–3.0), and floating-point rounding to prevent driftzoom-shortcuts.js): Registers ⌘=, ⌘-, and ⌘0 to dispatchrfa-zoomevents to the keepalive Livewire componentZoomRoleMenuItemwith plain labeled menu items (no role, no accelerator) so keystrokes fall through to the renderer keymap instead of being consumed by Electron's role system#[On('rfa-zoom')]listener to handle both keyboard and menu-click zoom requestsImplementation Details
livewire:navigatedevent (cleared by keymap-store on navigation)https://claude.ai/code/session_01CBuZ56PLvqwDjPDKK9CGYu
Summary by CodeRabbit
Release Notes
New Features
Improvements