From c9f4cd7e279d5e194cc34db6d1cff6b39e032a56 Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 5 Oct 2026 03:10:35 -0700 Subject: [PATCH] fix(editor): close stale duplicate documents safely Adapt the document-sibling cleanup from Pr1p's #23347 with exact owner and captured provenance checks. Preserve divergent drafts and the backing bytes needed by retained views, and keep one-pane close behavior intact. Co-authored-by: Chen --- .../editor-close-document-siblings.test.ts | 280 ++++++++++++++++++ .../editor/actions/close-file-action.ts | 185 +++++++++--- 2 files changed, 419 insertions(+), 46 deletions(-) create mode 100644 src/renderer/src/store/slices/editor-close-document-siblings.test.ts diff --git a/src/renderer/src/store/slices/editor-close-document-siblings.test.ts b/src/renderer/src/store/slices/editor-close-document-siblings.test.ts new file mode 100644 index 00000000000..334c202981d --- /dev/null +++ b/src/renderer/src/store/slices/editor-close-document-siblings.test.ts @@ -0,0 +1,280 @@ +import type { StoreApi } from 'zustand/vanilla' +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { AppState } from '../types' +import type { OpenFile } from './editor' +import { createEditorTabsStore } from './editor-slice-test-harness' +import { dispatchWorkspaceTabCommand } from '@/lib/workspace-tab-commands' +import { captureEditorFileOperationProvenance } from '@/lib/editor-file-operation-owner' + +const live = vi.hoisted((): { store: StoreApi | null } => ({ store: null })) +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => { + if (!live.store) { + throw new Error('No test store') + } + return live.store.getState() + } + } +})) + +let store: StoreApi +let file: OpenFile + +beforeEach(() => { + store = createEditorTabsStore() + live.store = store + store.getState().openFile({ + filePath: '/repo/note.md', + relativePath: 'note.md', + worktreeId: 'wt-1', + language: 'markdown', + mode: 'edit' + }) + const opened = store.getState().openFiles[0] + if (!opened) { + throw new Error('Document did not open') + } + file = opened +}) + +afterEach(() => { + live.store = null + vi.unstubAllGlobals() +}) + +function addSibling(overrides: Partial = {}): OpenFile { + const sibling = { ...file, id: 'duplicate-record', ...overrides } + store.setState({ openFiles: [...store.getState().openFiles, sibling] }) + store.getState().createUnifiedTab(sibling.worktreeId, 'editor', { + id: 'duplicate-tab', + entityId: sibling.id, + label: 'note.md', + activate: false + }) + return sibling +} + +function addSplitView(): void { + const originalTab = store.getState().unifiedTabsByWorktree['wt-1'][0] + if (!originalTab) { + throw new Error('Document tab did not open') + } + const split = store + .getState() + .createUnifiedTabInSplit( + 'wt-1', + 'editor', + { sourceGroupId: originalTab.groupId, splitDirection: 'right' }, + { id: 'second-pane-tab', entityId: file.id, label: 'note.md', activate: false } + ) + expect(split?.groupId).toBeTruthy() + expect(split?.groupId).not.toBe(originalTab.groupId) + expect(store.getState().groupsByWorktree['wt-1']).toHaveLength(2) +} + +describe('closing duplicate document records', () => { + it('closes clean duplicate records for the same document owner', () => { + addSibling() + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([]) + expect(store.getState().unifiedTabsByWorktree['wt-1']).toEqual([]) + expect(store.getState().groupsByWorktree['wt-1'][0]?.recentTabIds).toEqual([]) + expect(store.getState().recentlyClosedEditorTabsByWorktree['wt-1']).toHaveLength(1) + }) + + it('document close removes every tab linked to that record', () => { + addSplitView() + store.getState().closeFile(file.id) + expect(store.getState().unifiedTabsByWorktree['wt-1']).toEqual([]) + }) + + it('closing one pane tab preserves the document and its other view', () => { + const originalTab = store.getState().unifiedTabsByWorktree['wt-1'][0] + if (!originalTab) { + throw new Error('Document tab did not open') + } + addSplitView() + expect( + dispatchWorkspaceTabCommand({ + type: 'close', + target: { kind: 'tab', worktreeId: 'wt-1', tabId: originalTab.id } + }) + ).toBe(true) + expect(store.getState().openFiles).toEqual([file]) + expect(store.getState().unifiedTabsByWorktree['wt-1'].map((tab) => tab.id)).toEqual([ + 'second-pane-tab' + ]) + }) + + it.each([undefined, '', 'divergent pending text'])( + 'preserves a dirty sibling draft: %s', + (draft) => { + const sibling = addSibling({ isDirty: draft === undefined }) + if (draft !== undefined) { + store.setState({ editorDrafts: { [sibling.id]: draft } }) + } + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([sibling]) + if (draft !== undefined) { + expect(store.getState().editorDrafts[sibling.id]).toBe(draft) + } + } + ) + + it('preserves the same path on another runtime owner', () => { + const sibling = addSibling({ runtimeEnvironmentId: 'other-runtime' }) + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([sibling]) + }) + + it('preserves the same path on an external SSH target', () => { + const sibling = addSibling({ externalSshTargetId: 'other-ssh' }) + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([sibling]) + }) + + it('preserves the same path in another worktree', () => { + const sibling = addSibling({ worktreeId: 'wt-2' }) + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([sibling]) + }) + + it('preserves a sibling with different captured owner provenance', () => { + const provenance = file.operationProvenance + if (!provenance) { + throw new Error('Owner provenance did not capture') + } + const sibling = addSibling({ + operationProvenance: { + ...provenance, + generation: { ...provenance.generation, runtimeConnectionGeneration: 9 } + } + }) + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([sibling]) + }) + + it.each([{ readOnly: true }, { mode: 'diff' } as const])( + 'preserves another file mode or access type: %j', + (overrides) => { + const sibling = addSibling(overrides) + store.getState().closeFile(file.id) + expect(store.getState().openFiles).toEqual([sibling]) + } + ) + + it('closes a disconnected runtime document without requiring its worktree catalog', () => { + const provenance = file.operationProvenance + if (!provenance) { + throw new Error('Owner provenance did not capture') + } + file = { + ...file, + runtimeEnvironmentId: 'offline-owner', + operationProvenance: { + ...provenance, + generation: { + ...provenance.generation, + route: { executionHostId: 'runtime:offline-owner', runtimeEnvironmentId: 'offline-owner' } + } + } + } + store.setState({ openFiles: [file], worktreesByRepo: {}, runtimeEnvironments: [] }) + addSibling() + const originalTab = store + .getState() + .unifiedTabsByWorktree['wt-1'].find((tab) => tab.entityId === file.id) + if (!originalTab) { + throw new Error('Document tab did not open') + } + expect( + dispatchWorkspaceTabCommand({ + type: 'close', + target: { kind: 'tab', worktreeId: 'wt-1', tabId: originalTab.id } + }) + ).toBe(true) + expect(store.getState().openFiles).toEqual([]) + expect(store.getState().unifiedTabsByWorktree['wt-1']).toEqual([]) + }) + + it.each(['edit', 'read-only', 'diff', 'old-generation', 'external-ssh'] as const)( + 'keeps an untouched placeholder backing a surviving sibling draft: %s', + async (view) => { + const directory = await mkdtemp(join(tmpdir(), 'orca-close-draft-')) + const targetPath = join(directory, 'untitled.md') + const deletions: Promise[] = [] + const deletePath = vi.fn(({ targetPath: path }: { targetPath: string }) => { + const deletion = rm(path, { force: true }) + deletions.push(deletion) + return deletion + }) + vi.stubGlobal('window', { + api: { + fs: { stat: vi.fn(async () => ({ size: 0, isDirectory: false, mtime: 0 })), deletePath } + } + }) + try { + await writeFile(targetPath, '') + file = { ...file, filePath: targetPath, relativePath: 'untitled.md', isUntitled: true } + if (view === 'external-ssh') { + store.setState({ + repos: store.getState().repos.map((repo) => ({ ...repo, connectionId: 'same-ssh' })), + sshConnectionStates: new Map([ + [ + 'same-ssh', + { + targetId: 'same-ssh', + status: 'connected', + error: null, + reconnectAttempt: 0, + connectionGeneration: 7 + } + ] + ]) + }) + file = { + ...file, + externalSshTargetId: 'same-ssh', + operationProvenance: captureEditorFileOperationProvenance( + store.getState(), + file.worktreeId, + undefined, + false + ) + } + } + store.setState({ openFiles: [file] }) + const overrides: Partial = { isDirty: true } + if (view === 'read-only') { + overrides.readOnly = true + } else if (view === 'diff') { + overrides.mode = 'diff' + } else if (view === 'old-generation') { + const provenance = file.operationProvenance + if (!provenance) { + throw new Error('Owner provenance did not capture') + } + overrides.operationProvenance = { + ...provenance, + generation: { ...provenance.generation, runtimeConnectionGeneration: 9 } + } + } + const sibling = addSibling(overrides) + store.setState({ editorDrafts: { [sibling.id]: 'unsaved sibling text' } }) + store.getState().closeFile(file.id) + await new Promise((resolve) => setImmediate(resolve)) + await Promise.all(deletions) + expect(deletePath).not.toHaveBeenCalled() + expect(await readFile(targetPath, 'utf8')).toBe('') + expect(store.getState().openFiles).toEqual([sibling]) + expect(store.getState().editorDrafts[sibling.id]).toBe('unsaved sibling text') + } finally { + await rm(directory, { recursive: true, force: true }) + } + } + ) +}) diff --git a/src/renderer/src/store/slices/editor/actions/close-file-action.ts b/src/renderer/src/store/slices/editor/actions/close-file-action.ts index e84232d53a8..4475057a7e7 100644 --- a/src/renderer/src/store/slices/editor/actions/close-file-action.ts +++ b/src/renderer/src/store/slices/editor/actions/close-file-action.ts @@ -2,13 +2,70 @@ import type { EditorGet, EditorSet } from '../types/editor-set-get' import type { EditorSlice } from '../types/editor-slice' import { getRecentlyClosedTabPosition, pushRecentlyClosedTabKind } from '../../recently-closed-tabs' import { notifyHostOfMirroredEditorClose } from '@/runtime/close-mirrored-editor-tab' -import { type ClosedEditorTabSnapshot, MAX_RECENT_CLOSED_EDITOR_TABS } from '../types/open-file' +import { + type ClosedEditorTabSnapshot, + MAX_RECENT_CLOSED_EDITOR_TABS, + type OpenFile +} from '../types/open-file' import { removeMarkdownVisibilityKeys } from '../tabs/workspace-editor-item' import { deleteUntouchedUntitledFile, shouldDeleteUntouchedUntitledFile } from '../tabs/untitled-file-cleanup' import { unifiedTabsKeepWorktreeSelected } from './unified-tabs-keep-worktree-selected' +import { isSameEditorOwner } from '../file-ids/editor-file-ids' + +function isSameDocumentOwner(candidate: OpenFile, closedFile: OpenFile): boolean { + const expectedRoute = closedFile.operationProvenance?.generation.route + const actualRoute = candidate.operationProvenance?.generation.route + return ( + isSameEditorOwner(candidate, closedFile.worktreeId, closedFile.runtimeEnvironmentId) && + candidate.filePath === closedFile.filePath && + candidate.externalSshTargetId === closedFile.externalSshTargetId && + actualRoute?.executionHostId === expectedRoute?.executionHostId && + actualRoute?.runtimeEnvironmentId === expectedRoute?.runtimeEnvironmentId + ) +} + +function isSameOwnedDocument(candidate: OpenFile, closedFile: OpenFile): boolean { + const expected = closedFile.operationProvenance + const actual = candidate.operationProvenance + const expectedGeneration = expected?.generation + const actualGeneration = actual?.generation + return ( + isSameDocumentOwner(candidate, closedFile) && + !closedFile.externalSshTargetId && + !candidate.externalSshTargetId && + actual?.ownershipProjection === expected?.ownershipProjection && + actual?.expectedSshConnectionGeneration === expected?.expectedSshConnectionGeneration && + actualGeneration?.runtimeConnectionGeneration === + expectedGeneration?.runtimeConnectionGeneration && + actualGeneration?.runtimePairingRevision === expectedGeneration?.runtimePairingRevision && + actualGeneration?.runtimeSshGeneration === expectedGeneration?.runtimeSshGeneration && + actualGeneration?.nestedSshGeneration === expectedGeneration?.nestedSshGeneration && + actualGeneration?.directSshGeneration === expectedGeneration?.directSshGeneration + ) +} + +function findAdjacentOpenFileId( + files: readonly OpenFile[], + closedFileIds: ReadonlySet, + preferredFileId: string | null +): string | null { + const preferredIndex = files.findIndex((file) => file.id === preferredFileId) + const nextFile = files + .slice(Math.max(preferredIndex + 1, 0)) + .find((file) => !closedFileIds.has(file.id)) + if (nextFile) { + return nextFile.id + } + return ( + files + .slice(0, Math.max(preferredIndex, 0)) + .toReversed() + .find((file) => !closedFileIds.has(file.id))?.id ?? null + ) +} export function createCloseFileAction( set: EditorSet, @@ -18,27 +75,60 @@ export function createCloseFileAction( closeFile: (fileId) => { // Why: capture untitled+dirty state before set() mutates the store, so cleanup of throwaway untitled files can decide after removal. const preClose = get().openFiles.find((f) => f.id === fileId) + // Why: stale same-owner records survive tab close; retain other owners and pending drafts. + const fileIdsToClose = new Set( + preClose + ? get() + .openFiles.filter( + (file) => + file.id === fileId || + (preClose.mode === 'edit' && + file.mode === 'edit' && + preClose.readOnly !== true && + file.readOnly !== true && + isSameOwnedDocument(file, preClose) && + !file.isDirty && + !(file.id in get().editorDrafts)) + ) + .map((file) => file.id) + : [fileId] + ) + const preCloseFiles = get().openFiles.filter((file) => fileIdsToClose.has(file.id)) // Why: also check editorDrafts — isDirty is set by a debounced callback, so a draft can exist before isDirty flushes; a draft means the user typed something. const hasDraft = !!get().editorDrafts[fileId] - const shouldDeleteFromDisk = shouldDeleteUntouchedUntitledFile(preClose, hasDraft) + const shouldDeleteFromDisk = + preClose !== undefined && + shouldDeleteUntouchedUntitledFile(preClose, hasDraft) && + !get().openFiles.some( + (file) => !fileIdsToClose.has(file.id) && isSameDocumentOwner(file, preClose) + ) // Why: mirrored tabs are host-owned, so the host must close its copy or its next snapshot re-mirrors the file and the tab reopens. - notifyHostOfMirroredEditorClose(get(), preClose?.worktreeId, fileId) + for (const file of preCloseFiles) { + notifyHostOfMirroredEditorClose(get(), file.worktreeId, file.id) + } set((s) => { const closedFile = s.openFiles.find((f) => f.id === fileId) - const newFiles = s.openFiles.filter((f) => f.id !== fileId) + const newFiles = s.openFiles.filter((f) => !fileIdsToClose.has(f.id)) const newEditorDrafts = { ...s.editorDrafts } - delete newEditorDrafts[fileId] const newMarkdownViewMode = { ...s.markdownViewMode } - delete newMarkdownViewMode[fileId] const newMarkdownRichModeSizeOverride = { ...s.markdownRichModeSizeOverride } - delete newMarkdownRichModeSizeOverride[fileId] const newEditorViewMode = { ...s.editorViewMode } - delete newEditorViewMode[fileId] - const markdownVisibilityKeys = new Set([fileId]) - if (closedFile?.markdownPreviewSourceFileId) { - markdownVisibilityKeys.add(closedFile.markdownPreviewSourceFileId) + const newEditorCursorLine = { ...s.editorCursorLine } + for (const id of fileIdsToClose) { + delete newEditorDrafts[id] + delete newMarkdownViewMode[id] + delete newMarkdownRichModeSizeOverride[id] + delete newEditorViewMode[id] + // Why: editorCursorLine is keyed by fileId and grows unbounded across a long session without cleanup on close. + delete newEditorCursorLine[id] + } + const markdownVisibilityKeys = new Set(fileIdsToClose) + for (const file of preCloseFiles) { + if (file.markdownPreviewSourceFileId) { + markdownVisibilityKeys.add(file.markdownPreviewSourceFileId) + } } const visibilityKeysToRemove = [...markdownVisibilityKeys].filter( (key) => @@ -52,30 +142,29 @@ export function createCloseFileAction( visibilityKeysToRemove.length > 0 ? removeMarkdownVisibilityKeys(s.markdownTableOfContentsVisible, visibilityKeysToRemove) : s.markdownTableOfContentsVisible - // Why: editorCursorLine is keyed by fileId and grows unbounded across a long session without cleanup on close. - const newEditorCursorLine = { ...s.editorCursorLine } - delete newEditorCursorLine[fileId] let newActiveId = s.activeFileId const newActiveFileIdByWorktree = { ...s.activeFileIdByWorktree } - if (s.activeFileId === fileId) { - // Why: a stale activeFileId (e.g. an orphan editor tab promoted by closeUnifiedTab) is not in openFiles; scope the fallback to the active worktree. + if (s.activeFileId && fileIdsToClose.has(s.activeFileId)) { const worktreeId = closedFile?.worktreeId ?? s.activeWorktreeId const worktreeFiles = worktreeId - ? newFiles.filter((f) => f.worktreeId === worktreeId) - : [] - if (worktreeFiles.length === 0) { - newActiveId = null - } else { - // Pick adjacent file from same worktree; -1 (closed file not open) clamps to the first. - const closedWorktreeIdx = ( - worktreeId ? s.openFiles.filter((f) => f.worktreeId === worktreeId) : s.openFiles - ).findIndex((f) => f.id === fileId) - newActiveId = - worktreeFiles[Math.min(Math.max(closedWorktreeIdx, 0), worktreeFiles.length - 1)].id - } - if (worktreeId) { - newActiveFileIdByWorktree[worktreeId] = newActiveId + ? s.openFiles.filter((file) => file.worktreeId === worktreeId) + : s.openFiles + newActiveId = findAdjacentOpenFileId(worktreeFiles, fileIdsToClose, s.activeFileId) + } + + const closedWorktreeId = closedFile?.worktreeId + if (closedWorktreeId) { + const worktreeFiles = s.openFiles.filter((file) => file.worktreeId === closedWorktreeId) + const worktreeActiveFileId = s.activeFileIdByWorktree[closedWorktreeId] + if (worktreeActiveFileId && fileIdsToClose.has(worktreeActiveFileId)) { + newActiveFileIdByWorktree[closedWorktreeId] = findAdjacentOpenFileId( + worktreeFiles, + fileIdsToClose, + worktreeActiveFileId + ) + } else if (s.activeFileId && fileIdsToClose.has(s.activeFileId)) { + newActiveFileIdByWorktree[closedWorktreeId] = newActiveId } } @@ -112,7 +201,7 @@ export function createCloseFileAction( activeWorktreeId !== null && unifiedTabsKeepWorktreeSelected( s.unifiedTabsByWorktree?.[activeWorktreeId], - new Set([fileId]) + fileIdsToClose ) const shouldDeactivateWorktree = activeWorktreeId !== null && @@ -128,7 +217,7 @@ export function createCloseFileAction( ? { ...s.tabBarOrderByWorktree, [worktreeId]: (s.tabBarOrderByWorktree[worktreeId] ?? []).filter( - (entryId) => entryId !== fileId + (entryId) => !fileIdsToClose.has(entryId) ) } : s.tabBarOrderByWorktree @@ -192,7 +281,9 @@ export function createCloseFileAction( tabBarOrderByWorktree: nextTabBarOrderByWorktree, pendingEditorReveal: null, pendingEditorFocusRequest: - s.pendingEditorFocusRequest?.fileId === fileId ? null : s.pendingEditorFocusRequest, + s.pendingEditorFocusRequest && fileIdsToClose.has(s.pendingEditorFocusRequest.fileId) + ? null + : s.pendingEditorFocusRequest, recentlyClosedEditorTabsByWorktree: nextRecentlyClosed, recentlyClosedTabKindsByWorktree: nextRecentlyClosedKinds } @@ -204,19 +295,21 @@ export function createCloseFileAction( } // Why: route editor/diff closes through the unified close path (MRU + visual-neighbor fallback) so they match terminal/browser tab-close behavior. - for (const tabs of Object.values(get().unifiedTabsByWorktree ?? {})) { - const unifiedTab = tabs.find( - (entry) => - entry.entityId === fileId && - (entry.contentType === 'editor' || - entry.contentType === 'diff' || - entry.contentType === 'conflict-review' || - entry.contentType === 'check-details') - ) - if (unifiedTab) { - get().closeUnifiedTab(unifiedTab.id) - break - } + const unifiedTabIdsToClose = Object.values(get().unifiedTabsByWorktree ?? {}).flatMap( + (tabs) => + tabs + .filter( + (entry) => + fileIdsToClose.has(entry.entityId) && + (entry.contentType === 'editor' || + entry.contentType === 'diff' || + entry.contentType === 'conflict-review' || + entry.contentType === 'check-details') + ) + .map((entry) => entry.id) + ) + for (const unifiedTabId of unifiedTabIdsToClose) { + get().closeUnifiedTab(unifiedTabId) } } }