mirror of
https://github.com/stablyai/orca.git
synced 2026-10-26 16:02:42 +00:00
feat(editor): decide move echo vs external write by content identity
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 <help@stably.ai>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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<typeof EditorAutosaveModule>()
|
||||
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<string, unknown> = {}): Record<string, unknown> {
|
||||
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<string, unknown>): 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()
|
||||
})
|
||||
})
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, number>()
|
||||
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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<AppState, [], [], EditorSlice> = (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))
|
||||
|
||||
Reference in New Issue
Block a user