fix(hotkeys): match terminal Ctrl chords by event.code under non-Latin IME#3405
fix(hotkeys): match terminal Ctrl chords by event.code under non-Latin IME#3405AytuncYildizli wants to merge 1 commit into
Conversation
… non-Latin IME When a non-Latin IME is active (Turkish, Korean, Russian, etc.), `event.key` returns the IME-transformed character: Ctrl+C reports `ç` on Turkish, `ㅇ` on Korean 2-Set, etc. `matchesHotkeyEvent` only compared against `event.key`, so `Ctrl+C/D/Z` and other terminal-reserved chords stopped reaching the PTY under those layouts. Add a fallback that matches single-letter shortcuts against `event.code` (`KeyA`–`KeyZ`), mirroring the existing pattern for digit keys (line 272). The physical key is layout-independent, so the chord is detected regardless of IME state. Tests: 15/15 pass, including two new regressions for Turkish (`ç` + `KeyC`) and Korean (`ㅇ` + `KeyD`). Fixes superset-sh#3365.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes extend hotkey handling to support non-Latin IME layouts by using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 unit tests (beta)
⚔️ Resolve merge conflicts
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 adds a narrow IME/non-Latin keyboard layout fallback inside Key changes:
Confidence Score: 5/5Safe to merge — the change is minimal, intentionally narrow, follows an established in-codebase pattern, and is covered by two targeted regression tests. The fix is a single if-block that only activates for single a–z hotkeys via event.code, mirroring the digit-key fallback already present. Logic is correct, the test suite passes (15/15), and there are no side effects on non-Latin-key or non-letter hotkeys. The only finding is a trivial redundant guard (key.length === 1) that has no runtime impact. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[matchesHotkeyEvent called] --> B{canonicalizeHotkey returns key?}
B -- No --> Z1[return false]
B -- Yes --> C{Modifier keys match?}
C -- No --> Z2[return false]
C -- Yes --> D{key === 'slash'?}
D -- Yes --> E{eventKey or eventCode === 'slash'?}
E -- Yes --> Z3[return true]
E -- No --> F{Arrow key alias match?}
D -- No --> F
F -- Yes --> Z4[return true]
F -- No --> G{key matches digit pattern AND eventCode === digit+key?}
G -- Yes --> Z5[return true]
G -- No --> H{key is single a-z AND eventCode === 'key'+key?}
H -- Yes --> Z6[return true - NEW: IME fallback]
H -- No --> I{eventKey === key?}
I -- Yes --> Z7[return true]
I -- No --> Z8[return false]
style Z6 fill:#22c55e,color:#fff
style H fill:#bbf7d0
Reviews (1): Last reviewed commit: "fix(hotkeys): match terminal-reserved Ct..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (key.length === 1 && /^[a-z]$/.test(key) && eventCode === `key${key}`) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
key.length === 1 guard
The leading key.length === 1 check is already implied by /^[a-z]$/.test(key) — the anchored single-character regex can only ever match a string of length 1. Dropping it makes the condition a little cleaner and avoids the reader wondering whether the two predicates cover different cases.
| if (key.length === 1 && /^[a-z]$/.test(key) && eventCode === `key${key}`) { | |
| return true; | |
| } | |
| if (/^[a-z]$/.test(key) && eventCode === `key${key}`) { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Closing — investigated further and realized this PR targets |
1 participant
Summary / What changed
matchesHotkeyEvent(apps/desktop/src/shared/hotkeys.ts): when the parsed hotkey is a single lettera–z, also match againstevent.code(KeyA–KeyZ). Mirrors the existing digit-key fallback at line 272.hotkeys.test.tscover Turkish (ç+KeyC) and Korean (ㅇ+KeyD).Why
Originally reported by @eungkyu in #3365 for Korean 2-Set IME. Same bug reproduces on Turkish layout:
Ctrl+Cemitsevent.key = "ç"whileevent.codestays"KeyC". Without the fallback, terminal-reserved chords (Ctrl+C/D/Z/V…) stop reaching the PTY whenever a non-Latin IME is active.The patch is intentionally narrow: only single-letter
a–zhotkeys gain theevent.codefallback. This keeps existing matching behavior for everything else and follows the same pattern the code already uses for digits andslash.Tests
bun test apps/desktop/src/shared/hotkeys.test.tsReal-world validation
Ctrl+Cis swallowed; after the patch SIGINT reaches the PTY normally.ㅇ+KeyDmatches their reproduction.Issue
Fixes #3365.
Summary by cubic
Fixes terminal Ctrl letter chords failing under non-Latin IMEs by matching single-letter hotkeys against
event.code(KeyA–KeyZ) inmatchesHotkeyEvent(fixes #3365). Adds regression tests for Turkish (ç+KeyC) and Korean (ㅇ+KeyD).Written for commit c4f5db0. Summary will update on new commits.
Summary by CodeRabbit