fix: coalesce overlapping editor saves

2026-05-27 17:37:16 +02:00
parent 24f4ee3272
commit af83d270fd
3 changed files with 319 additions and 17 deletions
--- 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<null>((resolve) => { resolveSave = resolve }))
+    const { result } = renderSaveHook()
+
+    act(() => {
+      result.current.handleContentChange('/test/note-a.md', 'content A')
+    })
+
+    let firstSave!: Promise<boolean>
+    let secondSave!: Promise<boolean>
+    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()

--- a/src/hooks/useEditorSave.ts
+++ b/src/hooks/useEditorSave.ts
@@ -46,6 +46,29 @@ interface PendingContent {
  content: string
 }

+interface InFlightPendingSave {
+  pending: PendingContent
+  promise: Promise<boolean>
+}
+
+interface PersistPendingContentParams {
+  pending: PendingContent
+  pendingContentRef: MutableRefObject<PendingContent | null>
+  saveNote: (path: string, content: string) => Promise<void>
+  onBeforePersist?: EditorSaveConfig['onBeforePersist']
+  onNotePersisted?: EditorSaveConfig['onNotePersisted']
+  resolvePath?: EditorSaveConfig['resolvePath']
+  resolvePathBeforeSave?: EditorSaveConfig['resolvePathBeforeSave']
+  persistenceScopeRef: MutableRefObject<string | readonly string[] | undefined>
+}
+
+interface ReusableInFlightSaveParams {
+  inFlightSave: InFlightPendingSave | null
+  pending: PendingContent
+  pathFilter?: string
+  resolvePath?: EditorSaveConfig['resolvePath']
+}
+
 interface EditorSaveCommandsParams {
  pendingContentRef: MutableRefObject<PendingContent | null>
  autoSaveTimerRef: MutableRefObject<ReturnType<typeof setTimeout> | 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<boolean> | 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<boolean> {
+  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<boolean>
  persistenceScopeRef: MutableRefObject<string | readonly string[] | undefined>
 }) {
+  const inFlightSaveRef = useRef<InFlightPendingSave | null>(null)
+
  return useCallback(async (pathFilter?: string): Promise<boolean> => {
    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])
 }

--- 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<string, (args?: Record<string, unknown>) => unknown>
+  __saveBeforeSwitchProbe?: {
+    calls: Array<{ path: string; content: string }>
+    release: () => void
+  }
+  __saveBeforeSwitchProbeGate?: Promise<void>
+}
+
+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<void> {
+  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<void>((resolve) => { releaseSave = resolve })
+  probeWindow.__saveBeforeSwitchProbe = {
+    calls: [],
+    release: () => { releaseSave?.() },
+  }
+}
+
+function installDelayedSaveProbeInterceptor(): void {
+  const probeWindow = window as SaveBeforeSwitchProbeWindow
+
+  const readStringArg = (args: Record<string, unknown> | undefined, key: string) => {
+    const value = args?.[key]
+    return typeof value === 'string' ? value : ''
+  }
+
+  const patchHandlers = (handlers?: Record<string, (args?: Record<string, unknown>) => 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<string, unknown>) => {
+      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<void> {
+  await page.evaluate(initializeDelayedSaveProbeState)
+  await page.evaluate(installDelayedSaveProbeInterceptor)
+}
+
+async function releaseDelayedSave(page: Page): Promise<void> {
+  await page.evaluate(() => {
+    const probeWindow = window as SaveBeforeSwitchProbeWindow
+    probeWindow.__saveBeforeSwitchProbe?.release()
+  })
+}
+
+async function delayedSaveCount(page: Page): Promise<number> {
+  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')