Repository navigation
Conversation
|
@anthhub is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughUpdated Command-key shortcut handling in AppDelegate to normalize keyboard characters through KeyboardLayout when the original Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 |
Greptile SummaryThis PR fixes a bug where keyboard shortcuts (e.g. Cmd+T, Cmd+W) would fail to trigger NSMenu items when a non-Latin IME such as Korean or Russian is active, because Changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant W as NSWindow
participant AD as AppDelegate (cmux_performKeyEquivalent)
participant KL as KeyboardLayout
participant M as NSApp.mainMenu
W->>AD: performKeyEquivalent(event)
AD->>AD: firstResponderGhosttyView != nil &&\nshouldRouteCommandEquivalentDirectlyToMainMenu?
alt non-ASCII charactersIgnoringModifiers
AD->>KL: character(forKeyCode: event.keyCode)
KL-->>AD: normalized ASCII char (e.g. "t")
AD->>AD: synthesize NSEvent with\ncharactersIgnoringModifiers = "t"\n(modifierFlags preserved)
end
AD->>M: performKeyEquivalent(menuEvent)
M-->>AD: consumed: Bool
alt consumed
AD-->>W: true (shortcut fired)
else not consumed
AD->>AD: cmux_performKeyEquivalent(event)
AD-->>W: result
end
Reviews (1): Last reviewed commit: "Normalize keyboard event for menu dispat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Handled here to prevent AppKit's default NSDocumentController from opening | ||
| // the Documents folder when SwiftUI menu dispatch fails due to focus bugs. | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .openFolder)) { | ||
| showOpenFolderPanel() |
There was a problem hiding this comment.
showOpenFolderPanel() implementation
showOpenFolderPanel() is called here but is not defined anywhere in the codebase (checked all of Sources/, vendor directories, and the base branch). This will produce a Swift compilation error:
error: use of unresolved identifier 'showOpenFolderPanel'
The Open Folder: Cmd+O block is a valid addition (preventing NSDocumentController from hijacking the shortcut), but the implementation for showOpenFolderPanel() needs to be provided. Based on the existing openFolderURL(baseWebUIURL:directoryPath:) helper in the same file, this function presumably should open an NSOpenPanel configured for directory selection and dispatch the chosen path to the active workspace.
| menuEvent = NSEvent.keyEvent( | ||
| with: event.type, | ||
| location: event.locationInWindow, | ||
| modifierFlags: event.modifierFlags, | ||
| timestamp: event.timestamp, | ||
| windowNumber: event.windowNumber, | ||
| context: nil, | ||
| characters: normalized, | ||
| charactersIgnoringModifiers: normalized, | ||
| isARepeat: event.isARepeat, | ||
| keyCode: event.keyCode | ||
| ) ?? event |
There was a problem hiding this comment.
characters field mirrors charactersIgnoringModifiers in synthetic event
Both characters and charactersIgnoringModifiers are set to the same lowercase ASCII string (normalized, e.g. "t"). For a real Cmd+T key press, characters typically carries the control-character form (e.g. "\u{14}") while charactersIgnoringModifiers is "t".
This is harmless for NSMenu.performKeyEquivalent, which matches solely on charactersIgnoringModifiers + modifierFlags. However, if any downstream menu-item action handler ever inspects event.characters (rare but possible), it would receive "t" instead of the expected control character.
A safer alternative is to preserve the original characters value and only override charactersIgnoringModifiers:
| menuEvent = NSEvent.keyEvent( | |
| with: event.type, | |
| location: event.locationInWindow, | |
| modifierFlags: event.modifierFlags, | |
| timestamp: event.timestamp, | |
| windowNumber: event.windowNumber, | |
| context: nil, | |
| characters: normalized, | |
| charactersIgnoringModifiers: normalized, | |
| isARepeat: event.isARepeat, | |
| keyCode: event.keyCode | |
| ) ?? event | |
| menuEvent = NSEvent.keyEvent( | |
| with: event.type, | |
| location: event.locationInWindow, | |
| modifierFlags: event.modifierFlags, | |
| timestamp: event.timestamp, | |
| windowNumber: event.windowNumber, | |
| context: nil, | |
| characters: event.characters ?? normalized, | |
| charactersIgnoringModifiers: normalized, | |
| isARepeat: event.isARepeat, | |
| keyCode: event.keyCode | |
| ) ?? event |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 12480-12494: The code that synthesizes menuEvent when
event.charactersIgnoringModifiers is non-ASCII uses
KeyboardLayout.character(forKeyCode:) but drops modifier flags, stripping Shift
and breaking shifted shortcuts; update the call that builds the normalized
menuEvent to pass event.modifierFlags.intersection([.shift]) (or equivalent) as
the modifierFlags used when resolving/normalizing the character so
Shift-produced glyphs are preserved (affecting the branch that constructs
menuEvent via NSEvent.keyEvent and the use of
KeyboardLayout.character(forKeyCode:)).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/GhosttyConfigTests.swift">
<violation number="1" location="cmuxTests/GhosttyConfigTests.swift:2090">
P2: Multi-language CJK mapping test was weakened from range-level routing assertions to font-presence checks, reducing ability to catch incorrect Unicode range assignment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmuxTests/GhosttyConfigTests.swift (1)
2066-2067: Consider asserting the Hangul Jamo range too for fuller Korean coverage.You already assert
U+AC00-U+D7AF; addingU+1100-U+11FFwould catch partial Korean mapping regressions.Suggested test addition
let ranges = mappings!.map { $0.0 } XCTAssertTrue(ranges.contains("U+AC00-U+D7AF")) + XCTAssertTrue(ranges.contains("U+1100-U+11FF"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 2066 - 2067, The test currently checks for the Hangul syllables range via ranges derived from mappings in GhosttyConfigTests (the let ranges = mappings!.map { $0.0 } and XCTAssertTrue(ranges.contains("U+AC00-U+D7AF")) lines); add an additional assertion to also verify the Hangul Jamo range is present by asserting ranges.contains("U+1100-U+11FF") (place the new XCTAssertTrue immediately alongside the existing Hangul syllables assertion).Sources/AppDelegate.swift (1)
9312-9318: Consider routing the File menu through the same open-folder helper.
Cmd+Onow usesshowOpenFolderPanel(), butSources/cmuxApp.swift:589-605still appears to own a separateNSOpenPanelflow. Sharing one helper would keep panel configuration and workspace/window targeting from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 9312 - 9318, Cmd+O handling in AppDelegate uses showOpenFolderPanel(), but cmuxApp.swift still contains an independent NSOpenPanel flow (around lines 589-605); unify them by removing the duplicate NSOpenPanel logic in cmuxApp.swift and either call AppDelegate.showOpenFolderPanel() from the menu handler or extract the panel/configuration into a single shared helper that both use, ensuring the same panel configuration and workspace/window targeting are applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 2061-2067: The cjkFontMappings implementation currently skips
Korean language codes causing tests like
testCJKFontMappingsReturnsKoreanMappings and
testCJKFontMappingsReturnsKoreanMappingsWithDetailedLanguageTag to fail; update
GhosttyApp.cjkFontMappings to handle language tags starting with "ko" by setting
font = "Apple SD Gothic Neo" and langRanges = koreanRanges (the existing
koreanRanges constant) in the same style as the existing branches for "ja",
"zh-hant"/"tw"/"hk", and "zh", ensuring both simple "ko" and detailed tags like
"ko-KR" are handled.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 2066-2067: The test currently checks for the Hangul syllables
range via ranges derived from mappings in GhosttyConfigTests (the let ranges =
mappings!.map { $0.0 } and XCTAssertTrue(ranges.contains("U+AC00-U+D7AF"))
lines); add an additional assertion to also verify the Hangul Jamo range is
present by asserting ranges.contains("U+1100-U+11FF") (place the new
XCTAssertTrue immediately alongside the existing Hangul syllables assertion).
In `@Sources/AppDelegate.swift`:
- Around line 9312-9318: Cmd+O handling in AppDelegate uses
showOpenFolderPanel(), but cmuxApp.swift still contains an independent
NSOpenPanel flow (around lines 589-605); unify them by removing the duplicate
NSOpenPanel logic in cmuxApp.swift and either call
AppDelegate.showOpenFolderPanel() from the menu handler or extract the
panel/configuration into a single shared helper that both use, ensuring the same
panel configuration and workspace/window targeting are applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ada2fb6-b311-4732-86d0-9579724ef2e9
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/GhosttyConfigTests.swift
| func testCJKFontMappingsReturnsKoreanMappings() { | ||
| let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ko-KR"]) | ||
| XCTAssertNotNil(mappings) | ||
| let fonts = Set(mappings!.map { $0.1 }) | ||
| XCTAssertTrue(fonts.contains("Apple SD Gothic Neo")) | ||
| let ranges = mappings!.map { $0.0 } | ||
| XCTAssertTrue(ranges.contains("U+AC00-U+D7AF")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'GhosttyTerminalView.swift' | head -n1)"
if [ -z "${file:-}" ]; then
echo "GhosttyTerminalView.swift not found"
exit 1
fi
echo "Inspecting ${file}"
rg -n -C4 'static func cjkFontMappings|hasPrefix\("ko"|Apple SD Gothic Neo|koreanRanges|U\+AC00-U\+D7AF|U\+1100-U\+11FF' "${file}"Repository: manaflow-ai/cmux
Length of output: 932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'GhosttyTerminalView.swift' | head -n1)"
if [ -z "${file:-}" ]; then
echo "GhosttyTerminalView.swift not found"
exit 1
fi
# Extract the complete cjkFontMappings function (estimate ~100 lines from start)
echo "=== Full cjkFontMappings function implementation ==="
sed -n '1415,1550p' "${file}" | cat -nRepository: manaflow-ai/cmux
Length of output: 6615
Add Korean language support to cjkFontMappings to match test expectations.
The test expects Korean mappings for preferredLanguages: ["ko-KR"], but the production implementation lacks a Korean branch. The function currently handles only Japanese (ja), Traditional Chinese (zh-hant/tw/hk), and Simplified Chinese (zh). Korean language codes (ko*) fall through to else { continue } and return nil.
While koreanRanges is defined (with the exact ranges the test expects), the cjkFontMappings function must add:
else if lower.hasPrefix("ko") {
font = "Apple SD Gothic Neo"
langRanges = koreanRanges
}This applies to both test methods: testCJKFontMappingsReturnsKoreanMappings() (Line 2061) and testCJKFontMappingsReturnsKoreanMappingsWithDetailedLanguageTag() (Line 2086).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/GhosttyConfigTests.swift` around lines 2061 - 2067, The
cjkFontMappings implementation currently skips Korean language codes causing
tests like testCJKFontMappingsReturnsKoreanMappings and
testCJKFontMappingsReturnsKoreanMappingsWithDetailedLanguageTag to fail; update
GhosttyApp.cjkFontMappings to handle language tags starting with "ko" by setting
font = "Apple SD Gothic Neo" and langRanges = koreanRanges (the existing
koreanRanges constant) in the same style as the existing branches for "ja",
"zh-hant"/"tw"/"hk", and "zh", ensuring both simple "ko" and detailed tags like
"ko-KR" are handled.
When Korean or Russian IME is active, event.charactersIgnoringModifiers returns non-ASCII characters (e.g., "ㅅ" instead of "t"), causing NSMenu.performKeyEquivalent to fail matching ASCII-based shortcuts. Now we synthesize a normalized event with ASCII characters derived from the physical key code before dispatching to the main menu. Fixes manaflow-ai#1945 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lization Address review feedback: 1. Pass modifierFlags to KeyboardLayout.character(forKeyCode:) so Shift combinations (Cmd+Shift+[, Cmd+?) work under non-Latin IME 2. Preserve original event.characters instead of overwriting with normalized ASCII to avoid confusing downstream consumers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Korean (Hangul) characters were excluded from the automatic CJK font mapping, falling back to Ghostty's native CTFontCreateForString which ignores user font-family settings. Now Korean language preferences map Hangul ranges to Apple SD Gothic Neo via font-codepoint-map. Fixes manaflow-ai#1946 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
af62c01 to
60a3f38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 12613-12628: The new normalization branch for menuEvent can
produce an empty-string key equivalent and bypass the raw-event fallback; update
the if-condition around charactersIgnoringModifiers/normalized so you only
replace the event when normalized is non-nil and not empty (e.g. guard
normalized?.isEmpty == false), and treat empty string the same as nil so
performKeyEquivalent still falls back to the original/raw event; reference the
menuEvent and event variables and the call to
KeyboardLayout.character(forKeyCode:modifierFlags:) to locate and adjust the
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3916342c-e160-46fd-8f4f-6b61b70ca842
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/GhosttyConfigTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/GhosttyConfigTests.swift
| var menuEvent = event | ||
| if let chars = event.charactersIgnoringModifiers, | ||
| !chars.allSatisfy({ $0.isASCII }), | ||
| let normalized = KeyboardLayout.character(forKeyCode: event.keyCode, modifierFlags: event.modifierFlags) { | ||
| menuEvent = NSEvent.keyEvent( | ||
| with: event.type, | ||
| location: event.locationInWindow, | ||
| modifierFlags: event.modifierFlags, | ||
| timestamp: event.timestamp, | ||
| windowNumber: event.windowNumber, | ||
| context: nil, | ||
| characters: event.characters ?? normalized, | ||
| charactersIgnoringModifiers: normalized, | ||
| isARepeat: event.isARepeat, | ||
| keyCode: event.keyCode | ||
| ) ?? event |
There was a problem hiding this comment.
Cover empty-string key equivalents in the new normalization path.
This branch only normalizes non-ASCII charactersIgnoringModifiers, but the same file already handles synthetic key-equivalent paths that can arrive with nil/"". It also accepts KeyboardLayout.character(...) == "", while other call sites treat that as unusable. In either case you can end up handing performKeyEquivalent an event with no usable key equivalent and skipping the raw-event fallback.
🔧 Suggested change
var menuEvent = event
- if let chars = event.charactersIgnoringModifiers,
- !chars.allSatisfy({ $0.isASCII }),
- let normalized = KeyboardLayout.character(forKeyCode: event.keyCode, modifierFlags: event.modifierFlags) {
+ let rawChars = event.charactersIgnoringModifiers ?? ""
+ if (rawChars.isEmpty || !rawChars.allSatisfy(\.isASCII)),
+ let normalized = KeyboardLayout.character(forKeyCode: event.keyCode, modifierFlags: event.modifierFlags),
+ !normalized.isEmpty {
menuEvent = NSEvent.keyEvent(
with: event.type,
location: event.locationInWindow,
modifierFlags: event.modifierFlags,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 12613 - 12628, The new normalization
branch for menuEvent can produce an empty-string key equivalent and bypass the
raw-event fallback; update the if-condition around
charactersIgnoringModifiers/normalized so you only replace the event when
normalized is non-nil and not empty (e.g. guard normalized?.isEmpty == false),
and treat empty string the same as nil so performKeyEquivalent still falls back
to the original/raw event; reference the menuEvent and event variables and the
call to KeyboardLayout.character(forKeyCode:modifierFlags:) to locate and adjust
the logic.
- Add !normalized.isEmpty guard so performKeyEquivalent falls back to the original event when KeyboardLayout returns an empty string - Also normalize when charactersIgnoringModifiers is nil/empty (e.g. synthetic browser key equivalents), not just when non-ASCII Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 12615-12616: The code currently reconstructs
charactersIgnoringModifiers using the full event.modifierFlags which can include
Command/Option/Control and thus produces incorrect characters for non‑Latin
input methods; update the call to
KeyboardLayout.character(forKeyCode:modifierFlags:) and any reconstruction of
charactersIgnoringModifiers to pass only the Shift flag from event.modifierFlags
(e.g., derive a shiftOnlyFlags value via
event.modifierFlags.intersection(.shift) and use that) so rawChars,
KeyboardLayout.character(forKeyCode:modifierFlags:), and the
charactersIgnoringModifiers logic all use shift-only modifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (rawChars.isEmpty || !rawChars.allSatisfy(\.isASCII)), | ||
| let normalized = KeyboardLayout.character(forKeyCode: event.keyCode, modifierFlags: event.modifierFlags), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to Apple AppKit documentation, does NSEvent.charactersIgnoringModifiers ignore Command, Option, and Control while preserving only Shift?
💡 Result:
Yes, according to the official Apple AppKit documentation, NSEvent.charactersIgnoringModifiers ignores all modifier keys except Shift, preserving the Shift-modified character while ignoring Command (⌘), Option (⌥/Alt), and Control. The documentation states: "The characters generated by a key event as if no modifier key (except for Shift) applies." Examples include Option-e returning "e" (ignoring Option), and it is used alongside modifierFlags to detect combinations like Alt-S by checking for "s" and NSAlternateKeyMask.
Citations:
- 1: https://developer.apple.com/documentation/appkit/nsevent/charactersignoringmodifiers
- 2: https://developer.apple.com/documentation/appkit/nsevent/charactersignoringmodifiers?language=objc
- 3: https://developer.apple.com/documentation/appkit/nsevent/charactersignoringmodifiers?changes=_6
- 4: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/EventOverview/HandlingKeyEvents/HandlingKeyEvents.html
Use Shift-only modifiers when rebuilding charactersIgnoringModifiers.
Line 12616 derives charactersIgnoringModifiers with the full event.modifierFlags. According to Apple AppKit documentation, NSEvent.charactersIgnoringModifiers ignores all modifier keys except Shift. Passing Command, Option, and Control flags to KeyboardLayout.character() will synthesize the wrong character for those key combinations under non-Latin input methods.
Suggested fix
- if (rawChars.isEmpty || !rawChars.allSatisfy(\.isASCII)),
- let normalized = KeyboardLayout.character(forKeyCode: event.keyCode, modifierFlags: event.modifierFlags),
+ if (rawChars.isEmpty || !rawChars.allSatisfy(\.isASCII)),
+ let normalized = KeyboardLayout.character(
+ forKeyCode: event.keyCode,
+ modifierFlags: event.modifierFlags.intersection([.shift])
+ ),
!normalized.isEmpty {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 12615 - 12616, The code currently
reconstructs charactersIgnoringModifiers using the full event.modifierFlags
which can include Command/Option/Control and thus produces incorrect characters
for non‑Latin input methods; update the call to
KeyboardLayout.character(forKeyCode:modifierFlags:) and any reconstruction of
charactersIgnoringModifiers to pass only the Shift flag from event.modifierFlags
(e.g., derive a shiftOnlyFlags value via
event.modifierFlags.intersection(.shift) and use that) so rawChars,
KeyboardLayout.character(forKeyCode:modifierFlags:), and the
charactersIgnoringModifiers logic all use shift-only modifiers.
|
This is covered by the later CJK shortcut fix in #1649, which is on main. Closing as superseded. |
2 participants
Summary
event.charactersIgnoringModifiersreturns non-ASCII characters (e.g. "ㅅ" instead of "t")handleCustomShortcutalready handles this viaKeyboardLayout.normalizedCharactersmulti-layer fallbackmainMenu.performKeyEquivalent(with: event)incmux_performKeyEquivalentuses the raw event, so AppKit's NSMenu fails to match ASCII-based keyboard shortcutscharactersIgnoringModifiersderived from the physical key code before dispatching to the menuFixes #1945
Test plan
🤖 Generated with Claude Code
Summary by cubic
Fixes command shortcuts under non‑Latin IMEs by normalizing menu key events, including Shift combos. Also adds Korean Hangul font fallback and routes Cmd+O to Open Folder to avoid AppKit’s default.
NSMenu.performKeyEquivalent, synthesize anNSEventwith ASCIIcharactersIgnoringModifiersfrom the physicalkeyCodeand currentmodifierFlags; preserve originalcharacters, normalize whencharactersIgnoringModifiersis nil/empty, and ignore empty normalization results. Restores Cmd+T/W/C/V and Cmd+Shift+[/? under Korean/Russian IMEs.showOpenFolderPanel()to preventNSDocumentControllerfrom opening the Documents folder when SwiftUI focus breaks menu dispatch.Written for commit 10088a0. Summary will update on new commits.
Summary by CodeRabbit