Move diff viewer into React app source#5074
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a standalone ChangesDiff Viewer Feature
Sequence Diagram(s)sequenceDiagram
participant Browser
participant startDiffViewer
participant Network
participant WorkerPool
participant CodeView
participant FileTree
Browser->>startDiffViewer: call startDiffViewer(config)
startDiffViewer->>Network: fetch(payload.patchURL) (stream)
Network->>startDiffViewer: stream patch chunks (git/commit boundaries)
startDiffViewer->>WorkerPool: submit highlight tasks (optional)
WorkerPool->>CodeView: return highlighted fragments
startDiffViewer->>CodeView: append parsed diff items (batched)
startDiffViewer->>FileTree: update tree/navigation & git-status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ Passed checks (12 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 moves
Confidence Score: 5/5The PR is safe to merge; the inline viewer extraction, session-restore plumbing, and transparent-background wiring are all self-consistent and the protocol-version bump correctly forces stale local servers to restart. The Swift session-restore path correctly threads transparentBackground from the snapshot through panel creation before restoreSessionSnapshot is called. The long-poll deferred-load replacement from document.write to window.location.reload is the correct fix for ES-module re-evaluation. The protocol version is persisted so stale servers are always restarted. No logic errors found in the diff viewer restore flow, background compositing logic, or streaming renderer. No files require special attention. The two notes are about CI step ordering and TypeScript index-signature coverage in diff-viewer/src/types.ts. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux open diff (CLI)
participant FS as Disk (manifest + HTML)
participant HTTP as Local HTTP Server
participant App as macOS App
participant BP as BrowserPanel
participant WV as WKWebView
participant React as React App (main.mjs)
participant VC as viewer-controller.ts
CLI->>FS: write manifest + HTML shell (config JSON)
CLI->>HTTP: start local HTTP server (protocol: react-app-v1)
CLI->>App: browser.open_split (transparent_background: true)
App->>BP: "create BrowserPanel (usesTransparentBackground=true)"
BP->>WV: bind webview → applyConfiguredWebViewBackground()
BP->>WV: navigate to http://127.0.0.1 port/token/path
WV->>HTTP: GET /token/path
HTTP-->>WV: HTML shell (cmux-diff-viewer-config JSON + main.mjs)
WV->>React: execute main.mjs
React->>VC: startDiffViewer(config)
VC->>HTTP: fetch patchURL → stream diff data
HTTP-->>VC: streaming patch content
Note over App,BP: On next app launch (session restore)
App->>BP: create BrowserPanel (transparentBackground from snapshot)
App->>BP: restoreSessionSnapshot (diffViewerToken + requestPath)
BP->>FS: registerFromManifest(token) → read manifest JSON
BP->>WV: navigate cmux-diff-viewer://token/path
WV->>BP: WKURLSchemeHandler → serve file from FS
WV->>React: execute main.mjs (fresh load, no HTTP server needed)
Reviews (19): Last reviewed commit: "Inherit collapse state for streamed diff..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ! diff -qr "$OUT_DIR" "$tmp_dir" >/tmp/cmux-diff-viewer-app-diff.txt; then | ||
| cat /tmp/cmux-diff-viewer-app-diff.txt >&2 | ||
| echo "diff viewer app assets are stale; run ./scripts/build-diff-viewer-app.sh" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
/tmp/cmux-diff-viewer-app-diff.txt path. If two invocations of --check run concurrently on the same machine (e.g., a developer running it locally while CI also runs), they will race on the same file and one run's error output will be overwritten by the other. Using a temp file created by mktemp keeps each invocation isolated.
| if ! diff -qr "$OUT_DIR" "$tmp_dir" >/tmp/cmux-diff-viewer-app-diff.txt; then | |
| cat /tmp/cmux-diff-viewer-app-diff.txt >&2 | |
| echo "diff viewer app assets are stale; run ./scripts/build-diff-viewer-app.sh" >&2 | |
| exit 1 | |
| fi | |
| diff_output="$(mktemp)" | |
| if ! diff -qr "$OUT_DIR" "$tmp_dir" >"$diff_output"; then | |
| cat "$diff_output" >&2 | |
| rm -f "$diff_output" | |
| echo "diff viewer app assets are stale; run ./scripts/build-diff-viewer-app.sh" >&2 | |
| exit 1 | |
| fi | |
| rm -f "$diff_output" |
| const rootRef = useCallback((node) => { | ||
| if (!node || started.current) { | ||
| return; | ||
| } | ||
| started.current = true; | ||
| queueMicrotask(() => startDiffViewer(config)); | ||
| }, [config]); |
There was a problem hiding this comment.
startDiffViewer is dispatched via queueMicrotask inside a ref callback. React guarantees the full component tree is committed to the DOM before any ref callback fires, so all the document.getElementById calls in startDiffViewer (e.g. #viewer, #toolbar, #file-list) are already available at the point the ref fires — the microtask yield is redundant. The idiomatic React pattern for a post-commit, one-shot side-effect that touches the DOM is useEffect (or useLayoutEffect if it needs to run before paint), which makes the intent explicit and plays correctly with React Strict Mode's double-invocation in development. The started.current guard means the current approach is safe, but future readers will wonder why the extra yield exists.
Rule Used: Flag fixed sleeps, delayed dispatch, timers, polli... (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!
![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 `@diff-viewer/index.html`:
- Line 2: The HTML root element (<html>) is missing a lang attribute which
impairs screen-reader pronunciation and triggers HTMLHint; update the document's
root tag in index.html (the <html> element) to include an appropriate lang value
(e.g., lang="en" or the project's primary language) so accessibility tools and
linters recognize the page language.
🪄 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: 83139185-c0d9-4402-b5dd-20994432978f
⛔ Files ignored due to path filters (1)
diff-viewer/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCLI/cmux_open.swiftResources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/README.mddiff-viewer/index.htmldiff-viewer/package.jsondiff-viewer/src/App.jsxdiff-viewer/src/legacy-viewer.jsdiff-viewer/src/main.jsxdiff-viewer/src/styles.cssdiff-viewer/vite.config.mjsscripts/build-diff-viewer-app.sh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="diff-viewer/src/styles.css">
<violation number="1" location="diff-viewer/src/styles.css:483">
P2: Unconditional `will-change: scroll-position` in the stylesheet forces the browser to maintain a compositor layer for `#viewer` at all times, consuming GPU memory with no measured performance problem to solve. MDN explicitly warns this is a premature optimization anti-pattern: it should only be used as a last resort for existing jank, never speculatively in a stylesheet.</violation>
</file>
<file name="scripts/build-diff-viewer-app.sh">
<violation number="1" location="scripts/build-diff-viewer-app.sh:16">
P2: Avoid writing diff output to a shared hardcoded `/tmp` filename; concurrent runs can clobber each other’s output and produce misleading check failures.</violation>
</file>
<file name="diff-viewer/src/App.jsx">
<violation number="1" location="diff-viewer/src/App.jsx:117">
P3: Initialize the viewer directly from a post-commit effect instead of queueing a microtask in the ref callback; the extra async hop is unnecessary and makes lifecycle behavior harder to reason about in Strict Mode.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| return; | ||
| } | ||
| started.current = true; | ||
| queueMicrotask(() => startDiffViewer(config)); |
There was a problem hiding this comment.
P3: Initialize the viewer directly from a post-commit effect instead of queueing a microtask in the ref callback; the extra async hop is unnecessary and makes lifecycle behavior harder to reason about in Strict Mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At diff-viewer/src/App.jsx, line 117:
<comment>Initialize the viewer directly from a post-commit effect instead of queueing a microtask in the ref callback; the extra async hop is unnecessary and makes lifecycle behavior harder to reason about in Strict Mode.</comment>
<file context>
@@ -0,0 +1,131 @@
+ return;
+ }
+ started.current = true;
+ queueMicrotask(() => startDiffViewer(config));
+ }, [config]);
+
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
diff-viewer/index.html (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a
langattribute to<html>.Missing
langhurts screen-reader pronunciation and accessibility.♿ Proposed fix
-<html> +<html lang="en">🤖 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 `@diff-viewer/index.html` at line 2, Add a lang attribute to the root <html> element to improve accessibility and screen-reader pronunciation: locate the HTML root in index.html (the <html> tag) and set an appropriate language code (e.g., lang="en" or the project's primary language) so the tag becomes <html lang="...">.
🤖 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/legacy-viewer.ts`:
- Line 822: The patch variable is currently declared with loose types (const
patch: { remove?: string[]; set?: any[] } = {}) and menu items at the other
location use any[]; replace these with proper typed interfaces/discriminated
unions to restore type safety: define a Patch type (e.g., { remove?: string[];
set?: Array<YourPatchEntry> }) and a MenuItem discriminated union
(separator/action/segment) like the example in the comment, then update the
declarations (patch and the menu items array) and any functions that consume
them to use those new types (search for the identifier patch and the menu items
array around line 1202 to locate usages) so callers and reducers operate with
concrete fields instead of any.
---
Duplicate comments:
In `@diff-viewer/index.html`:
- Line 2: Add a lang attribute to the root <html> element to improve
accessibility and screen-reader pronunciation: locate the HTML root in
index.html (the <html> tag) and set an appropriate language code (e.g.,
lang="en" or the project's primary language) so the tag becomes <html
lang="...">.
🪄 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: 9bb54b92-57d1-400a-a013-3ddee8df6020
⛔ Files ignored due to path filters (1)
diff-viewer/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCLI/cmux_open.swiftResources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/index.htmldiff-viewer/package.jsondiff-viewer/src/App.tsxdiff-viewer/src/global.d.tsdiff-viewer/src/legacy-viewer.tsdiff-viewer/src/main.tsxdiff-viewer/src/types.tsdiff-viewer/tsconfig.jsondiff-viewer/vite.config.mjs
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="diff-viewer/src/App.jsx">
<violation number="1" location="diff-viewer/src/App.jsx:117">
P3: Initialize the viewer directly from a post-commit effect instead of queueing a microtask in the ref callback; the extra async hop is unnecessary and makes lifecycle behavior harder to reason about in Strict Mode.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
diff-viewer/src/legacy-viewer.ts (2)
74-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against missing
payload.titleandpayload.appearance.Line 74: If
payload.titleis undefined, the document title becomes the literal string"undefined".Line 75: If
payload.appearanceis undefined,applyViewerAppearancewill throw when accessingappearance.themes.light.background.🛡️ Suggested defensive guards
-document.title = payload.title; -applyViewerAppearance(payload.appearance); +if (typeof payload.title === "string") { + document.title = payload.title; +} +if (payload.appearance) { + applyViewerAppearance(payload.appearance); +}Alternatively, if
appearanceis a required contract field, consider adding validation early and throwing a clear error rather than a cryptic property access failure.🤖 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 `@diff-viewer/src/legacy-viewer.ts` around lines 74 - 75, Guard against missing payload.title and payload.appearance: before setting document.title check if payload.title is a non-empty string and only assign it (or fall back to a sensible default like an empty string or "Untitled"); before calling applyViewerAppearance validate payload.appearance (or its nested keys used by applyViewerAppearance, e.g., appearance.themes.light.background) and either supply a default appearance object or throw a clear validation error; update the call sites around document.title and applyViewerAppearance to perform these checks and fail-fast with a descriptive error if appearance is required.
12-16: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd type annotations to
resolveAssetURLparameters.The parameters have implicit
anytypes. Consider typing them explicitly:🔧 Suggested fix
- const resolveAssetURL = (value, name) => { + const resolveAssetURL = (value: unknown, name: string): string => { if (typeof value !== "string" || value.length === 0) { throw new Error(`Missing cmux diff viewer asset: ${name}`); } return new URL(value, window.location.href).href; };🤖 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 `@diff-viewer/src/legacy-viewer.ts` around lines 12 - 16, The function resolveAssetURL currently has implicit any parameters (value, name); update its signature to explicitly type the parameters (e.g., value: string, name: string) and its return type (e.g., : string) so TypeScript can validate inputs and outputs; locate the resolveAssetURL declaration and add the appropriate parameter and return type annotations while keeping the existing runtime checks intact.
🤖 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.
Outside diff comments:
In `@diff-viewer/src/legacy-viewer.ts`:
- Around line 74-75: Guard against missing payload.title and payload.appearance:
before setting document.title check if payload.title is a non-empty string and
only assign it (or fall back to a sensible default like an empty string or
"Untitled"); before calling applyViewerAppearance validate payload.appearance
(or its nested keys used by applyViewerAppearance, e.g.,
appearance.themes.light.background) and either supply a default appearance
object or throw a clear validation error; update the call sites around
document.title and applyViewerAppearance to perform these checks and fail-fast
with a descriptive error if appearance is required.
- Around line 12-16: The function resolveAssetURL currently has implicit any
parameters (value, name); update its signature to explicitly type the parameters
(e.g., value: string, name: string) and its return type (e.g., : string) so
TypeScript can validate inputs and outputs; locate the resolveAssetURL
declaration and add the appropriate parameter and return type annotations while
keeping the existing runtime checks intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff838dc1-cef9-4002-9251-42fce821b6a0
📒 Files selected for processing (2)
Resources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/src/legacy-viewer.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5827e15ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <body> | ||
| <script id="cmux-diff-viewer-config" type="application/json">\(configLiteral)</script> | ||
| <div id="root"></div> | ||
| <script type="module" src="\(appModuleURL)"></script> |
There was a problem hiding this comment.
The macOS unit step in .github/workflows/ci.yml:311-328 runs the cmux-unit suite, and cmuxTests/CMUXOpenCommandTests.swift:387-403 still asserts that the generated HTML string contains the old inline viewer DOM and script snippets such as id="files-sidebar" and WORKER_POOL_MODULE_URL. With this change the page only emits the config, root node, and external module script here, so those assertions no longer match and the unit job fails whenever this test runs. Please update the test to inspect the generated app asset or the new minimal HTML shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
diff-viewer/src/viewer-controller.ts (3)
74-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid unconditionally resetting
document.titleinside the controller.This overrides the fallback title already set in
main.tsxwhenpayload.titleis absent. Guard the assignment so it only runs for non-empty titles.Suggested fix
-document.title = payload.title; +if (typeof payload.title === "string" && payload.title.length > 0) { + document.title = payload.title; +}🤖 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 `@diff-viewer/src/viewer-controller.ts` at line 74, The controller currently unconditionally assigns document.title = payload.title which overwrites the fallback set in main.tsx; update the assignment to only run when payload.title is a non-empty string (e.g. check payload.title exists and payload.title.trim() !== '') so document.title is left intact when no title is provided. Locate the statement in viewer-controller.ts (the document.title / payload.title assignment) and replace it with a guarded conditional that only sets document.title for valid, non-empty titles.
263-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocalize the hardcoded
Commitlabel.
Commit ${index + 1}is user-facing English text in production TS code and should come from localized labels.As per coding guidelines: “For production user-facing text, fail partial localization … web UI … changes must read from
next-intlor another locale-specific source and update every locale …”.🤖 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 `@diff-viewer/src/viewer-controller.ts` at line 263, The hardcoded user-facing string `Commit ${index + 1}` should be replaced with a localized label: fetch the locale message (e.g., a message key like "commitLabel" with a placeholder) from next-intl (or your app's i18n helper) and format it with the index+1 value instead of returning the template literal directly; update the corresponding locale files to include the "commitLabel" entry (e.g., "Commit {number}") so all locales are covered.
1450-1454:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFlat file-tree refresh does repeated full-list rebuilds during streaming.
In fallback (non-Pierre) mode, each refresh clears and rebuilds the entire file list, which is repeated full-collection work on a hot path and will degrade with large diffs (~1000 files).
As per coding guidelines: apply
.github/review-bot-rules/algorithmic-complexity.mdand flag repeated full-collection work in hot paths for scalable user data.🤖 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 `@diff-viewer/src/viewer-controller.ts` around lines 1450 - 1454, The fallback refresh currently always clears fileList (fileList.textContent = "") and calls setupFlatFileExplorer(entries), causing repeated full-list rebuilds; change this hot-path to perform incremental updates: keep a cached previousEntries map (by path/id) in the viewer controller, compute a delta between sourceEntries(source) and previousEntries, call syncFileTreeSelectionMaps as needed, and update the DOM via targeted add/remove/update operations (or provide a new setupFlatFileExplorerIncremental(entriesDelta) helper) instead of clearing and rebuilding the entire list; ensure functions referenced (sourceEntries, syncFileTreeSelectionMaps, setupFlatFileExplorer, fileList) are updated to support incremental diff application and update previousEntries after applying deltas.
🤖 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.
Outside diff comments:
In `@diff-viewer/src/viewer-controller.ts`:
- Line 74: The controller currently unconditionally assigns document.title =
payload.title which overwrites the fallback set in main.tsx; update the
assignment to only run when payload.title is a non-empty string (e.g. check
payload.title exists and payload.title.trim() !== '') so document.title is left
intact when no title is provided. Locate the statement in viewer-controller.ts
(the document.title / payload.title assignment) and replace it with a guarded
conditional that only sets document.title for valid, non-empty titles.
- Line 263: The hardcoded user-facing string `Commit ${index + 1}` should be
replaced with a localized label: fetch the locale message (e.g., a message key
like "commitLabel" with a placeholder) from next-intl (or your app's i18n
helper) and format it with the index+1 value instead of returning the template
literal directly; update the corresponding locale files to include the
"commitLabel" entry (e.g., "Commit {number}") so all locales are covered.
- Around line 1450-1454: The fallback refresh currently always clears fileList
(fileList.textContent = "") and calls setupFlatFileExplorer(entries), causing
repeated full-list rebuilds; change this hot-path to perform incremental
updates: keep a cached previousEntries map (by path/id) in the viewer
controller, compute a delta between sourceEntries(source) and previousEntries,
call syncFileTreeSelectionMaps as needed, and update the DOM via targeted
add/remove/update operations (or provide a new
setupFlatFileExplorerIncremental(entriesDelta) helper) instead of clearing and
rebuilding the entire list; ensure functions referenced (sourceEntries,
syncFileTreeSelectionMaps, setupFlatFileExplorer, fileList) are updated to
support incremental diff application and update previousEntries after applying
deltas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a766da6-39bc-44d1-a71c-e0e31cf700ac
⛔ Files ignored due to path filters (1)
diff-viewer/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/ci.ymlResources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/package.jsondiff-viewer/src/App.tsxdiff-viewer/src/main.tsxdiff-viewer/src/styles.cssdiff-viewer/src/viewer-controller.tsdiff-viewer/vite.config.mjs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 327cbfa8ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| viewerElement.replaceChildren(status); | ||
| } | ||
| document.body.dataset.loading = options.loading === true || options.pending === true ? "true" : "false"; | ||
| document.body.dataset.statusOnly = "false"; |
There was a problem hiding this comment.
When the payload has a statusMessage (for example an error/no-diff page, or a pending replacement page), showStatusMessage now forces data-status-only to false. The previous generated HTML set data-status-only="true" for those pages and had CSS to hide #files-sidebar; with this React version, status pages keep the empty file sidebar and 0-count footer visible beside the message. Preserve the status-only state for status/pending pages or explicitly hide the sidebar in that branch.
Useful? React with 👍 / 👎.
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 `@scripts/build-diff-viewer-app.sh`:
- Around line 17-21: The failure handling for the diff command treats any
non-zero exit from diff as "stale assets"; change the logic around the diff
invocation (the diff command comparing "$OUT_DIR" and "$tmp_dir" writing to
"$diff_output") to inspect diff's exit status ($?) and differentiate status 1
(files differ) from status 2 (runtime/error): if status is 1, print the diff
content and exit 1 with the stale-assets message as currently done; if status is
2, print the diff output and/or a clear error message indicating diff failed
(include the error/errno if available) and exit with a distinct non-zero code
(e.g., 2) so callers can distinguish runtime errors from mismatches.
🪄 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: 46dc91af-5fab-4a79-bf53-e922ae75e681
📒 Files selected for processing (8)
Resources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/index.htmldiff-viewer/src/App.tsxdiff-viewer/src/global.d.tsdiff-viewer/src/styles.cssdiff-viewer/src/viewer-controller.tsdiff-viewer/tsconfig.jsonscripts/build-diff-viewer-app.sh
💤 Files with no reviewable changes (1)
- diff-viewer/src/styles.css
Match the markdown panel: the page paints no fill (body transparent) and lets the cmux window backdrop provide the background, so transparent Ghostty themes show through identically. `--cmux-diff-bg` stays opaque since it feeds text and menu color-mix blends. Removes the now-unused page-owned opacity baking. Enable React Compiler via babel-plugin-react-compiler (React 19 target), wired through @vitejs/plugin-react's babel option. Vite 7 + plugin-react 5 + React 19.2 (which ships react/compiler-runtime) support it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| "react": "19.2.3", | ||
| "react-dom": "19.2.3", | ||
| "vite": "^7.2.7" | ||
| }, |
There was a problem hiding this comment.
Build tools incorrectly listed as production dependencies
Medium Severity
@vitejs/plugin-react, vite, and other build-only packages are listed under dependencies instead of devDependencies. In Vite's lib mode, all packages in dependencies are externalized by default, meaning react and react-dom would produce bare-specifier imports (e.g., import ... from 'react') in the built main.mjs that cannot resolve in a browser context. Moving vite and @vitejs/plugin-react to devDependencies is also correct for package hygiene, but the main risk is that react and react-dom being in dependencies triggers externalization, producing a broken runtime bundle.
Reviewed by Cursor Bugbot for commit 11c4097. Configure here.
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 `@Sources/Panels/BrowserPanel.swift`:
- Around line 3444-3448: The usesTransparentBackground Bool is currently latched
for the BrowserPanel lifetime which keeps the WKWebView clear after navigations;
change this to be scoped to the current content/URL instead and update the code
paths that apply/clear the webview background (e.g. usesTransparentBackground,
applyWebViewBackground(color:), and the navigation/delegate handlers in
BrowserPanel such as the methods that handle newContent/navigation decisions) so
the clear background is only enabled when the active content is an internal cmux
surface (or URL scheme) and is explicitly disabled when the panel navigates away
or is repurposed; gate the clear-background logic on content kind or URL scheme
and ensure any navigation delegate callbacks clear the transparent state.
🪄 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: b077caa7-664f-4cf4-80a1-9c2005c1183e
⛔ Files ignored due to path filters (1)
diff-viewer/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CLI/cmux_open.swiftResources/markdown-viewer/diff-viewer-app/main.mjsSources/Panels/BrowserPanel.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/CMUXOpenCommandTests.swiftdiff-viewer/package.jsondiff-viewer/src/appearance.tsdiff-viewer/src/styles.cssdiff-viewer/test/appearance.test.tsdiff-viewer/vite.config.mjs
Save the diff viewer token, request path, and transparent-background flag in the browser surface session snapshot instead of discarding diff URLs as temporary. On restore, BrowserPanel re-registers the token from its on-disk manifest with the app-owned CmuxDiffViewerURLSchemeHandler and navigates via the cmux-diff-viewer:// custom scheme, so the diff comes back even though the local HTTP server that originally served it is gone. Restored surfaces also come back transparent. (Diff content currently lives in /tmp, which survives kill/reopen; a durable Application Support store is a follow-up.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
react-doctor flagged the diff viewer root callback ref as redundant manual memoization. React Compiler memoizes it, so use the bare function. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The job will cover more in-app React UIs than the diff viewer over time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8aa252e40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let fileObjects = object["files"] as? [[String: Any]] else { | ||
| return false | ||
| } | ||
| let files = fileObjects.compactMap { Self.registeredFile(from: $0) } |
There was a problem hiding this comment.
When a streamed GitHub diff is opened with CMUX_DIFF_VIEWER_STREAM_REMOTE=1, the manifest entry for the patch sidecar is written with remote_url and an empty file_path. This restoration path converts every manifest entry into a local RegisteredFile, so register(...) later rejects the remote patch entry as an unreadable local file and registerFromManifest returns false; the persisted remote diff viewer cannot be restored after an app restart. Preserve remote_url through the scheme handler or avoid trying to restore these manifests as local-only files.
Useful? React with 👍 / 👎.
| if let token = snapshot.diffViewerToken, | ||
| let requestPath = snapshot.diffViewerRequestPath, | ||
| CmuxDiffViewerURLSchemeHandler.shared.registerFromManifest(token: token), | ||
| let diffURL = CmuxDiffViewerURLSchemeHandler.diffViewerURL(token: token, requestPath: requestPath) { |
There was a problem hiding this comment.
For restored diff viewers that have source/repo/base selectors, the HTML payload still contains the absolute http://127.0.0.1:<old-port>/... option URLs generated when the page was first written. This branch only changes the currently restored page to cmux-diff-viewer://..., so after an app restart (when the original local HTTP server is gone) changing a selector navigates back to the stale port even though the alternate pages are in the manifest. Re-map the payload option URLs to the custom scheme during restore or store them as restorable URLs when the pages are generated.
Useful? React with 👍 / 👎.
Address Codex review: - shouldRenderWebViewForSessionSnapshot() returned false for diff viewers (their URL is "temporary", so the persisted URL is nil), so a restored diff surface registered its token but never navigated. Honor the render intent for diff viewers via diffViewerSessionComponents(). - registerFromManifest() now skips manifests with remote_url / empty file_path entries (streamed remote diffs the local scheme handler cannot serve) instead of throwing inside register(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Codex review: persisting any diff viewer left a blank panel on restart when the diff could not be restored via the custom scheme. Gate persistence on a new diffViewerRestorable() check that requires a local-only manifest (no remote_url streamed PR diffs) and an entry page that is not a pending placeholder (whose deferred-load wait endpoint only the local HTTP server serves). Factored manifest loading into localManifestFiles(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| private func localManifestFiles(token: String) -> [RegisteredFile]? { | ||
| guard Self.isValidToken(token) else { return nil } | ||
| let manifestURL = trustedRootURL.appendingPathComponent(".manifest-\(token).json", isDirectory: false) | ||
| guard let data = try? Data(contentsOf: manifestURL), | ||
| let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], | ||
| let fileObjects = object["files"] as? [[String: Any]], | ||
| !fileObjects.isEmpty else { | ||
| return nil | ||
| } | ||
| var files: [RegisteredFile] = [] | ||
| for fileObject in fileObjects { | ||
| let filePath = fileObject["file_path"] as? String ?? "" | ||
| if fileObject["remote_url"] is String || filePath.isEmpty { | ||
| return nil | ||
| } | ||
| guard let file = Self.registeredFile(from: fileObject) else { return nil } | ||
| files.append(file) | ||
| } | ||
| return files | ||
| } | ||
|
|
||
| /// Whether a diff viewer surface can be restored through the custom scheme. | ||
| /// Requires a local-only manifest and an entry page that is not a pending | ||
| /// placeholder — pending pages poll a deferred-load wait endpoint that only | ||
| /// the local HTTP server implements, so they would render-fail under the | ||
| /// custom scheme. | ||
| func diffViewerRestorable(token: String, requestPath: String) -> Bool { | ||
| guard let files = localManifestFiles(token: token), | ||
| let entry = files.first(where: { $0.requestPath == requestPath }), | ||
| let handle = try? FileHandle(forReadingFrom: entry.fileURL) else { | ||
| return false | ||
| } | ||
| defer { try? handle.close() } | ||
| let head = (try? handle.read(upToCount: 1024)) ?? Data() | ||
| if let text = String(data: head, encoding: .utf8), | ||
| text.contains("data-cmux-diff-pending=\"true\"") { | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| /// Extracts the diff viewer `(token, requestPath)` from a live diff viewer | ||
| /// URL, accepting both the custom scheme (`cmux-diff-viewer://<token>/<path>`) | ||
| /// and the local HTTP server form (`http://127.0.0.1:<port>/<token>/<path>#cmux-diff-viewer`). | ||
| static func diffViewerComponents(from url: URL?) -> (token: String, requestPath: String)? { |
There was a problem hiding this comment.
localManifestFiles performs blocking disk reads (Data(contentsOf:), FileHandle(forReadingFrom:), handle.read(upToCount:)) and is called from two @MainActor-isolated BrowserPanel paths:
restoreSessionSnapshot→registerFromManifest→localManifestFiles(which also feeds intoregister, itself doingFileManager.default.fileExistsandisReadableFilefor every registered file)shouldPersistSessionSnapshot→diffViewerRestorable→localManifestFiles+FileHandleread
Both paths run synchronously on the main actor — the session-restore path runs at app launch, and shouldPersistSessionSnapshot may be called on every save cycle. The full chain (manifest JSON read → file-exists checks per registered file → 1 KB header read) blocks the main thread for each diff viewer surface being restored or persisted. These helpers should be async, dispatching their I/O off the main actor (e.g. via nonisolated func ... async throws using structured concurrency), with the callers await-ing the result.
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!
Address Codex review: a restored diff viewer runs under cmux-diff-viewer://, but the source/repo/base option URLs were generated against the now-dead local HTTP server, so switching diff targets after restart hit a dead port. Rewrite those option URLs to the custom scheme (reusing the current token, dropping the query/fragment the scheme handler rejects) when the page runs under the custom scheme; same-session http pages are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3dd54907
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| navigateWithoutInsecureHTTPPrompt( | ||
| to: diffURL, | ||
| recordTypedNavigation: false, | ||
| preserveRestoredSessionHistory: false | ||
| ) |
There was a problem hiding this comment.
When a saved diff viewer is restored through cmux-diff-viewer://, the HTML is served by CmuxDiffViewerURLSchemeHandler, whose CSP sets default-src 'none' and never adds worker-src 'self'. In this restored-only path the React viewer still calls createCodeViewWorkerPool() and attempts to spawn the bundled diff worker, so WebKit blocks the worker and every restored large diff silently falls back to main-thread highlighting; same-session HTTP viewers do not hit that CSP. Please add a worker source directive for the custom-scheme HTML response before navigating restored viewers this way.
Useful? React with 👍 / 👎.
Address Codex review: deferred diff pages already evaluated main.mjs, and module scripts run once per realm, so document.write-ing the final replacement HTML left an empty #root (the module bootstrap never re-ran). Reload instead so a fresh document load re-bootstraps React against the now-final on-disk HTML. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 35c7679. Configure here.
#app is a grid with only grid-template-rows set, so its implicit single column defaulted to `auto` (max-content) and let #content size to its content width instead of the viewport. With a narrow pane that pushed the files sidebar past the window's right edge (measured #content 887px vs viewport 753px). Constrain #app's column to minmax(0, 1fr) so #content fits the viewport and the grid distributes viewer + files within it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Codex review: deferred git diffs overwrite the opening page with a data-cmux-diff-redirect stub that bounces to the original http://127.0.0.1:<port> URL. diffViewerRestorable only rejected pending placeholders, so a snapshot captured before the WebView followed the redirect would restore the stub and navigate to a dead local HTTP server. Reject redirect stubs too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Codex review: "Collapse all diffs" only rewrote already-flushed items, so items parsed afterward (while a large diff is still streaming) rendered expanded despite the toolbar showing collapsed. Initialize new diff items with collapsed: appState.collapsed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
cmux open diffunderdiff-viewer/.Resources/markdown-viewer/diff-viewer-app/, because the macOS app serves bundled static resources.CLI/cmux_open.swiftwith a small HTML shell plus JSON config../scripts/build-diff-viewer-app.shand a CI job that verifies generated assets and runs React Doctor with--fail-on error.Related follow-up: #5064
Verification
./scripts/build-diff-viewer-app.sh./scripts/build-diff-viewer-app.sh --checkbun run react-doctor:cifromdiff-viewer/(0 errors, 8 warnings remain in the legacy bridge)swiftc -parse CLI/cmux_open.swiftswiftc -parse Sources/Panels/BrowserPanel.swiftgit diff --cached --checkBrowser smoke note: the in-app Browser backend was unavailable in this session, so I could not complete the local browser fixture smoke before handoff.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Large behavioral surface (streaming diff, workers, session restore) moved out of Swift into a new bundle; mitigated by protocol versioning, asset check CI, and retained Pierre asset copy path.
Overview
The diff viewer UI moves from thousands of lines of inline HTML/CSS/JS in
cmux_open.swiftto a built React app (diff-viewer/) loaded via a minimal shell: JSON in#cmux-diff-viewer-configandmain.mjs. Swift now copies bundledcmux-diff-viewer-appassets alongside the existing Pierre bundles and passes a structuredassets+payloadconfig.CI adds
react-apps-check: verify generated assets (build-diff-viewer-app.sh --check), typecheck, tests, oxlint, and React Doctor fordiff-viewer/.Host integration changes include
transparent_backgroundon browser open,background-opacityfrom Ghostty config in appearance JSON, a bumped local HTTPprotocolVersion(react-app-v1) so stale servers restart, andcommitin localized labels. Diff viewer surfaces can persist/restore with the custom scheme (per PR description; Swift wiring supports the slimmer page contract).Reviewed by Cursor Bugbot for commit a8f5607. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Moved the diff viewer into a source-owned React/Vite TypeScript app that builds a single
main.mjs. Added transparent backdrops and reliable session restore; only restorable local diffs are kept (skips remote-only manifests and redirect stubs), restored diff switcher URLs use the custom scheme, streamed items inherit the collapse state, and a layout bug that clipped the files sidebar in narrow panes is fixed.Refactors
main.mjsintoResources/markdown-viewer/diff-viewer-app/; existing Pierre bundles remain.cmux-diff-viewer-configJSON; improves the loading shell.transparent_backgroundtobrowser.open_split, threaded throughWorkspace→BrowserPanel; the diff viewer opts in with a transparent page body so Ghostty themes show through.transparentBackground, token, and request path; on restoreBrowserPanelre-registers via the app-owned scheme handler and navigates withcmux-diff-viewer://, independent of the local HTTP server. Only persists restorable viewers (local-only manifests, non-pending entry); restore skips remote-only manifests and redirect stubs. Also rewrites diff switcher option URLs to the custom scheme so switching targets works after a restart.assets,payloadliketitle,statusMessage,pendingReplacement,externalURL,labels,appearance); moves logic intosrc/viewer-controller.tsandApp.tsx; converts to TypeScript, adds Tailwind via@tailwindcss/vite, and enables React Compiler (babel-plugin-react-compilervia@vitejs/plugin-react).react-app-v1and persistsprotocolVersion; ships default labels with an optional dev assert; renames the CI job toreact-apps-check(build assets via./scripts/build-diff-viewer-app.sh --check, typecheck, tests,oxlintdeny-warnings, React Doctor); updates tests for config parsing, file-tree refresh, backdrop behavior, Pierre tree bundle APIs, and session restore.document.writeso the app reboots cleanly when the final HTML is ready.#app’s grid column tominmax(0, 1fr).Migration
diff-viewer/, then run./scripts/build-diff-viewer-app.shand commit the updated assets../scripts/build-diff-viewer-app.sh --check(or rely on CI) to confirm assets are current and types/tests pass.Written for commit a8f5607. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Style
Tests / CI