Open diff viewer before rendering large git diffs#5016
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughDefers selected-page diff HTML when appropriate, emits a pending/status viewer (spinner + reduced-motion support), records per-page fallback candidates, completes deferred pages later by retrying alternate sources when allowed, tightens manifest caching, and adds integration tests and localization. ChangesDeferred Diff Viewer with Source Fallback
Sequence Diagram(s)sequenceDiagram
participant Browser
participant CMUXServer
participant GitProcess
participant ManifestCache
Browser->>CMUXServer: GET /diff viewer
CMUXServer->>Browser: 200 pending HTML (status/polling, data-cmux-diff-pending)
CMUXServer->>GitProcess: spawn git diff (async)
GitProcess-->>CMUXServer: diff output (when ready) or EmptyDiffSourceError
CMUXServer->>ManifestCache: update token manifest / write final HTML & patch
Browser->>CMUXServer: poll /__cmux_diff_viewer_wait (if pendingReplacement)
CMUXServer->>Browser: signal replacement → client swaps to final HTML
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 4 inconclusive)
✅ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR makes the diff viewer open immediately as a pending HTML page with a full toolbar and spinner before large git diffs finish computing, then replaces the page via an in-process long-poll redirect when the real diff is ready.
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (main thread)
participant HTTP as HTTP Server (bg thread)
participant Browser as Browser
CLI->>CLI: "writeOpeningGitDiffViewerHTMLSet()<br/>writes pending HTML → openingFileURL"
CLI->>CLI: "writeDiffViewerHTTPManifest()<br/>(manifest: openingFileURL only)"
CLI->>Browser: browser.open_split(openingURL)
Browser->>HTTP: GET openingURL → pending spinner HTML
Browser->>HTTP: GET /__cmux_diff_viewer_wait/[opening-path]
HTTP-->>HTTP: "waitForDiffViewerHTTPReplacement()<br/>watches openingFileURL via kqueue/fsevent"
CLI->>CLI: completeDeferredDiffViewer() → completeDeferred()
CLI->>CLI: "writeCompleteGitDiffViewerHTMLSet()<br/>runs git diff, writes selectedFileURL<br/>updates manifest (all files)"
CLI->>CLI: "completeDeferredDiffViewerSelectedSource()<br/>writes actual diff to selectedFileURL"
CLI->>CLI: "writeDiffViewerRedirectHTML()<br/>replaces openingFileURL with redirect → selectedURL"
Note over HTTP: fsevent fires on openingFileURL
HTTP->>HTTP: file no longer pending → serve redirect HTML
HTTP-->>Browser: 200 redirect HTML (data-cmux-diff-redirect)
Browser->>Browser: "replaceDocumentWith(redirectHTML)<br/>follows redirect to selectedURL"
Browser->>HTTP: GET selectedURL
HTTP->>HTTP: cache miss → refresh manifest
HTTP-->>Browser: 200 final diff HTML
CLI->>CLI: "completeDeferredDiffViewerSources()<br/>fills remaining source/repo/base pages"
CLI->>CLI: "print("OK surface=... pane=... path=...")"
Reviews (7): Last reviewed commit: "Scope diff viewer bare shortcut schema" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
3191
to
3192
| let message = diffViewerErrorMessage(error) | ||
| try? writePendingDiffViewerHTML(to: page.url, title: page.source.title, message: message, pollForReplacement: false) |
Contributor
There was a problem hiding this comment.
page.source.title (the default title for the source), but the DiffViewerDeferredSourcePage carries titleOverride for exactly this purpose. For the newly-deferred selected-source page, titleOverride is the user-supplied --title argument. Ignoring it means that if diff computation fails the error page displays the generic source name (e.g. "Unstaged changes") instead of the title the user specified.
Suggested change
| let message = diffViewerErrorMessage(error) | |
| try? writePendingDiffViewerHTML(to: page.url, title: page.source.title, message: message, pollForReplacement: false) | |
| let message = diffViewerErrorMessage(error) | |
| try? writePendingDiffViewerHTML(to: page.url, title: page.titleOverride ?? page.source.title, message: message, pollForReplacement: false) |
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!
Comment on lines
+3224
to
+3228
| return | ||
| } catch { | ||
| continue | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The bare catch { continue } swallows any error thrown by writeDeferredDiffViewerSource during fallback iteration — including non-EmptyDiffSourceError failures such as disk-full, git process crashes, or HTML write failures. If every fallback fails for a non-empty-diff reason, the loop falls through and re-throws the original EmptyDiffSourceError from the primary source. The displayed error message will say the selected source had no diff, not that a write or git command failed on the fallback that was actually attempted.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux_open.swift (1)
2885-2921:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlip the deferral predicate so the fallback path is reachable.
Line 2885 currently makes
.lastTurnthe only source that stays synchronous. In the only branch that loadsselectedInput, Line 2905 therefore always sees.lastTurn, so the fallback code below is dead and--last-turnstill blocks beforebrowser.open_split.Suggested fix
- let shouldDeferSelectedSource = requestedSource != .lastTurn + let shouldDeferSelectedSource = requestedSource == .lastTurn🤖 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 2885 - 2921, The deferral predicate is inverted so .lastTurn remains synchronous and the fallback path after nonEmptyGitDiffInput is never reached; change the initialization of shouldDeferSelectedSource from "requestedSource != .lastTurn" to "requestedSource == .lastTurn" so that .lastTurn is deferred. This will allow the existing branch that attempts to eagerly load selectedInput (the code around sourceContext(for:), nonEmptyGitDiffInput(source:context:), selectedInput and the fallback selection loop over DiffSource.allCases) to operate as intended and make the fallback reachable when the initially requested source is empty.
🤖 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 3211-3229: The loop currently swallows any error from
writeDeferredDiffViewerSource by using a blanket `catch { continue }`; change it
to only continue for EmptyDiffSourceError and rethrow other errors so real
failures aren't hidden — e.g., inside the loop replace the generic `catch` with
`catch let err as EmptyDiffSourceError { continue } catch { throw err }` while
keeping the surrounding logic that iterates DiffSource.allCases and checks
page.sourceFallbacks when page.allowsSourceFallback.
In `@cmuxTests/CMUXOpenCommandTests.swift`:
- Around line 1460-1463: The test currently only asserts the browser open
happened before the fake git diff completed; change it to assert the open
happened before git diff started by capturing whether diffStartedURL exists
inside the socket handler and recording that state when the open request
arrives; specifically, in the socket handler that sets diffStartedURL, add a
boolean flag (or record diffStartedURL presence) accessible to the test, and in
the open request callback that sets openedURLBox/openedHTMLURLBox/pendingHTMLBox
(and fulfills openHandled), assert that the captured flag shows diff had NOT
started (diffStartedURL was nil) at that moment so the browser.open request
occurred before git diff began. Ensure this pattern is applied similarly to the
other two locations referenced around lines 1478-1483 and 1512-1515.
---
Outside diff comments:
In `@CLI/cmux_open.swift`:
- Around line 2885-2921: The deferral predicate is inverted so .lastTurn remains
synchronous and the fallback path after nonEmptyGitDiffInput is never reached;
change the initialization of shouldDeferSelectedSource from "requestedSource !=
.lastTurn" to "requestedSource == .lastTurn" so that .lastTurn is deferred. This
will allow the existing branch that attempts to eagerly load selectedInput (the
code around sourceContext(for:), nonEmptyGitDiffInput(source:context:),
selectedInput and the fallback selection loop over DiffSource.allCases) to
operate as intended and make the fallback reachable when the initially requested
source is empty.
🪄 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: d77b34ad-b745-46c8-a36f-ed5ed4afa50c
📒 Files selected for processing (2)
CLI/cmux_open.swiftcmuxTests/CMUXOpenCommandTests.swift
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux_open.swift (1)
792-800:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEmit JSON after deferred completion has finalized the selected page.
For deferred sources,
viewer.inputandviewer.titleare placeholders until completion runs. If completion falls back from the requested source to another non-empty source, the JSON here can report the wrongsource/titleeven though the rendered viewer shows different content.Also applies to: 3352-3364
🤖 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 792 - 800, The JSON is emitted before deferred completion runs, so fields like viewer.input.sourceLabel and viewer.title can be stale; call completeDeferredDiffViewer(viewer) and wait for it to finalize the selected page before building the response payload and calling print(jsonString(formatIDs(..., mode: idFormat))); in other words, move the completeDeferredDiffViewer(viewer) invocation to precede creating/setting response["source"] and response["title"] (and do the same change in the analogous block around the other occurrence at lines ~3352-3364).
🤖 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 2944-2959: The code builds a localized message by concatenating
CMUXDiffViewerLocalization.string("diffViewer.loadingDiff", ...) with
selectedSource.menuLabel which breaks grammar in other locales; update the
message construction to use a single localized interpolated string (e.g., use
String(localized: or CMUXDiffViewerLocalization.string with a format
placeholder) that accepts the dynamic label) and pass that localized, fully
formatted sentence into writeDiffViewerStatusHTML (keep same parameters like
selectedFileURL, title, sourceLabel, layout, appearance, branchBaseRef). Apply
the same change to the other similar concatenations in the file (the blocks
referenced near the other occurrences) so all dynamic labels are inserted via a
single localization key with interpolation rather than by string concatenation.
In `@cmuxTests/CMUXOpenCommandTests.swift`:
- Around line 1522-1525: The assertion that checks absence of a main element is
too narrow — update the check on pendingHTMLBox (the
pendingHTMLBox.get()?.contains(...) call) to look for "<main" (so it catches
"<main>" and "<main ...>") or, if you intend to assert a specific container is
excluded, assert that specific container id/class instead; modify the
XCTAssertFalse that currently uses contains("<main>") to use contains("<main")
(or replace with an assertion targeting the full-layout container you want
excluded).
---
Outside diff comments:
In `@CLI/cmux_open.swift`:
- Around line 792-800: The JSON is emitted before deferred completion runs, so
fields like viewer.input.sourceLabel and viewer.title can be stale; call
completeDeferredDiffViewer(viewer) and wait for it to finalize the selected page
before building the response payload and calling print(jsonString(formatIDs(...,
mode: idFormat))); in other words, move the completeDeferredDiffViewer(viewer)
invocation to precede creating/setting response["source"] and response["title"]
(and do the same change in the analogous block around the other occurrence at
lines ~3352-3364).
🪄 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: 4b1b0679-ce7f-4d8c-9c68-6bbd445e4140
📒 Files selected for processing (2)
CLI/cmux_open.swiftcmuxTests/CMUXOpenCommandTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux_open.swift (1)
3458-3475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRethrow the fallback write failure, not the outer empty-source error.
This looks reintroduced. Line 3474 still throws the outer
EmptyDiffSourceError, so a real failure fromwriteDeferredDiffViewerSource(...)gets masked and the user sees the wrong cause.Suggested fix
- } catch { - throw error + } catch let fallbackError { + throw fallbackError }🤖 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 3458 - 3475, The catch around writeDeferredDiffViewerSource is rethrowing the outer EmptyDiffSourceError variable (named error) instead of the actual failure from the fallback attempt, masking the real cause; update the inner do-catch so the general catch captures the fallback error (e.g., catch let fallbackError) and rethrows that fallbackError (or just rethrow the caught error) instead of throwing the outer "error" value from the surrounding catch; references: EmptyDiffSourceError, writeDeferredDiffViewerSource(...), page.sourceFallbacks, DiffSource.allCases.
🤖 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 803-806: The success message is printed before deferred rendering
completes, which can misreport success if completeDeferredDiffViewer(viewer)
throws; call completeDeferredDiffViewer(viewer) and wait for it to succeed (or
wrap it in a do/catch) before printing the OK line, i.e. move the print("OK
surface=... pane=... path=...") to after a successful return from
completeDeferredDiffViewer(viewer) (or only print in the do block after
awaiting/try-ing completeDeferredDiffViewer), referencing formatHandle(payload,
kind: "surface"/"pane", idFormat: idFormat) and the viewer variable.
---
Duplicate comments:
In `@CLI/cmux_open.swift`:
- Around line 3458-3475: The catch around writeDeferredDiffViewerSource is
rethrowing the outer EmptyDiffSourceError variable (named error) instead of the
actual failure from the fallback attempt, masking the real cause; update the
inner do-catch so the general catch captures the fallback error (e.g., catch let
fallbackError) and rethrows that fallbackError (or just rethrow the caught
error) instead of throwing the outer "error" value from the surrounding catch;
references: EmptyDiffSourceError, writeDeferredDiffViewerSource(...),
page.sourceFallbacks, DiffSource.allCases.
🪄 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: aad673cc-db62-4cb1-bbea-d027f3c2b098
📒 Files selected for processing (3)
CLI/cmux_open.swiftResources/Localizable.xcstringscmuxTests/CMUXOpenCommandTests.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux_open.swift (1)
3553-3555:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve real fallback failures instead of continuing on all errors.
Line 3553 currently swallows every fallback failure, which can hide real write/render errors and incorrectly fall through as an empty-source outcome.
Suggested fix
- } catch { - continue + } catch is EmptyDiffSourceError { + continue + } catch let fallbackError { + throw fallbackError }🤖 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 3553 - 3555, The current catch block after the fallback write/render attempt unconditionally swallows all errors with "catch { continue }"; change it to capture the error (e.g., "catch let error") and only continue for explicit, expected recoverable cases (such as the empty-source/fallback-not-available condition), otherwise propagate or surface the error (rethrow, return failure, or log and break) so real write/render failures are not hidden; locate the catch directly following the fallback write/render call in CLI/cmux_open.swift and replace the unconditional continue with conditional handling that preserves and forwards unexpected errors.
🤖 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 `@cmuxTests/CMUXOpenCommandTests.swift`:
- Around line 635-639: The test reads the file returned by
diffViewerHTMLFileURL(for:from:) but that may be an intermediate redirect page;
instead detect if the returned HTML is a redirect (e.g., contains a meta-refresh
or a redirect anchor), extract the redirect target href, resolve the real viewer
URL (call diffViewerHTMLFileURL(for: redirectTarget, from:
stagedFallback.params) or reuse diffViewerOptionURL to get the actual viewer
path) and then read the final HTML before asserting its contents; update the
assertions to run against the resolved final unstagedHTML rather than the
redirect shell.
---
Duplicate comments:
In `@CLI/cmux_open.swift`:
- Around line 3553-3555: The current catch block after the fallback write/render
attempt unconditionally swallows all errors with "catch { continue }"; change it
to capture the error (e.g., "catch let error") and only continue for explicit,
expected recoverable cases (such as the empty-source/fallback-not-available
condition), otherwise propagate or surface the error (rethrow, return failure,
or log and break) so real write/render failures are not hidden; locate the catch
directly following the fallback write/render call in CLI/cmux_open.swift and
replace the unconditional continue with conditional handling that preserves and
forwards unexpected errors.
🪄 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: 2c5269f7-5f6c-4165-9e5b-4d3f441ea2c4
📒 Files selected for processing (2)
CLI/cmux_open.swiftcmuxTests/CMUXOpenCommandTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1baaba8. Configure here.
lawrencecchen
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Stale CodeRabbit review on an older commit. The actionable findings were addressed in later commits, review threads are resolved or outdated, and the current CodeRabbit check is passing/skipped on head f042011.
This was referenced May 31, 2026
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
Note
Medium Risk
Large change to CLI diff HTML lifecycle and blocking completion semantics; behavior is covered by new integration tests but touches browser open timing and deferred error/fallback paths.
Overview
Git diff opens in the browser before heavy
git diffwork finishes: the CLI serves a full pending viewer page (toolbar, spinner, poll-for-replace), then redirects to the real diff when generation completes..lastTurnstays synchronous; other sources defer the selected page and background-fill alternate source/repo/base pages.Deferred completion is centralized (
completeDeferred, per-page fallbacks, manifest refresh on cache miss) so CLI/JSON output andbrowser.open_splitreport the final path, URL, title, and source after the selected diff is ready.Adds diff-viewer-only shortcuts (
j/k,gg,/, etc.) in the embedded HTML,cmux.json(bare first strokes), settings UI, schema, docs, and strings; marks them as browser-content so they do not steal global chord routing.New tests cover opening while git is blocked, redirect resolution, shortcut payload from config, and preserving the unstaged-empty error when fallbacks fail.
Reviewed by Cursor Bugbot for commit f042011. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Open the diff viewer instantly with a full UI and pending state while large git diffs render in the background, then auto-redirect to the final page when ready. The selected source completes first,
.lastTurnremains synchronous, and CLI/JSON now report the finalized viewer path/URL/title/source.New Features
cmux.json/schema, recorder enforces modifiers elsewhere; docs and localized “Loading diff: %@” added.Bug Fixes
Written for commit f042011. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Bug Fixes
Tests