feat(desktop): scroll to first diff when opening file in diff viewer by Kitenite · Pull Request #623 · superset-sh/superset

Kitenite · 2026-01-06T19:26:41Z

Summary

Automatically scroll to the first diff location when opening a file in the diff viewer, improving UX so users don't have to manually scroll to find changes
Uses Monaco DiffEditor's onDidUpdateDiff callback to detect when diff computation completes, then scrolls to the first change using revealLineInCenter

Summary by CodeRabbit

New Features
- Added a public filePath prop to the diff viewer to ensure scrolling behavior resets when switching files.
Bug Fixes
- Diff viewer now automatically and reliably scrolls to the first difference once when a file is opened, with improved handling when navigating between files.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-06T19:26:53Z

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds tracking and lifecycle-managed listener to the DiffViewer that detects when Monaco diff computation finishes and automatically scrolls the diff editor to the first changed line, resetting behavior when the viewed file changes.

Changes

Cohort / File(s)	Summary
DiffViewer enhancement `apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/components/DiffViewer/DiffViewer.tsx`	Adds `filePath` prop to `DiffViewerProps`; introduces `diffEditorRef`, `hasScrolledToFirstDiffRef`, and `diffUpdateListenerRef`; attaches `onDidUpdateDiff` listener to trigger a one-time scroll to the first diff line; disposes previous listeners on remount/unmount and resets scroll flag when `filePath` changes.

Sequence Diagram(s)

(omitted — changes are localized to the component and do not introduce a multi-component sequential flow that meets diagram criteria)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat(desktop): add editable Monaco diff view for uncommitted changes #387 — Modifies the same DiffViewer.tsx and adds editor mount/shortcut and editable/onSave handling; related due to overlapping lifecycle/ref changes.

Poem

🐰 I hop along the diffing trail,
Listeners twitch when changes sail,
I scroll to the first bright line,
Keep things tidy, neat, and fine,
A little cleanup makes code prevail.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main feature: auto-scrolling to first diff when opening files in the diff viewer.
Description check	✅ Passed	The description provides a clear summary and implementation details, but lacks several required template sections like Related Issues, Type of Change, Testing, and Additional Notes.

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 83e16ec and b5dc454.

📒 Files selected for processing (1)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/components/DiffViewer/DiffViewer.tsx

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/components/DiffViewer/DiffViewer.tsx (1)
79-84: Clarify the comment about deletion handling.

The comment suggests using originalStartLineNumber for deletions, but modifiedStartLineNumber > 0 doesn't actually detect deletions—it will be > 0 even for pure deletions (pointing to where the deletion occurred). The fallback to originalStartLineNumber would only trigger if modifiedStartLineNumber is somehow 0, which is edge-case behavior.

The code works correctly, but the comment is misleading.
🔎 Suggested comment clarification
-					// Use modifiedStartLineNumber for scrolling in the modified editor
-					// If it's a deletion (modifiedStartLineNumber > modifiedEndLineNumber), use originalStartLineNumber
+					// Use modifiedStartLineNumber for scrolling in the modified editor
+					// Fallback to originalStartLineNumber if modifiedStartLineNumber is unavailable

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d4318dd and 83e16ec.

📒 Files selected for processing (1)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/components/DiffViewer/DiffViewer.tsx

🧰 Additional context used

📓 Path-based instructions (6)

apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc
Use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from @trpc/server/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/components/DiffViewer/DiffViewer.tsx

**/*.{ts,tsx}