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 <zwq19980411@gmail.com>
2026-10-09 08:02:35 +00:00 · 2026-10-06 17:46:39 -07:00
parent 9c6702bb07
commit c9f4cd7e27
2 changed files with 419 additions and 46 deletions
@@ -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<AppState> | null } => ({ store: null }))
+vi.mock('@/store', () => ({
+  useAppStore: {
+    getState: () => {
+      if (!live.store) {
+        throw new Error('No test store')
+      }
+      return live.store.getState()
+    }
+  }
+}))
+
+let store: StoreApi<AppState>
+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> = {}): 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<void>[] = []
+      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<OpenFile> = { 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<void>((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 })
+      }
+    }
+  )
+})
@@ -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<string>,
+  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)
      }
    }
  }