From af83d270fd7c34592f8f20e48b0f2ce703f47662 Mon Sep 17 00:00:00 2001 From: lucaronin Date: Wed, 27 May 2026 17:37:16 +0200 Subject: [PATCH] fix: coalesce overlapping editor saves --- src/hooks/useEditorSave.test.ts | 51 ++++++- src/hooks/useEditorSave.ts | 124 +++++++++++++-- tests/smoke/save-before-note-switch.spec.ts | 161 ++++++++++++++++++++ 3 files changed, 319 insertions(+), 17 deletions(-) diff --git a/src/hooks/useEditorSave.test.ts b/src/hooks/useEditorSave.test.ts index 435005d5..e78705a4 100644 --- a/src/hooks/useEditorSave.test.ts +++ b/src/hooks/useEditorSave.test.ts @@ -23,7 +23,8 @@ describe('useEditorSave', () => { updateVaultContent = vi.fn() setTabs = vi.fn() setToastMessage = vi.fn() - mockInvokeFn.mockClear() + mockInvokeFn.mockReset() + mockInvokeFn.mockResolvedValue(null) }) function renderSaveHook() { @@ -166,6 +167,32 @@ describe('useEditorSave', () => { }) }) + it('coalesces overlapping savePendingForPath calls for the same buffered content', async () => { + let resolveSave!: (value: null) => void + mockInvokeFn.mockReturnValue(new Promise((resolve) => { resolveSave = resolve })) + const { result } = renderSaveHook() + + act(() => { + result.current.handleContentChange('/test/note-a.md', 'content A') + }) + + let firstSave!: Promise + let secondSave!: Promise + await act(async () => { + firstSave = result.current.savePendingForPath('/test/note-a.md') + secondSave = result.current.savePendingForPath('/test/note-a.md') + await Promise.resolve() + await Promise.resolve() + }) + + expect(mockInvokeFn).toHaveBeenCalledTimes(1) + + await act(async () => { + resolveSave(null) + await expect(Promise.all([firstSave, secondSave])).resolves.toEqual([true, true]) + }) + }) + it('calls onAfterSave callback after successful save', async () => { const cb = vi.fn() const { result } = renderHook(() => @@ -246,6 +273,28 @@ describe('useEditorSave', () => { expect(updated[0].content).toBe('---\ntitle: T\n---\n\n# T\n\nLive edits') }) + it('does not replace current tab state again when saving content already synced from editor changes', async () => { + const { result } = renderSaveHook() + const path = '/test/note.md' + const content = '---\ntitle: T\n---\n\n# T\n\nLive edits' + + act(() => { + result.current.handleContentChange(path, content) + }) + + const liveUpdater = setTabs.mock.calls.at(-1)?.[0] + const initialTabs = [{ entry: { path }, content: 'stale' }] + const currentTabs = liveUpdater(initialTabs) + + await act(async () => { + await result.current.savePendingForPath(path) + }) + + const saveUpdater = setTabs.mock.calls.at(-1)?.[0] + expect(saveUpdater(currentTabs)).toBe(currentTabs) + expect(updateVaultContent).toHaveBeenCalledWith(path, content) + }) + it('save updates tab content with edited body, not original (regression)', async () => { const { result } = renderSaveHook() diff --git a/src/hooks/useEditorSave.ts b/src/hooks/useEditorSave.ts index 3224c517..a6459ce3 100644 --- a/src/hooks/useEditorSave.ts +++ b/src/hooks/useEditorSave.ts @@ -46,6 +46,29 @@ interface PendingContent { content: string } +interface InFlightPendingSave { + pending: PendingContent + promise: Promise +} + +interface PersistPendingContentParams { + pending: PendingContent + pendingContentRef: MutableRefObject + saveNote: (path: string, content: string) => Promise + onBeforePersist?: EditorSaveConfig['onBeforePersist'] + onNotePersisted?: EditorSaveConfig['onNotePersisted'] + resolvePath?: EditorSaveConfig['resolvePath'] + resolvePathBeforeSave?: EditorSaveConfig['resolvePathBeforeSave'] + persistenceScopeRef: MutableRefObject +} + +interface ReusableInFlightSaveParams { + inFlightSave: InFlightPendingSave | null + pending: PendingContent + pathFilter?: string + resolvePath?: EditorSaveConfig['resolvePath'] +} + interface EditorSaveCommandsParams { pendingContentRef: MutableRefObject autoSaveTimerRef: MutableRefObject | null> @@ -123,6 +146,26 @@ function matchesPendingContent( return matchesPendingPath(pending, path, resolvePath) && pending.content === content } +function matchesPendingSnapshot( + pending: PendingContent, + snapshot: PendingContent, + resolvePath?: EditorSaveConfig['resolvePath'], +): boolean { + return matchesPendingContent(pending, snapshot.path, snapshot.content, resolvePath) +} + +function reusableInFlightSave({ + inFlightSave, + pending, + pathFilter, + resolvePath, +}: ReusableInFlightSaveParams): Promise | null { + if (!inFlightSave) return null + if (!matchesPendingPath(inFlightSave.pending, pathFilter, resolvePath)) return null + if (!matchesPendingSnapshot(inFlightSave.pending, pending, resolvePath)) return null + return inFlightSave.promise +} + async function persistResolvedContent({ path, content, @@ -152,9 +195,48 @@ function applyTabContent( path: string, content: string, ): void { - setTabs((prev: Tab[]) => - prev.map((t) => t.entry.path === path ? { ...t, content } : t) - ) + setTabs((prev: Tab[]) => { + let changed = false + const next = prev.map((t) => { + if (t.entry.path !== path) return t + if (t.content === content) return t + changed = true + return { ...t, content } + }) + return changed ? next : prev + }) +} + +async function persistPendingContent({ + pending, + pendingContentRef, + saveNote, + onBeforePersist, + onNotePersisted, + resolvePath, + resolvePathBeforeSave, + persistenceScopeRef, +}: PersistPendingContentParams): Promise { + const { path, content } = pending + const targetPath = await persistResolvedContent({ + path, + content, + saveNote, + onBeforePersist, + resolvePath, + resolvePathBeforeSave, + persistenceScopeRef, + }) + if (targetPath === null) { + if (pendingContentRef.current === pending) pendingContentRef.current = null + return false + } + if (!matchesPendingContent(pendingContentRef.current, targetPath, content, resolvePath)) { + return false + } + pendingContentRef.current = null + onNotePersisted?.(targetPath, content) + return true } function scheduleAutoSave({ @@ -207,30 +289,40 @@ function usePendingContentFlush({ canPersistRef: MutableRefObject persistenceScopeRef: MutableRefObject }) { + const inFlightSaveRef = useRef(null) + return useCallback(async (pathFilter?: string): Promise => { const pending = pendingContentRef.current if (!matchesPendingPath(pending, pathFilter, resolvePath)) return false if (!canPersistRef.current) return false - const { path, content } = pending - const targetPath = await persistResolvedContent({ - path, - content, + + const inFlightSave = reusableInFlightSave({ + inFlightSave: inFlightSaveRef.current, + pending, + pathFilter, + resolvePath, + }) + if (inFlightSave) return inFlightSave + + const promise = persistPendingContent({ + pending, + pendingContentRef, saveNote, onBeforePersist, + onNotePersisted, resolvePath, resolvePathBeforeSave, persistenceScopeRef, }) - if (targetPath === null) { - if (pendingContentRef.current === pending) pendingContentRef.current = null - return false + inFlightSaveRef.current = { pending, promise } + + try { + return await promise + } finally { + if (inFlightSaveRef.current?.promise === promise) { + inFlightSaveRef.current = null + } } - if (!matchesPendingContent(pendingContentRef.current, targetPath, content, resolvePath)) { - return false - } - pendingContentRef.current = null - onNotePersisted?.(targetPath, content) - return true }, [canPersistRef, onBeforePersist, onNotePersisted, pendingContentRef, persistenceScopeRef, resolvePath, resolvePathBeforeSave, saveNote]) } diff --git a/tests/smoke/save-before-note-switch.spec.ts b/tests/smoke/save-before-note-switch.spec.ts index a14c72cc..e1e5d625 100644 --- a/tests/smoke/save-before-note-switch.spec.ts +++ b/tests/smoke/save-before-note-switch.spec.ts @@ -7,8 +7,38 @@ import { removeFixtureVaultCopy, } from '../helpers/fixtureVault' import { executeCommand, openCommandPalette } from './helpers' +import { triggerMenuCommand } from './testBridge' let tempVaultDir: string +type SaveBeforeSwitchProbeWindow = Window & typeof globalThis & { + __mockHandlers?: Record) => unknown> + __saveBeforeSwitchProbe?: { + calls: Array<{ path: string; content: string }> + release: () => void + } + __saveBeforeSwitchProbeGate?: Promise +} + +function isReactUpdateLoop(message: string): boolean { + return ( + message.includes('Maximum update depth') || + message.includes('React error #185') || + message.includes('#185') + ) +} + +function collectReactUpdateLoopErrors(page: Page): string[] { + const errors: string[] = [] + page.on('pageerror', (error) => { + if (isReactUpdateLoop(error.message)) errors.push(error.message) + }) + page.on('console', (message) => { + if (message.type() === 'error' && isReactUpdateLoop(message.text())) { + errors.push(message.text()) + } + }) + return errors +} async function openNote(page: Page, title: string) { const noteList = page.locator('[data-testid="note-list-container"]') @@ -56,6 +86,106 @@ async function setRawEditorContent(page: Page, content: string) { }, content) } +async function placeCaretAtEndOfBlock(page: Page, blockIndex: number): Promise { + const block = page.locator('.bn-block-content').nth(blockIndex) + await expect(block).toBeVisible({ timeout: 5_000 }) + + const placed = await block.evaluate((element) => { + const editable = element.closest('[contenteditable="true"]') + if (editable instanceof HTMLElement) editable.focus() + + const walker = document.createTreeWalker(element, NodeFilter.SHOW_TEXT) + let lastTextNode: Text | null = null + while (walker.nextNode()) { + if (walker.currentNode.textContent) lastTextNode = walker.currentNode as Text + } + if (!lastTextNode) return false + + const range = document.createRange() + range.setStart(lastTextNode, lastTextNode.textContent?.length ?? 0) + range.collapse(true) + const selection = window.getSelection() + selection?.removeAllRanges() + selection?.addRange(range) + document.dispatchEvent(new Event('selectionchange')) + return true + }) + + expect(placed).toBe(true) +} + +function initializeDelayedSaveProbeState(): void { + const probeWindow = window as SaveBeforeSwitchProbeWindow + let releaseSave: (() => void) | null = null + probeWindow.__saveBeforeSwitchProbeGate = new Promise((resolve) => { releaseSave = resolve }) + probeWindow.__saveBeforeSwitchProbe = { + calls: [], + release: () => { releaseSave?.() }, + } +} + +function installDelayedSaveProbeInterceptor(): void { + const probeWindow = window as SaveBeforeSwitchProbeWindow + + const readStringArg = (args: Record | undefined, key: string) => { + const value = args?.[key] + return typeof value === 'string' ? value : '' + } + + const patchHandlers = (handlers?: Record) => unknown> | null) => { + if (!handlers) return null + if (Reflect.get(handlers, '__saveBeforeSwitchProbePatched') === true) return handlers + + const originalSave = handlers.save_note_content + if (!originalSave) throw new Error('save_note_content handler is unavailable') + + handlers.save_note_content = async (args?: Record) => { + probeWindow.__saveBeforeSwitchProbe?.calls.push({ + path: readStringArg(args, 'path'), + content: readStringArg(args, 'content'), + }) + await probeWindow.__saveBeforeSwitchProbeGate + return originalSave(args) + } + Object.defineProperty(handlers, '__saveBeforeSwitchProbePatched', { + configurable: true, + enumerable: false, + value: true, + }) + return handlers + } + + let ref = patchHandlers(probeWindow.__mockHandlers) + Object.defineProperty(probeWindow, '__mockHandlers', { + configurable: true, + get() { + return patchHandlers(ref) ?? ref + }, + set(value) { + ref = patchHandlers(value) + }, + }) +} + +async function installDelayedSaveProbe(page: Page): Promise { + await page.evaluate(initializeDelayedSaveProbeState) + await page.evaluate(installDelayedSaveProbeInterceptor) +} + +async function releaseDelayedSave(page: Page): Promise { + await page.evaluate(() => { + const probeWindow = window as SaveBeforeSwitchProbeWindow + probeWindow.__saveBeforeSwitchProbe?.release() + }) +} + +async function delayedSaveCount(page: Page): Promise { + return page.evaluate(() => { + const probeWindow = window as SaveBeforeSwitchProbeWindow + return probeWindow.__saveBeforeSwitchProbe?.calls.length ?? 0 + }) +} + test.beforeEach(async ({ page }, testInfo) => { testInfo.setTimeout(60_000) await page.goto('/') @@ -89,6 +219,37 @@ test('@smoke switching notes persists unsaved raw edits without waiting for the ).toContain(appendedText) }) +test('@smoke switching notes during a slow rich-editor save writes once and opens the latest note', async ({ page }) => { + const errors = collectReactUpdateLoopErrors(page) + const noteBPath = path.join(tempVaultDir, 'note', 'note-b.md') + const appendedText = `Slow rich save before switch ${Date.now()}` + const noteList = page.locator('[data-testid="note-list-container"]') + + await installDelayedSaveProbe(page) + await openNote(page, 'Note B') + await expect(page.locator('.bn-editor h1').first()).toHaveText('Note B', { timeout: 5_000 }) + + await placeCaretAtEndOfBlock(page, 1) + await page.keyboard.type(` ${appendedText}`, { delay: 10 }) + await expect(page.locator('.bn-block-content').filter({ hasText: appendedText })).toBeVisible({ timeout: 5_000 }) + await page.waitForTimeout(100) + await triggerMenuCommand(page, 'file-save') + await expect.poll(() => delayedSaveCount(page), { timeout: 5_000 }).toBe(1) + + await noteList.getByText('Alpha Project', { exact: true }).click() + await noteList.getByText('Note C', { exact: true }).click() + + await expect.poll(() => delayedSaveCount(page), { timeout: 5_000 }).toBe(1) + await releaseDelayedSave(page) + + await expect(page.getByTestId('breadcrumb-filename-trigger')).toContainText('note-c', { timeout: 10_000 }) + await expect.poll( + () => fs.readFileSync(noteBPath, 'utf8'), + { timeout: 10_000 }, + ).toContain(appendedText) + expect(errors).toEqual([]) +}) + test('@smoke deleting a property after switching notes keeps the current note editable', async ({ page }) => { const alphaPath = path.join(tempVaultDir, 'project', 'alpha-project.md')