Repository navigation
Conversation
…rd collision Two actions may share a default chord only when their effective when-contexts are disjoint or router priority picks a deterministic winner; otherwise one shadows the other and the duplicate is unreachable. Sweep every shared default chord and fail on any collision outside an explicit intentional-overlap allowlist. Goes red on two shipped collisions: focusHistoryBack/Forward (⌘[ / ⌘], .application-scoped) shadow the browser pane's Back/Forward (browserBack/Forward, browser-scoped) whenever a browser panel is focused. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… ⌘] collision focusHistoryBack/Forward shipped .application-scoped on ⌘[ / ⌘], the same chords the browser pane binds to Back/Forward (browserBack/browserForward, browser-scoped). While a browser panel was focused both matched the keystroke, so the duplicate was an unresolved conflict — the browser's own Back/Forward is the natural owner of ⌘[ / ⌘] there. Scope focus-history navigation to .nonBrowserPanel (mirrored in both the CmuxSettings default-context map and the app target's runtime shortcutContext so the drift test stays aligned). Outside a browser panel it still navigates focus history; inside one it yields the chord to the browser. The chord stays rebindable in Settings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@notadev99 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughFocus-history back/forward shortcuts are explicitly configured to exclude browser and sidebar focus contexts. Changes span the action's focus-context definition, context routing in keyboard shortcut settings, and comprehensive tests verifying the routing prevents collisions and yields to browser back/forward behavior. ChangesFocus History Shortcut Context Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (20 passed)
✨ 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 |
Greptile SummaryThis PR fixes a keyboard shortcut collision where
Confidence Score: 4/5Safe to merge for the stated browser-collision fix; the only open question is whether silently disabling focus-history navigation while the right sidebar is focused is the intended behavior. The browser collision fix is correct and well-tested. The only wrinkle is that reusing
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
KEY["⌘[ or ⌘] keystroke"]
CTX{"Focus context?"}
BROWSER["browserFocus = true"]
SIDEBAR["sidebarFocus = true"]
OTHER["Neither browser nor sidebar"]
KEY --> CTX
CTX -->|browser panel focused| BROWSER
CTX -->|right sidebar focused| SIDEBAR
CTX -->|other| OTHER
BROWSER -->|fires| BB["browserBack / browserForward (.browserPanel)"]
BROWSER -->|suppressed| FHB["focusHistoryBack / focusHistoryForward (suppressed)"]
SIDEBAR -->|no handler| NOOP["no-op (previously focusHistory fired)"]
OTHER -->|fires| FH["focusHistoryBack / focusHistoryForward (.nonBrowserPanel)"]
Reviews (1): Last reviewed commit: "Scope focusHistory Back/Forward out of b..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case .focusHistoryBack, .focusHistoryForward: | ||
| // ⌘[ / ⌘] are the browser pane's Back/Forward. Scope workspace | ||
| // focus-history navigation out of browser panels so it yields the | ||
| // chord there instead of colliding with browserBack/browserForward. | ||
| return .and(.not(.atom(.browserFocus)), .not(.atom(.sidebarFocus))) |
There was a problem hiding this comment.
The new when-clause inherits .nonBrowserPanel's full condition — !browserFocus && !sidebarFocus — but the PR only motivates excluding browser focus. As a result, ⌘[ / ⌘] now silently stop navigating focus history whenever the right sidebar is focused (previously they fired at .application scope). Unlike renameTab / sendCtrlFToTerminal, there's no obvious sidebar-owned handler for these chords that would justify the exclusion, so this looks like an unintentional behavioral change: sidebar-focused users lose focus-history navigation without any competing action taking their place.
| @Test func focusHistoryNavigationYieldsToBrowserBackForward() { | ||
| for action in [ShortcutAction.focusHistoryBack, .focusHistoryForward] { | ||
| let clause = action.defaultFocusWhenClause | ||
| #expect(!clause.evaluate(ShortcutFocusState(browser: true, markdown: false, sidebar: false))) | ||
| #expect(clause.evaluate(ShortcutFocusState(browser: false, markdown: false, sidebar: false))) | ||
| } | ||
| } |
There was a problem hiding this comment.
focusHistoryNavigationYieldsToBrowserBackForward asserts the clause is false when browser: true and true when browser: false, sidebar: false. Since the underlying when-clause now also evaluates to false when sidebar: true, adding a third assertion (!clause.evaluate(ShortcutFocusState(browser: false, markdown: false, sidebar: true))) would make the test document this behavior explicitly — and catch it if the clause is ever narrowed back to browser-only exclusion without updating the sidebar dimension.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@Packages/CmuxSettings/Tests/CmuxSettingsTests/ShortcutDefaultCollisionTests.swift`:
- Around line 69-75: The test focusHistoryNavigationYieldsToBrowserBackForward
currently asserts browser focus yields and no-focus passes, but misses asserting
that sidebar focus is excluded; update the test (function
focusHistoryNavigationYieldsToBrowserBackForward) to also check that for each
action in [ShortcutAction.focusHistoryBack, .focusHistoryForward] the clause
(action.defaultFocusWhenClause) evaluates to false when given
ShortcutFocusState(browser: false, markdown: false, sidebar: true) so the
.not(.atom(.sidebarFocus)) part is covered.
🪄 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: 5aa1160e-586e-47a7-b29a-c3f73c85311b
📒 Files selected for processing (3)
Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/CmuxSettings/Tests/CmuxSettingsTests/ShortcutDefaultCollisionTests.swiftSources/KeyboardShortcutContext.swift
| @Test func focusHistoryNavigationYieldsToBrowserBackForward() { | ||
| for action in [ShortcutAction.focusHistoryBack, .focusHistoryForward] { | ||
| let clause = action.defaultFocusWhenClause | ||
| #expect(!clause.evaluate(ShortcutFocusState(browser: true, markdown: false, sidebar: false))) | ||
| #expect(clause.evaluate(ShortcutFocusState(browser: false, markdown: false, sidebar: false))) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test case for sidebar focus.
The test verifies that focus-history shortcuts yield to browser focus, but the when-clause also excludes sidebar focus (.and(.not(.atom(.browserFocus)), .not(.atom(.sidebarFocus)))). Add a test case to verify sidebar: true evaluates to false.
🧪 Proposed test case addition
`@Test` func focusHistoryNavigationYieldsToBrowserBackForward() {
for action in [ShortcutAction.focusHistoryBack, .focusHistoryForward] {
let clause = action.defaultFocusWhenClause
`#expect`(!clause.evaluate(ShortcutFocusState(browser: true, markdown: false, sidebar: false)))
`#expect`(clause.evaluate(ShortcutFocusState(browser: false, markdown: false, sidebar: false)))
+ `#expect`(!clause.evaluate(ShortcutFocusState(browser: false, markdown: false, sidebar: true)))
}
}🤖 Prompt for 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.
In
`@Packages/CmuxSettings/Tests/CmuxSettingsTests/ShortcutDefaultCollisionTests.swift`
around lines 69 - 75, The test focusHistoryNavigationYieldsToBrowserBackForward
currently asserts browser focus yields and no-focus passes, but misses asserting
that sidebar focus is excluded; update the test (function
focusHistoryNavigationYieldsToBrowserBackForward) to also check that for each
action in [ShortcutAction.focusHistoryBack, .focusHistoryForward] the clause
(action.defaultFocusWhenClause) evaluates to false when given
ShortcutFocusState(browser: false, markdown: false, sidebar: true) so the
.not(.atom(.sidebarFocus)) part is covered.
teamleaderleo
added
S3: minor
This branch has not been deployed
2 participants
Summary
Adds a regression test that sweeps every factory-default shortcut for an unresolved chord collision, and fixes the one genuine collision it surfaced.
Two actions may share a default chord only when their effective
when-contexts are disjoint (the same keystroke drives each in a different focus) or router priority picks a deterministic winner. Anything else is a shipped conflict: one action shadows the other and the duplicate chord is unreachable. This is the class behind #3467 (Rename Tab / Reload Page on ⌘R) and #5810 — but until now nothing guarded against re-introducing it.The sweep found that
focusHistoryBack/focusHistoryForward(⌘[ / ⌘]) collide with the browser pane's Back / Forward. Focus-history navigation ships.application-scoped, so while a browser panel is focused both it andbrowserBack/browserForward(browser-scoped) match the keystroke. The browser's own Back/Forward is the natural owner of ⌘[ / ⌘] there.Note on #5810
The pair the reporter screenshotted — Zoom In/Out vs Markdown Viewer: Zoom In/Out on ⌘= / ⌘- — is not a runtime collision: browser-zoom is scoped to
browserFocusand markdown-zoom tomarkdownFocus, which never co-occur, so the sweep correctly treats them as non-colliding. That half of #5810 is a Settings-display matter (no context badge on context-scoped duplicates), not a binding conflict. The sweep instead caught a real one elsewhere. The non-US-layout zoom-reachability half of #5810 is a separate keystroke-resolution concern and is out of scope here.Changes
ShortcutDefaultCollisionTests) — group actions by theirdefaultShortcut, and for every shared chord assert the pair does notbindingsCollideunder theirdefaultFocusWhenClause+ priority routing, except an explicitintentionalOverlapsallowlist. The allowlist is asserted to stay tight (no stale entries).focusHistoryBack/focusHistoryForwardmove to the non-browser-panel context, mirrored in bothShortcutAction.defaultFocusWhenClause(CmuxSettings) and the app target'sKeyboardShortcutContext.shortcutContextso the existing drift test stays aligned. Outside a browser panel they still navigate focus history; inside one they yield ⌘[ / ⌘] to the browser. The chord stays rebindable in Settings.Intentional overlap (allowlisted, not changed)
groupSelectedWorkspaces⇄toggleReactGrab(⌘⇧G) intentionally share a chord and are resolved at runtime — the group handler propagates the event when there are no eligible workspaces, so React Grab still fires where grouping would no-op. Documented inKeyboardShortcutSettings.swift; the test allowlists exactly this pair.Two-commit red/green
Per
AGENTS.md:Verification
swift testonPackages/CmuxSettings(Xcode 26 toolchain): full suite green (70 tests, 9 suites); the new suite red at commit 1 as above.I did not build the full app target locally (it needs the GhosttyKit xcframework / VM setup from
CONTRIBUTING.md). The app-target edit is a two-line addition to the existing.nonBrowserPanelcase ofshortcutContext, and the package and app mappings resolve to the identicalwhen-clause, sotestSettingsPackageDefaultWhenClausesMatchRuntimeShortcutContextsstays aligned by construction — flagging it for CI to confirm.Relates to #5810.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a default shortcut collision where ⌘[ / ⌘] for focus history conflicted with the browser pane’s Back/Forward, and adds a regression test to prevent future default-chord collisions.
Bug Fixes
focusHistoryBack/focusHistoryForwardto non-browser panels in bothCmuxSettingsdefaults and runtimeKeyboardShortcutContext, so the browser owns ⌘[ / ⌘] when a browser pane is focused; behavior outside browser panels is unchanged and shortcuts remain rebindable.New Features
ShortcutDefaultCollisionTeststo sweep factory defaults for unresolved chord collisions, with a minimal allowlist for the intentionalgroupSelectedWorkspaces⇄toggleReactGraboverlap (⌘⇧G).Written for commit 3b2e72e. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests