fix(keyboard): support CJK input methods for custom shortcuts - #1770
pstanton237 wants to merge 1 commit into
Conversation
When a CJK input method (Korean, Japanese, Chinese) is active, `event.charactersIgnoringModifiers` returns non-ASCII characters (e.g. "ㅅ" for the T key on Korean 2-Set). The strict character-match gate in `matchShortcut` treated these as valid ASCII characters and returned `false` before reaching keyCode-based fallback, causing shortcuts like Cmd+T and Cmd+Shift+L to silently fail. Add an ASCII check so the gate only blocks fallback when the event characters are actually ASCII — when they are non-ASCII (CJK IME), fall through to ANSI keyCode matching which correctly identifies the physical key. Shortcuts registered as SwiftUI menu items (Cmd+B, Cmd+D, etc.) were unaffected because AppKit's menu system processes them before the IME.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@pstanton237 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughModified keyboard shortcut handling in AppDelegate to properly support CJK input methods by introducing ASCII-specific character validation checks and adjusting keyCode-based fallback logic. Added tests to verify shortcuts function correctly when non-ASCII IME characters are provided. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
Closing as duplicate of #1649 which addresses the same CJK IME shortcut issue with a similar approach. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
Keyboard shortcuts handled through
handleCustomShortcut(not registered as SwiftUI menu items) fail when a CJK input method (Korean, Japanese, Chinese) is active. For example, Cmd+T (New Terminal) and Cmd+Shift+L (Open Browser) do not work with Korean 2-Set IME, while menu-registered shortcuts like Cmd+B (Toggle Sidebar) and Cmd+D (Split Right) work fine because AppKit's menu system processes them before the IME.Root cause: When a CJK IME is active,
event.charactersIgnoringModifiersreturns non-ASCII characters (e.g. "ㅅ" for the T key). Two gates inmatchShortcuttreat these non-ASCII characters as valid character data and block the ANSI keyCode fallback path:falseearly for letter-based Command shortcuts when characters are present but don't matchFix: Treat non-ASCII characters from CJK input methods as absent for shortcut matching purposes — they cannot match ASCII shortcut keys, so the function falls through to keyCode-based matching which correctly identifies the physical key.
Test plan
Fixes #1545
Summary by cubic
Fixes custom keyboard shortcuts failing when a CJK IME is active by ignoring non-ASCII IME characters and falling back to ANSI keyCode matching. Restores shortcuts like Cmd+T and Cmd+Shift+L under Korean/Japanese/Chinese IMEs without affecting menu-registered shortcuts.
charactersIgnoringModifiersand layout characters as usable when they are ASCII; otherwise match by physicalkeyCode.Written for commit d7acb86. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests