Skip to content

Diff viewer: review comments with per-repo persistence and TextBox attach #5768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lawrencecchen merged 13 commits into from
Jun 11, 2026
Merged

Diff viewer: review comments with per-repo persistence and TextBox attach #5768

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
053f3cb
Add diff comment persistence, native bridge, and TextBox attach
lawrencecchen Jun 10, 2026
a2982d1
Add review comments UI to the diff viewer webview
lawrencecchen Jun 10, 2026
954ec0d
Content-key the shared diff viewer app asset cache
lawrencecchen Jun 10, 2026
710e455
Fix diff comments trust gate for router-rewritten page URLs
lawrencecchen Jun 10, 2026
ed02a99
Refine attach targeting for multiple open TextBoxes, fix streaming ou…
lawrencecchen Jun 10, 2026
098bc33
Add explicit attach-target dropdown to the comment composer
lawrencecchen Jun 10, 2026
026cb93
Replace attach targeting with a workspace pending-comments pool
lawrencecchen Jun 10, 2026
dc198fa
Render submissions as fenced diffs; chip above input with hover preview
lawrencecchen Jun 10, 2026
297c537
Fix double-spaced excerpts, popover hover grace, Cmd+Enter save
lawrencecchen Jun 11, 2026
b236900
Chip popover opens on click; add padding above the chip
lawrencecchen Jun 11, 2026
feecfad
Fix CI budgets: MainActor trust-gate helpers, refresh file length budget
lawrencecchen Jun 11, 2026
80149ac
Merge remote-tracking branch 'origin/main' into feat-diff-comments
lawrencecchen Jun 11, 2026
02dab1d
Resolve DiffCommentStore.shared default argument in the MainActor ini…
lawrencecchen Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 73 additions & 14 deletions CLI/cmux_open.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import CryptoKit
import Darwin
import Foundation

Expand Down Expand Up @@ -400,7 +401,22 @@ extension CMUXCLI {
static func localized() -> DiffViewerLabels {
DiffViewerLabels(values: [
"additions": CMUXDiffViewerLocalization.string("diffViewer.additions", defaultValue: "Additions"),
"addComment": CMUXDiffViewerLocalization.string("diffViewer.addComment", defaultValue: "Add comment"),
"attachComment": CMUXDiffViewerLocalization.string("diffViewer.attachComment", defaultValue: "Attach"),
"attachFailed": CMUXDiffViewerLocalization.string("diffViewer.attachFailed", defaultValue: "Attach failed"),
"attachOnSave": CMUXDiffViewerLocalization.string("diffViewer.attachOnSave", defaultValue: "Attach to terminal TextBox"),
"attachedComment": CMUXDiffViewerLocalization.string("diffViewer.attachedComment", defaultValue: "Attached"),
"bars": CMUXDiffViewerLocalization.string("diffViewer.bars", defaultValue: "Bars"),
"cancelComment": CMUXDiffViewerLocalization.string("diffViewer.cancelComment", defaultValue: "Cancel"),
"chooseTerminal": CMUXDiffViewerLocalization.string("diffViewer.chooseTerminal", defaultValue: "Choose a terminal"),
"comments": CMUXDiffViewerLocalization.string("diffViewer.comments", defaultValue: "Comments"),
"commentPlaceholder": CMUXDiffViewerLocalization.string("diffViewer.commentPlaceholder", defaultValue: "Leave a comment"),
"deleteComment": CMUXDiffViewerLocalization.string("diffViewer.deleteComment", defaultValue: "Delete"),
"editComment": CMUXDiffViewerLocalization.string("diffViewer.editComment", defaultValue: "Edit"),
"noComments": CMUXDiffViewerLocalization.string("diffViewer.noComments", defaultValue: "No comments yet"),
"noTerminalForAttach": CMUXDiffViewerLocalization.string("diffViewer.noTerminalForAttach", defaultValue: "No terminal available"),
"outdatedComment": CMUXDiffViewerLocalization.string("diffViewer.outdatedComment", defaultValue: "Outdated"),
"saveComment": CMUXDiffViewerLocalization.string("diffViewer.saveComment", defaultValue: "Comment"),
"changedFiles": CMUXDiffViewerLocalization.string("diffViewer.changedFiles", defaultValue: "Changed files"),
"classic": CMUXDiffViewerLocalization.string("diffViewer.classic", defaultValue: "Classic"),
"commit": CMUXDiffViewerLocalization.string("about.commit", defaultValue: "Commit"),
Expand Down Expand Up @@ -833,21 +849,28 @@ extension CMUXCLI {
)
if let cwd = parsedArgs.cwd {
diffSourceContext.repoRoot = try gitRepoRoot(startingAt: resolvePath(cwd))
}
if parsedArgs.source != nil {
try resolveTargetIfNeeded()
var sourceContext = try canonicalDiffSourceContext(
workspaceHandle: workspaceHandle,
surfaceHandle: surfaceHandle,
windowHandle: windowHandle,
client: try connectedClient()
} else if parsedArgs.source == nil {
// Piped patches get a best-effort repo root from the CLI's cwd so
// diff comments can persist per repository.
diffSourceContext.repoRoot = try? gitRepoRoot(
startingAt: FileManager.default.currentDirectoryPath
)
sourceContext.repoRoot = diffSourceContext.repoRoot
sourceContext.branchBaseRef = diffSourceContext.branchBaseRef
diffSourceContext = sourceContext
workspaceHandle = sourceContext.workspaceId ?? workspaceHandle
surfaceHandle = sourceContext.surfaceId ?? surfaceHandle
}
// Resolve the opener workspace/surface before writing the viewer HTML
// (for piped patches too), so the payload's commentTarget always points
// at the terminal the diff was opened from.
try resolveTargetIfNeeded()
var sourceContext = try canonicalDiffSourceContext(
workspaceHandle: workspaceHandle,
surfaceHandle: surfaceHandle,
windowHandle: windowHandle,
client: try connectedClient()
)
sourceContext.repoRoot = diffSourceContext.repoRoot
sourceContext.branchBaseRef = diffSourceContext.branchBaseRef
diffSourceContext = sourceContext
workspaceHandle = sourceContext.workspaceId ?? workspaceHandle
surfaceHandle = sourceContext.surfaceId ?? surfaceHandle

let appearance = diffViewerAppearance(
socketPath: socketPath,
Expand Down Expand Up @@ -3086,6 +3109,9 @@ extension CMUXCLI {
layoutSource: layoutSource,
appearance: appearance,
sourceOptions: [],
repoRoot: context.repoRoot,
commentTargetWorkspaceId: context.workspaceId,
commentTargetSurfaceId: context.surfaceId,
runtime: runtime
)
let assets = try ensureDiffViewerAssets(nextTo: viewerFileURL, runtime: runtime)
Expand Down Expand Up @@ -3668,6 +3694,8 @@ extension CMUXCLI {
baseOptions: selectedSource == .branch ? baseOptions : [],
repoRoot: repoRoot,
branchBaseRef: selectedSource == .branch ? selectedContext.branchBaseRef : nil,
commentTargetWorkspaceId: selectedContext.workspaceId,
commentTargetSurfaceId: selectedContext.surfaceId,
runtime: target.runtime
)
} else if let selectedEmptyMessage {
Expand Down Expand Up @@ -3973,6 +4001,8 @@ extension CMUXCLI {
baseOptions: baseOptions,
repoRoot: pageContext.repoRoot,
branchBaseRef: source == .branch ? pageContext.branchBaseRef : nil,
commentTargetWorkspaceId: pageContext.workspaceId,
commentTargetSurfaceId: pageContext.surfaceId,
runtime: sourceSet.runtime
)
return DiffViewerDeferredCompletion(
Expand Down Expand Up @@ -5593,6 +5623,8 @@ extension CMUXCLI {
baseOptions: [DiffViewerSourceOption] = [],
repoRoot: String? = nil,
branchBaseRef: String? = nil,
commentTargetWorkspaceId: String? = nil,
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved
commentTargetSurfaceId: String? = nil,
statusMessage: String? = nil,
statusIsError: Bool = false,
pollForReplacement: Bool = false,
Expand Down Expand Up @@ -5632,6 +5664,16 @@ extension CMUXCLI {
if let branchBaseRef {
payload["branchBaseRef"] = branchBaseRef
}
if commentTargetWorkspaceId != nil || commentTargetSurfaceId != nil {
var commentTarget: [String: Any] = [:]
if let commentTargetWorkspaceId {
commentTarget["workspaceId"] = commentTargetWorkspaceId
}
if let commentTargetSurfaceId {
commentTarget["surfaceId"] = commentTargetSurfaceId
}
payload["commentTarget"] = commentTarget
}
let assets = try ensureDiffViewerAssets(nextTo: viewerURL, runtime: runtime)
let config: [String: Any] = [
"payload": payload,
Expand Down Expand Up @@ -5801,12 +5843,29 @@ extension CMUXCLI {
if FileManager.default.fileExists(atPath: appDirectory.path, isDirectory: &isDirectory),
isDirectory.boolValue,
FileManager.default.fileExists(atPath: entry.path) {
return (sourceDirectory: appDirectory, targetDirectoryName: candidate.targetName)
// The shared /tmp asset cache is written by every running cmux
// build (stable, nightly, each tagged dev app). Content-key the
// directory so builds with different webview bundles coexist
// instead of clobbering each other's chunks, which broke pages
// whose per-token allowlist no longer matched the files on disk.
let targetName = "\(candidate.targetName)-\(try diffViewerAppAssetContentKey(directory: appDirectory))"
return (sourceDirectory: appDirectory, targetDirectoryName: targetName)
Comment on lines +5821 to +5827

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prune stale hashed asset directories as part of diff-viewer cleanup.

This change starts creating a new assets/cmux-webviews-app-<hash> directory for each bundle revision, but pruneDiffViewerFiles(in:) only deletes stale HTML, patch, and manifest files. Old hashed asset trees are never reclaimed, so switching between tagged builds/nightlies will accumulate full copied app bundles under /tmp/cmux-diff-viewer-<uid>/assets until manual cleanup or reboot.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CLI/cmux_open.swift` around lines 5846 - 5852, pruneDiffViewerFiles(in:)
currently only removes stale HTML/patch/manifest files but not the per-bundle
asset directories created by using diffViewerAppAssetContentKey(directory:), so
add directory pruning logic: in pruneDiffViewerFiles(in:) scan the assets
directory for subdirectories named with the cmux-webviews-app-<hash> prefix (or
otherwise matching the targetName pattern used when building targetName), and
remove any directories whose content key/hash does not match the currently
active one (or are older than retention policy); ensure you reference and use
the same naming convention as diffViewerAppAssetContentKey(directory:) and the
targetName variable when deciding which directories are stale so full copied app
bundles are reclaimed during diff-viewer cleanup.

}
}
throw CLIError(message: "Bundled cmux diff viewer app assets not found")
}

private func diffViewerAppAssetContentKey(directory: URL) throws -> String {
var hasher = SHA256()
for relativePath in try diffViewerBundledAssetRelativePaths(in: directory).sorted() {
hasher.update(data: Data(relativePath.utf8))
let fileURL = directory.appendingPathComponent(relativePath, isDirectory: false)
hasher.update(data: try Data(contentsOf: fileURL, options: .mappedIfSafe))
}
let digest = hasher.finalize()
return digest.map { String(format: "%02x", $0) }.joined().prefix(12).lowercased()
Comment on lines +5833 to +5841

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Memoize the app-bundle hash instead of rehashing on every page write.

diffViewerAppAssetContentKey(directory:) re-enumerates and re-reads the entire webview bundle each time ensureDiffViewerAssets runs. writeCompleteGitDiffViewerHTMLSet can emit many pages per invocation, so one diff render now multiplies that full-bundle I/O by page count for a value that is constant within the process. Cache the hash per source directory (or per mtime snapshot) and reuse it. As per coding guidelines, .github/review-bot-rules/algorithmic-complexity.md asks us to flag repeated full-collection scans and repeated work in hot paths.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CLI/cmux_open.swift` around lines 5858 - 5866,
diffViewerAppAssetContentKey(directory:) currently re-reads and rehashes the
entire bundle on every call; modify it to memoize the computed hash per source
directory (keyed by directory URL or by directory path plus mtime snapshot) and
return the cached value on subsequent calls to avoid repeated I/O; update or add
a shared cache (e.g., a static [URL: (hash:String, mtime:Date)] dictionary) used
by diffViewerAppAssetContentKey and invalidate/update the entry when the
directory mtime changes; ensure callers like ensureDiffViewerAssets and
writeCompleteGitDiffViewerHTMLSet continue to call diffViewerAppAssetContentKey
unchanged so they benefit from the cache.

Source: Coding guidelines

}

private func copyDiffViewerAsset(relativePath: String, from sourceDirectory: URL, to targetDirectory: URL) throws {
let fileManager = FileManager.default
let sourceURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false)
Expand Down
Loading
Loading