[FIX] Improve-diffs #65
Conversation
…ibility in merge view
…bility in merge UI
- Add whitespace stripping setting (auto on)
- Show diffs correctly
📝 WalkthroughWalkthroughAdds whitespace-only auto-resolution, a toggleable "Base" column in the 3-way merge UI, and an undo/redo hotkey flag. Settings and package.json expose new config keys; conflict detection, resolver, and web UI are updated to propagate and act on these flags with whitespace-aware diff rendering. Changes
Sequence DiagramsequenceDiagram
participant Config as Extension Config
participant Settings as Settings Module
participant Detector as Conflict Detector
participant Resolver as Conflict Resolver
participant Panel as WebConflictPanel
participant UI as Web UI (React)
Config->>Settings: Load flags (autoResolveWhitespace, showBaseColumn, enableUndoRedoHotkeys)
Settings->>Detector: Provide autoResolveWhitespace
Detector->>Detector: Detect whitespace-only diffs
alt Whitespace-only
Detector->>Detector: Auto-resolve conflict (update counters/descriptions)
Detector->>Resolver: Return auto-resolved results
else Non-whitespace / remaining
Detector->>Resolver: Return remaining conflicts
end
Resolver->>Panel: Build UnifiedConflict (include showBaseColumn)
Panel->>UI: Send conflict payload to browser
UI->>UI: Conditionally render Base column and apply branch-aware diff styling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/web/webTypes.ts (1)
84-102:⚠️ Potential issue | 🟡 Minor
showBaseColumnis missing fromWebConflictData.
UnifiedConflict(Line 34) and the client-sideUnifiedConflictData(insrc/web/client/types.ts:58) both includeshowBaseColumn, butWebConflictData— described as the "web-compatible version of UnifiedConflict" — does not. The payload constructed inWebConflictPanel._sendConflictData()includesshowBaseColumn, so the browser receives it, but the type definition here is out of sync.📝 Add the missing field
// Display options hideNonConflictOutputs?: boolean; showCellHeaders?: boolean; + showBaseColumn?: boolean; // Branch information currentBranch?: string;
🤖 Fix all issues with AI agents
In `@src/conflictDetector.ts`:
- Around line 22-29: The current whitespace-normalization is too aggressive:
update the logic so it no longer strips newlines/indentation or auto-resolves
indentation-sensitive languages. Either (A) change stripAllWhitespace to only
normalize line endings and trim trailing spaces (preserve internal newlines and
indentation) and keep using isWhitespaceOnlyDifference, or (B) add a language
check before invoking isWhitespaceOnlyDifference (in the conflict resolution
paths referenced around conflictDetector usage at the sites noted, e.g., where
calls occur near the lines mentioned) to skip auto-resolution for
indentation-sensitive languages like Python (detect via notebook
kernelspec/language_info). Ensure you reference and update the functions
stripAllWhitespace and isWhitespaceOnlyDifference and the conflict resolution
callers so indentation differences in Python are not auto-resolved.
In `@src/web/client/styles.ts`:
- Around line 996-1018: Update the inline comments so the color labels match the
CSS variables: change the comment "Current branch highlights (green)" to reflect
blue (or mention --current-rgb / --current-border) and change "Incoming branch
highlights (blue)" to reflect green/teal (or mention --incoming-rgb /
--incoming-border); align the parenthetical color names with the variables used
by .diff-line-current, .diff-inline-current, .diff-line-incoming, and
.diff-inline-incoming.
🧹 Nitpick comments (4)
src/conflictDetector.ts (1)
62-68: Consider removing or gating these debug logs.These
console.logcalls fire for every semantic conflict detection and log raw content lengths and equality comparisons. In a VS Code extension, these will appear in the Extension Host output and add noise. Consider removing them or wrapping them behind a verbose/debug setting.src/web/client/MergeRow.tsx (1)
49-49: Default forshowBaseColumnis inconsistent with the parent component.
MergeRowdefaultsshowBaseColumntotrue, whileConflictResolverresolves it tofalsefrom the conflict payload. SinceConflictResolveralways passes the prop explicitly, this isn't a runtime bug, but the default here is misleading — it suggests base is shown by default, which contradicts the PR intent.Suggested fix
- showBaseColumn = true, + showBaseColumn = false,Also applies to: 75-75
src/web/client/CellContent.tsx (2)
93-101:sideprop is not forwarded fromCellContenttoDiffContent.
CellContentreceivessideas a prop but doesn't pass it through toDiffContent(lines 97–101).DiffContentfalls back to'current'(line 149). This is currently harmless becausediffMode='conflict'makessideirrelevant ingetDiffLineClass, but it will silently produce wrong branch-based coloring ifdiffMode='base'is ever used.Suggested fix
<DiffContent source={source} compareSource={normalizeCellSource((compareCell ?? baseCell)!.source)} diffMode={diffMode} + side={side} />Also applies to: 129-136
184-188:side='base'falls through to incoming styling in base diffMode.When
sideis'base'anddiffModeis'base', line 188 routes to'diff-line-incoming'because the ternary only checks for'current'. While currently unreachable (MergeRow always usesdiffMode='conflict'), this would produce incorrect coloring if a base-mode diff view is added later.Consider an explicit check:
Suggested fix
- return side === 'current' ? 'diff-line diff-line-current' : 'diff-line diff-line-incoming'; + if (side === 'base') return 'diff-line diff-line-conflict'; + return side === 'current' ? 'diff-line diff-line-current' : 'diff-line diff-line-incoming';
src/web/client/styles.ts
Outdated
| /* Current branch highlights (green) */ | ||
| .diff-line-current { | ||
| background: rgba(var(--current-rgb), 0.18); | ||
| border-left: 3px solid rgba(var(--current-rgb), 0.7); | ||
| } | ||
|
|
||
| .diff-inline-current { | ||
| background: rgba(var(--current-rgb), 0.35); | ||
| padding: 0 2px; | ||
| border-radius: 2px; | ||
| } | ||
|
|
||
| /* Incoming branch highlights (blue) */ | ||
| .diff-line-incoming { | ||
| background: rgba(var(--incoming-rgb), 0.18); | ||
| border-left: 3px solid rgba(var(--incoming-rgb), 0.7); | ||
| } | ||
|
|
||
| .diff-inline-incoming { | ||
| background: rgba(var(--incoming-rgb), 0.35); | ||
| padding: 0 2px; | ||
| border-radius: 2px; | ||
| text-decoration: line-through; | ||
| } |
There was a problem hiding this comment.
Swapped color labels in comments.
The comment on Line 996 says "Current branch highlights (green)" but --current-rgb is 64, 164, 223 (blue, matching --current-border: #40a4df). Similarly, Line 1008 says "Incoming branch highlights (blue)" but `--incoming-rgb` is `78, 201, 176` (teal/green, matching `--incoming-border: `#4ec9b0). The CSS is correct; only the parenthetical color names are swapped.
📝 Fix comments
-/* Current branch highlights (green) */
+/* Current branch highlights (blue) */
.diff-line-current {
background: rgba(var(--current-rgb), 0.18);
border-left: 3px solid rgba(var(--current-rgb), 0.7);
}
...
-/* Incoming branch highlights (blue) */
+/* Incoming branch highlights (teal) */
.diff-line-incoming {
background: rgba(var(--incoming-rgb), 0.18);
border-left: 3px solid rgba(var(--incoming-rgb), 0.7);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Current branch highlights (green) */ | |
| .diff-line-current { | |
| background: rgba(var(--current-rgb), 0.18); | |
| border-left: 3px solid rgba(var(--current-rgb), 0.7); | |
| } | |
| .diff-inline-current { | |
| background: rgba(var(--current-rgb), 0.35); | |
| padding: 0 2px; | |
| border-radius: 2px; | |
| } | |
| /* Incoming branch highlights (blue) */ | |
| .diff-line-incoming { | |
| background: rgba(var(--incoming-rgb), 0.18); | |
| border-left: 3px solid rgba(var(--incoming-rgb), 0.7); | |
| } | |
| .diff-inline-incoming { | |
| background: rgba(var(--incoming-rgb), 0.35); | |
| padding: 0 2px; | |
| border-radius: 2px; | |
| text-decoration: line-through; | |
| } | |
| /* Current branch highlights (blue) */ | |
| .diff-line-current { | |
| background: rgba(var(--current-rgb), 0.18); | |
| border-left: 3px solid rgba(var(--current-rgb), 0.7); | |
| } | |
| .diff-inline-current { | |
| background: rgba(var(--current-rgb), 0.35); | |
| padding: 0 2px; | |
| border-radius: 2px; | |
| } | |
| /* Incoming branch highlights (teal) */ | |
| .diff-line-incoming { | |
| background: rgba(var(--incoming-rgb), 0.18); | |
| border-left: 3px solid rgba(var(--incoming-rgb), 0.7); | |
| } | |
| .diff-inline-incoming { | |
| background: rgba(var(--incoming-rgb), 0.35); | |
| padding: 0 2px; | |
| border-radius: 2px; | |
| } |
🤖 Prompt for AI Agents
In `@src/web/client/styles.ts` around lines 996 - 1018, Update the inline comments
so the color labels match the CSS variables: change the comment "Current branch
highlights (green)" to reflect blue (or mention --current-rgb /
--current-border) and change "Incoming branch highlights (blue)" to reflect
green/teal (or mention --incoming-rgb / --incoming-border); align the
parenthetical color names with the variables used by .diff-line-current,
.diff-inline-current, .diff-line-incoming, and .diff-inline-incoming.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
closes [FIX] Diffing on 3 way merge conflicts #11
Summary by CodeRabbit