From 2f7689599b4b7b086d73f098a296290c5f679cf7 Mon Sep 17 00:00:00 2001 From: Luca Rossi Date: Wed, 25 Feb 2026 16:11:59 +0100 Subject: [PATCH] feat: add optimistic error recovery for note creation (#69) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Note creation was already optimistic (UI updates before disk write) but errors were silently swallowed. Now if the disk write fails, the optimistic entry is reverted (tab closed, entry removed) and the user sees an error toast. Each note creation is independent, so one failure in rapid Cmd+N presses doesn't affect the others. - Add removeEntry to useVaultLoader for reverting optimistic adds - Refactor persistNewNote to return a Promise for error handling - Extract navigateWikilink, persistOptimistic, createAndPersist, and runFrontmatterAndApply helpers to reduce hook complexity - Add tests for error recovery: single note, type, success path, and rapid creation with partial failure - Code health: 8.93→9.01 (CC fixed: 10→7, primitive obsession improved) Co-authored-by: Claude Opus 4.6 --- src/App.tsx | 2 +- src/hooks/useNoteActions.test.ts | 65 ++++++++++++++++- src/hooks/useNoteActions.ts | 115 +++++++++++++++++++------------ src/hooks/useVaultLoader.test.ts | 102 +++++++++++---------------- src/hooks/useVaultLoader.ts | 7 +- 5 files changed, 179 insertions(+), 112 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index cc6b0869..15fcae2b 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -67,7 +67,7 @@ function App() { useEffect(() => { setApiKey(settings.anthropic_key ?? '') }, [settings.anthropic_key]) - const notes = useNoteActions({ addEntry: vault.addEntry, updateContent: vault.updateContent, entries: vault.entries, setToastMessage, updateEntry: vault.updateEntry }) + const notes = useNoteActions({ addEntry: vault.addEntry, removeEntry: vault.removeEntry, updateContent: vault.updateContent, entries: vault.entries, setToastMessage, updateEntry: vault.updateEntry }) const { handleSave, handleContentChange, savePendingForPath, savePending } = useEditorSave({ updateVaultContent: vault.updateContent, diff --git a/src/hooks/useNoteActions.test.ts b/src/hooks/useNoteActions.test.ts index 0a83e39d..c6ce27b5 100644 --- a/src/hooks/useNoteActions.test.ts +++ b/src/hooks/useNoteActions.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { renderHook, act } from '@testing-library/react' +import { invoke } from '@tauri-apps/api/core' +import { isTauri } from '../mock-tauri' import type { VaultEntry } from '../types' import { slugify, @@ -17,7 +19,7 @@ import type { NoteActionsConfig } from './useNoteActions' // Mock dependencies vi.mock('@tauri-apps/api/core', () => ({ invoke: vi.fn() })) vi.mock('../mock-tauri', () => ({ - isTauri: () => false, + isTauri: vi.fn(() => false), addMockEntry: vi.fn(), updateMockContent: vi.fn(), mockInvoke: vi.fn().mockResolvedValue(''), @@ -280,16 +282,18 @@ describe('frontmatterToEntryPatch', () => { describe('useNoteActions hook', () => { const addEntry = vi.fn() + const removeEntry = vi.fn() const updateContent = vi.fn() const updateEntry = vi.fn() const setToastMessage = vi.fn() const makeConfig = (entries: VaultEntry[] = []): NoteActionsConfig => ({ - addEntry, updateContent, entries, setToastMessage, updateEntry, + addEntry, removeEntry, updateContent, entries, setToastMessage, updateEntry, }) beforeEach(() => { vi.clearAllMocks() + vi.mocked(isTauri).mockReturnValue(false) }) it('handleCreateNote calls addEntry and creates correct entry', () => { @@ -436,4 +440,61 @@ describe('useNoteActions hook', () => { expect(updateEntry).not.toHaveBeenCalled() expect(setToastMessage).toHaveBeenCalledWith('Property updated') }) + + describe('optimistic error recovery (Tauri mode)', () => { + beforeEach(() => { + vi.mocked(isTauri).mockReturnValue(true) + }) + + it.each([ + ['handleCreateNote', 'Failing Note', 'Note', 'note/failing-note.md'], + ['handleCreateType', 'Recipe', 'Type', 'type/recipe.md'], + ])('reverts optimistic creation via %s when disk write fails', async (method, title, type, pathFragment) => { + vi.mocked(invoke).mockRejectedValueOnce(new Error('disk full')) + const { result } = renderHook(() => useNoteActions(makeConfig())) + + await act(async () => { + if (method === 'handleCreateNote') result.current.handleCreateNote(title, type) + else result.current.handleCreateType(title) + await new Promise((r) => setTimeout(r, 0)) + }) + + expect(addEntry).toHaveBeenCalledTimes(1) + expect(removeEntry).toHaveBeenCalledWith(expect.stringContaining(pathFragment)) + expect(setToastMessage).toHaveBeenCalledWith('Failed to create note — disk write error') + }) + + it('does not revert when disk write succeeds', async () => { + vi.mocked(invoke).mockResolvedValueOnce(undefined) + const { result } = renderHook(() => useNoteActions(makeConfig())) + + await act(async () => { + result.current.handleCreateNote('Good Note', 'Note') + await new Promise((r) => setTimeout(r, 0)) + }) + + expect(removeEntry).not.toHaveBeenCalled() + expect(setToastMessage).not.toHaveBeenCalled() + }) + + it('handles rapid creation with one failure independently', async () => { + vi.mocked(invoke) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error('disk full')) + .mockResolvedValueOnce(undefined) + + const { result } = renderHook(() => useNoteActions(makeConfig())) + + await act(async () => { + result.current.handleCreateNoteImmediate() + result.current.handleCreateNoteImmediate() + result.current.handleCreateNoteImmediate() + await new Promise((r) => setTimeout(r, 0)) + }) + + expect(addEntry).toHaveBeenCalledTimes(3) + expect(removeEntry).toHaveBeenCalledTimes(1) + expect(removeEntry).toHaveBeenCalledWith(expect.stringContaining('untitled-note-2.md')) + }) + }) }) diff --git a/src/hooks/useNoteActions.ts b/src/hooks/useNoteActions.ts index 7dd2aa58..08d005db 100644 --- a/src/hooks/useNoteActions.ts +++ b/src/hooks/useNoteActions.ts @@ -21,6 +21,7 @@ interface RenameResult { export interface NoteActionsConfig { addEntry: (entry: VaultEntry, content: string) => void + removeEntry: (path: string) => void updateContent: (path: string, content: string) => void entries: VaultEntry[] setToastMessage: (msg: string | null) => void @@ -49,13 +50,10 @@ async function loadNoteContent(path: string): Promise { : mockInvoke('get_note_content', { path }) } -/** Fire-and-forget: persist a newly created note to disk. */ -function persistNewNote(path: string, content: string): void { - if (isTauri()) { - invoke('save_note_content', { path, content }).catch((err) => - console.error('Failed to persist new note:', err), - ) - } +/** Persist a newly created note to disk. Returns a Promise for error handling. */ +function persistNewNote(path: string, content: string): Promise { + if (!isTauri()) return Promise.resolve() + return invoke('save_note_content', { path, content }).then(() => {}) } export function buildNewEntry({ path, slug, title, type, status }: NewEntryParams): VaultEntry { @@ -178,6 +176,30 @@ function findWikilinkTarget(entries: VaultEntry[], target: string): VaultEntry | return entries.find((e) => entryMatchesTarget(e, targetLower, targetAsWords)) } +/** Navigate to a wikilink target, logging a warning if not found. */ +function navigateWikilink(entries: VaultEntry[], target: string, selectNote: (e: VaultEntry) => void): void { + const found = findWikilinkTarget(entries, target) + if (found) selectNote(found) + else console.warn(`Navigation target not found: ${target}`) +} + +/** Persist to disk; on failure, call the revert handler. */ +function persistOptimistic(path: string, content: string, onFail: (p: string) => void): void { + persistNewNote(path, content).catch(() => onFail(path)) +} + +/** Optimistically add entry to UI, open tab, and persist to disk. */ +function createAndPersist( + resolved: { entry: VaultEntry; content: string }, + addFn: (e: VaultEntry, c: string) => void, + openTab: (e: VaultEntry, c: string) => void, + onFail: (p: string) => void, +): void { + addEntryWithMock(resolved.entry, resolved.content, addFn) + openTab(resolved.entry, resolved.content) + persistOptimistic(resolved.entry.path, resolved.content, onFail) +} + async function executeFrontmatterOp(op: 'update' | 'delete', path: string, key: string, value?: FrontmatterValue): Promise { if (op === 'update') { return isTauri() ? invokeFrontmatter('update_frontmatter', { path, key, value }) : applyMockFrontmatterUpdate(path, key, value!) @@ -202,10 +224,26 @@ async function reloadTabsAfterRename( } } +/** Run a frontmatter update/delete and apply the result to state. */ +async function runFrontmatterAndApply( + op: 'update' | 'delete', path: string, key: string, value: FrontmatterValue | undefined, + callbacks: { updateTab: (p: string, c: string) => void; updateEntry: (p: string, patch: Partial) => void; toast: (m: string | null) => void }, +): Promise { + try { + callbacks.updateTab(path, await executeFrontmatterOp(op, path, key, value)) + const patch = frontmatterToEntryPatch(op, key, value) + if (Object.keys(patch).length > 0) callbacks.updateEntry(path, patch) + callbacks.toast(op === 'update' ? 'Property updated' : 'Property deleted') + } catch (err) { + console.error(`Failed to ${op} frontmatter:`, err) + callbacks.toast(`Failed to ${op} property`) + } +} + export function useNoteActions(config: NoteActionsConfig) { - const { addEntry, updateContent, entries, setToastMessage, updateEntry } = config + const { addEntry, removeEntry, updateContent, entries, setToastMessage, updateEntry } = config const tabMgmt = useTabManagement() - const { setTabs, handleSelectNote, openTabWithContent, activeTabPathRef, handleSwitchTab } = tabMgmt + const { setTabs, handleSelectNote, openTabWithContent, handleCloseTab, activeTabPathRef, handleSwitchTab } = tabMgmt const tabsRef = useRef(tabMgmt.tabs) // eslint-disable-next-line react-hooks/refs tabsRef.current = tabMgmt.tabs @@ -215,22 +253,23 @@ export function useNoteActions(config: NoteActionsConfig) { updateContent(path, newContent) }, [setTabs, updateContent]) - const handleNavigateWikilink = useCallback((target: string) => { - const found = findWikilinkTarget(entries, target) - if (found) handleSelectNote(found) - else console.warn(`Navigation target not found: ${target}`) - }, [entries, handleSelectNote]) + const handleNavigateWikilink = useCallback( + (target: string) => navigateWikilink(entries, target, handleSelectNote), + [entries, handleSelectNote], + ) + + const revertOptimisticNote = useCallback((path: string) => { + handleCloseTab(path) + removeEntry(path) + setToastMessage('Failed to create note — disk write error') + }, [handleCloseTab, removeEntry, setToastMessage]) const pendingNamesRef = useRef>(new Set()) const handleCreateNote = useCallback((title: string, type: string) => { - const { entry, content } = resolveNewNote(title, type) - addEntryWithMock(entry, content, addEntry) - openTabWithContent(entry, content) - persistNewNote(entry.path, content) - }, [openTabWithContent, addEntry]) + createAndPersist(resolveNewNote(title, type), addEntry, openTabWithContent, revertOptimisticNote) + }, [openTabWithContent, addEntry, revertOptimisticNote]) - /** Create a note immediately with an auto-generated unique title. Dedup-safe for rapid calls. */ const handleCreateNoteImmediate = useCallback((type?: string) => { const noteType = type || 'Note' const title = generateUntitledName(entries, noteType, pendingNamesRef.current) @@ -241,28 +280,19 @@ export function useNoteActions(config: NoteActionsConfig) { }, [entries, handleCreateNote]) const handleCreateType = useCallback((typeName: string) => { - const { entry, content } = resolveNewType(typeName) - addEntryWithMock(entry, content, addEntry) - openTabWithContent(entry, content) - persistNewNote(entry.path, content) - }, [openTabWithContent, addEntry]) + createAndPersist(resolveNewType(typeName), addEntry, openTabWithContent, revertOptimisticNote) + }, [openTabWithContent, addEntry, revertOptimisticNote]) - const runFrontmatterOp = useCallback(async (op: 'update' | 'delete', path: string, key: string, value?: FrontmatterValue) => { - try { - updateTabContent(path, await executeFrontmatterOp(op, path, key, value)) - const patch = frontmatterToEntryPatch(op, key, value) - if (Object.keys(patch).length > 0) updateEntry(path, patch) - setToastMessage(op === 'update' ? 'Property updated' : 'Property deleted') - } catch (err) { - console.error(`Failed to ${op} frontmatter:`, err) - setToastMessage(`Failed to ${op} property`) - } - }, [updateTabContent, updateEntry, setToastMessage]) + const fmCallbacks = { updateTab: updateTabContent, updateEntry, toast: setToastMessage } + + const runFrontmatterOp = useCallback( + (op: 'update' | 'delete', path: string, key: string, value?: FrontmatterValue) => + runFrontmatterAndApply(op, path, key, value, fmCallbacks), + [updateTabContent, updateEntry, setToastMessage], // eslint-disable-line react-hooks/exhaustive-deps -- fmCallbacks is stable when deps are + ) const handleRenameNote = useCallback(async ( - path: string, - newTitle: string, - vaultPath: string, + path: string, newTitle: string, vaultPath: string, onEntryRenamed: (oldPath: string, newEntry: Partial & { path: string }, newContent: string) => void, ) => { try { @@ -270,15 +300,10 @@ export function useNoteActions(config: NoteActionsConfig) { const newContent = await loadNoteContent(result.new_path) const entry = entries.find((e) => e.path === path) const newEntry = buildRenamedEntry(entry ?? {} as VaultEntry, newTitle, result.new_path) - - const otherTabPaths = tabsRef.current - .filter(t => t.entry.path !== path) - .map(t => t.entry.path) - + const otherTabPaths = tabsRef.current.filter(t => t.entry.path !== path).map(t => t.entry.path) setTabs((prev) => prev.map((t) => t.entry.path === path ? { entry: newEntry, content: newContent } : t)) if (activeTabPathRef.current === path) handleSwitchTab(result.new_path) onEntryRenamed(path, newEntry, newContent) - await reloadTabsAfterRename(otherTabPaths, updateTabContent) setToastMessage(renameToastMessage(result.updated_files)) } catch (err) { diff --git a/src/hooks/useVaultLoader.test.ts b/src/hooks/useVaultLoader.test.ts index 55404b81..bc0a6a87 100644 --- a/src/hooks/useVaultLoader.test.ts +++ b/src/hooks/useVaultLoader.test.ts @@ -49,17 +49,20 @@ vi.mock('../mock-tauri', () => ({ mockInvoke: (cmd: string, args?: Record) => mockInvokeFn(cmd, args), })) +/** Render the vault loader hook and wait for initial data to load. */ +async function renderVaultLoader() { + const hook = renderHook(() => useVaultLoader('/vault')) + await waitFor(() => { expect(hook.result.current.entries).toHaveLength(1) }) + return hook +} + describe('useVaultLoader', () => { beforeEach(() => { mockInvokeFn.mockImplementation(defaultMockInvoke) }) it('loads entries and content on mount', async () => { - const { result } = renderHook(() => useVaultLoader('/vault')) - - await waitFor(() => { - expect(result.current.entries).toHaveLength(1) - }) + const { result } = await renderVaultLoader() expect(result.current.entries[0].title).toBe('Hello') expect(result.current.allContent['/vault/note/hello.md']).toContain('# Hello') @@ -77,22 +80,10 @@ describe('useVaultLoader', () => { describe('addEntry', () => { it('prepends new entry and adds content', async () => { - const { result } = renderHook(() => useVaultLoader('/vault')) + const { result } = await renderVaultLoader() + const newEntry: VaultEntry = { ...mockEntries[0], path: '/vault/note/new.md', filename: 'new.md', title: 'New Note' } - await waitFor(() => { - expect(result.current.entries).toHaveLength(1) - }) - - const newEntry: VaultEntry = { - ...mockEntries[0], - path: '/vault/note/new.md', - filename: 'new.md', - title: 'New Note', - } - - act(() => { - result.current.addEntry(newEntry, '# New Note') - }) + act(() => { result.current.addEntry(newEntry, '# New Note') }) expect(result.current.entries).toHaveLength(2) expect(result.current.entries[0].title).toBe('New Note') @@ -100,18 +91,8 @@ describe('useVaultLoader', () => { }) it('ignores duplicate entry with same path', async () => { - const { result } = renderHook(() => useVaultLoader('/vault')) - - await waitFor(() => { - expect(result.current.entries).toHaveLength(1) - }) - - const newEntry: VaultEntry = { - ...mockEntries[0], - path: '/vault/note/new.md', - filename: 'new.md', - title: 'New Note', - } + const { result } = await renderVaultLoader() + const newEntry: VaultEntry = { ...mockEntries[0], path: '/vault/note/new.md', filename: 'new.md', title: 'New Note' } act(() => { result.current.addEntry(newEntry, '# New Note') @@ -124,31 +105,38 @@ describe('useVaultLoader', () => { describe('updateContent', () => { it('updates content for an existing path', async () => { - const { result } = renderHook(() => useVaultLoader('/vault')) + const { result } = await renderVaultLoader() - await waitFor(() => { - expect(result.current.allContent['/vault/note/hello.md']).toBeDefined() - }) - - act(() => { - result.current.updateContent('/vault/note/hello.md', '# Updated') - }) + act(() => { result.current.updateContent('/vault/note/hello.md', '# Updated') }) expect(result.current.allContent['/vault/note/hello.md']).toBe('# Updated') }) }) + describe('removeEntry', () => { + it('removes entry and content by path', async () => { + const { result } = await renderVaultLoader() + + act(() => { result.current.removeEntry('/vault/note/hello.md') }) + + expect(result.current.entries).toHaveLength(0) + expect(result.current.allContent['/vault/note/hello.md']).toBeUndefined() + }) + + it('is a no-op for non-existent paths', async () => { + const { result } = await renderVaultLoader() + + act(() => { result.current.removeEntry('/vault/note/nonexistent.md') }) + + expect(result.current.entries).toHaveLength(1) + }) + }) + describe('updateEntry', () => { it('patches an existing entry by path', async () => { - const { result } = renderHook(() => useVaultLoader('/vault')) + const { result } = await renderVaultLoader() - await waitFor(() => { - expect(result.current.entries).toHaveLength(1) - }) - - act(() => { - result.current.updateEntry('/vault/note/hello.md', { archived: true, status: 'Done' }) - }) + act(() => { result.current.updateEntry('/vault/note/hello.md', { archived: true, status: 'Done' }) }) expect(result.current.entries[0].archived).toBe(true) expect(result.current.entries[0].status).toBe('Done') @@ -168,22 +156,10 @@ describe('useVaultLoader', () => { }) it('returns new for freshly added entries', async () => { - const { result } = renderHook(() => useVaultLoader('/vault')) + const { result } = await renderVaultLoader() + const newEntry: VaultEntry = { ...mockEntries[0], path: '/vault/note/brand-new.md', filename: 'brand-new.md', title: 'Brand New' } - await waitFor(() => { - expect(result.current.entries).toHaveLength(1) - }) - - const newEntry: VaultEntry = { - ...mockEntries[0], - path: '/vault/note/brand-new.md', - filename: 'brand-new.md', - title: 'Brand New', - } - - act(() => { - result.current.addEntry(newEntry, '# Brand New') - }) + act(() => { result.current.addEntry(newEntry, '# Brand New') }) expect(result.current.getNoteStatus('/vault/note/brand-new.md')).toBe('new') }) diff --git a/src/hooks/useVaultLoader.ts b/src/hooks/useVaultLoader.ts index 6807247a..b3233703 100644 --- a/src/hooks/useVaultLoader.ts +++ b/src/hooks/useVaultLoader.ts @@ -91,6 +91,11 @@ export function useVaultLoader(vaultPath: string) { const updateEntry = useCallback((path: string, patch: Partial) => setEntries((prev) => prev.map((e) => e.path === path ? { ...e, ...patch } : e)), []) + const removeEntry = useCallback((path: string) => { + setEntries((prev) => prev.filter((e) => e.path !== path)) + setAllContent((prev) => { const next = { ...prev }; delete next[path]; return next }) + }, []) + const replaceEntry = useCallback((oldPath: string, patch: Partial & { path: string }, newContent: string) => { setEntries((prev) => prev.map((e) => e.path === oldPath ? { ...e, ...patch } : e)) setAllContent((prev) => { const next = { ...prev }; delete next[oldPath]; next[patch.path] = newContent; return next }) @@ -115,7 +120,7 @@ export function useVaultLoader(vaultPath: string) { return { entries, allContent, modifiedFiles, - addEntry, updateEntry, replaceEntry, updateContent, + addEntry, updateEntry, removeEntry, replaceEntry, updateContent, loadModifiedFiles, loadGitHistory, loadDiff, loadDiffAtCommit, getNoteStatus, commitAndPush, }