Repository navigation
Improve file explorer search navigation - #6968
Closed
austinywang wants to merge 32 commits into
Closed
austinywang wants to merge 32 commits into
austinywang wants to merge 32 commits into
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:
📝 WalkthroughWalkthroughFile preview open callbacks throughout the file explorer, sidebar, and right sidebar panel now carry optional line/column position, propagated through a new ChangesFile preview navigation and recursive watching
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FileExplorerPanelView.Coordinator
participant RightSidebarToolPanel
participant Workspace
participant FilePreviewPanel
FileExplorerPanelView.Coordinator->>RightSidebarToolPanel: onOpenFilePreview(path, lineNumber, columnNumber)
RightSidebarToolPanel->>Workspace: openFileSurfacesNavigatingTextPosition(filePath, lineNumber, columnNumber)
Workspace->>FilePreviewPanel: openFileSurfaces(filePath)
Workspace->>FilePreviewPanel: navigateToTextPosition(lineNumber, columnNumber)
FilePreviewPanel->>FilePreviewPanel: applyPendingTextNavigationIfReady() (after text load)
sequenceDiagram
participant FileExplorerStore
participant RecursivePathWatcher
participant FileSystemEventStream
participant GitStateWatcher
FileExplorerStore->>RecursivePathWatcher: updateDirectoryWatcher(rootPath, excludedPaths)
RecursivePathWatcher->>FileSystemEventStream: init(paths, excludedPaths)
FileSystemEventStream-->>FileExplorerStore: tree change event
FileExplorerStore->>FileExplorerStore: reload() + refreshGitStatus()
FileExplorerStore->>GitStateWatcher: startGitStateWatcher(gitMetadataDirectory)
GitStateWatcher-->>FileExplorerStore: metadata change event
FileExplorerStore->>FileExplorerStore: refreshGitStatus() (coalesced, generation-checked)
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 improves file-explorer search navigation by opening search results at the exact line and UTF-8 byte column in the cmux preview, upgrades the local directory watcher from a flat
Confidence Score: 4/5The navigation logic, watcher coalescing, and generation-guard are well-implemented, but a named The
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Search result selected] --> B[openSelectedSearchResult]
B --> C{Local provider?}
C -->|Yes| D[performFileExplorerFileOpen\npath + lineNumber + columnNumber]
C -->|No| E[onOpenFilePreview\npath + lineNumber + columnNumber]
D --> E
E --> F[openFilePreviewFromSidebar\nor RightSidebarToolPanel.openFilePreview]
F --> G{SSH provider?}
G -->|Yes| H[materializeRemoteFileForPreview\ndownload to local URL]
G -->|No| I[openFileSurfacesNavigatingTextPosition]
H --> I
I --> J[openFileSurfaces\nroutes to correct panel type]
J --> K{Panel type?}
K -->|FilePreviewPanel| L[navigateToTextPosition\nlineNumber + columnNumber]
K -->|MarkdownPanel| M[no-op]
K -->|ProjectPanel| N[no-op]
L --> O{Text already loaded?}
O -->|Yes + textView attached| P[applyPendingTextNavigationIfReady\nscroll + select]
O -->|No| Q[store as pendingTextNavigation]
Q --> R{File loads} --> P
Q --> S{textView attaches} --> P
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Search result selected] --> B[openSelectedSearchResult]
B --> C{Local provider?}
C -->|Yes| D[performFileExplorerFileOpen\npath + lineNumber + columnNumber]
C -->|No| E[onOpenFilePreview\npath + lineNumber + columnNumber]
D --> E
E --> F[openFilePreviewFromSidebar\nor RightSidebarToolPanel.openFilePreview]
F --> G{SSH provider?}
G -->|Yes| H[materializeRemoteFileForPreview\ndownload to local URL]
G -->|No| I[openFileSurfacesNavigatingTextPosition]
H --> I
I --> J[openFileSurfaces\nroutes to correct panel type]
J --> K{Panel type?}
K -->|FilePreviewPanel| L[navigateToTextPosition\nlineNumber + columnNumber]
K -->|MarkdownPanel| M[no-op]
K -->|ProjectPanel| N[no-op]
L --> O{Text already loaded?}
O -->|Yes + textView attached| P[applyPendingTextNavigationIfReady\nscroll + select]
O -->|No| Q[store as pendingTextNavigation]
Q --> R{File loads} --> P
Q --> S{textView attaches} --> P
Reviews (26): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/FileExplorerStore.swift (1)
892-910: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftRecursive root watching can turn subtree churn into repeated explorer reloads.
RecursivePathWatcher(paths: [rootPath])here listens to every descendant change, and this path doesn't filter ignored/high-churn directories. A busy tree can keep retriggeringreload()and kicking off new git-status fetches every coalesced event, repeatedly tearing down and rebuilding the explorer on the main actor. Scope the watch or drop events from.git,node_modules, and build-output subtrees before reloading.🤖 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/FileExplorerStore.swift` around lines 892 - 910, The recursive watcher setup in updateDirectoryWatcher() is reacting to every descendant change under rootPath, which can cause repeated reload() and refreshGitStatus() churn on busy trees. Update the FileExplorerStore watcher logic to scope RecursivePathWatcher or filter its events so high-churn subtrees like .git, node_modules, and build-output directories are ignored before triggering reloads. Keep the change localized to updateDirectoryWatcher() and the watcher event loop so the explorer only refreshes for relevant file changes.Source: Path instructions
Sources/ContentView.swift (1)
2305-2346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated preview-open + navigate logic across two surfaces.
openFilePreviewFromSidebarhere is nearly identical toRightSidebarToolPanel.openFilePreview(_:lineNumber:columnNumber:)(remote materialize →openFilePreviewSurfaces→navigateToTextPosition; locallineNumber != nilbranch vsopenFileSurfaces). Two copies of this branching will drift over time. Consider extracting a sharedWorkspacehelper (e.g.openFilePreviewSurfacesNavigating(inPane:filePaths:lineNumber:columnNumber:remoteMaterialize:)) so both entry points route through one path, per the repo's shared-behavior policy for multi-entrypoint actions.🤖 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/ContentView.swift` around lines 2305 - 2346, openFilePreviewFromSidebar duplicates the same preview-open-and-navigate flow that RightSidebarToolPanel.openFilePreview(_:lineNumber:columnNumber:) already uses, so the two entry points can drift. Extract the shared branching into a Workspace helper that handles remote materialization, openFilePreviewSurfaces vs openFileSurfaces, and navigateToTextPosition, then have both openFilePreviewFromSidebar and the sidebar tool panel call that single path.
🤖 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.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 2305-2346: openFilePreviewFromSidebar duplicates the same
preview-open-and-navigate flow that
RightSidebarToolPanel.openFilePreview(_:lineNumber:columnNumber:) already uses,
so the two entry points can drift. Extract the shared branching into a Workspace
helper that handles remote materialization, openFilePreviewSurfaces vs
openFileSurfaces, and navigateToTextPosition, then have both
openFilePreviewFromSidebar and the sidebar tool panel call that single path.
In `@Sources/FileExplorerStore.swift`:
- Around line 892-910: The recursive watcher setup in updateDirectoryWatcher()
is reacting to every descendant change under rootPath, which can cause repeated
reload() and refreshGitStatus() churn on busy trees. Update the
FileExplorerStore watcher logic to scope RecursivePathWatcher or filter its
events so high-churn subtrees like .git, node_modules, and build-output
directories are ignored before triggering reloads. Keep the change localized to
updateDirectoryWatcher() and the watcher event loop so the explorer only
refreshes for relevant file changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1355b07c-a807-4c13-a848-6acf03d6d79e
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (13)
Sources/ContentView.swiftSources/FileExplorerKeyboardShortcuts.swiftSources/FileExplorerStore.swiftSources/FileExplorerView.swiftSources/Panels/FilePreviewPanel.swiftSources/RightSidebarPanelView.swiftSources/RightSidebarToolPanel.swiftcmuxTests/FileExplorerShortcutSettingsTests.swiftcmuxTests/FileExplorerStoreTests.swiftcmuxTests/FilePreviewTextEditorTextKitTests.swiftcmuxTests/FileSearchRipgrepParserTests.swiftcmuxTests/HiddenRightSidebarContentMountingTests.swiftcmuxTests/WindowAndDragTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/FilePreviewTextEditorTextKitTests.swift (1)
178-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAttach the editor before exercising dirty pending navigation.
Both dirty-buffer regressions attach
SavingTextViewafter the reload, so a bug that moves an already-open dirty editor duringnavigateToTextPosition/loadTextContent(replacingDirtyContent: false)would still pass.Suggested test shape
await panel.loadTextContent().value panel.updateTextContent("dirty needle\n") `#expect`(panel.isDirty) + let textView = SavingTextView.makeFilePreviewTextView() + textView.string = panel.textContent + textView.setSelectedRange(NSRange(location: 0, length: 0)) + panel.attachTextView(textView) + panel.navigateToTextPosition(lineNumber: 1, columnNumber: 7) loader.result = FilePreviewTextLoader.Result.unavailable await panel.loadTextContent(replacingDirtyContent: false).value - let textView = SavingTextView.makeFilePreviewTextView() - textView.string = panel.textContent - textView.setSelectedRange(NSRange(location: 0, length: 0)) - panel.attachTextView(textView) - `#expect`(textView.selectedRange().location == 0)Apply the same setup-before-navigation pattern to the loaded-dirty-buffer test.
Also applies to: 211-217
🤖 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 `@cmuxTests/FilePreviewTextEditorTextKitTests.swift` around lines 178 - 185, The dirty-buffer tests currently attach SavingTextView after navigation and reload, which can hide regressions in already-open editors. Update the loaded-dirty-buffer test setup to attach the text view before calling panel.navigateToTextPosition and panel.loadTextContent(replacingDirtyContent: false), using the same pattern in FilePreviewTextEditorTextKitTests and the related dirty-buffer test block, so movement of an existing editor is actually exercised.
🤖 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/macOS/CmuxFoundation/Sources/CmuxFoundation/FileWatch/FileSystemEventStream.swift`:
- Around line 95-97: The exclusion handling in FileSystemEventStream currently
ignores the result of FSEventStreamSetExclusionPaths, so oversized ignore lists
can silently fail. Update the logic around the stream setup to check the boolean
return value, and adjust the excludedPaths strategy in FileSystemEventStream so
it stays within the FSEventStream limit by trimming or splitting the paths
before calling FSEventStreamSetExclusionPaths.
---
Outside diff comments:
In `@cmuxTests/FilePreviewTextEditorTextKitTests.swift`:
- Around line 178-185: The dirty-buffer tests currently attach SavingTextView
after navigation and reload, which can hide regressions in already-open editors.
Update the loaded-dirty-buffer test setup to attach the text view before calling
panel.navigateToTextPosition and panel.loadTextContent(replacingDirtyContent:
false), using the same pattern in FilePreviewTextEditorTextKitTests and the
related dirty-buffer test block, so movement of an existing editor is actually
exercised.
🪄 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: b25b6e7b-a718-4b1d-89a5-b3ad513cdf36
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/FileWatch/FileSystemEventStream.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/FileWatch/RecursivePathWatcher.swiftSources/ContentView.swiftSources/FileExplorerStore.swiftSources/Panels/FilePreviewWorkspaceOpenSupport.swiftSources/RightSidebarToolPanel.swiftcmuxTests/FilePreviewTextEditorTextKitTests.swift
…ve-an-ide-level-file-editor-that-is # Conflicts: # .github/swift-file-length-budget.tsv
The recursive directory watcher delivers a coalesced event roughly once per throttle window, but a sustained filesystem storm (build output, log writes, nested high-churn dirs not covered by the top-level-only FSEvents exclusion list) still drove one `refreshGitStatus()` per window. Each fetch spawned an uncancelled `git status` process, so on a large repo where the fetch outlasts the window those processes piled up unbounded. Guard `refreshGitStatus()` so at most one `git status` runs at a time and coalesce concurrent requests into a single trailing re-run. `reload()` already supersedes its own prior load via `cancelAllLoads()`, so the watcher loop now has bounded backpressure on both work items. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The main recursive watcher excludes .git (and build/dependency dirs) to avoid reload churn, but that also suppressed the events behind metadata- only git operations — commit, add, reset — which change `git status` output without touching the working tree. Badges could stay stale until an unrelated working-tree event happened. Add a second, low-churn watcher over `.git` (excluding the high-churn `objects`/`logs` subtrees) that triggers only `refreshGitStatus()`, never a tree `reload()`, since `.git` writes don't change the visible file tree. No-op when `.git` is absent or a worktree/submodule link file. Refresh the Swift file length budget for the added watcher. 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/FileExplorerStore.swift`:
- Around line 973-978: The git watcher in startGitStateWatcher(under:) only
handles .git as a directory, so it misses worktrees and submodules where .git is
a file. Update the .git detection logic to read and resolve a gitdir: pointer
when .git is a file, then use that target as the authoritative metadata
directory to watch instead of returning early. Keep the existing
FileManager-based existence check, but extend it so the watcher covers both
direct .git directories and external gitdir locations.
- Around line 916-918: Guard the git-status apply path against stale workspace
data: `applyGitStatusResult(_:)` currently updates `gitStatusByPath` without
verifying the result was fetched for the current `rootPath`, so a completion
from an old fetch can overwrite the new repository state. Capture the active
`path` when dispatching the git-status request from `FileExplorerStore`, thread
it through to `applyGitStatusResult(_:)`, and only apply the status when that
path still matches `rootPath`; otherwise discard the stale result or trigger a
fresh refresh immediately. Use the `rootPath`, `applyGitStatusResult(_:)`, and
the fetch/refresh entrypoint in `FileExplorerStore` to locate the change.
🪄 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: c9f77e86-a2fd-474c-80ab-361bb84cab65
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/FileExplorerStore.swiftSources/RightSidebarPanelView.swiftcmuxTests/HiddenRightSidebarContentMountingTests.swiftcmuxTests/WindowAndDragTests.swift
💤 Files with no reviewable changes (3)
- cmuxTests/WindowAndDragTests.swift
- cmuxTests/HiddenRightSidebarContentMountingTests.swift
- Sources/RightSidebarPanelView.swift
The in-flight coalescing gate was store-global, so on a workspace switch A -> B while A's `git status` was slow or hung: B's refresh only set the trailing flag (blocked until A returned), and A's stale result was then applied to the now-current B before the trailing refresh ran. Key each fetch to a context generation that bumps on every root/provider change. `invalidateGitStatusRefresh()` (called from setRootPath and setProvider) releases the in-flight gate so the new context's fetch starts immediately, and a completion whose generation no longer matches is dropped — so a slow/hung fetch for a previous workspace can neither block nor overwrite the active one. Same-root watcher churn stays coalesced to a single in-flight process plus one trailing refresh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
startGitStateWatcher only handled `.git` as a directory, so in Git worktrees and submodules — where `.git` is a file containing a `gitdir:` pointer to an external metadata directory — metadata-only operations (commit, add, reset) updated that external gitdir and the explorer's status badges never refreshed. Resolve the `gitdir:` pointer (absolute or relative to the root) to the authoritative metadata directory and watch that, so worktrees and submodules get the same badge-refresh behavior as plain checkouts. Add a deterministic unit test for the resolver covering plain `.git` dirs, absolute and relative gitdir pointers, missing repos, and dangling pointers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A `.git` file's `gitdir:` pointer (and a `.git` symlink) is controlled by repository contents, so a checked-out workspace could aim it at `/`, a home directory, or another large/sensitive tree and make cmux watch that recursively and re-run `git status` on unrelated churn — a trust-boundary issue. Before watching, require the resolved candidate to have the Git metadata shape: a `HEAD` file plus either `config` (main checkout or submodule) or `commondir` (linked worktree). This also rejects a `.git` symlink that resolves to a non-Git directory. Extend the resolver test with bare-`.git` and arbitrary-target rejection cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gitMetadataDirectory read a non-directory `.git` entry with String(contentsOfFile:), an unbounded synchronous read on the main actor during watcher setup. A malformed or malicious workspace with a huge `.git` regular file could hang the UI or exhaust memory just by being opened in the explorer. Read at most 64 KiB via a bounded FileHandle read — a real `gitdir:` pointer is a single short line — and bail if the pointer isn't found in that prefix. Add a regression test that a valid pointer buried past the cap is not parsed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The main tree watcher excludes `.git`, so when a non-git folder becomes a repository while open (git init, or adding a worktree/submodule) the `.git` creation isn't observed and no git-state watcher was installed — badges stayed empty until a root reset. Re-check on the next working-tree event: once `.git` resolves, install the git-state watcher so metadata changes refresh badges from then on. Cheap (a single stat per coalesced event, only while no repo is present) and self-disabling once installed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ve-an-ide-level-file-editor-that-is # Conflicts: # .github/swift-file-length-budget.tsv
FileExplorerPreviewOpenHandler took three positional arguments
(path, lineNumber, columnNumber). Every site that merely constructs the
handler — including several pre-existing XCTest suites unrelated to this
feature — had to change its closure arity, and the two Int? line/column
arguments were positionally swappable.
Collapse the handler to a single labeled-tuple parameter
(path:lineNumber:columnNumber:). Handler-constructing closures that
ignore the value revert to `{ _ in }`, matching origin/main, so the
unrelated XCTest files (FileSearchRipgrepParserTests, WindowAndDragTests)
are no longer touched by this PR.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
2331-2357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate remote/local navigate-open branching logic vs.
RightSidebarToolPanel.openFilePreview.The remote-materialize-then-navigate / local-navigate branching here is essentially identical to
RightSidebarToolPanel.openFilePreview(Sources/RightSidebarToolPanel.swift lines 93-124). Both now callopenFileSurfacesNavigatingTextPositionwith the same shape of arguments. Since this PR touches both call sites together, it's a good opportunity to extract one shared helper (e.g. aWorkspacemethod takingpaneId,filePath,lineNumber,columnNumber, and a materializer closure/store) so future navigation changes (e.g. editor support per the linked issue) don't need to be kept in sync across two files.Based on learnings, the repo's shared-behavior policy states: "When a behavior is exposed through multiple entrypoints ... implement one shared action/model path ... Do not patch one surface while leaving the others with duplicated logic."
🤖 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/ContentView.swift` around lines 2331 - 2357, The remote-vs-local open logic in the ContentView navigation path is duplicated with RightSidebarToolPanel.openFilePreview, so extract a shared helper instead of keeping two separate branches in sync. Move the materialize-then-open behavior into one reusable Workspace-level method or shared utility that accepts paneId, filePath, lineNumber, columnNumber, and the remote-materialization step, then have both ContentView and RightSidebarToolPanel call that single path. Keep the existing openFileSurfacesNavigatingTextPosition call as the common final step so future navigation changes only need to be made in one place.Source: Coding guidelines
🤖 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/FileExplorerKeyboardShortcuts.swift`:
- Line 5: Replace the labeled tuple payload used by
FileExplorerPreviewOpenHandler with a small dedicated request type, such as
FileExplorerPreviewOpenRequest, so the preview-open API is easier to extend.
Update the handler signature and all call sites that pass or consume the
preview-open payload to use the new struct. Make the new request type conform to
Equatable and Sendable to match the intended navigation/editor support growth.
In `@Sources/FileExplorerStore.swift`:
- Line 981: The git watcher setup in FileExplorerStore should be triggered from
a reliable `.git` creation signal rather than waiting for a working-tree change
that may never occur after `git init`. Update the logic around
installGitStateWatcherIfNeeded and the surrounding git-state watcher
registration so the parent/root observes the `.git` entry until it exists, then
hand off to the metadata watcher; avoid filtering out a missing `.git` path
before it can be watched. This keeps status refreshes from staying stale when
the first post-init operation is metadata-only.
- Around line 1035-1042: The `readGitPointerFile(at:)` path can still block
because it opens whatever is passed without verifying it is a regular file
first. Update the caller that decides whether to read a `.git` pointer file to
reject non-regular files before invoking `readGitPointerFile(at:)`, using
file-type checks on the path and only proceeding for regular files. Keep the fix
scoped around the `readGitPointerFile(at:)` flow and any nearby `.git` pointer
detection logic.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 2331-2357: The remote-vs-local open logic in the ContentView
navigation path is duplicated with RightSidebarToolPanel.openFilePreview, so
extract a shared helper instead of keeping two separate branches in sync. Move
the materialize-then-open behavior into one reusable Workspace-level method or
shared utility that accepts paneId, filePath, lineNumber, columnNumber, and the
remote-materialization step, then have both ContentView and
RightSidebarToolPanel call that single path. Keep the existing
openFileSurfacesNavigatingTextPosition call as the common final step so future
navigation changes only need to be made in one place.
🪄 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: ce8302d4-e6f3-4dd8-a79f-3602ff786c04
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Sources/ContentView.swiftSources/FileExplorerKeyboardShortcuts.swiftSources/FileExplorerStore.swiftSources/FileExplorerView.swiftSources/RightSidebarToolPanel.swiftcmuxTests/FileExplorerStoreTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/FileExplorerStoreTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Address review findings on the file-explorer git-watcher and file-preview navigation changes: - Reject non-regular `.git` entries before opening. A repository-controlled `.git` can be a FIFO/socket/device; `FileHandle(forReadingFrom:)` blocks on open() for a FIFO reader (waits for a writer), hanging watcher setup on the main actor before the byte cap applies. Gate on `stat`+S_IFREG, which follows symlinks (matching the existing `fileExists` probe) and never blocks. Add a timeout-raced regression test that fails cleanly rather than wedging CI. - Single-source the FSEvents exclusion-path cap: RecursivePathWatcher now references FileSystemEventStream.maximumExclusionPathCount instead of a duplicate constant, so the two truncation sites can never drift. - Document that CRLF (`\r\n`) is one Swift `Character` for which `isNewline` is true once, so the file-preview line counter advances once per CRLF. Add a pure-function regression test covering CRLF and LF offsets. - Document that text-position navigation applies only to plain-text previews; Markdown/Xcode-project surfaces intentionally ignore it. Refresh the Swift file-length budget for the documented files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ve-an-ide-level-file-editor-that-is # Conflicts: # .github/swift-file-length-budget.tsv
The main recursive tree watcher excludes `.git`, so `git init` in a folder opened before it was a repository writes only under the excluded `.git` and produces no main-watcher event. The git-state watcher's only recovery path, `installGitStateWatcherIfNeeded()`, runs solely from that main-watcher loop, so status badges stayed empty until an unrelated working-tree change fired. Add a short-lived bootstrap watcher on the same root that does NOT exclude `.git`, so `.git`'s creation is observed as an ordinary descendant event. Its handler installs the git-state watcher and then tears the bootstrap watcher down, so it never observes steady-state `.git` churn. It keeps every other high-churn exclusion (node_modules, build, etc.) so it stays cheap while it waits on a folder that may never become a repository. The regression test is deterministic and pure — it asserts the bootstrap watcher's exclusions equal the main watcher's minus `.git` (a regression to reusing the main exclusions would silently reinstate the stale-badge bug). A behavioral test would require live FSEvents timing; the pure-function guard covers the load-bearing mechanism and cannot compile without the fix, so test and fix land together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`navigateToTextPosition` queues a `pendingTextNavigation` jump that is applied once the text content finishes loading. A nil line number returned early without touching that queued state, so this sequence left a stale jump armed: a search-result open queues a jump for line N while the text is still loading, then the same file is reopened normally (no line target, `reuseExisting: true`) before the load completes. The nil-line open did not cancel the older jump, so when the load finished the reused panel scrolled to the stale search location instead of staying put. Treat a nil line as an explicit no-navigation request and clear `pendingTextNavigation` before returning. Both sidebar open paths (`openFilePreviewFromSidebar`, `RightSidebarToolPanel.openFilePreview`) take an optional line number and reuse the existing preview, so the nil path is reachable in normal use. Adds a deterministic regression test: with no text view attached the queued jump cannot be consumed, so the test asserts a line-carrying open sets the pending jump and a subsequent nil-line open clears it, without depending on async load timing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The git-state watcher started by `startGitStateWatcher` only calls `refreshGitStatus()` on *subsequent* `.git` events. For a repository that appears after the folder is opened (`git init`), the bootstrap creation watcher installs the git-state watcher only after `.git` already exists — its initial metadata writes are finished, and no further `.git` event is guaranteed. Status badges therefore stayed empty until some unrelated working-tree or metadata change happened to fire the watcher. Trigger an eager `refreshGitStatus()` the moment `installGitStateWatcherIfNeeded()` successfully installs the watcher, mirroring the eager refresh the already-a-repo path gets from `setRootPath`. Both watcher entrypoints (main tree watcher and bootstrap creation watcher) share this path; `refreshGitStatus()` coalesces, so the main-watcher entrypoint's redundant call (it refreshes immediately before) collapses into a single `git status` process. Adds a deterministic regression test that drives the shared install path against a freshly `git init`-ed fixture and asserts the install kicks off a status refresh. `refreshGitStatus()` sets its in-flight flag synchronously before dispatching, and the MainActor test never yields before asserting, so the check is race-free. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The FIFO probe raced gitMetadataDirectory() against a timeout inside a withTaskGroup. Structured concurrency will not let the group scope return until every child finishes, and cancelAll() cannot interrupt a synchronous open(), so if the blocking-open regression returned, the probe child would wedge the group forever — the test would hang CI instead of reaching the .timedOut assertion, leaving the main-actor-hang regression without a clean red signal. Run the probe on a detached OS thread raced against a timeout through a single-resume checked continuation. On timeout the continuation settles and the test records the failure while the wedged thread is simply abandoned (parked in open(), leaked, harmless — the test has already failed and the process is torn down). The passing path is unchanged: the regular-file guard rejects the FIFO without opening, so the probe resumes .completed(nil) at once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The FIFO-hang regression test drove FileExplorerStore through three DEBUG-only accessors (simulateGitRepositoryAppearedForTesting, isGitStatusRefreshInFlightForTesting, stopWatchersForTesting). Per the Aziz test/debug-seam policy, test-only accessors do not belong in production Sources/. Remove them and widen the five members the test drives (directoryWatchPath, gitStateWatcher, isGitStatusRefreshInFlight, installGitStateWatcherIfNeeded, stopDirectoryWatcher) from private to internal, reaching them from the test via the existing @testable import — the same resolution applied in #6452. The test now points the store at the repo (rootPath/directoryWatchPath) and calls installGitStateWatcherIfNeeded() directly, asserting the git-state watcher installs and eagerly kicks off a status refresh. Behavior is unchanged; the grandfathered setProviderForTesting stays. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ve-an-ide-level-file-editor-that-is # Conflicts: # .github/swift-file-length-budget.tsv
…ve-an-ide-level-file-editor-that-is # Conflicts: # .github/swift-file-length-budget.tsv
…ve-an-ide-level-file-editor-that-is # Conflicts: # .github/swift-file-length-budget.tsv # Sources/Panels/FilePreviewPanel.swift # cmuxTests/FilePreviewTextEditorTextKitTests.swift
…ve-an-ide-level-file-editor-that-is Resolve the .github/swift-file-length-budget.tsv conflict by regenerating it with scripts/swift_file_length_budget.py --write-budget on the merged tree (branch grew FileExplorerStore.swift, main grew the iOS TerminalInputTextView.swift). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
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
3 participants
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.
Fixes #5741
Summary
Validation
No reload.sh or xcodebuild run per issue instructions.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Opens file‑explorer search results at the exact line and UTF‑8 byte column, applying the jump after text loads or when the editor attaches. Keeps git‑status badges current by watching
.gitmetadata (including worktrees/submodules) and installing the watcher when a repo appears after a folder is opened.New Features
.xcodeprojrouting is preserved..gitwatcher resolvesgitdir:, excludesobjects/logs, installs on.gitcreation, and triggers an eager refresh.Bug Fixes
git statusruns with a per‑root/provider generation guard so slow or stale results don’t pile up or overwrite current state..gitpointer reads to 64 KiB and reject non‑regular.gitentries to avoid hangs.Written for commit 88a32b9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests