Add Finder-like file explorer sidebar with SSH support#1963
Add Finder-like file explorer sidebar with SSH support#1963lawrencecchen merged 11 commits intomainfrom
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 a right-side, SSH-aware File Explorer: new models and root resolver, SwiftUI + AppKit outline bridge, actor-backed async tree store, local and remote providers (remote via new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d326a85b9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| - [ ] **Step 2: Run the targeted tests to verify they fail** | ||
|
|
||
| Run: `xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -only-testing:cmuxTests/SessionPersistenceTests -only-testing:cmuxTests/AppDelegateShortcutRoutingTests test` |
There was a problem hiding this comment.
This plan directs local execution of xcodebuild ... test, but /workspace/cmux/AGENTS.md (Testing policy) explicitly requires that tests not be run locally and instead be executed via GitHub Actions/VM. Keeping local test commands here will cause implementers to violate repo policy and risk interfering with local app instances, so these steps should be rewritten as CI-triggered checks (or clearly marked CI-only) throughout the plan.
Useful? React with 👍 / 👎.
|
|
||
| Run: `xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -only-testing:cmuxTests/AppDelegateShortcutRoutingTests -only-testing:cmuxTests/WorkspaceUnitTests test` | ||
|
|
||
| Run: `gh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=task-file-explorer-ssh-sidebar -f test_filter="FileExplorerSidebarUITests" -f record_video=true` |
There was a problem hiding this comment.
This command dispatches test-e2e.yml with -f ref=task-file-explorer-ssh-sidebar before the plan’s later push step, so the referenced branch may not exist on origin yet. In .github/workflows/test-e2e.yml, checkout uses ref: ${{ inputs.ref || github.ref }}, so this ordering can make the validation run fail on checkout for an unpushed branch; move the push earlier or dispatch using a pushed SHA/ref.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md (1)
462-465: Makegh run watchdeterministic
<run-id>is easy to misuse manually. Add a one-liner that resolves the newest run id for the workflow/ref before watching.Suggested command snippet
RUN_ID=$(gh run list --repo manaflow-ai/cmux --workflow test-e2e.yml --branch task-file-explorer-ssh-sidebar --limit 1 --json databaseId -q '.[0].databaseId') gh run watch --repo manaflow-ai/cmux "$RUN_ID"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md` around lines 462 - 465, Replace the manual "<run-id>" placeholder in the gh run watch instruction by first resolving the newest run id for the workflow/ref into a variable and then passing that variable to gh run watch; specifically, run gh run list (with the same --repo, --workflow and --branch/ref options and --limit 1) and extract the latest run's databaseId into RUN_ID, then call gh run watch --repo ... "$RUN_ID" so the documentation shows a deterministic one-liner that uses gh run list and gh run watch together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md`:
- Around line 540-542: Replace machine-specific absolute paths in the runbook
commands (the lines invoking cd and git worktree remove) with parameterized
placeholders or environment variables (e.g. $REPO_DIR and $WORKTREE_PATH or
<repo-dir> and <worktree-path>), and update the surrounding text to explain how
to set those placeholders; specifically edit the entries that currently show the
exact paths used (the cd /Users/... and git worktree remove /Users/...) so they
no longer contain personal local paths and instead reference the placeholders or
variables.
- Around line 147-149: Clarify and enforce the primary-vs-secondary ownership
pattern for the new FileExplorerSidebarState: update MainWindowContext and
registerMainWindow(...) to carry the state object and document that the primary
window must pass SwiftUI-owned state from ContentView.onAppear while secondary
windows must construct/pass their own FileExplorerSidebarState via
createMainWindow; add the new toggleFileExplorerInActiveMainWindow() next to
toggleSidebarInActiveMainWindow() and ensure registerMainWindow rejects or
errors on attempts to re-register a window with mismatched ownership
(SwiftUI-owned vs manually-created) to prevent state re-registration bugs.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md`:
- Around line 462-465: Replace the manual "<run-id>" placeholder in the gh run
watch instruction by first resolving the newest run id for the workflow/ref into
a variable and then passing that variable to gh run watch; specifically, run gh
run list (with the same --repo, --workflow and --branch/ref options and --limit
1) and extract the latest run's databaseId into RUN_ID, then call gh run watch
--repo ... "$RUN_ID" so the documentation shows a deterministic one-liner that
uses gh run list and gh run watch together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 175e41dc-0af2-492c-80ed-658bc154c270
📒 Files selected for processing (1)
docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md
| - Extend `MainWindowContext` and `registerMainWindow(...)` to carry the new state object. | ||
| - Add `toggleFileExplorerInActiveMainWindow()` beside `toggleSidebarInActiveMainWindow()`. | ||
|
|
There was a problem hiding this comment.
Make registerMainWindow ownership rules explicit to avoid state mismatch
This plan should explicitly call out the primary-vs-secondary window ownership pattern for the new FileExplorerSidebarState (primary window passes SwiftUI-owned state from ContentView.onAppear; secondary windows construct/pass their own state via createMainWindow). Without this, implementation can regress into re-registration mismatches.
Suggested doc patch
- Extend `MainWindowContext` and `registerMainWindow(...)` to carry the new state object.
+- Follow existing window ownership semantics explicitly:
+ - Primary window: register from `ContentView.onAppear` and pass the SwiftUI-owned `FileExplorerSidebarState`.
+ - Secondary windows: `AppDelegate.createMainWindow` constructs and passes a dedicated `FileExplorerSidebarState`.
+ - Avoid re-registering a different state object for the same window lifecycle.Based on learnings, AppDelegate.registerMainWindow already uses a strict primary/secondary ownership pattern to avoid re-registration mismatches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md` around lines
147 - 149, Clarify and enforce the primary-vs-secondary ownership pattern for
the new FileExplorerSidebarState: update MainWindowContext and
registerMainWindow(...) to carry the state object and document that the primary
window must pass SwiftUI-owned state from ContentView.onAppear while secondary
windows must construct/pass their own FileExplorerSidebarState via
createMainWindow; add the new toggleFileExplorerInActiveMainWindow() next to
toggleSidebarInActiveMainWindow() and ensure registerMainWindow rejects or
errors on attempts to re-register a window with mismatched ownership
(SwiftUI-owned vs manually-created) to prevent state re-registration bugs.
| cd /Users/lawrence/fun/cmuxterm-hq/repo | ||
| git worktree remove /Users/lawrence/fun/cmuxterm-hq/worktrees/task-file-explorer-ssh-sidebar | ||
| ``` |
There was a problem hiding this comment.
Avoid machine-specific absolute paths in committed runbooks
These cleanup commands are not portable and leak personal local path details. Prefer parameterized paths/placeholders.
Suggested doc patch
-cd /Users/lawrence/fun/cmuxterm-hq/repo
-git worktree remove /Users/lawrence/fun/cmuxterm-hq/worktrees/task-file-explorer-ssh-sidebar
+cd <repo-root>
+git worktree remove <worktree-root>/task-file-explorer-ssh-sidebar📝 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.
| cd /Users/lawrence/fun/cmuxterm-hq/repo | |
| git worktree remove /Users/lawrence/fun/cmuxterm-hq/worktrees/task-file-explorer-ssh-sidebar | |
| ``` | |
| cd <repo-root> | |
| git worktree remove <worktree-root>/task-file-explorer-ssh-sidebar |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-22-ssh-file-explorer-sidebar.md` around lines
540 - 542, Replace machine-specific absolute paths in the runbook commands (the
lines invoking cd and git worktree remove) with parameterized placeholders or
environment variables (e.g. $REPO_DIR and $WORKTREE_PATH or <repo-dir> and
<worktree-path>), and update the surrounding text to explain how to set those
placeholders; specifically edit the entries that currently show the exact paths
used (the cd /Users/... and git worktree remove /Users/...) so they no longer
contain personal local paths and instead reference the placeholders or
variables.
Greptile SummaryThis PR adds an execution-ready implementation plan for a toggleable right-side file explorer sidebar that supports both local and SSH workspaces, with nested-root merging, lazy loading, and a new
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as FileExplorerSidebarView
participant Store as FileExplorerStore (actor)
participant Resolver as FileExplorerRootResolver
participant LP as LocalFileExplorerProvider
participant RP as RemoteFileExplorerProvider
participant Daemon as cmuxd-remote (fs.list RPC)
UI->>Store: refreshRoots(workspace.terminalRoots)
Store->>Resolver: resolve(orderedTerminalRoots, workspace)
Resolver-->>Store: [FileExplorerResolvedRoot] (nested paths merged)
alt Local workspace
Store->>LP: listChildren(request)
LP->>LP: FileManager.contentsOfDirectory
LP-->>Store: [FileExplorerEntry]
else SSH workspace (daemon ready)
Store->>RP: listChildren(request)
RP->>Daemon: fs.list { path }
Daemon-->>RP: { entries: [{name, path, kind}] }
RP-->>Store: [FileExplorerEntry]
else SSH workspace (daemon not ready / error)
Store-->>UI: .error(inline retryable row)
end
Store-->>UI: snapshot (sorted dirs-before-files)
Reviews (1): Last reviewed commit: "docs: add ssh file explorer sidebar plan" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| Resolver rules: | ||
| - Read terminal directories in visual order from the selected workspace. | ||
| - Reuse `SidebarBranchOrdering.canonicalDirectoryKey(...)` and `SidebarBranchOrdering.inferredRemoteHomeDirectory(...)` so tilde expansion stays consistent with the existing sidebar. |
There was a problem hiding this comment.
fileprivate helpers inaccessible from new file
Both SidebarBranchOrdering.canonicalDirectoryKey(...) and SidebarBranchOrdering.inferredRemoteHomeDirectory(...) are declared fileprivate static inside Workspace.swift (lines 4798 and 4848). A new Sources/FileExplorer/FileExplorerRootResolver.swift file cannot call them — the compiler will reject the access as a visibility error.
The plan needs an explicit step to promote these helpers (or extract the relevant logic into a shared location) before the resolver can reuse them. Options:
- Change the access level to
internal(the default) inWorkspace.swift— lowest friction. - Extract tilde-expansion helpers into a new
Sources/PathNormalization.swift(or similar) and have bothSidebarBranchOrderingandFileExplorerRootResolvercall the shared copy.
Without this step the Task 2 Step 3 implementation will fail to compile.
| func testToggleFileExplorerShortcutMetadata() { | ||
| XCTAssertEqual(KeyboardShortcutSettings.Action.toggleFileExplorer.label, "Toggle File Explorer") | ||
| } |
There was a problem hiding this comment.
CLAUDE.md explicitly says:
Do not add tests that only verify source code text, method signatures, AST fragments, or grep-style patterns.
Asserting KeyboardShortcutSettings.Action.toggleFileExplorer.label == "Toggle File Explorer" is essentially verifying a hardcoded string literal, not observable runtime behavior. If someone renames the label the test breaks for the wrong reason, and if someone accidentally deletes the action the test is silent.
A more policy-compliant replacement would be to assert routing/behavioral outcomes — e.g. that invoking the new action through AppDelegate actually toggles fileExplorerState.isVisible on the active window — mirroring the existing testToggleSidebarShortcutUsesActiveMainWindowContext pattern referenced earlier in Task 1.
| func testToggleFileExplorerShortcutMetadata() { | |
| XCTAssertEqual(KeyboardShortcutSettings.Action.toggleFileExplorer.label, "Toggle File Explorer") | |
| } | |
| func testToggleFileExplorerShortcutRoutesToActiveWindow() { | |
| // Mirror the existing sidebar routing test: invoke the action and assert | |
| // that fileExplorerState.isVisible toggles on the active main window context. | |
| } |
| - In Swift, use `workspace.remoteDaemonStatus.remotePath` plus `workspace.remoteConfiguration` to create the provider. | ||
| - It is acceptable for v1 to use a short-lived `WorkspaceRemoteDaemonRPCClient` per load, because the tree store will cache responses and loads are user-driven rather than keystroke-hot. |
There was a problem hiding this comment.
remoteDaemonStatus.remotePath is Optional<String> — nil case unaddressed
WorkspaceRemoteDaemonStatus.remotePath is declared as var remotePath: String? (it is nil until the daemon has bootstrapped and reported its path). The plan doesn't say what the provider should do when this is nil — e.g. daemon is still bootstrapping, or the workspace just reconnected.
The implementation note should specify the fallback: either surface the inline retryable error row (consistent with the "fail soft" policy described in Chunk 1) or skip provider creation and show a "waiting for daemon" state until remoteDaemonStatus.state == .ready and remotePath != nil.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ea64c9ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #endif | ||
|
|
||
| func attachUpdateAccessory(to window: NSWindow) { | ||
| guard !isRunningUnderXCTestCached else { return } |
There was a problem hiding this comment.
This new guard skips attachUpdateAccessory whenever isRunningUnderXCTestCached is true, but detectRunningUnderXCTest also treats CMUX_UI_TEST_* as XCTest signals. In UI-test app launches (CMUX_UI_TEST_MODE=1), the accessory never attaches, so titlebarControl.* buttons are missing and UI flows/tests that rely on titlebar controls regress.
Useful? React with 👍 / 👎.
| Task { | ||
| await store.toggleExpansion(for: nodeID) | ||
| treeState = await store.snapshot() |
There was a problem hiding this comment.
toggleExpansion writes treeState from an async task without validating that the captured store is still current. If configure(using:) replaces the store (for example, workspace switch or remote context change) while this task is in flight, the old task can overwrite the new workspace’s tree with stale data; configure already uses configurationGeneration, but this path does not.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 28 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:377">
P2: `isRunningUnderXCTestHost()` is too broad for this branch and can match XCUITest app launches, causing the app to show only the bootstrap view and close its window instead of rendering `ContentView`.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:6449">
P2: Only expose a remote file-explorer path when the daemon advertises `fs.list`; otherwise this code selects the remote provider and triggers unsupported RPC calls on older daemons.</violation>
</file>
<file name="Sources/FileExplorer/RemoteFileExplorerProvider.swift">
<violation number="1" location="Sources/FileExplorer/RemoteFileExplorerProvider.swift:93">
P2: Do not trim remote `path`/`name`; it can corrupt valid filenames and paths returned by the SSH side.</violation>
</file>
<file name="Sources/FileExplorer/FileExplorerStore.swift">
<violation number="1" location="Sources/FileExplorer/FileExplorerStore.swift:67">
P2: Avoid overlapping loads for the same node ID; actor re-entrancy during `await` can trigger duplicate requests and stale/out-of-order state updates.</violation>
</file>
<file name="Sources/Update/UpdateTitlebarAccessory.swift">
<violation number="1" location="Sources/Update/UpdateTitlebarAccessory.swift:582">
P2: Adding the file-explorer control without increasing the hidden-titlebar host width will clip titlebar controls in minimal mode. The fixed `hostWidth` (124) is now too small for four buttons plus existing padding.</violation>
</file>
<file name="Sources/FileExplorer/FileExplorerSidebarView.swift">
<violation number="1" location="Sources/FileExplorer/FileExplorerSidebarView.swift:216">
P2: Async update tasks (`toggleExpansion`/`refreshNode`/`refreshAll`) can apply stale snapshots after provider/workspace changes because they do not check `configurationGeneration` before assigning `treeState`.</violation>
<violation number="2" location="Sources/FileExplorer/FileExplorerSidebarView.swift:243">
P1: `refreshAll` currently refreshes file/symlink nodes as if they were directories, which can produce per-file errors and extra provider calls. Restrict refresh traversal to directory nodes.</violation>
</file>
<file name="Sources/FileExplorer/FileExplorerOutlineView.swift">
<violation number="1" location="Sources/FileExplorer/FileExplorerOutlineView.swift:79">
P3: `itemByID` is dead state: it is rebuilt on every update but never used, adding unnecessary O(n) work for each tree refresh.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:2401">
P2: The new file-explorer divider reuses cursor-band logic that still only supports the left sidebar, so right-divider hover/resize cursor state is force-reset when the left sidebar is hidden.</violation>
</file>
<file name="daemon/remote/cmd/cmuxd-remote/main.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/main.go:412">
P2: `fs.list` returns unbounded entry payloads, which can exceed the client's 256 KiB frame limit and terminate the remote daemon transport.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| private func refreshableNodeIDs(in nodes: [FileExplorerNodeState]) -> [FileExplorerNodeID] { | ||
| nodes.flatMap { node in | ||
| let descendantIDs = node.isExpanded ? refreshableNodeIDs(in: node.children) : [] | ||
| return [node.id] + descendantIDs |
There was a problem hiding this comment.
P1: refreshAll currently refreshes file/symlink nodes as if they were directories, which can produce per-file errors and extra provider calls. Restrict refresh traversal to directory nodes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/FileExplorer/FileExplorerSidebarView.swift, line 243:
<comment>`refreshAll` currently refreshes file/symlink nodes as if they were directories, which can produce per-file errors and extra provider calls. Restrict refresh traversal to directory nodes.</comment>
<file context>
@@ -0,0 +1,294 @@
+ private func refreshableNodeIDs(in nodes: [FileExplorerNodeState]) -> [FileExplorerNodeID] {
+ nodes.flatMap { node in
+ let descendantIDs = node.isExpanded ? refreshableNodeIDs(in: node.children) : []
+ return [node.id] + descendantIDs
+ }
+ }
</file context>
| .environmentObject(sidebarState) | ||
| .environmentObject(sidebarSelectionState) | ||
| .onAppear { | ||
| if SessionRestorePolicy.isRunningUnderXCTestHost() { |
There was a problem hiding this comment.
P2: isRunningUnderXCTestHost() is too broad for this branch and can match XCUITest app launches, causing the app to show only the bootstrap view and close its window instead of rendering ContentView.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/cmuxApp.swift, line 377:
<comment>`isRunningUnderXCTestHost()` is too broad for this branch and can match XCUITest app launches, causing the app to show only the bootstrap view and close its window instead of rendering `ContentView`.</comment>
<file context>
@@ -332,152 +374,162 @@ struct cmuxApp: App {
- .environmentObject(sidebarState)
- .environmentObject(sidebarSelectionState)
- .onAppear {
+ if SessionRestorePolicy.isRunningUnderXCTestHost() {
+ XCTestHostWindowBootstrapView()
+ } else {
</file context>
| if SessionRestorePolicy.isRunningUnderXCTestHost() { | |
| if SessionRestorePolicy.isRunningUnderXCTestHost() && !SessionRestorePolicy.isRunningUnderUITestApp() { |
| let canonicalPath = (rawEntry["path"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| let displayName = (rawEntry["name"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" |
There was a problem hiding this comment.
P2: Do not trim remote path/name; it can corrupt valid filenames and paths returned by the SSH side.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/FileExplorer/RemoteFileExplorerProvider.swift, line 93:
<comment>Do not trim remote `path`/`name`; it can corrupt valid filenames and paths returned by the SSH side.</comment>
<file context>
@@ -0,0 +1,119 @@
+ }
+
+ private static func parseListedEntry(_ rawEntry: [String: Any]) throws -> ListedEntry {
+ let canonicalPath = (rawEntry["path"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
+ let displayName = (rawEntry["name"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
+ let rawKind = (rawEntry["kind"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
</file context>
| let canonicalPath = (rawEntry["path"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| let displayName = (rawEntry["name"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| let canonicalPath = (rawEntry["path"] as? String) ?? "" | |
| let displayName = (rawEntry["name"] as? String) ?? "" |
| return | ||
| } | ||
|
|
||
| loadingNodeIDs.insert(nodeID) |
There was a problem hiding this comment.
P2: Avoid overlapping loads for the same node ID; actor re-entrancy during await can trigger duplicate requests and stale/out-of-order state updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/FileExplorer/FileExplorerStore.swift, line 67:
<comment>Avoid overlapping loads for the same node ID; actor re-entrancy during `await` can trigger duplicate requests and stale/out-of-order state updates.</comment>
<file context>
@@ -0,0 +1,205 @@
+ return
+ }
+
+ loadingNodeIDs.insert(nodeID)
+ defer { loadingNodeIDs.remove(nodeID) }
+
</file context>
| func toggleExpansion(_ nodeID: FileExplorerNodeID) { | ||
| guard let store else { return } | ||
| Task { | ||
| await store.toggleExpansion(for: nodeID) |
There was a problem hiding this comment.
P2: Async update tasks (toggleExpansion/refreshNode/refreshAll) can apply stale snapshots after provider/workspace changes because they do not check configurationGeneration before assigning treeState.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/FileExplorer/FileExplorerSidebarView.swift, line 216:
<comment>Async update tasks (`toggleExpansion`/`refreshNode`/`refreshAll`) can apply stale snapshots after provider/workspace changes because they do not check `configurationGeneration` before assigning `treeState`.</comment>
<file context>
@@ -0,0 +1,294 @@
+ func toggleExpansion(_ nodeID: FileExplorerNodeID) {
+ guard let store else { return }
+ Task {
+ await store.toggleExpansion(for: nodeID)
+ treeState = await store.snapshot()
+ }
</file context>
| return strings.Compare(entries[i].name, entries[j].name) < 0 | ||
| }) | ||
|
|
||
| payloadEntries := make([]map[string]any, 0, len(entries)) |
There was a problem hiding this comment.
P2: fs.list returns unbounded entry payloads, which can exceed the client's 256 KiB frame limit and terminate the remote daemon transport.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/remote/cmd/cmuxd-remote/main.go, line 412:
<comment>`fs.list` returns unbounded entry payloads, which can exceed the client's 256 KiB frame limit and terminate the remote daemon transport.</comment>
<file context>
@@ -360,6 +364,106 @@ func (s *rpcServer) handleRequest(req rpcRequest) rpcResponse {
+ return strings.Compare(entries[i].name, entries[j].name) < 0
+ })
+
+ payloadEntries := make([]map[string]any, 0, len(entries))
+ for _, entry := range entries {
+ payloadEntries = append(payloadEntries, map[string]any{
</file context>
|
|
||
| var nextItemByID: [FileExplorerNodeID: OutlineItem] = [:] | ||
| collectItems(rootItems, into: &nextItemByID) | ||
| itemByID = nextItemByID |
There was a problem hiding this comment.
P3: itemByID is dead state: it is rebuilt on every update but never used, adding unnecessary O(n) work for each tree refresh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/FileExplorer/FileExplorerOutlineView.swift, line 79:
<comment>`itemByID` is dead state: it is rebuilt on every update but never used, adding unnecessary O(n) work for each tree refresh.</comment>
<file context>
@@ -0,0 +1,434 @@
+
+ var nextItemByID: [FileExplorerNodeID: OutlineItem] = [:]
+ collectItems(rootItems, into: &nextItemByID)
+ itemByID = nextItemByID
+
+ guard let outlineView else { return }
</file context>
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
3762-3787:⚠️ Potential issue | 🟠 MajorMake
fileExplorerStatemandatory onregisterMainWindow(...)to prevent silent state splits.Line 3767's default value silently creates a fresh
FileExplorerSidebarStatewhenever a caller omits the parameter. This diverges from the SwiftUI-owned state that should be registered, causing shortcuts and session restore to mutate an orphan instance instead. All production call sites (ContentView, AppDelegate.createMainWindow) already passfileExplorerStateexplicitly, mirroring the requiredSidebarStatepattern. Removing the default value requires updating 10 test call sites incmuxTests/WindowAndDragTests.swiftandcmuxTests/AppDelegateShortcutRoutingTests.swift.Suggested fix
- fileExplorerState: FileExplorerSidebarState = FileExplorerSidebarState(), + fileExplorerState: FileExplorerSidebarState,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3762 - 3787, The registerMainWindow(_:windowId:tabManager:sidebarState:fileExplorerState:sidebarSelectionState:) signature currently gives fileExplorerState a default value, which silently creates an orphan FileExplorerSidebarState; remove the default so fileExplorerState becomes a required parameter in the MainWindow registration function (update the parameter declaration in registerMainWindow to drop "= FileExplorerSidebarState()"), then update all call sites (including the 10 test invocations in the WindowAndDragTests and AppDelegateShortcutRoutingTests suites) to pass an explicit FileExplorerSidebarState instance (or the existing SwiftUI-owned instance) when calling registerMainWindow to ensure the shared state is used.
🧹 Nitpick comments (2)
daemon/remote/cmd/cmuxd-remote/main.go (1)
431-452: Minor: Redundant symlink check in fallback branch.Lines 444-445 check for symlink again in the fallback, but the fallback is only reached when the initial
mode&os.ModeSymlink != 0check (line 434) already failed. This code path handles special file types (devices, sockets, pipes) and will correctly return "file" for them. The redundant check is harmless but could be simplified.♻️ Optional simplification
default: info, err := entry.Info() if err == nil { - switch { - case info.Mode()&os.ModeSymlink != 0: - return "symlink" - case info.IsDir(): + if info.IsDir() { return "directory" } } return "file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daemon/remote/cmd/cmuxd-remote/main.go` around lines 431 - 452, The fallback branch in fsListEntryKind currently re-checks for symlink using entry.Info() even though the top-level mode&os.ModeSymlink check already failed; simplify the fallback by removing the redundant info.Mode()&os.ModeSymlink check and only use info.IsDir() to decide between returning "directory" or "file" (i.e., in fsListEntryKind, call entry.Info(), if err == nil and info.IsDir() return "directory", otherwise return "file").Sources/FileExplorer/FileExplorerModels.swift (1)
67-79: Consider normalizing trailing slashes in path matching.The
matchesReferencedPathmethod trims whitespace but doesn't normalize trailing slashes. Paths like/Users/foo/and/Users/foowould fail to match even though they refer to the same directory. This could cause subtle lookup failures depending on howdisplayPath/canonicalPathare populated upstream.♻️ Proposed fix to normalize trailing slashes
private func matchesReferencedPath(_ path: String) -> Bool { let normalized = path.trimmingCharacters(in: .whitespacesAndNewlines) guard !normalized.isEmpty else { return false } + let normalizedNoSlash = normalized.hasSuffix("/") ? String(normalized.dropLast()) : normalized + let displayNoSlash = displayPath.hasSuffix("/") ? String(displayPath.dropLast()) : displayPath + let canonicalNoSlash = canonicalPath.hasSuffix("/") ? String(canonicalPath.dropLast()) : canonicalPath if isExplicitSurfaceRoot, - displayPath == normalized || canonicalPath == normalized { + displayNoSlash == normalizedNoSlash || canonicalNoSlash == normalizedNoSlash { return true } - return children.contains { $0.matchesReferencedPath(normalized) } + return children.contains { $0.matchesReferencedPath(normalizedNoSlash) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/FileExplorer/FileExplorerModels.swift` around lines 67 - 79, The path comparison misses trailing-slash normalization in matchesReferencedPath, so paths like "/Users/foo/" won't match "/Users/foo"; update matchesReferencedPath (used by containsReferencedDescendant) to normalize the incoming path and the stored paths before comparing: after trimming whitespace produce a normalized string that strips any trailing "/" (but preserves root "/"), or use a standard path-normalization helper (e.g., NSString/URL standardizing) and then compare normalized == normalized displayPath and canonicalPath; ensure the same normalization is applied when recursing into children (children.contains { $0.matchesReferencedPath(normalized) }) and when checking isExplicitSurfaceRoot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 3244-3248: closeAllOpenWindows currently uses a fixed 50ms RunLoop
sleep which can cause flakiness; replace that sleep with the existing
waitForCondition helper so we poll until all windows are closed/hidden.
Specifically, in closeAllOpenWindows() (the method that iterates NSApp.windows
and calls window.performClose(nil)), remove the RunLoop.main.run(...) and call
waitForCondition with a predicate like "NSApp.windows.allSatisfy { !$0.isVisible
}" (or "NSApp.windows.isEmpty" if that better matches the test harness) and a
reasonable timeout (e.g., 1s) so the helper waits until no open windows remain.
In `@cmuxTests/FileExplorerStoreTests.swift`:
- Around line 10-26: The test testLoadingChildrenSortsDirectoriesBeforeFiles
creates a temporary directory (tempDirectory) but never removes it; add cleanup
by removing tempDirectory when the test completes (e.g., immediately after
creating tempDirectory, register a defer or finally-style cleanup that calls
FileManager.default.removeItem(at: tempDirectory) or try? removeItem to avoid
failing the test) so the filesystem is not polluted; reference the tempDirectory
variable created in the test and ensure the cleanup runs even if assertions
throw.
In `@cmuxTests/RemoteFileExplorerProviderTests.swift`:
- Line 2: Replace the hardcoded testable import in
RemoteFileExplorerProviderTests.swift so it uses the conditional import pattern
other tests use: wrap the existing "@testable import cmux_DEV" in a compile-time
guard that tries cmux_DEV first and falls back to cmux (i.e. use `#if`
canImport(cmux_DEV) / `@testable` import cmux_DEV / `#else` / `@testable` import cmux
/ `#endif`) so the file builds for both DEV and non-DEV module names.
In `@cmuxUITests/FileExplorerSidebarUITests.swift`:
- Around line 10-18: Normalize persisted UI state right after launching
XCUIApplication by resetting the window/titlebar accessory and sidebar
visibility: add/declare a test-only launchEnvironment flag (e.g.
CMUX_UI_TEST_RESET or reuse CMUX_UI_TEST_MODE) that your app checks at startup
to clear persisted UI defaults, or, if you prefer a pure-test approach, after
app.launch() assert and restore state by ensuring the existence of the
titlebarControl.toggleFileExplorer button and that FileExplorerSidebar is hidden
(if FileExplorerSidebar.exists then toggle via
titlebarControl.toggleFileExplorer until hidden; if the toggle button is missing
restore the accessory via the reset flag or a known UI flow). Target symbols:
XCUIApplication, CMUX_UI_TEST_MODE (or CMUX_UI_TEST_RESET),
titlebarControl.toggleFileExplorer, FileExplorerSidebar.
In `@Sources/AppDelegate.swift`:
- Around line 2247-2248: The titlebar accessory setup is gated by a broad XCTest
detection that treats UI-test app processes as tests; replace those checks to
use the host-only test signal so the app-under-test gets the accessory: where
the code currently calls detectRunningUnderXCTest(environment:) or uses
isRunningUnderUITestApp, switch the condition to use the already-computed
SessionRestorePolicy.isRunningUnderXCTestHost(environment: env) (or the local
variable isRunningUnderXCTestHost) instead (apply the same change at the other
two occurrences that currently gate on the broader XCTest flag).
In `@Sources/cmuxApp.swift`:
- Around line 443-459: The newly added user-facing menu and button titles inside
the CommandMenu("Update Pill") and the other CommandMenu block use bare string
literals; replace each literal (e.g., "Update Pill", "Show Update Pill", "Show
Long Nightly Pill", "Show Loading State", "Hide Update Pill", "Automatic Update
Pill" and the other titles in the 501-531 block) with localized calls using
String(localized: "key.name", defaultValue: "English text") and keep the Button
actions calling appDelegate.showUpdatePill(_:), showUpdatePillLongNightly(_:),
showUpdatePillLoading(_:), hideUpdatePill(_:), clearUpdatePillOverride(_:); then
add matching keys and translations (English + Japanese) to
Resources/Localizable.xcstrings.
- Around line 136-148: The removeMainWindowAutosaveFrames cleanup currently runs
every launch; change it to a one-time migration by wrapping the call in a guard
that checks and sets a migration flag in UserDefaults (e.g.,
"WindowFrameAutosaveCleanup.migrationDone.v1"); only call
WindowFrameAutosaveCleanup.removeMainWindowAutosaveFrames(...) if the flag is
absent, and then set the flag to true after successfully removing keys. Keep the
existing enum and method intact (removeMainWindowAutosaveFrames) and only add
the migration-check logic where the cleanup is invoked so future launches skip
the removal.
In `@Sources/ContentView.swift`:
- Line 2775: The persisted width assignment can remain non-finite because
normalizedFileExplorerWidth(_:) maps nan/inf but subsequent tolerance checks use
arithmetic that yields nan; to fix, sanitize the persisted value to a finite
number before any comparisons or assignments: call
normalizedFileExplorerWidth(...) or otherwise coerce
fileExplorerState.persistedWidth to a safe default first, then perform the
abs(...) tolerance checks and finally assign to fileExplorerWidth; apply the
same sanitization pattern around the related blocks that reference
normalizedFileExplorerWidth(_:), fileExplorerWidth, and
fileExplorerState.persistedWidth (also present at the other noted ranges).
- Around line 2028-2031: The cursor-band logic currently only checks the left
divider and gates on sidebarState.isVisible, so the new
SidebarResizerHandle.fileExplorerDivider loses the stabilized resize cursor when
the left sidebar is hidden; update the updateSidebarResizerBandState() function
to also consider SidebarResizerHandle.fileExplorerDivider and the file
explorer's visibility (e.g. fileExplorerState.isVisible or equivalent) when
deciding which divider band is active, and ensure the pointer grab/release logic
that tests the left divider band is extended to test the fileExplorerDivider as
well so resize cursor stabilization works for the file explorer divider.
In `@Sources/FileExplorer/FileExplorerSidebarView.swift`:
- Around line 229-245: The traversal currently returns every visible node ID and
causes refreshNode(_:) to be called for files; update refreshableNodeIDs(in:) so
it only yields "listable" nodes (e.g., directories or nodes with a dedicated
isListable flag) — use FileExplorerNodeState's kind or add/use an isListable
property to filter out regular files (and if directory symlinks need support,
handle them explicitly, e.g. treat kind == .directory or .directorySymlink as
listable). Then call store.refreshNode(_:) only for those filtered IDs in
refreshAll(using:), keeping the expanded-children traversal logic but only
descending/returning when the parent is listable as appropriate.
- Around line 25-32: The sidebar is being mutated by detached tasks and
observers that can run after the user has switched workspaces; fix this by
fencing all configuration work with a generation token: when starting any
configure flow (including in FileExplorerWorkspaceObserver and the .task(id:
workspace?.id) blocks that call viewModel.configure(using:)), increment a
viewModel.configurationGeneration and capture it as a localGeneration at the
start of the async flow, then after every await before mutating sidebar state or
publishing snapshots, check that viewModel.configurationGeneration ==
localGeneration and return early if it differs; also ensure observer
invalidations are routed into the same current-workspace task (use the .task(id:
workspace?.id) or a single `@MainActor` Task on the viewModel) so late observer
callbacks publish only when the generation matches.
In `@Sources/FileExplorer/FileExplorerStore.swift`:
- Around line 36-40: The provider.listChildren call can return after
refreshRoots has pruned a node, so change the listing code (the async block that
awaits provider.listChildren and then writes children/error state) to re-check
that the target node is still present/reachable in the actor state before
applying results; specifically, after the await re-run the same
reachability/attachment check used by pruneDetachedState (or verify the node
exists in seedNodesByID/roots) and drop the returned children or error if the
node was removed; apply the same guard to the other affected listing site noted
around the 62-76 region so late results cannot resurrect pruned directories.
In `@Sources/FileExplorer/RemoteFileExplorerProvider.swift`:
- Around line 92-99: In parseListedEntry, stop trimming the remote path/name so
daemon-provided whitespace is preserved: replace the trimmed assignments for
canonicalPath and displayName with their raw string values (let canonicalPath =
rawEntry["path"] as? String ?? "" and let displayName = rawEntry["name"] as?
String ?? "") and perform the emptiness guard on those raw values (guard
!canonicalPath.isEmpty, !displayName.isEmpty ...). Do not alter whitespace in
path/name; leave rawKind handling unchanged or treat separately if needed.
- Around line 7-14: The errorDescription computed property in
RemoteFileExplorerProvider uses bare string literals for the hostScopeMismatch
and invalidResponse cases; replace those literals with localized strings using
String(localized: "fileExplorer.error.hostScopeMismatch", defaultValue: "Remote
file explorer request did not match the active SSH target.") and
String(localized: "fileExplorer.error.invalidResponse", defaultValue:
"<detail>") (interpolating or returning the detail appropriately) and add the
corresponding keys to Resources/Localizable.xcstrings; update the same pattern
for the other occurrences mentioned (lines 98–110) to use String(localized:...,
defaultValue:...) instead of raw literals.
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 376-390: HiddenTitlebarSidebarControlsView is still reserving
width for only three TitlebarControlButton items while TitlebarControlsView now
renders four (including the new file-explorer toggle created with
TitlebarControlButton + iconLabel and onToggleFileExplorer), causing clipping in
hidden/minimal titlebar modes; update HiddenTitlebarSidebarControlsView (and the
other similar reserved-width code mentioned) to compute its reserved width
dynamically based on the actual number of controls (or expand it to accommodate
four buttons) so the trailing TitlebarControlButton (file-explorer toggle) is
not clipped or hidden.
In `@Sources/Workspace.swift`:
- Around line 794-795: The capability-based daemon refresh check in
WorkspaceRemoteSessionController.bootstrapDaemonLocked() is missing fs.list, so
daemons that lack WorkspaceRemoteDaemonRPCClient.fsListCapability won't be
refreshed; update the capability check to include
WorkspaceRemoteDaemonRPCClient.fsListCapability (i.e., add static let
fsListCapability = "fs.list" to the set of required capabilities used in
bootstrapDaemonLocked()) so same-version hosts that don't advertise fs.list will
trigger the bootstrap reupload/refresh path.
- Around line 6394-6401: The current fileExplorerHostScope() flips the whole
workspace to SSH whenever workspace.remoteConfiguration exists; instead, change
it to compute host scope and home expansion per-terminal root: add a variant
like fileExplorerHostScope(for root: TerminalRoot or for startupCommand:
String?) that checks the terminal-specific startup command / root metadata to
decide .local vs .ssh (using remoteConfiguration only when that particular
terminal indicates a remote target), update callers such as newTerminalSurface
and newTerminalSplit to call the per-terminal hostScope function or pass the
terminal's startupCommand/root info, and apply the same per-root logic where
hostScope/home expansion are used in the 6413-6440 region so local panel
directories are not canonicalized as remote. Ensure references to
remoteConfiguration, terminalStartupCommand, newTerminalSurface,
newTerminalSplit, hostScope, and home expansion are updated accordingly.
- Around line 979-986: The current listDirectory(path:) silently coerces a
malformed fs.list reply into an empty directory by using (result["entries"] as?
[[String: Any]]) ?? [], which hides protocol/version bugs; update
listDirectory(path:) to validate that result["entries"] exists and is of type
[[String: Any]] and, if not, throw a descriptive error (e.g. create/throw a
WorkspaceError.invalidResponse or a thrown NSError with context) instead of
returning []; keep the call to call(method: "fs.list", ...) and the returned
entries variable name but replace the nil-coalescing fallback with a
guard/if-let that raises the error including the raw result for debugging.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3762-3787: The
registerMainWindow(_:windowId:tabManager:sidebarState:fileExplorerState:sidebarSelectionState:)
signature currently gives fileExplorerState a default value, which silently
creates an orphan FileExplorerSidebarState; remove the default so
fileExplorerState becomes a required parameter in the MainWindow registration
function (update the parameter declaration in registerMainWindow to drop "=
FileExplorerSidebarState()"), then update all call sites (including the 10 test
invocations in the WindowAndDragTests and AppDelegateShortcutRoutingTests
suites) to pass an explicit FileExplorerSidebarState instance (or the existing
SwiftUI-owned instance) when calling registerMainWindow to ensure the shared
state is used.
---
Nitpick comments:
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 431-452: The fallback branch in fsListEntryKind currently
re-checks for symlink using entry.Info() even though the top-level
mode&os.ModeSymlink check already failed; simplify the fallback by removing the
redundant info.Mode()&os.ModeSymlink check and only use info.IsDir() to decide
between returning "directory" or "file" (i.e., in fsListEntryKind, call
entry.Info(), if err == nil and info.IsDir() return "directory", otherwise
return "file").
In `@Sources/FileExplorer/FileExplorerModels.swift`:
- Around line 67-79: The path comparison misses trailing-slash normalization in
matchesReferencedPath, so paths like "/Users/foo/" won't match "/Users/foo";
update matchesReferencedPath (used by containsReferencedDescendant) to normalize
the incoming path and the stored paths before comparing: after trimming
whitespace produce a normalized string that strips any trailing "/" (but
preserves root "/"), or use a standard path-normalization helper (e.g.,
NSString/URL standardizing) and then compare normalized == normalized
displayPath and canonicalPath; ensure the same normalization is applied when
recursing into children (children.contains {
$0.matchesReferencedPath(normalized) }) and when checking isExplicitSurfaceRoot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f2be80e-0ef1-4cdf-968a-6d23189bbcbd
📒 Files selected for processing (28)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/FileExplorer/FileExplorerModels.swiftSources/FileExplorer/FileExplorerOutlineView.swiftSources/FileExplorer/FileExplorerProvider.swiftSources/FileExplorer/FileExplorerRootResolver.swiftSources/FileExplorer/FileExplorerSidebarState.swiftSources/FileExplorer/FileExplorerSidebarView.swiftSources/FileExplorer/FileExplorerStore.swiftSources/FileExplorer/LocalFileExplorerProvider.swiftSources/FileExplorer/RemoteFileExplorerProvider.swiftSources/KeyboardShortcutSettings.swiftSources/SessionPersistence.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/FileExplorerRootResolverTests.swiftcmuxTests/FileExplorerStoreTests.swiftcmuxTests/RemoteFileExplorerProviderTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/WorkspaceUnitTests.swiftcmuxUITests/FileExplorerSidebarUITests.swiftdaemon/remote/README.mddaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.go
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
| func testLoadingChildrenSortsDirectoriesBeforeFiles() async throws { | ||
| let tempDirectory = try makeTemporaryDirectory() | ||
| let sourcesDirectory = tempDirectory.appendingPathComponent("Sources", isDirectory: true) | ||
| let readmeFile = tempDirectory.appendingPathComponent("README.md") | ||
|
|
||
| try FileManager.default.createDirectory(at: sourcesDirectory, withIntermediateDirectories: true) | ||
| try Data("hello".utf8).write(to: readmeFile) | ||
|
|
||
| let root = makeRoot(canonicalPath: tempDirectory.path, displayPath: tempDirectory.path) | ||
| let store = FileExplorerStore(provider: LocalFileExplorerProvider()) | ||
|
|
||
| await store.refreshRoots([root]) | ||
| await store.toggleExpansion(for: root.id) | ||
|
|
||
| let snapshot = await store.snapshot() | ||
| XCTAssertEqual(snapshot.roots.first?.children.map(\.displayName), ["Sources", "README.md"]) | ||
| } |
There was a problem hiding this comment.
Missing temp directory cleanup.
The test creates a temporary directory but doesn't clean it up after execution. This can lead to filesystem pollution over repeated test runs.
🧹 Proposed fix to add cleanup
func testLoadingChildrenSortsDirectoriesBeforeFiles() async throws {
let tempDirectory = try makeTemporaryDirectory()
+ addTeardownBlock {
+ try? FileManager.default.removeItem(at: tempDirectory)
+ }
let sourcesDirectory = tempDirectory.appendingPathComponent("Sources", isDirectory: true)📝 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.
| func testLoadingChildrenSortsDirectoriesBeforeFiles() async throws { | |
| let tempDirectory = try makeTemporaryDirectory() | |
| let sourcesDirectory = tempDirectory.appendingPathComponent("Sources", isDirectory: true) | |
| let readmeFile = tempDirectory.appendingPathComponent("README.md") | |
| try FileManager.default.createDirectory(at: sourcesDirectory, withIntermediateDirectories: true) | |
| try Data("hello".utf8).write(to: readmeFile) | |
| let root = makeRoot(canonicalPath: tempDirectory.path, displayPath: tempDirectory.path) | |
| let store = FileExplorerStore(provider: LocalFileExplorerProvider()) | |
| await store.refreshRoots([root]) | |
| await store.toggleExpansion(for: root.id) | |
| let snapshot = await store.snapshot() | |
| XCTAssertEqual(snapshot.roots.first?.children.map(\.displayName), ["Sources", "README.md"]) | |
| } | |
| func testLoadingChildrenSortsDirectoriesBeforeFiles() async throws { | |
| let tempDirectory = try makeTemporaryDirectory() | |
| addTeardownBlock { | |
| try? FileManager.default.removeItem(at: tempDirectory) | |
| } | |
| let sourcesDirectory = tempDirectory.appendingPathComponent("Sources", isDirectory: true) | |
| let readmeFile = tempDirectory.appendingPathComponent("README.md") | |
| try FileManager.default.createDirectory(at: sourcesDirectory, withIntermediateDirectories: true) | |
| try Data("hello".utf8).write(to: readmeFile) | |
| let root = makeRoot(canonicalPath: tempDirectory.path, displayPath: tempDirectory.path) | |
| let store = FileExplorerStore(provider: LocalFileExplorerProvider()) | |
| await store.refreshRoots([root]) | |
| await store.toggleExpansion(for: root.id) | |
| let snapshot = await store.snapshot() | |
| XCTAssertEqual(snapshot.roots.first?.children.map(\.displayName), ["Sources", "README.md"]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/FileExplorerStoreTests.swift` around lines 10 - 26, The test
testLoadingChildrenSortsDirectoriesBeforeFiles creates a temporary directory
(tempDirectory) but never removes it; add cleanup by removing tempDirectory when
the test completes (e.g., immediately after creating tempDirectory, register a
defer or finally-style cleanup that calls FileManager.default.removeItem(at:
tempDirectory) or try? removeItem to avoid failing the test) so the filesystem
is not polluted; reference the tempDirectory variable created in the test and
ensure the cleanup runs even if assertions throw.
| @@ -0,0 +1,93 @@ | |||
| import XCTest | |||
| @testable import cmux_DEV | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Conditional testable import usage in cmuxTests:"
rg -n --type swift '#if canImport\(cmux_DEV\)|@testable import cmux(_DEV)?|#elseif canImport\(cmux\)|#endif' cmuxTests
echo
echo "Top of RemoteFileExplorerProviderTests.swift:"
sed -n '1,20p' cmuxTests/RemoteFileExplorerProviderTests.swiftRepository: manaflow-ai/cmux
Length of output: 14678
Fix RemoteFileExplorerProviderTests.swift to use conditional testable imports.
Line 2 hardcodes cmux_DEV without guards, causing build failures when the module name is cmux (non-DEV targets). All other test files use conditional guards. Apply the standard pattern:
Fix
import XCTest
-@testable import cmux_DEV
+#if canImport(cmux_DEV)
+@testable import cmux_DEV
+#elseif canImport(cmux)
+@testable import cmux
+#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/RemoteFileExplorerProviderTests.swift` at line 2, Replace the
hardcoded testable import in RemoteFileExplorerProviderTests.swift so it uses
the conditional import pattern other tests use: wrap the existing "@testable
import cmux_DEV" in a compile-time guard that tries cmux_DEV first and falls
back to cmux (i.e. use `#if` canImport(cmux_DEV) / `@testable` import cmux_DEV /
`#else` / `@testable` import cmux / `#endif`) so the file builds for both DEV and
non-DEV module names.
| let app = XCUIApplication() | ||
| app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" | ||
| app.launch() | ||
|
|
||
| let toggleButton = app.buttons["titlebarControl.toggleFileExplorer"] | ||
| XCTAssertTrue(toggleButton.waitForExistence(timeout: 6.0)) | ||
|
|
||
| let sidebar = app.descendants(matching: .any).matching(identifier: "FileExplorerSidebar").firstMatch | ||
| XCTAssertFalse(sidebar.exists) |
There was a problem hiding this comment.
Reset persisted window/UI state before asserting the initial explorer state.
These tests assume the titlebar accessory is present and the explorer starts hidden, but both are persisted elsewhere in the app. On a reused defaults container, titlebarControl.toggleFileExplorer can disappear in minimal mode or FileExplorerSidebar can already exist before the first click, which makes the tests order-dependent. Normalize that state at launch (or immediately after launch) before asserting on the button/sidebar.
Also applies to: 35-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/FileExplorerSidebarUITests.swift` around lines 10 - 18, Normalize
persisted UI state right after launching XCUIApplication by resetting the
window/titlebar accessory and sidebar visibility: add/declare a test-only
launchEnvironment flag (e.g. CMUX_UI_TEST_RESET or reuse CMUX_UI_TEST_MODE) that
your app checks at startup to clear persisted UI defaults, or, if you prefer a
pure-test approach, after app.launch() assert and restore state by ensuring the
existence of the titlebarControl.toggleFileExplorer button and that
FileExplorerSidebar is hidden (if FileExplorerSidebar.exists then toggle via
titlebarControl.toggleFileExplorer until hidden; if the toggle button is missing
restore the accessory via the reset flag or a known UI flow). Target symbols:
XCUIApplication, CMUX_UI_TEST_MODE (or CMUX_UI_TEST_RESET),
titlebarControl.toggleFileExplorer, FileExplorerSidebar.
| private static func parseListedEntry(_ rawEntry: [String: Any]) throws -> ListedEntry { | ||
| let canonicalPath = (rawEntry["path"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| let displayName = (rawEntry["name"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| let rawKind = (rawEntry["kind"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
|
|
||
| guard !canonicalPath.isEmpty, !displayName.isEmpty else { | ||
| throw RemoteFileExplorerProviderError.invalidResponse("fs.list returned an entry with missing path or name") | ||
| } |
There was a problem hiding this comment.
Don't trim daemon paths or filenames.
fs.list should round-trip remote names verbatim. Trimming path/name turns " foo" or "foo " into a different entry, so copy/open/retry can target the wrong file. Validate emptiness without mutating the payload.
💡 Proposed fix
- let canonicalPath = (rawEntry["path"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
- let displayName = (rawEntry["name"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
+ let canonicalPath = rawEntry["path"] as? String ?? ""
+ let displayName = rawEntry["name"] as? String ?? ""
let rawKind = (rawEntry["kind"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/FileExplorer/RemoteFileExplorerProvider.swift` around lines 92 - 99,
In parseListedEntry, stop trimming the remote path/name so daemon-provided
whitespace is preserved: replace the trimmed assignments for canonicalPath and
displayName with their raw string values (let canonicalPath = rawEntry["path"]
as? String ?? "" and let displayName = rawEntry["name"] as? String ?? "") and
perform the emptiness guard on those raw values (guard !canonicalPath.isEmpty,
!displayName.isEmpty ...). Do not alter whitespace in path/name; leave rawKind
handling unchanged or treat separately if needed.
| static let fsListCapability = "fs.list" | ||
|
|
There was a problem hiding this comment.
Refresh same-version daemons that don’t advertise fs.list.
This adds a new remote-explorer RPC, but the bootstrap reupload path still only self-heals when proxy.stream.push is missing. Hosts that already have the current-version daemon installed can therefore stay on a binary without fs.list, so every SSH directory expansion keeps failing until that remote binary is manually replaced. Please include WorkspaceRemoteDaemonRPCClient.fsListCapability in the capability-based refresh check inside WorkspaceRemoteSessionController.bootstrapDaemonLocked().
Also applies to: 979-986
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 794 - 795, The capability-based daemon
refresh check in WorkspaceRemoteSessionController.bootstrapDaemonLocked() is
missing fs.list, so daemons that lack
WorkspaceRemoteDaemonRPCClient.fsListCapability won't be refreshed; update the
capability check to include WorkspaceRemoteDaemonRPCClient.fsListCapability
(i.e., add static let fsListCapability = "fs.list" to the set of required
capabilities used in bootstrapDaemonLocked()) so same-version hosts that don't
advertise fs.list will trigger the bootstrap reupload/refresh path.
| func listDirectory(path: String) throws -> [[String: Any]] { | ||
| let result = try call( | ||
| method: "fs.list", | ||
| params: ["path": path], | ||
| timeout: 8.0 | ||
| ) | ||
| return (result["entries"] as? [[String: Any]]) ?? [] | ||
| } |
There was a problem hiding this comment.
Don’t coerce a malformed fs.list reply into an empty folder.
Falling back to [] here hides protocol/version bugs and makes the explorer look empty instead of surfacing the inline error/retry state.
💡 Proposed fix
func listDirectory(path: String) throws -> [[String: Any]] {
let result = try call(
- method: "fs.list",
+ method: Self.fsListCapability,
params: ["path": path],
timeout: 8.0
)
- return (result["entries"] as? [[String: Any]]) ?? []
+ guard let entries = result["entries"] as? [[String: Any]] else {
+ throw NSError(domain: "cmux.remote.daemon.rpc", code: 18, userInfo: [
+ NSLocalizedDescriptionKey: "\(Self.fsListCapability) returned malformed entries",
+ ])
+ }
+ return entries
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 979 - 986, The current
listDirectory(path:) silently coerces a malformed fs.list reply into an empty
directory by using (result["entries"] as? [[String: Any]]) ?? [], which hides
protocol/version bugs; update listDirectory(path:) to validate that
result["entries"] exists and is of type [[String: Any]] and, if not, throw a
descriptive error (e.g. create/throw a WorkspaceError.invalidResponse or a
thrown NSError with context) instead of returning []; keep the call to
call(method: "fs.list", ...) and the returned entries variable name but replace
the nil-coalescing fallback with a guard/if-let that raises the error including
the raw result for debugging.
| private func fileExplorerHostScope() -> FileExplorerHostScope { | ||
| guard let remoteConfiguration else { return .local } | ||
| return .ssh( | ||
| destination: remoteConfiguration.destination, | ||
| port: remoteConfiguration.port, | ||
| identityFingerprint: remoteConfiguration.proxyBrokerTransportKey | ||
| ) | ||
| } |
There was a problem hiding this comment.
Scope explorer roots per terminal, not per workspace.
These helpers flip the whole workspace to SSH as soon as remoteConfiguration exists, but terminalStartupCommand is optional and newTerminalSurface / newTerminalSplit still create local terminals when it’s nil. In those mixed or browser-proxy workspaces, local panel directories get tagged and canonicalized as remote roots, so the explorer can browse or merge against the wrong machine. Please derive hostScope and home expansion from each terminal root instead of a workspace-wide switch.
Also applies to: 6413-6440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 6394 - 6401, The current
fileExplorerHostScope() flips the whole workspace to SSH whenever
workspace.remoteConfiguration exists; instead, change it to compute host scope
and home expansion per-terminal root: add a variant like
fileExplorerHostScope(for root: TerminalRoot or for startupCommand: String?)
that checks the terminal-specific startup command / root metadata to decide
.local vs .ssh (using remoteConfiguration only when that particular terminal
indicates a remote target), update callers such as newTerminalSurface and
newTerminalSplit to call the per-terminal hostScope function or pass the
terminal's startupCommand/root info, and apply the same per-root logic where
hostScope/home expansion are used in the 6413-6440 region so local panel
directories are not canonicalized as remote. Ensure references to
remoteConfiguration, terminalStartupCommand, newTerminalSurface,
newTerminalSplit, hostScope, and home expansion are updated accordingly.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Update/UpdateTitlebarAccessory.swift">
<violation number="1" location="Sources/Update/UpdateTitlebarAccessory.swift:1102">
P3: The file-explorer titlebar button tooltip is initialized once and will show stale shortcut text after shortcut settings change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/ContentView.swift (2)
2775-2775:⚠️ Potential issue | 🟡 MinorSanitize non-finite file-explorer widths before tolerance checks.
Line 2775 assigns persisted width directly, and later comparisons (Line 3200, Line 3260) use
abs(...)without finite guards. Anan/infwidth can bypass all repair branches and remain persisted.Possible fix
- fileExplorerWidth = fileExplorerState.persistedWidth + let restoredFileExplorerWidth = normalizedFileExplorerWidth(fileExplorerState.persistedWidth) + if !fileExplorerWidth.isFinite || abs(fileExplorerWidth - restoredFileExplorerWidth) > 0.5 { + fileExplorerWidth = restoredFileExplorerWidth + } + if !fileExplorerState.persistedWidth.isFinite + || abs(fileExplorerState.persistedWidth - restoredFileExplorerWidth) > 0.5 { + fileExplorerState.persistedWidth = restoredFileExplorerWidth + } view = AnyView(view.onChange(of: fileExplorerWidth) { _ in let sanitized = normalizedFileExplorerWidth(fileExplorerWidth) - if abs(fileExplorerWidth - sanitized) > 0.5 { + if !fileExplorerWidth.isFinite || abs(fileExplorerWidth - sanitized) > 0.5 { fileExplorerWidth = sanitized return } - if abs(fileExplorerState.persistedWidth - sanitized) > 0.5 { + if !fileExplorerState.persistedWidth.isFinite + || abs(fileExplorerState.persistedWidth - sanitized) > 0.5 { fileExplorerState.persistedWidth = sanitized } view = AnyView(view.onChange(of: fileExplorerState.isVisible) { _ in if fileExplorerState.isVisible { let restoredWidth = normalizedFileExplorerWidth(fileExplorerState.persistedWidth) - if abs(fileExplorerWidth - restoredWidth) > 0.5 { + if !fileExplorerWidth.isFinite || abs(fileExplorerWidth - restoredWidth) > 0.5 { fileExplorerWidth = restoredWidth } @@ view = AnyView(view.onChange(of: fileExplorerState.persistedWidth) { newValue in let sanitized = normalizedFileExplorerWidth(newValue) - if abs(newValue - sanitized) > 0.5 { + if !newValue.isFinite || abs(newValue - sanitized) > 0.5 { fileExplorerState.persistedWidth = sanitized return } guard activeResizerHandle != .fileExplorerDivider else { return } - if abs(fileExplorerWidth - sanitized) > 0.5 { + if !fileExplorerWidth.isFinite || abs(fileExplorerWidth - sanitized) > 0.5 { fileExplorerWidth = sanitized } })Also applies to: 3198-3206, 3225-3237, 3258-3267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` at line 2775, When assigning fileExplorerWidth from fileExplorerState.persistedWidth, ensure the value is finite before using it in tolerance checks: replace the direct assignment to fileExplorerWidth with a sanitized value (e.g., if persistedWidth.isFinite then persistedWidth else a safe fallback like a defaultWidth or clamp to a valid range). Also add finite guards before any abs(...) comparisons in the tolerance logic that references fileExplorerWidth (the comparisons around the tolerance branches) so NaN/inf can't bypass repair branches; use persistedWidth.isFinite (or fileExplorerWidth.isFinite) to decide whether to run the normal tolerance logic or to reset/repair the width. Ensure you update references to fileExplorerWidth, fileExplorerState.persistedWidth, and the tolerance-checking code paths so all branches operate on a verified finite value.
2148-2170:⚠️ Potential issue | 🟠 MajorHandle hover-band detection for the right divider too.
Line 2156 hard-gates on
sidebarState.isVisible, and Line 2169 only checks the left divider band. When the left sidebar is hidden and only the file explorer is visible, cursor stabilization can be force-released for the right divider.Possible fix
- private func dividerBandContains(pointInContent point: NSPoint, contentBounds: NSRect) -> Bool { + private func dividerBandContains( + pointInContent point: NSPoint, + contentBounds: NSRect, + handle: SidebarResizerHandle + ) -> Bool { guard point.y >= contentBounds.minY, point.y <= contentBounds.maxY else { return false } - let minX = sidebarWidth - sidebarResizerHitWidthPerSide - let maxX = sidebarWidth + sidebarResizerHitWidthPerSide + let centerX: CGFloat + switch handle { + case .divider: + centerX = sidebarWidth + case .fileExplorerDivider: + centerX = contentBounds.width - fileExplorerWidth + } + let minX = centerX - sidebarResizerHitWidthPerSide + let maxX = centerX + sidebarResizerHitWidthPerSide return point.x >= minX && point.x <= maxX } private func updateSidebarResizerBandState(using event: NSEvent? = nil) { - guard sidebarState.isVisible, + guard (sidebarState.isVisible || fileExplorerState.isVisible), let window = observedWindow, let contentView = window.contentView else { isResizerBandActive = false scheduleSidebarResizerCursorRelease(force: true) return } @@ - let isInDividerBand = dividerBandContains(pointInContent: pointInContent, contentBounds: contentView.bounds) + let isInDividerBand = + (sidebarState.isVisible && + dividerBandContains( + pointInContent: pointInContent, + contentBounds: contentView.bounds, + handle: .divider + )) + || (fileExplorerState.isVisible && + dividerBandContains( + pointInContent: pointInContent, + contentBounds: contentView.bounds, + handle: .fileExplorerDivider + )) isResizerBandActive = isInDividerBandAlso applies to: 2211-2240
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/ContentView.swift`:
- Line 2775: When assigning fileExplorerWidth from
fileExplorerState.persistedWidth, ensure the value is finite before using it in
tolerance checks: replace the direct assignment to fileExplorerWidth with a
sanitized value (e.g., if persistedWidth.isFinite then persistedWidth else a
safe fallback like a defaultWidth or clamp to a valid range). Also add finite
guards before any abs(...) comparisons in the tolerance logic that references
fileExplorerWidth (the comparisons around the tolerance branches) so NaN/inf
can't bypass repair branches; use persistedWidth.isFinite (or
fileExplorerWidth.isFinite) to decide whether to run the normal tolerance logic
or to reset/repair the width. Ensure you update references to fileExplorerWidth,
fileExplorerState.persistedWidth, and the tolerance-checking code paths so all
branches operate on a verified finite value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f875b4a7-00d9-475b-9258-4ddbdead30ae
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/Update/UpdateTitlebarAccessory.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Update/UpdateTitlebarAccessory.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 953a344f42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nodes.flatMap { node in | ||
| let descendantIDs = node.isExpanded ? refreshableNodeIDs(in: node.children) : [] | ||
| return [node.id] + descendantIDs |
There was a problem hiding this comment.
The refresh button currently walks every visible node ID, including files, because refreshableNodeIDs always returns node.id regardless of kind. In trees with expanded directories, refreshAll will therefore call store.refreshNode on file entries; local/remote providers then attempt directory listing on file paths, producing per-file errors and unnecessary RPC/file-system calls. This is user-visible after pressing Refresh: valid file rows can suddenly show error states even though only directory reloads were intended.
Useful? React with 👍 / 👎.
| cleanedPath := filepath.Clean(requestedPath) | ||
| directoryEntries, err := os.ReadDir(cleanedPath) |
There was a problem hiding this comment.
fs.list trims and cleans the incoming path, then directly calls os.ReadDir on it. When the client sends unresolved tilde roots (for example remote startup states where only ~/~/... directories are known before an absolute home is inferred), this reads a literal ~ path and returns not_found even though the target directory exists. Handling ~ expansion on the daemon side (or rejecting it explicitly with a dedicated error) would prevent these false failures in common SSH workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3775-3776: Remove the default FileExplorerSidebarState argument
from registerMainWindow so callers must pass the SwiftUI-owned instance; update
the registerMainWindow signature to require fileExplorerState:
FileExplorerSidebarState (no default) and change all callers (e.g.,
ContentView.onAppear where the SwiftUI-owned FileBrowserDrawerState is passed,
and AppDelegate.createMainWindow where secondary windows construct and pass
their own FileBrowserDrawerState) to supply the correct state object; ensure any
other overloads or nearby registerMainWindow usages are updated to match this
ownership pattern to prevent silent registration of detached instances.
| fileExplorerState: FileExplorerSidebarState = FileExplorerSidebarState(), | ||
| sidebarSelectionState: SidebarSelectionState |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Drop the default FileExplorerSidebarState argument in registerMainWindow.
Default-constructing state here can silently register a detached instance if a caller forgets to pass the SwiftUI-owned object, which risks window state desync.
♻️ Suggested fix
func registerMainWindow(
_ window: NSWindow,
windowId: UUID,
tabManager: TabManager,
sidebarState: SidebarState,
- fileExplorerState: FileExplorerSidebarState = FileExplorerSidebarState(),
+ fileExplorerState: FileExplorerSidebarState,
sidebarSelectionState: SidebarSelectionState
) {Based on learnings: "AppDelegate.registerMainWindow ownership pattern: the primary window registers from ContentView.onAppear and passes the SwiftUI-owned FileBrowserDrawerState; secondary windows created via AppDelegate.createMainWindow construct and pass their own FileBrowserDrawerState. This mirrors SidebarState and avoids re-registration mismatches."
Also applies to: 3790-3797
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 3775 - 3776, Remove the default
FileExplorerSidebarState argument from registerMainWindow so callers must pass
the SwiftUI-owned instance; update the registerMainWindow signature to require
fileExplorerState: FileExplorerSidebarState (no default) and change all callers
(e.g., ContentView.onAppear where the SwiftUI-owned FileBrowserDrawerState is
passed, and AppDelegate.createMainWindow where secondary windows construct and
pass their own FileBrowserDrawerState) to supply the correct state object;
ensure any other overloads or nearby registerMainWindow usages are updated to
match this ownership pattern to prevent silent registration of detached
instances.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddb22c0bc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| method: "fs.list", | ||
| params: ["path": path], | ||
| timeout: 8.0 |
There was a problem hiding this comment.
This new RPC path calls fs.list unconditionally, but WorkspaceRemoteDaemonRPCClient.start() still only enforces proxy.stream.push. On hosts that already have an older cmuxd-remote binary (same versioned path, no fs.list capability), every directory expansion now fails with unknown_method instead of triggering an upgrade path. Please check for fs.list capability (or force re-bootstrap) before issuing this call so remote explorer works after rolling updates.
Useful? React with 👍 / 👎.
| let canonicalPath = (rawEntry["path"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| let displayName = (rawEntry["name"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" |
There was a problem hiding this comment.
Trimming path and name here corrupts valid Unix filenames that intentionally start/end with spaces (for example "foo "), and can collapse distinct entries onto the same canonical path. That causes wrong display names and follow-up refresh/open requests to target the wrong file. These fields should be parsed as-is and only validated for decode failure, not normalized with whitespace trimming.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxUITests/FileExplorerSidebarUITests.swift (1)
9-18:⚠️ Potential issue | 🟠 MajorNormalize persisted explorer state before initial assertions/clicks.
Line 18 and Line 41 assume a hidden-at-launch sidebar, but explorer visibility is persisted/restored, so these tests can become order-dependent/flaky on reused state. Please normalize to a known hidden state right after launch (or use a dedicated reset launch flag) before asserting/clicking.
Proposed stabilization diff
final class FileExplorerSidebarUITests: XCTestCase { @@ func testTitlebarButtonTogglesFileExplorerSidebar() { - let app = XCUIApplication() - app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" - app.launch() - - let toggleButton = app.buttons["titlebarControl.toggleFileExplorer"] - XCTAssertTrue(toggleButton.waitForExistence(timeout: 6.0)) - - let sidebar = app.descendants(matching: .any).matching(identifier: "FileExplorerSidebar").firstMatch - XCTAssertFalse(sidebar.exists) + let (app, toggleButton, sidebar) = launchAppWithExplorerNormalizedHidden() + XCTAssertFalse(sidebar.exists) toggleButton.click() @@ func testFileExplorerSidebarResizerTracksDrag() { - let app = XCUIApplication() - app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" - app.launch() - - let toggleButton = app.buttons["titlebarControl.toggleFileExplorer"] - XCTAssertTrue(toggleButton.waitForExistence(timeout: 6.0)) + let (app, toggleButton, _) = launchAppWithExplorerNormalizedHidden() toggleButton.click() @@ } + private func launchAppWithExplorerNormalizedHidden() -> (app: XCUIApplication, toggleButton: XCUIElement, sidebar: XCUIElement) { + let app = XCUIApplication() + app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" + app.launch() + + let toggleButton = app.buttons["titlebarControl.toggleFileExplorer"] + XCTAssertTrue(toggleButton.waitForExistence(timeout: 6.0)) + + let sidebar = app.descendants(matching: .any).matching(identifier: "FileExplorerSidebar").firstMatch + if sidebar.exists { + toggleButton.click() + let hidden = XCTNSPredicateExpectation( + predicate: NSPredicate(format: "exists == false"), + object: sidebar + ) + XCTAssertEqual(XCTWaiter().wait(for: [hidden], timeout: 6.0), .completed) + } + return (app, toggleButton, sidebar) + } + private func waitForElementHittable(_ element: XCUIElement, timeout: TimeInterval) -> Bool {Also applies to: 34-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/FileExplorerSidebarUITests.swift` around lines 9 - 18, The test testTitlebarButtonTogglesFileExplorerSidebar is assuming the sidebar is hidden at launch but explorer visibility is persisted, so after launching the app (XCUIApplication().launch()) normalize state: find the sidebar (app.descendants(matching: .any).matching(identifier: "FileExplorerSidebar").firstMatch) and if it exists, use the titlebar toggle button (app.buttons["titlebarControl.toggleFileExplorer"]) to hide it (or set a dedicated launchEnvironment flag like "RESET_EXPLORER_STATE" and assert state is hidden) before performing the initial XCTAssertFalse and subsequent clicks; apply the same normalization in the other similar test block (lines 34-45).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxUITests/FileExplorerSidebarUITests.swift`:
- Around line 9-18: The test testTitlebarButtonTogglesFileExplorerSidebar is
assuming the sidebar is hidden at launch but explorer visibility is persisted,
so after launching the app (XCUIApplication().launch()) normalize state: find
the sidebar (app.descendants(matching: .any).matching(identifier:
"FileExplorerSidebar").firstMatch) and if it exists, use the titlebar toggle
button (app.buttons["titlebarControl.toggleFileExplorer"]) to hide it (or set a
dedicated launchEnvironment flag like "RESET_EXPLORER_STATE" and assert state is
hidden) before performing the initial XCTAssertFalse and subsequent clicks;
apply the same normalization in the other similar test block (lines 34-45).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4202cf25-e8a8-4ffc-aebc-13ec49a9fd65
📒 Files selected for processing (1)
cmuxUITests/FileExplorerSidebarUITests.swift
lawrencecchen
force-pushed
the
task-file-explorer-ssh-sidebar
branch
from
29168ac to
011d9b5
Compare
lawrencecchen
changed the title
Adds a right-side file explorer panel toggled via Cmd-Shift-E or a titlebar button. Uses native NSOutlineView for Finder-like disclosure, rounded row selection, alternating backgrounds, and 13pt medium text. Local workspaces use FileManager, SSH workspaces use ssh commands via the existing connection. Root paths display with ~ for home-relative paths. Expanded nodes persist across provider changes so SSH nodes re-hydrate when the connection becomes available. Includes configurable keyboard shortcut (KeyboardShortcutSettings), localized strings (EN/JA), and unit tests for path resolution and store hydration behavior.
lawrencecchen
force-pushed
the
task-file-explorer-ssh-sidebar
branch
from
011d9b5 to
84dc4ea
Compare
lawrencecchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84dc4ea8c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| rootNodes = children | ||
| isRootLoading = false |
There was a problem hiding this comment.
cancelAllLoads() cancels in-flight tasks, but loadChildren still writes results to the model after await provider.listDirectory(...) without checking cancellation or request generation. Because the SSH provider path is continuation-based and does not cooperatively cancel, an older request can finish later and overwrite rootNodes/children for a newer directory or workspace after rapid tab/directory switches.
Useful? React with 👍 / 👎.
| let newCount = store.rootNodes.count | ||
| if newCount != lastRootNodeCount { | ||
| lastRootNodeCount = newCount |
There was a problem hiding this comment.
reloadIfNeeded only calls outlineView.reloadData() when the number of root nodes changes. If the user switches to a different directory with the same item count, the outline keeps stale row items from the previous root and refreshLoadedNodes only refreshes those existing items, so the sidebar can display the wrong folder contents.
Useful? React with 👍 / 👎.
| if let existingProvider = fileExplorerStore.provider as? SSHFileExplorerProvider, | ||
| existingProvider.destination == config?.destination { |
There was a problem hiding this comment.
Provider reuse is keyed only by destination, so workspaces that share host/user but differ in port, identityFile, or sshOptions will keep using the old provider configuration. That causes directory listing requests to run with stale SSH connection/auth parameters, which can point to the wrong endpoint or fail unexpectedly when switching between such workspaces.
Useful? React with 👍 / 👎.
| let toggle = FileExplorerTitlebarAccessoryViewController(onToggle: { | ||
| AppDelegate.shared?.fileExplorerState?.toggle() | ||
| }) |
There was a problem hiding this comment.
Each created main window gets its own FileExplorerState, but this titlebar toggle always mutates AppDelegate.shared?.fileExplorerState (a single global reference). In multi-window use, clicking the toggle in one window can change a different window’s sidebar visibility and leave the clicked window unchanged.
Useful? React with 👍 / 👎.
The FileExplorerTitlebarAccessoryViewController triggered an infinite Auto Layout loop by calling invalidateIntrinsicContentSize during viewDidLayout, causing NSGenericException. Replaced with a one-shot size computation in init + a single re-measure in viewDidAppear.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3ac7d679d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lastSidebarSelectionIndex = tabManager.tabs.firstIndex { $0.id == newValue } | ||
| } | ||
| updateTitlebarText() | ||
| syncFileExplorerDirectory() |
There was a problem hiding this comment.
syncFileExplorerDirectory() is only wired in onAppear and onChange(of: tabManager.selectedTabId), so the sidebar root is never re-synced when the selected workspace’s currentDirectory changes in-place (for example after cd in the same tab). In that common flow, the file explorer keeps showing the old folder until the user switches tabs or recreates the view.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| do { | ||
| let entries = try await provider.listDirectory(path: path) |
There was a problem hiding this comment.
loadChildren runs on @MainActor and directly awaits provider.listDirectory(path:); for LocalFileExplorerProvider this performs synchronous FileManager directory reads, so expanding large directories will block the UI thread. The listing work should run off-main and only publish model updates back on main.
Useful? React with 👍 / 👎.
| case .providerUnavailable: | ||
| return String(localized: "fileExplorer.error.unavailable", defaultValue: "File explorer is not available") | ||
| case .sshCommandFailed(let detail): | ||
| return String(localized: "fileExplorer.error.sshFailed", defaultValue: "SSH command failed: \(detail)") |
There was a problem hiding this comment.
FileExplorerError.sshCommandFailed uses the key fileExplorer.error.sshFailed, but this commit does not add that key to Resources/Localizable.xcstrings. That means localized builds fall back to the English default for this user-facing error path instead of showing a translated string.
Useful? React with 👍 / 👎.
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleFileExplorer)) { | ||
| fileExplorerState?.toggle() | ||
| return true |
There was a problem hiding this comment.
createMainWindow creates a separate FileExplorerState per window, but the Cmd-Shift-E handler toggles the single AppDelegate.fileExplorerState reference. In multi-window sessions this can toggle the wrong window (typically the primary one) instead of the focused window, so shortcut behavior is inconsistent with per-window sidebar state.
Useful? React with 👍 / 👎.
|
great PR! pls pls merge it |
…les, FS watching - Wire FileExplorerView into right panel with resize handle - Reactive CWD sync via Combine (no polling) - Git status colors (modified/added/deleted/renamed/untracked) for local and SSH - Context menu: Open in Default Editor, Reveal in Finder, Copy Path, Copy Relative Path - Drag file to terminal pastes path - Hidden files toggle (eye icon in header) - DispatchSource-based directory watching with 300ms debounce - Footer folder icon toggle button - Shortcut changed to Cmd+Option+B - Fix SSH pipe deadlock (read before waitUntilExit) - Fix SSH infinite reload loop (guard hydrateExpandedNodes on path change) - Debug logging for file explorer state changes
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 165d312960
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| workspace.$currentDirectory.eraseToAnyPublisher() | ||
| } | ||
| .switchToLatest() | ||
| .removeDuplicates() |
There was a problem hiding this comment.
The .removeDuplicates() here dedupes only by currentDirectory string, so switching to a different workspace that happens to have the same directory value does not call syncFileExplorerDirectory(). In that case the explorer can remain bound to the previous workspace/provider context (e.g., local vs SSH, destination/availability) until the directory text changes later, which makes the sidebar show stale data after normal tab switches.
Useful? React with 👍 / 👎.
| DispatchQueue.main.async { [weak self] in | ||
| self?.gitStatusByPath = status | ||
| } |
There was a problem hiding this comment.
refreshGitStatus() captures rootPath and fetches status off-main, but completion always assigns gitStatusByPath without verifying the request still matches the current root/provider. If users change directories quickly, an older/slower fetch can overwrite badges for the new tree with stale statuses from the previous path. Add request-generation or path/provider checks before assignment.
Useful? React with 👍 / 👎.
| ? String(localized: "sidebar.fileExplorer.hide", defaultValue: "Hide File Explorer") | ||
| : String(localized: "sidebar.fileExplorer.show", defaultValue: "Show File Explorer")) | ||
| .accessibilityLabel(fileExplorerState.isVisible | ||
| ? String(localized: "sidebar.fileExplorer.hide", defaultValue: "Hide File Explorer") | ||
| : String(localized: "sidebar.fileExplorer.show", defaultValue: "Show File Explorer")) |
There was a problem hiding this comment.
These new UI strings use keys like sidebar.fileExplorer.hide/show, but those keys are not added in Resources/Localizable.xcstrings in this commit (same for several other new file-explorer keys). That causes localized builds to fall back to English defaults for user-facing labels/tooltips, which is a regression in localization coverage for this feature.
Useful? React with 👍 / 👎.
- Move file explorer button into trailing NSTitlebarAccessoryVC (right-aligned) - Share modifier state via .titlebarShortcutHintsVisibilityChanged notification from TitlebarShortcutHintModifierMonitor (same timing, same delay as left hints) - Render hint pill in child NSPanel (borderless, nonactivating, ignoresMouseEvents) to float above terminal portal z-order - Extract ShortcutHintPill as shared component (sidebar tabs, left titlebar, explorer) - Fix animation: DispatchQueue.main.async in performKeyEquivalent to escape AppKit's implicit NSAnimationContext; .transition(.identity) on panel views; always-in-tree rendering with width=0 when hidden instead of conditional insertion - Show hidden files by default, remove toggle button from header - Increase disclosure triangle leading margin via frameOfOutlineCell/frameOfCell - Add .fullScreenAuxiliary + .ignoresCycle to hint panel - Style debug window in Debug > Debug Windows > File Explorer Style Debug
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Update/UpdateTitlebarAccessory.swift">
<violation number="1" location="Sources/Update/UpdateTitlebarAccessory.swift:1290">
P2: Feature-flag toggles won’t update existing windows because reattach short-circuits on already attached windows.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
cmux/Sources/Update/UpdateTitlebarAccessory.swift
Line 1377 in 451e807
Re-attach file explorer accessory after feature-flag flips
reattachIfFileExplorerFlagChanged() calls attachToExistingWindows(), but attachIfNeeded returns early when the window is already in attachedWindows. Because of that early return, toggling fileExplorer.featureEnabled at runtime does not actually add or remove the trailing titlebar file-explorer button on existing windows; the UI only updates after recreating windows.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private func syncFileExplorerDirectory() { | ||
| guard let selectedId = tabManager.selectedTabId, | ||
| let tab = tabManager.tabs.first(where: { $0.id == selectedId }) else { | ||
| return |
There was a problem hiding this comment.
syncFileExplorerDirectory() runs unconditionally from the currentDirectory stream, and this method immediately configures providers and starts store work even when fileExplorerState.isFeatureEnabled is false. Since setRootPath triggers reload and git-status fetches (including SSH commands for remote workspaces), users with the feature disabled still pay background filesystem/network cost for a hidden panel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/FileExplorerStore.swift">
<violation number="1" location="Sources/FileExplorerStore.swift:466">
P2: Observing only `.fileExplorerFeatureToggled` misses non-UI/defaults-driven changes to the feature flag, so `FileExplorerState.isFeatureEnabled` can drift from `UserDefaults`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebf2df8384
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tabManager.$selectedTabId | ||
| .compactMap { [weak tabManager] tabId -> Workspace? in | ||
| guard let tabId, let tabManager else { return nil } | ||
| return tabManager.tabs.first(where: { $0.id == tabId }) | ||
| } | ||
| .map { workspace -> AnyPublisher<String, Never> in | ||
| workspace.$currentDirectory.eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
The explorer sync pipeline only subscribes to workspace.$currentDirectory, so remote availability flips (for example remoteDaemonStatus.state moving from non-ready to ready with the same directory string) do not trigger syncFileExplorerDirectory(). In that case the SSH provider stays marked unavailable and the tree never hydrates until the directory text changes, which can leave the sidebar stuck empty after a normal remote connect.
Useful? React with 👍 / 👎.
| if newProvider?.isAvailable == true { | ||
| reload() | ||
| } |
There was a problem hiding this comment.
setProvider(_:) immediately calls reload() when the new provider is available, but callers then set a new root path right after. During workspace/provider switches this causes an unnecessary first load on the previous rootPath with the new provider (often invalid remotely), producing extra IO/SSH calls and transient error state before the real path load runs.
Useful? React with 👍 / 👎.
2 participants
Summary
NSOutlineViewfor disclosure, rounded inset row selection, alternating backgrounds, folder icons, and 13pt medium textFileManager, SSH workspaces usesshcommands with the existing connection config~for home-relative paths (both local and SSH)KeyboardShortcutSettingsand shows its hint pill when holding CmdTesting
./scripts/reload.sh --tag task-file-explorer-ssh-sidebarFileExplorerRootResolverTests(path display with~, SSH home paths, edge cases) andFileExplorerStoreTests(expansion persistence, SSH hydration, error clearing, provider recreation)Related