From c60669d278c1fcdcb905ea04b8a38f4678a8dc3d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 19 Jul 2026 19:59:11 -0700 Subject: [PATCH] fix(editor): don't flag editor-initiated moves as changed-on-disk An in-app move/rename (explorer drag-drop, inline rename, tab rename) re-homes the open tab to the new path and carries its unsaved draft forward. The move also physically relocates the file, which the worktree watcher reports as delete(old)+create(new) a few ms later. Because the tab already lives at the new path by then, that create echo was treated as an external write landing on a dirty tab and raised a spurious "changed on disk" banner. Add a short-lived self-move registry (the move analog of the existing self-write registry) stamped at the single remap choke point, and have the external-watch handler recognize the move's own watcher echo: suppress the changed-on-disk mark on the re-homed dirty tab and the tombstone on the source path. Genuine external edits are unaffected. Covered by unit tests for the registry, the remap recorder, and the watch-hook suppression (plus a no-over-suppression guard). Co-authored-by: Orca --- .../editor/editor-self-move-registry.test.ts | 65 +++++++++ .../editor/editor-self-move-registry.ts | 123 ++++++++++++++++++ .../src/hooks/useEditorExternalWatch.test.ts | 60 +++++++++ .../src/hooks/useEditorExternalWatch.ts | 22 +++- ...p-open-editor-tabs-for-path-change.test.ts | 43 +++++- .../remap-open-editor-tabs-for-path-change.ts | 17 +++ 6 files changed, 326 insertions(+), 4 deletions(-) create mode 100644 src/renderer/src/components/editor/editor-self-move-registry.test.ts create mode 100644 src/renderer/src/components/editor/editor-self-move-registry.ts diff --git a/src/renderer/src/components/editor/editor-self-move-registry.test.ts b/src/renderer/src/components/editor/editor-self-move-registry.test.ts new file mode 100644 index 00000000000..03630e3722c --- /dev/null +++ b/src/renderer/src/components/editor/editor-self-move-registry.test.ts @@ -0,0 +1,65 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + __clearSelfMoveRegistryForTests, + __getSelfMoveRegistrySizeForTests, + isRecentSelfMoveSource, + isRecentSelfMoveTarget, + recordSelfMove, + SELF_MOVE_REMOTE_TTL_MS +} from './editor-self-move-registry' + +describe('editor-self-move-registry', () => { + beforeEach(() => { + vi.useFakeTimers() + __clearSelfMoveRegistryForTests() + }) + + afterEach(() => { + vi.useRealTimers() + __clearSelfMoveRegistryForTests() + }) + + it('records both endpoints with the right direction', () => { + recordSelfMove('/repo/old.md', '/repo/sub/new.md') + + expect(isRecentSelfMoveSource('/repo/old.md')).toBe(true) + expect(isRecentSelfMoveTarget('/repo/sub/new.md')).toBe(true) + // Directions don't cross: the source is not a target and vice-versa. + expect(isRecentSelfMoveTarget('/repo/old.md')).toBe(false) + expect(isRecentSelfMoveSource('/repo/sub/new.md')).toBe(false) + }) + + it('expires stamps after the TTL', () => { + recordSelfMove('/repo/old.md', '/repo/new.md', undefined, 750) + expect(isRecentSelfMoveTarget('/repo/new.md')).toBe(true) + + vi.advanceTimersByTime(751) + expect(isRecentSelfMoveTarget('/repo/new.md')).toBe(false) + expect(isRecentSelfMoveSource('/repo/old.md')).toBe(false) + }) + + it('honors the longer remote TTL for runtime-backed moves', () => { + recordSelfMove('/repo/old.md', '/repo/new.md', 'env-1', SELF_MOVE_REMOTE_TTL_MS) + + vi.advanceTimersByTime(1000) + // A local-sized window would already have expired; the remote TTL keeps it. + expect(isRecentSelfMoveTarget('/repo/new.md', 'env-1')).toBe(true) + }) + + it('isolates stamps by runtime owner', () => { + recordSelfMove('/repo/old.md', '/repo/new.md', 'env-1') + + expect(isRecentSelfMoveTarget('/repo/new.md', 'env-1')).toBe(true) + // A different owner (or the local client) must not match the remote stamp. + expect(isRecentSelfMoveTarget('/repo/new.md', 'env-2')).toBe(false) + expect(isRecentSelfMoveTarget('/repo/new.md')).toBe(false) + }) + + it('caps the number of stamps', () => { + for (let i = 0; i < 400; i++) { + recordSelfMove(`/repo/from-${i}.md`, `/repo/to-${i}.md`) + } + // Two endpoints per move, bounded by the 256 cap. + expect(__getSelfMoveRegistrySizeForTests()).toBeLessThanOrEqual(256) + }) +}) diff --git a/src/renderer/src/components/editor/editor-self-move-registry.ts b/src/renderer/src/components/editor/editor-self-move-registry.ts new file mode 100644 index 00000000000..0979e4e6be1 --- /dev/null +++ b/src/renderer/src/components/editor/editor-self-move-registry.ts @@ -0,0 +1,123 @@ +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 right before the remap 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. +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. +export const SELF_MOVE_REMOTE_TTL_MS = 3000 +const SELF_MOVE_MAX_STAMPS = 256 + +type SelfMoveDirection = 'source' | 'target' + +type SelfMoveStamp = { + direction: SelfMoveDirection + expiresAt: number +} + +const stamps = new Map() + +function selfMoveKey(absolutePath: string, runtimeEnvironmentId?: string | null): string { + return `${runtimeEnvironmentId?.trim() || 'client'}::${normalizeAbsolutePathForComparison(absolutePath)}` +} + +function pruneExpiredSelfMoves(now: number): void { + for (const [key, stamp] of stamps) { + if (now > stamp.expiresAt) { + stamps.delete(key) + } + } +} + +function enforceSelfMoveStampLimit(): void { + while (stamps.size > SELF_MOVE_MAX_STAMPS) { + const oldest = stamps.keys().next().value + if (oldest === undefined) { + break + } + stamps.delete(oldest) + } +} + +function stampSelfMoveEndpoint( + absolutePath: string, + direction: SelfMoveDirection, + runtimeEnvironmentId: string | null | undefined, + expiresAt: number +): void { + const key = selfMoveKey(absolutePath, runtimeEnvironmentId) + stamps.delete(key) + stamps.set(key, { direction, expiresAt }) +} + +/** + * 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 a pre-remap delete does not tombstone + * the tab, the target so the post-remap create does not raise a changed-on-disk + * banner on the (already re-homed, still-dirty) tab. + */ +export function recordSelfMove( + fromPath: string, + toPath: string, + runtimeEnvironmentId?: string | null, + ttlMs: number = SELF_MOVE_TTL_MS +): void { + const now = Date.now() + pruneExpiredSelfMoves(now) + const expiresAt = now + ttlMs + stampSelfMoveEndpoint(fromPath, 'source', runtimeEnvironmentId, expiresAt) + stampSelfMoveEndpoint(toPath, 'target', runtimeEnvironmentId, expiresAt) + enforceSelfMoveStampLimit() +} + +function hasRecentSelfMove( + absolutePath: string, + direction: SelfMoveDirection, + runtimeEnvironmentId?: string | null +): boolean { + const key = selfMoveKey(absolutePath, runtimeEnvironmentId) + const stamp = stamps.get(key) + if (!stamp) { + return false + } + if (Date.now() > stamp.expiresAt) { + stamps.delete(key) + return false + } + return stamp.direction === direction +} + +/** True when `path` is the destination of a recent Orca-initiated move. */ +export function isRecentSelfMoveTarget( + absolutePath: string, + runtimeEnvironmentId?: string | null +): boolean { + return hasRecentSelfMove(absolutePath, 'target', runtimeEnvironmentId) +} + +/** True when `path` is the origin of a recent Orca-initiated move. */ +export function isRecentSelfMoveSource( + absolutePath: string, + runtimeEnvironmentId?: string | null +): boolean { + return hasRecentSelfMove(absolutePath, 'source', runtimeEnvironmentId) +} + +export function __clearSelfMoveRegistryForTests(): void { + stamps.clear() +} + +export function __getSelfMoveRegistrySizeForTests(): number { + return stamps.size +} diff --git a/src/renderer/src/hooks/useEditorExternalWatch.test.ts b/src/renderer/src/hooks/useEditorExternalWatch.test.ts index 90029020d0c..81c9f1f29cc 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch.test.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch.test.ts @@ -33,6 +33,10 @@ import { __clearSelfWriteRegistryForTests, recordSelfWrite } from '@/components/editor/editor-self-write-registry' +import { + __clearSelfMoveRegistryForTests, + recordSelfMove +} from '@/components/editor/editor-self-move-registry' describe('getWatchedTargetKey', () => { it('changes when a worktree gains an SSH connection id', () => { @@ -225,6 +229,7 @@ describe('createExternalWatchEventHandler tombstone coalescing', () => { vi.useRealTimers() vi.unstubAllGlobals() __clearSelfWriteRegistryForTests() + __clearSelfMoveRegistryForTests() }) function payload(events: FsChangedPayload['events']): FsChangedPayload { @@ -541,6 +546,61 @@ 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('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 9fd18202b98..7bba587cd4d 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch.ts @@ -19,6 +19,10 @@ import { getRecentSelfWrite, type RecentSelfWrite } from '@/components/editor/editor-self-write-registry' +import { + isRecentSelfMoveSource, + isRecentSelfMoveTarget +} from '@/components/editor/editor-self-move-registry' import type { FsChangedPayload } from '../../../shared/types' import { findWorktreeById } from '@/store/slices/worktree-helpers' import type { OpenFile } from '@/store/slices/editor' @@ -486,7 +490,13 @@ export function createExternalWatchEventHandler( target.worktreeId, 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. + const file = openFilesAtStart.find((f) => f.id === fileId) + return !file || !isRecentSelfMoveSource(file.filePath, target.runtimeEnvironmentId) + }) // Why: correlate creates to deletes by basename OR parent directory to // avoid mislabelling unrelated create+delete pairs in a batched payload // as "renamed". When we can't correlate, default to 'deleted' — that's @@ -642,8 +652,15 @@ export function createExternalWatchEventHandler( } continue } + const absolutePath = joinPath(notification.worktreePath, notification.relativePath) const dirtyMatches = matching.filter((f) => f.isDirty) - if (dirtyMatches.length > 0) { + // Why: an Orca-initiated move re-homes the dirty tab to this path and + // carries its draft. The move's own create(new) 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. Reload of a + // clean moved tab (below) is harmless, so only the dirty mark is gated. + 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 @@ -664,7 +681,6 @@ export function createExternalWatchEventHandler( // Clean sibling tabs (e.g. an unstaged diff of the same path) still // reload below; every notification consumer skips dirty files. } - const absolutePath = joinPath(notification.worktreePath, notification.relativePath) const recentSelfWrite = getRecentSelfWrite(absolutePath, target.runtimeEnvironmentId) if (recentSelfWrite) { scheduleSelfWriteAwareExternalReload(target, notification, matching[0], recentSelfWrite) 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..be889bd60a3 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 @@ -1,7 +1,12 @@ -import { beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { useAppStore } from '@/store' import { remapOpenEditorTabsForPathChange } from './remap-open-editor-tabs-for-path-change' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' +import { + __clearSelfMoveRegistryForTests, + isRecentSelfMoveSource, + isRecentSelfMoveTarget +} from '@/components/editor/editor-self-move-registry' function ownedEditorFileId( filePath: string, @@ -15,6 +20,11 @@ function ownedEditorFileId( describe('remapOpenEditorTabsForPathChange', () => { beforeEach(() => { useAppStore.setState(useAppStore.getInitialState(), true) + __clearSelfMoveRegistryForTests() + }) + + afterEach(() => { + __clearSelfMoveRegistryForTests() }) it('preserves runtime owners, drafts, dirty state, and markdown preview sources', () => { @@ -192,4 +202,35 @@ describe('remapOpenEditorTabsForPathChange', () => { }) expect(useAppStore.getState().openFiles[0].isUntitled).toBeUndefined() }) + + it('records a self-move so the watcher echo is not seen as an external change', () => { + const state = useAppStore.getState() + const oldPath = '/repo/docs/readme.md' + const newPath = '/repo/notes/readme.md' + + state.openFile( + { + filePath: oldPath, + relativePath: 'docs/readme.md', + worktreeId: 'wt-1', + runtimeEnvironmentId: null, + language: 'markdown', + mode: 'edit' + }, + { suppressActiveRuntimeFallback: true } + ) + const editId = useAppStore.getState().openFiles[0]?.id + state.setEditorDraft(editId!, 'unsaved work') + state.markFileDirty(editId!, true) + + remapOpenEditorTabsForPathChange({ + fromPath: oldPath, + toPath: newPath, + worktreePath: '/repo', + worktreeId: 'wt-1' + }) + + expect(isRecentSelfMoveSource(oldPath)).toBe(true) + expect(isRecentSelfMoveTarget(newPath)).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 5f354f06b1a..0d31ce76d83 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 @@ -1,6 +1,10 @@ import { useAppStore } from '@/store' import { detectLanguage } from '@/lib/language-detect' import { basename } from '@/lib/path' +import { + recordSelfMove, + SELF_MOVE_REMOTE_TTL_MS +} from '@/components/editor/editor-self-move-registry' import { normalizeRuntimePathSeparators, relativePathInsideRoot @@ -160,6 +164,19 @@ export function remapOpenEditorTabsForPathChange({ const draft = state.editorDrafts[file.id] const wasDirty = file.isDirty + // Why: stamp this Orca-initiated move BEFORE re-homing the tab so the + // worktree watcher's delete(old)+create(new) echo is recognized as + // self-initiated. Without it, the create(new) landing on the re-homed dirty + // tab raises a false "changed on disk" banner whose Reload discards the + // draft. Runtime-backed tabs use the longer remote TTL because their watcher + // echo travels a poll-plus-network path. + recordSelfMove( + oldFilePath, + updatedPath, + file.runtimeEnvironmentId, + file.runtimeEnvironmentId ? SELF_MOVE_REMOTE_TTL_MS : undefined + ) + // Why: renameRuntimePath already moved the file. Clear the untitled marker // before closeFile so its cleanup path does not try to delete the old path. if (file.isUntitled) {