Fix Cmd+I and notifications shortcut conflict - #1135
austinywang wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR fixes keyboard shortcut routing by introducing layout-aware character matching that differentiates semantic characters from physical key codes. It reassigns Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR resolves a shortcut conflict (issue #1091) by redistributing the Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
KE[Key Event received\nin AppDelegate] --> SN{Cmd+Shift+I?\nshowNotifications}
SN -- yes --> SNA[Toggle Notifications Popover\nreturn true]
SN -- no --> BDT{Cmd+I?\ntoggleBrowserDeveloperTools}
BDT -- yes --> BDTA[Toggle Dev Tools\nNSSound.beep if not handled\nreturn true]
BDT -- no --> BJC{Option+Cmd+C?\nshowBrowserJavaScriptConsole}
BJC -- yes --> BJCA[Show JS Console\nNSSound.beep if not handled\nreturn true]
BJC -- no --> OTHER[Continue to other\nshortcut checks]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func testDefaultShortcutForShowNotifications() { | ||
| let shortcut = KeyboardShortcutSettings.Action.showNotifications.defaultShortcut | ||
| XCTAssertEqual(shortcut.key, "i") | ||
| XCTAssertTrue(shortcut.command) | ||
| XCTAssertTrue(shortcut.shift) | ||
| XCTAssertFalse(shortcut.option) | ||
| XCTAssertFalse(shortcut.control) | ||
| } |
There was a problem hiding this comment.
Test added to mismatched class
testDefaultShortcutForShowNotifications tests the showNotifications action, but it has been placed inside the BrowserDeveloperToolsShortcutDefaultsTests class. That class name (and its two existing tests) is scoped to the browser developer-tools shortcuts. Adding an unrelated notification shortcut test here will make the suite harder to navigate over time.
Consider either renaming the class (e.g. BrowserAndNotificationShortcutDefaultsTests) or moving the new test to an existing class that covers general shortcut defaults.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
9247-9308:⚠️ Potential issue | 🟠 MajorDon't short-circuit before the layout-aware branch runs.
Line 9278 returns
falsefor Command-letter shortcuts beforeshortcutLayoutCharacterProvider(...)is consulted. That makes the new semantic-layout path unreachable whenever either event string is populated, so alternate input sources that still surface physical or Option-transformed characters can still missCmd+I/⌥Cmd+C.Suggested fix
let hasEventChars = !(eventCharacters?.isEmpty ?? true) || !(eventCharsIgnoringModifiers?.isEmpty ?? true) - if hasEventChars, - flags.contains(.command), - !flags.contains(.control), - shouldRequireCharacterMatchForCommandShortcut(shortcutKey: shortcutKey) { - return false - } // Match using the current keyboard layout so Command shortcuts stay character-based // across layouts (QWERTY, Dvorak, etc.) instead of being tied to ANSI physical keys. let layoutCharacter = shortcutLayoutCharacterProvider(event.keyCode, event.modifierFlags) if shortcutCharacterMatches( @@ ) { return true } + + if hasEventChars, + flags.contains(.command), + !flags.contains(.control), + shouldRequireCharacterMatchForCommandShortcut(shortcutKey: shortcutKey) { + return false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 9247 - 9308, The early return that blocks layout-aware matching is too aggressive: instead of returning false immediately in the block that checks hasEventChars && flags.contains(.command) && !flags.contains(.control) && shouldRequireCharacterMatchForCommandShortcut(shortcutKey:), compute or retrieve the layoutCharacter via shortcutLayoutCharacterProvider(event.keyCode, event.modifierFlags) first and only short-circuit when that layoutCharacter is empty (i.e., no semantic layout character is available); in practice move or duplicate the shortcutLayoutCharacterProvider call so the check becomes conditional on layoutCharacter?.isEmpty (so the layout-aware branch still runs when a semantic character exists) while preserving the existing strictness for command-letter shortcuts when no layout character is present.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
8958-8968: Keep the debug probe semantic too.This probe still treats ANSI key codes
34and8as literalCmd+I/⌥Cmd+C. On Dvorak, semanticCmd+Ccan still arrive on ANSI34, so the new debug trace will label copy as a developer-tools candidate again. ReusingmatchShortcut(...)here keeps the diagnostics aligned with the real routing logic.Suggested change
- let chars = (event.charactersIgnoringModifiers ?? "").lowercased() - let flags = event.modifierFlags.intersection(.deviceIndependentFlagsMask) - if flags == [.command] { - if chars == "i" || event.keyCode == 34 { - return "toggle.literal" - } - } - if flags == [.command, .option] { - if chars == "c" || event.keyCode == 8 { - return "console.literal" - } - } + if matchShortcut( + event: event, + shortcut: StoredShortcut(key: "i", command: true, shift: false, option: false, control: false) + ) { + return "toggle.literal" + } + if matchShortcut( + event: event, + shortcut: StoredShortcut(key: "c", command: true, shift: false, option: true, control: false) + ) { + return "console.literal" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8958 - 8968, The debug probe currently treats ANSI key codes (event.keyCode == 34 / 8) as literal shortcuts which breaks semantics on non‑ANSI layouts; replace the direct keyCode checks in the block that returns "toggle.literal" and "console.literal" so they call the existing matchShortcut(...) helper (passing flags, chars and/or event as its signature requires) instead of comparing event.keyCode, and only return the literal probe label when matchShortcut(...) indicates the semantic shortcut matches; keep the existing flags and chars checks but remove the hardcoded numeric keyCode comparisons to ensure the probe uses the same semantic routing logic as the app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 9247-9308: The early return that blocks layout-aware matching is
too aggressive: instead of returning false immediately in the block that checks
hasEventChars && flags.contains(.command) && !flags.contains(.control) &&
shouldRequireCharacterMatchForCommandShortcut(shortcutKey:), compute or retrieve
the layoutCharacter via shortcutLayoutCharacterProvider(event.keyCode,
event.modifierFlags) first and only short-circuit when that layoutCharacter is
empty (i.e., no semantic layout character is available); in practice move or
duplicate the shortcutLayoutCharacterProvider call so the check becomes
conditional on layoutCharacter?.isEmpty (so the layout-aware branch still runs
when a semantic character exists) while preserving the existing strictness for
command-letter shortcuts when no layout character is present.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 8958-8968: The debug probe currently treats ANSI key codes
(event.keyCode == 34 / 8) as literal shortcuts which breaks semantics on
non‑ANSI layouts; replace the direct keyCode checks in the block that returns
"toggle.literal" and "console.literal" so they call the existing
matchShortcut(...) helper (passing flags, chars and/or event as its signature
requires) instead of comparing event.keyCode, and only return the literal probe
label when matchShortcut(...) indicates the semantic shortcut matches; keep the
existing flags and chars checks but remove the hardcoded numeric keyCode
comparisons to ensure the probe uses the same semantic routing logic as the app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2437a7bc-40e8-440c-9342-a4d9ff04f1be
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Closes #1091
Summary by CodeRabbit
Bug Fixes
Tests