From bf13eed3ab4e3627683100be755f9364fc3fc599 Mon Sep 17 00:00:00 2001 From: lucaronin Date: Wed, 22 Apr 2026 22:37:01 +0200 Subject: [PATCH] fix: handle filename collisions in create flows --- src-tauri/src/commands/vault.rs | 16 + src-tauri/src/commands/vault/file_cmds.rs | 39 ++- src-tauri/src/lib.rs | 1 + src-tauri/src/vault/file.rs | 23 ++ src-tauri/src/vault/mod.rs | 2 +- .../src/vault/mod_tests/type_and_links.rs | 26 ++ src/App.tsx | 39 ++- src/components/CreateTypeDialog.test.tsx | 13 + src/components/CreateTypeDialog.tsx | 8 +- src/components/DynamicPropertiesPanel.tsx | 2 +- src/components/Editor.tsx | 4 +- src/components/EditorRightPanel.tsx | 4 +- src/components/TypeSelector.tsx | 12 +- .../inspector/useInspectorPropertyActions.ts | 4 +- src/hooks/useNoteActions.hook.test.ts | 83 ++--- src/hooks/useNoteCreation.test.ts | 41 +++ src/hooks/useNoteCreation.ts | 331 +++++++++++++----- tests/smoke/collision-create-flows.spec.ts | 85 +++++ 18 files changed, 562 insertions(+), 171 deletions(-) create mode 100644 tests/smoke/collision-create-flows.spec.ts diff --git a/src-tauri/src/commands/vault.rs b/src-tauri/src/commands/vault.rs index 0a0d9bbd..d7fdfefa 100644 --- a/src-tauri/src/commands/vault.rs +++ b/src-tauri/src/commands/vault.rs @@ -139,6 +139,22 @@ mod tests { assert_eq!(err, ACTIVE_VAULT_PATH_ERROR); } + #[test] + fn test_create_note_content_rejects_traversal_outside_active_vault() { + let dir = tempfile::TempDir::new().unwrap(); + let vault_path = dir.path(); + let escape_path = vault_path.join("../outside.md"); + + let err = create_note_content( + escape_path, + "# Outside\n".to_string(), + vault_path_arg(vault_path), + ) + .expect_err("expected traversal create to be rejected"); + + assert_eq!(err, ACTIVE_VAULT_PATH_ERROR); + } + #[test] fn test_create_vault_folder_rejects_escape_path() { let dir = tempfile::TempDir::new().unwrap(); diff --git a/src-tauri/src/commands/vault/file_cmds.rs b/src-tauri/src/commands/vault/file_cmds.rs index f1de5f97..07743441 100644 --- a/src-tauri/src/commands/vault/file_cmds.rs +++ b/src-tauri/src/commands/vault/file_cmds.rs @@ -39,6 +39,22 @@ fn with_requested_root_path( with_requested_root(raw_vault_path.as_ref(), action) } +fn with_writable_note_path( + path: PathBuf, + vault_path: Option, + action: impl FnOnce(&str) -> Result, +) -> Result { + with_validated_path( + path.to_string_lossy().as_ref(), + vault_path + .as_ref() + .map(|value| value.to_string_lossy()) + .as_deref(), + ValidatedPathMode::Writable, + action, + ) +} + #[tauri::command] pub fn get_note_content(path: PathBuf, vault_path: Option) -> Result { with_note_path( @@ -55,15 +71,20 @@ pub fn save_note_content( content: String, vault_path: Option, ) -> Result<(), String> { - with_validated_path( - path.to_string_lossy().as_ref(), - vault_path - .as_ref() - .map(|value| value.to_string_lossy()) - .as_deref(), - ValidatedPathMode::Writable, - |validated_path| vault::save_note_content(validated_path, &content), - ) + with_writable_note_path(path, vault_path, |validated_path| { + vault::save_note_content(validated_path, &content) + }) +} + +#[tauri::command] +pub fn create_note_content( + path: PathBuf, + content: String, + vault_path: Option, +) -> Result<(), String> { + with_writable_note_path(path, vault_path, |validated_path| { + vault::create_note_content(validated_path, &content) + }) } #[tauri::command] diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 0adee2a3..ca1a3e03 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -196,6 +196,7 @@ macro_rules! app_invoke_handler { commands::list_vault, commands::list_vault_folders, commands::get_note_content, + commands::create_note_content, commands::save_note_content, commands::update_frontmatter, commands::delete_frontmatter_property, diff --git a/src-tauri/src/vault/file.rs b/src-tauri/src/vault/file.rs index 1a0d97e7..e40fa555 100644 --- a/src-tauri/src/vault/file.rs +++ b/src-tauri/src/vault/file.rs @@ -1,4 +1,5 @@ use std::fs; +use std::io::{ErrorKind, Write}; use std::path::Path; use std::time::UNIX_EPOCH; @@ -63,3 +64,25 @@ pub fn save_note_content(path: &str, content: &str) -> Result<(), String> { validate_save_path(file_path, path)?; fs::write(file_path, content).map_err(|e| format!("Failed to save {}: {}", path, e)) } + +/// Create a new note file without overwriting any existing file. +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))?; + } + } + validate_save_path(file_path, path)?; + let mut file = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(file_path) + .map_err(|e| match e.kind() { + ErrorKind::AlreadyExists => format!("File already exists: {}", path), + _ => format!("Failed to create {}: {}", path, e), + })?; + file.write_all(content.as_bytes()) + .map_err(|e| format!("Failed to save {}: {}", path, e)) +} diff --git a/src-tauri/src/vault/mod.rs b/src-tauri/src/vault/mod.rs index ffbecff9..25478e6d 100644 --- a/src-tauri/src/vault/mod.rs +++ b/src-tauri/src/vault/mod.rs @@ -20,7 +20,7 @@ pub use config_seed::{ seed_config_files, AiGuidanceFileState, VaultAiGuidanceStatus, }; pub use entry::{FolderNode, VaultEntry}; -pub use file::{get_note_content, save_note_content}; +pub use file::{create_note_content, get_note_content, save_note_content}; pub use folders::{delete_folder, rename_folder, FolderRenameResult}; pub use getting_started::{create_getting_started_vault, default_vault_path, vault_exists}; pub use image::{copy_image_to_vault, save_image}; diff --git a/src-tauri/src/vault/mod_tests/type_and_links.rs b/src-tauri/src/vault/mod_tests/type_and_links.rs index fd8886ea..4dc0ef05 100644 --- a/src-tauri/src/vault/mod_tests/type_and_links.rs +++ b/src-tauri/src/vault/mod_tests/type_and_links.rs @@ -165,3 +165,29 @@ fn test_save_note_content_deeply_nested_new_directory() { assert!(path.exists()); assert_eq!(fs::read_to_string(&path).unwrap(), content); } + +#[test] +fn test_create_note_content_creates_parent_directory() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("new-type/briefing.md"); + let content = "---\ntitle: Briefing\ntype: Note\n---\n"; + + assert!(!path.parent().unwrap().exists()); + create_note_content(path.to_str().unwrap(), content).unwrap(); + + assert!(path.exists()); + assert_eq!(fs::read_to_string(&path).unwrap(), content); +} + +#[test] +fn test_create_note_content_rejects_existing_file() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("briefing.md"); + fs::write(&path, "# Existing\n").unwrap(); + + let err = create_note_content(path.to_str().unwrap(), "# Replacement\n") + .expect_err("expected create-only write to reject collisions"); + + assert!(err.contains("already exists")); + assert_eq!(fs::read_to_string(&path).unwrap(), "# Existing\n"); +} diff --git a/src/App.tsx b/src/App.tsx index e99cb040..984e3b69 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -30,7 +30,7 @@ import { useVaultLoader } from './hooks/useVaultLoader' import { useAiAgentPreferences } from './hooks/useAiAgentPreferences' import { useSettings } from './hooks/useSettings' import { useNoteActions } from './hooks/useNoteActions' -import { slugify } from './hooks/useNoteCreation' +import { planNewTypeCreation } from './hooks/useNoteCreation' import { useCommitFlow } from './hooks/useCommitFlow' import { useGitRemoteStatus } from './hooks/useGitRemoteStatus' import { useViewMode, type ViewMode } from './hooks/useViewMode' @@ -937,37 +937,40 @@ function App() { const shouldLoadGitHistory = !layout.inspectorCollapsed && !showAIChat const gitHistory = useGitHistory(notes.activeTabPath, vault.loadGitHistory, shouldLoadGitHistory) - const handleCreateType = useCallback((name: string) => { - notes.handleCreateType(name) - setToastMessage(`Type "${name}" created`) + const handleCreateType = useCallback(async (name: string) => { + const created = await notes.handleCreateType(name) + if (created) setToastMessage(`Type "${name}" created`) + return created }, [notes, setToastMessage]) const handleCreateMissingType = useCallback(async (path: string, missingType: string, nextTypeName: string) => { const trimmed = nextTypeName.trim() - if (!trimmed) return + if (!trimmed) return false - const targetFilename = `${slugify(trimmed)}.md` - const exactType = vault.entries.find((entry) => entry.isA === 'Type' && entry.title === trimmed) - const slugMatch = vault.entries.find((entry) => entry.isA === 'Type' && slugify(entry.title) === slugify(trimmed)) - const filenameCollision = vault.entries.find((entry) => entry.filename.toLowerCase() === targetFilename) - const resolvedTypeName = exactType?.title ?? slugMatch?.title ?? trimmed - - if (filenameCollision && filenameCollision.isA !== 'Type') { - setToastMessage(`Cannot create type "${trimmed}" because ${targetFilename} already exists`) - throw new Error(`Type filename collision for ${targetFilename}`) + const plan = planNewTypeCreation({ entries: vault.entries, typeName: trimmed, vaultPath: resolvedPath }) + if (plan.status === 'blocked') { + setToastMessage(plan.message) + return false } - if (!exactType && !slugMatch) { - await notes.createTypeEntrySilent(trimmed) + let resolvedTypeName = plan.status === 'existing' ? plan.entry.title : trimmed + + if (plan.status === 'create') { + try { + resolvedTypeName = (await notes.createTypeEntrySilent(trimmed)).title + } catch { + return false + } } await notes.handleUpdateFrontmatter(path, 'type', resolvedTypeName) setToastMessage( - resolvedTypeName === missingType + plan.status === 'create' && resolvedTypeName === missingType ? `Type "${resolvedTypeName}" created` : `Type set to "${resolvedTypeName}"`, ) - }, [notes, setToastMessage, vault.entries]) + return true + }, [notes, resolvedPath, setToastMessage, vault.entries]) const handleCreateOrUpdateView = useCallback(async (definition: ViewDefinition) => { const editing = dialogs.editingView diff --git a/src/components/CreateTypeDialog.test.tsx b/src/components/CreateTypeDialog.test.tsx index 5a51ff89..03dba6f0 100644 --- a/src/components/CreateTypeDialog.test.tsx +++ b/src/components/CreateTypeDialog.test.tsx @@ -46,6 +46,19 @@ describe('CreateTypeDialog', () => { await waitFor(() => expect(onClose).toHaveBeenCalled()) }) + it('stays open when create reports a handled collision', async () => { + const onClose = vi.fn() + const onCreate = vi.fn().mockResolvedValue(false) + render() + + fireEvent.change(screen.getByPlaceholderText('e.g. Recipe, Book, Habit...'), { target: { value: 'Recipe' } }) + fireEvent.click(screen.getByRole('button', { name: 'Create' })) + + await waitFor(() => expect(onCreate).toHaveBeenCalledWith('Recipe')) + expect(onClose).not.toHaveBeenCalled() + expect(screen.getByText('Create New Type')).toBeInTheDocument() + }) + it('calls onClose when Cancel is clicked', () => { const onClose = vi.fn() render( {}} />) diff --git a/src/components/CreateTypeDialog.tsx b/src/components/CreateTypeDialog.tsx index 56a992f9..39c629c6 100644 --- a/src/components/CreateTypeDialog.tsx +++ b/src/components/CreateTypeDialog.tsx @@ -6,14 +6,14 @@ import { Input } from '@/components/ui/input' interface CreateTypeDialogProps { open: boolean onClose: () => void - onCreate: (name: string) => void | Promise + onCreate: (name: string) => boolean | void | Promise initialName?: string } interface CreateTypeDialogFormProps { initialName: string onClose: () => void - onCreate: (name: string) => void | Promise + onCreate: (name: string) => boolean | void | Promise } function CreateTypeDialogForm({ initialName, onClose, onCreate }: CreateTypeDialogFormProps) { @@ -23,8 +23,8 @@ function CreateTypeDialogForm({ initialName, onClose, onCreate }: CreateTypeDial e.preventDefault() const trimmed = name.trim() if (!trimmed) return - await onCreate(trimmed) - onClose() + const created = await onCreate(trimmed) + if (created !== false) onClose() } return ( diff --git a/src/components/DynamicPropertiesPanel.tsx b/src/components/DynamicPropertiesPanel.tsx index e923807b..a1b11bcf 100644 --- a/src/components/DynamicPropertiesPanel.tsx +++ b/src/components/DynamicPropertiesPanel.tsx @@ -239,7 +239,7 @@ export function DynamicPropertiesPanel({ onDeleteProperty?: (key: string) => void onAddProperty?: (key: string, value: FrontmatterValue) => void onNavigate?: (target: string) => void - onCreateMissingType?: (typeName: string) => void | Promise + onCreateMissingType?: (typeName: string) => boolean | void | Promise }) { const { editingKey, setEditingKey, showAddDialog, setShowAddDialog, displayOverrides, diff --git a/src/components/Editor.tsx b/src/components/Editor.tsx index da00c66a..e2de9d7f 100644 --- a/src/components/Editor.tsx +++ b/src/components/Editor.tsx @@ -52,7 +52,7 @@ interface EditorProps { onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise onDeleteProperty?: (path: string, key: string) => Promise onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise - onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise + onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void showAIChat?: boolean @@ -360,7 +360,7 @@ function EditorLayout({ onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise onDeleteProperty?: (path: string, key: string) => Promise onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise - onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise + onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void onFileCreated?: (relativePath: string) => void diff --git a/src/components/EditorRightPanel.tsx b/src/components/EditorRightPanel.tsx index cd029cb4..e0eafd36 100644 --- a/src/components/EditorRightPanel.tsx +++ b/src/components/EditorRightPanel.tsx @@ -27,7 +27,7 @@ interface EditorRightPanelProps { onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise onDeleteProperty?: (path: string, key: string) => Promise onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise - onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise + onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void onToggleRawEditor?: () => void @@ -110,7 +110,7 @@ export function EditorRightPanel({ onUpdateFrontmatter={onUpdateFrontmatter} onDeleteProperty={onDeleteProperty} onAddProperty={onAddProperty} - onCreateMissingType={onCreateMissingType} + onCreateMissingType={onCreateMissingType as ((path: string, missingType: string, nextTypeName: string) => Promise) | undefined} onCreateAndOpenNote={onCreateAndOpenNote} onInitializeProperties={onInitializeProperties} onToggleRawEditor={onToggleRawEditor} diff --git a/src/components/TypeSelector.tsx b/src/components/TypeSelector.tsx index cbb9f13a..f03d4d8b 100644 --- a/src/components/TypeSelector.tsx +++ b/src/components/TypeSelector.tsx @@ -118,7 +118,7 @@ function MissingTypeWarning({ onCreateMissingType, }: { missingTypeName: string - onCreateMissingType?: (typeName: string) => void | Promise + onCreateMissingType?: (typeName: string) => boolean | void | Promise }) { const [dialogOpen, setDialogOpen] = useState(false) const canCreateMissingType = Boolean(onCreateMissingType) @@ -148,9 +148,7 @@ function MissingTypeWarning({ setDialogOpen(false)} - onCreate={async (typeName) => { - await onCreateMissingType?.(typeName) - }} + onCreate={(typeName) => onCreateMissingType?.(typeName)} initialName={missingTypeName} /> )} @@ -165,7 +163,7 @@ function TypeRowValue({ }: { children: ReactNode missingTypeName?: string | null - onCreateMissingType?: (typeName: string) => void | Promise + onCreateMissingType?: (typeName: string) => boolean | void | Promise }) { return (
@@ -191,7 +189,7 @@ function ReadOnlyType({ customColorKey?: string | null onNavigate?: (target: string) => void missingTypeName?: string | null - onCreateMissingType?: (typeName: string) => void | Promise + onCreateMissingType?: (typeName: string) => boolean | void | Promise }) { if (!isA) return null return ( @@ -230,7 +228,7 @@ interface TypeSelectorProps { onUpdateProperty?: (key: string, value: FrontmatterValue) => void onNavigate?: (target: string) => void missingTypeName?: string | null - onCreateMissingType?: (typeName: string) => void | Promise + onCreateMissingType?: (typeName: string) => boolean | void | Promise } export function TypeSelector({ onUpdateProperty, ...props }: TypeSelectorProps) { diff --git a/src/components/inspector/useInspectorPropertyActions.ts b/src/components/inspector/useInspectorPropertyActions.ts index db33434b..3306cb90 100644 --- a/src/components/inspector/useInspectorPropertyActions.ts +++ b/src/components/inspector/useInspectorPropertyActions.ts @@ -8,7 +8,7 @@ interface InspectorPropertyActionsConfig { onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise onDeleteProperty?: (path: string, key: string) => Promise onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise - onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise + onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise } function bindEntryAction( @@ -21,7 +21,7 @@ function bindEntryAction( function bindMissingTypeAction( entry: VaultEntry | null, - action: ((path: string, missingType: string, nextTypeName: string) => Promise) | undefined, + action: ((path: string, missingType: string, nextTypeName: string) => Promise) | undefined, ) { const missingType = entry?.isA if (!entry || !missingType || !action) return undefined diff --git a/src/hooks/useNoteActions.hook.test.ts b/src/hooks/useNoteActions.hook.test.ts index 37ada99e..18d59379 100644 --- a/src/hooks/useNoteActions.hook.test.ts +++ b/src/hooks/useNoteActions.hook.test.ts @@ -70,18 +70,48 @@ describe('useNoteActions hook', () => { vi.useRealTimers() }) - it('handleCreateNote calls addEntry and creates correct entry', () => { - const { result } = renderHook(() => useNoteActions(makeConfig())) + function renderActions(entries: VaultEntry[] = []) { + return renderHook(() => useNoteActions(makeConfig(entries))) + } + + function createImmediateEntry(type?: string) { + vi.spyOn(Date, 'now').mockReturnValue(1700000000000) + const { result } = renderActions() + act(() => { + result.current.handleCreateNoteImmediate(type) + }) + const [createdEntry] = addEntry.mock.calls[0] + vi.restoreAllMocks() + return createdEntry as VaultEntry + } + + it.each([ + { + name: 'handleCreateNote', + run: (result: ReturnType['result']) => result.current.handleCreateNote('Test Note', 'Note'), + expectedTitle: 'Test Note', + expectedType: 'Note', + expectedPathFragment: 'test-note.md', + }, + { + name: 'handleCreateType', + run: (result: ReturnType['result']) => result.current.handleCreateType('Recipe'), + expectedTitle: 'Recipe', + expectedType: 'Type', + expectedPathFragment: 'recipe.md', + }, + ])('$name creates the expected entry', ({ run, expectedTitle, expectedType, expectedPathFragment }) => { + const { result } = renderActions() act(() => { - result.current.handleCreateNote('Test Note', 'Note') + run(result) }) expect(addEntry).toHaveBeenCalledTimes(1) const [createdEntry] = addEntry.mock.calls[0] - expect(createdEntry.title).toBe('Test Note') - expect(createdEntry.isA).toBe('Note') - expect(createdEntry.path).toContain('test-note.md') + expect(createdEntry.title).toBe(expectedTitle) + expect(createdEntry.isA).toBe(expectedType) + expect(createdEntry.path).toContain(expectedPathFragment) }) it('handleCreateNote opens tab immediately (before addEntry resolves)', () => { @@ -102,19 +132,6 @@ describe('useNoteActions hook', () => { expect(result.current.activeTabPath).toContain('fast-note.md') }) - it('handleCreateType creates type entry', () => { - const { result } = renderHook(() => useNoteActions(makeConfig())) - - act(() => { - result.current.handleCreateType('Recipe') - }) - - expect(addEntry).toHaveBeenCalledTimes(1) - const [createdEntry] = addEntry.mock.calls[0] - expect(createdEntry.isA).toBe('Type') - expect(createdEntry.title).toBe('Recipe') - }) - it('handleNavigateWikilink finds entry by title', async () => { const target = makeEntry({ title: 'Target Note', path: '/vault/target.md' }) @@ -178,19 +195,10 @@ describe('useNoteActions hook', () => { }) it('handleCreateNoteImmediate creates note with timestamp-based title', () => { - vi.spyOn(Date, 'now').mockReturnValue(1700000000000) - const { result } = renderHook(() => useNoteActions(makeConfig())) - - act(() => { - result.current.handleCreateNoteImmediate() - }) - - expect(addEntry).toHaveBeenCalledTimes(1) - const [createdEntry] = addEntry.mock.calls[0] + const createdEntry = createImmediateEntry() expect(createdEntry.title).toBe('Untitled Note 1700000000') expect(createdEntry.filename).toBe('untitled-note-1700000000.md') expect(createdEntry.isA).toBe('Note') - vi.restoreAllMocks() }) it('handleCreateNoteImmediate generates unique names on rapid calls via timestamp', () => { @@ -217,18 +225,9 @@ describe('useNoteActions hook', () => { }) it('handleCreateNoteImmediate accepts custom type', () => { - vi.spyOn(Date, 'now').mockReturnValue(1700000000000) - const { result } = renderHook(() => useNoteActions(makeConfig())) - - act(() => { - result.current.handleCreateNoteImmediate('Project') - }) - - expect(addEntry).toHaveBeenCalledTimes(1) - const [createdEntry] = addEntry.mock.calls[0] + const createdEntry = createImmediateEntry('Project') expect(createdEntry.filename).toMatch(/^untitled-project-\d+\.md$/) expect(createdEntry.isA).toBe('Project') - vi.restoreAllMocks() }) it('handleCreateNote uses default template for Project type', () => { @@ -435,7 +434,11 @@ describe('useNoteActions hook', () => { expect(addEntry).toHaveBeenCalledTimes(1) expect(removeEntry).toHaveBeenCalledWith(expect.stringContaining(pathFragment)) - expect(setToastMessage).toHaveBeenCalledWith('Failed to create note — disk write error') + expect(setToastMessage).toHaveBeenCalledWith( + type === 'Type' + ? 'Failed to create type — disk write error' + : 'Failed to create note — disk write error', + ) }) it('does not revert when disk write succeeds', async () => { diff --git a/src/hooks/useNoteCreation.test.ts b/src/hooks/useNoteCreation.test.ts index 37bd3ef6..10968e3e 100644 --- a/src/hooks/useNoteCreation.test.ts +++ b/src/hooks/useNoteCreation.test.ts @@ -348,6 +348,21 @@ describe('useNoteCreation hook', () => { expect(addEntry.mock.calls[0][0].title).toBe('Recipe') }) + it('handleCreateType blocks when a non-type file already uses the target filename', async () => { + const existing = makeEntry({ path: '/test/vault/briefing.md', filename: 'briefing.md', title: 'Briefing', isA: 'Note' }) + const { result } = renderHook(() => useNoteCreation(makeConfig([existing]), tabDeps)) + + let created = true + await act(async () => { + created = await result.current.handleCreateType('Briefing') + }) + + expect(created).toBe(false) + expect(addEntry).not.toHaveBeenCalled() + expect(openTabWithContent).not.toHaveBeenCalled() + expect(setToastMessage).toHaveBeenCalledWith('Cannot create type "Briefing" because briefing.md already exists') + }) + it('createTypeEntrySilent persists without opening tab', async () => { const { result } = renderHook(() => useNoteCreation(makeConfig(), tabDeps)) const entry = await act(async () => result.current.createTypeEntrySilent('Recipe')) @@ -356,6 +371,32 @@ describe('useNoteCreation hook', () => { expect(entry.isA).toBe('Type') }) + it('createTypeEntrySilent reuses an existing slug-equivalent type', async () => { + const existing = makeEntry({ path: '/test/vault/briefing.md', filename: 'briefing.md', title: 'Briefing', isA: 'Type' }) + const { result } = renderHook(() => useNoteCreation(makeConfig([existing]), tabDeps)) + + const entry = await act(async () => result.current.createTypeEntrySilent('briefing')) + + expect(entry).toBe(existing) + expect(addEntry).not.toHaveBeenCalled() + expect(openTabWithContent).not.toHaveBeenCalled() + }) + + it('handleCreateNoteForRelationship blocks when the generated filename already exists', async () => { + const existing = makeEntry({ path: '/test/vault/briefing.md', filename: 'briefing.md', title: 'Existing Briefing', isA: 'Note' }) + const { result } = renderHook(() => useNoteCreation(makeConfig([existing]), tabDeps)) + + let created = true + await act(async () => { + created = await result.current.handleCreateNoteForRelationship('Briefing') + }) + + expect(created).toBe(false) + expect(addEntry).not.toHaveBeenCalled() + expect(openTabWithContent).not.toHaveBeenCalled() + expect(setToastMessage).toHaveBeenCalledWith('Cannot create note "Briefing" because briefing.md already exists') + }) + it('reverts optimistic creation when disk write fails (Tauri)', async () => { vi.mocked(isTauri).mockReturnValue(true) vi.mocked(invoke).mockRejectedValueOnce(new Error('disk full')) diff --git a/src/hooks/useNoteCreation.ts b/src/hooks/useNoteCreation.ts index 8b0c5f88..b3434f27 100644 --- a/src/hooks/useNoteCreation.ts +++ b/src/hooks/useNoteCreation.ts @@ -132,10 +132,104 @@ export function resolveNewType({ typeName, vaultPath }: NewTypeParams): { entry: return { entry, content: `---\ntype: Type\n---\n` } } +type ResolvedEntry = { entry: VaultEntry; content: string } + +interface BlockedCreationPlan { + status: 'blocked' + message: string +} + +interface ReadyCreationPlan { + status: 'create' + resolved: ResolvedEntry +} + +interface ExistingTypeCreationPlan { + status: 'existing' + entry: VaultEntry +} + +export type NoteCreationPlan = BlockedCreationPlan | ReadyCreationPlan +export type TypeCreationPlan = BlockedCreationPlan | ExistingTypeCreationPlan | ReadyCreationPlan + +function normalizeComparablePath(path: string): string { + return path.replace(/\\/g, '/').toLocaleLowerCase() +} + +function findPathCollision(entries: VaultEntry[], path: string): VaultEntry | undefined { + const target = normalizeComparablePath(path) + return entries.find((entry) => normalizeComparablePath(entry.path) === target) +} + +function buildCreationCollisionMessage({ noun, title, path }: { noun: 'note' | 'type'; title: string; path: string }): string { + const filename = path.split('/').pop() ?? path + return `Cannot create ${noun} "${title}" because ${filename} already exists` +} + +function findEquivalentTypeEntry(entries: VaultEntry[], typeName: string): VaultEntry | undefined { + const trimmed = typeName.trim() + const targetSlug = slugify(trimmed) + return entries.find((entry) => + entry.isA === 'Type' && (entry.title === trimmed || slugify(entry.title) === targetSlug) + ) +} + +export function planNewNoteCreation({ + entries, + title, + type, + vaultPath, + template, +}: NewNoteParams & { entries: VaultEntry[] }): NoteCreationPlan { + const resolved = resolveNewNote({ title, type, vaultPath, template }) + const collision = findPathCollision(entries, resolved.entry.path) + if (collision) { + return { + status: 'blocked', + message: buildCreationCollisionMessage({ noun: 'note', title, path: resolved.entry.path }), + } + } + return { status: 'create', resolved } +} + +export function planNewTypeCreation({ + entries, + typeName, + vaultPath, +}: NewTypeParams & { entries: VaultEntry[] }): TypeCreationPlan { + const existingType = findEquivalentTypeEntry(entries, typeName) + if (existingType) return { status: 'existing', entry: existingType } + + const resolved = resolveNewType({ typeName, vaultPath }) + const collision = findPathCollision(entries, resolved.entry.path) + if (collision) { + return { + status: 'blocked', + message: buildCreationCollisionMessage({ noun: 'type', title: typeName, path: resolved.entry.path }), + } + } + return { status: 'create', resolved } +} + +function isAlreadyExistsError(error: unknown): boolean { + const message = error instanceof Error ? error.message : String(error) + return /already exists|file exists|eexist/i.test(message) +} + +function createPersistFailureMessage(entry: VaultEntry, error: unknown): string { + if (isAlreadyExistsError(error)) { + const noun = entry.isA === 'Type' ? 'type' : 'note' + return buildCreationCollisionMessage({ noun, title: entry.title, path: entry.path }) + } + return entry.isA === 'Type' + ? 'Failed to create type — disk write error' + : 'Failed to create note — disk write error' +} + /** Persist a newly created note to disk. Returns a Promise for error handling. */ export function persistNewNote(path: string, content: string): Promise { if (!isTauri()) return Promise.resolve() - return invoke('save_note_content', { path, content }).then(() => {}) + return invoke('create_note_content', { path, content }).then(() => {}) } // Rapid Cmd+N bursts can outpace the note-list render path on desktop. Keep @@ -156,33 +250,125 @@ function signalFocusEditor(opts?: { selectTitle?: boolean; path?: string }): voi } interface PersistCallbacks { - onFail: (p: string) => void onStart?: (p: string) => void onEnd?: (p: string) => void onPersisted?: () => void } -/** Persist to disk; track pending state via onStart/onEnd; revert on failure. */ -function persistOptimistic(path: string, content: string, cbs: PersistCallbacks): void { +/** Persist to disk; track pending state via onStart/onEnd. */ +async function persistOptimistic(path: string, content: string, cbs: PersistCallbacks): Promise { cbs.onStart?.(path) - persistNewNote(path, content) - .then(() => { cbs.onEnd?.(path); cbs.onPersisted?.() }) - .catch(() => { cbs.onEnd?.(path); cbs.onFail(path) }) + try { + await persistNewNote(path, content) + cbs.onPersisted?.() + } finally { + cbs.onEnd?.(path) + } } -type ResolvedNote = { entry: VaultEntry; content: string } -type PersistFn = (resolved: ResolvedNote) => void +interface PersistResolvedOptions { + openTab?: boolean +} -/** Optimistically open note, add entry to vault, and persist to disk. */ -function createAndPersist( - resolved: ResolvedNote, - addFn: (e: VaultEntry) => void, - openTab: (e: VaultEntry, c: string) => void, - cbs: PersistCallbacks, -): void { - openTab(resolved.entry, resolved.content) - addEntryWithMock(resolved.entry, resolved.content, addFn) - persistOptimistic(resolved.entry.path, resolved.content, cbs) +type PersistResolvedEntryFn = ( + resolved: ResolvedEntry, + options?: PersistResolvedOptions, +) => Promise + +interface CreationDeps { + entries: VaultEntry[] + vaultPath: string + setToastMessage: (msg: string | null) => void + persistResolvedEntry: PersistResolvedEntryFn +} + +interface NoteCreationRequest extends CreationDeps { + title: string + type: string + creationPath?: 'plus_button' +} + +async function createNamedNote({ + entries, + title, + type, + vaultPath, + setToastMessage, + persistResolvedEntry, + creationPath, +}: NoteCreationRequest): Promise { + const template = resolveTemplate({ entries, typeName: type }) + const plan = planNewNoteCreation({ entries, title, type, vaultPath, template }) + if (plan.status === 'blocked') { + setToastMessage(plan.message) + return false + } + + try { + await persistResolvedEntry(plan.resolved) + if (creationPath) { + trackEvent('note_created', { has_type: type !== 'Note' ? 1 : 0, creation_path: creationPath }) + } + return true + } catch (error) { + setToastMessage(createPersistFailureMessage(plan.resolved.entry, error)) + return false + } +} + +interface TypeCreationRequest extends CreationDeps { + typeName: string +} + +async function createTypeFromName({ + entries, + typeName, + vaultPath, + setToastMessage, + persistResolvedEntry, +}: TypeCreationRequest): Promise { + const plan = planNewTypeCreation({ entries, typeName, vaultPath }) + if (plan.status === 'existing') { + setToastMessage(`Type "${plan.entry.title}" already exists`) + return false + } + if (plan.status === 'blocked') { + setToastMessage(plan.message) + return false + } + + try { + await persistResolvedEntry(plan.resolved) + trackEvent('type_created') + return true + } catch (error) { + setToastMessage(createPersistFailureMessage(plan.resolved.entry, error)) + return false + } +} + +async function createTypeSilently({ + entries, + typeName, + vaultPath, + setToastMessage, + persistResolvedEntry, +}: TypeCreationRequest): Promise { + const plan = planNewTypeCreation({ entries, typeName, vaultPath }) + if (plan.status === 'existing') return plan.entry + if (plan.status === 'blocked') { + setToastMessage(plan.message) + throw new Error(plan.message) + } + + try { + await persistResolvedEntry(plan.resolved, { openTab: false }) + return plan.resolved.entry + } catch (error) { + const message = createPersistFailureMessage(plan.resolved.entry, error) + setToastMessage(message) + throw new Error(message) + } } interface ImmediateCreateDeps { @@ -318,30 +504,6 @@ function useImmediateCreateQueue(config: ImmediateCreateQueueConfig): (type?: st }, [syncDeps, executeRequest, scheduleQueuedBurst]) } -interface RelationshipCreateDeps { - entries: VaultEntry[] - vaultPath: string - openTabWithContent: (entry: VaultEntry, content: string) => void - addEntry: (entry: VaultEntry) => void - removeEntry: (path: string) => void - setToastMessage: (msg: string | null) => void - onNewNotePersisted?: () => void -} - -/** Create a note for a relationship link; persist in background. */ -function createNoteForRelationship(deps: RelationshipCreateDeps, title: string): void { - const template = resolveTemplate({ entries: deps.entries, typeName: 'Note' }) - const resolved = resolveNewNote({ title, type: 'Note', vaultPath: deps.vaultPath, template }) - deps.openTabWithContent(resolved.entry, resolved.content) - addEntryWithMock(resolved.entry, resolved.content, deps.addEntry) - persistNewNote(resolved.entry.path, resolved.content) - .then(() => deps.onNewNotePersisted?.()) - .catch(() => { - deps.removeEntry(resolved.entry.path) - deps.setToastMessage('Failed to create note — disk write error') - }) -} - export interface NoteCreationConfig { addEntry: (entry: VaultEntry) => void removeEntry: (path: string) => void @@ -362,60 +524,59 @@ interface CreationTabDeps { } export function useNoteCreation(config: NoteCreationConfig, tabDeps: CreationTabDeps) { - const { addEntry, removeEntry, entries, setToastMessage, addPendingSave, removePendingSave } = config + const { addEntry, removeEntry, entries, setToastMessage, addPendingSave, removePendingSave, vaultPath } = config const { openTabWithContent } = tabDeps - const revertOptimisticNote = useCallback((path: string) => { - removeEntry(path) - setToastMessage('Failed to create note — disk write error') - }, [removeEntry, setToastMessage]) + const persistResolvedEntry = useCallback(async ( + resolved: ResolvedEntry, + options?: PersistResolvedOptions, + ): Promise => { + if (options?.openTab !== false) openTabWithContent(resolved.entry, resolved.content) + addEntryWithMock(resolved.entry, resolved.content, addEntry) + try { + await persistOptimistic(resolved.entry.path, resolved.content, { + onStart: addPendingSave, + onEnd: removePendingSave, + onPersisted: config.onNewNotePersisted, + }) + } catch (error) { + removeEntry(resolved.entry.path) + throw error + } + }, [openTabWithContent, addEntry, addPendingSave, removePendingSave, config.onNewNotePersisted, removeEntry]) - const persistNew: PersistFn = useCallback( - (resolved) => createAndPersist(resolved, addEntry, openTabWithContent, { - onFail: revertOptimisticNote, - onStart: addPendingSave, - onEnd: removePendingSave, - onPersisted: config.onNewNotePersisted, - }), - [openTabWithContent, addEntry, revertOptimisticNote, addPendingSave, removePendingSave, config.onNewNotePersisted], - ) + const creationDeps = { + entries, + vaultPath, + setToastMessage, + persistResolvedEntry, + } + + const handleCreateNote = useCallback((title: string, type: string): Promise => + createNamedNote({ ...creationDeps, title, type, creationPath: 'plus_button' }), + [creationDeps]) + + const handleCreateType = useCallback((typeName: string): Promise => + createTypeFromName({ ...creationDeps, typeName }), + [creationDeps]) + + const createTypeEntrySilent = useCallback((typeName: string): Promise => + createTypeSilently({ ...creationDeps, typeName }), + [creationDeps]) + + const handleCreateNoteForRelationship = useCallback((title: string): Promise => + createNamedNote({ ...creationDeps, title, type: 'Note' }), + [creationDeps]) const handleCreateNoteImmediate = useImmediateCreateQueue({ entries, - vaultPath: config.vaultPath, + vaultPath, addEntry, openTabWithContent, trackUnsaved: config.trackUnsaved, markContentPending: config.markContentPending, }) - const handleCreateNote = useCallback((title: string, type: string) => { - const template = resolveTemplate({ entries, typeName: type }) - persistNew(resolveNewNote({ title, type, vaultPath: config.vaultPath, template })) - trackEvent('note_created', { has_type: type !== 'Note' ? 1 : 0, creation_path: 'plus_button' }) - }, [entries, persistNew, config.vaultPath]) - - const handleCreateNoteForRelationship = useCallback((title: string): Promise => { - createNoteForRelationship({ - entries, vaultPath: config.vaultPath, openTabWithContent, addEntry, - removeEntry, setToastMessage, onNewNotePersisted: config.onNewNotePersisted, - }, title) - return Promise.resolve(true) - }, [entries, openTabWithContent, addEntry, removeEntry, setToastMessage, config.vaultPath, config.onNewNotePersisted]) - - const handleCreateType = useCallback((typeName: string) => { - persistNew(resolveNewType({ typeName, vaultPath: config.vaultPath })) - trackEvent('type_created') - }, [persistNew, config.vaultPath]) - - /** Create a Type entry file silently (no tab opened). Adds to state and persists to disk. */ - const createTypeEntrySilent = useCallback(async (typeName: string): Promise => { - const resolved = resolveNewType({ typeName, vaultPath: config.vaultPath }) - addEntryWithMock(resolved.entry, resolved.content, addEntry) - await persistNewNote(resolved.entry.path, resolved.content) - return resolved.entry - }, [addEntry, config.vaultPath]) - return { handleCreateNote, handleCreateNoteImmediate, diff --git a/tests/smoke/collision-create-flows.spec.ts b/tests/smoke/collision-create-flows.spec.ts new file mode 100644 index 00000000..632dd9f0 --- /dev/null +++ b/tests/smoke/collision-create-flows.spec.ts @@ -0,0 +1,85 @@ +import { test, expect } from '@playwright/test' +import fs from 'fs' +import path from 'path' +import { createFixtureVaultCopy, openFixtureVaultDesktopHarness, removeFixtureVaultCopy } from '../helpers/fixtureVault' +import { sendShortcut } from './helpers' + +let tempVaultDir: string + +function writeFixtureNote(vaultPath: string, filename: string, content: string): string { + const notePath = path.join(vaultPath, filename) + fs.writeFileSync(notePath, content) + return notePath +} + +function toast(page: import('@playwright/test').Page) { + return page.locator('.fixed.bottom-8') +} + +test.describe('Collision-safe create flows', () => { + test.beforeEach(async ({ page }) => { + tempVaultDir = createFixtureVaultCopy() + await page.setViewportSize({ width: 1600, height: 900 }) + }) + + test.afterEach(() => { + removeFixtureVaultCopy(tempVaultDir) + }) + + test('missing-type creation keeps the dialog open when a root filename already exists', async ({ page }) => { + const collidingPath = writeFixtureNote( + tempVaultDir, + 'hotel.md', + '---\ntype: Note\n---\n# Existing Hotel Note\n', + ) + writeFixtureNote( + tempVaultDir, + 'hotel-guide.md', + '---\ntype: Hotel\nstatus: Active\n---\n# Hotel Guide\n', + ) + + await openFixtureVaultDesktopHarness(page, tempVaultDir) + await page.getByText('Hotel Guide', { exact: true }).click() + await sendShortcut(page, 'i', ['Control', 'Shift']) + + const warning = page.getByTestId('missing-type-warning') + await expect(warning).toBeVisible() + await warning.focus() + await page.keyboard.press('Enter') + + const dialog = page.getByRole('dialog') + await expect(dialog).toBeVisible() + await expect(dialog.getByPlaceholder('e.g. Recipe, Book, Habit...')).toHaveValue('Hotel') + await page.keyboard.press('Enter') + + await expect(dialog).toBeVisible() + await expect(toast(page)).toContainText('Cannot create type "Hotel" because hotel.md already exists') + expect(fs.readFileSync(collidingPath, 'utf8')).toContain('# Existing Hotel Note') + }) + + test('relationship create-and-open keeps the inline input active on filename collision', async ({ page }) => { + const collidingPath = writeFixtureNote( + tempVaultDir, + 'briefing.md', + '---\ntype: Note\n---\n# Weekly Sync\n', + ) + + await openFixtureVaultDesktopHarness(page, tempVaultDir) + await page.getByText('Team Meeting', { exact: true }).click() + await sendShortcut(page, 'i', ['Control', 'Shift']) + + const addButton = page.getByTestId('add-relation-ref').first() + await expect(addButton).toBeVisible() + await addButton.click() + const input = page.getByTestId('add-relation-ref-input') + await expect(input).toBeVisible() + await input.fill('Briefing!') + await page.waitForTimeout(300) + + await page.getByTestId('create-and-open-option').click() + + await expect(input).toBeVisible() + await expect(toast(page)).toContainText('Cannot create note "Briefing!" because briefing.md already exists') + expect(fs.readFileSync(collidingPath, 'utf8')).toContain('# Weekly Sync') + }) +})