Repository navigation
Conversation
Add a narrow minimap strip alongside the diff viewer that shows colored markers for additions (green) and deletions (red), proportionally positioned to reflect where changes sit in the file. Clicking a marker smoothly scrolls to that change. A semi-transparent viewport thumb shows the current scroll position. Closes #2315
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 (1)
📝 WalkthroughWalkthroughAdds a new DiffScrollIndicators component and integrates it into FileViewerContent by changing the diff layout to a two-column structure, placing the LightDiffViewer and the scrollbar indicators as sibling panels and moving diff event handlers to an inner diff container. Changes
Sequence DiagramsequenceDiagram
participant User
participant FileViewerContent
participant DiffScrollIndicators
participant DiffParser
participant ScrollContainer as Scroll Container
User->>FileViewerContent: Open file diff
FileViewerContent->>DiffScrollIndicators: Mount with original, modified, filePath, scrollRef
DiffScrollIndicators->>DiffParser: parseDiffFromFile(original, modified)
DiffParser-->>DiffScrollIndicators: regions (add/delete/modify)
DiffScrollIndicators->>ScrollContainer: attach scroll & resize listeners
DiffScrollIndicators->>DiffScrollIndicators: compute band positions & render viewport thumb
User->>DiffScrollIndicators: click on a band
DiffScrollIndicators->>ScrollContainer: scrollTo(targetPosition)
ScrollContainer-->>User: visible viewport jumps to change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx (2)
27-36: Consider extracting hardcoded colors to theme tokens.The PR objective mentions using "existing theme color tokens (e.g., --diff-add-bg, --diff-remove-bg) for visual consistency." While
MIDNIGHT_CODE_COLORSis used for deletion/addition colors,TRACK_BG,TRACK_BG_HOVER, and the viewport thumb colors (lines 242-244) are hardcoded hex values. This could cause visual inconsistency if themes change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx` around lines 27 - 36, Replace hardcoded track and thumb hex colors in the DiffScrollIndicators component with theme tokens: swap TRACK_BG and TRACK_BG_HOVER for CSS variables (e.g., var(--diff-track-bg, "#1e2127") and var(--diff-track-bg-hover, "#22262e")) and replace the viewport/thumb colors currently set near the thumb rendering (the thumb fill styles) with vars like var(--diff-thumb-bg, "<current-fallback>") and var(--diff-thumb-border, "<current-fallback>"). Update the constants TRACK_BG and TRACK_BG_HOVER and any inline styles that set the viewport/thumbnail colors to use these CSS variables so themes can override while preserving the existing hex values as fallbacks; keep existing DELETION_COLOR and ADDITION_COLOR unchanged if they already use MIDNIGHT_CODE_COLORS.
197-207: Consider using CSS:hoverpseudo-class instead of JS handlers.The hover effect could be achieved with pure CSS, eliminating the need for these handlers and the inline
transitionstyle:♻️ CSS-based hover alternative
- onMouseEnter={handleMouseEnter} - onMouseLeave={handleMouseLeave}Then in a stylesheet or Tailwind class:
.diff-scroll-track:hover { background-color: `#22262e`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx` around lines 197 - 207, The handleMouseEnter and handleMouseLeave functions (and the inline transition/background style on the track element using trackRef) should be replaced with a CSS :hover rule: remove handleMouseEnter, handleMouseLeave and any direct style manipulations of trackRef, add a stable class on the track element (e.g. "diff-scroll-track") and put the base background (TRACK_BG) and hover background (TRACK_BG_HOVER) into CSS (or Tailwind classes) with a transition rule there so the hover color is handled purely by CSS; keep the trackRef only if still needed for other logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx`:
- Around line 27-36: Replace hardcoded track and thumb hex colors in the
DiffScrollIndicators component with theme tokens: swap TRACK_BG and
TRACK_BG_HOVER for CSS variables (e.g., var(--diff-track-bg, "#1e2127") and
var(--diff-track-bg-hover, "#22262e")) and replace the viewport/thumb colors
currently set near the thumb rendering (the thumb fill styles) with vars like
var(--diff-thumb-bg, "<current-fallback>") and var(--diff-thumb-border,
"<current-fallback>"). Update the constants TRACK_BG and TRACK_BG_HOVER and any
inline styles that set the viewport/thumbnail colors to use these CSS variables
so themes can override while preserving the existing hex values as fallbacks;
keep existing DELETION_COLOR and ADDITION_COLOR unchanged if they already use
MIDNIGHT_CODE_COLORS.
- Around line 197-207: The handleMouseEnter and handleMouseLeave functions (and
the inline transition/background style on the track element using trackRef)
should be replaced with a CSS :hover rule: remove handleMouseEnter,
handleMouseLeave and any direct style manipulations of trackRef, add a stable
class on the track element (e.g. "diff-scroll-track") and put the base
background (TRACK_BG) and hover background (TRACK_BG_HOVER) into CSS (or
Tailwind classes) with a transition rule there so the hover color is handled
purely by CSS; keep the trackRef only if still needed for other logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49eb1750-38cf-44f9-af61-c2423739b463
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/index.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx:332">
P2: By wrapping both the diff container and `DiffScrollIndicators` inside `DiffViewerContextMenu`, the diff's context menu will unexpectedly trigger when right-clicking the scroll indicators. This breaks menu actions: 'Edit Here' will jump to a stale location (the last line right-clicked in the diff), and 'Select All' will select text in the adjacent diff viewer. Move the flex layout wrapper outside of `DiffViewerContextMenu` so the menu is strictly bound to the diff container.</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx:369">
P1: The `hideUnchangedRegions` prop is not passed to or handled by `DiffScrollIndicators`. When unchanged regions are collapsed, the diff viewer's scroll height is drastically reduced. However, the scroll indicators still position markers based on the full file's line count. This causes the visual markers and click-to-scroll targets to be completely out of sync with the rendered diff. Consider conditionally hiding the indicators (e.g., `{!hideUnchangedRegions && <DiffScrollIndicators ... />}`) when regions are hidden.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx:231">
P2: Clamp region start/size before converting to percentages so deletion markers at EOF are still visible in the minimap.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| contents={diffData} | ||
| viewMode={diffViewMode} | ||
| hideUnchangedRegions={hideUnchangedRegions} | ||
| <div className="flex h-full min-h-0"> |
Contributor
There was a problem hiding this comment.
P2: By wrapping both the diff container and DiffScrollIndicators inside DiffViewerContextMenu, the diff's context menu will unexpectedly trigger when right-clicking the scroll indicators. This breaks menu actions: 'Edit Here' will jump to a stale location (the last line right-clicked in the diff), and 'Select All' will select text in the adjacent diff viewer. Move the flex layout wrapper outside of DiffViewerContextMenu so the menu is strictly bound to the diff container.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx, line 332:
<comment>By wrapping both the diff container and `DiffScrollIndicators` inside `DiffViewerContextMenu`, the diff's context menu will unexpectedly trigger when right-clicking the scroll indicators. This breaks menu actions: 'Edit Here' will jump to a stale location (the last line right-clicked in the diff), and 'Select All' will select text in the adjacent diff viewer. Move the flex layout wrapper outside of `DiffViewerContextMenu` so the menu is strictly bound to the diff container.</comment>
<file context>
@@ -328,40 +329,48 @@ export function FileViewerContent({
- contents={diffData}
- viewMode={diffViewMode}
- hideUnchangedRegions={hideUnchangedRegions}
+ <div className="flex h-full min-h-0">
+ <div
+ ref={diffContainerRef}
</file context>
| onMouseLeave={handleMouseLeave} | ||
| > | ||
| {regions.map((region) => { | ||
| const topPercent = ((region.startLine - 1) / totalLines) * 100; |
Contributor
There was a problem hiding this comment.
P2: Clamp region start/size before converting to percentages so deletion markers at EOF are still visible in the minimap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/DiffScrollIndicators/DiffScrollIndicators.tsx, line 231:
<comment>Clamp region start/size before converting to percentages so deletion markers at EOF are still visible in the minimap.</comment>
<file context>
@@ -0,0 +1,250 @@
+ onMouseLeave={handleMouseLeave}
+ >
+ {regions.map((region) => {
+ const topPercent = ((region.startLine - 1) / totalLines) * 100;
+ const heightPercent = (region.lineCount / totalLines) * 100;
+ return renderBand(region, topPercent, heightPercent);
</file context>
…psed When hideUnchangedRegions is true, the diff viewer collapses context lines, reducing scroll height. The scroll indicators position bands based on full file line counts, causing markers and click-to-scroll targets to be out of sync with the rendered diff. Hide the indicators in this mode to avoid the coordinate mismatch.
Author
PR Feedback TriageReviewed all AI bot comments. Here's the breakdown: ✅ Resolved (1)
❌ Rejected (3)
📋 Deferred (1)
|
Contributor
|
Acknowledged on my two rejected nitpicks:
I'll resolve my open comments and record the codebase conventions. ✏️ Learnings added
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
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
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.
Hi! Love superset and I use it all the time. Took the opportunity to create this PR (using superset haha) and add something I noticed that went missing in the latest versions (current v1.1.16). Lmk if there's anything you guys need from me, thank you!
Summary
Closes #2315
Changes
DiffScrollIndicators— renders the minimap strip using@pierre/diffsto parse hunks and compute change regionsFileViewerContent.tsx— wraps the diff container in a flex layout and mounts the indicator strip alongside itTest plan
Summary by CodeRabbit