Open selected file explorer item from keyboard#6036
Open
azooz2003-bit wants to merge 7 commits into
Open
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:
📝 WalkthroughWalkthroughThis PR adds a new keyboard shortcut ChangesFile Explorer Open Selection Shortcut
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR implements keyboard activation for the file explorer: Return/keypad-Enter open files or toggle folders while the explorer is focused, and a new configurable
Confidence Score: 5/5Safe to merge — the change is well-scoped, all locales are covered, the shared activation path is consistent, and the when-clause correctly gates the configured shortcut to sidebar focus. The keyboard activation logic is straightforward: Return/keypad-Enter are intercepted before quick-search and always owned by the view, the configured ⌘↓ shortcut respects the existing sidebar when-clause, and the refactored activateNode path unifies double-click and keyboard without behavioral regression. Localization is complete across all 20 web locales and all Xcode xcstrings locales. Tests cover the added code paths. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
KD[keyDown / performKeyEquivalent] --> MSC{Mode shortcut?}
MSC -- yes --> HMS[handleModeShortcut]
HMS --> DONE[Consumed]
MSC -- no --> OSS{handleOpenSelectionShortcut?}
OSS -- "Return / KPEnter\nor matches ⌘↓ + whenClause" --> EQS[endQuickSearch]
EQS --> OSI[openSelectedItem]
OSI --> RSR[resolvedSelectionRow]
RSR --> OVR{outlineView.selectedRow ≥ 0?}
OVR -- yes --> AN["activateNode\n→ selectRow"]
AN --> DIR{isDirectory?}
DIR -- yes --> TOG[expand / collapse]
DIR -- no --> LP{LocalFileExplorerProvider?}
LP -- yes --> OFP[onOpenFilePreview]
LP -- no --> NOOP["no-op, return true"]
OVR -- no --> STR{store.selectedPath?}
STR -- found in outline --> AN
STR -- not found --> RET_FALSE[return false]
OSS -- no match --> QSA{quickSearchActive?}
QSA -- yes --> QSK[handleQuickSearchKey]
QSA -- no --> NAV[nav / disclosure / slash / super]
Reviews (5): Last reviewed commit: "Run CI display setup before AppKit start..." | Re-trigger Greptile |
![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 `@Sources/FileExplorerOutlineView.swift`:
- Around line 148-154: The handler handleOpenSelectionShortcut currently fires
the openFileExplorerSelection action without checking the action’s effective
when clause; update handleOpenSelectionShortcut to first verify the action is
allowed (same check used by the mode-shortcut path) before calling
endQuickSearch() and fileExplorerCoordinator?.openSelectedItem(in:);
specifically, call the existing shortcut/permission evaluation helper used
elsewhere (or evaluate the action's .when predicate for
openFileExplorerSelection in the current context) and return false if that check
fails so the action only runs when its when-clause permits it.
🪄 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: 2157a143-16a8-489f-8f22-d289db370152
📒 Files selected for processing (7)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swiftSources/FileExplorerOutlineView.swiftSources/FileExplorerView.swiftSources/KeyboardShortcutSettings.swiftcmux.xcodeproj/project.pbxprojcmuxTests/FileExplorerKeyboardActivationTests.swiftcmuxTests/WorkspaceUnitTests.swift
💤 Files with no reviewable changes (2)
- cmuxTests/WorkspaceUnitTests.swift
- Sources/FileExplorerView.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+148
to
+154
| private func handleOpenSelectionShortcut(_ event: NSEvent) -> Bool { | ||
| guard FileExplorerKeyboardActivation.matchesOpenSelectionShortcut(event) else { | ||
| return false | ||
| } | ||
| endQuickSearch() | ||
| return fileExplorerCoordinator?.openSelectedItem(in: self) ?? false | ||
| } |
Contributor
There was a problem hiding this comment.
selectedRow == -1 and store.selectedPath == nil), handleOpenSelectionShortcut calls endQuickSearch() — setting quickSearchActive = false — and then returns false because openSelectedItem found no row. The caller now sees quickSearchActive == false, so if quickSearchActive, handleQuickSearchKey(event) short-circuits and the Return event falls all the way through to super.keyDown, triggering a system alert beep. Before this PR, handleQuickSearchKey consumed Return unconditionally when quick search was active. The intent is clearly that any Return/keypad-Enter event is fully owned by this view when it matches matchesOpenSelectionShortcut. Returning true regardless of whether an item was actually opened fixes the beep while keeping the open-if-possible behaviour.
Suggested change
| private func handleOpenSelectionShortcut(_ event: NSEvent) -> Bool { | |
| guard FileExplorerKeyboardActivation.matchesOpenSelectionShortcut(event) else { | |
| return false | |
| } | |
| endQuickSearch() | |
| return fileExplorerCoordinator?.openSelectedItem(in: self) ?? false | |
| } | |
| private func handleOpenSelectionShortcut(_ event: NSEvent) -> Bool { | |
| guard FileExplorerKeyboardActivation.matchesOpenSelectionShortcut(event) else { | |
| return false | |
| } | |
| endQuickSearch() | |
| _ = fileExplorerCoordinator?.openSelectedItem(in: self) | |
| return true | |
| } |
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 @.github/workflows/ci.yml:
- Around line 634-643: Replace the fragile "sleep 3" + "kill -0" readiness check
by invoking the helper with its --ready-path and --display-id-path flags (from
scripts/create-virtual-display.m), write the helper's PID to VDISPLAY_PID as
before, then poll for the ready file (with a timeout) and read the display id
from the display-id file into the environment (echo "VDISPLAY_ID=..." >>
"$GITHUB_ENV"); if the ready file isn't created in time, kill the helper and
fail. Ensure you still export VDISPLAY_PID to GITHUB_ENV and keep robust cleanup
on timeout.
🪄 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: 297f793a-1c4d-4abb-a322-14bc2ca1be70
📒 Files selected for processing (1)
.github/workflows/ci.yml
Comment on lines
+634
to
+643
| - name: Create virtual display | ||
| run: | | ||
| set -euo pipefail | ||
| clang -framework Foundation -framework CoreGraphics \ | ||
| -o /tmp/create-virtual-display scripts/create-virtual-display.m | ||
| /tmp/create-virtual-display & | ||
| VDISPLAY_PID=$! | ||
| echo "VDISPLAY_PID=$VDISPLAY_PID" >> "$GITHUB_ENV" | ||
| sleep 3 | ||
| kill -0 "$VDISPLAY_PID" |
There was a problem hiding this comment.
Virtual display readiness check is fragile; consider using helper's signal files.
The sleep 3 + kill -0 pattern only confirms the helper process is running, not that the virtual display was successfully created. The helper supports --ready-path and --display-id-path flags (see scripts/create-virtual-display.m) which signal when the display is actually ready. The same workflow in test-e2e.yml uses these for robust polling.
Since this step was moved earlier, both the CoreAnimation regression and workspace churn typing-lag regression now depend on it. If display creation takes longer than 3 seconds on a loaded runner, downstream steps could fail intermittently.
🛡️ Suggested improvement using readiness files
- name: Create virtual display
run: |
set -euo pipefail
clang -framework Foundation -framework CoreGraphics \
-o /tmp/create-virtual-display scripts/create-virtual-display.m
- /tmp/create-virtual-display &
+ VDISPLAY_READY="$RUNNER_TEMP/cmux-virtual-display.ready"
+ VDISPLAY_ID_PATH="$RUNNER_TEMP/cmux-virtual-display.id"
+ rm -f "$VDISPLAY_READY" "$VDISPLAY_ID_PATH"
+ /tmp/create-virtual-display \
+ --ready-path "$VDISPLAY_READY" \
+ --display-id-path "$VDISPLAY_ID_PATH" &
VDISPLAY_PID=$!
echo "VDISPLAY_PID=$VDISPLAY_PID" >> "$GITHUB_ENV"
- sleep 3
- kill -0 "$VDISPLAY_PID"
+ for _ in $(seq 1 60); do
+ if [ -s "$VDISPLAY_READY" ]; then break; fi
+ if ! kill -0 "$VDISPLAY_PID" 2>/dev/null; then
+ echo "Virtual display helper exited before ready" >&2
+ exit 1
+ fi
+ sleep 0.5
+ done
+ if [ ! -s "$VDISPLAY_READY" ]; then
+ echo "Timed out waiting for virtual display" >&2
+ exit 1
+ fi
+ echo "Virtual display ready"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Create virtual display | |
| run: | | |
| set -euo pipefail | |
| clang -framework Foundation -framework CoreGraphics \ | |
| -o /tmp/create-virtual-display scripts/create-virtual-display.m | |
| /tmp/create-virtual-display & | |
| VDISPLAY_PID=$! | |
| echo "VDISPLAY_PID=$VDISPLAY_PID" >> "$GITHUB_ENV" | |
| sleep 3 | |
| kill -0 "$VDISPLAY_PID" | |
| - name: Create virtual display | |
| run: | | |
| set -euo pipefail | |
| clang -framework Foundation -framework CoreGraphics \ | |
| -o /tmp/create-virtual-display scripts/create-virtual-display.m | |
| VDISPLAY_READY="$RUNNER_TEMP/cmux-virtual-display.ready" | |
| VDISPLAY_ID_PATH="$RUNNER_TEMP/cmux-virtual-display.id" | |
| rm -f "$VDISPLAY_READY" "$VDISPLAY_ID_PATH" | |
| /tmp/create-virtual-display \ | |
| --ready-path "$VDISPLAY_READY" \ | |
| --display-id-path "$VDISPLAY_ID_PATH" & | |
| VDISPLAY_PID=$! | |
| echo "VDISPLAY_PID=$VDISPLAY_PID" >> "$GITHUB_ENV" | |
| for _ in $(seq 1 60); do | |
| if [ -s "$VDISPLAY_READY" ]; then break; fi | |
| if ! kill -0 "$VDISPLAY_PID" 2>/dev/null; then | |
| echo "Virtual display helper exited before ready" >&2 | |
| exit 1 | |
| fi | |
| sleep 0.5 | |
| done | |
| if [ ! -s "$VDISPLAY_READY" ]; then | |
| echo "Timed out waiting for virtual display" >&2 | |
| exit 1 | |
| fi | |
| echo "Virtual display ready" |
🤖 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 @.github/workflows/ci.yml around lines 634 - 643, Replace the fragile "sleep
3" + "kill -0" readiness check by invoking the helper with its --ready-path and
--display-id-path flags (from scripts/create-virtual-display.m), write the
helper's PID to VDISPLAY_PID as before, then poll for the ready file (with a
timeout) and read the display id from the display-id file into the environment
(echo "VDISPLAY_ID=..." >> "$GITHUB_ENV"); if the ready file isn't created in
time, kill the helper and fail. Ensure you still export VDISPLAY_PID to
GITHUB_ENV and keep robust cleanup on timeout.
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
openFileExplorerSelectionto shortcut settings, cmux.json schema support, Settings UI ordering, localization, and docs.Issue
Acceptance Criteria
Verification
git diff --check./scripts/lint-pbxproj-test-wiring.shswift testinPackages/CmuxSettingsbun run lintinweb(passed with existing warnings)SKIP_ENV_VALIDATION=1 bun run buildinweb./scripts/reload-cloud.sh --tag fexopenattempted cloud builder, fell back because no macfleet manifest/no slot./scripts/reload.sh --tag fexopenDogfood Preflight
fexopen/Users/abdulazizalbahar/Dev/Manaflow/cmuxterm-hq/cmux-assets/issue-5996-file-explorer-return-open/fixture/Users/abdulazizalbahar/Dev/Manaflow/cmuxterm-hq/cmux-assets/issue-5996-file-explorer-return-open/auto-feature/20260612-222230/preflight/before-open.png| TLDR: File explorer focused on fixture withSourcesselected./Users/abdulazizalbahar/Dev/Manaflow/cmuxterm-hq/cmux-assets/issue-5996-file-explorer-return-open/auto-feature/20260612-222230/preflight/return-expanded-folder.png| TLDR: Return expanded the selectedSourcesfolder./Users/abdulazizalbahar/Dev/Manaflow/cmuxterm-hq/cmux-assets/issue-5996-file-explorer-return-open/auto-feature/20260612-222230/preflight/return-opened-appswift.png| TLDR: Return opened selectedApp.swiftinto a file preview./Users/abdulazizalbahar/Dev/Manaflow/cmuxterm-hq/cmux-assets/issue-5996-file-explorer-return-open/auto-feature/20260612-222230/preflight/cmd-down-opened-readme.png| TLDR: Cmd-Down opened selectedREADME.mdinto a markdown preview.Baseline Cloud Recording
cmux-loader-27457701812.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds keyboard activation for the file explorer: Return/Enter and a configurable ⌘↓ shortcut open files or toggle folders, unified with double‑click. Default is ⌘↓ (
openFileExplorerSelection); it honors right‑sidebar focus and prefers the visible outline selection, addressing #5996.New Features
openFileExplorerSelection(default ⌘↓) in Settings andcmux.json; grouped with right‑sidebar/find; docs updated; label localized.Bug Fixes
Written for commit 54d53f4. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests