Open diff viewer pane immediately#5047
Open
lawrencecchen wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR splits diff viewer HTML into an immediate opening page (app-scheme) and a completion page (HTTP), adds client polling that requests a scheme-handler wait URL, implements a replacement-wait watcher that streams replacement HTML when ready (or times out), and updates tests and URL resolution to exercise the new flows. ChangesDiff Viewer URL Transport and Opening Pipeline
Browser Panel Replacement-Wait Scheme Handler
Test Updates for Pending Viewer Flow
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (8 errors, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR changes the git diff viewer open flow to show a lightweight
Confidence Score: 4/5Safe to merge with the previously noted blocking I/O and repoRoot defects acknowledged; no new correctness regressions introduced by this PR's changes. The event-driven wait mechanism in BrowserPanel.swift is well-synchronized (replacementWaitFinished flag, NSCondition drain, DispatchSource cancel on every exit path), and the two-phase opening/completion target split in cmux_open.swift is structurally sound. The previously flagged blocking I/O calls in startWaitingForReplacement and the raw-cwd repoRoot passed to the error-state writeDiffViewerStatusHTML both remain open; neither is introduced fresh by this PR but neither has been fixed. No new defects were found beyond those already on record. Sources/Panels/BrowserPanel.swift (startWaitingForReplacement blocking I/O — previous thread); CLI/cmux_open.swift (context.repoRoot in error catch — previous outside-diff comment) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux_open CLI
participant App as cmux App (main thread)
participant SH as CmuxDiffViewerURLSchemeHandler
participant SQ as streamQueue
participant Browser as WKWebView
CLI->>CLI: writeDiffViewerOpeningStatusHTML()
CLI->>App: open_split(cmux-diff-viewer://token/opening.html)
App->>SH: webView(_:start:) for opening.html
SH->>SQ: enqueueStreamingFile(opening.html)
SQ-->>Browser: HTTP/200 pending HTML
Note over Browser: Shows spinner toolbar
par Deferred work
CLI->>CLI: makeDiffViewerGitCompletionHTMLSetTarget()
CLI->>CLI: writeCompleteGitDiffViewerHTMLSet()
CLI->>CLI: writeDiffViewerEmbeddedHTML() overwrites opening.html
and JS polling
Browser->>SH: fetch(cmux-diff-viewer://token/__cmux_diff_viewer_wait/opening.html)
SH->>SH: startWaitingForReplacement() DispatchSource watches file
SH-->>SQ: file replaced, enqueueStreamingFile(updated opening.html)
SQ-->>Browser: HTTP/200 embedded HTML with data-cmux-diff-embed-url
Browser->>Browser: showEmbeddedViewer() or replaceDocumentWith()
Browser->>Browser: iframe loads http://127.0.0.1/token/diff.html
end
Reviews (2): Last reviewed commit: "fix: open diff viewer pane immediately" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2795
to
+2851
| private func startWaitingForReplacement( | ||
| _ file: RegisteredFile, | ||
| requestURL: URL, | ||
| urlSchemeTask: WKURLSchemeTask, | ||
| taskID: ObjectIdentifier | ||
| ) { | ||
| guard isSchemeTaskActive(taskID) else { return } | ||
| guard diffViewerReplacementIsPending(file.fileURL) else { | ||
| enqueueStreamingFile(file, requestURL: requestURL, urlSchemeTask: urlSchemeTask, taskID: taskID) | ||
| return | ||
| } | ||
|
|
||
| let fd = open(file.fileURL.path, O_EVTONLY) | ||
| guard fd >= 0 else { | ||
| failSchemeTask(taskID, urlSchemeTask: urlSchemeTask, errorCode: NSURLErrorFileDoesNotExist) | ||
| return | ||
| } | ||
|
|
||
| let watcher = DispatchSource.makeFileSystemObjectSource( | ||
| fileDescriptor: fd, | ||
| eventMask: [.write, .extend, .attrib, .delete, .rename], | ||
| queue: streamQueue | ||
| ) | ||
| let timeout = DispatchSource.makeTimerSource(queue: streamQueue) | ||
|
|
||
| watcher.setEventHandler { [weak self, weak watcher, weak timeout] in | ||
| guard let self else { return } | ||
| guard !self.diffViewerReplacementIsPending(file.fileURL) else { return } | ||
| guard self.finishReplacementWait(taskID, watcher: watcher, timeout: timeout) else { return } | ||
| self.enqueueStreamingFile(file, requestURL: requestURL, urlSchemeTask: urlSchemeTask, taskID: taskID) | ||
| } | ||
| watcher.setCancelHandler { | ||
| close(fd) | ||
| } | ||
| timeout.setEventHandler { [weak self, weak watcher, weak timeout] in | ||
| guard let self else { return } | ||
| guard self.finishReplacementWait(taskID, watcher: watcher, timeout: timeout) else { return } | ||
| self.failSchemeTask(taskID, urlSchemeTask: urlSchemeTask, errorCode: NSURLErrorTimedOut) | ||
| } | ||
| timeout.schedule(deadline: .now() + Self.replacementWaitTimeout) | ||
|
|
||
| guard storeReplacementWaitSources(taskID, watcher: watcher, timeout: timeout) else { | ||
| watcher.cancel() | ||
| watcher.resume() | ||
| timeout.cancel() | ||
| timeout.resume() | ||
| return | ||
| } | ||
| watcher.resume() | ||
| timeout.resume() | ||
|
|
||
| guard !diffViewerReplacementIsPending(file.fileURL) else { return } | ||
| guard finishReplacementWait(taskID, watcher: watcher, timeout: timeout) else { return } | ||
| enqueueStreamingFile(file, requestURL: requestURL, urlSchemeTask: urlSchemeTask, taskID: taskID) | ||
| } | ||
|
|
||
| private func diffViewerReplacementIsPending(_ fileURL: URL) -> Bool { |
Contributor
There was a problem hiding this comment.
webView(_:start:) is called on the main thread, so startWaitingForReplacement runs there too. It calls diffViewerReplacementIsPending (which does a full Data(contentsOf:) read) once before setting up the watcher, then again after watcher.resume() to close the TOCTOU window. It also calls open(file.fileURL.path, O_EVTONLY) between them — another blocking syscall on the main thread. For the small HTML files involved this is typically sub-millisecond, but calling blocking I/O APIs directly from a WKURLSchemeHandler delegate is prohibited by the cmux-swift-blocking-runtime rule. The pattern already used by enqueueStreamingFile — dispatching to streamQueue.async before doing any I/O — should be applied here too: dispatch to streamQueue first, then do the pending check, open, and watcher setup from that queue.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
lawrencecchen
force-pushed
the
task-diff-viewer-404-instant-pane
branch
from
febec6a to
c7e3ecf
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux_open.swift`:
- Around line 3140-3155: The error formatting currently falls back to
error.localizedDescription which loses detail for NSError/CocoaError; update the
fallback in diffViewerErrorMessage to use String(describing: error) (or ensure
callers pass String(describing: error)) so the full error is preserved, and
update the related call sites around writeDiffViewerStatusHTML /
diffViewerErrorMessage in CLI/cmux_open.swift (including the similar branch at
lines ~3164-3178) to pass or display the String(describing: error) output rather
than relying on localizedDescription.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2505-2507: The app-scheme wait path currently hardcodes
replacementWaitTimeout = 120 and reads the whole file on each recheck which
causes inconsistent timeouts vs the HTTP side and unnecessary I/O; update
BrowserPanel to read the wait timeout from the same environment/config key used
by CLI (CMUX_DIFF_VIEWER_WAIT_TIMEOUT_SECONDS) instead of hardcoding 120
(replace usage of replacementWaitTimeout), and change the pending-marker check
(pendingReplacementMarker and replacementWaitPrefix code paths) to only read and
inspect the first 8192 bytes of the file when detecting the pending marker to
match the HTTP contract and avoid full-file reads on each filesystem event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b817320e-51f3-4820-a970-ca38969940b8
📒 Files selected for processing (3)
CLI/cmux_open.swiftSources/Panels/BrowserPanel.swiftcmuxTests/CMUXOpenCommandTests.swift
Comment on lines
+3140
to
3155
| try? writeDiffViewerStatusHTML( | ||
| to: openingFileURL, | ||
| title: title, | ||
| targetURL: completed.url | ||
| sourceLabel: sourceLabel, | ||
| message: diffViewerErrorMessage(error), | ||
| isError: true, | ||
| pollForReplacement: false, | ||
| layout: layout, | ||
| appearance: appearance, | ||
| sourceOptions: [], | ||
| repoOptions: [], | ||
| baseOptions: [], | ||
| repoRoot: context.repoRoot, | ||
| branchBaseRef: selectedSource == .branch ? context.branchBaseRef : nil | ||
| ) | ||
| throw error |
There was a problem hiding this comment.
Use String(describing:) before expanding this error path.
These new failure branches route more pending-pane errors through diffViewerErrorMessage(error), but that helper still falls back to error.localizedDescription. That will collapse many NSError/CocoaError failures into generic text in the new UI. Please switch the helper fallback to String(describing: error) before using it here.
Based on learnings, use String(describing: error) instead of error.localizedDescription when formatting errors in the cmux Swift CLI (CLI/**/*.swift) so the real cause is preserved.
Also applies to: 3164-3178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/cmux_open.swift` around lines 3140 - 3155, The error formatting currently
falls back to error.localizedDescription which loses detail for
NSError/CocoaError; update the fallback in diffViewerErrorMessage to use
String(describing: error) (or ensure callers pass String(describing: error)) so
the full error is preserved, and update the related call sites around
writeDiffViewerStatusHTML / diffViewerErrorMessage in CLI/cmux_open.swift
(including the similar branch at lines ~3164-3178) to pass or display the
String(describing: error) output rather than relying on localizedDescription.
Comment on lines
+2505
to
+2507
| private static let replacementWaitPrefix = "/__cmux_diff_viewer_wait/" | ||
| private static let pendingReplacementMarker = Data("data-cmux-diff-pending=\"true\"".utf8) | ||
| private static let replacementWaitTimeout: TimeInterval = 120 |
There was a problem hiding this comment.
Keep the app-scheme wait path aligned with the HTTP wait contract.
CLI/cmux_open.swift already makes this timeout configurable via CMUX_DIFF_VIEWER_WAIT_TIMEOUT_SECONDS and only checks the first 8192 bytes for the pending marker. Hardcoding 120 here and loading the whole file on each recheck means the same diff-viewer flow can time out differently by transport and does unnecessary full-file I/O on every filesystem event.
Suggested fix
- private static let replacementWaitTimeout: TimeInterval = 120
+ private static func replacementWaitTimeout() -> TimeInterval {
+ let defaultTimeout: TimeInterval = 120
+ let key = "CMUX_DIFF_VIEWER_WAIT_TIMEOUT_SECONDS"
+ guard let raw = ProcessInfo.processInfo.environment[key]?.trimmingCharacters(in: .whitespacesAndNewlines),
+ let value = TimeInterval(raw),
+ value.isFinite else {
+ return defaultTimeout
+ }
+ return min(max(value, 0.05), 600)
+ }
...
- timeout.schedule(deadline: .now() + Self.replacementWaitTimeout)
+ timeout.schedule(deadline: .now() + Self.replacementWaitTimeout())
...
private func diffViewerReplacementIsPending(_ fileURL: URL) -> Bool {
- guard let data = try? Data(contentsOf: fileURL) else {
+ guard let handle = try? FileHandle(forReadingFrom: fileURL) else {
return true
}
- return data.range(of: Self.pendingReplacementMarker) != nil
+ defer { try? handle.close() }
+ guard let data = try? handle.read(upToCount: 8192),
+ !data.isEmpty else {
+ return true
+ }
+ return data.range(of: Self.pendingReplacementMarker) != nil
}Also applies to: 2834-2834, 2851-2855
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Panels/BrowserPanel.swift` around lines 2505 - 2507, The app-scheme
wait path currently hardcodes replacementWaitTimeout = 120 and reads the whole
file on each recheck which causes inconsistent timeouts vs the HTTP side and
unnecessary I/O; update BrowserPanel to read the wait timeout from the same
environment/config key used by CLI (CMUX_DIFF_VIEWER_WAIT_TIMEOUT_SECONDS)
instead of hardcoding 120 (replace usage of replacementWaitTimeout), and change
the pending-marker check (pendingReplacementMarker and replacementWaitPrefix
code paths) to only read and inspect the first 8192 bytes of the file when
detecting the pending marker to match the HTTP contract and avoid full-file
reads on each filesystem event.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Notes
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches WKWebView custom scheme handling, long-lived wait requests, and CSP framing for localhost; behavior is covered by tests but spans CLI and browser security boundaries.
Overview
Git diff panes now open on a minimal
cmux-diff-viewer://pending page (toolbar + spinner) before git, the local HTTP asset server, or full diff HTML run.DiffViewerURLMappergains an app-scheme transport alongside HTTP; deferred completion rebuilds URLs against127.0.0.1and swaps the opening page to an embedded localhost viewer instead of an immediate meta-redirect.CmuxDiffViewerURLSchemeHandlerserves/__cmux_diff_viewer_wait/..., blocking on disk until the pending marker clears (file watcher + 120s timeout), and relaxes CSPframe-srcfor localhost. CLI--cwduses a resolved path without forcinggit rev-parseup front. Tests assert open uses the app scheme, two registered files, and no git beforebrowser.open_split.Reviewed by Cursor Bugbot for commit c7e3ecf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Open the diff viewer pane instantly with a lightweight
cmux-diff-viewer://pending page, then embed the full HTTP viewer once git and assets are ready. This removes the initial blank delay and keeps the UI responsive even when git is slow.cmux-diff-viewer://with a minimal pending HTML page; no git, assets, or HTTP server work before the pane appears./__cmux_diff_viewer_wait/...that streams the replacement after the pending marker clears; 120s timeout and non-blocking.frame-srcandchild-srcforhttp://127.0.0.1:*.--cwdfor repo root to avoid upfrontgit, and update tests for app-scheme open, two registered files, and “no git before open.”Written for commit c7e3ecf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability