Repository navigation
Fix important issue regressions - #5240
azooz2003-bit wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR enhances remote workspace file handling and Claude session restoration. It adds SSH-based remote file downloads for preview caching, introduces explicit root-path support for remote workspaces, improves Claude workflow transcript selection during session restoration, refactors terminal text retrieval into a snapshot/payload pipeline, and provides comprehensive test coverage for the new features. ChangesRemote File Preview & Session Management
Sequence Diagram(s)sequenceDiagram
participant FileExplorerStore
participant ProcessSSHFileExplorerTransport
participant SSHDownloadCommandProcess
participant LocalFileSystem
FileExplorerStore->>ProcessSSHFileExplorerTransport: downloadFile(path)
ProcessSSHFileExplorerTransport->>SSHDownloadCommandProcess: execute SSH command
SSHDownloadCommandProcess->>LocalFileSystem: create destination
SSHDownloadCommandProcess->>LocalFileSystem: write file content
SSHDownloadCommandProcess-->>ProcessSSHFileExplorerTransport: result
ProcessSSHFileExplorerTransport-->>FileExplorerStore: local URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (6 errors, 1 warning, 3 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes three regressions: SSH file-browser roots now follow the workspace
Confidence Score: 3/5The SSH preview download path has two production correctness issues that need addressing before merge. The Claude Workflow resume fix and the TerminalController refactor are clean and well-tested. The SSH remote file preview feature introduces a concurrent-write race — two rapid clicks on the same remote file produce two SSHDownloadCommandProcess instances that both truncate and stream to the same cache path simultaneously, which can silently corrupt the preview. Separately, copyDataToEndOfFileOrDiscard returns early on a write failure without draining its input pipe, leaving the SSH child process blocked on a full pipe buffer while waitUntilExit() spins forever on the background thread. Sources/FileExplorerStore.swift (materializeRemoteFileForPreview and SSHDownloadCommandProcess) and Sources/ProcessPipeReader.swift (copyDataToEndOfFileOrDiscard) Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (click file)
participant CV as ContentView / RightSidebarToolPanel
participant FES as FileExplorerStore
participant SSH as SSHDownloadCommandProcess
participant FS as FileSystem (tmp)
participant WS as Workspace
U->>CV: openFilePreview(filePath)
CV->>CV: isRemoteWorkspace?
alt remote
CV->>FES: materializeRemoteFileForPreview(path)
FES->>FES: remotePreviewCacheURL(displayTarget, remotePath)
FES->>SSH: downloadFile(path, to: cacheURL)
SSH->>FS: createFile(cacheURL) [truncate]
SSH->>SSH: ssh … cat -- 'path'
SSH->>FS: copyDataToEndOfFileOrDiscard(stdout → cacheURL)
SSH-->>FES: cacheURL (local URL)
FES-->>CV: localURL
CV->>WS: openFileSurfaces([localURL.path])
else local
CV->>WS: openFileSurfaces([filePath])
end
Reviews (1): Last reviewed commit: "fix: address important issue regressions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func materializeRemoteFileForPreview(path: String) async throws -> URL { | ||
| guard let sshProvider = provider as? SSHFileExplorerProvider else { | ||
| throw FileExplorerError.providerUnavailable | ||
| } | ||
| let cacheURL = Self.remotePreviewCacheURL( | ||
| displayTarget: sshProvider.displayTarget, | ||
| remotePath: path | ||
| ) | ||
| try await sshProvider.downloadFile(path: path, to: cacheURL) | ||
| return cacheURL | ||
| } |
There was a problem hiding this comment.
remotePreviewCacheURL is deterministic — two rapid calls for the same remote path (e.g. a quick double-click) produce the same cacheURL. Each call then creates a separate SSHDownloadCommandProcess that truncates the file at line 579 (FileManager.default.createFile(atPath: outputURL.path, contents: nil)) and opens an independent FileHandle for writing. Both SSH processes stream bytes to the same path concurrently, producing an interleaved/corrupt file. A guard that serialises or coalesces in-flight downloads for a given path is needed before the downloadFile call.
| } catch { | ||
| logReadFailure( | ||
| ProcessPipeReadError(operation: "copyDataToEndOfFile.write", errnoCode: EIO), | ||
| fileDescriptor: input.fileDescriptor, | ||
| partialByteCount: copiedBytes | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
waitUntilExit()
When output.write throws (e.g. disk full), the function returns at line 116 with input still live and unread. The remote cat process continues writing into the SSH pipe until the 64 KB kernel pipe buffer fills, then blocks. The caller in SSHDownloadCommandProcess.run() subsequently calls process.waitUntilExit() at line 607, which never returns because the blocked child process is waiting for the reader — a permanent deadlock on the DispatchQueue.global thread. When write fails, the loop should fall through to read-and-discard mode rather than returning immediately, or the process should be terminated before returning.
| let result = try await withTaskCancellationHandler { | ||
| try await withCheckedThrowingContinuation { continuation in | ||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| continuation.resume(with: Result { try commandProcess.run() }) | ||
| } | ||
| } | ||
| } onCancel: { | ||
| commandProcess.terminate() | ||
| } |
There was a problem hiding this comment.
DispatchQueue.global for async work — cmux-swift-concurrency-modernization
withCheckedThrowingContinuation + DispatchQueue.global.async to bridge a blocking Process.run() into Swift concurrency is the legacy completion-handler pattern flagged by the concurrency-modernization rule. The existing SSHCommandProcess in this file uses the same pattern; both should use a dedicated blocking executor rather than borrowing from the cooperative pool-adjacent global queue.
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } catch { | ||
| NSSound.beep() | ||
| } |
There was a problem hiding this comment.
Both RightSidebarToolPanel.openFilePreview (line 107) and ContentView (line 2594) swallow all errors from materializeRemoteFileForPreview with only NSSound.beep(). The user sees the file explorer tree, clicks a file, hears a beep, and has no way to tell whether the remote connection dropped, the file was deleted, or disk space ran out. At minimum the error should be surfaced through the workspace's existing error-presentation path with a message stating what happened in cmux terms.
This branch was successfully deployed
1 participant
Summary
Issues
Verification
Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes regressions in remote SSH browsing, Claude Workflow auto-resume, and terminal text capture, addressing #5237 #5224 #5221 #5209. Remote files open reliably, sessions resume correctly, and terminal reads are faster.
Bug Fixes
.jsonltranscript for auto-resume.Refactors
Written for commit 48f7d9c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests