From fb39c6679aee6e0a0fd87e255a4bfd57e91fe66d Mon Sep 17 00:00:00 2001 From: lucaronin Date: Mon, 27 Apr 2026 04:04:04 +0200 Subject: [PATCH] fix: handle invalid Windows save paths --- docs/ABSTRACTIONS.md | 1 + src-tauri/src/commands/vault/file_cmds.rs | 22 +++++ src-tauri/src/vault/file.rs | 81 ++++++++++++++++-- src/hooks/useAppSave.test.ts | 29 +++++++ src/hooks/useAppSave.ts | 8 +- src/hooks/useEditorSave.test.ts | 62 ++++++++++++++ src/hooks/useEditorSave.ts | 35 +++++++- tests/smoke/windows-save-path-failure.spec.ts | 84 +++++++++++++++++++ 8 files changed, 307 insertions(+), 15 deletions(-) create mode 100644 tests/smoke/windows-save-path-failure.spec.ts diff --git a/docs/ABSTRACTIONS.md b/docs/ABSTRACTIONS.md index ff09a255..764ed059 100644 --- a/docs/ABSTRACTIONS.md +++ b/docs/ABSTRACTIONS.md @@ -266,6 +266,7 @@ Tolaria separates **display title** from the file identifier: - **Explicit filename actions** (`rename_note`): breadcrumb rename/sync actions stage crash-safe note renames through a hidden `.tolaria-rename-txn/` transaction directory, recover unfinished renames on the next vault scan, update wikilinks across the vault, and surface any failed backlink rewrites instead of silently reporting partial success. The editor body remains the title editing surface. - **Unicode-aware note stems** (`src/utils/noteSlug.ts`, `vault/rename.rs`): frontend and backend slugging preserve Unicode letters/digits in note filenames, untitled-rename detection, and fallback wikilink targets while still collapsing symbol-only titles to `untitled`. - **Portable filename validation** (`vault/filename_rules.rs`): note filenames, folder names, and custom view filenames all reject Windows-reserved device names, invalid characters, and trailing dot/space suffixes so a vault created on macOS/Linux still clones and syncs cleanly on Windows. +- **Recoverable save failures** (`useEditorSave`, `vault/file.rs`): invalid platform path syntax is reported as a clear retryable save error, while the editor keeps the unsaved buffer intact for another attempt. - **Untitled drafts** start as `untitled-*.md` and are auto-renamed on save once the note gains an H1. ### Title Surface (UI) diff --git a/src-tauri/src/commands/vault/file_cmds.rs b/src-tauri/src/commands/vault/file_cmds.rs index b0850ab8..18191def 100644 --- a/src-tauri/src/commands/vault/file_cmds.rs +++ b/src-tauri/src/commands/vault/file_cmds.rs @@ -266,6 +266,28 @@ mod tests { .contains("title: Command Note")); } + #[test] + fn note_content_commands_accept_windows_sensitive_valid_segments() { + let dir = TempDir::new().unwrap(); + let root = vault_root(&dir); + let note = root + .join("@raflymln") + .join("notes with spaces") + .join("résumé note.md"); + + save_note_content( + note.clone(), + "# Windows-Sensitive Path\n\nBody\n".to_string(), + Some(root.clone()), + ) + .unwrap(); + + assert_eq!( + get_note_content(note, Some(root)).unwrap(), + "# Windows-Sensitive Path\n\nBody\n" + ); + } + #[test] fn folder_and_listing_commands_use_expanded_vault_root() { let dir = TempDir::new().unwrap(); diff --git a/src-tauri/src/vault/file.rs b/src-tauri/src/vault/file.rs index 7e43d56a..a4f5a018 100644 --- a/src-tauri/src/vault/file.rs +++ b/src-tauri/src/vault/file.rs @@ -1,5 +1,5 @@ use std::fs; -use std::io::{ErrorKind, Write}; +use std::io::{Error, ErrorKind, Write}; use std::path::Path; use std::time::UNIX_EPOCH; @@ -25,6 +25,49 @@ fn invalid_utf8_text_error(path: &Path) -> String { format!("File is not valid UTF-8 text: {}", path.display()) } +fn is_invalid_platform_path_error(error: &Error) -> bool { + error.kind() == ErrorKind::InvalidInput || error.raw_os_error() == Some(123) +} + +#[derive(Clone, Copy)] +enum NoteIoOperation { + Save, + Create, +} + +#[derive(Clone, Copy)] +struct NotePathDisplay<'a> { + value: &'a str, +} + +impl<'a> NotePathDisplay<'a> { + fn new(value: &'a str) -> Self { + Self { value } + } +} + +impl NoteIoOperation { + fn verb(self) -> &'static str { + match self { + Self::Save => "save", + Self::Create => "create", + } + } +} + +fn note_io_error(operation: NoteIoOperation, path: NotePathDisplay<'_>, error: &Error) -> String { + let verb = operation.verb(); + if is_invalid_platform_path_error(error) { + let path = path.value; + format!( + "Failed to {verb} note: the path is invalid on this platform. Rename the note or move it to a valid folder, then try again. Path: {path}" + ) + } else { + let path = path.value; + format!("Failed to {verb} {path}: {error}") + } +} + /// Read the content of a single note file. pub fn get_note_content(path: &Path) -> Result { if !path.exists() { @@ -62,12 +105,14 @@ pub fn save_note_content(path: &str, content: &str) -> Result<(), String> { let file_path = Path::new(path); if let Some(parent) = file_path.parent() { if !parent.exists() { - fs::create_dir_all(parent) - .map_err(|e| format!("Failed to create directory {}: {}", parent.display(), e))?; + fs::create_dir_all(parent).map_err(|e| { + note_io_error(NoteIoOperation::Save, NotePathDisplay::new(path), &e) + })?; } } validate_save_path(file_path, path)?; - fs::write(file_path, content).map_err(|e| format!("Failed to save {}: {}", path, e)) + fs::write(file_path, content) + .map_err(|e| note_io_error(NoteIoOperation::Save, NotePathDisplay::new(path), &e)) } /// Create a new note file without overwriting any existing file. @@ -75,8 +120,9 @@ pub fn create_note_content(path: &str, content: &str) -> Result<(), String> { let file_path = Path::new(path); if let Some(parent) = file_path.parent() { if !parent.exists() { - fs::create_dir_all(parent) - .map_err(|e| format!("Failed to create directory {}: {}", parent.display(), e))?; + fs::create_dir_all(parent).map_err(|e| { + note_io_error(NoteIoOperation::Create, NotePathDisplay::new(path), &e) + })?; } } validate_save_path(file_path, path)?; @@ -86,8 +132,27 @@ pub fn create_note_content(path: &str, content: &str) -> Result<(), String> { .open(file_path) .map_err(|e| match e.kind() { ErrorKind::AlreadyExists => format!("File already exists: {}", path), - _ => format!("Failed to create {}: {}", path, e), + _ => note_io_error(NoteIoOperation::Create, NotePathDisplay::new(path), &e), })?; file.write_all(content.as_bytes()) - .map_err(|e| format!("Failed to save {}: {}", path, e)) + .map_err(|e| note_io_error(NoteIoOperation::Save, NotePathDisplay::new(path), &e)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn formats_windows_invalid_path_syntax_as_recoverable_save_error() { + let path = r"C:\Users\@raflymln\notes\untitled-note-1777236475.md"; + let message = note_io_error( + NoteIoOperation::Save, + NotePathDisplay::new(path), + &Error::from_raw_os_error(123), + ); + + assert!(message.contains("path is invalid on this platform")); + assert!(message.contains("Rename the note or move it to a valid folder")); + assert!(!message.contains("os error 123")); + } } diff --git a/src/hooks/useAppSave.test.ts b/src/hooks/useAppSave.test.ts index cd7da4af..04b38545 100644 --- a/src/hooks/useAppSave.test.ts +++ b/src/hooks/useAppSave.test.ts @@ -169,6 +169,35 @@ describe('useAppSave', () => { // Should complete without error }) + it('handles Windows invalid path save failures without clearing unsaved content', async () => { + vi.mocked(isTauri).mockReturnValue(true) + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const path = 'C:\\Users\\@raflymln\\notes\\untitled-note-1777236475.md' + const entry = makeEntry(path, 'Untitled Note 1777236475', 'untitled-note-1777236475.md') + vi.mocked(invoke).mockRejectedValueOnce( + new Error('The filename, directory name, or volume label syntax is incorrect. (os error 123)'), + ) + + const { result } = renderSave({ + tabs: [{ entry, content: '# Draft\n\nBody' }], + activeTabPath: path, + unsavedPaths: new Set([path]), + }) + + let saved = true + await act(async () => { + saved = await result.current.handleSave() + }) + + expect(saved).toBe(false) + expect(deps.setToastMessage).toHaveBeenCalledWith( + 'Save failed: The note path is invalid on this platform. Rename the note or move it to a valid folder, then try again.', + ) + expect(deps.clearUnsaved).not.toHaveBeenCalled() + expect(vi.mocked(invoke)).not.toHaveBeenCalledWith('auto_rename_untitled', expect.anything()) + consoleSpy.mockRestore() + }) + it('handleContentChange is a function', () => { const { result } = renderSave() expect(typeof result.current.handleContentChange).toBe('function') diff --git a/src/hooks/useAppSave.ts b/src/hooks/useAppSave.ts index 28d63ddc..bf40e9ef 100644 --- a/src/hooks/useAppSave.ts +++ b/src/hooks/useAppSave.ts @@ -565,7 +565,7 @@ function useHandleSaveAction({ flushPendingUntitledRename, resolveCurrentPath, }: { - handleSaveRaw: (unsavedFallback?: { path: string; content: string }) => Promise + handleSaveRaw: (unsavedFallback?: { path: string; content: string }) => Promise tabs: TabState[] activeTabPath: string | null unsavedPaths: Set @@ -574,12 +574,14 @@ function useHandleSaveAction({ }) { return useCallback(async () => { const resolvedActiveTabPath = activeTabPath ? resolveCurrentPath(activeTabPath) : null - await handleSaveRaw(findUnsavedFallback({ + const saveCompleted = await handleSaveRaw(findUnsavedFallback({ tabs, activeTabPath: resolvedActiveTabPath, unsavedPaths, })) + if (!saveCompleted) return false await flushPendingUntitledRename(resolvedActiveTabPath ?? undefined) + return true }, [handleSaveRaw, tabs, activeTabPath, unsavedPaths, flushPendingUntitledRename, resolveCurrentPath]) } @@ -696,7 +698,7 @@ function useAppSaveHandlers({ resolvedPath: string replaceRenamedEntry: (oldPath: string, newEntry: Partial & { path: string }, newContent: string) => void loadModifiedFiles: AppSaveDeps['loadModifiedFiles'] - handleSaveRaw: (unsavedFallback?: { path: string; content: string }) => Promise + handleSaveRaw: (unsavedFallback?: { path: string; content: string }) => Promise tabs: TabState[] unsavedPaths: Set }) { diff --git a/src/hooks/useEditorSave.test.ts b/src/hooks/useEditorSave.test.ts index 9325a59a..47a45b2e 100644 --- a/src/hooks/useEditorSave.test.ts +++ b/src/hooks/useEditorSave.test.ts @@ -84,6 +84,42 @@ describe('useEditorSave', () => { consoleSpy.mockRestore() }) + it('keeps failed Windows path saves pending with a recoverable error toast', async () => { + const path = 'C:\\Users\\@raflymln\\notes\\untitled-note-1777236475.md' + mockInvokeFn.mockRejectedValueOnce( + new Error(`Failed to save ${path}: The filename, directory name, or volume label syntax is incorrect. (os error 123)`), + ) + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { result } = renderSaveHook() + + act(() => { + result.current.handleContentChange(path, '# Draft\n\nUnsaved body') + }) + + let saved = true + await act(async () => { + saved = await result.current.handleSave() + }) + + expect(saved).toBe(false) + expect(setToastMessage).toHaveBeenCalledWith( + 'Save failed: The note path is invalid on this platform. Rename the note or move it to a valid folder, then try again.', + ) + expect(updateVaultContent).not.toHaveBeenCalled() + + await act(async () => { + saved = await result.current.handleSave() + }) + + expect(saved).toBe(true) + expect(mockInvokeFn).toHaveBeenLastCalledWith('save_note_content', { + path, + content: '# Draft\n\nUnsaved body', + }) + expect(updateVaultContent).toHaveBeenCalledWith(path, '# Draft\n\nUnsaved body') + consoleSpy.mockRestore() + }) + it('savePendingForPath saves content only for the matching path', async () => { const { result } = renderSaveHook() @@ -320,6 +356,32 @@ describe('useEditorSave', () => { expect(setToastMessage).not.toHaveBeenCalled() }) + it('auto-save reports invalid path failures and leaves content retryable', async () => { + const path = 'C:\\Users\\@raflymln\\notes\\untitled-note-1777236475.md' + mockInvokeFn.mockRejectedValueOnce(new Error('The filename, directory name, or volume label syntax is incorrect. (os error 123)')) + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { result } = renderHook(() => + useEditorSave({ updateVaultContent, setTabs, setToastMessage }) + ) + + act(() => { result.current.handleContentChange(path, 'draft from auto-save') }) + await act(async () => { await vi.advanceTimersByTimeAsync(500) }) + + expect(setToastMessage).toHaveBeenCalledWith( + 'Save failed: The note path is invalid on this platform. Rename the note or move it to a valid folder, then try again.', + ) + expect(updateVaultContent).not.toHaveBeenCalled() + + await act(async () => { await result.current.handleSave() }) + + expect(mockInvokeFn).toHaveBeenLastCalledWith('save_note_content', { + path, + content: 'draft from auto-save', + }) + expect(updateVaultContent).toHaveBeenCalledWith(path, 'draft from auto-save') + consoleSpy.mockRestore() + }) + it('Cmd+S cancels pending auto-save and saves immediately', async () => { const { result } = renderHook(() => useEditorSave({ updateVaultContent, setTabs, setToastMessage }) diff --git a/src/hooks/useEditorSave.ts b/src/hooks/useEditorSave.ts index ed0c26d8..ed1dbbfc 100644 --- a/src/hooks/useEditorSave.ts +++ b/src/hooks/useEditorSave.ts @@ -28,12 +28,31 @@ interface EditorSaveConfig { const noop = () => {} const AUTO_SAVE_DEBOUNCE_MS = 500 +const INVALID_PATH_SAVE_MESSAGE = 'Save failed: The note path is invalid on this platform. Rename the note or move it to a valid folder, then try again.' interface PendingContent { path: string content: string } +function errorMessage(error: unknown): string { + if (error instanceof Error) return error.message + return String(error) +} + +function isInvalidPathSaveError(message: string): boolean { + const normalized = message.toLowerCase() + return normalized.includes('os error 123') + || normalized.includes('filename, directory name, or volume label syntax is incorrect') + || normalized.includes('path is invalid on this platform') +} + +function formatSaveFailureMessage(error: unknown): string { + const message = errorMessage(error) + if (isInvalidPathSaveError(message)) return INVALID_PATH_SAVE_MESSAGE + return `Save failed: ${message}` +} + function resolveBufferedPath(path: string, resolvePath?: EditorSaveConfig['resolvePath']): string { return resolvePath?.(path) ?? path } @@ -91,10 +110,12 @@ function scheduleAutoSave({ autoSaveTimerRef, flushPending, onAfterSaveRef, + setToastMessage, }: { autoSaveTimerRef: MutableRefObject | null> flushPending: () => Promise onAfterSaveRef: MutableRefObject<() => void> + setToastMessage: EditorSaveConfig['setToastMessage'] }): void { autoSaveTimerRef.current = setTimeout(async () => { autoSaveTimerRef.current = null @@ -103,6 +124,7 @@ function scheduleAutoSave({ if (saved) onAfterSaveRef.current() } catch (err) { console.error('Auto-save failed:', err) + setToastMessage(formatSaveFailureMessage(err)) } }, AUTO_SAVE_DEBOUNCE_MS) } @@ -198,7 +220,7 @@ function useImmediateSaveCommands({ resolvePath?: EditorSaveConfig['resolvePath'] resolvePathBeforeSave?: EditorSaveConfig['resolvePathBeforeSave'] }) { - const handleSave = useCallback(async (unsavedFallback?: { path: string; content: string }) => { + const handleSave = useCallback(async (unsavedFallback?: { path: string; content: string }): Promise => { cancelAutoSave() try { const saved = await flushPending() @@ -211,9 +233,11 @@ function useImmediateSaveCommands({ }) setToastMessage(saved || savedFallback ? 'Saved' : 'Nothing to save') onAfterSave() + return true } catch (err) { console.error('Save failed:', err) - setToastMessage(`Save failed: ${err}`) + setToastMessage(formatSaveFailureMessage(err)) + return false } }, [cancelAutoSave, flushPending, onAfterSave, onNotePersisted, resolvePath, resolvePathBeforeSave, saveNote, setToastMessage]) @@ -231,6 +255,7 @@ function useContentChangeCommand({ pendingContentRef, autoSaveTimerRef, setTabs, + setToastMessage, cancelAutoSave, flushPending, onAfterSaveRef, @@ -238,6 +263,7 @@ function useContentChangeCommand({ pendingContentRef: MutableRefObject autoSaveTimerRef: MutableRefObject | null> setTabs: EditorSaveConfig['setTabs'] + setToastMessage: EditorSaveConfig['setToastMessage'] cancelAutoSave: () => void flushPending: () => Promise onAfterSaveRef: MutableRefObject<() => void> @@ -246,8 +272,8 @@ function useContentChangeCommand({ pendingContentRef.current = { path, content } applyTabContent(setTabs, path, content) cancelAutoSave() - scheduleAutoSave({ autoSaveTimerRef, flushPending, onAfterSaveRef }) - }, [autoSaveTimerRef, cancelAutoSave, flushPending, onAfterSaveRef, pendingContentRef, setTabs]) + scheduleAutoSave({ autoSaveTimerRef, flushPending, onAfterSaveRef, setToastMessage }) + }, [autoSaveTimerRef, cancelAutoSave, flushPending, onAfterSaveRef, pendingContentRef, setTabs, setToastMessage]) } function useEditorSaveCommands({ @@ -295,6 +321,7 @@ function useEditorSaveCommands({ pendingContentRef, autoSaveTimerRef, setTabs, + setToastMessage, cancelAutoSave, flushPending: () => flushPending(), onAfterSaveRef, diff --git a/tests/smoke/windows-save-path-failure.spec.ts b/tests/smoke/windows-save-path-failure.spec.ts new file mode 100644 index 00000000..dcb9a64c --- /dev/null +++ b/tests/smoke/windows-save-path-failure.spec.ts @@ -0,0 +1,84 @@ +import { test, expect, type Page } from '@playwright/test' +import { executeCommand, openCommandPalette, sendShortcut } from './helpers' + +const RAW_EDITOR = '.cm-content' + +type MockHandler = (args?: Record) => unknown + +async function openFirstNote(page: Page) { + await page.waitForSelector('[data-testid="sidebar-top-nav"]', { timeout: 10_000 }) + const noteList = page.locator('[data-testid="note-list-container"]') + await noteList.waitFor({ timeout: 5_000 }) + await noteList.locator('.cursor-pointer').first().click() + await expect(page.locator('.bn-editor')).toBeVisible({ timeout: 5_000 }) +} + +async function setRawEditorContent(page: Page, content: string) { + await page.evaluate((nextContent) => { + const el = document.querySelector('.cm-content') + if (!el) throw new Error('CodeMirror content element is missing') + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const view = (el as any).cmTile?.view + if (!view) throw new Error('CodeMirror view is missing') + view.dispatch({ + changes: { from: 0, to: view.state.doc.length, insert: nextContent }, + }) + view.focus() + }, content) +} + +async function openRawMode(page: Page) { + await openCommandPalette(page) + await executeCommand(page, 'Toggle Raw') + await expect(page.locator(RAW_EDITOR)).toBeVisible({ timeout: 5_000 }) +} + +async function installFailOnceSaveMock(page: Page) { + await page.waitForFunction(() => Boolean(window.__mockHandlers?.save_note_content)) + await page.evaluate(() => { + const handlers = window.__mockHandlers as Record + const originalSaveNoteContent = handlers.save_note_content + let shouldFail = true + window.__laputaTest = { + ...window.__laputaTest, + saveAttempts: [], + } + handlers.save_note_content = (args?: Record) => { + window.__laputaTest?.saveAttempts?.push(args) + if (shouldFail) { + shouldFail = false + throw new Error('The filename, directory name, or volume label syntax is incorrect. (os error 123)') + } + return originalSaveNoteContent?.(args) + } + }) +} + +test('failed Windows path saves show a recoverable toast and retry the draft', async ({ page }) => { + const pageErrors: string[] = [] + page.on('pageerror', (err) => { pageErrors.push(err.message) }) + await page.route('**/api/vault/ping', async (route) => { + await route.fulfill({ status: 404, body: '' }) + }) + + await page.goto('/') + await openFirstNote(page) + await installFailOnceSaveMock(page) + + await openRawMode(page) + await setRawEditorContent(page, '# Retryable Windows Save\n\nDraft that must survive failure') + await page.waitForTimeout(550) + + await sendShortcut(page, 's', ['Control']) + await expect(page.locator('.fixed.bottom-8')).toContainText('note path is invalid on this platform', { timeout: 5_000 }) + expect(pageErrors.filter((message) => message.includes('os error 123'))).toHaveLength(0) + + await sendShortcut(page, 's', ['Control']) + await expect(page.locator('.fixed.bottom-8')).toContainText('Saved', { timeout: 5_000 }) + + const saveAttempts = await page.evaluate(() => window.__laputaTest?.saveAttempts ?? []) + expect(saveAttempts).toHaveLength(2) + expect(saveAttempts[1]).toEqual(expect.objectContaining({ + content: '# Retryable Windows Save\n\nDraft that must survive failure', + })) +})