Repository navigation
Add a configurable keyboard shortcut to open the diff viewer - #5178
Merged
Merged
Conversation
cmux already opens the diff viewer via the `cmux diff` CLI and the "Open Diff Viewer" command-palette entry (which spawns that CLI). This adds a first-class, user-rebindable keyboard shortcut that opens the diff viewer for the focused workspace through the same shared path, with no duplicated diff-open logic. - Register an `openDiffViewer` action in KeyboardShortcutSettings (label "Open Diff Viewer"); it is automatically visible/editable in Settings and configurable via ~/.config/cmux/cmux.json. - Default Cmd+Ctrl+D: Cmd+Shift+D collides with Split Down and the whole Cmd-based "D" family is taken by split actions, so use Cmd+Ctrl+D, which is free and matches cmux's Cmd+Ctrl+<letter> app-shortcut family (Close Window, Toggle Full Screen). - Extract the command palette's diff launcher into a shared AppDelegate.openDiffViewerForFocusedWorkspace(for:); the palette and the shortcut both call it (spawning `cmux diff`). Lift cwd resolution to Workspace.resolvedWorkingDirectory(). - Update docs (web/data/cmux-shortcuts.ts feeds both the keyboard shortcut and configuration docs pages) and add en/ja localization. - Add a dispatch test asserting Cmd+Ctrl+D routes to the shared path and that the default is conflict-free. Closes #5169 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable Cmd+Ctrl+Shift+D keyboard shortcut and central AppDelegate entrypoint to open the diff viewer for the focused workspace; implements subprocess launch/retention, workspace cwd resolution, command-palette wiring, a routing test, and localization/web shortcut metadata. ChangesOpen Diff Viewer Keyboard Shortcut
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 (16 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 SummaryAdds a configurable
Confidence Score: 5/5Safe to merge — the change is a straightforward shortcut wiring and subprocess-launch refactor with no new data paths or auth surfaces. The diff-launch logic is correctly centralized, actor isolation is handled (termination-handler mutations go through Task @mainactor), all 20 app locales are covered, and the test seam prevents subprocess spawning in CI. No correctness issues found in the changed paths. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppKit as AppKit Event Monitor
participant AD as AppDelegate
participant WS as Workspace
participant CLI as cmux diff CLI
Note over User,CLI: Keyboard shortcut path (new)
User->>AppKit: Cmd+Ctrl+Shift+D key event
AppKit->>AD: handleCustomShortcut(event:)
AD->>AD: matchConfiguredShortcut(.openDiffViewer)
AD->>AD: openDiffViewerForFocusedWorkspace(for: tabManager)
AD->>WS: resolvedWorkingDirectory()
WS-->>AD: cwd string
AD->>AD: launchDiffViewerProcess(...)
AD->>CLI: Process.run()
CLI-->>AD: terminationHandler (background)
AD->>AD: "Task @MainActor remove from diffViewerProcesses"
Note over User,CLI: Command palette path (unchanged)
User->>AD: palette.openDiffViewer
AD->>AD: openDiffViewerForFocusedWorkspace(for: tabManager)
AD->>CLI: same launchDiffViewerProcess path
Reviews (7): Last reviewed commit: "Fix Open Diff Viewer: show in Settings U..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 5945-5956: The debug logging in the openDiffViewer exit path
(around outputCollector.finish(), terminatedProcess.processIdentifier and
terminatedProcess.terminationStatus) currently emits raw child process output
and workspace paths via cmuxDebugLog; change it to avoid any raw stdout/stderr
or cwd strings and instead log only non-sensitive metadata: terminationStatus,
processIdentifier, and a byte/character count of the trimmed output (e.g.,
output.count or limitedDetail.count), and remove any direct detail or path
contents; apply the same change to the other similar cmuxDebugLog usage that
logs diff-viewer output so both places report status, ids and size only.
In `@web/data/cmux-shortcuts.ts`:
- Around line 223-227: The shortcut entry openDiffViewer in cmux-shortcuts.ts
currently provides description only for en and ja so other locales fall back to
English; update localization by moving the description strings into next-intl
message keys (e.g., "shortcuts.openDiffViewer") and replace the hardcoded
description object in openDiffViewer with a reference that resolves via the same
i18n pipeline used by keyboard-shortcuts.tsx, or alternatively add explicit
localized strings for every supported locale from your routing list; ensure
keyboard-shortcuts.tsx reads the message key for rendering so all locales get
proper translations rather than defaulting to English.
🪄 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: c3e3544f-ae9c-4990-a69d-e89d9ad8eae6
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/Workspace.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftweb/data/cmux-shortcuts.ts
…g logs - Populate all 20 catalog locales for shortcut.openDiffViewer.label, reusing command.openDiffViewer.title's existing translations (identical English source). [greptile P1] - resolvedWorkingDirectory() now delegates to the existing configTrackingDirectory(for:) helper instead of duplicating the candidate-resolution logic. [cubic P3] - Diff-viewer DEBUG logs report only status, pid, and an output byte count — no raw child output or workspace cwd. [coderabbit] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 6026-6028: The debug log in the openDiffViewer failure currently
prints error.localizedDescription which may contain file paths; change the log
to avoid sensitive content by logging only the error type (e.g., type(of:
error)) and any already-redacted metadata (do not include raw startup commands,
cwd, or localizedDescription). Update the call to cmuxDebugLog in the
openDiffViewer failure path to emit a non-sensitive message containing the
error's type and contextual, non-private flags/ids/byte counts instead of
error.localizedDescription.
In `@Sources/Workspace.swift`:
- Around line 11593-11597: resolvedWorkingDirectory() currently returns only
configTrackingDirectory(for: focusedPanelId); update it to implement the
documented 3-tier fallback: if configTrackingDirectory(for: focusedPanelId) is
non-nil return it, otherwise try terminalRequestedDirectory(for:
focusedPanelId), and if that is nil return the workspace property
currentDirectory (or nil if currentDirectory is nil) so the function returns the
first non-nil of configTrackingDirectory(for:),
terminalRequestedDirectory(for:), and currentDirectory.
🪄 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: a52699a1-cb36-4826-bde6-d066dd7e1cec
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/Workspace.swift
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ebug seam `diffViewerProcesses` and the `debugOpenDiffViewerHandler` test seam were declared inside AppDelegate's `#if DEBUG` block, but `openDiffViewerForFocusedWorkspace`/`launchDiffViewerProcess` referenced them unconditionally — so the Release build failed with "cannot find 'debugOpenDiffViewerHandler' in scope" and "no member 'diffViewerProcesses'" (release-build CI job; the Debug `tests` build compiled fine, which is why it slipped through initially). - `diffViewerProcesses` is production state (CLI process retention) → declared outside `#if DEBUG`. - `debugOpenDiffViewerHandler` stays a DEBUG-only seam; its use is now `#if DEBUG`-gated, matching the existing `debugCloseMainWindowConfirmationHandler` pattern. Verified with a local `xcodebuild -configuration Release` (arm64): the cmux app target compiles cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ailure Process spawn failures surface file paths via error.localizedDescription (e.g. "The file doesn't exist at /path/to/bin/cmux"). Consistent with the rest of this DEBUG log's path/output redaction, log type(of: error) instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ngDirectory CodeRabbit (blocking) flagged that the delegating one-liner didn't visibly honor the documented three-tier order. Spell the tiers out in the method body — panelDirectories[focusedPanelId] -> focused terminal's requestedWorkingDirectory -> currentDirectory -> nil — matching configTrackingDirectory(for:)'s behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 c79752d. Configure here.
…'t eat Two real bugs the first pass missed (verified by driving the running app): 1. Settings UI: the keyboard-shortcuts pane is rendered from the CmuxSettings.ShortcutAction package enum, NOT KeyboardShortcutSettings.Action. `openDiffViewer` was never added there, so "Open Diff Viewer" never appeared in Settings and couldn't be rebound in the UI. Add the case (+ group, displayName, defaultStroke) and a search synonym so "open diff" surfaces the Keyboard Shortcuts section. 2. Default shortcut: macOS reserves Cmd+Ctrl+D for "Look Up & data detectors" and swallows the key before it reaches the app's monitor (confirmed: a Cmd+Ctrl+D keystroke never reaches handleCustomShortcut, while Cmd+, does). The rest of the Cmd+D family is taken by split actions, so move the default to Cmd+Ctrl+Shift+D, which reaches cmux and opens the viewer (verified end-to-end). Updates the dispatch test and the keyboard-shortcut/configuration docs accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review All previously-flagged items are addressed in the latest commits:
Also fixed two functional bugs since: the shortcut now appears in the Settings UI (added to the CmuxSettings.ShortcutAction registry), and the default moved off the macOS-reserved Cmd+Ctrl+D to Cmd+Ctrl+Shift+D. Please re-review. |
|
✅ Actions performedFull review triggered. |
Merged
This was referenced Jun 8, 2026
This was referenced Jun 12, 2026
Browser back/forward get cmd+opt+[ / cmd+opt+]; cmd+[ / cmd+] stay focus history in every pane
#5939
Closed
Closed
This was referenced Jun 19, 2026
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
Adds a configurable keyboard shortcut to open the diff viewer for the focused workspace, rebindable in Settings like every other cmux shortcut.
cmux already opens the diff viewer via the
cmux diffCLI and the Open Diff Viewer command-palette entry (which spawns that CLI). This wires up a first-class keyboard shortcut that goes through the same shared diff-open path — no duplicated diff-open logic.What changed
openDiffVieweraction inKeyboardShortcutSettings.Action(label "Open Diff Viewer"). It's automatically visible/editable in Settings → Keyboard Shortcuts (settingsVisibleActions+ShortcutSettingRow) and configurable via~/.config/cmux/cmux.json(shortcuts.openDiffViewer) throughKeyboardShortcutSettingsFileStore.AppDelegate.openDiffViewerForFocusedWorkspace(for:). Both the command palette and the new shortcut now funnel through it (it spawnscmux diff --unstaged --workspace … --focus truefor the focused workspace). The per-workspace working-directory resolution was lifted toWorkspace.resolvedWorkingDirectory()and reused by the existing configured-action path too.AppDelegate.handleCustomShortcutgets amatchConfiguredShortcut(event:action:.openDiffViewer)branch that opens the diff viewer for the event window's focused workspace (beeps on failure, matching the palette).Default shortcut:
Cmd+Ctrl+DThe issue proposed
Cmd+Shift+D("D" for Diff), but that collides with Split Down. The entire Cmd-based "D" family is already taken by split actions:Cmd+DCmd+Shift+DCmd+Opt+DCmd+Shift+Opt+DCmd+Ctrl+Dis free, keeps the "D for Diff" mnemonic, and matches cmux's existingCmd+Ctrl+<letter>app-shortcut family (Cmd+Ctrl+WClose Window,Cmd+Ctrl+FToggle Full Screen). The added dispatch test asserts the default is conflict-free.Docs & localization
web/data/cmux-shortcuts.tsgains anopenDiffViewerentry (renders into both the keyboard-shortcuts docs page and theshortcuts.bindingsconfiguration docs table automatically).shortcut.openDiffViewer.label(en + ja) toResources/Localizable.xcstrings.Tests
Added
testOpenDiffViewerShortcutDefaultsToCmdCtrlDAndRoutesToSharedDiffPathtocmuxTests/AppDelegateShortcutRoutingTests.swift: dispatches aCmd+Ctrl+Dkey event throughhandleCustomShortcutand asserts it routes to the shared diff-open path (via adebugOpenDiffViewerHandlertest seam, so no subprocess is spawned), the default resolves toCmd+Ctrl+D, it's conflict-free, and it's insettingsVisibleActions.Closes #5169
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
UI and shortcut wiring around an existing CLI spawn path; no auth or data-model changes, with minor subprocess-launch surface consolidated from the palette.
Overview
Adds a first-class Open Diff Viewer keyboard shortcut (default ⌃⌘⇧D) that opens unstaged diffs for the focused workspace via the bundled
cmux diffCLI, rebindable in Settings andcmux.jsonlike other shortcuts.⌃⌘⇧D avoids macOS swallowing plain ⌃⌘D (“Look Up”) and conflicts with existing ⌘D split chords. Settings,
ShortcutAction, docs (cmux-shortcuts.ts), search synonyms, and broadshortcut.openDiffViewer.labellocalizations are wired through.Diff launching is centralized in
AppDelegate.openDiffViewerForFocusedWorkspace(for:)with subprocess retention and safer DEBUG logging; the command palette and the new shortcut both call it.ContentViewdrops its duplicate launcher.Workspace.resolvedWorkingDirectory()picks the cwd for diff/commands (focused panel → terminal request → workspace directory).Failures beep, matching the palette. A routing test uses
debugOpenDiffViewerHandlerso no real subprocess runs.Reviewed by Cursor Bugbot for commit f5c3051. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a configurable keyboard shortcut to open the diff viewer for the focused workspace through the same shared path as the command palette. Default is Cmd+Ctrl+Shift+D to avoid macOS “Look Up” and split-view conflicts (addresses #5169).
New Features
openDiffVieweraction, visible in Settings → Keyboard Shortcuts and configurable via~/.config/cmux/cmux.json(shortcuts.openDiffViewer); default Cmd+Ctrl+Shift+D with palette-style beep on failure.web/data/cmux-shortcuts.ts) and added label localization across all catalog locales.Refactors
AppDelegate.openDiffViewerForFocusedWorkspace(...)(spawnscmux diff --unstaged ...), with subprocess retention in production and sanitized DEBUG logs (pid/status/output-bytes); palette now calls this shared path. Fixed Release build by hoistingdiffViewerProcessesout of#if DEBUGand gatingdebugOpenDiffViewerHandlerbehind DEBUG.Workspace.resolvedWorkingDirectory()with an explicit fallback order (focused panel’s tracked directory → its terminal’s requested directory → workspace’s current directory). Updated dispatch test to assert the Cmd+Ctrl+Shift+D default, no conflicts, and shared-path routing.Written for commit f5c3051. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests