Make browser find shortcuts respect remaps - #3728
Merged
Merged
Conversation
Add regression coverage for the three reported shortcut actions and for the browser find preflight path that still classifies Find in Directory from a literal key combination. Constraint: Local test execution is disabled for this repo; CI must run the new XCTest. Confidence: high Scope-risk: narrow Directive: Keep the browser find preflight tied to KeyboardShortcutSettings so remapped find-family shortcuts do not leave stale browser key-equivalent behavior behind. Tested: git diff --check Not-tested: XCTest execution; local tests are prohibited by project policy
Browser find command-equivalent preflight used literal Command-family combos to classify Find, Find in Directory, next/previous find, hide find, and Use Selection for Find before menu fallback. That meant a remapped or unbound shortcut could still be treated as its default command equivalent on the browser path.\n\nRoute the classifier through the same KeyboardShortcutSettings.Action defaults and persisted overrides as the central shortcut dispatcher, while preserving the existing browser-first exclusions for cmux-owned Find and Find in Directory.\n\nConstraint: Local test execution is disabled by project policy and this task forbids direct xcodebuild usage.\nRejected: Patch only Cmd+Shift+F | the same hardcoded classifier owned the rest of the browser find-family equivalents.\nConfidence: high\nScope-risk: narrow\nDirective: Browser find-family command equivalents must remain mapped through KeyboardShortcutSettings.Action before deciding browser-first routing.\nTested: git diff --check\nNot-tested: XCTest locally; CI is the validation path for this repo
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors browser “Find” routing to map enum cases to KeyboardShortcutSettings.Action and match incoming NSEvents against StoredShortcut values via a shortcut-driven matcher; introduces public/settings-visible action lists and default-binding changes; updates UI, template, web/schema, README, and localization; adds tests and test-teardown cleanup for shortcut lookup observation. ChangesShortcut & Routing Change DAG
Sequence Diagram(s)sequenceDiagram
participant UserEvent
participant AppDelegate
participant KeyboardShortcutSettings
participant StoredShortcutMatcher
participant Observer
UserEvent->>AppDelegate: NSEvent arrives
AppDelegate->>KeyboardShortcutSettings: request StoredShortcut via shortcutForAction(action)
KeyboardShortcutSettings->>StoredShortcutMatcher: provide StoredShortcut
StoredShortcutMatcher->>AppDelegate: match result (matches / no)
AppDelegate->>Observer: notify lookup (DEBUG observer)
AppDelegate->>AppDelegate: return BrowserFindCommandEquivalent? or nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR corrects a long-standing mismatch between the shipped default shortcuts and the Settings/docs surface for three actions (Toggle Right Sidebar →
Confidence Score: 5/5Safe to merge. The routing rewrite eliminates hardcoded modifier-flag literals in favor of the existing StoredShortcut matching path, and every changed default is covered by unit assertions. The browser find routing is now driven by the same StoredShortcut.matches(event:) path used everywhere else in the shortcut system, removing a class of stale-literal bugs. All six find-family actions are regression-tested via the shortcutLookupObserver hook. Default swaps for toggleFileExplorer and focusRightSidebar are asserted in WorkspaceUnitTests, and the public/non-public split for switchRightSidebarTo* actions is tested at both the settings-visibility and config-template levels. No actor isolation, blocking primitive, or SwiftUI state concerns were introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as AppDelegate
participant Routing as ShortcutRoutingSupport
participant KSS as KeyboardShortcutSettings
participant SS as StoredShortcut
App->>Routing: shouldRouteBrowserFindCommandEquivalent(event)
Routing->>Routing: browserFindCommandEquivalent(for: event)
loop BrowserFindCommandEquivalent.allCases
Routing->>KSS: shortcut(for: command.action)
KSS-->>Routing: StoredShortcut
Routing->>SS: matches(event:)
SS-->>Routing: Bool
end
Routing-->>App: BrowserFindCommandEquivalent?
App->>App: Route or pass through
Reviews (7): Last reviewed commit: "Name the file explorer shortcut as open ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review feedback pointed out that the browser preflight regression only observed Find in Directory even though the implementation now maps the whole browser find-family through KeyboardShortcutSettings.\n\nExtend the observer test across Find, Find in Directory, Find Next, Find Previous, Hide Find, and Use Selection for Find, and move the new tests into explicit shortcut-settings/browser-find sections so the non-Latin section remains scoped.\n\nConstraint: Local test execution is disabled by project policy and this task forbids direct xcodebuild usage.\nRejected: Cover only Cmd+G | every mapped browser find-family action now shares the same configurable lookup path.\nConfidence: high\nScope-risk: narrow\nDirective: Keep browser find-family regression coverage aligned with BrowserFindCommandEquivalent.action mappings.\nTested: git diff --check\nNot-tested: XCTest locally; CI is the validation path for this repo
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 134c3d1. Configure here.
Cursor Bugbot flagged that replacing the hardcoded browser Find Previous route changes Cmd+Shift+G behavior. The configured cmux default and docs use Cmd+Option+G for Find Previous, while Cmd+Shift+G is the documented Toggle React Grab shortcut.\n\nUpdate the browser preflight regression tests to assert the settings-owned Find Previous default and to keep Cmd+Shift+G out of the browser find-family classifier, preserving remappability without reviving the hidden hardcoded duplicate.\n\nConstraint: Local test execution is disabled by project policy and this task forbids direct xcodebuild usage.\nRejected: Change the Find Previous default to Cmd+Shift+G | that would conflict with the documented Toggle React Grab default.\nConfidence: high\nScope-risk: narrow\nDirective: Browser find preflight should follow configured KeyboardShortcutSettings defaults; do not add an unremappable Cmd+Shift+G browser-only exception.\nTested: git diff --check\nNot-tested: XCTest locally; CI is the validation path for this repo
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The Settings UI was exposing right-sidebar mode selectors as first-class shortcuts and the defaults for file explorer/right-sidebar focus were swapped from product intent. Keep the legacy action cases available for existing cmux.json bindings, but remove their public defaults and Settings/docs exposure. Constraint: Do not run local XCTest or direct xcodebuild per repo/task policy Rejected: Delete right-sidebar mode action cases | existing cmux.json users can still have explicit bindings, and the parser coverage depends on those cases Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest/XCUITest/direct xcodebuild per policy
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 212-215: Update the README Find table entry for "Find Previous" to
match the actual default shortcut used by the code: change the documented key
from "⌘ ⇧ G" to "⌥ ⌘ G" (Option+Command+G) so it aligns with the findPrevious
case that returns StoredShortcut(key: "g", command: true, shift: false, option:
true, control: false); ensure the symbol and plain-text description reflect
Option+Command+G consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 752b7481-b431-474b-812f-9041afc190b6
📒 Files selected for processing (10)
README.mdSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swiftcmuxTests/WorkspaceUnitTests.swiftweb/data/cmux-shortcuts.tsweb/data/cmux.schema.json
The user-facing shortcut action toggles focus between the terminal and right sidebar, so the Settings row and View menu should say Toggle Right Sidebar instead of Focus Right Sidebar. Keep the internal action id unchanged so existing cmux.json bindings continue to work. Constraint: Do not run local XCTest or direct xcodebuild per repo/task policy Rejected: Rename the action case or config key | would churn persisted shortcut identifiers for a display-only change Confidence: high Scope-risk: narrow Tested: git diff --check; rg confirmed no user-facing Focus Right Sidebar text remains Not-tested: Local XCTest/XCUITest/direct xcodebuild per policy
CodeRabbit caught that the README still described Find Previous as Cmd+Shift+G even though the configured default is Cmd+Option+G. Update the docs so the shortcut table matches KeyboardShortcutSettings. Constraint: Do not run local XCTest or direct xcodebuild per repo/task policy Confidence: high Scope-risk: narrow Tested: git diff --check; rg confirmed README uses Option+Command+G Not-tested: Local XCTest/XCUITest/direct xcodebuild per policy
Legacy right-sidebar mode actions are no longer public defaults or Settings rows, but users may still have explicit cmux.json bindings. Keeping the schema enum aligned with runtime parsing prevents editor/schema validation from rejecting those existing configs while still adding the new toggleFileExplorer action. Constraint: User requested the Ctrl+1 through Ctrl+5 shortcuts disappear from Settings/defaults, not that legacy config files become invalid. Rejected: Remove the legacy action ids from the schema | runtime still parses them, so schema validation would be stricter than the app. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local tests per repository policy; CI will rerun after push.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai resume |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
✅ Actions performedReviews resumed. |
The Settings shortcut list follows the public action order by default, which placed Toggle Right Sidebar, Find in Directory, and Toggle File Explorer in separate parts of the list. The Settings visible-action projection now colocates those action rows in the requested order while leaving their displayed chords driven by KeyboardShortcutSettings. Constraint: Do not hardcode the default key equivalents in the Settings UI; only order action cases. Rejected: Move enum cases to force UI order | enum order is shared with defaults/schema/template expectations and would make the setting-specific intent less clear. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest per repository policy; CI will rerun after push.
The Settings shortcut row should describe the action users invoke, not the historical internal action id. Keep the stable toggleFileExplorer id for persisted configs, but show Open File Explorer and place the three related rows in the requested order: Toggle Right Sidebar, Open File Explorer, Find in Directory. Constraint: Shortcut key equivalents must remain driven by KeyboardShortcutSettings, not hardcoded UI labels. Rejected: Rename the Action case | would churn persisted shortcut/config identifiers without changing runtime behavior. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest per repository policy; CI will rerun after push.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 161: Replace the text "Open file explorer" in the keyboard shortcuts
table row (the entry showing "⌘ ⇧ E") with "Toggle file explorer" so it matches
the wording used for "Toggle sidebar" and "Toggle right sidebar", ensuring
consistent use of "Toggle" across related entries in README.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93e1f08e-1c18-4520-b226-9cb6b99be6b9
📒 Files selected for processing (5)
README.mdResources/Localizable.xcstringsSources/KeyboardShortcutSettings.swiftcmuxTests/WorkspaceUnitTests.swiftweb/data/cmux-shortcuts.ts
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
austinywang
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 8, 2026 21:07
Dismissed stale CodeRabbit review after the README Find Previous thread was fixed and resolved; current checks are green.
This was referenced May 9, 2026
Merged
4 tasks
This was referenced May 12, 2026
2 of 6 tasks
This was referenced Jul 2, 2026
This was referenced Jul 10, 2026
Closed
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
⌘⇧E, Toggle Right Sidebar is now⌥⌘B, and Find in Directory remains⌘⇧F..focusRightSidebaraction id for existing config compatibility..toggleFileExploreraction id for existing config compatibility.KeyboardShortcutSettings.Ctrl+1throughCtrl+5) from public shortcut defaults, Settings search/display, docs, and generated config templates while keeping legacy explicitcmux.jsonbindings parseable and schema-valid.KeyboardShortcutSettings.shortcut(for:)instead of hardcoded literals.Changed Sites
Sources/KeyboardShortcutSettings.swift: swapped the File Explorer / right-sidebar defaults, renamed the visible labels to Toggle Right Sidebar and Open File Explorer, marked right-sidebar mode selector actions non-public, unbound their built-in defaults, and orders the Settings-visible action list so Toggle Right Sidebar, Open File Explorer, and Find in Directory are adjacent without hardcoding their key chords.Sources/cmuxApp.swift: Settings -> Keyboard Shortcuts rendersKeyboardShortcutSettings.settingsVisibleActions; View menu label now says Toggle Right Sidebar.Resources/Localizable.xcstrings: updated English/Japanese/Korean translations for the Settings shortcut labels.Sources/SettingsSearchAliases.swift: Settings search aliases use the same visible action list.Sources/KeyboardShortcutSettingsFileStore.swift: generatedcmux.jsonshortcut templates include public shortcut actions only.Sources/App/ShortcutRoutingSupport.swift: browser find-family command-equivalent routing reads configured shortcuts.cmuxTests/AppDelegateShortcutRoutingTests.swift: regression coverage for.toggleFileExplorer,.focusRightSidebar, and.findInDirectoryuses the corrected default chords.cmuxTests/WorkspaceUnitTests.swift: asserts the corrected defaults, visible Toggle Right Sidebar/Open File Explorer labels, public Settings visibility/order for the three reported actions, and non-public/unbound status for right-sidebar mode selectors.cmuxTests/ShortcutAndCommandPaletteTests.swift: assertsCtrl+1...5do not switch right-sidebar modes by default while explicit configured bindings still work.README.md,web/data/cmux-shortcuts.ts: docs now match the corrected public shortcut surface and order; README also documents Find Previous as⌥⌘G.web/data/cmux.schema.json: addstoggleFileExplorerwhile preserving legacyswitchRightSidebar*action IDs so existing explicitcmux.jsonbindings remain schema-valid.Shortcut Audit
Already data-driven
.keyboardShortcut(...)sites:Sources/NotificationsPage.swift: Jump to Latest Unread usesKeyboardShortcutSettings.shortcut(for: .jumpToUnread).Sources/WorkspaceContentView.swift: empty-pane New Terminal/Open Browser buttons useKeyboardShortcutSettings.shortcut(for:).Sources/ContentView.swift: rename/edit/close context-menu shortcuts useKeyboardShortcutSettings.shortcut(for:).Sources/cmuxApp.swift: numbered workspace selection usesmenuShortcut(for: .selectWorkspaceByNumber), and menu command buttons usemenuShortcut(for:).Allowed non-remappable
.keyboardShortcut(...)sites:.defaultAction/.cancelActionin alerts and popovers.Changed
.keyboardShortcut(...)sites:.keyboardShortcutliteral.NSEvent and Menu Audit
AppDelegate.handleCustomShortcutdispatches.toggleFileExplorer,.focusRightSidebar, and.findInDirectorythroughmatchConfiguredShortcut(event:action:).Sources/cmuxApp.swiftreadKeyboardShortcutSettings.menuShortcut(for:); no hardcoded"b","e", or"f"modifier sets remain for the reported actions.NSMenuItemconstruction sites either use configured menu shortcuts or system/default/cancel equivalents.Settings UI / E2E Validation
Expected Settings -> Keyboard Shortcuts rows after this correction:
⌥⌘B.⇧⌘E.⇧⌘F.Show Sidebar Files/Find/Vault/Feed/Dock(^1...^5) are no longer public Settings rows or default shortcuts.Tagged dev launch validation is executed with
./scripts/reload.sh --tag issue-3726-remappable-keyboard-shortcuts --launch.Test Plan
git diff --check5ac69852a: 18 passed, 0 failed, 0 pending, 7 skipped.xcodebuildwas run.