Repository navigation
Diff viewer: review comments with per-repo persistence and TextBox attach - #5768
Conversation
Per-repo DiffCommentStore under Application Support, a cmuxDiffComments WKScriptMessageHandlerWithReply gated to registered diff viewer pages, TerminalPanel.insertTextBoxAttachments with a pending queue that never steals focus, and a commentTarget (opener workspace/surface) embedded in the diff viewer payload by the CLI for opener-first attach targeting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pierre-style line annotations: gutter utility opens a draft composer, saved comments render inline and in a sidebar list, comments re-anchor by line text across regenerated diffs (outdated ones stay sidebar-only), and saving auto-attaches to the opener terminal's TextBox with an opt-out toggle and an in-page terminal picker for ambiguous targets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 end-to-end diff comment support: CLI comment-target and asset-key wiring, a persistent DiffCommentStore, a WK bridge for list/save/delete/attach with trusted-session checks, terminal attach buffering and non-focusing insertion, webview types/bridge client/anchoring/formatting, React UI/components/styles, and unit tests. ChangesDiff Comments System for Diff Viewer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: a2982d18c8
ℹ️ 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".
| ) else { | ||
| return false | ||
| } | ||
| return CmuxDiffViewerURLSchemeHandler.shared.hasActiveSession(token: components.token) |
There was a problem hiding this comment.
When a diff viewer is served from the local HTTP URL but its manifest contains a streamed remote patch (remote_url with an empty file_path, the remote PR-diff case called out in BrowserPanel.localManifestFiles), there is no custom-scheme registration from browser.open_split, and registerFromManifest refuses that manifest. This trust check therefore returns false for an otherwise valid diff viewer frame, so every comments.list/comments.save bridge call gets not_allowed and the new comments UI is unusable for remote-streamed diffs.
Useful? React with 👍 / 👎.
Greptile SummaryAdds Pierre/DiffHub-style review comments to the diff viewer: gutter composer, inline annotations, sidebar, per-repo JSON persistence (
Confidence Score: 4/5Safe to merge once the open items from prior review threads are resolved; the new code in this pass is well-structured and the trust gate is sound. The prior review threads surfaced two substantive concerns that remain open: synchronous disk I/O in DiffCommentStore on the main thread (every save/load blocks the main runloop), and a double-click race in saveDraft that can create duplicate comments. Until those are addressed the feature has known correctness and responsiveness gaps on the save path. Sources/DiffCommentStore.swift (main-thread disk I/O) and webviews/src/App.tsx / webviews/src/comments/CommentComposer.tsx (saveDraft in-flight guard) are the files most worth re-examining before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as Webview (JS)
participant B as DiffCommentsBridge (Swift)
participant S as DiffCommentStore
participant P as DiffCommentSubmissionPool
participant T as TextBoxInputContainer
W->>B: comments.list(repoRoot)
B->>S: comments(repoRoot:)
S-->>B: [DiffComment]
B->>P: setPending(entry, workspaceId) for each unconsumed
B-->>W: "{ comments: [...] }"
W->>B: comments.save(repoRoot, comment)
B->>S: upsert(comment, repoRoot:)
S-->>B: DiffComment (saved)
B->>P: setPending(entry, workspaceId)
B-->>W: "{ comment: saved }"
Note over T: chip shows pending count
T->>P: consumeAll(workspaceId)
P-->>T: [Entry]
T->>T: append bundle to partsToSend
T->>T: TextBoxSubmit.send(partsToSend)
alt submit succeeded
T->>S: markConsumed(ids, repoRoot)
else submit failed
T->>P: restorePending(entries, workspaceId)
end
W->>B: comments.delete(repoRoot, id)
B->>P: removePending(commentId)
B->>S: delete(id, repoRoot)
B-->>W: "{ deleted: true }"
Reviews (12): Last reviewed commit: "Resolve DiffCommentStore.shared default ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }, | ||
| "diffViewer.addComment": { | ||
| "extractionState": "manual", | ||
| "localizations": { |
There was a problem hiding this comment.
All 14 new diffViewer.* comment UI strings and diffComments.bridge.notAllowed are added for 18 locales (ar, bs, da, de, en, es, fr, it, ja, km, ko, nb, pl, pt-BR, ru, th, tr, uk). Only en and ja have real translations; the remaining 16 locales all carry the English source string as the "translated" value — users in Arabic, German, Spanish, French, Korean, etc. will see English text in the comment UI.
The full-internationalization rule requires catalog additions to include translated entries for every locale already supported by the touched catalog. The 16 non-Japanese, non-English locales need actual translations (or "state": "needs_review" placeholders the translation toolchain can pick up) before this merges.
Rule Used: Flag production user-facing text that is not fully... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/DiffCommentStoreTests.swift`:
- Around line 155-172: Add a new unit test function named
testMultipleActiveTextBoxesWithOpenerGoneShowsPicker that calls
DiffCommentsBridge.resolveAttachTarget with a non-nil requested UUID that is not
present in the candidates list and with two candidates created via
candidate(UUID(), textBox: true); assert the result matches the .picker case
(guard case .picker(let candidates) = resolution) and
XCTAssertEqual(candidates.count, 2) to verify a picker is returned when the
requested opener is gone but multiple active text boxes exist.
In `@Sources/DiffCommentStore.swift`:
- Around line 47-58: The upsert/delete flow currently calls saveFile(...) which
updates cacheByRepoKey and swallows disk errors, causing in-memory success when
persistence failed; modify saveFile to propagate errors (make it throwable) or,
if keeping non-throwing API, detect write failures and roll back cacheByRepoKey
before returning; update callers (upsert(_:repoRoot:), delete(...), and any
other uses around lines noted) to handle the throwable saveFile
(propagate/return failure) or to check save result and revert the cached File
state so callers only report success when the disk write actually succeeded.
- Around line 37-57: The cache (cacheByRepoKey) can be stale and upsert(_:,
repoRoot:) (and the delete path) currently mutates the in-memory
RepoCommentsFile without re-reading the on-disk JSON, risking overwriting newer
changes from other processes; update upsert(_:, repoRoot:) and the delete
handler to reload the authoritative file from disk via loadFile(repoRoot:) or a
new loadFromDisk(repoRoot:) just before merging, perform an id-based merge of
comments (preserving existing createdAt for matched ids and replacing/adding
others), resolve conflicts deterministically (e.g., last-write-wins or reject on
mtime/version), and only then call saveFile(file, repoRoot:) so you never write
from a stale cached snapshot.
In `@Sources/Panels/BrowserPanel.swift`:
- Line 4219: DiffCommentsBridge.associate(panelId: id, workspaceId: workspaceId,
with: webView) is only invoked when the web view is first bound, so later
workspace changes (via updateWorkspaceId(_:) or the no-store-swap path in
reattachToWorkspace(...)) leave the bridge pointing at the old workspace; ensure
you refresh the bridge metadata by calling DiffCommentsBridge.associate with the
current panel id, the new workspaceId, and the webView whenever the workspace
changes—add that call into updateWorkspaceId(_:) and into the
reattachToWorkspace(...) no-store-swap path so comment attach resolution uses
the updated workspace context.
In `@Sources/Panels/DiffCommentsBridge.swift`:
- Around line 296-317: The comment(fromJSON:) function currently coerces unknown
anchor metadata and invalid ranges; change it to validate inputs strictly by
returning nil on bad data: verify "side" is exactly "additions" or "deletions"
(do not default unknowns to "additions"), if "endSide" is present ensure it is
also exactly "additions" or "deletions" (otherwise treat as nil/reject), and
validate that startLine and endLine are positive and endLine >= startLine
(reject otherwise) before constructing the DiffComment so upsert receives only
supported anchor values and valid ranges.
- Around line 124-150: Replace hard-coded English messages passed to
BridgeError.invalidRequest with localized strings using the same localization
API used for .notAllowed (e.g., NSLocalizedString or a project wrapper) so
userMessage is localized; update all occurrences in DiffCommentsBridge.swift
where BridgeError.invalidRequest(...) is thrown (including the blocks handling
repoRoot, comments.save malformed comment, comments.delete missing id,
comments.attach failures and the default unsupported method branch, and the
similar strings referenced around lines 169-204) to pass a localized key and
comment instead of raw English text, ensuring keys are added to the string
catalog for each message.
- Around line 121-150: The handler currently trusts params["repoRoot"]; instead,
resolve the repoRoot from the authorized diff-viewer session associated with the
incoming webView/frame and use that canonical root for all operations
(comments.list, comments.save, comments.delete, comments.attach). In practice,
inside handle(body:webView:) replace the params-derived repoRoot check with a
lookup of the registered viewer session for the provided webView (or frame),
obtain its canonical repo root, and if params["repoRoot"] exists and does not
equal the session root, throw BridgeError.invalidRequest("repoRoot mismatch");
then pass the session-derived repoRoot into store.comments(...),
store.upsert(...), store.delete(...), and into handleAttach(params:webView:) so
all persistence is scoped to the authorized session. Ensure unsupported or
missing session lookups also throw BridgeError.invalidRequest.
🪄 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: 20429d75-6615-4e27-ab28-d6ee2d42cfd5
📒 Files selected for processing (26)
CLI/cmux_open.swiftResources/Localizable.xcstringsResources/markdown-viewer/webviews-app/chunks/agentSessionSurface.mjsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsResources/markdown-viewer/webviews-app/chunks/installWebviewStyles.mjsSources/DiffCommentStore.swiftSources/Panels/BrowserPanel.swiftSources/Panels/DiffCommentsBridge.swiftSources/Panels/TerminalPanel.swiftSources/TextBoxInput.swiftcmux.xcodeproj/project.pbxprojcmuxTests/DiffCommentStoreTests.swiftwebviews/src/App.tsxwebviews/src/comments/CommentComposer.tsxwebviews/src/comments/CommentsSection.tsxwebviews/src/comments/SavedComment.tsxwebviews/src/comments/anchor.tswebviews/src/comments/annotations.tswebviews/src/comments/bridge.tswebviews/src/comments/format.tswebviews/src/comments/labels.tswebviews/src/comments/types.tswebviews/src/comments/useCommentsBootstrap.tswebviews/src/diff-stream.tswebviews/src/styles.csswebviews/test/comments.test.ts
| func testAmbiguousTerminalsBecomePicker() { | ||
| let resolution = DiffCommentsBridge.resolveAttachTarget( | ||
| requested: nil, | ||
| candidates: [candidate(UUID(), textBox: true), candidate(UUID(), textBox: true)] | ||
| ) | ||
| guard case .picker(let candidates) = resolution else { | ||
| return XCTFail("Expected picker, got \(resolution)") | ||
| } | ||
| XCTAssertEqual(candidates.count, 2) | ||
| } | ||
|
|
||
| func testNoTerminalsIsUnavailable() { | ||
| let resolution = DiffCommentsBridge.resolveAttachTarget(requested: nil, candidates: []) | ||
| guard case .unavailable = resolution else { | ||
| return XCTFail("Expected unavailable, got \(resolution)") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: Consider adding edge-case test for requested opener gone with multiple active text boxes.
The current tests cover the case where the requested opener is gone and a single active text box wins (line 131), and where no opener is requested (nil) with multiple terminals (line 155). An additional edge case would be: requested opener is gone (not nil, but not in candidates) AND multiple active text boxes exist. This should likely resolve to a picker, similar to the nil-opener case.
However, the core attach resolution paths are well-covered, so this is a nice-to-have rather than critical.
Example edge-case test
func testMultipleActiveTextBoxesWithOpenerGoneShowsPicker() {
let resolution = DiffCommentsBridge.resolveAttachTarget(
requested: UUID(), // Requested opener not in candidates
candidates: [candidate(UUID(), textBox: true), candidate(UUID(), textBox: true)]
)
guard case .picker(let candidates) = resolution else {
return XCTFail("Expected picker, got \(resolution)")
}
XCTAssertEqual(candidates.count, 2)
}🤖 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/DiffCommentStoreTests.swift` around lines 155 - 172, Add a new unit
test function named testMultipleActiveTextBoxesWithOpenerGoneShowsPicker that
calls DiffCommentsBridge.resolveAttachTarget with a non-nil requested UUID that
is not present in the candidates list and with two candidates created via
candidate(UUID(), textBox: true); assert the result matches the .picker case
(guard case .picker(let candidates) = resolution) and
XCTAssertEqual(candidates.count, 2) to verify a picker is returned when the
requested opener is gone but multiple active text boxes exist.
| private var cacheByRepoKey: [String: RepoCommentsFile] = [:] | ||
|
|
||
| init(directoryURL: URL? = DiffCommentStore.defaultDirectoryURL()) { | ||
| self.directoryURL = directoryURL | ||
| } | ||
|
|
||
| func comments(repoRoot: String) -> [DiffComment] { | ||
| loadFile(repoRoot: repoRoot).comments | ||
| } | ||
|
|
||
| @discardableResult | ||
| func upsert(_ comment: DiffComment, repoRoot: String) -> DiffComment { | ||
| var file = loadFile(repoRoot: repoRoot) | ||
| var stored = comment | ||
| if let index = file.comments.firstIndex(where: { $0.id == comment.id }) { | ||
| stored.createdAt = file.comments[index].createdAt | ||
| file.comments[index] = stored | ||
| } else { | ||
| file.comments.append(stored) | ||
| } | ||
| saveFile(file, repoRoot: repoRoot) |
There was a problem hiding this comment.
Reload or merge the on-disk JSON before mutating cached comments.
After the first loadFile(...), cacheByRepoKey becomes the source of truth for that repo. A later upsert/delete in this process will therefore overwrite newer comments written by another cmux process/session from a stale in-memory snapshot, which is data loss on a persisted history path. Re-read and merge by id (or reject on version/mtime mismatch) before writing.
As per coding guidelines, persistence/history paths that substitute cached values for authoritative reads must handle cold and stale caches explicitly.
Also applies to: 71-89
🤖 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 `@Sources/DiffCommentStore.swift` around lines 37 - 57, The cache
(cacheByRepoKey) can be stale and upsert(_:, repoRoot:) (and the delete path)
currently mutates the in-memory RepoCommentsFile without re-reading the on-disk
JSON, risking overwriting newer changes from other processes; update upsert(_:,
repoRoot:) and the delete handler to reload the authoritative file from disk via
loadFile(repoRoot:) or a new loadFromDisk(repoRoot:) just before merging,
perform an id-based merge of comments (preserving existing createdAt for matched
ids and replacing/adding others), resolve conflicts deterministically (e.g.,
last-write-wins or reject on mtime/version), and only then call saveFile(file,
repoRoot:) so you never write from a stale cached snapshot.
Source: Coding guidelines
| @discardableResult | ||
| func upsert(_ comment: DiffComment, repoRoot: String) -> DiffComment { | ||
| var file = loadFile(repoRoot: repoRoot) | ||
| var stored = comment | ||
| if let index = file.comments.firstIndex(where: { $0.id == comment.id }) { | ||
| stored.createdAt = file.comments[index].createdAt | ||
| file.comments[index] = stored | ||
| } else { | ||
| file.comments.append(stored) | ||
| } | ||
| saveFile(file, repoRoot: repoRoot) | ||
| return stored |
There was a problem hiding this comment.
Do not return success when the disk write failed.
saveFile(...) updates cacheByRepoKey before encoding/writing and then swallows any error. That means upsert(...) and delete(...) can report success to the bridge even though nothing was persisted, so comments appear saved for the session and disappear on restart. Please make the write path throwable or roll back the cache when persistence fails.
Also applies to: 61-68, 87-102
🤖 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 `@Sources/DiffCommentStore.swift` around lines 47 - 58, The upsert/delete flow
currently calls saveFile(...) which updates cacheByRepoKey and swallows disk
errors, causing in-memory success when persistence failed; modify saveFile to
propagate errors (make it throwable) or, if keeping non-throwing API, detect
write failures and roll back cacheByRepoKey before returning; update callers
(upsert(_:repoRoot:), delete(...), and any other uses around lines noted) to
handle the throwable saveFile (propagate/return failure) or to check save result
and revert the cached File state so callers only report success when the disk
write actually succeeded.
| } | ||
|
|
||
| private func bindWebView(_ webView: CmuxWebView) { | ||
| DiffCommentsBridge.associate(panelId: id, workspaceId: workspaceId, with: webView) |
There was a problem hiding this comment.
Refresh the diff-comments bridge metadata on workspace moves.
Line 4219 only captures workspaceId when the web view is first bound. updateWorkspaceId(_:) and the no-store-swap path in reattachToWorkspace(...) can change the panel’s workspace later without rebinding, so comment attach resolution can keep using the old workspace and target the wrong terminal after a move.
Suggested fix
func updateWorkspaceId(_ newWorkspaceId: UUID) {
workspaceId = newWorkspaceId
+ DiffCommentsBridge.associate(panelId: id, workspaceId: newWorkspaceId, with: webView)
}
func reattachToWorkspace(
_ newWorkspaceId: UUID,
isRemoteWorkspace: Bool,
@@
) {
workspaceId = newWorkspaceId
+ DiffCommentsBridge.associate(panelId: id, workspaceId: newWorkspaceId, with: webView)
usesRemoteWorkspaceProxy = isRemoteWorkspace && !bypassesRemoteWorkspaceProxy
let targetStore = isRemoteWorkspace
? WKWebsiteDataStore(forIdentifier: remoteWebsiteDataStoreIdentifier ?? newWorkspaceId)
: BrowserProfileStore.shared.websiteDataStore(for: profileID)Based on the PR objective, attach fallback is scoped to the diff viewer workspace.
🤖 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 `@Sources/Panels/BrowserPanel.swift` at line 4219,
DiffCommentsBridge.associate(panelId: id, workspaceId: workspaceId, with:
webView) is only invoked when the web view is first bound, so later workspace
changes (via updateWorkspaceId(_:) or the no-store-swap path in
reattachToWorkspace(...)) leave the bridge pointing at the old workspace; ensure
you refresh the bridge metadata by calling DiffCommentsBridge.associate with the
current panel id, the new workspaceId, and the webView whenever the workspace
changes—add that call into updateWorkspaceId(_:) and into the
reattachToWorkspace(...) no-store-swap path so comment attach resolution uses
the updated workspace context.
| throw BridgeError.invalidRequest("Malformed bridge request") | ||
| } | ||
| let params = body["params"] as? [String: Any] ?? [:] | ||
| guard let repoRoot = (params["repoRoot"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !repoRoot.isEmpty else { | ||
| throw BridgeError.invalidRequest("Missing repoRoot") | ||
| } | ||
|
|
||
| switch method { | ||
| case "comments.list": | ||
| return ["comments": store.comments(repoRoot: repoRoot).map(Self.commentJSON)] | ||
| case "comments.save": | ||
| guard let commentParams = params["comment"] as? [String: Any], | ||
| let comment = Self.comment(fromJSON: commentParams) else { | ||
| throw BridgeError.invalidRequest("Malformed comment") | ||
| } | ||
| let saved = store.upsert(comment, repoRoot: repoRoot) | ||
| return ["comment": Self.commentJSON(saved)] | ||
| case "comments.delete": | ||
| guard let rawId = params["id"] as? String, let id = UUID(uuidString: rawId) else { | ||
| throw BridgeError.invalidRequest("Missing comment id") | ||
| } | ||
| return ["deleted": store.delete(id: id, repoRoot: repoRoot)] | ||
| case "comments.attach": | ||
| return try handleAttach(params: params, webView: webView) | ||
| default: | ||
| throw BridgeError.invalidRequest("Unsupported method '\(method)'") |
There was a problem hiding this comment.
Localize the invalidRequest reply messages.
These detail strings are returned as error.userMessage, so failures like “Missing repoRoot”, “Malformed attachment”, and “Terminal no longer exists” will surface in English even when the app is running in Japanese. Route them through localization keys the same way .notAllowed already does.
As per coding guidelines, "All user-facing strings must be localized" and Swift text must use localized APIs backed by translated string-catalog entries for every supported locale.
Also applies to: 169-204
🤖 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 `@Sources/Panels/DiffCommentsBridge.swift` around lines 124 - 150, Replace
hard-coded English messages passed to BridgeError.invalidRequest with localized
strings using the same localization API used for .notAllowed (e.g.,
NSLocalizedString or a project wrapper) so userMessage is localized; update all
occurrences in DiffCommentsBridge.swift where BridgeError.invalidRequest(...) is
thrown (including the blocks handling repoRoot, comments.save malformed comment,
comments.delete missing id, comments.attach failures and the default unsupported
method branch, and the similar strings referenced around lines 169-204) to pass
a localized key and comment instead of raw English text, ensuring keys are added
to the string catalog for each message.
Source: Coding guidelines
| nonisolated private static func comment(fromJSON json: [String: Any]) -> DiffComment? { | ||
| guard let filePath = json["filePath"] as? String, !filePath.isEmpty, | ||
| let side = json["side"] as? String, | ||
| let startLine = json["startLine"] as? Int, | ||
| let endLine = json["endLine"] as? Int, | ||
| let message = json["message"] as? String else { | ||
| return nil | ||
| } | ||
| let id = (json["id"] as? String).flatMap(UUID.init(uuidString:)) ?? UUID() | ||
| let now = Date() | ||
| return DiffComment( | ||
| id: id, | ||
| filePath: filePath, | ||
| side: side == "deletions" ? "deletions" : "additions", | ||
| startLine: startLine, | ||
| endLine: endLine, | ||
| endSide: json["endSide"] as? String, | ||
| lineText: json["lineText"] as? String ?? "", | ||
| message: message, | ||
| createdAt: now, | ||
| updatedAt: now | ||
| ) |
There was a problem hiding this comment.
Reject unsupported anchor metadata instead of coercing it.
Line 309 maps any unknown side to "additions", Line 312 accepts any endSide, and this path never checks that the line range is valid. If the web client ever sends a stale enum value or impossible range, the bridge will persist a comment against the wrong anchor instead of failing fast. Validate side/endSide against the supported set and reject non-positive or inverted ranges before calling upsert.
🤖 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 `@Sources/Panels/DiffCommentsBridge.swift` around lines 296 - 317, The
comment(fromJSON:) function currently coerces unknown anchor metadata and
invalid ranges; change it to validate inputs strictly by returning nil on bad
data: verify "side" is exactly "additions" or "deletions" (do not default
unknowns to "additions"), if "endSide" is present ensure it is also exactly
"additions" or "deletions" (otherwise treat as nil/reject), and validate that
startLine and endLine are positive and endLine >= startLine (reject otherwise)
before constructing the DiffComment so upsert receives only supported anchor
values and valid ranges.
Every running cmux build wrote its webview bundle to the same /tmp/cmux-diff-viewer-<uid>/assets/cmux-webviews-app directory, so two concurrent builds with different bundles clobbered each other's chunks and broke pages whose per-token allowlist no longer matched the files on disk (blank diff pane, 'Importing a module script failed'). The target directory is now cmux-webviews-app-<sha256(bundle)> so distinct bundles coexist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954ec0de8c
ℹ️ 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 targetParams = params["target"] as? [String: Any] | ||
| let requestedSurfaceId = (targetParams?["surfaceId"] as? String).flatMap(UUID.init(uuidString:)) | ||
| let candidates = Self.attachCandidates(in: workspace) |
There was a problem hiding this comment.
When the diff viewer has been moved/restored into a different workspace, the saved commentTarget can still identify the opener via both workspaceId and surfaceId, but the bridge only reads surfaceId and builds candidates from the workspace that currently contains the browser panel. In that case the requested opener surface is filtered out even though it still exists, so auto-attach falls through to the wrong terminal/picker instead of the opener-first behavior described by the payload.
Useful? React with 👍 / 👎.
| pendingProgrammaticTextBoxAttachments.append(contentsOf: attachments) | ||
| // Keep the published mirror truthful so session snapshots taken before | ||
| // the view mounts still include the queued attachments. | ||
| textBoxAttachments.append(contentsOf: attachments) |
There was a problem hiding this comment.
In the branch where a TextBox view exists but is not in a window, the attachment is only queued and mirrored into textBoxAttachments; however sessionTextBoxDraftSnapshot() still returns textBoxInputView.sessionDraftSnapshot(...) whenever textBoxInputView is non-nil, so it ignores this mirror. If a session snapshot is taken before the view moves to a window, the queued diff-comment attachment is lost on restore despite this path intending to make snapshots include it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux_open.swift (1)
850-857:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the repo-root lookup to git-backed diffs.
Line 850 now throws whenever
--cwd/--repopoints outside a git repo, even for patch/stdin/URL inputs. That regresses commands likecmux diff some.patch --repo /tmp: the repo root here is only needed for git sources or best-effort comment persistence, but this path now hard-fails before the patch is opened.Suggested fix
- if let cwd = parsedArgs.cwd { - diffSourceContext.repoRoot = try gitRepoRoot(startingAt: resolvePath(cwd)) - } else if parsedArgs.source == nil { + if let cwd = parsedArgs.cwd, parsedArgs.source != nil { + diffSourceContext.repoRoot = try gitRepoRoot(startingAt: resolvePath(cwd)) + } 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 + startingAt: parsedArgs.cwd.map(resolvePath) ?? FileManager.default.currentDirectoryPath ) }🤖 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 850 - 857, When computing diffSourceContext.repoRoot from parsedArgs.cwd using gitRepoRoot(startingAt: resolvePath(cwd)), don't let a missing/non-git repo throw for non-git-backed inputs; change the lookup to a non-throwing best-effort call (use try? instead of try) so parsedArgs.cwd is resolved but failure to find a git repo yields nil instead of crashing; update the assignment to diffSourceContext.repoRoot = try? gitRepoRoot(startingAt: resolvePath(cwd)) so behavior matches the piped/URL branch which already uses try?.
🤖 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 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.
- Around line 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.
---
Outside diff comments:
In `@CLI/cmux_open.swift`:
- Around line 850-857: When computing diffSourceContext.repoRoot from
parsedArgs.cwd using gitRepoRoot(startingAt: resolvePath(cwd)), don't let a
missing/non-git repo throw for non-git-backed inputs; change the lookup to a
non-throwing best-effort call (use try? instead of try) so parsedArgs.cwd is
resolved but failure to find a git repo yields nil instead of crashing; update
the assignment to diffSourceContext.repoRoot = try? gitRepoRoot(startingAt:
resolvePath(cwd)) so behavior matches the piped/URL branch which already uses
try?.
🪄 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: e3eb0c84-550e-4c29-8e6d-4ae1121abe30
📒 Files selected for processing (1)
CLI/cmux_open.swift
| // 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) |
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
🧹 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
Live diff viewer pages carry '#/cmux-diff-viewer' (the in-page router rewrites the original '#cmux-diff-viewer' fragment), so the bridge rejected every comments call with not_allowed: saving did nothing and attach never ran. Token extraction is now fragment-insensitive for loopback page URLs; the registered-token check stays the trust authority. Adds parser regression tests and a Pierre-style polish pass (blue rounded gutter + button, cleaner comment cards). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 710e45554c
ℹ️ 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".
| const lineRef = comment.endLine > comment.startLine | ||
| ? `lines ${comment.startLine}-${comment.endLine}` | ||
| : `line ${comment.startLine}`; | ||
| const version = comment.side === "deletions" ? "old" : "new"; | ||
| const excerpt = excerptFor(fileDiff, comment.side, comment.startLine, comment.endLine); | ||
| const sections = [`Review comment on ${comment.filePath} ${lineRef} (${version} version):`]; |
There was a problem hiding this comment.
When a non-English user attaches a saved comment to a terminal TextBox, the inserted attachment text is always English (Review comment on ..., line(s), old/new version) because this formatter builds the user-facing text from hard-coded literals instead of the localized label payload/strings. The repo instructions require user-facing strings to be localized, so the new attach flow is partially untranslated in Japanese and any other supported locale.
Useful? React with 👍 / 👎.
…tdated badge, Pierre UI pass A closed-TextBox opener no longer silently wins over terminals with visibly open TextBoxes (picker instead, open-TextBox candidates listed first). Sidebar comments whose file has not streamed in yet show as loading instead of flashing an Outdated badge before converging. Second Pierre styling pass: borderless composer field with a card-level focus ring, hover-revealed card actions, softer elevation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed02a990d5
ℹ️ 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 resolution = Self.resolveAttachTarget( | ||
| requested: requestedSurfaceId, | ||
| candidates: candidates | ||
| ) |
There was a problem hiding this comment.
When the attach flow returns a picker and the user clicks a specific terminal, the webview sends explicit: true, but this bridge ignores that flag and runs the same opener-first heuristic again. If the selected terminal does not currently have an active TextBox while any other candidate does, resolveAttachTarget returns .picker(candidates) again, so that picker item can never be chosen even though the user explicitly selected it.
Useful? React with 👍 / 👎.
The composer's bare attach checkbox becomes an 'Attach to' dropdown listing every terminal in the workspace (open-TextBox terminals first, opener-first default, titles disambiguated by directory and ordinal) plus a Don't attach option, backed by a new comments.targets bridge method. Lets the user aim a comment at any terminal when several TextBoxes are open instead of relying on the opener heuristic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 098bc33aeb
ℹ️ 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".
| return; | ||
| } | ||
| const input = { | ||
| filePath: fileName(item.fileDiff, ""), |
There was a problem hiding this comment.
When a streamed patch contains multiple commits that touch the same file, appendFileDiffToModel gives those entries distinct tree paths by prefixing the commit label, but the saved comment anchor keeps only fileName(item.fileDiff, ""). The annotation/sidebar code later matches comments by this filePath, so a comment left on src/foo.ts in one commit can render under every other src/foo.ts entry (or jump to the first one) in the same multi-commit diff. Persist enough of the item/tree path to distinguish prefixed entries before saving.
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)
webviews/src/comments/CommentComposer.tsx (1)
1-91: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider memoizing
targetLabelscomputation.The
attachTargetOptionLabels(candidates)call on line 38 runs on every render. For typical use (few terminals) this is fine, but if a workspace has many terminals, the three-pass disambiguation logic could become a hot path. Consider wrapping it inuseMemo([candidates], ...)to avoid recomputing unchanged candidate lists.🤖 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 `@webviews/src/comments/CommentComposer.tsx` around lines 1 - 91, The computation of targetLabels via attachTargetOptionLabels(candidates) is run on every render and should be memoized to avoid repeated work; wrap the call in a useMemo that depends on candidates (e.g., const targetLabels = useMemo(() => attachTargetOptionLabels(candidates), [candidates])) so attachTargetOptionLabels, targetLabels and candidates are only recomputed when candidates changes.CLI/cmux_open.swift (2)
852-860:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't key every non-git diff to the caller's cwd repo.
Lines 852-860 now assign
repoRootfromFileManager.default.currentDirectoryPathfor every non-source input. That misfiles comments for remote PR URLs and patch files outside the cwd repo under the wrong repository hash, so persisted comments can bleed into unrelated repos later. Restrict the cwd fallback to stdin/piped patches, and derive local patch files from the patch file’s directory instead.Suggested fix
- if let cwd = parsedArgs.cwd { - diffSourceContext.repoRoot = try gitRepoRoot(startingAt: resolvePath(cwd)) - } 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 - ) - } + if let cwd = parsedArgs.cwd { + diffSourceContext.repoRoot = try gitRepoRoot(startingAt: resolvePath(cwd)) + } else if parsedArgs.source == nil { + let rawInput = parsedArgs.inputs.first + if rawInput == nil || rawInput == "-" { + diffSourceContext.repoRoot = try? gitRepoRoot( + startingAt: FileManager.default.currentDirectoryPath + ) + } else if let rawInput, + diffInputPatchURL(rawInput) == nil, + diffInputTrustedRemotePatchURL(rawInput) == nil { + let patchPath = resolvePath(rawInput) + diffSourceContext.repoRoot = try? gitRepoRoot( + startingAt: URL(fileURLWithPath: patchPath).deletingLastPathComponent().path + ) + } + }🤖 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 852 - 860, The current logic sets diffSourceContext.repoRoot to FileManager.default.currentDirectoryPath for any non-source input, which misattributes remote PR URLs and local patch files; change the branching in the parsedArgs handling so the cwd fallback (using gitRepoRoot(startingAt: FileManager.default.currentDirectoryPath)) is only used when the input is piped/stdin (i.e., when parsedArgs indicates piped content), while for a local patch file (parsedArgs.source points to a file path) derive repoRoot from the patch file’s directory via resolvePath(parsedArgs.source) and gitRepoRoot(startingAt: that directory); update the branches around parsedArgs.cwd, parsedArgs.source, diffSourceContext.repoRoot and calls to gitRepoRoot/resolvePath accordingly.
861-875:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
commentTargetis following the split target, not the opener terminal.The new payload is built from
sourceContextafterresolveTargetIfNeeded(), and that resolver prefers--workspace/--surface/--windowover the opener env. If I runcmux diff --workspace ...from terminal A, the saved comment will bias toward the destination workspace instead of A. Downstream,DiffCommentsBridge.handleTargetsresolves candidates from the viewer workspace and only uses the requested surface as an in-workspace bias, so the real opener cannot be recovered later. Capture opener ids before applying target overrides, or update the bridge contract to honor cross-workspace opener metadata.🤖 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 861 - 875, The payload's commentTarget is being set from sourceContext after resolveTargetIfNeeded()/canonicalDiffSourceContext, which means CLI flags (--workspace/--surface/--window) override the original opener terminal; before calling resolveTargetIfNeeded() and canonicalDiffSourceContext(), capture the original opener identifiers (e.g., openerWorkspaceId/openerSurfaceId/openerWindowId from diffSourceContext or the opener env) and preserve them on the payload (or on diffSourceContext.opener* fields) so commentTarget can be built from the opener IDs rather than the post-resolution sourceContext; update places that consume these fields (notably DiffCommentsBridge.handleTargets) to prefer the explicit opener* metadata when present so the opener terminal can be recovered even if the user specified workspace/surface/window overrides.
🤖 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 `@CLI/cmux_open.swift`:
- Around line 852-860: The current logic sets diffSourceContext.repoRoot to
FileManager.default.currentDirectoryPath for any non-source input, which
misattributes remote PR URLs and local patch files; change the branching in the
parsedArgs handling so the cwd fallback (using gitRepoRoot(startingAt:
FileManager.default.currentDirectoryPath)) is only used when the input is
piped/stdin (i.e., when parsedArgs indicates piped content), while for a local
patch file (parsedArgs.source points to a file path) derive repoRoot from the
patch file’s directory via resolvePath(parsedArgs.source) and
gitRepoRoot(startingAt: that directory); update the branches around
parsedArgs.cwd, parsedArgs.source, diffSourceContext.repoRoot and calls to
gitRepoRoot/resolvePath accordingly.
- Around line 861-875: The payload's commentTarget is being set from
sourceContext after resolveTargetIfNeeded()/canonicalDiffSourceContext, which
means CLI flags (--workspace/--surface/--window) override the original opener
terminal; before calling resolveTargetIfNeeded() and
canonicalDiffSourceContext(), capture the original opener identifiers (e.g.,
openerWorkspaceId/openerSurfaceId/openerWindowId from diffSourceContext or the
opener env) and preserve them on the payload (or on diffSourceContext.opener*
fields) so commentTarget can be built from the opener IDs rather than the
post-resolution sourceContext; update places that consume these fields (notably
DiffCommentsBridge.handleTargets) to prefer the explicit opener* metadata when
present so the opener terminal can be recovered even if the user specified
workspace/surface/window overrides.
In `@webviews/src/comments/CommentComposer.tsx`:
- Around line 1-91: The computation of targetLabels via
attachTargetOptionLabels(candidates) is run on every render and should be
memoized to avoid repeated work; wrap the call in a useMemo that depends on
candidates (e.g., const targetLabels = useMemo(() =>
attachTargetOptionLabels(candidates), [candidates])) so
attachTargetOptionLabels, targetLabels and candidates are only recomputed when
candidates changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 113a8272-0980-4166-853f-38bea3d818d9
📒 Files selected for processing (12)
CLI/cmux_open.swiftResources/Localizable.xcstringsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Panels/DiffCommentsBridge.swiftwebviews/src/App.tsxwebviews/src/comments/CommentComposer.tsxwebviews/src/comments/bridge.tswebviews/src/comments/format.tswebviews/src/comments/labels.tswebviews/src/comments/types.tswebviews/src/styles.csswebviews/test/comments.test.ts
Design pivot from dogfood: saved diff comments no longer attach to one chosen terminal. Every comment is saved with a precomputed submission text and registered in a workspace-scoped pending pool; every open TextBox in the workspace shows one '[N comments]' chip, whichever TextBox submits first appends the formatted comments to its submission and marks them consumed (chips clear everywhere; failed submits restore the pool). Comments re-register on viewer load so chips survive app restarts. Deletes the opener heuristic, picker, composer dropdown, and the TerminalPanel attachment-insert machinery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 026cb9311c
ℹ️ 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".
| if !pendingComments.isEmpty { | ||
| for (repoRoot, entries) in Dictionary(grouping: pendingComments, by: \.repoRoot) { | ||
| DiffCommentStore.shared.markConsumed(ids: entries.map(\.commentId), repoRoot: repoRoot) | ||
| } |
There was a problem hiding this comment.
When the same repo diff is open in more than one workspace, comments.list can register the same unconsumed comment into multiple workspace pools. A successful submit here only clears the current workspace via consumeAll and marks the store record consumed, but it leaves the same comment ID pending in the other workspace pools, so a later TextBox submit there appends and sends an already-consumed review comment again. Remove the consumed IDs from DiffCommentSubmissionPool globally when marking them consumed.
Useful? React with 👍 / 👎.
| side, | ||
| startLine: Math.min(range.start, range.end), | ||
| endLine: Math.max(range.start, range.end), |
There was a problem hiding this comment.
For split-diff selections that start on one side and end on the other, SelectedLineRange carries endSide, but the draft stores only side and normalizes the two line numbers with Math.min/Math.max. That turns a cross-side selection into a same-side range, so the saved excerpt/anchor can refer to unrelated old or new lines instead of the selected range. Carry endSide through the draft/comment path (or reject cross-side selections) instead of collapsing everything onto range.side.
Useful? React with 👍 / 👎.
| .then((saved) => { | ||
| dispatch({ type: "upsert-comment", comment: saved }); | ||
| dispatch({ type: "set-draft", draft: null }); | ||
| }) | ||
| .catch((error) => console.warn("cmux diff comment save failed", error)); | ||
| }; | ||
|
|
||
| const editMessage = ( | ||
| comment: DiffCommentRecord, | ||
| message: string, | ||
| fileDiff: CommentFileDiff | null | undefined, | ||
| ) => { | ||
| if (message.trim() === "") { | ||
| return; | ||
| } | ||
| const edited = { ...comment, message, updatedAt: new Date().toISOString() }; | ||
| const updated = { ...edited, submissionText: commentSubmissionText(edited, fileDiff) }; | ||
| const save = bridgeAvailable && repoRoot != null |
There was a problem hiding this comment.
saveDraft starts an async bridge call and returns immediately; there is no in-flight guard on the CommentComposer during that window. Two separate paths produce incorrect behavior:
-
Double-click: the "Comment" button is only
disabledwhenmessage.trim() === "", so a second click before the firstbridgeSaveCommentresolves enterssaveDraftagain —latestState.current.draftis still set, noidis provided, and the native layer generates a fresh UUID for each call, yielding two identical saved comments. -
Cancel-during-save: clicking "Cancel" dispatches
set-draft: nullimmediately, which unmounts the composer, but the in-flightsave.then()still runs to completion and dispatchesupsert-comment, committing the comment the user intended to discard.
A simple fix is to track an isSaving flag in CommentComposer (or as a ref in saveDraft) and disabled-gate the footer buttons until the save settles.
Comment submission text now carries a ```diff fence with paired -/+ rows and trimmed context built from the hunk content (numbered-line fallback when hunk content is unavailable). The pending-comments chip moves above the TextBox input styled like an attachment chip (bubble icon, count, X to dismiss without sending) and shows the formatted comments in a popover on hover. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pierre's line arrays keep each line's trailing newline, which double-spaced the diff excerpts in submissions and the chip popover; line content is now stripped in one helper (also dedups anchor matching). The chip popover survives the cursor traveling into it (bounded cancellable grace task) so it can be scrolled, gets real top padding, and Cmd+Enter in the composer saves the comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 297c537bcb
ℹ️ 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".
| private func registerPending(_ comment: DiffComment, repoRoot: String, workspaceId: UUID) { | ||
| guard comment.consumedAt == nil, | ||
| let submissionText = comment.submissionText, | ||
| !submissionText.isEmpty else { | ||
| return | ||
| } | ||
| DiffCommentSubmissionPool.shared.setPending( | ||
| DiffCommentSubmissionPool.Entry( | ||
| commentId: comment.id, | ||
| repoRoot: DiffCommentStore.canonicalRepoRoot(repoRoot), | ||
| submissionText: submissionText | ||
| ), |
There was a problem hiding this comment.
When an unsent comment is persisted and the diff is regenerated with shifted line numbers, the web UI re-anchors it from lineText, but the TextBox pool still queues the old persisted submissionText here. In that scenario the sidebar/inline marker can point at the moved line while the eventual agent submission still says the original line range/excerpt, sending stale review context. Queue text should be refreshed from the current diff/anchor on load or save before registering it pending.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b236900199
ℹ️ 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".
| lineText: json["lineText"] as? String ?? "", | ||
| message: message, | ||
| submissionText: json["submissionText"] as? String, | ||
| consumedAt: nil, |
There was a problem hiding this comment.
When an already-submitted or dismissed comment is edited, the web client saves the existing id back through comments.save, but the bridge reconstructs it with consumedAt: nil and upsert only preserves createdAt. Because comments.save immediately calls registerPending(saved, ...), editing a previously consumed sidebar comment re-queues it and the next TextBox submission sends the same review comment again. Preserve the stored consumedAt for existing ids unless the user explicitly asks to re-submit it.
Useful? React with 👍 / 👎.
The nonisolated token/trust helpers called MainActor scheme-handler APIs (6 new warnings); they are main-thread-only callers so they become MainActor, and the token tests follow. File length budget refreshed for the feature growth in cmux_open/BrowserPanel/TerminalPanel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feecfad740
ℹ️ 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".
| if (anchor.state === "outdated") { | ||
| continue; |
There was a problem hiding this comment.
When a saved comment becomes outdated after the diff is regenerated, this branch prevents rendering the only SavedComment card that exposes edit/delete actions. I checked the sidebar path in webviews/src/comments/CommentsSection.tsx; it only renders a selectable row with an outdated badge, so these persisted comments can be viewed in the list but cannot be deleted or edited and will remain stuck in the per-repo store unless they happen to re-anchor later.
Useful? React with 👍 / 👎.
# Conflicts: # .github/swift-file-length-budget.tsv # cmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 80149ac. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80149ac454
ℹ️ 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".
| guard TextBoxSubmissionFormatter.hasSubmittableContent(submittedParts) else { | ||
| let poolWorkspaceId = surface.owningWorkspace()?.id | ||
| let hasTypedContent = TextBoxSubmissionFormatter.hasSubmittableContent(submittedParts) | ||
| guard hasTypedContent || pendingCommentCount > 0 else { |
There was a problem hiding this comment.
When another TextBox in the same workspace (or the dismiss button) consumes the shared comment pool after this view last rendered, pendingCommentCount can still be nonzero here, so an otherwise empty TextBox passes this guard; consumeAll then returns no entries and partsToSend remains the empty submittedParts. TextBoxSubmit.dispatchEvents(for:) turns empty parts into a bare Return key, so the stale enabled send button can submit a blank enter to the terminal instead of beeping. Base the guard on the entries claimed from the pool, or re-check after consumeAll, before calling TextBoxSubmit.send.
Useful? React with 👍 / 👎.
…t body Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Application Support/cmux/diff-comments/<repo-hash>.json) keyed by the canonical repo root, so regenerating a diff for the same repo shows the same comments. Each comment stores its anchor line's text and re-anchors by content when line numbers shift; comments whose line is gone or ambiguous show as Outdated in the sidebar only.cmux diffnow embeds the opener workspace/surface (commentTarget) in the viewer payload, including for piped patches and deferred git-source pages. If the opener terminal is gone, it falls back to the only terminal with an open TextBox, then the only terminal in the diff viewer's workspace; anything ambiguous shows an in-page terminal picker. Insertion never steals focus, and attachments queue until the TextBox view mounts when the target panel is hidden.cmuxDiffComments(comments.list/save/delete/attach), installed on browser webviews but rejecting every frame that is not a main-frame page of a registered diff viewer session token.Testing
bun testinwebviews/(134 pass, including new anchor/re-anchor, excerpt, formatting, annotation-derivation, and sidebar-entry tests),bun run typecheck,bun run lint:ciall clean.cmuxTests/DiffCommentStoreTests.swift(store CRUD round-trip, createdAt preservation, repo isolation, key canonicalization) andDiffCommentsAttachResolutionTests(opener-first/fallback/picker matrix), wired into project.pbxproj; runs in CItests.diffc).Issues
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches webview↔native IPC, on-disk comment storage, and TextBox submission merging; bridge is session-gated but incorrect handling could leak or drop user review data.
Overview
Adds review comments to the diff viewer (gutter composer, inline annotations, sidebar) with per-repository JSON persistence and a workspace pending-comments pool that surfaces as a chip above terminal TextBoxes and is appended on the next submit.
cmux diff/ CLI wiresrepoRootinto viewer pages (including piped patches via the CLI’s cwd), adds localized comment UI labels, and content-keys shared/tmpdiff-viewer web assets with SHA-256 so parallel app builds don’t clobber each other’s bundles.cmuxDiffCommentsis the gated native bridge for list/save/delete and pool updates; newtextbox.diffComments.*strings cover the chip UX.CI Swift file-length budgets are bumped for touched large files.
Reviewed by Cursor Bugbot for commit 02dab1d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds review comments to the diff viewer with per‑repo persistence and a workspace pending‑comments pool that appends to the next TextBox submission. Submission text renders as fenced diffs; a pending chip sits above the input with preview/dismiss, and per‑terminal attach targeting plus the “Attach to” dropdown are removed.
New Features
Application Support/cmux/diff-comments/<repo-hash>.json; anchor by line text; outdated in the sidebar only.cmuxDiffComments(comments.list/save/delete) on browser webviews, gated to main‑frame pages of registered diff viewer sessions;cmux diffembedsrepoRoot; labels localized (EN/JA with EN fallbacks).Bug Fixes
DiffCommentStore.shared’s default argument on MainActor to avoid init-time issues.Written for commit 02dab1d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Style
Tests
Bug Fixes