mirror of
https://github.com/stablyai/orca.git
synced 2026-10-31 00:57:45 +00:00
fix(diff): restore the combined diff view when its tab is reopened (#30943)
Tab-scoped combined-diff view keys include the tab id, which is fresh for every opened tab, so closing "All Changes" and reopening it (View all or reopen-closed-tab) always started at the top. A fresh tab now inherits the latest view a closed tab left for the same diff in the same workspace. Views of mounted viewers or still-open inactive tabs are never taken, the entry is moved rather than copied, and older closed views of that diff are dropped.
This commit is contained in:
@@ -180,6 +180,7 @@ export function EditorContent({
|
||||
key={viewStateScopeId}
|
||||
file={activeFile}
|
||||
viewStateKey={diffViewStateKey}
|
||||
viewStateScopeId={viewStateScopeId}
|
||||
/>
|
||||
)
|
||||
}
|
||||
|
||||
@@ -19,6 +19,7 @@ import { useCombinedDiffSectionRetry } from './load-sections/use-combined-diff-s
|
||||
import { useCombinedDiffSectionRevalidation } from './load-sections/use-combined-diff-section-revalidation'
|
||||
import { useCombinedDiffViewPersist } from './remember-view/use-combined-diff-view-persist'
|
||||
import { useCombinedDiffViewRestore } from './remember-view/use-combined-diff-view-restore'
|
||||
import { buildCombinedDiffViewTabSuccession } from './remember-view/combined-diff-view-tab-scope'
|
||||
import { useCombinedDiffDirectScrollInput } from './scroll-viewport/use-combined-diff-direct-scroll-input'
|
||||
import { useCombinedDiffScrollAnchors } from './scroll-viewport/use-combined-diff-scroll-anchors'
|
||||
import { useCombinedDiffScrollPersistence } from './scroll-viewport/use-combined-diff-scroll-persistence'
|
||||
@@ -42,10 +43,12 @@ import { useDocumentDarkTheme } from '@/hooks/use-document-dark-theme'
|
||||
|
||||
export default function CombinedDiffViewer({
|
||||
file,
|
||||
viewStateKey
|
||||
viewStateKey,
|
||||
viewStateScopeId
|
||||
}: {
|
||||
file: OpenFile
|
||||
viewStateKey: string
|
||||
viewStateScopeId?: string
|
||||
}): React.JSX.Element {
|
||||
const settings = useAppStore((s) => s.settings)
|
||||
const gitStatusEntries = useAppStore(
|
||||
@@ -104,6 +107,7 @@ export default function CombinedDiffViewer({
|
||||
setSectionHeights,
|
||||
setSections,
|
||||
setSideBySide: preferences.setSideBySide,
|
||||
succession: buildCombinedDiffViewTabSuccession(file.id, viewStateScopeId),
|
||||
viewStateKey,
|
||||
worktreeId: file.worktreeId
|
||||
})
|
||||
|
||||
+115
@@ -0,0 +1,115 @@
|
||||
import { afterEach, describe, expect, it } from 'vitest'
|
||||
import {
|
||||
claimCombinedDiffView,
|
||||
clearCombinedDiffViewMemory,
|
||||
forgetCombinedDiffViewsForWorktrees,
|
||||
readCombinedDiffViewMemory,
|
||||
releaseCombinedDiffView,
|
||||
rememberCombinedDiffView,
|
||||
type CombinedDiffViewSuccession
|
||||
} from './combined-diff-view-cache'
|
||||
|
||||
const worktreeId = 'repo::/workspace/a'
|
||||
const contentKey = `${worktreeId}::all-diffs::uncommitted`
|
||||
|
||||
function succession(
|
||||
scopeId: string,
|
||||
openScopes: readonly string[] = [scopeId],
|
||||
content = contentKey
|
||||
): CombinedDiffViewSuccession {
|
||||
return { contentKey: content, scopeId, isScopeOpen: (id) => openScopes.includes(id) }
|
||||
}
|
||||
|
||||
function keyFor(scopeId: string, content = contentKey): string {
|
||||
return `${content}::${scopeId}`
|
||||
}
|
||||
|
||||
/** Mounts a tab viewer, scrolls it, then unmounts it as a tab close would. */
|
||||
function viewThenUnmount(scopeId: string, scrollTop: number, content = contentKey): void {
|
||||
const lease = claimCombinedDiffView(
|
||||
keyFor(scopeId, content),
|
||||
worktreeId,
|
||||
succession(scopeId, [scopeId], content)
|
||||
)
|
||||
releaseCombinedDiffView(lease)
|
||||
// Why: the viewer's unmount snapshot writes after release, so mirror that order.
|
||||
rememberCombinedDiffView(lease, { scrollTop })
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
clearCombinedDiffViewMemory()
|
||||
})
|
||||
|
||||
describe('combined diff view succession', () => {
|
||||
it('moves a closed tab view to the fresh tab that reopens the same diff', () => {
|
||||
viewThenUnmount('tab-1', 640)
|
||||
|
||||
const nextKey = keyFor('tab-2')
|
||||
expect(
|
||||
readCombinedDiffViewMemory(nextKey, { worktreeId, succession: succession('tab-2') })
|
||||
?.scrollTop
|
||||
).toBe(640)
|
||||
|
||||
claimCombinedDiffView(nextKey, worktreeId, succession('tab-2'))
|
||||
|
||||
expect(readCombinedDiffViewMemory(nextKey)?.scrollTop).toBe(640)
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-1'))).toBeUndefined()
|
||||
})
|
||||
|
||||
it('keeps a still-open but inactive tab view out of reach of a second tab', () => {
|
||||
viewThenUnmount('tab-1', 640)
|
||||
|
||||
const nextKey = keyFor('tab-2')
|
||||
const bothOpen = succession('tab-2', ['tab-1', 'tab-2'])
|
||||
expect(readCombinedDiffViewMemory(nextKey, { worktreeId, succession: bothOpen })).toBe(
|
||||
undefined
|
||||
)
|
||||
claimCombinedDiffView(nextKey, worktreeId, bothOpen)
|
||||
|
||||
expect(readCombinedDiffViewMemory(nextKey)).toBeUndefined()
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-1'))?.scrollTop).toBe(640)
|
||||
})
|
||||
|
||||
it('never takes the view of a viewer that is still mounted', () => {
|
||||
const mounted = claimCombinedDiffView(keyFor('tab-1'), worktreeId, succession('tab-1'))
|
||||
rememberCombinedDiffView(mounted, { scrollTop: 320 })
|
||||
|
||||
// The mounted tab's scope already reads as closed, so only the lease protects it.
|
||||
claimCombinedDiffView(keyFor('tab-2'), worktreeId, succession('tab-2'))
|
||||
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-2'))).toBeUndefined()
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-1'))?.scrollTop).toBe(320)
|
||||
})
|
||||
|
||||
it('inherits the latest closed view and drops older ones for the same diff', () => {
|
||||
viewThenUnmount('tab-1', 100)
|
||||
viewThenUnmount('tab-2', 200)
|
||||
|
||||
claimCombinedDiffView(keyFor('tab-3'), worktreeId, succession('tab-3'))
|
||||
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-3'))?.scrollTop).toBe(200)
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-1'))).toBeUndefined()
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-2'))).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not inherit across different diffs or workspaces', () => {
|
||||
const stagedKey = `${contentKey}::staged`
|
||||
viewThenUnmount('tab-1', 100, stagedKey)
|
||||
|
||||
claimCombinedDiffView(keyFor('tab-2'), worktreeId, succession('tab-2'))
|
||||
claimCombinedDiffView(keyFor('tab-3'), 'repo::/workspace/b', succession('tab-3'))
|
||||
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-2'))).toBeUndefined()
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-3'))).toBeUndefined()
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-1', stagedKey))?.scrollTop).toBe(100)
|
||||
})
|
||||
|
||||
it('leaves nothing to inherit once the workspace is forgotten', () => {
|
||||
viewThenUnmount('tab-1', 640)
|
||||
forgetCombinedDiffViewsForWorktrees([worktreeId])
|
||||
|
||||
claimCombinedDiffView(keyFor('tab-2'), worktreeId, succession('tab-2'))
|
||||
|
||||
expect(readCombinedDiffViewMemory(keyFor('tab-2'))).toBeUndefined()
|
||||
})
|
||||
})
|
||||
+86
-7
@@ -13,14 +13,27 @@ export type CachedCombinedDiffViewState = {
|
||||
|
||||
type CombinedDiffViewMemory = {
|
||||
worktreeId: string
|
||||
succession?: CombinedDiffViewSuccessionTag
|
||||
view?: CachedCombinedDiffViewState
|
||||
scrollTop?: number
|
||||
scrollAnchor?: NonNullable<VirtualizedScrollAnchor>
|
||||
}
|
||||
|
||||
/** The diff a tab-scoped key shows and the tab it belongs to. */
|
||||
export type CombinedDiffViewSuccessionTag = {
|
||||
contentKey: string
|
||||
scopeId: string
|
||||
}
|
||||
|
||||
/** Lets a fresh tab inherit the view its closed predecessor left for the same diff. */
|
||||
export type CombinedDiffViewSuccession = CombinedDiffViewSuccessionTag & {
|
||||
isScopeOpen: (scopeId: string) => boolean
|
||||
}
|
||||
|
||||
/** A mounted viewer's write access to its key; revoked only when its workspace is forgotten. */
|
||||
export type CombinedDiffViewLease = {
|
||||
readonly viewKey: string
|
||||
readonly succession?: CombinedDiffViewSuccessionTag
|
||||
worktreeId: string
|
||||
revoked: boolean
|
||||
}
|
||||
@@ -28,17 +41,79 @@ export type CombinedDiffViewLease = {
|
||||
const memoryByViewKey = new Map<string, CombinedDiffViewMemory>()
|
||||
const mountedLeases = new Set<CombinedDiffViewLease>()
|
||||
|
||||
export function readCombinedDiffViewMemory(
|
||||
viewKey: string
|
||||
): Readonly<CombinedDiffViewMemory> | undefined {
|
||||
return memoryByViewKey.get(viewKey)
|
||||
function isMountedViewKey(viewKey: string): boolean {
|
||||
for (const lease of mountedLeases) {
|
||||
if (lease.viewKey === viewKey) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
export function claimCombinedDiffView(viewKey: string, worktreeId: string): CombinedDiffViewLease {
|
||||
// Why: tab ids are fresh per opened tab, so a reopened tab only finds its view through the closed tab's entry.
|
||||
// Open-but-inactive tabs are unmounted too, so the scope check keeps another live tab's view from being taken.
|
||||
function findInheritableViewKeys(
|
||||
viewKey: string,
|
||||
worktreeId: string,
|
||||
succession: CombinedDiffViewSuccession
|
||||
): string[] {
|
||||
const keys: string[] = []
|
||||
for (const [candidateKey, memory] of memoryByViewKey) {
|
||||
if (
|
||||
candidateKey !== viewKey &&
|
||||
memory.worktreeId === worktreeId &&
|
||||
memory.succession?.contentKey === succession.contentKey &&
|
||||
!succession.isScopeOpen(memory.succession.scopeId) &&
|
||||
!isMountedViewKey(candidateKey)
|
||||
) {
|
||||
keys.push(candidateKey)
|
||||
}
|
||||
}
|
||||
// Map order is LRU order, so the last key is the most recently written view.
|
||||
return keys
|
||||
}
|
||||
|
||||
/** Without `inherit` this reads only `viewKey`; with it, a key that has no view yet previews what it would inherit. */
|
||||
export function readCombinedDiffViewMemory(
|
||||
viewKey: string,
|
||||
inherit?: { worktreeId: string; succession: CombinedDiffViewSuccession }
|
||||
): Readonly<CombinedDiffViewMemory> | undefined {
|
||||
const own = memoryByViewKey.get(viewKey)
|
||||
if (own || !inherit) {
|
||||
return own
|
||||
}
|
||||
const inheritedKey = findInheritableViewKeys(viewKey, inherit.worktreeId, inherit.succession).at(
|
||||
-1
|
||||
)
|
||||
return inheritedKey === undefined ? undefined : memoryByViewKey.get(inheritedKey)
|
||||
}
|
||||
|
||||
export function claimCombinedDiffView(
|
||||
viewKey: string,
|
||||
worktreeId: string,
|
||||
succession?: CombinedDiffViewSuccession
|
||||
): CombinedDiffViewLease {
|
||||
if (memoryByViewKey.get(viewKey)?.worktreeId !== worktreeId) {
|
||||
memoryByViewKey.delete(viewKey)
|
||||
}
|
||||
const lease = { viewKey, worktreeId, revoked: false }
|
||||
const tag = succession ? { contentKey: succession.contentKey, scopeId: succession.scopeId } : null
|
||||
if (succession && tag && !memoryByViewKey.has(viewKey)) {
|
||||
const inheritableKeys = findInheritableViewKeys(viewKey, worktreeId, succession)
|
||||
const inherited = memoryByViewKey.get(inheritableKeys.at(-1) ?? '')
|
||||
// Why: move rather than copy, and drop older closed tabs' views of this diff — they hold file bodies nothing can reach.
|
||||
for (const key of inheritableKeys) {
|
||||
memoryByViewKey.delete(key)
|
||||
}
|
||||
if (inherited) {
|
||||
setWithLRU(memoryByViewKey, viewKey, { ...inherited, succession: tag })
|
||||
}
|
||||
}
|
||||
const lease: CombinedDiffViewLease = {
|
||||
viewKey,
|
||||
...(tag ? { succession: tag } : {}),
|
||||
worktreeId,
|
||||
revoked: false
|
||||
}
|
||||
mountedLeases.add(lease)
|
||||
return lease
|
||||
}
|
||||
@@ -57,7 +132,11 @@ export function rememberCombinedDiffView(
|
||||
}
|
||||
const memory = memoryByViewKey.get(lease.viewKey)
|
||||
const owned = memory?.worktreeId === lease.worktreeId ? memory : { worktreeId: lease.worktreeId }
|
||||
setWithLRU(memoryByViewKey, lease.viewKey, { ...owned, ...patch })
|
||||
setWithLRU(memoryByViewKey, lease.viewKey, {
|
||||
...owned,
|
||||
...patch,
|
||||
...(lease.succession ? { succession: lease.succession } : {})
|
||||
})
|
||||
}
|
||||
|
||||
export function forgetCombinedDiffViewsShowingPath(relativePath: string): void {
|
||||
|
||||
+19
@@ -0,0 +1,19 @@
|
||||
import { useAppStore } from '@/store'
|
||||
import type { CombinedDiffViewSuccession } from './combined-diff-view-cache'
|
||||
|
||||
function isOpenUnifiedTab(tabId: string): boolean {
|
||||
return Object.values(useAppStore.getState().unifiedTabsByWorktree).some((tabs) =>
|
||||
tabs.some((tab) => tab.id === tabId)
|
||||
)
|
||||
}
|
||||
|
||||
/** Tab-scoped viewers inherit from closed tabs of the same diff; the unscoped legacy key is already stable. */
|
||||
export function buildCombinedDiffViewTabSuccession(
|
||||
contentKey: string,
|
||||
viewStateScopeId: string | undefined
|
||||
): CombinedDiffViewSuccession | undefined {
|
||||
if (viewStateScopeId === undefined || viewStateScopeId === contentKey) {
|
||||
return undefined
|
||||
}
|
||||
return { contentKey, scopeId: viewStateScopeId, isScopeOpen: isOpenUnifiedTab }
|
||||
}
|
||||
+63
-2
@@ -10,8 +10,10 @@ import type { CombinedDiffEntrySet } from '../resolve-changes/use-combined-diff-
|
||||
import {
|
||||
claimCombinedDiffView,
|
||||
clearCombinedDiffViewMemory,
|
||||
readCombinedDiffViewMemory,
|
||||
releaseCombinedDiffView,
|
||||
rememberCombinedDiffView
|
||||
rememberCombinedDiffView,
|
||||
type CombinedDiffViewSuccession
|
||||
} from './combined-diff-view-cache'
|
||||
import { useCombinedDiffViewRestore } from './use-combined-diff-view-restore'
|
||||
import { createStore } from 'zustand/vanilla'
|
||||
@@ -42,7 +44,11 @@ function buildAllModeEntrySet(
|
||||
}
|
||||
}
|
||||
|
||||
function restoreSections(entrySet: CombinedDiffEntrySet, viewStateKey: string): DiffSection[] {
|
||||
function restoreSections(
|
||||
entrySet: CombinedDiffEntrySet,
|
||||
viewStateKey: string,
|
||||
succession?: CombinedDiffViewSuccession
|
||||
): DiffSection[] {
|
||||
let sections: DiffSection[] = []
|
||||
renderHook(() => {
|
||||
const registry = useCombinedDiffSectionLoadRegistry([])
|
||||
@@ -56,6 +62,7 @@ function restoreSections(entrySet: CombinedDiffEntrySet, viewStateKey: string):
|
||||
sections = typeof value === 'function' ? value(sections) : value
|
||||
},
|
||||
setSideBySide: () => {},
|
||||
succession,
|
||||
viewStateKey,
|
||||
worktreeId: 'wt-1'
|
||||
})
|
||||
@@ -147,6 +154,60 @@ describe('useCombinedDiffViewRestore deferral', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('restores a closed tab-scoped review in the fresh tab that reopens it', () => {
|
||||
const entrySet = buildAllModeEntrySet(
|
||||
[{ path: 'src/app.ts', status: 'modified', area: 'unstaged', added: 5 }],
|
||||
[]
|
||||
)
|
||||
const contentKey = 'wt-1::all-diffs::uncommitted'
|
||||
const tabSuccession = (scopeId: string): CombinedDiffViewSuccession => ({
|
||||
contentKey,
|
||||
scopeId,
|
||||
// Only the reopened tab exists; the first tab was closed.
|
||||
isScopeOpen: (id) => id === 'tab-reopened'
|
||||
})
|
||||
const loadedSections: DiffSection[] = [
|
||||
{
|
||||
key: 'unstaged:src/app.ts',
|
||||
path: 'src/app.ts',
|
||||
status: 'modified',
|
||||
area: 'unstaged',
|
||||
added: 5,
|
||||
originalContent: 'before',
|
||||
modifiedContent: 'after',
|
||||
collapsed: false,
|
||||
loading: false,
|
||||
dirty: false,
|
||||
diffResult: null,
|
||||
largeDiffRenderLimit: null
|
||||
}
|
||||
]
|
||||
const lease = claimCombinedDiffView(
|
||||
`${contentKey}::tab-closed`,
|
||||
'wt-1',
|
||||
tabSuccession('tab-closed')
|
||||
)
|
||||
rememberCombinedDiffView(lease, {
|
||||
scrollTop: 900,
|
||||
view: {
|
||||
entrySignature: entrySet.entrySignature,
|
||||
gitStatusSignature: '',
|
||||
sections: loadedSections,
|
||||
sectionHeights: {},
|
||||
loadedIndices: [0],
|
||||
sideBySide: false
|
||||
}
|
||||
})
|
||||
releaseCombinedDiffView(lease)
|
||||
|
||||
const reopenedKey = `${contentKey}::tab-reopened`
|
||||
expect(restoreSections(entrySet, reopenedKey, tabSuccession('tab-reopened'))).toEqual(
|
||||
loadedSections
|
||||
)
|
||||
expect(readCombinedDiffViewMemory(reopenedKey)?.scrollTop).toBe(900)
|
||||
expect(readCombinedDiffViewMemory(`${contentKey}::tab-closed`)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('auto-loads an uncounted tracked row once its own pass counted something', () => {
|
||||
const sections = restoreSections(
|
||||
buildAllModeEntrySet(
|
||||
|
||||
+25
-4
@@ -21,7 +21,8 @@ import {
|
||||
readCombinedDiffViewMemory,
|
||||
releaseCombinedDiffView,
|
||||
rememberCombinedDiffView,
|
||||
type CombinedDiffViewLease
|
||||
type CombinedDiffViewLease,
|
||||
type CombinedDiffViewSuccession
|
||||
} from './combined-diff-view-cache'
|
||||
|
||||
export type CombinedDiffViewRestore = {
|
||||
@@ -40,6 +41,7 @@ export function useCombinedDiffViewRestore({
|
||||
setSectionHeights,
|
||||
setSections,
|
||||
setSideBySide,
|
||||
succession,
|
||||
viewStateKey,
|
||||
worktreeId
|
||||
}: {
|
||||
@@ -50,6 +52,8 @@ export function useCombinedDiffViewRestore({
|
||||
setSectionHeights: React.Dispatch<React.SetStateAction<Record<number, number>>>
|
||||
setSections: React.Dispatch<React.SetStateAction<DiffSection[]>>
|
||||
setSideBySide: React.Dispatch<React.SetStateAction<boolean>>
|
||||
/** Set for tab-scoped keys so a reopened tab restores the view its closed predecessor left. */
|
||||
succession?: CombinedDiffViewSuccession
|
||||
viewStateKey: string
|
||||
worktreeId: string
|
||||
}): CombinedDiffViewRestore {
|
||||
@@ -75,7 +79,11 @@ export function useCombinedDiffViewRestore({
|
||||
// throws the result away, and an anchor seeds legitimately to null so a nullish guard would keep
|
||||
// re-reading. useState's initializer runs once without writing a ref during render.
|
||||
const [restoreSeed] = useState<{ offset: number; anchor: VirtualizedScrollAnchor }>(() => {
|
||||
const memory = readCombinedDiffViewMemory(viewStateKey)
|
||||
// Why: preview the inherited view here — the claim below moves it only after this first render seeds the virtualizer.
|
||||
const memory = readCombinedDiffViewMemory(
|
||||
viewStateKey,
|
||||
succession ? { worktreeId, succession } : undefined
|
||||
)
|
||||
return { offset: memory?.scrollTop ?? 0, anchor: memory?.scrollAnchor ?? null }
|
||||
})
|
||||
const scrollOffsetRef = useRef(restoreSeed.offset)
|
||||
@@ -84,11 +92,24 @@ export function useCombinedDiffViewRestore({
|
||||
|
||||
// Why: keep above CombinedDiffViewer's persist/scroll hooks so the lease exists before their effects write.
|
||||
const memoryLeaseRef = useRef<CombinedDiffViewLease | null>(null)
|
||||
const successionContentKey = succession?.contentKey
|
||||
const successionScopeId = succession?.scopeId
|
||||
const isSuccessionScopeOpen = succession?.isScopeOpen
|
||||
useLayoutEffect(() => {
|
||||
const lease = claimCombinedDiffView(viewStateKey, worktreeId)
|
||||
const lease = claimCombinedDiffView(
|
||||
viewStateKey,
|
||||
worktreeId,
|
||||
successionContentKey !== undefined && successionScopeId !== undefined && isSuccessionScopeOpen
|
||||
? {
|
||||
contentKey: successionContentKey,
|
||||
scopeId: successionScopeId,
|
||||
isScopeOpen: isSuccessionScopeOpen
|
||||
}
|
||||
: undefined
|
||||
)
|
||||
memoryLeaseRef.current = lease
|
||||
return () => releaseCombinedDiffView(lease)
|
||||
}, [viewStateKey, worktreeId])
|
||||
}, [isSuccessionScopeOpen, successionContentKey, successionScopeId, viewStateKey, worktreeId])
|
||||
|
||||
// Why: tab/worktree switches unmount this viewer; cache by pane key so remount restores sections+scroll before repaint.
|
||||
const initializedEntryStateRef = useRef<{
|
||||
|
||||
Reference in New Issue
Block a user