style(desktop): polish v2 diff pane header, file path, and unmodified-lines strip#3899
Merged
style(desktop): polish v2 diff pane header, file path, and unmodified-lines strip#3899
Conversation
…-lines strip - Match ChangesHeader chrome on each file header (border-y + bg-muted/30) - Split filename into dir + basename so the basename always stays visible on narrow widths instead of being truncated behind ellipsis - Add a Copy path button next to the filename, drop Copy file contents - Move StatusIndicator next to the +/- diff stats - Blend diff body with the terminal pane surface color - Flatten the "N unmodified lines" expander flush to the pane edges (kills pierre/diffs' wrapper / content / expand-button rounding + inline gaps for both line-info and line-info-basic separators) - Standardize icon-button padding, drop the inline DiffViewModeToggle in favour of a co-located DiffPaneHeaderExtras component
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChanges introduce a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Greptile SummaryThis PR polishes the v2 diff pane: the file header gets a unified muted-strip style matching the sidebar Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/clarity suggestions with no hard correctness issues. All findings are P2 — one clarifying question about intentionally dropped diff-highlight color overrides, one low-risk CSS-injection note in a Shadow DOM context, and a minor tooltip delay tweak. No data loss, broken flows, or runtime errors. WorkspaceDiff.tsx — confirm whether the removal of
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx | Refactored header layout: flex-nowrap, dir/basename split for truncation, Copy path button added, Copy contents removed, StatusIndicator moved right, p-1 padding standardized, Tooltip delay removed. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/WorkspaceDiff/WorkspaceDiff.tsx | Removed gitDecorationColors CSS-var overrides and copy-contents logic; added terminal surface background blending via useTerminalTheme; injected flush-separator and user-select CSS via unsafeCSS. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx | Loading state now shows "Loading…" when files are empty and fetching; removed outer padding on Virtualizer contentClassName for flush layout. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffPaneHeaderExtras/DiffPaneHeaderExtras.tsx | New co-located component extracted from usePaneRegistry; mirrors FilePaneHeaderExtras / CommentPaneHeaderExtras pattern; no logic changes. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/StatusIndicator/StatusIndicator.tsx | Added iconClassName prop (default "w-3 h-3") so call sites can override icon size; backward-compatible. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/utils/gitDecorationColors/gitDecorationColors.ts | New shared constant GIT_STAT_TEXT_CLASSES for addition/deletion/modified Tailwind color strings; extracted from inline styles for reuse. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileEntry/DiffFileEntry.tsx | Removed outer rounded-md border wrapper; replaced hand-rolled "Show diff" button with <Button size="xs" variant="outline">. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx | Removed inline DiffViewModeToggle; replaced with imported DiffPaneHeaderExtras; cleaned up now-unused imports. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
DP["DiffPane"] --> DFE["DiffFileEntry (per file)"]
DFE --> DFH["DiffFileHeader\n(collapse · path+dir/basename · copy path\n· StatusIndicator · +/- stats · viewed · eye · discard)"]
DFE --> WD["WorkspaceDiff\n(MultiFileDiff)"]
WD -->|"useTerminalTheme()\nsurfaceBg"| BG["Diff body background\nblends with terminal surface"]
WD -->|"unsafeCSS"| CSS["[data-diff] bg override\n+ flush separator styles"]
PR["usePaneRegistry"] -->|"renderHeaderExtras"| DPHE["DiffPaneHeaderExtras\n(unified / split toggle)"]
PR --> DP
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/WorkspaceDiff/WorkspaceDiff.tsx:68-76
**Diff line highlight color overrides silently dropped**
The previous code set `--diffs-addition-color-override`, `--diffs-deletion-color-override`, and `--diffs-modified-color-override` on the `MultiFileDiff` style to keep line highlights in sync with the file tree's git decoration colors. Those three CSS-variable entries are gone from `themeVars` in this PR and are not replaced anywhere (neither in `unsafeCSS` nor in `getDiffViewerStyle`). The diff body will now fall back to whatever the pierre/diffs library defaults are, which may no longer match the sidebar. Is this drop intentional, or should the overrides be re-applied alongside the new background blending?
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/WorkspaceDiff/WorkspaceDiff.tsx:151-174
**`surfaceBg` interpolated into `unsafeCSS` without sanitization**
`surfaceBg` is injected verbatim into the CSS string (`--diffs-light-bg: ${surfaceBg} !important`). The value comes from `terminalTheme?.background`, which is user-controllable via custom themes. A crafted `background` value containing `}` or `;` could escape the declaration block. Given `unsafeCSS` already runs inside a Shadow DOM scoped context, the practical impact is limited, but a whitelist check (e.g. valid hex, `rgb(…)`, `var(…)`) would make this airtight.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx:68
**`Tooltip` instant-open on the file path button**
The old code used `<Tooltip delayDuration={300}>` for the open-file tooltip. The new code drops `delayDuration`, which typically defaults to `0` in Radix UI, so the `CLICK_HINT_TOOLTIP` will flash open on any hover without a settle delay. A short delay (200–300 ms) is the convention used elsewhere in this header and keeps the tooltip from feeling intrusive when the user is scanning file names.
```suggestion
<Tooltip delayDuration={300}>
```
Reviews (1): Last reviewed commit: "style(desktop): polish v2 diff pane head..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
68
to
76
| const themeVars = { | ||
| ...baseThemeVars, | ||
| "--diffs-addition-color-override": gitDecorationColors.addition, | ||
| "--diffs-deletion-color-override": gitDecorationColors.deletion, | ||
| "--diffs-modified-color-override": gitDecorationColors.modified, | ||
| ...getDiffViewerStyle(activeTheme, { | ||
| fontFamily: fontSettings?.editorFontFamily ?? undefined, | ||
| fontSize: Number.isFinite(parsedEditorFontSize) | ||
| ? parsedEditorFontSize | ||
| : undefined, | ||
| }), | ||
| backgroundColor: surfaceBg, | ||
| }; |
There was a problem hiding this comment.
The previous code set --diffs-addition-color-override, --diffs-deletion-color-override, and --diffs-modified-color-override on the MultiFileDiff style to keep line highlights in sync with the file tree's git decoration colors. Those three CSS-variable entries are gone from themeVars in this PR and are not replaced anywhere (neither in unsafeCSS nor in getDiffViewerStyle). The diff body will now fall back to whatever the pierre/diffs library defaults are, which may no longer match the sidebar. Is this drop intentional, or should the overrides be re-applied alongside the new background blending?
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/WorkspaceDiff/WorkspaceDiff.tsx
Line: 68-76
Comment:
**Diff line highlight color overrides silently dropped**
The previous code set `--diffs-addition-color-override`, `--diffs-deletion-color-override`, and `--diffs-modified-color-override` on the `MultiFileDiff` style to keep line highlights in sync with the file tree's git decoration colors. Those three CSS-variable entries are gone from `themeVars` in this PR and are not replaced anywhere (neither in `unsafeCSS` nor in `getDiffViewerStyle`). The diff body will now fall back to whatever the pierre/diffs library defaults are, which may no longer match the sidebar. Is this drop intentional, or should the overrides be re-applied alongside the new background blending?
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
151
to
174
| unsafeCSS: ` | ||
| * { | ||
| user-select: text; | ||
| -webkit-user-select: text; | ||
| * { user-select: text; -webkit-user-select: text; } | ||
| /* Pierre sets --diffs-light-bg/--diffs-dark-bg | ||
| * inline on <pre data-diff> from the Shiki theme; | ||
| * inline beats :host so we override at the pre. */ | ||
| [data-diff] { | ||
| --diffs-light-bg: ${surfaceBg} !important; | ||
| --diffs-dark-bg: ${surfaceBg} !important; | ||
| } | ||
| /* Flatten the "N unmodified lines" strip flush to | ||
| * the pane edges (kills wrapper/content/expand- | ||
| * button rounding + inline gap on both | ||
| * line-info and line-info-basic). */ | ||
| [data-separator^='line-info'] [data-separator-wrapper], | ||
| [data-separator^='line-info'] [data-separator-content], | ||
| [data-separator^='line-info'] [data-expand-up], | ||
| [data-separator^='line-info'] [data-expand-down], | ||
| [data-separator^='line-info'] [data-expand-both] { | ||
| border-radius: 0 !important; | ||
| margin-inline: 0 !important; | ||
| padding-inline: 0 !important; | ||
| } | ||
| `, | ||
| }} |
There was a problem hiding this comment.
surfaceBg interpolated into unsafeCSS without sanitization
surfaceBg is injected verbatim into the CSS string (--diffs-light-bg: ${surfaceBg} !important). The value comes from terminalTheme?.background, which is user-controllable via custom themes. A crafted background value containing } or ; could escape the declaration block. Given unsafeCSS already runs inside a Shadow DOM scoped context, the practical impact is limited, but a whitelist check (e.g. valid hex, rgb(…), var(…)) would make this airtight.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/WorkspaceDiff/WorkspaceDiff.tsx
Line: 151-174
Comment:
**`surfaceBg` interpolated into `unsafeCSS` without sanitization**
`surfaceBg` is injected verbatim into the CSS string (`--diffs-light-bg: ${surfaceBg} !important`). The value comes from `terminalTheme?.background`, which is user-controllable via custom themes. A crafted `background` value containing `}` or `;` could escape the declaration block. Given `unsafeCSS` already runs inside a Shadow DOM scoped context, the practical impact is limited, but a whitelist check (e.g. valid hex, `rgb(…)`, `var(…)`) would make this airtight.
How can I resolve this? If you propose a fix, please make it concise.| </button> | ||
| <StatusIndicator status={status} /> | ||
| <Tooltip delayDuration={300}> | ||
| <Tooltip> |
There was a problem hiding this comment.
Tooltip instant-open on the file path button
The old code used <Tooltip delayDuration={300}> for the open-file tooltip. The new code drops delayDuration, which typically defaults to 0 in Radix UI, so the CLICK_HINT_TOOLTIP will flash open on any hover without a settle delay. A short delay (200–300 ms) is the convention used elsewhere in this header and keeps the tooltip from feeling intrusive when the user is scanning file names.
Suggested change
| <Tooltip> | |
| <Tooltip delayDuration={300}> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx
Line: 68
Comment:
**`Tooltip` instant-open on the file path button**
The old code used `<Tooltip delayDuration={300}>` for the open-file tooltip. The new code drops `delayDuration`, which typically defaults to `0` in Radix UI, so the `CLICK_HINT_TOOLTIP` will flash open on any hover without a settle delay. A short delay (200–300 ms) is the convention used elsewhere in this header and keeps the tooltip from feeling intrusive when the user is scanning file names.
```suggestion
<Tooltip delayDuration={300}>
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ChangesHeaderchrome (border-y+bg-muted/30), with theStatusIndicatormoved next to the +/- diff stats.…instead of hiding the file you actually care about.--diffs-light-bg/--diffs-dark-bgon[data-diff]).line-infoandline-info-basicseparators, kills the wrapper / content / expand-button rounding and inline gaps).DiffViewModeTogglefromusePaneRegistry.tsxinto a co-locatedDiffPaneHeaderExtras(matches theFilePaneHeaderExtras/CommentPaneHeaderExtraspattern); standardized icon-button padding (p-0.5→p-1); replaced the hand-rolled "Show diff" button with<Button size=\"xs\" variant=\"outline\">; loading state distinguishes "Loading…" from "No changes".Test plan
Summary by cubic
Polishes the v2 Changes diff pane for clearer headers and consistent styling with the sidebar and terminal. Adds a Copy path action, better path display, and small UX tweaks for a smoother review experience.
New Features
ChangesHeader(border-y + muted bg); status icon sits next to the +/- counters.Refactors
DiffPaneHeaderExtrasfor the diff view toggle (replaces inline toggle).<Button size="xs" variant="outline">.Written for commit eb74dce. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Refactor