Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { useCombinedDiffSectionRevalidation } from './load-sections/use-combined
import { useCombinedDiffViewPersist } from './remember-view/use-combined-diff-view-persist'
import { useCombinedDiffViewRestore } from './remember-view/use-combined-diff-view-restore'
import { useCombinedDiffDirectScrollInput } from './scroll-viewport/use-combined-diff-direct-scroll-input'
import { planCombinedDiffSectionToggle } from './scroll-viewport/combined-diff-collapse-scroll-anchor'
import { useCombinedDiffScrollAnchors } from './scroll-viewport/use-combined-diff-scroll-anchors'
import { useCombinedDiffScrollPersistence } from './scroll-viewport/use-combined-diff-scroll-persistence'
import { useCombinedDiffScrollbar } from './scroll-viewport/use-combined-diff-scrollbar'
Expand Down Expand Up @@ -163,15 +164,17 @@ export default function CombinedDiffViewer({

const toggleSection = useCallback(
(index: number) => {
const shouldLoadAfterExpand = registry.sectionsRef.current[index]?.collapsed ?? false
setSections((prev) =>
prev.map((s, i) => (i === index ? { ...s, collapsed: !s.collapsed } : s))
)
if (shouldLoadAfterExpand) {
const plan = planCombinedDiffSectionToggle({
index,
sections: registry.sectionsRef.current,
anchor: restore.scrollAnchorRef.current
})
setSections((prev) => plan.next(prev))
if (plan.shouldLoadAfterExpand) {
registry.loadSchedulerRef.current.request(index)
}
},
[registry.loadSchedulerRef, registry.sectionsRef]
[registry.loadSchedulerRef, registry.sectionsRef, restore.scrollAnchorRef]
)

const treeNavigation = useCombinedDiffTreeNavigation({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { describe, expect, it } from 'vitest'
import type { VirtualizedScrollAnchor } from '@/hooks/useVirtualizedScrollAnchor'
import {
pinScrollAnchorWhenCollapsingSection,
planCombinedDiffSectionToggle
} from './combined-diff-collapse-scroll-anchor'

function anchor(offset: number): NonNullable<VirtualizedScrollAnchor> {
return { key: 'src/app.ts', offset, scrollTop: 2_000 }
}

describe('pinScrollAnchorWhenCollapsingSection', () => {
it('pins the header when the anchored section is the one being collapsed', () => {
Comment thread
pullfrog[bot] marked this conversation as resolved.
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', () => {
Comment on lines +13 to +21

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Viewport behavior remains untested. This test checks that the helper changes the anchor, but not that collapsing a section keeps its header in view. The scroll restore can still be skipped after the anchor changes, so that viewport regression could return without this test failing. Please test a collapse from a nonzero position inside the section and assert where the viewport lands.

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const current = anchor(0)
pinScrollAnchorWhenCollapsingSection(current, { collapsed: true, key: 'src/app.ts' })
expect(current.offset).toBe(0)
})

it('leaves the anchor when a different section collapses', () => {
const current = anchor(400)
pinScrollAnchorWhenCollapsingSection(current, { collapsed: false, key: 'src/other.ts' })
expect(current.offset).toBe(400)
})

it('pins a mid-section anchor before the collapsed flag is published', () => {
const current = anchor(1_800)
const open = { collapsed: false, key: 'src/app.ts', marker: 'body' }
const plan = planCombinedDiffSectionToggle({
index: 0,
sections: [open],
anchor: current
})

expect(current.offset).toBe(0)
expect(current.scrollTop).toBe(2_000)
expect(plan.shouldLoadAfterExpand).toBe(false)

const published = plan.next([open])
expect(published[0]?.collapsed).toBe(true)
expect(published[0]?.marker).toBe('body')
})

it('does nothing without an anchor or a section', () => {
expect(() => pinScrollAnchorWhenCollapsingSection(null, undefined)).not.toThrow()
const current = anchor(40)
pinScrollAnchorWhenCollapsingSection(current, undefined)
expect(current.offset).toBe(40)
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import type { VirtualizedScrollAnchor } from '@/hooks/useVirtualizedScrollAnchor'

type CollapsibleSection = {
collapsed: boolean
key: string
}

/**
* Collapsing the section the viewport is inside drops that section's body.
* The recorded offset still points into the body, so restore lands on later
* sections. Pin the header (offset 0) and leave scrollTop alone: restore
* treats a matching scrollTop as "the user has not moved" and then applies
* the offset. Expanding, or collapsing some other section, keeps the anchor
* so a later section stays where it was.
*/
export function pinScrollAnchorWhenCollapsingSection(
anchor: VirtualizedScrollAnchor,
section: CollapsibleSection | undefined
): void {
if (!anchor || !section || section.collapsed || anchor.key !== section.key) {
return
}
anchor.offset = 0
}

export function planCombinedDiffSectionToggle<T extends CollapsibleSection>(input: {
index: number
sections: readonly T[]
anchor: VirtualizedScrollAnchor
}): {
next: (sections: readonly T[]) => T[]
shouldLoadAfterExpand: boolean
} {
const section = input.sections[input.index]
const shouldLoadAfterExpand = section?.collapsed ?? false
pinScrollAnchorWhenCollapsingSection(input.anchor, section)
return {
shouldLoadAfterExpand,
next: (sections) =>
sections.map((row, index) =>
index === input.index ? { ...row, collapsed: !row.collapsed } : row
)
}
}