[codex] Fix changes pane scroll to file headers#4615
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 4bdc0ed on May 16, 2026 12:33am UTC. |
|
Caution Review failedPull request was closed or merged during review 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 (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe diff pane component hierarchy is refactored to separate header rendering from diff content. WorkspaceDiff's public contract is narrowed to accept ChangesDiff Component Header Extraction and Scrolling
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Greptile SummaryThis PR fixes scroll-to-file in the changes pane by lifting
Confidence Score: 5/5Safe to merge — the change is a targeted refactoring that moves header rendering out of the lazily-mounted WorkspaceDiff into DiffFileEntry, with no logic changes to data fetching or state management. The scroll fix is mechanically sound: the new header attribute is emitted by every render of DiffFileHeader (including the DeferredDiffPlaceholder path), so ScrollToFile will always find both query targets when a file entry is in the DOM. State (collapsed, viewed, expandUnchanged) flows correctly through the lifted header. WorkspaceDiff is only trimmed of props it no longer needs. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx | ScrollToFile now queries a dedicated header attribute for the scroll offset and falls back to the entry wrapper only for line-element lookup; logic is correct and handles both deferred and normal paths. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileEntry/DiffFileEntry.tsx | Header lifted out of WorkspaceDiff; rendered unconditionally before the lazy shouldMount guard, ensuring it's always in the DOM. DeferredDiffPlaceholder retains its own DiffFileHeader call and inherits the new attribute automatically. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx | Minimal change: adds data-diff-entry-header-path attribute to the root div; no logic changes. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/WorkspaceDiff/WorkspaceDiff.tsx | DiffFileHeader and all its related props removed; component now only manages diff body and comment annotations. |
Sequence Diagram
sequenceDiagram
participant CP as Changes Pane
participant SF as ScrollToFile
participant DOM as DOM
participant DFE as DiffFileEntry
participant DFH as DiffFileHeader
participant WD as WorkspaceDiff
CP->>SF: path change / focusTick change
SF->>DOM: querySelector([data-diff-path])
DOM-->>SF: entry element
SF->>DOM: querySelector([data-diff-entry-header-path])
DOM-->>SF: header element
Note over SF: Abort if either missing
SF->>SF: getOffsetInScrollContainer(header)
SF->>DOM: scrollTo(headerOffset)
Note over SF: focusLine + tickChanged?
SF->>SF: findLineElement(entry, focusLine)
SF->>DOM: lineEl.scrollIntoView(center)
Note over DFE,WD: Render path (after this PR)
DFE->>DFH: always render header (data-diff-entry-header-path)
DFE->>WD: "render only when shouldMount=true"
WD-->>DFE: diff body + comment annotations
Reviews (1): Last reviewed commit: "fix(desktop): scroll changes pane to fil..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
DiffFileEntryeven before the lazy diff body mounts.WorkspaceDiffso it only owns the diff body and comment annotations.Root Cause
The changes pane click path selected the file entry wrapper as the scroll target. When comment annotations or lazy-mounted diff content changed the entry layout, the jump could land relative to the comment/body area instead of consistently landing on the file entry header.
Impact
Clicking a file in the changes pane now consistently lands at that file's header. Comment-focused jumps still use the entry root for line lookup after the header jump.
Validation
bunx biome checkon the touched diff-pane filesbun run --cwd apps/desktop typecheckbun run lint:fixbun run lintSummary by cubic
Scroll jumps from the changes pane now land on each file’s header for stable positioning, even when diff content or comments mount later. The file header always renders;
WorkspaceDiffnow only renders the diff body and annotations.Bug Fixes
[data-diff-entry-header-path]) as the scroll target instead of the entry wrapper.Refactors
DiffFileHeaderunconditionally inDiffFileEntry; lazy‑mount only the diff body.WorkspaceDiff.Written for commit 4bdc0ed. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Refactor