Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Oct 30, 2025

This PR improves how edits are displayed in the main chat view:

Before After
image
image
image

Scope: UI-only presentation improvements for diffs and counters; no changes to tool behavior.


Important

Improved diff appearance in chat view with new components for rendering and statistics, and updated backend tools to support unified diffs.

  • UI Enhancements:
    • Improved diff appearance in ChatRow.tsx and BatchDiffApproval.tsx using DiffView component.
    • Added inline diff statistics (added/removed lines) in CodeAccordian.tsx.
  • New Components and Utilities:
    • Added DiffView.tsx for rendering unified diffs with line numbers.
    • Added diffStats.ts for computing diff statistics.
    • Added diffUtils.ts for diff normalization and extraction.
  • Backend Integration:
    • Updated applyDiffTool.ts, multiApplyDiffTool.ts, and writeToFileTool.ts to generate unified diffs for display.
  • Testing:
    • Added ChatRow.diff-actions.spec.tsx to test inline diff stats and actions.
  • Dependencies:
    • Added diff and @types/diff to package.json.

This description was created by Ellipsis for 7906320. You can customize this summary. It will automatically update as commits are pushed.

Copilot AI review requested due to automatic review settings October 30, 2025 16:03
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 30, 2025
@roomote
Copy link

roomote bot commented Oct 30, 2025

See this task on Roo Code Cloud

Review Summary

All previously flagged issues have been successfully resolved. No new issues found in the latest commit.

Issues to Fix

  • Incorrect line counting for new files (ChatRow.tsx:472-481): The special case for newFileCreated counts all lines in the unified diff format (including headers like --- /dev/null, +++ filename, @@ ...), resulting in inflated counts. The computeDiffStats function already correctly handles unified diffs - remove the special case and use it consistently.
  • Hardcoded white color breaks light theme (DiffView.tsx:230): The +/- indicators use hardcoded #ffffff which will be invisible against light backgrounds. Use a theme-aware color variable instead.
  • Redundant no-op replacements in stripCData (diffUtils.ts:59-63): The case-insensitive replacements on lines 59 and 62 already remove both HTML-encoded and raw CDATA markers, making the subsequent case-sensitive replacements on lines 60 and 63 no-ops. Remove lines 60 and 63 to eliminate the CodeQL warning.

Previous Reviews

Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request.

@dosubot dosubot bot added the UI/UX UI/UX related or focused label Oct 30, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new DiffView component to render unified diffs with VSCode-style formatting, and enhances the UI to display diff statistics (+/- line counts) for file operations. The changes improve the visual presentation of code changes across the application.

  • Adds a DiffView component for rendering unified diffs with side-by-side line numbers
  • Implements diff stats computation and display for file edit operations
  • Converts SEARCH/REPLACE and new file formats to unified diff format

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
webview-ui/src/components/common/DiffView.tsx New component to render unified diffs with VSCode styling
webview-ui/src/components/common/CodeAccordian.tsx Adds diff stats display and integrates DiffView for diff language
webview-ui/src/components/chat/ChatRow.tsx Computes diff stats and converts various diff formats to unified diff
webview-ui/src/components/chat/BatchDiffApproval.tsx Adds diff stats computation and SEARCH/REPLACE conversion
webview-ui/src/components/chat/tests/ChatRow.diff-actions.spec.tsx New test file for diff stats functionality
webview-ui/src/components/chat/ChatView.tsx Comment clarification update
webview-ui/package.json Adds diff library dependencies
pnpm-lock.yaml Lockfile updates for new dependencies
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 30, 2025
Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

One issue has been resolved, but the hardcoded white color issue remains. Please address the remaining issue before approval.

@hannesrudolph
Copy link
Collaborator Author

Addressed remaining review item: replaced hardcoded #ffffff with theme foreground for +/- indicators in DiffView.tsx. All CI checks are green on the latest run. The github-advanced-security entry posted a blank comment and CodeQL is passing; there are no actionable security findings attached to this PR right now.

Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

One issue has been resolved (hardcoded white color), but there is still one outstanding issue to address: incorrect line counting for new files in ChatRow.tsx.

@hannesrudolph
Copy link
Collaborator Author

Resolved the CodeQL warning shown in your screenshot. Updated stripCData() to: (1) convert HTML-encoded CDATA markers to raw, then (2) strip raw markers. This removes the no-op replacement flagged by CodeQL. Changes around lines 58-64.

Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Review complete. One issue found that needs to be addressed before approval.

@hannesrudolph
Copy link
Collaborator Author

hannesrudolph commented Oct 30, 2025

fix(webview-ui): Correct new-file diff line counting by ignoring trailing newline in unified diff; added a unit test to cover this behavior.
Key changes:

  • webview-ui/src/utils/diffUtils.ts (convertNewFileToUnifiedDiff)
  • webview-ui/src/components/chat/tests/ChatRow.diff-actions.spec.tsx
  • All webview-ui tests pass locally (vitest). Ready for re-review.

- Add DiffView with line numbers, +/- column, wrapping
- Show inline +/− counts in headers
- Convert SEARCH/REPLACE to unified diffs
- Remove Undo mechanism (UI/backend)
- Update tests and deps (diff, @types/diff)
…e special-case counting and add test (addresses roomote review)\n\n- Remove special-case line counting for newFileCreated in [diffStatsForInline](webview-ui/src/components/chat/ChatRow.tsx:472)\n- Always use [computeDiffStats()](webview-ui/src/components/chat/ChatRow.tsx:118)\n- Add test to assert +N/-0 for new files in [ChatRow.diff-actions.spec.tsx](webview-ui/src/components/chat/__tests__/ChatRow.diff-actions.spec.tsx)
…onents/common/DiffView.tsx)\n\n- Tint only content and +/- gutter; keep line-number columns neutral\n- Match look of VSCode unified diff blocks (clear red/green bands)\n
…yling

- Integrate highlighter for code syntax based on file language
- Adjust background colors for additions and deletions to match VSCode styles
- Refactor rendering logic to include inline highlighting and gutter styles
…iew rendering

Add diffUtils with unified diff normalization:
- extractUnifiedDiff, convertSearchReplaceToUnifiedDiff, convertNewFileToUnifiedDiff

BatchDiffApproval:
- normalize tool output to unified diff via extractUnifiedDiff
- compute unified diff stats
- remove CDATA handling and legacy conversion paths

DiffView:
- add compact gap rows between hunks
- dedicated +/- column
- improved gutter/background and layout tweaks

ChatRow:
- integrate unified diff normalization pipeline
…L-encoded CDATA markers and removing no-op replacement
…s without no-op replacements (resolves CodeQL warning)
…ling newline in unified diff; add test for ChatRow
… header detection; reuse extractUnifiedDiff for new-file rendering
…unified patch in single-file approval; remove client-side fallback normalization for appliedDiff; align gap-row styling to editor background
@hannesrudolph hannesrudolph force-pushed the feat/diff-view-chatview branch from f0ff55c to 5256aed Compare November 4, 2025 03:17
- Fix stripCData() to properly handle HTML-encoded CDATA markers
- Fix convertNewFileToUnifiedDiff() to emit valid hunks for empty files
- Improve DiffView patch selection to match by filePath
- Add performance optimization to disable highlighting for large diffs (>1000 lines)
- Improve theme detection using proper VS Code theme classes
- Extract duplicate diff stats logic into shared utility module
…ting mode

When PREVENT_FOCUS_DISRUPTION experiment is enabled, write_to_file was not generating a unified diff for existing files, causing the diff not to display in the chat view. Now generates the same diff format as non-background mode for consistency.
content: newContent,
content: fileExists ? undefined : newContent,
diff: fileExists
? formatResponse.createPrettyPatch(relPath, cline.diffViewProvider.originalContent, newContent)
Copy link

Choose a reason for hiding this comment

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

When editing an existing file, the completeMessage now omits 'content' in favor of a 'diff' field generated from cline.diffViewProvider.originalContent. Ensure that originalContent is reliably loaded before this call (especially in the preventFocusDisruption branch) so the diff patch is accurate. Consider moving the original file read before constructing completeMessage if needed.

@hannesrudolph hannesrudolph moved this from PR [Draft / In Progress] to PR [Needs Prelim Review] in Roo Code Roadmap Nov 4, 2025
…d mode

Moved originalContent loading before diff generation to ensure proper diff comparison. Previously, the diff was being generated with empty originalContent, causing all lines to appear as additions.
Comment on lines +207 to 211
content: fileExists ? undefined : newContent,
diff: fileExists
? formatResponse.createPrettyPatch(relPath, cline.diffViewProvider.originalContent, newContent)
: undefined,
} satisfies ClineSayTool)
Copy link

Choose a reason for hiding this comment

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

The originalContent is accessed before it's loaded. In the preventFocusDisruption flow, cline.diffViewProvider.originalContent is undefined at line 209, but isn't populated until line 223 (after the askApproval call). This causes createPrettyPatch to generate a diff against empty content, showing all lines as additions instead of the actual changes. The original content needs to be loaded before creating the diff message.

Fix it with Roo Code or mention @roomote and request a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR - Needs Preliminary Review size:XL This PR changes 500-999 lines, ignoring generated files. UI/UX UI/UX related or focused

Projects

Status: PR [Needs Prelim Review]

Development

Successfully merging this pull request may close these issues.

2 participants