Repository navigation
Conversation
On Swedish (and other non-US ISO) layouts the "+" key sits at the US "-"
physical position (keyCode 27). Pressing Cmd++ in a focused terminal pane
fired the zoom-out action because:
1. Ghostty's internal keybind matcher uses US-layout keycodes, so when
cmux forwarded the raw NSEvent it resolved Cmd++ to Cmd+- (decrease
font size). Fixed by dispatching the action via
`ghostty_surface_binding_action` based on the result of the existing
layout-aware `browserZoomShortcutAction` helper rather than handing
the raw event to Ghostty.
2. The Swift-side `KeyboardShortcut.matches` ANSI-keycode fallback fired
unconditionally for symbol command shortcuts, masking a typed ASCII
character that simply did not match. Now the same character-match
gate already used for letter shortcuts also applies to symbols.
3. `normalizedShortcutEventCharacter` only mapped "+" to "=" (and "_" to
"-") when shift was held. On layouts that produce "+" without shift
the existing zoom-in binding ("=") never matched. Those two
mappings are now unconditional; no shortcut binds to literal "+" or
"_" so there is no conflict.
Adds KeyboardShortcutLayoutTests covering Swedish ⌘+, Russian ⌘W
(non-Latin layout keycode fallback), and US baselines.
Fixes manaflow-ai#3362
Refs manaflow-ai#1456
|
@OneMuppet is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes keyboard shortcut matching for alternative keyboard layouts (addressing issue ChangesKeyboard Layout Awareness for Shortcuts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes terminal zoom misfiring on non-US ISO keyboard layouts (e.g. Swedish, where
Confidence Score: 4/5Safe to merge for the documented Swedish/ISO layout fix; two minor edge cases around numpad+ shortcut storage and residual US-keycode OR-branches in browserZoomShortcutAction are worth addressing but do not affect the corrected zoom paths. The three-layer fix correctly handles Swedish cmd+/cmd-, preserves non-Latin (Russian) cmd+W fallback, and keeps US baselines intact. The one concrete gap is that numpad+ (keyCode 69) is absent from the storedKey switch, so a user who records a binding from numpad+ stores literal '+' as the key, and the new unconditional '+' to '=' normalization causes that binding to silently miss. Sources/KeyboardShortcutSettings.swift — storedKey switch and normalizedShortcutEventCharacter; Sources/AppDelegate.swift — browserZoomShortcutAction keyCode OR-branch documentation. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let zoomAction = browserZoomShortcutAction( | ||
| flags: event.modifierFlags, | ||
| chars: event.charactersIgnoringModifiers ?? "", | ||
| keyCode: event.keyCode, | ||
| literalChars: event.characters | ||
| ) { | ||
| ghosttyView.keyDown(with: event) | ||
| let bindingActionName: String | ||
| switch zoomAction { | ||
| case .zoomIn: bindingActionName = "increase_font_size:1" | ||
| case .zoomOut: bindingActionName = "decrease_font_size:1" | ||
| case .reset: bindingActionName = "reset_font_size" | ||
| } | ||
| _ = ghosttyView.performBindingAction(bindingActionName) | ||
| #if DEBUG | ||
| cmuxDebugLog("zoom.shortcut stage=window.ghosttyKeyDownDirect event=\(Self.keyDescription(event)) handled=1") | ||
| cmuxDebugLog("zoom.shortcut stage=window.ghosttyBindingActionDirect action=\(bindingActionName) event=\(Self.keyDescription(event)) handled=1") | ||
| #endif | ||
| return true | ||
| } |
There was a problem hiding this comment.
browserZoomShortcutAction "layout-aware," but the function still has || keyCode == 24 and || keyCode == 27 as unconditional OR conditions alongside character-based detection (in ShortcutRoutingSupport.swift). For a layout whose key at keyCode 27 produces an ASCII character that is neither "-" nor "_", keyCode == 27 still returns .zoomOut and performBindingAction("decrease_font_size:1") is called. The effective behavior matches the old keyDown forwarding path (Ghostty also used US keycodes), so this is not a regression — but documenting the known limitation directly on browserZoomShortcutAction would clarify the invariant for future maintainers.
|
Thank you for this, and for the thorough layout tests! Part of it is already on main: The rest no longer applies cleanly (the AppDelegate routing has moved), and one piece worries me: dropping the keycode fallback for all non-letter Command shortcuts. That fallback is what keeps shortcuts like Cmd+[ reachable on layouts where that physical key types a different ASCII character (French AZERTY, for example), so removing it everywhere could break those users. If Cmd+"+" still zooms out in the terminal on a Swedish layout with a current nightly, a rebased PR with just the terminal part (dispatching |
teamleaderleo
added
S3: minor
This branch has not been deployed
2 participants
Summary
On Swedish (and other non-US ISO) layouts, pressing ⌘+ in a focused terminal pane fires zoom-out instead of zoom-in. The
+key on Swedish sits at the physical position of US-(keyCode 27), so US-keycode-based matchers route the keystroke to the wrong action.This PR fixes it at three layers:
AppDelegate.swift— main fix. When a zoom shortcut is detected and the terminal owns first responder, cmux was forwarding the rawNSEventto Ghostty. Ghostty's internal keybind matcher then resolved the event by US-layout keycode, mis-firing decrease_font_size on Swedish ⌘+. Instead, dispatch the action by name viaghostty_surface_binding_actionusing the result of the already-layout-awarebrowserZoomShortcutActionhelper.KeyboardShortcutSettings.swift—matches. The ANSI-keycode fallback fired unconditionally for symbol command shortcuts, masking the typed ASCII character even when it clearly didn't match the bound shortcut key. Letter shortcuts already required a character match in that case; symbol shortcuts now do too. Non-Latin layouts still fall back to US keycodes (hasEventChars && !eventCharsAreASCII) so ⌘W remains reachable on Cyrillic/etc.KeyboardShortcutSettings.swift—normalizedShortcutEventCharacter."+"→"="and"_"→"-"were gated onapplyShiftSymbolNormalization. On Swedish (and similar ISO layouts)+is typed without shift, so the gate left the typed"+"un-mapped and Cmd++could not match the zoom-in binding stored as"=". These two mappings are now unconditional; nothing binds to literal+/_so there is no conflict.Test plan
KeyboardShortcutLayoutTests(9 tests, all passing):.browserZoomInand does not match.browserZoomOut.browserZoomOut+), ⌘− baselines preservedFixes #3362
Refs #1456
🤖 Generated with Claude Code
Summary by cubic
Fixes terminal zoom shortcuts on non‑US ISO layouts (e.g., Swedish). Cmd + now zooms in (not out) when a terminal pane is focused.
browserZoomShortcutAction, instead of forwarding rawNSEvent.KeyboardShortcutLayoutTestscovering Swedish ⌘+, US baselines, and Russian ⌘W.Written for commit 0804909. Summary will update on new commits. Review in cubic
Summary by CodeRabbit