-
Notifications
You must be signed in to change notification settings - Fork 47
Detect file renames without triggering deletions #587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
db4eaf4
1da84ea
6823e94
99e5e37
3197137
e58ffc6
814beea
26a71c0
019119e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| import fs from "fs/promises" | ||
| import os from "os" | ||
| import path from "path" | ||
| import { beforeEach, describe, expect, it, vi } from "vitest" | ||
| import type { WebSocket } from "ws" | ||
| import { executeEffect } from "./controller.ts" | ||
| import type { Config } from "./types.ts" | ||
| import { hashFileContent } from "./utils/state-persistence.ts" | ||
|
|
||
| const { sendMessage } = vi.hoisted(() => ({ | ||
| sendMessage: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock("./helpers/connection.ts", () => ({ | ||
| initConnection: vi.fn(), | ||
| sendMessage, | ||
| })) | ||
|
|
||
| const mockSocket = {} as WebSocket | ||
|
|
||
| describe("rename confirmation bookkeeping", () => { | ||
| beforeEach(() => { | ||
| sendMessage.mockReset() | ||
| }) | ||
|
|
||
| it("waits for file-synced before deleting old tracking", async () => { | ||
| sendMessage.mockResolvedValue(true) | ||
|
|
||
| const hashTracker = { | ||
| remember: vi.fn(), | ||
| shouldSkip: vi.fn(), | ||
| forget: vi.fn(), | ||
| clear: vi.fn(), | ||
| markDelete: vi.fn(), | ||
| shouldSkipDelete: vi.fn(), | ||
| clearDelete: vi.fn(), | ||
| } | ||
| const fileMetadataCache = { | ||
| recordDelete: vi.fn(), | ||
| } | ||
| const pendingRenames = new Map<string, { oldFileName: string; content: string }>() | ||
|
|
||
| await executeEffect( | ||
| { | ||
| type: "SEND_FILE_RENAME", | ||
| oldFileName: "Old.tsx", | ||
| newFileName: "New.tsx", | ||
| content: "export const New = () => null", | ||
| }, | ||
| { | ||
| config: { | ||
| port: 0, | ||
| projectHash: "project", | ||
| projectDir: null, | ||
| filesDir: null, | ||
| dangerouslyAutoDelete: false, | ||
| allowUnsupportedNpm: false, | ||
| } satisfies Config, | ||
| hashTracker: hashTracker as never, | ||
| installer: null, | ||
| fileMetadataCache: fileMetadataCache as never, | ||
| pendingRenames, | ||
| userActions: {} as never, | ||
| syncState: { | ||
| mode: "watching", | ||
| socket: {} as never, | ||
| pendingRemoteChanges: [], | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| expect(sendMessage).toHaveBeenCalledWith( | ||
| expect.anything(), | ||
| { | ||
| type: "file-rename", | ||
| oldFileName: "Old.tsx", | ||
| newFileName: "New.tsx", | ||
| content: "export const New = () => null", | ||
| } | ||
| ) | ||
| expect(hashTracker.forget).not.toHaveBeenCalled() | ||
| expect(hashTracker.remember).not.toHaveBeenCalled() | ||
| expect(fileMetadataCache.recordDelete).not.toHaveBeenCalled() | ||
| expect(pendingRenames.get("New.tsx")).toEqual({ | ||
| oldFileName: "Old.tsx", | ||
| content: "export const New = () => null", | ||
| }) | ||
| }) | ||
|
|
||
| it("applies old-file cleanup after file-synced arrives", async () => { | ||
| const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "code-link-rename-")) | ||
| const filesDir = path.join(tmpDir, "files") | ||
| await fs.mkdir(filesDir, { recursive: true }) | ||
| await fs.writeFile(path.join(filesDir, "New.tsx"), "export const New = () => null", "utf-8") | ||
|
|
||
| const hashTracker = { | ||
| remember: vi.fn(), | ||
| shouldSkip: vi.fn(), | ||
| forget: vi.fn(), | ||
| clear: vi.fn(), | ||
| markDelete: vi.fn(), | ||
| shouldSkipDelete: vi.fn(), | ||
| clearDelete: vi.fn(), | ||
| } | ||
| const fileMetadataCache = { | ||
| recordSyncedSnapshot: vi.fn(), | ||
| recordDelete: vi.fn(), | ||
| } | ||
| const pendingRenames = new Map<string, { oldFileName: string; content: string }>([ | ||
| ["New.tsx", { oldFileName: "Old.tsx", content: "export const New = () => null" }], | ||
| ]) | ||
|
|
||
| await executeEffect( | ||
| { | ||
| type: "UPDATE_FILE_METADATA", | ||
| fileName: "New.tsx", | ||
| remoteModifiedAt: 1234, | ||
| }, | ||
| { | ||
| config: { | ||
| port: 0, | ||
| projectHash: "project", | ||
| projectDir: tmpDir, | ||
| filesDir, | ||
| dangerouslyAutoDelete: false, | ||
| allowUnsupportedNpm: false, | ||
| } satisfies Config, | ||
| hashTracker: hashTracker as never, | ||
| installer: null, | ||
| fileMetadataCache: fileMetadataCache as never, | ||
| pendingRenames, | ||
| userActions: {} as never, | ||
| syncState: { | ||
| mode: "watching", | ||
| socket: mockSocket, | ||
| pendingRemoteChanges: [], | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| expect(fileMetadataCache.recordSyncedSnapshot).toHaveBeenCalledWith( | ||
| "New.tsx", | ||
| hashFileContent("export const New = () => null"), | ||
| 1234 | ||
| ) | ||
| expect(hashTracker.forget).toHaveBeenCalledWith("Old.tsx") | ||
| expect(fileMetadataCache.recordDelete).toHaveBeenCalledWith("Old.tsx") | ||
| expect(hashTracker.remember).toHaveBeenCalledWith("New.tsx", "export const New = () => null") | ||
| expect(pendingRenames.has("New.tsx")).toBe(false) | ||
|
|
||
| await fs.rm(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,6 +164,12 @@ type Effect = | |
| type: "LOCAL_INITIATED_FILE_DELETE" | ||
| fileNames: string[] | ||
| } | ||
| | { | ||
| type: "SEND_FILE_RENAME" | ||
| oldFileName: string | ||
| newFileName: string | ||
| content: string | ||
| } | ||
| | { type: "PERSIST_STATE" } | ||
| | { | ||
| type: "SYNC_COMPLETE" | ||
|
|
@@ -177,6 +183,11 @@ type Effect = | |
| message: string | ||
| } | ||
|
|
||
| interface PendingRename { | ||
| oldFileName: string | ||
| content: string | ||
| } | ||
|
|
||
| /** Log helper */ | ||
| function log(level: "info" | "debug" | "warn" | "success", message: string): Effect { | ||
| return { type: "LOG", level, message } | ||
|
|
@@ -556,6 +567,23 @@ function transition(state: SyncState, event: SyncEvent): { state: SyncState; eff | |
| }) | ||
| break | ||
| } | ||
|
|
||
| case "rename": { | ||
| if (content === undefined || !event.event.oldRelativePath) { | ||
| effects.push(log("warn", `Rename event missing data: ${relativePath}`)) | ||
| return { state, effects } | ||
| } | ||
| effects.push( | ||
| log("debug", `Local rename detected: ${event.event.oldRelativePath} → ${relativePath}`), | ||
| { | ||
| type: "SEND_FILE_RENAME", | ||
| oldFileName: event.event.oldRelativePath, | ||
| newFileName: relativePath, | ||
| content, | ||
| } | ||
| ) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return { state, effects } | ||
|
|
@@ -675,11 +703,12 @@ async function executeEffect( | |
| hashTracker: ReturnType<typeof createHashTracker> | ||
| installer: Installer | null | ||
| fileMetadataCache: FileMetadataCache | ||
| pendingRenames: Map<string, PendingRename> | ||
| userActions: PluginUserPromptCoordinator | ||
| syncState: SyncState | ||
| } | ||
| ): Promise<SyncEvent[]> { | ||
| const { config, hashTracker, installer, fileMetadataCache, userActions, syncState } = context | ||
| const { config, hashTracker, installer, fileMetadataCache, pendingRenames, userActions, syncState } = context | ||
|
|
||
| switch (effect.type) { | ||
| case "INIT_WORKSPACE": { | ||
|
|
@@ -852,12 +881,21 @@ async function executeEffect( | |
|
|
||
| // Read current file content to compute hash | ||
| const currentContent = await readFileSafe(effect.fileName, config.filesDir) | ||
| const pendingRename = pendingRenames.get(effect.fileName) | ||
| const syncedContent = currentContent ?? pendingRename?.content ?? null | ||
|
|
||
| if (currentContent !== null) { | ||
| const contentHash = hashFileContent(currentContent) | ||
| if (syncedContent !== null) { | ||
| const contentHash = hashFileContent(syncedContent) | ||
| fileMetadataCache.recordSyncedSnapshot(effect.fileName, contentHash, effect.remoteModifiedAt) | ||
| } | ||
|
|
||
| if (pendingRename) { | ||
| hashTracker.forget(pendingRename.oldFileName) | ||
| fileMetadataCache.recordDelete(pendingRename.oldFileName) | ||
| hashTracker.remember(effect.fileName, pendingRename.content) | ||
| pendingRenames.delete(effect.fileName) | ||
| } | ||
|
|
||
| return [] | ||
| } | ||
|
|
||
|
|
@@ -903,6 +941,35 @@ async function executeEffect( | |
| return [] | ||
| } | ||
|
|
||
| case "SEND_FILE_RENAME": { | ||
| try { | ||
| if (!syncState.socket) { | ||
| warn(`No socket available to send rename ${effect.oldFileName} -> ${effect.newFileName}`) | ||
| return [] | ||
| } | ||
|
|
||
| const sent = await sendMessage(syncState.socket, { | ||
| type: "file-rename", | ||
| oldFileName: effect.oldFileName, | ||
| newFileName: effect.newFileName, | ||
| content: effect.content, | ||
| }) | ||
| if (!sent) { | ||
| warn(`Failed to send rename ${effect.oldFileName} -> ${effect.newFileName}`) | ||
| return [] | ||
| } | ||
|
|
||
| pendingRenames.set(effect.newFileName, { | ||
| oldFileName: effect.oldFileName, | ||
| content: effect.content, | ||
| }) | ||
| } catch (err) { | ||
| warn(`Failed to send rename ${effect.oldFileName} -> ${effect.newFileName}`) | ||
| } | ||
|
|
||
| return [] | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename handler lacks echo prevention checksMedium Severity The new Additional Locations (1) |
||
|
|
||
| case "LOCAL_INITIATED_FILE_DELETE": { | ||
| // Echo prevention: filter out remote-initiated deletes | ||
| const filesToDelete = effect.fileNames.filter(fileName => { | ||
|
|
@@ -1013,6 +1080,7 @@ export async function start(config: Config): Promise<void> { | |
|
|
||
| const hashTracker = createHashTracker() | ||
| const fileMetadataCache = new FileMetadataCache() | ||
| const pendingRenames = new Map<string, PendingRename>() | ||
| let installer: Installer | null = null | ||
|
|
||
| // State machine state | ||
|
|
@@ -1052,6 +1120,7 @@ export async function start(config: Config): Promise<void> { | |
| hashTracker, | ||
| installer, | ||
| fileMetadataCache, | ||
| pendingRenames, | ||
| userActions, | ||
| syncState, | ||
| }) | ||
|
|
@@ -1208,6 +1277,13 @@ export async function start(config: Config): Promise<void> { | |
| } | ||
| break | ||
|
|
||
| case "error": | ||
| if (message.fileName) { | ||
| pendingRenames.delete(message.fileName) | ||
| } | ||
| warn(message.message) | ||
| return | ||
|
|
||
| case "conflicts-resolved": | ||
| event = { | ||
| type: "CONFLICTS_RESOLVED", | ||
|
|
@@ -1287,4 +1363,4 @@ export async function start(config: Config): Promise<void> { | |
| } | ||
|
|
||
| // Export for testing | ||
| export { transition } | ||
| export { executeEffect, transition } | ||


Uh oh!
There was an error while loading. Please reload this page.