Repository navigation
Make diff pane selectable and jump to raw edit - #2227
Merged
Kitenite merged 21 commits intoMar 8, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds diff hover events, DOM-to-raw-position mapping utilities and tests, a read-only diff context menu, wiring to open the raw file at a mapped location (including double-click), preview-pane location updates, and diff viewer CSS to allow text selection. Changes
Sequence DiagramsequenceDiagram
participant User
participant LightDiffViewer
participant FileViewerContent
participant FileViewerPane
participant DiffViewerContextMenu
participant Editor
User->>LightDiffViewer: hover on diff line
LightDiffViewer->>FileViewerContent: onDiffLineEnter(event)
FileViewerContent->>FileViewerContent: update lastDiffLocationRef
User->>LightDiffViewer: double-click diff line
LightDiffViewer->>FileViewerContent: onLineClick / double-click
FileViewerContent->>FileViewerContent: mapDiffLocationToRawPosition(...)
FileViewerContent->>FileViewerPane: onSwitchToRawAtLocation(line,column)
FileViewerPane->>FileViewerPane: switchToMode("raw",{line,column})
FileViewerPane->>Editor: render raw view at location
Editor->>User: show raw editor at position
User->>DiffViewerContextMenu: right-click diff container
DiffViewerContextMenu->>DiffViewerContextMenu: validate selection, expose actions
DiffViewerContextMenu->>User: display context menu
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 8 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/utils/diff-location.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts:87">
P2: Context lines on the deletion side carry old-file line numbers, but this condition maps them directly to the modified file. When prior hunks have added or removed lines, the old line number no longer matches the new file position, causing the editor to jump to the wrong line on double-click.
The `lineType === "context"` check should not bypass the mapping when `side === "deletions"` — those lines still need old→new translation. One approach: extend `mapDeletedLineToRawLine` to track and match context lines, then simplify this condition to branch solely on `side`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts`:
- Around line 40-75: The function mapDeletedLineToRawLine is treating
chunk.lines, chunk.deletions, and chunk.additions as arrays and calling .length,
but the `@pierre/diffs` types use numbers; fix by removing .length and using the
numeric values directly: when handling chunk.type === "context" increment
currentOldLine and currentNewLine by chunk.lines (number), change the deletion
loop to for (let index = 0; index < chunk.deletions; index += 1) (using
chunk.deletions as a number), and increment currentNewLine by chunk.additions
(number) instead of .length; keep the rest of the logic (parseDiffFromFile,
clampLineNumber, hunk iteration) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c58c6df-1731-4e15-8eb7-63f028a60e8d
📒 Files selected for processing (8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/LightDiffViewer/LightDiffViewer.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/DiffViewerContextMenu/DiffViewerContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/DiffViewerContextMenu/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.tsapps/desktop/src/renderer/stores/tabs/store.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx (1)
166-190:⚠️ Potential issue | 🟠 MajorReset the initial-location guard when leaving Raw mode.
hasAppliedInitialLocationRefsurvives raw → diff/rendered → raw transitions. WhenviewModeis not "raw",CodeEditorunmounts (lines 330–577 early-return before the editor at line 595). When switching back to raw with unchangedinitialLine/initialColumn, the guard remains true because it is only reset on file/cursor changes (lines 222–230), not on mode changes. The remounted editor then skipsrevealPositionat line 319, leaving the pane at the wrong scroll position.Suggested fix
+useEffect(() => { + if (viewMode !== "raw") { + hasAppliedInitialLocationRef.current = false; + } +}, [viewMode]); + // biome-ignore lint/correctness/useExhaustiveDependencies: Reset cached diff interaction when the file changes useEffect(() => { lastDiffLocationRef.current = null; }, [filePath]);🤖 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/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx` around lines 166 - 190, The hasAppliedInitialLocationRef guard persists across viewMode transitions causing the remounted CodeEditor to skip revealPosition; update the effects that reset hasAppliedInitialLocationRef (currently tied to filePath and [initialLine, initialColumn]) to also reset when viewMode changes back to "raw". Concretely, add viewMode to the dependency array (or add a separate useEffect watching viewMode) and set hasAppliedInitialLocationRef.current = false when viewMode === "raw" so that CodeEditor (component referenced as CodeEditor and the revealPosition call) will apply the initialLine/initialColumn on remount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx`:
- Around line 187-208: The ref lastDiffLocationRef is only reset when filePath
changes, but visual diff row indices can change when diffData,
hideUnchangedRegions, or diffViewMode change; update the useEffect that
currently depends on [filePath] (and its comment) to also depend on diffData,
hideUnchangedRegions, and diffViewMode (or any other prop used to compute the
rendered diff) so lastDiffLocationRef.current is cleared whenever the rendered
diff changes, ensuring getDiffSelectionLines() maps to a correct raw position.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx`:
- Around line 166-190: The hasAppliedInitialLocationRef guard persists across
viewMode transitions causing the remounted CodeEditor to skip revealPosition;
update the effects that reset hasAppliedInitialLocationRef (currently tied to
filePath and [initialLine, initialColumn]) to also reset when viewMode changes
back to "raw". Concretely, add viewMode to the dependency array (or add a
separate useEffect watching viewMode) and set
hasAppliedInitialLocationRef.current = false when viewMode === "raw" so that
CodeEditor (component referenced as CodeEditor and the revealPosition call) will
apply the initialLine/initialColumn on remount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44d143b2-1958-4cfb-b3fc-d1a4c2943bfb
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts`:
- Around line 67-97: The function fails to shift old-side lines that fall after
earlier hunks because it returns the raw lineNumber unchanged; fix by carrying a
cumulative line delta across diff.hunks (e.g., track lineDelta or update
cumulative offsets rather than resetting per hunk) and apply that delta to the
fallback return; specifically, when iterating diff.hunks (and using
currentOldLine/currentNewLine), accumulate (hunk.additions.length -
hunk.deletions.length) into a lineDelta and return clampLineNumber(lineNumber +
lineDelta, modifiedLines) instead of clampLineNumber(lineNumber, modifiedLines).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4431c261-4fff-49c1-9e48-87af93895a40
📒 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/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.ts (1)
7-11: Extract the repeatedFileContentsfixture.A tiny helper would keep each case focused on the mapping behavior instead of re-declaring the same object shape six times.
♻️ Suggested cleanup
+const makeContents = (original: string, modified: string): FileContents => ({ + original, + modified, + language: "text", +}); + describe("mapDiffLocationToRawPosition", () => { it("keeps addition-side clicks on the same modified line", () => { - const contents: FileContents = { - original: "one\ntwo\nthree", - modified: "one\ntwo changed\nthree", - language: "text", - }; + const contents = makeContents("one\ntwo\nthree", "one\ntwo changed\nthree");Also applies to: 28-32, 48-52, 68-72, 88-92, 108-112
🤖 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/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.ts` around lines 7 - 11, Tests repeat the same FileContents fixture multiple times; extract a small helper (e.g., createFileContents or makeFileContents) that returns the base FileContents object and use it in each test instead of re-declaring the literal. Update occurrences that declare const contents: FileContents = { original: "one\n...", modified: "...", language: "text" } to call the helper and, when needed, override fields with object spread (e.g., { ...createFileContents(), modified: "..." }) so each case remains focused on mapping behavior; reference the FileContents type and the existing contents declarations in diff-location.test.ts when making the change.
🤖 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/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.ts`:
- Around line 7-11: Tests repeat the same FileContents fixture multiple times;
extract a small helper (e.g., createFileContents or makeFileContents) that
returns the base FileContents object and use it in each test instead of
re-declaring the literal. Update occurrences that declare const contents:
FileContents = { original: "one\n...", modified: "...", language: "text" } to
call the helper and, when needed, override fields with object spread (e.g., {
...createFileContents(), modified: "..." }) so each case remains focused on
mapping behavior; reference the FileContents type and the existing contents
declarations in diff-location.test.ts when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4579f6a5-65af-407f-ab2b-fdf56bc591d5
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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:268">
P2: `handleDiffLineEnter` stores `diffContainerRef.current` (the entire scrollable container) as `lineElement`, but downstream helpers like `getDiffCodeElement` call `lineElement.querySelector('[data-code]')` expecting a single-line element. When this ref is later consumed by `openRawFromDiffLocation` without an explicit column (the `location.column ??` fallback branch), the query hits the first `[data-code]` in the whole diff instead of the hovered line, potentially producing an incorrect column.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
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/stores/tabs/store.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/tabs/store.ts:733">
P2: `shouldUpdateViewerState` doesn't account for the new `selectionStartLine`/`selectionEndLine` options. If a caller passes only selection range properties (without `line`/`column` or a viewMode change), the guard evaluates to false and the store update is skipped, silently discarding the selection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| options.line ?? existingFileViewer.initialLine, | ||
| initialColumn: | ||
| options.column ?? existingFileViewer.initialColumn, | ||
| initialSelectionStartLine: |
Contributor
There was a problem hiding this comment.
P2: shouldUpdateViewerState doesn't account for the new selectionStartLine/selectionEndLine options. If a caller passes only selection range properties (without line/column or a viewMode change), the guard evaluates to false and the store update is skipped, silently discarding the selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/stores/tabs/store.ts, line 733:
<comment>`shouldUpdateViewerState` doesn't account for the new `selectionStartLine`/`selectionEndLine` options. If a caller passes only selection range properties (without `line`/`column` or a viewMode change), the guard evaluates to false and the store update is skipped, silently discarding the selection.</comment>
<file context>
@@ -730,6 +730,12 @@ export const useTabsStore = create<TabsStore>()(
options.line ?? existingFileViewer.initialLine,
initialColumn:
options.column ?? existingFileViewer.initialColumn,
+ initialSelectionStartLine:
+ options.selectionStartLine ??
+ existingFileViewer.initialSelectionStartLine,
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/components/CodeEditor/CodeEditor.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/CodeEditor/CodeEditor.tsx:158">
P2: This `console.debug` with the `[DiffEditHereDebug]` tag looks like leftover development instrumentation. It fires on every `revealPosition` call and logs internal editor state that isn't useful outside debugging this feature. Remove it before merging.
(Based on your team's feedback about removing excessive logs and dead comments proactively.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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/utils/diff-location.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/utils/diff-location.ts:117">
P3: The fallback column measurement uses code-point indexing (`Array.from`) but the rest of the file uses UTF-16 column indexing (`.length`). This can produce incorrect jump columns for lines containing emoji/astral characters.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Kitenite
deleted the
kitenite/make-the-diff-pane-view-selectable-and-add-the-rig
branch
March 8, 2026 21:38
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
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
Verification
Summary by CodeRabbit
New Features
Tests
Behavior