Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — the fix is sound and correctly scoped. Two follow-ups worth a look before merge.
Reviewed changes
- Pin the collapsed section's header — new pure helper
pinScrollAnchorWhenCollapsingSectionzeroes the scroll anchor's within-section offset when the anchored section is the one collapsing, andCombinedDiffViewer.toggleSectioncalls it before flippingcollapsed. - Scope guards — expanding, and collapsing a section other than the anchor, leave the anchor untouched so a later section stays put.
- Unit test — covers the helper's four branches (pin, expand, other section, missing inputs).
The identity test is sound: anchor keys are section.key for both virtual and DOM recording, and restore applies the offset verbatim for an exact key, so offset = 0 does pin the header. Calling it before setSections is also correct — the resulting structureRevision bump changes the restore signal, and the layout-phase restore runs before paint.
ℹ️ "Collapse All" still exhibits the original jump
The toolbar's Collapse All path sets every section's collapsed flag directly and never routes through toggleSection, so pinScrollAnchorWhenCollapsingSection is not called. The stale non-zero anchor offset survives, restore resolves it against the now header-only row, and the viewport lands past it — the same behavior #20665 reported, just via a different entry point. If collapsing one section at the current view should keep its header visible, Collapse All likely wants the same treatment.
Technical details
# Collapse All bypasses the new pin
## Affected sites
- `src/renderer/src/components/editor/combined-diff/review-controls/use-combined-diff-view-preferences.ts:67-82` — `setAllSectionsCollapsed` does `setSections((prev) => prev.map((section) => ({ ...section, collapsed })))` without pinning the anchor.
- Wired at `CombinedDiffViewer.tsx:343` (`setAllSectionsCollapsed={preferences.setAllSectionsCollapsed}`), invoked from `combined-diff-toolbar.tsx:168`.
## Required outcome
- Collapsing the anchored section via Collapse All keeps that section's header in view (same intent as the single-toggle fix).
## Suggested approach (optional)
- Thread `restore.scrollAnchorRef` (and the `sectionsRef`) into `useCombinedDiffViewPreferences`, or move the pin to a shared callback the toolbar handler calls before `setSections`. Only pin when `collapsed === true`.
- Note the guard must run before the state update, matching `toggleSection`.
## Open questions for the human
- Is preserving the anchor section's header the desired outcome for Collapse All, or should it stay wherever the browser reflows since all rows become headers?ℹ️ Chosen behavior differs from the issue's requested fix
#20665 asks for GitHub "Viewed"-parity: on collapse, jump to the next section's header. This PR instead pins the collapsed section's own header at the top, which the author documents in the PR's tradeoffs. That is a defensible choice for a manual header click and definitely beats the arbitrary jump, but it does not deliver the behavior the issue requested — worth confirming with the reporter/product which is intended before closing the issue.
Technical details
# Pin-header vs jump-to-next-section
## Affected sites
- `src/renderer/src/components/editor/combined-diff/scroll-viewport/combined-diff-collapse-scroll-anchor.ts:16-24` — forces `offset = 0` on the collapsed (anchored) section.
- Issue #20665 "Expected behavior" requests scrolling so the *next* section's header lands at the top.
## Open questions for the human
- Should collapsing jump to the next section's header (issue's ask), or keep the collapsed header pinned (this PR)? The two produce different viewport positions.
- If jump-to-next is wanted, the anchor would need to move to the next section key rather than zeroing the current offset.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('pins the header when the anchored section is the one being collapsed', () => { | ||
| const current = anchor(1_800) | ||
| pinScrollAnchorWhenCollapsingSection(current, { collapsed: false, key: 'src/app.ts' }) | ||
| expect(current.offset).toBe(0) | ||
| expect(current.scrollTop).toBe(2_000) | ||
| expect(current.key).toBe('src/app.ts') | ||
| }) | ||
|
|
||
| it('leaves the anchor when that section is expanding', () => { |
There was a problem hiding this comment.
Knowledge Base Used: Desktop renderer experience
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!
There was a problem hiding this comment.
The viewer calls planCombinedDiffSectionToggle before setSections. That plan pins a mid-body anchor to offset 0 before it returns the updater that flips collapsed, and the new test fails if that pin has not happened yet. Five anchor tests passed. I did not render CombinedDiffViewer, so this does not assert the viewport pixel position.
There was a problem hiding this comment.
That addresses the ordering invariant: the anchor is pinned before the updater publishes the collapsed section, while the updater still preserves the section data and only toggles collapsed.
You’re also right that this does not assert the rendered pixel position; it verifies the helper’s input to the existing structural restore rather than exercising the Electron viewport. Given the focused scope and the five anchor tests, I consider the unit coverage sufficient and withdraw the request for an additional viewer-render test.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change preserves the collapsed section’s own header rather than navigating to the next header. Source inspection supports the intended behavior; no merge-blocking issue was identified. Live viewport behavior remains untested. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Capture the next section or equivalent scroll anchor before collapse. Restore the viewport to that section's header after reflow. Add automated tests for a collapsed section above or at the viewed area, including a long section, and verify the next header position.
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 |
Collapsing the section the viewport is inside left the scroll anchor's within-section offset pointing at a body that no longer exists, so restore landed on later sections. Pin that anchor to the header before the collapse flips. Collapsing or expanding any other section keeps its anchor. Fixes stablyai#20665
The viewer asks for that plan before it publishes the collapsed flag, and the regression fails if the header pin happens later. Co-authored-by: Cursor <cursoragent@cursor.com>
930a554 to
c20289d
Compare
Sync update (
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Toggle extracted into a testable plan —
planCombinedDiffSectionTogglenow computes the anchor pin and theshouldLoadAfterExpandread and returns the state updater in one call, sotoggleSectionbuilds the plan beforesetSections. The pin-before-publish ordering is now a tested contract of the plan rather than an incidental line ordering. - Ordering regression test added — asserts the anchor reaches
offset 0at plan-construction time, beforeplan.nextpublishes the collapsed flag, and that unrelated row fields survive the update. This can fail if the pin is deferred or dropped, addressing the prior review's test-coverage thread.
No inline comments: the incremental delta is a behavior-preserving refactor plus the ordering test, and the earlier open question about the Collapse All path (a separate entry point that does not route through the toggle) remains a product-scoping call rather than a defect in this change.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
Description
In the View all diff, collapsing the section the viewport is inside left the scroll anchor pointing at an offset inside that section's body. The body is gone after the collapse, so restore kept the old offset and the viewport landed on later sections.
Focused fix
scrollTopon the anchor is left unchanged, because restore only runs when the viewport still matches the recorded scroll position.Preserves
Collapse still toggles the same section and still loads a section when it expands. An anchor on a later section is not rewritten, so collapsing a section above the one being read still restores that later section.
Evidence
node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts --cache false src/renderer/src/components/editor/combined-diff/scroll-viewport/combined-diff-collapse-scroll-anchor.test.ts4 passed. oxlint on the three changed files reported 0 warnings. The live Electron diff view was not rechecked.
User-regression-tradeoffs
A user who was scrolled into a long section and then collapses it sees that section's header at the top of the viewport, rather than the later files the old offset jumped to. A user reading a different section is unaffected.
Fixes #20665