Repository navigation
cmux open: defer smart branch-base resolution past diff viewer first paint - #7605
Closed
lawrencecchen wants to merge 6 commits into
Closed
lawrencecchen wants to merge 6 commits into
lawrencecchen wants to merge 6 commits into
Conversation
Regression test for the slow initial "Loading diff: Unstaged" state: opening a git diff viewer with a non-branch selected source eagerly resolved smart branch bases (gh pr view, up to 4s timeout) for the selected repo and up to 12 sibling repo candidates before the selected diff was computed. The fake gh on PATH logs every invocation; the test asserts the log is still empty at the moment the opening page gains its redirect.
The opening "Loading diff: <source>" page was only replaced after writeCompleteGitDiffViewerHTMLSet resolved a smart branch base (each a gh pr view network call, ~0.8s online / 4s timeout offline) for the selected repo and every sibling repo candidate (up to 12), serially, before the selected diff was even computed. In a checkout with many sibling worktrees that blocked first paint for 10-45s. Branch pages now carry resolveBranchBaseOnComplete and resolve their smart base (cmuxBase -> PR base -> upstream -> origin/HEAD, or an explicit --base as manual/high) at deferred-page completion time, after the selected page is done and the opening page has redirected. Only a selected .branch page still resolves its own repo's base up front, since that base determines the rendered diff. Resolution is cached per repo for the completion pass; picker payloads stay gated on the persisted branch session, and a completed branch page still renders against the exact ref its picker advertises.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe diff viewer now defers branch-base resolution and placeholder completion work until after first paint, while refreshing manifests and preparing completed pages from deferred state. Tests were updated for resolved asset URLs and a new regression covers the browser-open ordering. ChangesDeferred diff viewer completion
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR speeds up diff viewer first paint by deferring branch-base work. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "tests: split CMUXOpenCommandTests shared..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _ page: DiffViewerDeferredSourcePage, | ||
| sourceSet: DiffViewerDeferredSourceSet | ||
| ) throws -> DiffViewerDeferredCompletion { | ||
| let preparedPage = try preparedDeferredDiffViewerSourcePage(page, sourceSet: sourceSet) |
Contributor
There was a problem hiding this comment.
When a deferred branch page resolves its base on completion and that repo has no usable PR base, upstream, or origin fallback, this try runs before the existing error/empty-state handling in the completion body. The loading page can be left polling forever instead of being replaced with the same inline error page that later diff failures use.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1815-1997: The test in
testDiffCommandDoesNotResolveSmartBranchBaseBeforeUnstagedFirstPaint only checks
that ghLogURL stays empty before the first paint, but it never verifies that
deferred smart-branch-base resolution eventually runs. Update the test to add a
post-exit assertion after process.waitUntilExit() that confirms the gh
invocation happened (for example by checking ghLogURL is non-empty or that the
expected branch-resolution output was produced), using the existing helpers and
fixtures like ghLogURL, openedHTMLURLBox, and the diff command flow.
🪄 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: 78b1ce77-08c2-4b90-b17d-29f96e46d184
📒 Files selected for processing (2)
CLI/cmux_open.swiftcmuxTests/CMUXOpenCommandTests.swift
Comment on lines
+1815
to
+1997
| func testDiffCommandDoesNotResolveSmartBranchBaseBeforeUnstagedFirstPaint() throws { | ||
| let cliPath = try bundledCLIPath() | ||
| let rootURL = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent(UUID().uuidString, isDirectory: true) | ||
| let repoURL = rootURL.appendingPathComponent("repo", isDirectory: true) | ||
| let fakeBinURL = rootURL.appendingPathComponent("bin", isDirectory: true) | ||
| let fakeGitURL = fakeBinURL.appendingPathComponent("git", isDirectory: false) | ||
| let fakeGHURL = fakeBinURL.appendingPathComponent("gh", isDirectory: false) | ||
| let diffStartedURL = rootURL.appendingPathComponent("diff-started", isDirectory: false) | ||
| let releaseDiffURL = rootURL.appendingPathComponent("release-diff", isDirectory: false) | ||
| let alternateStartedURL = rootURL.appendingPathComponent("alternate-started", isDirectory: false) | ||
| let releaseAlternateURL = rootURL.appendingPathComponent("release-alternate", isDirectory: false) | ||
| let ghLogURL = rootURL.appendingPathComponent("gh.log", isDirectory: false) | ||
| try FileManager.default.createDirectory(at: repoURL.appendingPathComponent(".git", isDirectory: true), withIntermediateDirectories: true) | ||
| try FileManager.default.createDirectory(at: fakeBinURL, withIntermediateDirectories: true) | ||
| defer { try? FileManager.default.removeItem(at: rootURL) } | ||
|
|
||
| try """ | ||
| #!/bin/sh | ||
| if [ "${1:-}" = "-C" ]; then | ||
| shift 2 | ||
| fi | ||
| if [ "${1:-}" = "rev-parse" ] && [ "${2:-}" = "--show-toplevel" ]; then | ||
| printf '%s\\n' "$CMUX_FAKE_GIT_REPO_ROOT" | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "rev-parse" ] && [ "${2:-}" = "--abbrev-ref" ]; then | ||
| printf 'feature/dvfast\\n' | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "rev-parse" ] && [ "${2:-}" = "--verify" ]; then | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "symbolic-ref" ]; then | ||
| printf 'origin/main\\n' | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "config" ]; then | ||
| exit 1 | ||
| fi | ||
| if [ "${1:-}" = "merge-base" ]; then | ||
| printf 'mergebase\\n' | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "diff" ]; then | ||
| for arg in "$@"; do | ||
| if [ "$arg" = "--cached" ]; then | ||
| : > "$CMUX_FAKE_GIT_ALTERNATE_STARTED" | ||
| while [ ! -f "$CMUX_FAKE_GIT_RELEASE_ALTERNATE" ]; do | ||
| sleep 0.05 | ||
| done | ||
| cat <<'PATCH' | ||
| diff --git a/staged.txt b/staged.txt | ||
| index 1111111..2222222 100644 | ||
| --- a/staged.txt | ||
| +++ b/staged.txt | ||
| @@ -1 +1 @@ | ||
| -old staged | ||
| +new staged | ||
| PATCH | ||
| exit 0 | ||
| fi | ||
| done | ||
| : > "$CMUX_FAKE_GIT_STARTED" | ||
| while [ ! -f "$CMUX_FAKE_GIT_RELEASE" ]; do | ||
| sleep 0.05 | ||
| done | ||
| cat <<'PATCH' | ||
| diff --git a/worktree.txt b/worktree.txt | ||
| index 1111111..2222222 100644 | ||
| --- a/worktree.txt | ||
| +++ b/worktree.txt | ||
| @@ -1 +1 @@ | ||
| -old line | ||
| +new line | ||
| PATCH | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "for-each-ref" ]; then | ||
| exit 0 | ||
| fi | ||
| if [ "${1:-}" = "rev-list" ]; then | ||
| printf '0\\t1\\n' | ||
| exit 0 | ||
| fi | ||
| exit 1 | ||
| """.write(to: fakeGitURL, atomically: true, encoding: .utf8) | ||
| try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: fakeGitURL.path) | ||
|
|
||
| try """ | ||
| #!/bin/sh | ||
| printf '%s\\n' "$*" >> "$CMUX_FAKE_GH_LOG" | ||
| printf 'main\\n' | ||
| exit 0 | ||
| """.write(to: fakeGHURL, atomically: true, encoding: .utf8) | ||
| try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: fakeGHURL.path) | ||
|
|
||
| let socketPath = makeSocketPath("diff-gh") | ||
| let listenerFD = try bindUnixSocket(at: socketPath) | ||
| let state = MockSocketServerState() | ||
| let openedURLBox = AsyncValueBox<String?>(nil) | ||
| let openedHTMLURLBox = AsyncValueBox<URL?>(nil) | ||
| let openHandled = expectation(description: "browser opened before smart branch base lookup") | ||
| defer { | ||
| Darwin.close(listenerFD) | ||
| unlink(socketPath) | ||
| } | ||
|
|
||
| let serverClosed = startMockServer(listenerFD: listenerFD, state: state) { line in | ||
| guard let payload = Self.v2Payload(from: line), | ||
| let id = payload["id"] as? String, | ||
| let method = payload["method"] as? String, | ||
| method == "browser.open_split", | ||
| let params = payload["params"] as? [String: Any], | ||
| let rawURL = params["url"] as? String else { | ||
| return Self.v2Response(id: "unknown", ok: false, error: ["code": "unexpected"]) | ||
| } | ||
| openedURLBox.set(rawURL) | ||
| if let htmlURL = Self.diffViewerHTMLFileURLFromHTTPManifest(for: rawURL) { | ||
| openedHTMLURLBox.set(htmlURL) | ||
| } | ||
| openHandled.fulfill() | ||
| return Self.v2Response( | ||
| id: id, | ||
| ok: true, | ||
| result: ["surface_id": "surface-id", "pane_id": "pane-id", "url": rawURL] | ||
| ) | ||
| } | ||
|
|
||
| let process = Process() | ||
| let stdoutPipe = Pipe() | ||
| let stderrPipe = Pipe() | ||
| var environment = ProcessInfo.processInfo.environment | ||
| environment["PATH"] = "\(fakeBinURL.path):\(environment["PATH"] ?? "")" | ||
| environment["CMUX_SOCKET_PATH"] = socketPath | ||
| environment["CMUX_CLI_SENTRY_DISABLED"] = "1" | ||
| environment["CMUX_CLAUDE_HOOK_SENTRY_DISABLED"] = "1" | ||
| environment["CMUX_FAKE_GIT_REPO_ROOT"] = repoURL.path | ||
| environment["CMUX_FAKE_GIT_STARTED"] = diffStartedURL.path | ||
| environment["CMUX_FAKE_GIT_RELEASE"] = releaseDiffURL.path | ||
| environment["CMUX_FAKE_GIT_ALTERNATE_STARTED"] = alternateStartedURL.path | ||
| environment["CMUX_FAKE_GIT_RELEASE_ALTERNATE"] = releaseAlternateURL.path | ||
| environment["CMUX_FAKE_GH_LOG"] = ghLogURL.path | ||
| process.executableURL = URL(fileURLWithPath: cliPath) | ||
| process.arguments = ["diff", "--unstaged", "--cwd", repoURL.path, "--title", "Fast first paint", "--no-focus"] | ||
| process.environment = environment | ||
| process.currentDirectoryURL = repoURL | ||
| process.standardInput = FileHandle.nullDevice | ||
| process.standardOutput = stdoutPipe | ||
| process.standardError = stderrPipe | ||
| try process.run() | ||
| defer { terminateProcess(process) } | ||
|
|
||
| wait(for: [openHandled], timeout: 5) | ||
| XCTAssertNotNil(openedURLBox.get()) | ||
| XCTAssertFalse(FileManager.default.fileExists(atPath: ghLogURL.path)) | ||
| FileManager.default.createFile(atPath: releaseDiffURL.path, contents: Data()) | ||
|
|
||
| let openingHTMLURL = try XCTUnwrap(openedHTMLURLBox.get()) | ||
| XCTAssertTrue(waitUntil(timeout: 5) { | ||
| let html = (try? String(contentsOf: openingHTMLURL, encoding: .utf8)) ?? "" | ||
| return html.contains("data-cmux-diff-redirect=") | ||
| && FileManager.default.fileExists(atPath: alternateStartedURL.path) | ||
| }) | ||
| let ghLogAtFirstPaint = (try? String(contentsOf: ghLogURL, encoding: .utf8)) ?? "" | ||
| XCTAssertEqual(ghLogAtFirstPaint, "") | ||
|
|
||
| FileManager.default.createFile(atPath: releaseAlternateURL.path, contents: Data()) | ||
| let finished = DispatchSemaphore(value: 0) | ||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| process.waitUntilExit() | ||
| finished.signal() | ||
| } | ||
| XCTAssertEqual(finished.wait(timeout: .now() + 5), .success) | ||
| wait(for: [serverClosed], timeout: 5) | ||
|
|
||
| let stdout = String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | ||
| let stderr = String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | ||
| XCTAssertEqual(process.terminationStatus, 0, stderr) | ||
| XCTAssertTrue(stdout.contains("OK surface=surface-id pane=pane-id"), stdout) | ||
| XCTAssertTrue(FileManager.default.fileExists(atPath: diffStartedURL.path)) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Test only proves absence, never proves the deferred resolution actually happens.
The test asserts gh.log is empty before/at the redirect (Lines 1970, 1980), which correctly guards the "no premature gh call" regression. But it never asserts that gh (or the branch-base resolution path) is eventually invoked once the process completes — the process is torn down without re-checking ghLogURL's contents. If deferred branch-base resolution silently regresses to a no-op (e.g., the completion pass is skipped entirely), this test would still pass, since it only checks an early-state absence.
Consider adding a final assertion after the process exits, e.g. confirming ghLogURL is non-empty (or the branch page picker data was eventually populated), to ensure the test also catches a "resolution never runs" regression, not just a "runs too early" regression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/CMUXOpenCommandTests.swift` around lines 1815 - 1997, The test in
testDiffCommandDoesNotResolveSmartBranchBaseBeforeUnstagedFirstPaint only checks
that ghLogURL stays empty before the first paint, but it never verifies that
deferred smart-branch-base resolution eventually runs. Update the test to add a
post-exit assertion after process.waitUntilExit() that confirms the gh
invocation happened (for example by checking ghLogURL is non-empty or that the
expected branch-resolution output was produced), using the existing helpers and
fixtures like ghLogURL, openedHTMLURLBox, and the diff command flow.
Round 1 removed the gh network calls from the pre-redirect path, but profiling the built app on a checkout with 1134 sibling worktrees still showed 7-24s to first paint: writeCompleteGitDiffViewerHTMLSet wrote ~60 placeholder pages (4 sources x 12 repos + base candidates) before completing the selected page, and every page write re-verified the whole webview asset bundle (a getattrlist sweep per page). Pre-redirect work is now only: repo/source/base option lists (git-only), the opening + selected placeholder, and a manifest listing just the files that exist. Non-selected placeholders are written after the opening page redirects, then completed as before. ensureDiffViewerAssets is memoized per (source bundle, target dir) behind os_unfair_lock. The manifest loader now skips a listed-but-missing local html page (404 for that entry) instead of failing the whole token, and deferred manifest refreshes go through the flock-merge helper so regenerate entries are not dropped. Deferred-page completion support moves to CMUXCLI+DiffViewerDeferredPages.swift to satisfy the swift file length budget; first-paint regression tests live in CMUXOpenDiffViewerFirstPaintTests.swift and now also assert the selected page serves HTTP 200 at redirect time while sibling pages are still pending. Two stale assertions in testDiffCommandGeneratesCodeViewAndOpensBrowserSplit (pre-existing rot: the fda9078 stdout contract and content-keyed asset dirs) are updated to pin current main behavior.
… split DiffViewerURLMapper became internal for the CMUXCLI+DiffViewerDeferredPages split, so its allowedFile/allowedRemotePatchFile results can no longer be a private type.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/CMUXOpenDiffViewerFirstPaintTests.swift`:
- Around line 180-184: The current wait in CMUXOpenDiffViewerFirstPaintTests
around `selectedPageURL` and `alternateStartedURL` is using file absence as the
handoff signal, which can race and cause false timeouts. Update the `waitUntil`
in this test to wait only for the redirect via
`Self.diffViewerRedirectURL(from:)`, then verify the alternate diff start with a
separate positive assertion or explicit release gate instead of
`!FileManager.default.fileExists(atPath:)`.
🪄 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: 557842b8-6528-4863-885f-06362dda9384
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/CMUXCLI+DiffViewerDeferredPages.swiftCLI/cmux_open.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CMUXOpenCommandTests.swiftcmuxTests/CMUXOpenDiffViewerFirstPaintTests.swift
Comment on lines
+180
to
+184
| XCTAssertTrue(waitUntil(timeout: 5) { | ||
| let html = (try? String(contentsOf: openingHTMLURL, encoding: .utf8)) ?? "" | ||
| selectedPageURL = Self.diffViewerRedirectURL(from: html) | ||
| return selectedPageURL != nil && !FileManager.default.fileExists(atPath: alternateStartedURL.path) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test and the sibling reference.
git ls-files cmuxTests/CMUXOpenDiffViewerFirstPaintTests.swift cmuxTests/CMUXOpenCommandTests.swift
echo '--- CMUXOpenDiffViewerFirstPaintTests.swift (around lines 150-230) ---'
sed -n '150,230p' cmuxTests/CMUXOpenDiffViewerFirstPaintTests.swift
echo '--- CMUXOpenCommandTests.swift (around the referenced lines) ---'
sed -n '1768,1805p' cmuxTests/CMUXOpenCommandTests.swiftRepository: manaflow-ai/cmux
Length of output: 6875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper logic that creates the staged/alternate signals.
echo '--- CMUXOpenDiffViewerFirstPaintTests.swift (helpers and setup) ---'
sed -n '1,170p' cmuxTests/CMUXOpenDiffViewerFirstPaintTests.swift
echo '--- Search for the fake git env vars and alternate-start handling ---'
rg -n "CMUX_FAKE_GIT_(ALTERNATE_STARTED|RELEASE_ALTERNATE|STARTED|RELEASE)|alternateStartedURL|releaseAlternateURL|releaseDiffURL" cmuxTests Sources CLI Packages -g '!**/DerivedData/**'Repository: manaflow-ai/cmux
Length of output: 11543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the diff-viewer redirect and alternate-diff sequencing in production code.
rg -n "data-cmux-diff-redirect|diffViewerRedirect|alternate-started|release-alternate|release-diff|browser.open_split|gh" Sources CLI Packages cmuxTests -g '!**/DerivedData/**'
echo '--- waitUntil helper definition ---'
rg -n "func waitUntil|waitUntil\\(" cmuxTests Sources CLI Packages -g '!**/DerivedData/**'Repository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the production diff flow that writes the redirect and triggers the alternate diff.
rg -n "data-cmux-diff-redirect|pendingReplacement|browser.open_split|diff.*cached|git diff --cached|gh " Sources/Panels Sources -g '!**/DerivedData/**' | head -n 200
echo '--- likely diff flow around redirect ---'
# Try to locate the smallest file/section with the redirect logic.
fd -a ".*Diff.*swift|.*Open.*swift|.*Browser.*swift" Sources cmuxTests | sed -n '1,80p'Repository: manaflow-ai/cmux
Length of output: 29283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the smallest relevant production/test symbols, then read the exact slices.
echo '--- BrowserPanel.swift outline: diff-viewer related symbols ---'
ast-grep outline Sources/Panels/BrowserPanel.swift --view expanded | sed -n '1,220p'
echo '--- TerminalController.swift outline: browser.open_split handlers ---'
ast-grep outline Sources/TerminalController.swift --view expanded | sed -n '2220,2295p'
echo '--- BrowserPanel.swift around diff redirect / pending page handling ---'
sed -n '2320,2385p' Sources/Panels/BrowserPanel.swift
echo '--- TerminalController.swift around browser.open_split ---'
sed -n '2240,2278p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 20596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact diff-viewer branch route and nearby flow.
sed -n '2110,2188p' Sources/Panels/BrowserPanel.swift
echo '---'
sed -n '2188,2260p' Sources/Panels/BrowserPanel.swift
echo '---'
sed -n '2390,2475p' Sources/Panels/BrowserPanel.swift
echo '--- v2BrowserOpenSplit implementation ---'
rg -n "func v2BrowserOpenSplit|v2BrowserOpenSplit\\(" Sources/TerminalController.swift Sources/Panels/BrowserPanel.swift
sed -n '2488,2535p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 12513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '6601,6735p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 6899
Use a stable handoff signal here. The !alternateStartedURL.exists check can miss the brief window between the redirect appearing and the alternate diff starting, turning this into a timeout even when the ordering is correct. Wait on the redirect itself, then assert the alternate path with a separate positive signal or release gate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/CMUXOpenDiffViewerFirstPaintTests.swift` around lines 180 - 184,
The current wait in CMUXOpenDiffViewerFirstPaintTests around `selectedPageURL`
and `alternateStartedURL` is using file absence as the handoff signal, which can
race and cause false timeouts. Update the `waitUntil` in this test to wait only
for the redirect via `Self.diffViewerRedirectURL(from:)`, then verify the
alternate diff start with a separate positive assertion or explicit release gate
instead of `!FileManager.default.fileExists(atPath:)`.
…TestSupport CMUXOpenCommandTests.swift is over the 900-line hard cap, so the +6 line growth from the resolved-asset-URL assertions failed workflow-guard-tests. Move the suite's shared process/socket/mock-server helpers into a new CMUXOpenCommandTestSupport.swift extension file (wired into cmuxTests) and ratchet the file's length budget down 2882 -> 2507.
Contributor
Author
|
Triaged Greptile's P2 (deferred resolution skips error page): not reproducible in the code path. Throws from Also pushed 3cd6357: workflow-guard-tests failed on the 900-line hard cap for cmuxTests/CMUXOpenCommandTests.swift (any growth fails; the budget bump to 2882 could not satisfy it). Split the suite's shared process/socket/mock-server helpers into cmuxTests/CMUXOpenCommandTestSupport.swift (wired into cmuxTests) and ratcheted the budget entry down 2882 -> 2507. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 3cd6357. Configure here.
| ) { | ||
| let message = diffViewerErrorMessage(error) | ||
| try? writeDiffViewerStatusHTML( | ||
| to: page.url, |
There was a problem hiding this comment.
Deferred errors drop branch picker
Medium Severity
When deferred branch page completion fails after preparedDeferredDiffViewerSourcePage has resolved the smart base, writeDeferredDiffViewerError still receives the original DiffViewerDeferredSourcePage from the loop. That struct never gets updated branchPickerBase or branchBaseRef, so error HTML omits the branch picker and resolved base even though resolution already ran.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3cd6357. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The diff viewer's initial "Loading diff: Unstaged" placeholder took 10-30s+ to be replaced. Before the selected diff was computed,
writeCompleteGitDiffViewerHTMLSet(1) resolved a smart branch base for the selected repo AND every sibling repo candidate (capped at 12), each shelling out togh pr view(~0.8s online, 4s timeout offline) serially, and (2) wrote ~60 placeholder pages (4 sources x 12 repos + base candidates), re-verifying the whole webview asset bundle per page (a getattrlist sweep over ~100 files each). In a checkout whoseworktrees/dir has 1000+ sibling repos the 12-candidate cap is always hit.Fix
Pre-redirect work is now only: git-only option lists, the opening + selected placeholder, the selected diff itself, and a manifest listing only files that exist. Branch pages carry
resolveBranchBaseOnCompleteand resolve their smart base (cmuxBase -> PR base -> upstream -> origin/HEAD, or explicit--baseas manual/high) at deferred-page completion, after the opening page has redirected; only a selected.branchpage resolves its own repo's base up front. Non-selected placeholders are written after the redirect, then completed as before.ensureDiffViewerAssetsis memoized per (source bundle, target dir) behindos_unfair_lock. The manifest loader skips a listed-but-missing local html page (that entry 404s) instead of failing the whole token, and deferred manifest refreshes use the flock-merge helper.Deferred-page completion support moved to
CLI/CMUXCLI+DiffViewerDeferredPages.swift(swift file length budget); first-paint regression tests live incmuxTests/CMUXOpenDiffViewerFirstPaintTests.swift.Measured on the 1134-sibling-worktree repro (warm): first paint 24-32s before -> 1.0s after (placeholder at 0.27s). The remaining sources/repos/bases complete in the background after the diff is visible.
Two stale assertions in
testDiffCommandGeneratesCodeViewAndOpensBrowserSplitare updated to pin current main behavior (the fda9078 no-path=stdout contract and content-keyed asset dirs); they were failing on main-shaped code already, indicating the tests job hadn't been exercising them.Test structure
Commit 1 adds only the regression test (red), commit 2 the gh deferral (green for that test). The first-paint test also asserts the selected page serves HTTP 200 through the viewer server at redirect time while sibling pages are still pending, and that the fake
ghlog is empty at first paint.Note
Medium Risk
Touches the diff viewer open path, HTTP manifest allow-listing, and git/
ghbranch-base heurics; behavior changes are covered by new first-paint regression tests but the surface area is user-visible and CLI-critical.Overview
cmux diffno longer blocks first paint on smart branch-base resolution (gh/ per-sibling-repo work) or on writing every sibling placeholder up front.Pre-redirect work is narrowed to git option lists, the opening/selected path, the selected diff, and an HTTP manifest that only lists files that already exist. Branch pages get
resolveBranchBaseOnCompleteso smart bases resolve during deferred completion (with a per-repo cache); only the selected branch source resolves its base before redirect when applicable. Placeholders for other sources/repos are written after redirect viawriteDeferredDiffViewerPlaceholder, with manifest refreshes throughrefreshDeferredDiffViewerHTTPManifest.ensureDiffViewerAssetsis memoized under a lock; manifest loading tolerates listed-but-missing HTML pages instead of failing the whole token.Deferred helpers move to
CMUXCLI+DiffViewerDeferredPages.swift(file-length budget). Tests split intoCMUXOpenCommandTestSupport,CMUXOpenDiffViewerFirstPaintTests(noghbefore first paint), and updated assertions for stdout and content-keyed asset paths.Reviewed by Cursor Bugbot for commit 3cd6357. Bugbot is set up for automated code reviews on this repo. Configure here.