Uh oh!
There was an error while loading. Please reload this page.
Comment on diff to give agent context - #1003
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
21614f6 to
37918a0Comparefac67c6 to
f453ce5CompareUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1f98db7. Configure here.
| images: composerImages, | ||
| persistedAttachments: composerPersistedAttachments, | ||
| terminalContexts: composerTerminalContexts, | ||
| diffContextComments: composerDiffContextComments, |
There was a problem hiding this comment.
Send state derived inconsistently for diff context comments
Medium Severity
deriveComposerSendState doesn't account for diffContextComments, so hasSendableContent is false when only diff comments are pending. ChatComposer patches this by overriding hasSendableContent after the fact, but the onSend handler in ChatView.tsx calls deriveComposerSendState independently and works around it with a separate hasPendingDiffContextComments check. This split means the expired-terminal-context toast logic at line 2431 can still fire even when there are valid diff comments to send — the hasSendableContent is false, so the code enters the "no sendable content" branch and shows a misleading warning before continuing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1f98db7. Configure here.
| extractedDiffComments.promptText, | ||
| ); | ||
| const terminalContexts = displayedUserMessage.contexts; | ||
| const userMessageCopyText = row.message.text; |
There was a problem hiding this comment.
Copy button exposes raw XML context block to user
Medium Severity
The user message copy text was changed from displayedUserMessage.copyText (which was the text after terminal context extraction) to row.message.text (the raw message including the <diff_context_comments> XML block). Clicking the copy button now puts the raw structured XML block into the user's clipboard, which is an unintended UX regression — the hidden context block is specifically designed to be invisible to the user.
Reviewed by Cursor Bugbot for commit 1f98db7. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new user-facing feature (diff context comments) with significant scope across multiple core components. Three unresolved review comments identify bugs: inconsistent send state handling, a copy-to-clipboard regression exposing raw XML, and a high-severity issue causing terminal context labels to appear twice. The combination of new feature complexity and identified bugs warrants human review. You can customize Macroscope's approvability policy. Learn more. |
| const inlineEntries = [ | ||
| ...props.terminalContexts.map((context) => ({ | ||
| kind: "terminal" as const, | ||
| key: `user-terminal-context-inline:${context.header}`, | ||
| label: formatInlineTerminalContextLabel(context.header), | ||
| node: ( | ||
| <UserMessageTerminalContextInlineLabel | ||
| key={`user-terminal-context-inline:${context.header}`} | ||
| context={context} | ||
| /> | ||
| ), | ||
| })), | ||
| ...props.diffContextComments.map((comment) => ({ | ||
| kind: "diff" as const, | ||
| key: `user-diff-context-comment-inline:${comment.header}`, | ||
| label: formatInlineDiffContextCommentLabel(comment.header), | ||
| node: ( | ||
| <UserMessageDiffContextCommentInlineLabel | ||
| key={`user-diff-context-comment-inline:${comment.header}`} | ||
| comment={comment} | ||
| /> | ||
| ), | ||
| })), | ||
| ]; | ||
| const matchedInlineEntries = inlineEntries | ||
| .map((entry) => ({ ...entry, matchIndex: props.text.indexOf(entry.label) })) | ||
| .filter((entry) => entry.matchIndex >= 0) | ||
| .toSorted((left, right) => left.matchIndex - right.matchIndex); | ||
| let cursor = 0; | ||
| for (const context of props.terminalContexts) { | ||
| const label = formatInlineTerminalContextLabel(context.header); | ||
| const matchIndex = props.text.indexOf(label, cursor); | ||
| if (matchIndex === -1) { | ||
| inlineNodes.length = 0; | ||
| break; | ||
| } | ||
| if (matchIndex > cursor) { | ||
| inlineNodes.push( | ||
| <span key={`user-terminal-context-inline-before:${context.header}:${cursor}`}> | ||
| {props.text.slice(cursor, matchIndex)} | ||
| </span>, | ||
| ); | ||
| if (matchedInlineEntries.length === inlineEntries.length) { | ||
| for (const entry of matchedInlineEntries) { | ||
| const matchIndex = props.text.indexOf(entry.label, cursor); | ||
| if (matchIndex < cursor) { |
There was a problem hiding this comment.
🟠 Highchat/MessagesTimeline.tsx:723
When only terminal context labels are embedded in the message text (without diff comment labels), the code enters the inline replacement block but fails the matchedInlineEntries.length === inlineEntries.length check because inlineEntries contains both terminal and diff entries. It then falls back to appending all chips followed by the full props.text, causing embedded terminal labels to appear twice: once as chips and once as raw text.
- if (hasEmbeddedInlineLabels || hasEmbeddedDiffLabels) {+ if (hasEmbeddedInlineLabels || hasEmbeddedDiffLabels) {
const inlineEntries = [
...props.terminalContexts.map((context) => ({
kind: "terminal" as const,
@@ -747,3 +747,5 @@
...props.diffContextComments.map((comment) => ({
kind: "diff" as const,
key: `user-diff-context-comment-inline:${comment.header}`,
label: formatInlineDiffContextCommentLabel(comment.header),
node: (
@@ -751,2 +753,6 @@
),
})),
- ];+ ].filter((entry) =>+ entry.kind === "terminal"+ ? hasEmbeddedInlineLabels+ : hasEmbeddedDiffLabels+ );🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/components/chat/MessagesTimeline.tsx around lines 723-756:
When only terminal context labels are embedded in the message text (without diff comment labels), the code enters the inline replacement block but fails the `matchedInlineEntries.length === inlineEntries.length` check because `inlineEntries` contains both terminal and diff entries. It then falls back to appending all chips followed by the full `props.text`, causing embedded terminal labels to appear twice: once as chips and once as raw text.
juliusmarminge
commented
Jun 10, 2026
Closing as part of an open-PR triage — main has moved a lot since this was opened (the branch conflicts and the files it touches have been substantially reworked), so it's no longer reviewable as-is. If you're still interested, please feel free to resubmit against current main. Thanks! |
I'm aware that this is a large PR, and a big no no per the Contributing.md..., will be opening anyways since it may serve as a basis as Julius mentioned 🫡 .
closes#79
Here's a video of how it would work:
t3code-diff-context.mp4
This PR would cover:
Checklist
Note
Add inline diff context comments to the chat composer and diff panel
DiffPanel.logic.tsintroduces path normalization utilities, line range helpers, anduseDiffContextCommentDraftshook managing draft lifecycle (create, edit, delete, cancel).composerDraftStore.tsadds full diff context comment state management including persistence, hydration, and placeholder synchronization in the prompt string.ComposerPromptEditor.tsxintroducesComposerDiffContextCommentNode, a new inline chip token representing a diff comment, with backspace handling and controlled update support.diffContextComments.tsandpromptContextBlock.tsprovide serialization, extraction, and placeholder utilities shared across the feature.MessagesTimelinecopies the original message text including comment markers.U+E000) for each inline diff comment; any prompt processing that doesn't strip placeholders will see unexpected characters.📊 Macroscope summarized f453ce5. 17 files reviewed, 4 issues evaluated, 1 issue filtered, 1 comment posted
🗂️ Filtered Issues
apps/web/src/components/ChatView.tsx — 0 comments posted, 2 evaluated, 1 filtered
.catchhandler ofonSend), prompt, images, and terminal contexts are checked via refs (promptRef.current,composerImagesRef.current,composerTerminalContextsRef.current), but diff context comments are checked by reading the store viauseComposerDraftStore.getState().getComposerDraft(composerDraftTarget)?.diffContextComments.length. BecauseclearComposerDraftContentalready cleared the store on line 2539, the store read will always be 0 regardless of whether the user added new diff-context comments between the clear and the error. If a user rapidly adds a diff-context comment while the failed send is in flight, the guard will incorrectly evaluate totrueand overwrite the user's newly-added comment with the snapshot from the failed send. The other three fields avoid this problem by using refs that theChatComposerupdates synchronously on user interaction. [ Failed validation ]Note
Medium Risk
Adds a new persisted draft-comment flow that modifies composer draft storage, send payload construction, and message rendering; bugs could lead to lost draft state or malformed prompts, but changes are scoped to the chat/diff UI.
Overview
Enables selecting lines in the diff viewer to create/edit/delete draft diff comments that are attached to the composer as inline chips and persisted per-thread.
On send, the composer now appends a structured
<diff_context_comments>block (with inline@diff:labels replacing placeholder tokens) and treats pending diff comments as sendable content even when the text prompt is empty; failed sends restore prompt/images/terminal contexts/diff comments from a single snapshot.Adds a reusable
promptContextBlockparser/builder and refactors terminal context serialization to use it, plus updates the editor/cursor tokenization and timeline rendering to recognize and display diff-comment chips while hiding the raw trailing block.Reviewed by Cursor Bugbot for commit 1f98db7. Bugbot is set up for automated code reviews on this repo. Configure here.