Repository navigation
Add smooth Vim and Emacs viewer navigation - #7921
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 shared keyboard navigation for diff and Markdown viewers, including new scrolling and adjacent-file actions, shortcut routing, embedded JavaScript integration, localized labels, schemas, CLI mappings, and Swift/TypeScript coverage. ChangesViewer navigation shortcuts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShortcutSettings
participant MarkdownWebView
participant ViewerBridge
participant CmuxViewerNavigation
participant ViewerScroller
ShortcutSettings->>MarkdownWebView: resolve configured shortcut
MarkdownWebView->>ViewerBridge: send navigation action
ViewerBridge->>CmuxViewerNavigation: performAction(action, scroller)
CmuxViewerNavigation->>ViewerScroller: apply smooth scroll motion
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 adds shared keyboard navigation to the diff and Markdown viewers. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (38): Last reviewed commit: "Split viewer navigation support types" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/KeyboardShortcutActionContext.swift (1)
93-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
overlaps()logic is correct but dense; a matrix unit test would future-proof it.Traced every new
.viewerPanelpairing against the correspondingisAvailable/defaultWhenClausepredicates (lines 27-91) and found no mismatches — the branch ordering and exclusions are correct, including thecanvasLayoutOutsideFocusedContentinteraction. Given thecode_block_complexity_highnature of this function and that it now has 7 cases with pairwise special-casing, a small parametrized test enumerating allShortcutContext×ShortcutContextcombinations (assertingoverlapsagrees with simultaneous-truth of the underlyingisAvailableflags) would catch regressions here more cheaply than manual tracing on future edits.🤖 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 `@Sources/KeyboardShortcutActionContext.swift` around lines 93 - 133, Add a parametrized matrix unit test for ShortcutContext.overlaps, covering every ShortcutContext × ShortcutContext pairing and asserting the result matches simultaneous truth of the corresponding isAvailable predicates. Keep the existing overlaps implementation unchanged and use the test to guard future pairing or exclusion changes.
🤖 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 `@cmuxTests/MarkdownPanelTests.swift`:
- Around line 669-715: Update testMarkdownViewerUsesSmoothVimAndEmacsNavigation
to save the current KeyboardShortcutSettings, install explicit bindings for the
tested j, Ctrl-D, and Ctrl-P shortcuts before loading shellHTML, and restore the
original settings in the existing defer cleanup. Keep the test’s navigation
assertions unchanged.
In `@Sources/Panels/MarkdownViewerAssets.swift`:
- Around line 70-87: Mark the pure helper methods viewerShortcutJSONObject and
viewerShortcutStrokeJSONObject as nonisolated, since they only transform their
value arguments and do not access main-actor state. Keep the
KeyboardShortcutSettings snapshot main-actor isolated and verify StoredShortcut
and ShortcutStroke support nonisolated use.
---
Outside diff comments:
In `@Sources/KeyboardShortcutActionContext.swift`:
- Around line 93-133: Add a parametrized matrix unit test for
ShortcutContext.overlaps, covering every ShortcutContext × ShortcutContext
pairing and asserting the result matches simultaneous truth of the corresponding
isAvailable predicates. Keep the existing overlaps implementation unchanged and
use the test to guard future pairing or exclusion changes.
🪄 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: 84540e9b-826f-47ff-bbef-16e3fb672582
📒 Files selected for processing (18)
CLI/cmux_open.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/ShortcutActionNumberedDigitTests.swiftResources/Localizable.xcstringsResources/markdown-viewer/shell.htmlResources/markdown-viewer/viewer-navigation.jsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/KeyboardShortcutActionContext.swiftSources/KeyboardShortcutSettings.swiftSources/Panels/MarkdownViewerAssets.swiftcmuxTests/CMUXOpenCommandTests.swiftcmuxTests/MarkdownPanelTests.swiftweb/data/cmux-shortcuts.tsweb/data/cmux.schema.jsonwebviews/src/App.tsxwebviews/src/global.d.tswebviews/test/viewer-navigation.test.ts
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/Panels/MarkdownWebSupport.swift`:
- Around line 36-38: Remove the DEBUG-only onViewerNavigationActionForTesting
property from MarkdownWebSupport, along with any production references to it.
Preserve viewer navigation behavior and update tests to observe emitted
JavaScript or use a test subclass instead.
- Around line 71-86: Update the chord handling around
pendingViewerNavigationChordPrefix to track a 700 ms expiration deadline,
matching viewer-navigation.js. When processing a pending prefix, clear it if the
deadline has passed so the current event is evaluated as a new first stroke;
only perform the two-stroke action while the prefix remains valid, and set or
refresh the deadline when accepting a new first stroke.
🪄 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: e7830eb8-ab1c-4805-b0b0-967d377aeae6
📒 Files selected for processing (9)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftResources/markdown-viewer/shell.htmlResources/markdown-viewer/viewer-navigation.jsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Panels/MarkdownWebSupport.swiftcmuxTests/MarkdownPanelTests.swiftweb/data/cmux-shortcuts.tswebviews/src/global.d.tswebviews/test/viewer-navigation.test.ts
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 `@cmuxTests/CMUXOpenCommandTests.swift`:
- Around line 376-388: Split the tuple-based XCTAssertEqual checks in the
shortcut assertions into separate field-level assertions so they compile without
requiring (String?, Bool?) Equatable conformance. Update the checks for
diffViewerScrollDown, diffViewerScrollUp, diffViewerNextFile, and
diffViewerPreviousFile while preserving each expected key and control value.
In `@webviews/src/App.tsx`:
- Around line 330-336: Memoize the adjacent-file navigation callbacks with
useCallback: wrap scrollToItem and jumpAdjacentFile in App, preserving their
existing behavior and including only their actual dependencies, so
useKeyboardShortcuts receives a stable onJumpAdjacentFile reference and does not
recreate listeners on unrelated state updates.
🪄 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: 954aaf2f-cd86-4fb7-94a3-0f7305cc51e9
📒 Files selected for processing (17)
CLI/cmux_open.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+PriorityRouting.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/ShortcutActionNumberedDigitTests.swiftResources/Localizable.xcstringsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/KeyboardShortcutActionContext.swiftSources/KeyboardShortcutSettings.swiftSources/Panels/MarkdownWebSupport.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CMUXOpenCommandTests.swiftcmuxTests/ViewerNavigationTests.swiftweb/data/cmux-shortcuts.tsweb/data/cmux.schema.jsonwebviews/src/App.tsxwebviews/test/app.test.tsx
💤 Files with no reviewable changes (1)
- Sources/Panels/MarkdownWebSupport.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/KeyboardShortcutActionContext.swift (1)
141-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnreachable guard added to
canvasLayoutOutsideFocusedContentbranch.The new
self != .viewerPanel && other != .viewerPanelconditions at lines 169-170 can never be evaluated: the earlierif self == .viewerPanel || other == .viewerPanel { ... return ... }block (lines 141-149) unconditionally returns (it has adefault: return falsecase) whenever either side is.viewerPanel, so control never reaches thecanvasLayoutOutsideFocusedContentbranch for that pairing. The result happens to be correct (false either way), but the added condition is dead code that misrepresents the actual control flow.♻️ Proposed cleanup
if self == .canvasLayoutOutsideFocusedContent || other == .canvasLayoutOutsideFocusedContent { return self != .browserPanel && other != .browserPanel && self != .browserOrFilePreviewTextEditor && other != .browserOrFilePreviewTextEditor && self != .markdownPanel - && other != .markdownPanel - && self != .viewerPanel - && other != .viewerPanel + && other != .markdownPanel }🤖 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 `@Sources/KeyboardShortcutActionContext.swift` around lines 141 - 171, Remove the redundant self != .viewerPanel and other != .viewerPanel conditions from the canvasLayoutOutsideFocusedContent branch in the surrounding keyboard shortcut context comparison logic. The earlier viewerPanel branch already returns for any pairing involving .viewerPanel; preserve the remaining exclusions and behavior of the canvasLayoutOutsideFocusedContent check.
🤖 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 `@cmuxTests/ViewerNavigationTests.swift`:
- Around line 84-92: Strengthen the timeout test around
handleViewerNavigationKey by inspecting window.__cmuxNativeNavigationCalls after
the g@10 and g@11 presses and asserting no navigation calls occurred, confirming
the stale chord expired. Then retain the existing assertions after g@11.1 to
verify the newly armed chord completes the navigation, and ensure the 10→11
interval exceeds Self.viewerNavigationChordTimeout.
In `@Resources/markdown-viewer/webviews-app/chunks/diffSurface.mjs`:
- Line 1: Review the source implementation of CmuxViewerNavigation.install and
the existing pn file-navigation handler to determine whether both use the same
DOM and lifecycle scope. If they do, route diffViewerOpenFileSearch,
diffViewerNextFile, and diffViewerPreviousFile through the shared
CmuxViewerNavigation chord handling and remove pn’s duplicate pending-chord,
timeout, and matching logic; otherwise preserve the separation and document the
distinct scope in the implementation.
---
Outside diff comments:
In `@Sources/KeyboardShortcutActionContext.swift`:
- Around line 141-171: Remove the redundant self != .viewerPanel and other !=
.viewerPanel conditions from the canvasLayoutOutsideFocusedContent branch in the
surrounding keyboard shortcut context comparison logic. The earlier viewerPanel
branch already returns for any pairing involving .viewerPanel; preserve the
remaining exclusions and behavior of the canvasLayoutOutsideFocusedContent
check.
🪄 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: bd104525-d032-415b-93f6-b9c538189ce4
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
CLI/CMUXCLI+DiffViewerShortcuts.swiftCLI/cmux_open.swiftResources/markdown-viewer/shell.htmlResources/markdown-viewer/viewer-navigation.jsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/KeyboardShortcutActionContext.swiftSources/KeyboardShortcutSettings.swiftSources/Panels/MarkdownViewerAssets.swiftSources/Panels/MarkdownWebSupport.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ViewerNavigationTests.swiftwebviews/test/viewer-navigation.test.ts
💤 Files with no reviewable changes (2)
- Sources/Panels/MarkdownViewerAssets.swift
- Resources/markdown-viewer/shell.html
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…-navigation # Conflicts: # cmux.xcodeproj/project.pbxproj
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jul 16, 2026
* Add smooth Vim and Emacs viewer navigation * Fix Markdown viewer key routing and scroll step * Add Vim diff file navigation * Fix viewer navigation review findings * Fix viewer navigation test module import * Remove unavailable CLI test dependency * Reset Markdown smooth scroll after manual input * Reset smooth scroll for native navigation keys * Fix viewer shortcut context and chord stability * Fix command palette context availability * Fix viewer shortcut context routing * Route viewer shortcuts through native context * Navigate from visible diff file * Cache viewer bindings and preserve search input * Preserve editable Markdown input * Reset smooth target before file jumps * Extract viewer navigation helpers * Wire viewer helpers into app target * Route shortcuts in live remote diffs * Confirm live diff viewer page identity * Reset diff identity on navigation * Isolate diff viewer focus identity * Track isolated editable focus deeply * Align palette shortcut visibility context * Route all viewer key entrypoints safely * Scope viewer keys to focused webview * Split CmuxWebView support helpers * Import shortcut settings in viewer tests * Test inactive diff viewer URL trust * Trust only active diff viewer sessions * Test live HTTP diff viewer trust * Register live HTTP diff viewer sessions * Cache trusted viewer state off key path * Test cancelled diff navigation state * Restore viewer state after cancelled navigation * Close viewer navigation ordering gaps * Preserve diff search focus state * Handshake viewer navigation readiness * Test live diff viewer trust renewal * Renew active diff viewer trust * Fix merged command palette weak capture * Split viewer navigation support types (cherry picked from commit c1253dd)
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
Testing
Issues
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes focus-scoped global shortcut routing and bare-key bindings in viewers, which can steal keystrokes if context clauses drift, but scope is limited to diff/Markdown viewer focus.
Overview
Adds shared smooth keyboard navigation for diff and Markdown viewers via new
viewer-navigation.js, wired into the Markdown shell so the native app can drive the same scroll actions (j/k, Ctrl-D/U, Ctrl-N/P, G/gg) with accumulated smooth scrolling when keys repeat.Expands Settings / CLI shortcut surface: half-page and Emacs line motions,
]f/[fnext/previous file in the diff viewer, defaults centralized inCMUXCLI+DiffViewerShortcuts, and scroll bindings now apply under browser or markdown focus (file jumps stay browser/diff-focused). Scroll shortcut labels are renamed to “Viewers:” with new localizations;hasPriorityShortcutRoutingmoves to its own file and command palette next/previous join priority routing.Reviewed by Cursor Bugbot for commit 5bb76e6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds smooth Vim/Emacs scrolling to diff and Markdown viewers and
]f/[fnext/previous file navigation. Adds a readiness handshake so viewer keys only route when loaded and not in inputs; tightens diff viewer trust with live HTTP session renewal; fixes visible-order file jumps and preserves search/input focus.New Features
viewer-navigation.jsadds line, half-page, and edge motions with held-key accumulation; exposed as__cmuxPerformViewerNavigationAction(Markdown) and__cmuxPerformDiffViewerNavigationAction(diff).j/k,Ctrl-D/U,Ctrl-N/P,G/gg; diff-only]f/[ffrom the visible file.CMUXCLI+DiffViewerShortcuts.Bug Fixes
ViewerNavigationKeyRouterto the focused webview; chords stable; bare keys scoped; ignored while typing; command palette next/previous use priority routing with the “effectively visible” context.Written for commit 27b664e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests