Repository navigation
Conversation
…anaflow-ai#2198) When the kitty keyboard protocol is active, charactersIgnoringModifiers may carry a synthetic value that does not match the shortcut key. The previous code checked this guard before attempting the keyCode-based layout translation, causing matchShortcut to return false for letter shortcuts (like Cmd+Shift+U) even though the keyCode and modifiers were correct. The event then fell through to the terminal, producing the escape sequence [12629;10u. Fix by moving the layout-character match (shortcutLayoutCharacterProvider) before the early-return guard. The keyCode→layout translation reliably identifies the physical key regardless of keyboard protocol mode. The guard is preserved after both character sources have been tried, so the cross-layout collision protection for letter shortcuts remains intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@anthhub is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdjusted the shortcut matching algorithm in AppDelegate to modify when Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
10683-10703: Consider caching the translated layout character once per event.
handleCustomShortcut(event:)probesmatchShortcut(event:shortcut:)many times for the sameNSEvent. Now that this lookup runs before the Cmd-letter guard, each non-matching Cmd-letter probe repeats the same keyboard-layout translation work. Precomputing the translated character once inhandleCustomShortcut(event:)and threading it through here would keep the fix while trimming that overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 10683 - 10703, The code repeatedly calls shortcutLayoutCharacterProvider for the same NSEvent causing redundant layout translations when handleCustomShortcut(event:) invokes matchShortcut(event:shortcut:) multiple times; modify handleCustomShortcut(event:) to compute the translated layout character once (e.g., call shortcutLayoutCharacterProvider(event.keyCode, event.modifierFlags) and store it) and pass that precomputed character into matchShortcut(event:shortcut:) (and/or change matchShortcut signature or add an overload/parameter to accept a precomputed layoutCharacter) so that the existing Cmd-letter guard block (which compares eventCharacter: layoutCharacter) uses the cached value instead of recomputing it each probe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 10683-10703: The code repeatedly calls
shortcutLayoutCharacterProvider for the same NSEvent causing redundant layout
translations when handleCustomShortcut(event:) invokes
matchShortcut(event:shortcut:) multiple times; modify
handleCustomShortcut(event:) to compute the translated layout character once
(e.g., call shortcutLayoutCharacterProvider(event.keyCode, event.modifierFlags)
and store it) and pass that precomputed character into
matchShortcut(event:shortcut:) (and/or change matchShortcut signature or add an
overload/parameter to accept a precomputed layoutCharacter) so that the existing
Cmd-letter guard block (which compares eventCharacter: layoutCharacter) uses the
cached value instead of recomputing it each probe.
Greptile SummaryThis PR fixes a Cmd+Shift+U escape sequence leak in kitty keyboard protocol mode by reordering the Confidence Score: 4/5Safe to merge; the reordering is logically correct and the hasUsableEventChars removal is semantically equivalent, but a regression test should accompany the fix per project policy. The fix correctly moves the layout-character match before the cross-layout guard, which is exactly the minimal change needed to unblock the kitty protocol path. The hasUsableEventChars → hasEventChars substitution is provably equivalent. Non-US layout protection and ANSI fallback behaviour are preserved. The only remaining action is adding a regression test per CLAUDE.md policy, which is practical with the existing AppDelegateShortcutRoutingTests harness. Sources/AppDelegate.swift (matchShortcut, lines 10652–10727) — the sole changed file; logic is correct but lacks regression test coverage. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[matchShortcut called] --> B{flags == shortcut.modifierFlags?}
B -- No --> Z[return false]
B -- Yes --> C{shortcutKey == Return?}
C -- Yes --> D{keyCode 36 or 76?}
D -- Yes --> T[return true]
D -- No --> Z
C -- No --> E[Try eventCharsIgnoringModifiers match]
E -- Match --> T
E -- No match --> F[Compute hasEventChars / eventCharsAreASCII]
F --> G[Try layoutCharacter match - keyCode to layout translation]
G -- Match --> T
G -- No match --> H{hasEventChars AND ASCII AND Cmd AND letter shortcut?}
H -- Yes --> Z
H -- No --> I{allowANSIKeyCodeFallback? Ctrl / non-letter-Cmd / non-ASCII / empty+no-layout}
I -- Yes --> J{keyCode == expectedKeyCode?}
J -- Yes --> T
J -- No --> Z
I -- No --> Z
style G fill:#d4edda,stroke:#28a745,color:#000
style H fill:#fff3cd,stroke:#ffc107,color:#000
Reviews (1): Last reviewed commit: "fix: prevent Cmd+Shift+U escape sequence..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -10699,6 +10690,18 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent | |||
| return true | |||
| } | |||
|
|
|||
| // For command-based shortcuts, once both character sources have been tried, block | |||
| // further keyCode fallback for letter shortcuts to avoid physical-key collisions | |||
| // across layouts. Non-ASCII characters (Russian, Korean, CJK, etc.) cannot match | |||
| // a Latin shortcut key, so always allow keyCode fallback in that case. | |||
| if hasEventChars, | |||
| eventCharsAreASCII, | |||
| flags.contains(.command), | |||
| !flags.contains(.control), | |||
| shouldRequireCharacterMatchForCommandShortcut(shortcutKey: shortcutKey) { | |||
| return false | |||
| } | |||
There was a problem hiding this comment.
CLAUDE.md requires a two-commit regression test structure for bug fixes: the test is added first (CI goes red), then the fix (CI goes green). This scenario is directly unit-testable using the existing harness — AppDelegateShortcutRoutingTests already overrides shortcutLayoutCharacterProvider and constructs synthetic NSEvents (see the Cyrillic-layout tests around line 3022). A companion test could look like:
// Kitty keyboard protocol: charactersIgnoringModifiers carries a synthetic ASCII
// escape sequence, but the keyCode-based layout translation resolves the physical key.
appDelegate.shortcutLayoutCharacterProvider = { keyCode, _ in
keyCode == 32 ? "u" : nil // kVK_ANSI_U
}
guard let event = NSEvent.keyEvent(
with: .keyDown,
location: .zero,
modifierFlags: [.command, .shift],
timestamp: 0,
windowNumber: window.windowNumber,
context: nil,
characters: "[12629;10u",
charactersIgnoringModifiers: "[12629;10u", // synthetic kitty value
isARepeat: false,
keyCode: 32
) else { return }
// matchShortcut should return true (event consumed, no escape leak)
XCTAssertTrue(appDelegate.matchShortcut(event: event, shortcut: /* jumpToUnread shortcut */))Without a regression test, a future refactor could silently reintroduce the ordering bug.
Context Used: CLAUDE.md (source)
|
Closing — this conflicts with PR #2216 (physical keyCode refactor by @lawrencecchen) which touches the same |
1 participant
Fixes #2198
When no unread notifications exist, pressing Cmd+Shift+U would output the escape sequence
[12629;10uinto the terminal. This happened because the kitty keyboard protocol encodes key events differently, causingmatchShortcutto fail character matching and letting the event pass through to the terminal.Root Cause
In
matchShortcut, the layout-character match (shortcutLayoutCharacterProvider, which uses keyCode → keyboard layout translation) was placed after an early-return guard that blocked further matching for letter shortcuts whencharactersIgnoringModifierswas non-empty ASCII but didn't match the shortcut key. In kitty protocol mode,charactersIgnoringModifierscarries a synthetic value, so the guard fired before the reliable keyCode-based translation had a chance to run.Fix
Move the
shortcutLayoutCharacterProvidermatch before the guard. The keyCode → layout translation reliably identifies the physical key (e.g. keyCode 32 → "u") regardless of keyboard protocol mode. The guard is preserved after both character sources have been tried, so cross-layout collision protection for letter shortcuts remains intact.Test plan
echo -e '\e[>1u')Summary by cubic
Fixes #2198. Stops Cmd+Shift+U from printing a kitty keyboard protocol escape sequence when there are no unread notifications by matching the keyCode→layout character before the early guard.
Written for commit fbb7d8f. Summary will update on new commits.
Summary by CodeRabbit