From 4b6fd9283ab7cdb06bd327d2ddce8cf2eecc3e83 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 19 Oct 2026 21:00:58 -0700 Subject: [PATCH] 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. --- .../src/components/editor/EditorContent.tsx | 1 + .../combined-diff/CombinedDiffViewer.tsx | 6 +- .../combined-diff-view-cache.test.ts | 115 ++++++++++++++++++ .../remember-view/combined-diff-view-cache.ts | 93 ++++++++++++-- .../combined-diff-view-tab-scope.ts | 19 +++ .../use-combined-diff-view-restore.test.tsx | 65 +++++++++- .../use-combined-diff-view-restore.ts | 29 ++++- 7 files changed, 314 insertions(+), 14 deletions(-) create mode 100644 src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.test.ts create mode 100644 src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-tab-scope.ts diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index f7e33c13f59..e17c5f4fe2a 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -180,6 +180,7 @@ export function EditorContent({ key={viewStateScopeId} file={activeFile} viewStateKey={diffViewStateKey} + viewStateScopeId={viewStateScopeId} /> ) } diff --git a/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx b/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx index e5d8c9b1500..c060d62682c 100644 --- a/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx +++ b/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx @@ -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 }) diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.test.ts b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.test.ts new file mode 100644 index 00000000000..0f2b6ca0ed3 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.test.ts @@ -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() + }) +}) diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.ts b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.ts index 58fac496617..8f450488087 100644 --- a/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.ts +++ b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.ts @@ -13,14 +13,27 @@ export type CachedCombinedDiffViewState = { type CombinedDiffViewMemory = { worktreeId: string + succession?: CombinedDiffViewSuccessionTag view?: CachedCombinedDiffViewState scrollTop?: number scrollAnchor?: NonNullable } +/** 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() const mountedLeases = new Set() -export function readCombinedDiffViewMemory( - viewKey: string -): Readonly | 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 | 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 { diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-tab-scope.ts b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-tab-scope.ts new file mode 100644 index 00000000000..98440b859c5 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-tab-scope.ts @@ -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 } +} diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx index f859c6718f1..9c194153e2a 100644 --- a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx +++ b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx @@ -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( diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts index f63b68f4b77..e10bb4bc499 100644 --- a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts +++ b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts @@ -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>> setSections: React.Dispatch> setSideBySide: React.Dispatch> + /** 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(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<{