From 86ef3b374a20ff219f7477ad795099f1a778a0ee Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:15:17 -0700 Subject: [PATCH] feat(editor): decide move echo vs external write by content identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the time-bounded self-move suppression heuristic with a correct-by-construction identity check, so a genuine external write to a just-moved path is never swallowed and the move's own echo is never a false conflict — regardless of watcher event-kind coalescing or timing. - Remap now carries the edit-session identity (lastKnownDiskSignature, externalMutation, pendingDiskBaselineVerification) onto the re-homed tab. Previously the close+reopen dropped it, so a moved tab lost its disk baseline (and any pre-existing changed-on-disk conflict silently vanished on move). - On a live self-move-target dirty event the watch hook now reads the destination and compares getDiskBaselineSignature(disk) to the tab's carried baseline: equal => move echo (suppress), differ/binary => genuine write (banner). Autosave is suspended synchronously before the read so a write landing mid-read can't be overwritten; a generation token makes overlapping reads safe. Fails CLOSED (marks changed) on a missing baseline or read error — never blind-suppresses. - The self-move registry now only scopes WHEN to verify. The source-side delete still can't be content-verified (nothing to read), so it stays a bounded, documented suppression. - Trim the verbose Why-comments across these files to 1-2 lines. Adds content-identity verification tests (echo/differ/no-baseline/read- error/binary/autosave-gate) and a remap test for the carried identity; splits the watch-hook suite to stay under the max-lines limit. Co-authored-by: Orca --- .../editor/editor-self-move-registry.ts | 55 +--- .../useEditorExternalWatch-self-move.test.ts | 269 ++++++++++++++++++ .../src/hooks/useEditorExternalWatch.test.ts | 125 +------- .../src/hooks/useEditorExternalWatch.ts | 147 +++++++--- .../src/lib/record-self-move-for-open-tabs.ts | 26 +- ...p-open-editor-tabs-for-path-change.test.ts | 32 +++ .../remap-open-editor-tabs-for-path-change.ts | 19 +- .../src/lib/rename-open-editor-tabs-path.ts | 16 +- src/renderer/src/store/slices/editor.ts | 15 + 9 files changed, 466 insertions(+), 238 deletions(-) create mode 100644 src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts diff --git a/src/renderer/src/components/editor/editor-self-move-registry.ts b/src/renderer/src/components/editor/editor-self-move-registry.ts index c7f2ac09838..77cb6a95b8c 100644 --- a/src/renderer/src/components/editor/editor-self-move-registry.ts +++ b/src/renderer/src/components/editor/editor-self-move-registry.ts @@ -1,39 +1,18 @@ import { normalizeAbsolutePathForComparison } from '@/components/right-sidebar/file-explorer-paths' -// Why: an Orca-initiated move (explorer drag-drop, inline/tab rename) physically -// relocates a file, which the worktree watcher reports as delete(old) + -// create(new) a few ms later. The move also re-homes the open tab to the new -// path (remapOpenEditorTabsForPathChange) carrying its unsaved draft. Once the -// tab already lives at the new path, that create(new) echo looks — to -// useEditorExternalWatch — like an external write landing on a dirty tab, so it -// raises a false "changed on disk" banner whose Reload discards the draft. -// Stamping both endpoints of an Orca move (see recordSelfMoveForOpenTabs, called -// BEFORE the on-disk rename) lets the watch hook recognize the echo as -// self-initiated and skip the false conflict. This is the move analog of -// editor-self-write-registry (which covers content writes); matching is -// path-only because a move changes no content. +// Records recent Orca-initiated moves so the watch hook knows a create/delete +// echo at a moved path may be the move's own, not an external write. Scopes WHEN +// to verify a target echo (see useEditorExternalWatch); source deletes are +// suppressed outright since a deletion can't be content-verified. const SELF_MOVE_TTL_MS = 750 -// Why: SSH/runtime watcher echoes travel a poll-plus-network path and can land -// seconds after the move. A local-sized TTL would expire before the remote echo -// arrives, re-opening the false-banner window on runtime-backed tabs. +// SSH/runtime watcher echoes travel a poll-plus-network path and land later. export const SELF_MOVE_REMOTE_TTL_MS = 3000 -// Why: sized well above any realistic count of simultaneously-open dirty tabs in -// one directory move (each moved tab stamps two distinct path keys), so a single -// bulk move never self-evicts its own not-yet-echoed stamps. Purely a safety -// valve — TTL pruning is the real bound, since stamps self-expire within seconds. +// Safety valve above any realistic bulk-move tab count; TTL pruning is the real bound. const SELF_MOVE_MAX_STAMPS = 1024 -// Why: each stamp is one independent registration carrying its own expiry, held -// as a list per role. This keeps concurrent registrations of the same path+role -// (drag two files named report.md into one dir → both stamp that dir's -// report.md as a target) fully separate, so: -// - a role is live while ANY of its registrations is unexpired, -// - `clearSelfMove` retracts EXACTLY the registration a failed move added (by -// its ticket's expiry), never a concurrent move's, -// - an expired registration can't be "resurrected" by a later stamp on the -// same key, because liveness is computed per registration, not from a shared -// scalar. (A single shared expiry would over-extend the suppression window -// when the max-contributing registration is cleared.) +// Each stamp is an independent registration with its own expiry (a list per +// role), so concurrent moves on the same path, precise ticket retraction, and +// expiry never interfere — a shared scalar expiry would over-extend or resurrect. type SelfMoveStamp = { source: number[] target: number[] @@ -115,12 +94,8 @@ function removeRegistration( } /** - * Records an Orca-initiated move so the worktree-watch hook can recognize the - * delete(old)+create(new) echo as self-initiated instead of an external change. - * Both endpoints are stamped: the source so the delete does not tombstone the - * tab, the target so the create does not raise a changed-on-disk banner on the - * (re-homed, still-dirty) tab. Returns a ticket to retract this exact stamp with - * {@link clearSelfMove} if the move turns out not to happen. + * Stamps both endpoints of an Orca-initiated move. Returns a ticket to retract + * this exact stamp with {@link clearSelfMove} if the move doesn't happen. */ export function recordSelfMove( fromPath: string, @@ -137,12 +112,8 @@ export function recordSelfMove( return { fromPath, toPath, runtimeEnvironmentId: runtimeEnvironmentId ?? null, expiresAt } } -/** - * Retracts exactly the registration a {@link recordSelfMove} added, so a rename - * that FAILED after stamping does not keep suppressing genuine watcher events - * for the paths it never moved. A concurrent move's registration on the same - * path+role is untouched. - */ +/** Retracts exactly the registration {@link recordSelfMove} added; leaves a + * concurrent move's registration on the same path untouched. */ export function clearSelfMove(ticket: SelfMoveTicket): void { removeRegistration(ticket.fromPath, 'source', ticket.runtimeEnvironmentId, ticket.expiresAt) removeRegistration(ticket.toPath, 'target', ticket.runtimeEnvironmentId, ticket.expiresAt) diff --git a/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts b/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts new file mode 100644 index 00000000000..b0952ee7b90 --- /dev/null +++ b/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts @@ -0,0 +1,269 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as EditorAutosaveModule from '@/components/editor/editor-autosave' +import type { FsChangedPayload } from '../../../shared/types' + +vi.mock('@/store', () => ({ + useAppStore: { getState: vi.fn() } +})) +vi.mock('@/components/editor/editor-autosave', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + notifyEditorExternalFileChange: vi.fn(), + getOpenFilesForExternalFileChange: vi.fn(() => []) + } +}) + +import { createExternalWatchEventHandler } from './useEditorExternalWatch' +import { useAppStore } from '@/store' +import { getOpenFilesForExternalFileChange } from '@/components/editor/editor-autosave' +import { __clearSelfWriteRegistryForTests } from '@/components/editor/editor-self-write-registry' +import { + __clearSelfMoveRegistryForTests, + recordSelfMove +} from '@/components/editor/editor-self-move-registry' +import { getDiskBaselineSignature } from '@/components/editor/diff-content-signature' + +const findTarget = (worktreePath: string, runtimeEnvironmentId: string | null = null) => + worktreePath === '/repo' + ? { + worktreeId: 'wt-1', + worktreePath: '/repo', + connectionId: undefined, + runtimeEnvironmentId + } + : undefined + +function payload(events: FsChangedPayload['events']): FsChangedPayload { + return { worktreePath: '/repo', events } +} + +describe('self-move suppression (registry-scoped)', () => { + const setExternalMutation = vi.fn() + const fileNotes = { + id: 'file-notes', + worktreeId: 'wt-1', + worktreePath: '/repo', + filePath: '/repo/notes.md', + relativePath: 'notes.md', + mode: 'edit' as const, + isDirty: false + } + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + vi.mocked(useAppStore.getState).mockReturnValue({ + openFiles: [fileNotes], + setExternalMutation + } as never) + }) + + afterEach(() => { + vi.useRealTimers() + vi.unstubAllGlobals() + __clearSelfWriteRegistryForTests() + __clearSelfMoveRegistryForTests() + }) + + it('does not tombstone a tab still at the old path when its delete is a self-move echo', () => { + // Pre-remap ordering: delete(old) can arrive while the tab is still at the + // old path. The stamp is recorded before the rename, so the source guard + // still suppresses the tombstone — the tab is being re-homed, not deleted. + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'delete', absolutePath: '/repo/notes.md' }])) + vi.advanceTimersByTime(200) + + expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'deleted') + expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'renamed') + dispose() + }) + + it('surfaces a changed-on-disk conflict once the self-move TTL has expired', () => { + const movedDirtyTab = { + ...fileNotes, + filePath: '/repo/subdir/notes.md', + relativePath: 'subdir/notes.md', + isDirty: true + } + vi.mocked(useAppStore.getState).mockReturnValue({ + openFiles: [movedDirtyTab], + setExternalMutation + } as never) + vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([movedDirtyTab] as never) + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + // Past the local TTL the path is no longer a live self-move target, so a real + // external write goes through the normal changed-on-disk path. + vi.advanceTimersByTime(1000) + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + vi.advanceTimersByTime(200) + + expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') + dispose() + }) + + it('marks changed for an event with no self-move stamp (no over-suppression)', () => { + const movedDirtyTab = { + ...fileNotes, + filePath: '/repo/subdir/notes.md', + relativePath: 'subdir/notes.md', + isDirty: true + } + vi.mocked(useAppStore.getState).mockReturnValue({ + openFiles: [movedDirtyTab], + setExternalMutation + } as never) + vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([movedDirtyTab] as never) + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + vi.advanceTimersByTime(200) + + expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') + dispose() + }) +}) + +describe('self-move echo verification (content identity)', () => { + const setExternalMutation = vi.fn() + const setPendingDiskBaselineVerification = vi.fn() + const findTarget = (worktreePath: string, runtimeEnvironmentId: string | null = null) => + worktreePath === '/repo' + ? { + worktreeId: 'wt-1', + worktreePath: '/repo', + connectionId: undefined, + runtimeEnvironmentId + } + : undefined + + const BASELINE_CONTENT = 'the file on disk\n' + const baselineSignature = getDiskBaselineSignature(BASELINE_CONTENT) + + function movedDirtyTab(overrides: Record = {}): Record { + return { + id: 'file-notes', + worktreeId: 'wt-1', + worktreePath: '/repo', + filePath: '/repo/subdir/notes.md', + relativePath: 'subdir/notes.md', + mode: 'edit' as const, + isDirty: true, + lastKnownDiskSignature: baselineSignature, + ...overrides + } + } + + function mockState(file: Record): void { + vi.mocked(useAppStore.getState).mockReturnValue({ + openFiles: [file], + setExternalMutation, + setPendingDiskBaselineVerification + } as never) + vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([file] as never) + } + + function payload(events: FsChangedPayload['events']): FsChangedPayload { + return { worktreePath: '/repo', events } + } + + // Real readRuntimeFileContent reads local files via window.api.fs.readFile. + function stubDiskRead(result: unknown): void { + vi.stubGlobal('window', { + api: { fs: { readFile: vi.fn().mockResolvedValue(result) } } + }) + } + + function stubDiskReadError(error: Error): void { + vi.stubGlobal('window', { + api: { fs: { readFile: vi.fn().mockRejectedValue(error) } } + }) + } + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + __clearSelfMoveRegistryForTests() + __clearSelfWriteRegistryForTests() + }) + + afterEach(() => { + vi.useRealTimers() + vi.unstubAllGlobals() + __clearSelfMoveRegistryForTests() + }) + + it('suspends autosave synchronously and suppresses when disk matches the baseline', async () => { + mockState(movedDirtyTab()) + stubDiskRead({ isBinary: false, content: BASELINE_CONTENT }) + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + // Autosave must be gated BEFORE the async read so it can't overwrite mid-read. + expect(setPendingDiskBaselineVerification).toHaveBeenCalledWith('file-notes', true) + await vi.advanceTimersByTimeAsync(100) + + expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'changed') + expect(setPendingDiskBaselineVerification).toHaveBeenCalledWith('file-notes', false) + dispose() + }) + + it('marks changed when a genuine external write differs from the baseline in-window', async () => { + mockState(movedDirtyTab()) + stubDiskRead({ isBinary: false, content: 'an agent rewrote this file\n' }) + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + await vi.advanceTimersByTimeAsync(100) + + expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') + expect(setPendingDiskBaselineVerification).toHaveBeenCalledWith('file-notes', false) + dispose() + }) + + it('fails closed (marks changed) when the tab has no disk baseline', async () => { + mockState(movedDirtyTab({ lastKnownDiskSignature: undefined })) + stubDiskRead({ isBinary: false, content: BASELINE_CONTENT }) + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + await vi.advanceTimersByTimeAsync(100) + + expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') + dispose() + }) + + it('fails closed when the destination read errors', async () => { + mockState(movedDirtyTab()) + stubDiskReadError(new Error('EACCES')) + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + await vi.advanceTimersByTimeAsync(100) + + expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') + expect(setPendingDiskBaselineVerification).toHaveBeenCalledWith('file-notes', false) + dispose() + }) + + it('fails closed when the destination is binary', async () => { + mockState(movedDirtyTab()) + stubDiskRead({ isBinary: true, content: '' }) + recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') + const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) + + handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) + await vi.advanceTimersByTimeAsync(100) + + expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') + dispose() + }) +}) diff --git a/src/renderer/src/hooks/useEditorExternalWatch.test.ts b/src/renderer/src/hooks/useEditorExternalWatch.test.ts index a5b4135a9ba..3629dbf2dce 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch.test.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch.test.ts @@ -33,10 +33,7 @@ import { __clearSelfWriteRegistryForTests, recordSelfWrite } from '@/components/editor/editor-self-write-registry' -import { - __clearSelfMoveRegistryForTests, - recordSelfMove -} from '@/components/editor/editor-self-move-registry' +import { __clearSelfMoveRegistryForTests } from '@/components/editor/editor-self-move-registry' describe('getWatchedTargetKey', () => { it('changes when a worktree gains an SSH connection id', () => { @@ -266,22 +263,6 @@ describe('createExternalWatchEventHandler tombstone coalescing', () => { dispose() }) - it('does not tombstone a tab still at the old path when its delete is a self-move echo', () => { - // Pre-remap ordering: the watcher's delete(old) can arrive while the tab is - // still at the old path (remap hasn't run yet). Because the stamp is - // recorded before the rename, the source guard must still suppress the - // tombstone — the tab is about to be re-homed, not deleted. - recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') - const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) - - handleFsChanged(payload([{ kind: 'delete', absolutePath: '/repo/notes.md' }])) - vi.advanceTimersByTime(200) - - expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'deleted') - expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'renamed') - dispose() - }) - it('resolves batched delete+create in a single payload synchronously as renamed', () => { const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) handleFsChanged( @@ -562,110 +543,6 @@ describe('createExternalWatchEventHandler tombstone coalescing', () => { dispose() }) - it('does not mark changed-on-disk for the create echo of an Orca-initiated move', () => { - // State after the explorer move re-homed the dirty tab to the new path. - const movedDirtyTab = { - ...fileNotes, - filePath: '/repo/subdir/notes.md', - relativePath: 'subdir/notes.md', - isDirty: true - } - vi.mocked(useAppStore.getState).mockReturnValue({ - openFiles: [movedDirtyTab], - setExternalMutation - } as never) - vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([movedDirtyTab] as never) - // The remap stamps the move before the watcher echo arrives. - recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') - const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) - - handleFsChanged( - payload([ - { kind: 'delete', absolutePath: '/repo/notes.md' }, - { kind: 'create', absolutePath: '/repo/subdir/notes.md' } - ]) - ) - vi.advanceTimersByTime(200) - - // The user only moved their own file — no false changed-on-disk banner, and - // the source delete must not tombstone the re-homed tab either. - expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'changed') - expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'deleted') - expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'renamed') - dispose() - }) - - it('suppresses even an update echo of a move within the TTL (coalescing-robust)', () => { - // The local watcher can coalesce the move's create+attr-change into a lone - // `update`, so suppression must not depend on the event kind. - const movedDirtyTab = { - ...fileNotes, - filePath: '/repo/subdir/notes.md', - relativePath: 'subdir/notes.md', - isDirty: true - } - vi.mocked(useAppStore.getState).mockReturnValue({ - openFiles: [movedDirtyTab], - setExternalMutation - } as never) - vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([movedDirtyTab] as never) - recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') - const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) - - handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) - vi.advanceTimersByTime(200) - - expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'changed') - dispose() - }) - - it('surfaces a changed-on-disk conflict once the self-move TTL has expired', () => { - const movedDirtyTab = { - ...fileNotes, - filePath: '/repo/subdir/notes.md', - relativePath: 'subdir/notes.md', - isDirty: true - } - vi.mocked(useAppStore.getState).mockReturnValue({ - openFiles: [movedDirtyTab], - setExternalMutation - } as never) - vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([movedDirtyTab] as never) - recordSelfMove('/repo/notes.md', '/repo/subdir/notes.md') - const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) - - // Suppression is time-bounded: past the local TTL a real external write is - // no longer mistaken for the move's echo. - vi.advanceTimersByTime(1000) - handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) - vi.advanceTimersByTime(200) - - expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') - dispose() - }) - - it('still marks changed-on-disk for a create with no self-move stamp (no over-suppression)', () => { - const movedDirtyTab = { - ...fileNotes, - filePath: '/repo/subdir/notes.md', - relativePath: 'subdir/notes.md', - isDirty: true - } - vi.mocked(useAppStore.getState).mockReturnValue({ - openFiles: [movedDirtyTab], - setExternalMutation - } as never) - vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([movedDirtyTab] as never) - // No recordSelfMove: a genuine external write at this path must still surface. - const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) - - handleFsChanged(payload([{ kind: 'update', absolutePath: '/repo/subdir/notes.md' }])) - vi.advanceTimersByTime(200) - - expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') - dispose() - }) - it('still reloads a clean sibling diff tab when the edit tab for the path is dirty', () => { const dirtyFile = { ...fileNotes, diff --git a/src/renderer/src/hooks/useEditorExternalWatch.ts b/src/renderer/src/hooks/useEditorExternalWatch.ts index aa936f09bdc..e9f8600abd0 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch.ts @@ -34,6 +34,7 @@ import { } from './worktree-file-change-event' import { isGitRepoKind } from '../../../shared/repo-kind' import { markFileChangedOnDisk } from '@/components/editor/editor-changed-on-disk-mark' +import { getDiskBaselineSignature } from '@/components/editor/diff-content-signature' // Why: atomic-write patterns (Claude Code's Edit tool, editors like vim, // VSCode) land as a short burst of `update` events — or `delete + create` on @@ -491,12 +492,9 @@ export function createExternalWatchEventHandler( target.runtimeEnvironmentId, openFilesAtStart ).filter((fileId) => { - // Why: if this delete is the source side of an Orca-initiated move, the - // tab has already been (or is about to be) re-homed to the new path — do - // not tombstone it 'deleted'/'renamed' from the move's own watcher echo. - // (Bounded by the TTL: if the rename then fails and something else really - // deleted the source in that window, the tombstone is missed until a later - // event — a save simply recreates the file, so no data is lost.) + // Don't tombstone the source side of an Orca move (the tab is being + // re-homed). A deletion has nothing to content-verify, so this stays a + // bounded suppression; a real concurrent delete is recoverable on save. const file = openFilesAtStart.find((f) => f.id === fileId) return !file || !isRecentSelfMoveSource(file.filePath, target.runtimeEnvironmentId) }) @@ -657,36 +655,21 @@ export function createExternalWatchEventHandler( } const absolutePath = joinPath(notification.worktreePath, notification.relativePath) const dirtyMatches = matching.filter((f) => f.isDirty) - // Why: an Orca-initiated move re-homes the dirty tab to this path and - // carries its draft. The move's own watcher echo is not an external edit, - // so suppress the changed-on-disk mark for it — otherwise the false - // banner's Reload discards the just-carried draft. We can't gate on the - // event kind: the main-process watcher coalesces a create+attr-change - // burst down to a lone `update`, so a create-only gate would let the - // move's own echo through on some hosts. Suppression is therefore bounded - // by the self-move TTL instead — a genuine external write landing on this - // exact path within that short window is the accepted trade-off (the - // in-memory draft is preserved either way; only an unheralded save could - // overwrite it, the same limit as any missed write on a dirty tab). A move - // whose rename ultimately fails retracts its stamp, but an event consumed - // during that pre-failure window is already swallowed; it only matters for - // the rare case of an unrelated dirty tab already open at the move's - // destination (the common non-overlapping move has no dirty tab there yet). - // A move changes no bytes, so there is nothing to echo-verify the way - // self-writes do. - const isSelfMoveTargetPath = isRecentSelfMoveTarget(absolutePath, target.runtimeEnvironmentId) - if (dirtyMatches.length > 0 && !isSelfMoveTargetPath) { - // Why: an external write landing on a dirty tab must not vanish - // silently (issue #7265) — the user was left with a stale tab and a - // save that clobbered the newer disk content. Mark the tab so the - // editor shows a changed-on-disk banner with an explicit reload path. - scheduleChangedOnDiskMark( - target, - notification, - // Why: canAutoSaveOpenFile is exactly the set of tabs that can hold - // unsaved edits (edit + unstaged diff) — the tabs the banner serves. - dirtyMatches.filter((dirtyFile) => canAutoSaveOpenFile(dirtyFile)).map((f) => f.id) - ) + if (dirtyMatches.length > 0) { + // canAutoSaveOpenFile is the set of tabs that can hold unsaved edits — + // the tabs the changed-on-disk banner serves. + const dirtyIds = dirtyMatches.filter((f) => canAutoSaveOpenFile(f)).map((f) => f.id) + if (isRecentSelfMoveTarget(absolutePath, target.runtimeEnvironmentId)) { + // This path is a live self-move destination, so the event may be the + // move's own echo. Distinguish it from a real external write by disk + // identity, not by trusting the stamp: an echo leaves disk == the + // tab's baseline, a genuine write does not. + scheduleSelfMoveEchoVerification(target, notification, dirtyIds) + } else { + // An external write on a dirty tab must not vanish silently (issue + // #7265); mark it so the editor shows the reload banner. + scheduleChangedOnDiskMark(target, notification, dirtyIds) + } if (dirtyMatches.length === matching.length) { if (hasCombinedDiffConsumer) { scheduleDebouncedExternalReload(notification) @@ -801,6 +784,98 @@ function scheduleChangedOnDiskMark( }) } +// Per-file generation for live self-move echo verification: a newer event's +// read supersedes an older one so overlapping reads can't clear each other's +// autosave gate or apply a stale verdict. +const liveMoveVerifyGeneration = new Map() +let liveMoveVerifyCounter = 0 + +type LiveMoveVerifyCandidate = { + fileId: string + baseline: string | undefined + generation: number +} + +// Resolves one candidate against the disk read. `diskSignature` is null for a +// binary/unreadable target. Fails CLOSED — anything but a proven baseline match +// surfaces the conflict, so a real external write is never silently swallowed. +function resolveLiveMoveVerification( + candidate: LiveMoveVerifyCandidate, + diskSignature: string | null, + connectionId: string | undefined +): void { + const { fileId, baseline, generation } = candidate + if (liveMoveVerifyGeneration.get(fileId) !== generation) { + return // a newer verification owns this tab and its autosave gate + } + liveMoveVerifyGeneration.delete(fileId) + const state = useAppStore.getState() + state.setPendingDiskBaselineVerification(fileId, false) + const file = state.openFiles.find((f) => f.id === fileId) + // A save/reload/dismissal in the interim owns the newer state: skip if the tab + // resolved, or its baseline advanced past the one we captured. + if ( + !file || + !file.isDirty || + file.externalMutation === 'changed' || + file.lastKnownDiskSignature !== baseline + ) { + return + } + const isMoveEcho = baseline !== undefined && diskSignature === baseline + if (!isMoveEcho) { + markFileChangedOnDisk(state, file, { connectionId, origin: 'live' }) + } +} + +// The move's own watcher echo leaves disk == the tab's disk baseline; a genuine +// external write does not. Read the destination once and settle each dirty tab +// by content identity. Autosave is suspended synchronously first so a write +// landing mid-read can't be overwritten before we decide. +function scheduleSelfMoveEchoVerification( + target: WatchedTarget, + notification: ExternalWatchNotification, + fileIds: string[] +): void { + if (fileIds.length === 0) { + return + } + const state = useAppStore.getState() + const candidates: LiveMoveVerifyCandidate[] = [] + for (const fileId of fileIds) { + const file = state.openFiles.find((f) => f.id === fileId) + if (!file || !file.isDirty || file.externalMutation === 'changed') { + continue + } + const generation = ++liveMoveVerifyCounter + liveMoveVerifyGeneration.set(fileId, generation) + state.setPendingDiskBaselineVerification(fileId, true) + candidates.push({ fileId, baseline: file.lastKnownDiskSignature, generation }) + } + if (candidates.length === 0) { + return + } + const absolutePath = joinPath(notification.worktreePath, notification.relativePath) + void readFileForEchoVerification({ + runtimeEnvironmentId: target.runtimeEnvironmentId, + filePath: absolutePath, + relativePath: notification.relativePath, + worktreeId: notification.worktreeId, + connectionId: target.connectionId + }) + .then((result) => { + const diskSignature = result.isBinary ? null : getDiskBaselineSignature(result.content) + for (const candidate of candidates) { + resolveLiveMoveVerification(candidate, diskSignature, target.connectionId) + } + }) + .catch(() => { + for (const candidate of candidates) { + resolveLiveMoveVerification(candidate, null, target.connectionId) + } + }) +} + function scheduleSelfWriteAwareExternalReload( target: WatchedTarget, notification: ExternalWatchNotification, diff --git a/src/renderer/src/lib/record-self-move-for-open-tabs.ts b/src/renderer/src/lib/record-self-move-for-open-tabs.ts index df4e8bc3c8e..c987fbd994a 100644 --- a/src/renderer/src/lib/record-self-move-for-open-tabs.ts +++ b/src/renderer/src/lib/record-self-move-for-open-tabs.ts @@ -16,10 +16,6 @@ type OpenTabMove = { ttlMs: number | undefined } -// Why: runtime-backed AND SSH-connected tabs both take a watcher echo on a -// poll-plus-network path that lands later than a local one, so both need the -// longer remote TTL. An SSH tab can carry runtimeEnvironmentId=null yet still be -// remote via its worktree connection, so key remoteness off either signal. function collectOpenTabMoves(fromPath: string, toPath: string): OpenTabMove[] { const moves: OpenTabMove[] = [] for (const file of useAppStore.getState().openFiles) { @@ -27,12 +23,13 @@ function collectOpenTabMoves(fromPath: string, toPath: string): OpenTabMove[] { continue } const runtimeOwner = file.runtimeEnvironmentId?.trim() || null + // Remote via runtime owner OR an SSH worktree connection (an SSH tab can have + // a null runtime owner); both take a later poll-plus-network watcher echo. const isRemote = runtimeOwner !== null || !!getConnectionIdForFile(file.worktreeId, file.filePath) moves.push({ file, - // Mirror remap's absolute-path suffix swap so a directory move stamps each - // contained tab's real new path. + // Mirror remap's suffix swap so a directory move stamps each tab's new path. newFilePath: toPath + file.filePath.slice(fromPath.length), runtimeOwner, ttlMs: isRemote ? SELF_MOVE_REMOTE_TTL_MS : undefined @@ -41,26 +38,15 @@ function collectOpenTabMoves(fromPath: string, toPath: string): OpenTabMove[] { return moves } -/** - * Stamps every open editor tab affected by an Orca-initiated move so the - * worktree watcher's delete(old)+create(new) echo is recognized as - * self-initiated (see editor-self-move-registry). - * - * Prefer `renameOpenTabsPathOnDisk`, which brackets the on-disk rename with the - * before/after stamps and the failure clear. This is exported for that wrapper - * (and tests); call it directly only when you own the rename lifecycle. - */ +/** Stamps every open tab a move affects. Prefer `renameOpenTabsPathOnDisk`, + * which brackets the rename with the stamps; exported for it and tests. */ export function recordSelfMoveForOpenTabs(fromPath: string, toPath: string): SelfMoveTicket[] { return collectOpenTabMoves(fromPath, toPath).map((move) => recordSelfMove(move.file.filePath, move.newFilePath, move.runtimeOwner, move.ttlMs) ) } -/** - * Retracts the exact registrations from {@link recordSelfMoveForOpenTabs} for a - * move that did not happen. Takes the tickets that call returned so a concurrent - * move's stamp on a shared path is never dropped. - */ +/** Retracts the exact registrations {@link recordSelfMoveForOpenTabs} returned. */ export function clearSelfMoveForOpenTabs(tickets: SelfMoveTicket[]): void { for (const ticket of tickets) { clearSelfMove(ticket) diff --git a/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.test.ts b/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.test.ts index d641666359a..2a25e039db3 100644 --- a/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.test.ts +++ b/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.test.ts @@ -192,4 +192,36 @@ describe('remapOpenEditorTabsForPathChange', () => { }) expect(useAppStore.getState().openFiles[0].isUntitled).toBeUndefined() }) + + it('carries the disk baseline and live conflict state onto the re-homed tab', () => { + const state = useAppStore.getState() + const oldPath = '/repo/notes.md' + const newPath = '/repo/archive/notes.md' + state.openFile( + { + filePath: oldPath, + relativePath: 'notes.md', + worktreeId: 'wt-1', + runtimeEnvironmentId: null, + language: 'markdown', + mode: 'edit' + }, + { suppressActiveRuntimeFallback: true } + ) + const oldId = useAppStore.getState().openFiles[0]!.id + state.setEditorDraft(oldId, 'unsaved work') + state.markFileDirty(oldId, true) + state.setLastKnownDiskSignature(oldId, 'sig-abc') + state.setExternalMutation(oldId, 'changed') + + remapOpenEditorTabsForPathChange({ fromPath: oldPath, toPath: newPath, worktreePath: '/repo' }) + + const moved = useAppStore.getState().openFiles.find((f) => f.filePath === newPath) + expect(moved).toBeTruthy() + // The re-homed tab must keep the identity that lets the watcher distinguish + // the move echo from a real external write, and any pre-existing conflict. + expect(moved?.lastKnownDiskSignature).toBe('sig-abc') + expect(moved?.externalMutation).toBe('changed') + expect(moved?.isDirty).toBe(true) + }) }) diff --git a/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.ts b/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.ts index 7432ac77c5f..b9fba2f98b2 100644 --- a/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.ts +++ b/src/renderer/src/lib/remap-open-editor-tabs-for-path-change.ts @@ -157,12 +157,14 @@ export function remapOpenEditorTabsForPathChange({ initiatingWorktreeId: worktreeId, initiatingWorktreePath: worktreePath }) - // Why: the self-move stamp that suppresses the watcher echo is recorded at - // the call site BEFORE the on-disk rename (recordSelfMoveForOpenTabs), not - // here — the watcher can observe the physical move before this remap runs, - // so stamping after re-home would lose the race for the source-side delete. const draft = state.editorDrafts[file.id] const wasDirty = file.isDirty + // Carry the edit-session identity forward: the disk baseline lets the + // re-homed tab tell an external write from the move's own watcher echo, and + // any live conflict/verification state must not be dropped by the re-home. + const carriedSignature = file.lastKnownDiskSignature + const carriedMutation = file.externalMutation + const carriedPendingVerification = file.pendingDiskBaselineVerification // Why: renameRuntimePath already moved the file. Clear the untitled marker // before closeFile so its cleanup path does not try to delete the old path. @@ -225,5 +227,14 @@ export function remapOpenEditorTabsForPathChange({ if (wasDirty) { freshState.markFileDirty(reopenedFileId, true) } + if (carriedSignature !== undefined) { + freshState.setLastKnownDiskSignature(reopenedFileId, carriedSignature) + } + if (carriedMutation) { + freshState.setExternalMutation(reopenedFileId, carriedMutation) + } + if (carriedPendingVerification) { + freshState.setPendingDiskBaselineVerification(reopenedFileId, true) + } } } diff --git a/src/renderer/src/lib/rename-open-editor-tabs-path.ts b/src/renderer/src/lib/rename-open-editor-tabs-path.ts index fe55a783c8b..2193dd4b2a0 100644 --- a/src/renderer/src/lib/rename-open-editor-tabs-path.ts +++ b/src/renderer/src/lib/rename-open-editor-tabs-path.ts @@ -5,18 +5,10 @@ import { } from './record-self-move-for-open-tabs' /** - * Renames a path on disk that may back open editor tabs, keeping the editor's - * move-tracking correct so the watcher echo isn't mistaken for an external edit. - * - * Why the stamp is recorded both BEFORE and AFTER the rename: - * - Before: the main-process watcher detects the physical move independently and - * can push its `fs:changed` event before this `await` resumes — the pre-stamp - * closes that race structurally instead of hoping to win it. - * - After: the rename is an awaited round-trip that on a slow SSH/runtime host - * can outlive the stamp's TTL; re-stamping on success gives a fresh window - * that covers watcher latency measured from when the file actually moved. - * On failure the stamps are cleared so a rename that never happened can't - * suppress genuine events for the untouched paths. + * Renames an on-disk path that may back open editor tabs, stamping the move so + * the watcher echo isn't mistaken for an external edit. Stamp BEFORE the rename + * (the watcher can fire before this `await` resumes), re-stamp on success (a slow + * remote rename can outlive the first TTL), and retract on failure. */ export async function renameOpenTabsPathOnDisk( context: RuntimeFileOperationArgs, diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index bd2bc1fddeb..357b6232882 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -527,6 +527,7 @@ export type EditorSlice = { setExternalMutation: (fileId: string, mutation: 'deleted' | 'renamed' | 'changed' | null) => void setLastKnownDiskSignature: (fileId: string, signature: string) => void clearPendingDiskBaselineVerification: (fileId: string) => void + setPendingDiskBaselineVerification: (fileId: string, value: boolean) => void clearUntitled: (fileId: string) => void openDiff: ( worktreeId: string, @@ -2555,6 +2556,20 @@ export const createEditorSlice: StateCreator = (s } }), + setPendingDiskBaselineVerification: (fileId, value) => + set((s) => { + const file = s.openFiles.find((f) => f.id === fileId) + const next = value || undefined + if (!file || file.pendingDiskBaselineVerification === next) { + return s + } + return { + openFiles: s.openFiles.map((f) => + f.id === fileId ? { ...f, pendingDiskBaselineVerification: next } : f + ) + } + }), + clearUntitled: (fileId) => set((s) => ({ openFiles: s.openFiles.map((f) => (f.id === fileId ? { ...f, isUntitled: undefined } : f))