From 48714de953c49fa78064de217e8f7a2b7ad59ed9 Mon Sep 17 00:00:00 2001 From: Steve Woodrow Date: Thu, 5 Mar 2026 22:53:19 -0800 Subject: [PATCH] fix: avoid false delete events during atomic saves --- src/main/services/FileWatcherService.ts | 40 ++++++- src/shared/constants/index.ts | 6 + .../main/services/FileWatcherService.test.ts | 112 ++++++++++++++++-- 3 files changed, 143 insertions(+), 15 deletions(-) diff --git a/src/main/services/FileWatcherService.ts b/src/main/services/FileWatcherService.ts index fd675ae..8d94743 100644 --- a/src/main/services/FileWatcherService.ts +++ b/src/main/services/FileWatcherService.ts @@ -4,7 +4,7 @@ */ import { readFile, stat } from 'node:fs/promises'; -import { FILE_WATCH_DEBOUNCE_MS } from '@shared/constants'; +import { FILE_DELETE_CONFIRM_MS, FILE_WATCH_DEBOUNCE_MS } from '@shared/constants'; import { FileWatchError } from '@shared/errors'; import { watch } from 'chokidar'; @@ -27,6 +27,7 @@ interface WindowCallbacks { export class FileWatcherService { private watchedFiles: Map = new Map(); private windowCallbacks: Map = new Map(); + private pendingDeleteTimers: Map> = new Map(); /** * Start watching a file for a specific window. @@ -56,11 +57,17 @@ export class FileWatcherService { }; watcher.on('change', (changedPath: string) => { + this.cancelPendingDelete(changedPath); void this.processFileChange(changedPath); }); + watcher.on('add', (addedPath: string) => { + this.cancelPendingDelete(addedPath); + void this.processFileChange(addedPath); + }); + watcher.on('unlink', (deletedPath: string) => { - this.handleFileDelete(deletedPath); + this.scheduleFileDeleteConfirmation(deletedPath); }); watcher.on('error', (error: unknown) => { @@ -166,6 +173,7 @@ export class FileWatcherService { } private async closeWatcherEntry(filePath: string, entry: WatchedFileEntry): Promise { + this.cancelPendingDelete(filePath); await entry.watcher.close(); this.watchedFiles.delete(filePath); } @@ -230,6 +238,34 @@ export class FileWatcherService { // Remove the entry and close the watcher (fire-and-forget since file is already gone) this.watchedFiles.delete(filePath); void entry.watcher.close(); + this.cancelPendingDelete(filePath); + } + + private scheduleFileDeleteConfirmation(filePath: string): void { + this.cancelPendingDelete(filePath); + + const timer = setTimeout(() => { + void this.confirmAndHandleFileDelete(filePath); + }, FILE_DELETE_CONFIRM_MS); + + this.pendingDeleteTimers.set(filePath, timer); + } + + private cancelPendingDelete(filePath: string): void { + const timer = this.pendingDeleteTimers.get(filePath); + if (!timer) return; + + clearTimeout(timer); + this.pendingDeleteTimers.delete(filePath); + } + + private async confirmAndHandleFileDelete(filePath: string): Promise { + this.pendingDeleteTimers.delete(filePath); + + const stats = await this.getFileStats(filePath); + if (stats) return; + + this.handleFileDelete(filePath); } private async getFileStats(filePath: string): Promise { diff --git a/src/shared/constants/index.ts b/src/shared/constants/index.ts index 5984412..92d4052 100644 --- a/src/shared/constants/index.ts +++ b/src/shared/constants/index.ts @@ -8,6 +8,12 @@ export const MARKDOWN_EXTENSIONS = ['.md', '.markdown', '.mdown', '.mkdn', '.mkd */ export const FILE_WATCH_DEBOUNCE_MS = 300; +/** + * Delay before confirming file deletion after unlink (ms). + * Prevents false deletes from atomic-save workflows. + */ +export const FILE_DELETE_CONFIRM_MS = 750; + /** * Maximum file size to render (bytes) - 5MB */ diff --git a/tests/unit/main/services/FileWatcherService.test.ts b/tests/unit/main/services/FileWatcherService.test.ts index 712205f..ff1309d 100644 --- a/tests/unit/main/services/FileWatcherService.test.ts +++ b/tests/unit/main/services/FileWatcherService.test.ts @@ -10,6 +10,7 @@ import { getFileWatcherService, } from '@main/services/FileWatcherService'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { FILE_DELETE_CONFIRM_MS } from '@shared/constants'; interface MockWatcher { on: ReturnType; @@ -41,6 +42,17 @@ function getWatcher(index: number): MockWatcher { return watcher; } +function getWatcherEventHandler(index: number, eventName: string): (filePath: string) => void { + const call = getWatcher(index).on.mock.calls.find( + (entry: unknown[]) => entry[0] === eventName + ); + const handler = call?.[1]; + if (typeof handler !== 'function') { + throw new Error(`No ${eventName} handler found for watcher ${index}`); + } + return handler as (filePath: string) => void; +} + vi.mock('chokidar', () => ({ watch: vi.fn(() => { const watcher = createMockWatcher(); @@ -66,6 +78,7 @@ describe('FileWatcherService', () => { }); afterEach(async () => { + vi.useRealTimers(); await service.destroy(); try { await fs.rm(tempDir, { recursive: true, force: true }); @@ -94,6 +107,12 @@ describe('FileWatcherService', () => { expect(getWatcher(0).on).toHaveBeenCalledWith('change', expect.any(Function)); }); + it('should register add event handler', async () => { + await service.watch(testFile, 1); + + expect(getWatcher(0).on).toHaveBeenCalledWith('add', expect.any(Function)); + }); + it('should register unlink event handler', async () => { await service.watch(testFile, 1); @@ -232,13 +251,18 @@ describe('FileWatcherService', () => { service.onFileDelete(1, callback); await service.watch(testFile, 1); + vi.useFakeTimers(); - const unlinkCall = getWatcher(0).on.mock.calls.find( - (call: unknown[]) => call[0] === 'unlink' - ); - const unlinkHandler = unlinkCall?.[1] as (path: string) => void; + const unlinkHandler = getWatcherEventHandler(0, 'unlink'); + await fs.rm(testFile); unlinkHandler(testFile); + await vi.advanceTimersByTimeAsync(FILE_DELETE_CONFIRM_MS + 1); + await vi.waitFor(() => { + expect(callback).toHaveBeenCalledTimes(1); + }); + + vi.useRealTimers(); expect(callback).toHaveBeenCalledWith( expect.objectContaining({ filePath: testFile }) @@ -254,13 +278,18 @@ describe('FileWatcherService', () => { service.onFileDelete(1, errorCallback); await service.watch(testFile, 1); + vi.useFakeTimers(); - const unlinkCall = getWatcher(0).on.mock.calls.find( - (call: unknown[]) => call[0] === 'unlink' - ); - const unlinkHandler = unlinkCall?.[1] as (path: string) => void; + const unlinkHandler = getWatcherEventHandler(0, 'unlink'); + await fs.rm(testFile); expect(() => unlinkHandler(testFile)).not.toThrow(); + await vi.advanceTimersByTimeAsync(FILE_DELETE_CONFIRM_MS + 1); + await vi.waitFor(() => { + expect(errorCallback).toHaveBeenCalledTimes(1); + }); + vi.useRealTimers(); + expect(consoleSpy).toHaveBeenCalledWith( 'Error in file delete callback:', expect.any(Error) @@ -271,15 +300,65 @@ describe('FileWatcherService', () => { it('should remove window from watcher after deletion', async () => { await service.watch(testFile, 1); + vi.useFakeTimers(); - const unlinkCall = getWatcher(0).on.mock.calls.find( - (call: unknown[]) => call[0] === 'unlink' - ); - const unlinkHandler = unlinkCall?.[1] as (path: string) => void; + const unlinkHandler = getWatcherEventHandler(0, 'unlink'); + await fs.rm(testFile); unlinkHandler(testFile); + await vi.advanceTimersByTimeAsync(FILE_DELETE_CONFIRM_MS + 1); + await vi.waitFor(() => { + expect(service.isWatchingFile(testFile)).toBe(false); + }); + vi.useRealTimers(); + }); - expect(service.isWatchingFile(testFile)).toBe(false); + it('should not emit delete for transient unlink followed by add', async () => { + const deleteCallback = vi.fn(); + const changeCallback = vi.fn(); + service.onFileDelete(1, deleteCallback); + service.onFileChange(1, changeCallback); + + await service.watch(testFile, 1); + vi.useFakeTimers(); + + const unlinkHandler = getWatcherEventHandler(0, 'unlink'); + const addHandler = getWatcherEventHandler(0, 'add'); + + unlinkHandler(testFile); + addHandler(testFile); + await vi.advanceTimersByTimeAsync(FILE_DELETE_CONFIRM_MS + 1); + await vi.waitFor(() => { + expect(changeCallback).toHaveBeenCalledTimes(1); + }); + vi.useRealTimers(); + + expect(deleteCallback).not.toHaveBeenCalled(); + expect(service.isWatchingFile(testFile)).toBe(true); + }); + + it('should not emit delete for transient unlink followed by change', async () => { + const deleteCallback = vi.fn(); + const changeCallback = vi.fn(); + service.onFileDelete(1, deleteCallback); + service.onFileChange(1, changeCallback); + + await service.watch(testFile, 1); + vi.useFakeTimers(); + + const unlinkHandler = getWatcherEventHandler(0, 'unlink'); + const changeHandler = getWatcherEventHandler(0, 'change'); + + unlinkHandler(testFile); + changeHandler(testFile); + await vi.advanceTimersByTimeAsync(FILE_DELETE_CONFIRM_MS + 1); + await vi.waitFor(() => { + expect(changeCallback).toHaveBeenCalledTimes(1); + }); + vi.useRealTimers(); + + expect(deleteCallback).not.toHaveBeenCalled(); + expect(service.isWatchingFile(testFile)).toBe(true); }); }); @@ -372,6 +451,7 @@ describe('FileWatcherService', () => { await service.watch(testFile, 1); await service.watch(secondFile, 2); + vi.useFakeTimers(); // Trigger delete on secondFile (watched by window 2) const unlinkCall = getWatcher(1).on.mock.calls.find( @@ -379,7 +459,13 @@ describe('FileWatcherService', () => { ); const unlinkHandler = unlinkCall?.[1] as (path: string) => void; + await fs.rm(secondFile); unlinkHandler(secondFile); + await vi.advanceTimersByTimeAsync(FILE_DELETE_CONFIRM_MS + 1); + await vi.waitFor(() => { + expect(callback2).toHaveBeenCalledTimes(1); + }); + vi.useRealTimers(); expect(callback1).not.toHaveBeenCalled(); expect(callback2).toHaveBeenCalledWith(