mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 00:03:20 +00:00
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 <help@stably.ai>
This commit is contained in:
@@ -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)
|
||||
})
|
||||
})
|
||||
@@ -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<string, SelfMoveStamp>()
|
||||
|
||||
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
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user