Render diff viewer empty state instead of raw error (issue #5246)#5252
Merged
Conversation
A last-turn diff with no recorded baseline (and any all-empty git diff source) currently throws a developer-facing CLIError that renders the raw "No last-turn diff baseline recorded..." string in the diff panel and exits non-zero, which trips the diff-viewer launcher's NSSound.beep(). These tests assert the intended behavior: the diff viewer opens (exit 0), renders a friendly non-error empty state (statusIsError == false) with the source switcher still present, and never shows the raw baseline error. They fail on the current code (which errors instead of opening) and pass once the empty-state fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When there is nothing to diff — no recorded last-turn baseline, or every
git source is empty — the diff viewer threw a CLIError. That raw,
developer-facing string ("No last-turn diff baseline recorded for this
workspace and surface yet...") was rendered verbatim as the panel's empty
state, and the non-zero CLI exit tripped the diff-viewer launcher's
NSSound.beep() (AppDelegate.launchDiffViewerProcess termination handler) —
the "unable to click" beep the reporter heard.
Root cause: "nothing to diff" was modeled as an error rather than a
legitimate empty state. Fix it at the source of truth:
- latestAgentTurnDiffBaseline returns nil for a missing baseline instead
of throwing; readGitDiffInput emits an empty patch for that case, so it
flows through the same EmptyDiffSourceError path as "no changes".
- Route EmptyDiffSourceError to a friendly, non-error empty-state page
(isError: false) with the Unstaged/Staged/Branch switcher visible, in
both the synchronous selected-source path and the deferred-source path,
instead of throwing CLIError / rendering isError: true. Last turn keeps
its no-silent-fallback behavior but now shows its own empty state.
- The CLI now exits 0 for an empty diff, so the launcher no longer beeps.
Reuses the existing localized diffViewer.empty.* strings (no new copy; the
localized source switcher is the actionable guidance).
Defensive: add an empty doCommand(by:) override to CmuxWebView (mirrors
GhosttyTerminalView's documented NSBeep suppression) so any unhandled
command in a non-interactive status-only page is a harmless no-op.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe diff transforms ChangesEmpty Diff Friendly State
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 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 fixes two symptoms caused by treating "nothing to diff" as a thrown error rather than an empty state: a raw developer-facing CLI error shown in the diff viewer panel, and a system beep when opening an empty-diff panel. The root fix is that
Confidence Score: 5/5Safe to merge. The change narrows a thrown-error path to an in-process empty state; no new mutable shared state, no new concurrency primitives, and no data is written unless it was already being written. The fix correctly models 'nothing to diff' as a first-class empty state: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["cmux diff --last-turn"] --> B{latestAgentTurnDiffBaseline}
B -- "record found" --> C["Build patch from git diff + untracked"]
B -- "nil (no baseline yet)" --> D["patch = empty string"]
C --> E["nonEmptyGitDiffInput"]
D --> E
E -- "patch is non-empty" --> F["DiffInput with content"]
E -- "patch is empty" --> G["throw EmptyDiffSourceError"]
F --> H["Write diff HTML + exit 0"]
G --> I{selectedSource == .lastTurn?}
I -- yes --> J["selectedEmptyMessage = error.message\nselectedInput = nil"]
I -- no, try fallback --> K{Any fallback non-empty?}
K -- yes --> L["Render fallback; mark original as empty state"]
K -- no --> J
J --> M["writeDiffViewerStatusHTML\nisError: false\nwith source switcher"]
M --> N["Exit 0 — friendly empty state shown\nNo NSSound.beep"]
L --> N
Reviews (3): Last reviewed commit: "Polish diff viewer empty state into a ce..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@CLI/cmux_open.swift`:
- Around line 3745-3777: The function writeDeferredDiffViewerEmptyState
currently swallows failures by using try? when calling writeDiffViewerStatusHTML
and always returns a successful DiffViewerDeferredCompletion; change
writeDeferredDiffViewerEmptyState to propagate or handle write errors instead of
ignoring them: either make writeDeferredDiffViewerEmptyState a throwing function
and call try writeDiffViewerStatusHTML so failures bubble up, or check the
Result/error from writeDiffViewerStatusHTML and return an
error/completed-failure path (or nil) instead of the success
DiffViewerDeferredCompletion; update callers to handle the thrown error or
optional/failure return accordingly so a failed write does not report a
completed deferred page.
🪄 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: 5051338b-1c91-43dc-afa4-f38de2ce0d17
📒 Files selected for processing (3)
CLI/cmux_open.swiftSources/Panels/CmuxWebView.swiftcmuxTests/CMUXOpenCommandTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…te write errors (#5246) - Remove the empty doCommand(by:) override on CmuxWebView. It was placed on the shared base class and silently swallowed every unhandled command for ALL web pages (not just empty diff pages), bypassing the default NSResponder forward to nextResponder (greptile P2, cubic P1). The "unable to click" beep is the diff-viewer launcher's NSSound.beep() on a non-zero CLI exit, which the empty-state-exits-0 change already fixes, so the broad override is unnecessary. - Split writeDeferredDiffViewerEmptyState into a throwing page writer (writeDiffViewerEmptyStatePage) plus a pure completion builder. The returned-completion paths now use `try` so a failed status-page write propagates instead of reporting success with a stale loading page (coderabbit major). The secondary fallback page render stays best-effort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
austinywang
dismissed
coderabbitai[bot]’s stale review
CodeRabbit confirmed the fix in its follow-up ("The split looks correct…") and resolved its thread; dismissing the stale changes-requested review. The try? swallow was fixed in 3b81bb2.
The empty diff state rendered as a small left-aligned status strip floating in the panel. Restyle it as a proper centered empty state (icon badge above a centered message), closer to a polished "nothing to show" page. - App.tsx: split #status into a persistent #status-icon badge and a #status-text node (so updating the message text no longer wipes the icon). - viewer-controller: write the message into #status-text. - styles.css: center the status-only layer, stack icon + message, and show a themed (currentColor-masked) document glyph only for the friendly empty state — not for errors or while loading. Reuses the existing localized message; no new user-facing strings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 `@diff-viewer/src/styles.css`:
- Line 623: Replace the invalid keyword-case usage in the CSS declaration that
sets background-color: currentColor; — locate the rule using the
background-color property (the declaration shown as background-color:
currentColor;) and change the value to lowercase "currentcolor" so it complies
with the Stylelint value-keyword-case rule and avoids CI failures.
🪄 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: 415e7da9-15a5-493c-a204-f69672a36450
📒 Files selected for processing (4)
Resources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/src/App.tsxdiff-viewer/src/styles.cssdiff-viewer/src/viewer-controller.ts
austinywang
dismissed
coderabbitai[bot]’s stale review
The single flagged item (currentColor casing) is a false positive: this repo has no Stylelint, the file already uses currentColor in 5 places, and the keyword is valid CSS. All other findings were fixed. Dismissing the stale changes-requested review.
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.
Problem (issue #5246)
When the diff viewer has nothing to show — most notably a last-turn diff with no recorded baseline (e.g. a clean checkout before any agent turn ran cmux hooks) — two things go wrong:
Root cause (one cause, both symptoms)
"Nothing to diff" was modeled as a thrown error (
CLIError) rather than a legitimate empty state:latestAgentTurnDiffBaselinethrew raw jargon when no baseline existed.EmptyDiffSourceErrorfor last turn (and the all-sources-empty case) was re-thrown asCLIError.The thrown error (1) was rendered verbatim via the
isError: truestatus page, and (2) made the diff-viewer CLI exit non-zero, which trippedAppDelegate.launchDiffViewerProcess's termination handlerNSSound.beep()— that's the beep.Fix (empty diff = empty state, not error)
latestAgentTurnDiffBaselinereturnsnilfor a missing baseline instead of throwing;readGitDiffInputemits an empty patch, folding it into the sameEmptyDiffSourceErrorpath as "no changes".EmptyDiffSourceErrorto a friendly, non-error empty-state page (isError: false) with the Unstaged/Staged/Branch switcher visible, in both the synchronous selected-source path and the deferred-source path — instead ofthrow CLIError/isError: true. Last turn keeps its no-silent-fallback behavior but now shows its own empty state.NSSound.beep()never fires — this is what eliminates the "unable to click" beep.try?on the returned-completion paths), so the deferred pipeline never reports success while a stale loading page remains.Visual polish (empty state)
The empty state is now a proper centered "nothing to show" layout instead of a small left-aligned status strip:
currentColor-masked document glyph that adapts to light/dark) above the message.Implemented in the
diff-viewer/web app (App.tsx,viewer-controller.ts,styles.css); rebuilt assets are committed underResources/markdown-viewer/diff-viewer-app/(CI staleness check passes).Tests (two-commit red/green)
CMUXOpenCommandTeststhat fail on current code: the diff viewer must open (exit 0), render a friendly empty state (statusIsError == false) with the source switcher present (last turn still selected — no silent fallback), and never show the raw baseline string. Covers the no-baseline path, the empty-last-turn path, the wrong-surface (deferred) path, and the all-sources-empty path.The beep itself is downstream of the CLI exit code: exiting 0 for the empty case removes the
NSSound.beep()trip, and that exit-code behavior is covered by the CLI tests above. The remaining responder/visual behavior is verified by manual dogfood.Localization audit
No new user-facing strings; reuses existing localized
diffViewer.empty.*anddiffViewer.source.*keys (en + ja inResources/Localizable.xcstrings). The empty-state polish is presentational (icon + layout), reusing the same localized message. Nothing indiff-viewer/copy changed.