mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 00:03:20 +00:00
fix(editor): resolve the move verify gate proactively (review round 3)
The rekey gates every dirty moved tab pending a destination content check, but verification only ran when a watcher event arrived for that path. If the watcher was down, throttled, or the event coalesced away, the gate never cleared and autosave stayed suspended for the tab. The coordinator now drives verification for every tab it gated once the rename has committed, so the gate resolves on its own. That makes the destination-side event latch redundant, so the in-flight tracker is source-only again. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -4,7 +4,6 @@ import {
|
||||
__clearEditorPathMovesForTests,
|
||||
beginEditorPathMove,
|
||||
isActiveMoveSourcePath,
|
||||
noteEditorPathMoveDestinationEvent,
|
||||
settleEditorPathMove
|
||||
} from './editor-path-move-inflight'
|
||||
|
||||
@@ -16,8 +15,7 @@ describe('editor-path-move-inflight', () => {
|
||||
operationId: 'op-1',
|
||||
worktreeId: 'wt-1',
|
||||
runtimeEnvironmentId: null,
|
||||
sourcePaths: ['/repo/a.md'],
|
||||
targetPaths: ['/repo/sub/a.md']
|
||||
sourcePaths: ['/repo/a.md']
|
||||
})
|
||||
expect(isActiveMoveSourcePath('wt-1', null, '/repo/a.md')).toBe(true)
|
||||
|
||||
@@ -31,44 +29,25 @@ describe('editor-path-move-inflight', () => {
|
||||
operationId: 'op-1',
|
||||
worktreeId: 'wt-1',
|
||||
runtimeEnvironmentId: 'env-1',
|
||||
sourcePaths: ['/repo/a.md'],
|
||||
targetPaths: []
|
||||
sourcePaths: ['/repo/a.md']
|
||||
})
|
||||
expect(isActiveMoveSourcePath('wt-1', 'env-1', '/repo/a.md')).toBe(true)
|
||||
expect(isActiveMoveSourcePath('wt-1', null, '/repo/a.md')).toBe(false)
|
||||
expect(isActiveMoveSourcePath('wt-2', 'env-1', '/repo/a.md')).toBe(false)
|
||||
})
|
||||
|
||||
it('latches destination events and returns them on settle', () => {
|
||||
beginEditorPathMove({
|
||||
operationId: 'op-1',
|
||||
worktreeId: 'wt-1',
|
||||
runtimeEnvironmentId: null,
|
||||
sourcePaths: ['/repo/a.md'],
|
||||
targetPaths: ['/repo/sub/a.md']
|
||||
})
|
||||
// A destination event that arrives before the rekey is latched, not lost.
|
||||
expect(noteEditorPathMoveDestinationEvent('wt-1', null, '/repo/sub/a.md')).toBe(true)
|
||||
expect(noteEditorPathMoveDestinationEvent('wt-1', null, '/repo/other.md')).toBe(false)
|
||||
|
||||
const latched = settleEditorPathMove('op-1')
|
||||
expect(latched).toEqual(['/repo/sub/a.md'.toLowerCase()])
|
||||
})
|
||||
|
||||
it('keeps concurrent operations independent (settling one leaves the other)', () => {
|
||||
beginEditorPathMove({
|
||||
operationId: 'op-1',
|
||||
worktreeId: 'wt-1',
|
||||
runtimeEnvironmentId: null,
|
||||
sourcePaths: ['/repo/a.md'],
|
||||
targetPaths: []
|
||||
sourcePaths: ['/repo/a.md']
|
||||
})
|
||||
beginEditorPathMove({
|
||||
operationId: 'op-2',
|
||||
worktreeId: 'wt-1',
|
||||
runtimeEnvironmentId: null,
|
||||
sourcePaths: ['/repo/b.md'],
|
||||
targetPaths: []
|
||||
sourcePaths: ['/repo/b.md']
|
||||
})
|
||||
|
||||
settleEditorPathMove('op-1')
|
||||
|
||||
@@ -5,20 +5,15 @@ import { normalizeAbsolutePathForComparison } from '@/components/right-sidebar/f
|
||||
// for the exact duration of its filesystem rename + atomic rekey, so there is
|
||||
// no TTL to race a slow SSH rename and no cross-operation aliasing.
|
||||
//
|
||||
// Source side: while an operation is live, a watcher delete of one of its exact
|
||||
// source paths is the move's own echo, not an external delete — don't tombstone.
|
||||
// Destination side: a create/update at a target path may be the move's echo but
|
||||
// may also be a concurrent external write; the operation only LATCHES that such
|
||||
// an event was seen (before the rekey installs the tab) so the coordinator can
|
||||
// route it into content verification after commit — the latch never suppresses.
|
||||
// Only the source side needs this: while an operation is live, a watcher delete
|
||||
// of one of its exact source paths is the move's own echo, not an external
|
||||
// delete — don't tombstone. The destination is handled after commit: the
|
||||
// coordinator proactively content-verifies every gated moved tab.
|
||||
|
||||
type MoveOperation = {
|
||||
operationId: string
|
||||
worktreeId: string
|
||||
runtimeEnvironmentId: string | null
|
||||
sourcePaths: Set<string>
|
||||
targetPaths: Set<string>
|
||||
destinationSeen: Set<string>
|
||||
}
|
||||
|
||||
const operations = new Map<string, MoveOperation>()
|
||||
@@ -36,38 +31,16 @@ export function beginEditorPathMove(args: {
|
||||
worktreeId: string
|
||||
runtimeEnvironmentId: string | null | undefined
|
||||
sourcePaths: readonly string[]
|
||||
targetPaths: readonly string[]
|
||||
}): void {
|
||||
operations.set(args.operationId, {
|
||||
operationId: args.operationId,
|
||||
worktreeId: args.worktreeId,
|
||||
runtimeEnvironmentId: owner(args.runtimeEnvironmentId),
|
||||
sourcePaths: new Set(args.sourcePaths.map(normalize)),
|
||||
targetPaths: new Set(args.targetPaths.map(normalize)),
|
||||
destinationSeen: new Set()
|
||||
sourcePaths: new Set(args.sourcePaths.map(normalize))
|
||||
})
|
||||
}
|
||||
|
||||
/** Removes the operation and returns the target paths whose destination event
|
||||
* was latched while in flight, so the coordinator can verify them post-rekey. */
|
||||
export function settleEditorPathMove(operationId: string): string[] {
|
||||
const operation = operations.get(operationId)
|
||||
export function settleEditorPathMove(operationId: string): void {
|
||||
operations.delete(operationId)
|
||||
return operation ? Array.from(operation.destinationSeen) : []
|
||||
}
|
||||
|
||||
function matches(
|
||||
operation: MoveOperation,
|
||||
worktreeId: string,
|
||||
runtimeEnvironmentId: string | null,
|
||||
normalizedPath: string,
|
||||
paths: Set<string>
|
||||
): boolean {
|
||||
return (
|
||||
operation.worktreeId === worktreeId &&
|
||||
operation.runtimeEnvironmentId === runtimeEnvironmentId &&
|
||||
paths.has(normalizedPath)
|
||||
)
|
||||
}
|
||||
|
||||
/** True when this delete is the source side of a live Orca-owned move. */
|
||||
@@ -79,33 +52,17 @@ export function isActiveMoveSourcePath(
|
||||
const normalizedPath = normalize(absolutePath)
|
||||
const scopedOwner = owner(runtimeEnvironmentId)
|
||||
for (const operation of operations.values()) {
|
||||
if (matches(operation, worktreeId, scopedOwner, normalizedPath, operation.sourcePaths)) {
|
||||
if (
|
||||
operation.worktreeId === worktreeId &&
|
||||
operation.runtimeEnvironmentId === scopedOwner &&
|
||||
operation.sourcePaths.has(normalizedPath)
|
||||
) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
/** Latch that a destination event was observed for a live move (used when the
|
||||
* event arrives before the atomic rekey installs the tab). Returns true if it
|
||||
* belonged to a live move. Never suppresses on its own. */
|
||||
export function noteEditorPathMoveDestinationEvent(
|
||||
worktreeId: string,
|
||||
runtimeEnvironmentId: string | null | undefined,
|
||||
absolutePath: string
|
||||
): boolean {
|
||||
const normalizedPath = normalize(absolutePath)
|
||||
const scopedOwner = owner(runtimeEnvironmentId)
|
||||
let latched = false
|
||||
for (const operation of operations.values()) {
|
||||
if (matches(operation, worktreeId, scopedOwner, normalizedPath, operation.targetPaths)) {
|
||||
operation.destinationSeen.add(normalizedPath)
|
||||
latched = true
|
||||
}
|
||||
}
|
||||
return latched
|
||||
}
|
||||
|
||||
export function __clearEditorPathMovesForTests(): void {
|
||||
operations.clear()
|
||||
}
|
||||
|
||||
@@ -67,8 +67,7 @@ describe('self-move source suppression (in-flight)', () => {
|
||||
operationId: 'op-1',
|
||||
worktreeId: 'wt-1',
|
||||
runtimeEnvironmentId: null,
|
||||
sourcePaths: ['/repo/notes.md'],
|
||||
targetPaths: ['/repo/subdir/notes.md']
|
||||
sourcePaths: ['/repo/notes.md']
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -19,10 +19,7 @@ import {
|
||||
getRecentSelfWrite,
|
||||
type RecentSelfWrite
|
||||
} from '@/components/editor/editor-self-write-registry'
|
||||
import {
|
||||
isActiveMoveSourcePath,
|
||||
noteEditorPathMoveDestinationEvent
|
||||
} from '@/components/editor/editor-path-move-inflight'
|
||||
import { isActiveMoveSourcePath } from '@/components/editor/editor-path-move-inflight'
|
||||
import type { FsChangedPayload } from '../../../shared/types'
|
||||
import { findWorktreeById } from '@/store/slices/worktree-helpers'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
@@ -647,14 +644,6 @@ export function createExternalWatchEventHandler(
|
||||
const absolutePath = joinPath(notification.worktreePath, notification.relativePath)
|
||||
const matching = getOpenFilesForExternalFileChange(openFilesSnapshot, notification)
|
||||
if (matching.length === 0) {
|
||||
// Latch a destination event that arrived before the rekey installed the
|
||||
// tab (drag-drop's refresh-before-remap window), so the coordinator can
|
||||
// content-verify it post-rekey instead of dropping it.
|
||||
noteEditorPathMoveDestinationEvent(
|
||||
target.worktreeId,
|
||||
target.runtimeEnvironmentId,
|
||||
absolutePath
|
||||
)
|
||||
// Why: notify the combined-diff tab so its section reloads. Its own
|
||||
// dirty/section guards make a blanket reload safe, and there is no
|
||||
// in-memory editor content to clobber, so self-write suppression is
|
||||
|
||||
@@ -15,6 +15,7 @@ vi.mock('@/components/editor/editor-autosave', async (importOriginal) => {
|
||||
import { useAppStore } from '@/store'
|
||||
import { executeOpenEditorPathMove } from './execute-open-editor-path-move'
|
||||
import { __activeEditorPathMoveCountForTests } from '@/components/editor/editor-path-move-inflight'
|
||||
import { getDiskBaselineSignature } from '@/components/editor/diff-content-signature'
|
||||
|
||||
const CONTEXT = {
|
||||
settings: null,
|
||||
@@ -77,6 +78,33 @@ describe('executeOpenEditorPathMove', () => {
|
||||
expect(__activeEditorPathMoveCountForTests()).toBe(0)
|
||||
})
|
||||
|
||||
it('resolves the verify gate proactively when no destination watcher event arrives', async () => {
|
||||
const DISK_CONTENT = 'the file as it exists on disk\n'
|
||||
const id = openDirtyTab()
|
||||
useAppStore.getState().setLastKnownDiskSignature(id, getDiskBaselineSignature(DISK_CONTENT))
|
||||
vi.stubGlobal('window', {
|
||||
api: {
|
||||
fs: { readFile: vi.fn().mockResolvedValue({ isBinary: false, content: DISK_CONTENT }) }
|
||||
}
|
||||
})
|
||||
|
||||
await executeOpenEditorPathMove({
|
||||
context: CONTEXT,
|
||||
fromPath: '/repo/a.md',
|
||||
toPath: '/repo/sub/a.md',
|
||||
worktreeId: 'wt-1',
|
||||
worktreePath: '/repo'
|
||||
})
|
||||
await new Promise((resolve) => setTimeout(resolve, 20))
|
||||
|
||||
const moved = useAppStore.getState().openFiles[0]!
|
||||
// Disk matched the carried baseline: gate released, no false conflict banner.
|
||||
expect(moved.pendingLiveDiskVerification).toBeFalsy()
|
||||
expect(moved.pendingSelfMoveEcho).toBeUndefined()
|
||||
expect(moved.externalMutation).toBeUndefined()
|
||||
expect(moved.isDirty).toBe(true)
|
||||
})
|
||||
|
||||
it('undoes the on-disk rename when the editor rekey collides', async () => {
|
||||
openDirtyTab()
|
||||
// A distinct live session already occupies the destination id.
|
||||
|
||||
@@ -10,7 +10,6 @@ import {
|
||||
remapOpenEditorTabsForPathChange
|
||||
} from '@/lib/remap-open-editor-tabs-for-path-change'
|
||||
import { verifyLatchedMoveDestinations } from '@/hooks/useEditorExternalWatch'
|
||||
import { normalizeAbsolutePathForComparison } from '@/components/right-sidebar/file-explorer-paths'
|
||||
import { notifyHostOfMirroredEditorClose } from '@/runtime/close-mirrored-editor-tab'
|
||||
|
||||
let moveOperationCounter = 0
|
||||
@@ -40,7 +39,6 @@ export async function executeOpenEditorPathMove(args: {
|
||||
const affected = useAppStore
|
||||
.getState()
|
||||
.openFiles.filter((f) => isPathInsideOrEqual(fromPath, f.filePath))
|
||||
const newPathOf = (filePath: string): string => toPath + filePath.slice(fromPath.length)
|
||||
|
||||
// In-flight source suppression is scoped per (worktree, runtime owner): the
|
||||
// same path can be open and watched under more than one worktree/owner.
|
||||
@@ -59,8 +57,7 @@ export async function executeOpenEditorPathMove(args: {
|
||||
operationId: subOperationId,
|
||||
worktreeId: first.worktreeId,
|
||||
runtimeEnvironmentId: first.runtimeEnvironmentId?.trim() || null,
|
||||
sourcePaths: scopeFiles.map((f) => f.filePath),
|
||||
targetPaths: scopeFiles.map((f) => newPathOf(f.filePath))
|
||||
sourcePaths: scopeFiles.map((f) => f.filePath)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -122,21 +119,21 @@ export async function executeOpenEditorPathMove(args: {
|
||||
notifyHostOfMirroredEditorClose(mirrorState, file.worktreeId, file.id)
|
||||
}
|
||||
|
||||
const latchedTargets = new Set<string>()
|
||||
for (const subOperationId of ownerSubOps) {
|
||||
for (const target of settleEditorPathMove(subOperationId)) {
|
||||
latchedTargets.add(target)
|
||||
}
|
||||
settleEditorPathMove(subOperationId)
|
||||
}
|
||||
if (latchedTargets.size > 0) {
|
||||
const latchedTabIds = useAppStore
|
||||
.getState()
|
||||
.openFiles.filter(
|
||||
(f) =>
|
||||
f.pendingSelfMoveEcho &&
|
||||
latchedTargets.has(normalizeAbsolutePathForComparison(f.pendingSelfMoveEcho.targetPath))
|
||||
)
|
||||
.map((f) => f.id)
|
||||
verifyLatchedMoveDestinations(worktreePath, context.connectionId, latchedTabIds)
|
||||
|
||||
// Drive verification for EVERY tab the rekey gated — not just paths whose
|
||||
// watcher event was latched. The rename has completed, so disk is authoritative
|
||||
// now; verifying proactively guarantees the autosave gate resolves even if the
|
||||
// destination watcher event never arrives (watcher down / dropped / coalesced).
|
||||
// A move echo reads disk == baseline (suppress); a genuine write differs
|
||||
// (banner). A later watcher event finds the provenance consumed and no-ops.
|
||||
const gatedTabIds = useAppStore
|
||||
.getState()
|
||||
.openFiles.filter((f) => f.pendingSelfMoveEcho?.operationId === operationId)
|
||||
.map((f) => f.id)
|
||||
if (gatedTabIds.length > 0) {
|
||||
verifyLatchedMoveDestinations(worktreePath, context.connectionId, gatedTabIds)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user