From f0e32cfd5fec0526eb32a494f02911b853918eb2 Mon Sep 17 00:00:00 2001 From: Elias Stiffel Date: Sat, 23 May 2026 08:03:47 +0200 Subject: [PATCH 1/8] fix: nest new folders under the selected parent The Create Folder action ignored the sidebar selection and always wrote to the vault root. Now it threads the selected folder as parent_path to the backend, and expands the parent in the tree so the new child is visible without re-clicking. Fixes: https://github.com/refactoringhq/tolaria/issues/728 Co-Authored-By: Claude Opus 4.7 (1M context) --- src-tauri/src/commands/vault.rs | 49 ++++++- src-tauri/src/commands/vault/file_cmds.rs | 16 +- src/App.tsx | 12 +- src/components/FolderTree.test.tsx | 138 ++++++++++++++++++ src/components/FolderTree.tsx | 23 ++- src/components/Sidebar.tsx | 4 +- .../folder-tree/useFolderTreeDisclosure.ts | 11 +- src/types.ts | 10 ++ 8 files changed, 246 insertions(+), 17 deletions(-) diff --git a/src-tauri/src/commands/vault.rs b/src-tauri/src/commands/vault.rs index 3a5335c1..6dca1fd9 100644 --- a/src-tauri/src/commands/vault.rs +++ b/src-tauri/src/commands/vault.rs @@ -192,7 +192,7 @@ mod tests { fn test_create_vault_folder_rejects_escape_path() { let dir = tempfile::TempDir::new().unwrap(); - let err = create_vault_folder(dir.path().into(), "../escape".into()) + let err = create_vault_folder(dir.path().into(), "../escape".into(), None) .expect_err("expected escaping folder path to be rejected"); assert_eq!(err, ACTIVE_VAULT_PATH_ERROR); @@ -202,12 +202,57 @@ mod tests { fn test_create_vault_folder_rejects_windows_invalid_names() { let dir = tempfile::TempDir::new().unwrap(); - let err = create_vault_folder(dir.path().into(), "con".into()) + let err = create_vault_folder(dir.path().into(), "con".into(), None) .expect_err("expected Windows-invalid folder name to be rejected"); assert_eq!(err, "Invalid folder name"); } + #[test] + fn test_create_vault_folder_nests_inside_parent_path() { + let dir = tempfile::TempDir::new().unwrap(); + std::fs::create_dir_all(dir.path().join("Projects")).unwrap(); + + let name = create_vault_folder( + dir.path().into(), + "Laputa".into(), + Some(std::path::PathBuf::from("Projects")), + ) + .expect("expected nested folder to be created"); + + assert_eq!(name, "Laputa"); + assert!(dir.path().join("Projects").join("Laputa").is_dir()); + } + + #[test] + fn test_create_vault_folder_rejects_escape_via_parent_path() { + let dir = tempfile::TempDir::new().unwrap(); + + let err = create_vault_folder( + dir.path().into(), + "Laputa".into(), + Some(std::path::PathBuf::from("../escape")), + ) + .expect_err("expected escaping parent path to be rejected"); + + assert_eq!(err, ACTIVE_VAULT_PATH_ERROR); + } + + #[test] + fn test_create_vault_folder_treats_empty_parent_as_root() { + let dir = tempfile::TempDir::new().unwrap(); + + let name = create_vault_folder( + dir.path().into(), + "Inbox".into(), + Some(std::path::PathBuf::from("")), + ) + .expect("expected empty parent to fall back to vault root"); + + assert_eq!(name, "Inbox"); + assert!(dir.path().join("Inbox").is_dir()); + } + #[test] fn test_save_view_cmd_rejects_nested_filename() { assert_save_view_cmd_rejects_invalid_filename("../escape.yml"); diff --git a/src-tauri/src/commands/vault/file_cmds.rs b/src-tauri/src/commands/vault/file_cmds.rs index a8cdf906..29dd8073 100644 --- a/src-tauri/src/commands/vault/file_cmds.rs +++ b/src-tauri/src/commands/vault/file_cmds.rs @@ -191,11 +191,19 @@ pub fn batch_delete_notes(paths: Vec) -> Result, String> { } #[tauri::command] -pub fn create_vault_folder(vault_path: PathBuf, folder_name: PathBuf) -> Result { +pub fn create_vault_folder( + vault_path: PathBuf, + folder_name: PathBuf, + parent_path: Option, +) -> Result { let raw_vault_path = vault_path.to_string_lossy(); with_boundary(Some(raw_vault_path.as_ref()), |boundary| { let folder_name = folder_name.to_string_lossy(); - let folder_path = boundary.child_path(folder_name.as_ref())?; + let relative_path = match parent_path.as_deref() { + Some(parent) if !parent.as_os_str().is_empty() => parent.join(folder_name.as_ref()), + _ => PathBuf::from(folder_name.as_ref()), + }; + let folder_path = boundary.child_path(&relative_path.to_string_lossy())?; validate_folder_name(folder_name.as_ref())?; ensure_missing_folder(&folder_path, folder_name.as_ref())?; std::fs::create_dir_all(&folder_path) @@ -370,7 +378,7 @@ mod tests { fs::write(dir.path().join("root.md"), "# Root\n").unwrap(); assert_eq!( - create_vault_folder(root.clone(), PathBuf::from("Projects")).unwrap(), + create_vault_folder(root.clone(), PathBuf::from("Projects"), None).unwrap(), "Projects" ); fs::write(dir.path().join("Projects/project.md"), "# Project\n").unwrap(); @@ -394,7 +402,7 @@ mod tests { assert!(error.contains("Path must stay inside the active vault")); let folder_error = - create_vault_folder(vault.path().to_path_buf(), PathBuf::from("../escape")) + create_vault_folder(vault.path().to_path_buf(), PathBuf::from("../escape"), None) .unwrap_err(); assert!(folder_error.contains("Path must stay inside the active vault")); } diff --git a/src/App.tsx b/src/App.tsx index 8496592b..3b011191 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -877,12 +877,18 @@ function App() { }) }, [updateConfig, vaultConfig.inbox]) - const handleCreateFolder = useCallback(async (name: string) => { + const handleCreateFolder = useCallback(async ( + name: string, + parent?: { path: string; rootPath?: string }, + ) => { try { + const vaultPath = parent?.rootPath?.trim() ? parent.rootPath : resolvedPath + const parentPath = parent?.path && parent.path.length > 0 ? parent.path : null + const args = { vaultPath, folderName: name, parentPath } if (isTauri()) { - await invoke('create_vault_folder', { vaultPath: resolvedPath, folderName: name }) + await invoke('create_vault_folder', args) } else { - await mockInvoke('create_vault_folder', { vaultPath: resolvedPath, folderName: name }) + await mockInvoke('create_vault_folder', args) } await vault.reloadFolders() setToastMessage(`Created folder "${name}"`) diff --git a/src/components/FolderTree.test.tsx b/src/components/FolderTree.test.tsx index f7bb42fb..fbaf873a 100644 --- a/src/components/FolderTree.test.tsx +++ b/src/components/FolderTree.test.tsx @@ -235,6 +235,144 @@ describe('FolderTree', () => { expect(screen.getByTestId('new-folder-input')).toBeInTheDocument() }) + it('passes the selected folder as the parent when creating a new folder', async () => { + const onCreateFolder = vi.fn().mockResolvedValue(true) + render( + , + ) + + fireEvent.click(screen.getByTestId('create-folder-btn')) + const input = screen.getByTestId('new-folder-input') + fireEvent.change(input, { target: { value: 'laputa' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await vi.waitFor(() => { + expect(onCreateFolder).toHaveBeenCalledWith('laputa', { + path: 'projects', + rootPath: vaultRootPath, + }) + }) + }) + + it('passes the target vault root when the vault root is selected', async () => { + const onCreateFolder = vi.fn().mockResolvedValue(true) + const otherVault = '/Users/luca/Team' + const folders: FolderNode[] = [ + { + name: 'Personal', + path: '', + rootPath: vaultRootPath, + children: [{ name: 'areas', path: 'areas', rootPath: vaultRootPath, children: [] }], + }, + { + name: 'Team', + path: '', + rootPath: otherVault, + children: [{ name: 'areas', path: 'areas', rootPath: otherVault, children: [] }], + }, + ] + + render( + , + ) + + fireEvent.click(screen.getByTestId('create-folder-btn')) + const input = screen.getByTestId('new-folder-input') + fireEvent.change(input, { target: { value: 'inbox' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await vi.waitFor(() => { + expect(onCreateFolder).toHaveBeenCalledWith('inbox', { + path: '', + rootPath: otherVault, + }) + }) + }) + + it('omits parent context when nothing folder-like is selected', async () => { + const onCreateFolder = vi.fn().mockResolvedValue(true) + render( + , + ) + + fireEvent.click(screen.getByTestId('create-folder-btn')) + const input = screen.getByTestId('new-folder-input') + fireEvent.change(input, { target: { value: 'inbox' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await vi.waitFor(() => { + expect(onCreateFolder).toHaveBeenCalledWith('inbox', undefined) + }) + }) + + it('expands the selected parent after a successful create so the new child is visible', async () => { + const onCreateFolder = vi.fn().mockResolvedValue(true) + render( + , + ) + + // Sanity: selecting a folder auto-expands its ancestors but not the folder + // itself, so 'projects' starts collapsed. + expect(screen.getByRole('button', { name: 'projects' })).toHaveAttribute('aria-expanded', 'false') + + fireEvent.click(screen.getByTestId('create-folder-btn')) + const input = screen.getByTestId('new-folder-input') + fireEvent.change(input, { target: { value: 'laputa' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await vi.waitFor(() => { + expect(onCreateFolder).toHaveBeenCalled() + }) + await vi.waitFor(() => { + expect(screen.getByRole('button', { name: 'projects' })).toHaveAttribute('aria-expanded', 'true') + }) + }) + + it('leaves expansion alone when create resolves falsy (validation rejected, etc.)', async () => { + const onCreateFolder = vi.fn().mockResolvedValue(false) + render( + , + ) + + fireEvent.click(screen.getByTestId('create-folder-btn')) + const input = screen.getByTestId('new-folder-input') + fireEvent.change(input, { target: { value: 'laputa' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await vi.waitFor(() => { + expect(onCreateFolder).toHaveBeenCalled() + }) + expect(screen.getByRole('button', { name: 'projects' })).toHaveAttribute('aria-expanded', 'false') + }) + it('starts rename on folder double-click', () => { const onStartRenameFolder = vi.fn() render( diff --git a/src/components/FolderTree.tsx b/src/components/FolderTree.tsx index 9035bcb8..49d4f566 100644 --- a/src/components/FolderTree.tsx +++ b/src/components/FolderTree.tsx @@ -8,7 +8,7 @@ import { Plus, } from '@phosphor-icons/react' import { Button } from '@/components/ui/button' -import type { FolderNode, SidebarSelection } from '../types' +import type { FolderCreationParent, FolderNode, SidebarSelection } from '../types' import { FolderContextMenu } from './folder-tree/FolderContextMenu' import { FolderNameInput } from './folder-tree/FolderNameInput' import { FolderTreeRow } from './folder-tree/FolderTreeRow' @@ -23,7 +23,7 @@ interface FolderTreeProps { folders: FolderNode[] selection: SidebarSelection onSelect: (selection: SidebarSelection) => void - onCreateFolder?: (name: string) => Promise | boolean + onCreateFolder?: (name: string, parent?: FolderCreationParent) => Promise | boolean onRenameFolder?: (folderPath: string, nextName: string) => Promise | boolean onDeleteFolder?: (folderPath: string) => void folderFileActions?: FolderFileActions @@ -113,6 +113,7 @@ export const FolderTree = memo(function FolderTree({ const { closeCreateForm, expanded, + expandFolder, handleToggleSection, isCreating, openCreateForm, @@ -146,10 +147,22 @@ export const FolderTree = memo(function FolderTree({ return true } - const created = await onCreateFolder(nextName) - if (created) closeCreateForm() + const parent: FolderCreationParent | undefined = selection.kind === 'folder' + ? { path: selection.path, rootPath: selection.rootPath } + : undefined + const created = await onCreateFolder(nextName, parent) + if (created) { + closeCreateForm() + // Ensure the parent folder is open so the freshly created child is visible. + // The selection→ancestor-expansion machinery deliberately doesn't expand the + // selected node itself, so we expand it explicitly here. Vault-root keys are + // open by default and don't need this. + if (parent && parent.path) { + expandFolder(folderNodeKey(parent)) + } + } return created - }, [closeCreateForm, onCreateFolder]) + }, [closeCreateForm, expandFolder, onCreateFolder, selection]) const handleCreateFolderClick = useCallback(() => { closeContextMenu() diff --git a/src/components/Sidebar.tsx b/src/components/Sidebar.tsx index b22c5f5e..813d4311 100644 --- a/src/components/Sidebar.tsx +++ b/src/components/Sidebar.tsx @@ -1,6 +1,6 @@ import { useCallback, memo } from 'react' import type { - VaultEntry, FolderNode, SidebarSelection, ViewDefinition, ViewFile, + FolderCreationParent, FolderNode, SidebarSelection, VaultEntry, ViewDefinition, ViewFile, } from '../types' import { KeyboardSensor, PointerSensor, useSensor, useSensors, type DragEndEvent, @@ -55,7 +55,7 @@ interface SidebarProps { onUpdateViewDefinition?: (filename: string, patch: Partial, rootPath?: string) => void onReorderViews?: (orderedFilenames: string[]) => void folders?: FolderNode[] - onCreateFolder?: (name: string) => Promise | boolean + onCreateFolder?: (name: string, parent?: FolderCreationParent) => Promise | boolean onRenameFolder?: (folderPath: string, nextName: string) => Promise | boolean onDeleteFolder?: (folderPath: string) => void folderFileActions?: FolderFileActions diff --git a/src/components/folder-tree/useFolderTreeDisclosure.ts b/src/components/folder-tree/useFolderTreeDisclosure.ts index 923fe03c..e2d244bd 100644 --- a/src/components/folder-tree/useFolderTreeDisclosure.ts +++ b/src/components/folder-tree/useFolderTreeDisclosure.ts @@ -35,8 +35,16 @@ function useExpandedFolders(selection: SidebarSelection, renamingFolderPath?: st }) }, []) + const expandFolder = useCallback((key: string) => { + setManualExpanded((current) => { + if (Reflect.get(current, key) === true) return current + return { ...current, [key]: true } + }) + }, []) + return { expanded, + expandFolder, toggleFolder, } } @@ -85,7 +93,7 @@ export function useFolderTreeDisclosure({ renamingFolderPath, selection, }: UseFolderTreeDisclosureInput) { - const { expanded, toggleFolder } = useExpandedFolders(selection, renamingFolderPath) + const { expanded, expandFolder, toggleFolder } = useExpandedFolders(selection, renamingFolderPath) const { closeCreateForm, handleToggleSection, @@ -97,6 +105,7 @@ export function useFolderTreeDisclosure({ return { closeCreateForm, expanded, + expandFolder, handleToggleSection, isCreating, openCreateForm, diff --git a/src/types.ts b/src/types.ts index 1b6f6882..874fd049 100644 --- a/src/types.ts +++ b/src/types.ts @@ -276,3 +276,13 @@ export interface FolderNode { rootPath?: string children: FolderNode[] } + +/** + * Context for a folder-create request: where the new folder should land. + * `path` is vault-relative (`''` means vault root); `rootPath` identifies the + * target vault when multiple workspaces are mounted. + */ +export interface FolderCreationParent { + path: string + rootPath?: string +} From 5dd3b98fec99836b5563019f6918580edd18c8a9 Mon Sep 17 00:00:00 2001 From: Elias Stiffel Date: Sat, 23 May 2026 13:29:19 +0200 Subject: [PATCH 2/8] fix: avoid duplicate H1 when template begins with one `buildNoteBody` unconditionally prepended an empty `# ` placeholder whenever `initialEmptyHeading` was true, which collided with templates whose first line is already an H1 (e.g. a Weekly Notes type whose template starts with `# Woche 2026.21`). The placeholder is now suppressed when the template's first non-whitespace line is `# `. Fixes: https://github.com/refactoringhq/tolaria/issues/736 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/hooks/useNoteCreation.test.ts | 22 ++++++++++++++++++++++ src/hooks/useNoteCreation.ts | 3 ++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/hooks/useNoteCreation.test.ts b/src/hooks/useNoteCreation.test.ts index 1616b948..5994ebee 100644 --- a/src/hooks/useNoteCreation.test.ts +++ b/src/hooks/useNoteCreation.test.ts @@ -148,6 +148,28 @@ describe('buildNoteContent', () => { }) expect(content).toBe('---\ntype: Project\nstatus: Active\n---\n\n# \n\n## Objective\n\n') }) + + it('skips the empty H1 when the template already starts with one', () => { + const content = buildNoteContent({ + title: null, + type: 'Weekly', + status: null, + template: '# Woche 2026.21\n\nWochennotiz\n', + initialEmptyHeading: true, + }) + expect(content).toBe('---\ntype: Weekly\n---\n\n# Woche 2026.21\n\nWochennotiz\n') + }) + + it('skips the empty H1 when the template starts with an H1 after leading whitespace', () => { + const content = buildNoteContent({ + title: null, + type: 'Weekly', + status: null, + template: '\n\n# Woche 2026.21\n', + initialEmptyHeading: true, + }) + expect(content).toBe('---\ntype: Weekly\n---\n\n\n\n# Woche 2026.21\n') + }) }) describe('resolveNewNote', () => { diff --git a/src/hooks/useNoteCreation.ts b/src/hooks/useNoteCreation.ts index be023ff7..07ced057 100644 --- a/src/hooks/useNoteCreation.ts +++ b/src/hooks/useNoteCreation.ts @@ -117,7 +117,8 @@ export interface TypeInstanceDefault { } function buildNoteBody({ template, initialEmptyHeading }: Pick): string { - if (initialEmptyHeading) { + const templateStartsWithH1 = template?.trimStart().startsWith('# ') ?? false + if (initialEmptyHeading && !templateStartsWithH1) { return template ? `\n# \n\n${template}` : '\n# \n\n' } return template ? `\n${template}` : '' From d337e2204ab3016de6444184e572ae5e6355758f Mon Sep 17 00:00:00 2001 From: Elias Stiffel Date: Sat, 23 May 2026 18:10:34 +0200 Subject: [PATCH 3/8] fix(lint): use optional chain in FolderTree create handler --- src/components/FolderTree.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/FolderTree.tsx b/src/components/FolderTree.tsx index 49d4f566..b35b89c9 100644 --- a/src/components/FolderTree.tsx +++ b/src/components/FolderTree.tsx @@ -157,7 +157,7 @@ export const FolderTree = memo(function FolderTree({ // The selection→ancestor-expansion machinery deliberately doesn't expand the // selected node itself, so we expand it explicitly here. Vault-root keys are // open by default and don't need this. - if (parent && parent.path) { + if (parent?.path) { expandFolder(folderNodeKey(parent)) } } From da3e9b98651514e0922620eb0cbf18ebf11b1bd1 Mon Sep 17 00:00:00 2001 From: Elias Stiffel Date: Sat, 23 May 2026 18:43:24 +0200 Subject: [PATCH 4/8] fix: switch active vault when unmounting it via the vault switcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of the fix for #735 — top-level folder creation invisible in multi-workspace mode. The primary route, where the create-folder flow ignored the sidebar selection and always wrote to the legacy `resolvedPath`, is fixed in #728 (selection-aware routing in `handleCreateFolder` and `FolderTree`). This commit fixes the complementary state-machine bug: the vault switcher could leave `vaultPath` pointing at an unmounted vault, so any consumer that still reads `vaultPath` would operate on a vault the user no longer sees. The vault switcher tracks two related paths: - `defaultWorkspacePath` — the workspace the user is currently "on" (used as the destination for new notes, the graph default, etc.). - `vaultPath` — the active vault the switcher tracks; read by the file watcher, folder/view reloaders, and the active-vault fallback in the folder loader. Unchecking a vault in the switcher previously re-routed only `defaultWorkspacePath` to another mounted vault, leaving `vaultPath` pointing at the just-unmounted vault. The two paths drifted apart and every downstream consumer that read `vaultPath` operated on stale state — for example the folder loader's active-vault fallback (when enabled) would silently re-introduce the unmounted vault into the sidebar, and the file watcher would keep listening to it. `applyMountedChange` now also calls `onSwitchVault(nextPath)` when the unmounted vault is the active one, mirroring the existing reroute for `defaultWorkspacePath`. After the change `vaultPath` and mounted state stay consistent for every consumer. `applyMountedChange` is extracted from `VaultMenu.tsx` to a sibling helper module (`vaultMenuMountedChange.ts`) so it can be unit-tested directly without tripping `react-refresh/only-export-components`. Tests: - New `VaultMenu.applyMountedChange.test.ts` covers the four reroute permutations (default+active, active only, default only, neither), the bail-out when no alternative mounted vault exists, and the remount no-op. Fixes: https://github.com/refactoringhq/tolaria/issues/735 Related: https://github.com/refactoringhq/tolaria/pull/728 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../VaultMenu.applyMountedChange.test.ts | 109 ++++++++++++++++++ src/components/status-bar/VaultMenu.tsx | 36 ++---- .../status-bar/vaultMenuMountedChange.ts | 35 ++++++ 3 files changed, 152 insertions(+), 28 deletions(-) create mode 100644 src/components/status-bar/VaultMenu.applyMountedChange.test.ts create mode 100644 src/components/status-bar/vaultMenuMountedChange.ts diff --git a/src/components/status-bar/VaultMenu.applyMountedChange.test.ts b/src/components/status-bar/VaultMenu.applyMountedChange.test.ts new file mode 100644 index 00000000..d4d8db77 --- /dev/null +++ b/src/components/status-bar/VaultMenu.applyMountedChange.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it, vi } from 'vitest' +import { applyMountedChange } from './vaultMenuMountedChange' +import type { VaultOption } from './types' + +function vault(path: string, mounted = true): VaultOption { + return { label: path, path, alias: path.replace('/', ''), mounted, available: true } +} + +interface ChangeOverrides { + defaultPath?: string + vaultPath?: string + path?: string + mounted?: boolean + includedVaults?: VaultOption[] +} + +function callApply(overrides: ChangeOverrides) { + const onSetDefaultWorkspace = vi.fn() + const onSwitchVault = vi.fn() + const onUpdateWorkspaceIdentity = vi.fn() + applyMountedChange({ + defaultPath: overrides.defaultPath ?? '/a', + vaultPath: overrides.vaultPath ?? '/a', + includedVaults: overrides.includedVaults ?? [vault('/a'), vault('/b')], + mounted: overrides.mounted ?? false, + onSetDefaultWorkspace, + onSwitchVault, + onUpdateWorkspaceIdentity, + path: overrides.path ?? '/a', + }) + return { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } +} + +describe('applyMountedChange', () => { + it('reroutes both the default workspace and the active vault when the unmounted vault is both', () => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ + defaultPath: '/a', + vaultPath: '/a', + path: '/a', + }) + + expect(onSetDefaultWorkspace).toHaveBeenCalledWith('/b') + expect(onSwitchVault).toHaveBeenCalledWith('/b') + expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/a', { mounted: false }) + }) + + it('reroutes the active vault when only the active vault is unmounted (default workspace differs)', () => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ + defaultPath: '/b', + vaultPath: '/a', + path: '/a', + }) + + expect(onSetDefaultWorkspace).not.toHaveBeenCalled() + expect(onSwitchVault).toHaveBeenCalledWith('/b') + expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/a', { mounted: false }) + }) + + it('reroutes the default workspace when only the default is unmounted (active vault differs)', () => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ + defaultPath: '/a', + vaultPath: '/b', + path: '/a', + }) + + expect(onSetDefaultWorkspace).toHaveBeenCalledWith('/b') + expect(onSwitchVault).not.toHaveBeenCalled() + expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/a', { mounted: false }) + }) + + it('does not reroute when the unmounted vault is neither default nor active', () => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ + defaultPath: '/a', + vaultPath: '/a', + path: '/b', + includedVaults: [vault('/a'), vault('/b'), vault('/c')], + }) + + expect(onSetDefaultWorkspace).not.toHaveBeenCalled() + expect(onSwitchVault).not.toHaveBeenCalled() + expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/b', { mounted: false }) + }) + + it('bails out without changing anything when no alternative mounted vault exists', () => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ + defaultPath: '/a', + vaultPath: '/a', + path: '/a', + includedVaults: [vault('/a')], + }) + + expect(onSetDefaultWorkspace).not.toHaveBeenCalled() + expect(onSwitchVault).not.toHaveBeenCalled() + expect(onUpdateWorkspaceIdentity).not.toHaveBeenCalled() + }) + + it('only marks the vault unmounted (no reroute) when remounting', () => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ + defaultPath: '/a', + vaultPath: '/a', + path: '/b', + mounted: true, + }) + + expect(onSetDefaultWorkspace).not.toHaveBeenCalled() + expect(onSwitchVault).not.toHaveBeenCalled() + expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/b', { mounted: true }) + }) +}) diff --git a/src/components/status-bar/VaultMenu.tsx b/src/components/status-bar/VaultMenu.tsx index 14bfc79a..5d6715e5 100644 --- a/src/components/status-bar/VaultMenu.tsx +++ b/src/components/status-bar/VaultMenu.tsx @@ -18,6 +18,7 @@ import type { VaultOption } from './types' import { useDismissibleLayer } from './useDismissibleLayer' import { workspaceAliasFromOption, workspaceIdentityFromVault } from '../../utils/workspaces' import { reorderVaultPath, vaultPathList } from '../../utils/vaultOrdering' +import { applyMountedChange } from './vaultMenuMountedChange' interface VaultMenuProps { vaults: VaultOption[] @@ -89,6 +90,7 @@ interface VaultMenuInteractionOptions { onSwitchVault: (path: string) => void onUpdateWorkspaceIdentity?: (path: string, patch: Partial) => void setOpen: (open: boolean) => void + vaultPath: string } interface MountToggleRequest { @@ -103,10 +105,6 @@ interface VaultPathSelection extends VaultMenuInteractionOptions { path: string } -interface VaultMountChangeRequest extends VaultMenuInteractionOptions { - mounted: boolean - path: string -} function getVaultTriggerClassName(open: boolean, compact: boolean) { if (compact) { @@ -297,10 +295,6 @@ function useIncludedVaults(vaults: VaultOption[], defaultPath: string): VaultOpt return useMemo(() => vaults.filter((vault) => isIncludedVault(vault, defaultPath)), [defaultPath, vaults]) } -function nextIncludedVaultPath(includedVaults: VaultOption[], currentPath: string): string | null { - return includedVaults.find((vault) => vault.path !== currentPath)?.path ?? null -} - function shouldDisableMountToggle({ canSetDefaultWorkspace, defaultPath, @@ -325,22 +319,6 @@ function selectVaultPath({ setOpen(false) } -function applyMountedChange({ - defaultPath, - includedVaults, - mounted, - onSetDefaultWorkspace, - onUpdateWorkspaceIdentity, - path, -}: VaultMountChangeRequest): void { - if (!mounted && path === defaultPath) { - const nextDefaultPath = nextIncludedVaultPath(includedVaults, path) - if (!nextDefaultPath) return - onSetDefaultWorkspace?.(nextDefaultPath) - } - onUpdateWorkspaceIdentity?.(path, { mounted }) -} - function useVaultMenuInteractions({ defaultPath, includedVaults, @@ -349,6 +327,7 @@ function useVaultMenuInteractions({ onSwitchVault, onUpdateWorkspaceIdentity, setOpen, + vaultPath, }: VaultMenuInteractionOptions) { const disableMountToggleForPath = useCallback((path: string) => ( shouldDisableMountToggle({ @@ -370,22 +349,22 @@ function useVaultMenuInteractions({ onUpdateWorkspaceIdentity, path, setOpen, + vaultPath, }) - }, [defaultPath, includedVaults, multiWorkspaceEnabled, onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity, setOpen]) + }, [defaultPath, includedVaults, multiWorkspaceEnabled, onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity, setOpen, vaultPath]) const handleMountedChange = useCallback((path: string, mounted: boolean) => { applyMountedChange({ defaultPath, includedVaults, mounted, - multiWorkspaceEnabled, onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity, path, - setOpen, + vaultPath, }) - }, [defaultPath, includedVaults, multiWorkspaceEnabled, onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity, setOpen]) + }, [defaultPath, includedVaults, onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity, vaultPath]) return { disableMountToggleForPath, handleMountedChange, handleSelectVault } } @@ -751,6 +730,7 @@ export function VaultMenu(props: VaultMenuProps) { onSwitchVault, onUpdateWorkspaceIdentity, setOpen, + vaultPath, }) useDismissibleLayer(open, menuRef, () => setOpen(false)) diff --git a/src/components/status-bar/vaultMenuMountedChange.ts b/src/components/status-bar/vaultMenuMountedChange.ts new file mode 100644 index 00000000..0bf6b462 --- /dev/null +++ b/src/components/status-bar/vaultMenuMountedChange.ts @@ -0,0 +1,35 @@ +import type { VaultOption } from './types' + +export interface VaultMountChangeRequest { + defaultPath: string + includedVaults: VaultOption[] + mounted: boolean + onSetDefaultWorkspace?: (path: string) => void + onSwitchVault: (path: string) => void + onUpdateWorkspaceIdentity?: (path: string, patch: Partial) => void + path: string + vaultPath: string +} + +function nextIncludedVaultPath(includedVaults: VaultOption[], currentPath: string): string | null { + return includedVaults.find((vault) => vault.path !== currentPath)?.path ?? null +} + +export function applyMountedChange({ + defaultPath, + includedVaults, + mounted, + onSetDefaultWorkspace, + onSwitchVault, + onUpdateWorkspaceIdentity, + path, + vaultPath, +}: VaultMountChangeRequest): void { + if (!mounted && (path === defaultPath || path === vaultPath)) { + const nextPath = nextIncludedVaultPath(includedVaults, path) + if (!nextPath) return + if (path === defaultPath) onSetDefaultWorkspace?.(nextPath) + if (path === vaultPath) onSwitchVault(nextPath) + } + onUpdateWorkspaceIdentity?.(path, { mounted }) +} From 42c3b938862d145ffc8c01b49db934a8c07bf2af Mon Sep 17 00:00:00 2001 From: Elias Stiffel Date: Sun, 24 May 2026 00:45:01 +0200 Subject: [PATCH 5/8] refactor: group applyMountedChange callbacks under a single field Code review on pr-735 flagged `applyMountedChange` for exceeding the 8-parameter limit. The three effect callbacks (`onSetDefaultWorkspace`, `onSwitchVault`, `onUpdateWorkspaceIdentity`) are now grouped under a single `callbacks` field. The function's destructure surface drops from 8 to 6 fields, and the semantic split between "state + change" and "effects" is now explicit in the type. No behaviour change; the six existing tests pass unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../VaultMenu.applyMountedChange.test.ts | 4 +-- src/components/status-bar/VaultMenu.tsx | 10 ++++--- .../status-bar/vaultMenuMountedChange.ts | 26 ++++++++++--------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/components/status-bar/VaultMenu.applyMountedChange.test.ts b/src/components/status-bar/VaultMenu.applyMountedChange.test.ts index d4d8db77..b83aacdb 100644 --- a/src/components/status-bar/VaultMenu.applyMountedChange.test.ts +++ b/src/components/status-bar/VaultMenu.applyMountedChange.test.ts @@ -23,10 +23,8 @@ function callApply(overrides: ChangeOverrides) { vaultPath: overrides.vaultPath ?? '/a', includedVaults: overrides.includedVaults ?? [vault('/a'), vault('/b')], mounted: overrides.mounted ?? false, - onSetDefaultWorkspace, - onSwitchVault, - onUpdateWorkspaceIdentity, path: overrides.path ?? '/a', + callbacks: { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity }, }) return { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } } diff --git a/src/components/status-bar/VaultMenu.tsx b/src/components/status-bar/VaultMenu.tsx index 5d6715e5..f5c266e0 100644 --- a/src/components/status-bar/VaultMenu.tsx +++ b/src/components/status-bar/VaultMenu.tsx @@ -356,13 +356,15 @@ function useVaultMenuInteractions({ const handleMountedChange = useCallback((path: string, mounted: boolean) => { applyMountedChange({ defaultPath, + vaultPath, includedVaults, mounted, - onSetDefaultWorkspace, - onSwitchVault, - onUpdateWorkspaceIdentity, path, - vaultPath, + callbacks: { + onSetDefaultWorkspace, + onSwitchVault, + onUpdateWorkspaceIdentity, + }, }) }, [defaultPath, includedVaults, onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity, vaultPath]) diff --git a/src/components/status-bar/vaultMenuMountedChange.ts b/src/components/status-bar/vaultMenuMountedChange.ts index 0bf6b462..a70fe92f 100644 --- a/src/components/status-bar/vaultMenuMountedChange.ts +++ b/src/components/status-bar/vaultMenuMountedChange.ts @@ -1,14 +1,18 @@ import type { VaultOption } from './types' -export interface VaultMountChangeRequest { - defaultPath: string - includedVaults: VaultOption[] - mounted: boolean +export interface VaultMountChangeCallbacks { onSetDefaultWorkspace?: (path: string) => void onSwitchVault: (path: string) => void onUpdateWorkspaceIdentity?: (path: string, patch: Partial) => void - path: string +} + +export interface VaultMountChangeRequest { + defaultPath: string vaultPath: string + includedVaults: VaultOption[] + mounted: boolean + path: string + callbacks: VaultMountChangeCallbacks } function nextIncludedVaultPath(includedVaults: VaultOption[], currentPath: string): string | null { @@ -17,19 +21,17 @@ function nextIncludedVaultPath(includedVaults: VaultOption[], currentPath: strin export function applyMountedChange({ defaultPath, + vaultPath, includedVaults, mounted, - onSetDefaultWorkspace, - onSwitchVault, - onUpdateWorkspaceIdentity, path, - vaultPath, + callbacks, }: VaultMountChangeRequest): void { if (!mounted && (path === defaultPath || path === vaultPath)) { const nextPath = nextIncludedVaultPath(includedVaults, path) if (!nextPath) return - if (path === defaultPath) onSetDefaultWorkspace?.(nextPath) - if (path === vaultPath) onSwitchVault(nextPath) + if (path === defaultPath) callbacks.onSetDefaultWorkspace?.(nextPath) + if (path === vaultPath) callbacks.onSwitchVault(nextPath) } - onUpdateWorkspaceIdentity?.(path, { mounted }) + callbacks.onUpdateWorkspaceIdentity?.(path, { mounted }) } From 8764b6f78e0599d696b0dd18fa4eecf51fe0a4dd Mon Sep 17 00:00:00 2001 From: mehmet turac Date: Sun, 24 May 2026 21:00:37 +0300 Subject: [PATCH 6/8] fix: respect Kiro default agent fallback --- src/hooks/useAiAgentPreferences.test.ts | 2 ++ src/lib/aiTargets.test.ts | 26 +++++++++++++++++++++++++ src/lib/aiTargets.ts | 25 ++++++++++++++++++++---- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/hooks/useAiAgentPreferences.test.ts b/src/hooks/useAiAgentPreferences.test.ts index 25a79103..5a0ff62b 100644 --- a/src/hooks/useAiAgentPreferences.test.ts +++ b/src/hooks/useAiAgentPreferences.test.ts @@ -18,6 +18,7 @@ const aiAgentsStatus = { opencode: { status: 'missing' as const, version: null }, pi: { status: 'missing' as const, version: null }, gemini: { status: 'missing' as const, version: null }, + kiro: { status: 'missing' as const, version: null }, } describe('useAiAgentPreferences', () => { @@ -86,6 +87,7 @@ describe('useAiAgentPreferences', () => { opencode: { status: 'missing', version: null }, pi: { status: 'missing', version: null }, gemini: { status: 'missing', version: null }, + kiro: { status: 'missing', version: null }, }, })) diff --git a/src/lib/aiTargets.test.ts b/src/lib/aiTargets.test.ts index 36062290..1c8c4521 100644 --- a/src/lib/aiTargets.test.ts +++ b/src/lib/aiTargets.test.ts @@ -59,6 +59,32 @@ describe('ai target provider contract', () => { }) }) + it('accepts legacy agent ids saved in the default target field', () => { + const target = resolveAiTarget({ + default_ai_agent: 'claude_code', + default_ai_target: 'kiro', + } as Settings) + + expect(target).toMatchObject({ + kind: 'agent', + agent: 'kiro', + id: 'agent:kiro', + }) + }) + + it('uses the legacy default agent when a saved agent target is stale', () => { + const target = resolveAiTarget({ + default_ai_agent: 'kiro', + default_ai_target: 'agent:claude_code', + } as Settings) + + expect(target).toMatchObject({ + kind: 'agent', + agent: 'kiro', + id: 'agent:kiro', + }) + }) + it('keeps provider defaults in one catalog with stable grouping metadata', () => { const entries = aiModelProviderCatalog() const kinds = entries.map((entry) => entry.kind) diff --git a/src/lib/aiTargets.ts b/src/lib/aiTargets.ts index da05d20e..8b52e10f 100644 --- a/src/lib/aiTargets.ts +++ b/src/lib/aiTargets.ts @@ -125,11 +125,16 @@ export function agentTargets(): AiTarget[] { export function resolveAiTarget(settings: Settings): AiTarget { const providers = normalizeAiModelProviders(settings.ai_model_providers) const targets = [...agentTargets(), ...configuredModelTargets(providers)] - const storedTarget = settings.default_ai_target - const target = targets.find((candidate) => candidate.id === storedTarget) - if (target) return target + const storedLegacyAgent = normalizeStoredAiAgent(settings.default_ai_agent) + const legacyAgent = storedLegacyAgent ?? DEFAULT_AI_AGENT + const target = resolveStoredAiTarget(settings.default_ai_target, targets) + if (target) { + if (target.kind === 'agent' && storedLegacyAgent && target.agent === DEFAULT_AI_AGENT && target.agent !== storedLegacyAgent) { + return agentTargets().find((candidate) => candidate.kind === 'agent' && candidate.agent === legacyAgent) ?? target + } + return target + } - const legacyAgent = normalizeStoredAiAgent(settings.default_ai_agent) ?? DEFAULT_AI_AGENT return agentTargets().find((candidate) => candidate.kind === 'agent' && candidate.agent === legacyAgent) ?? agentTargets()[0] } @@ -137,6 +142,18 @@ export function targetAgent(target: AiTarget): AiAgentId { return target.kind === 'agent' ? target.agent : DEFAULT_AI_AGENT } +function resolveStoredAiTarget(storedTarget: string | null | undefined, targets: AiTarget[]): AiTarget | null { + const target = storedTarget?.trim() + if (!target) return null + + const exactTarget = targets.find((candidate) => candidate.id === target) + if (exactTarget) return exactTarget + + const legacyAgent = normalizeStoredAiAgent(target) + if (!legacyAgent) return null + return targets.find((candidate) => candidate.kind === 'agent' && candidate.agent === legacyAgent) ?? null +} + export function normalizeAiModelProviders(providers: AiModelProvider[] | null | undefined): AiModelProvider[] { return (providers ?? []).map(normalizeAiModelProvider).filter((provider): provider is AiModelProvider => provider !== null) } From 7516dfe48a7ea9243dadeae98d420d77d9081ccd Mon Sep 17 00:00:00 2001 From: mehmet turac Date: Sun, 24 May 2026 21:58:42 +0300 Subject: [PATCH 7/8] fix: treat name-only git remotes as local-only --- src-tauri/src/commands/git.rs | 2 +- src-tauri/src/git/remote.rs | 83 +++++++++++++++++++++++++++++++++-- src/hooks/useAutoSync.test.ts | 34 ++++++++++++++ src/hooks/useAutoSync.ts | 12 ++++- src/types.ts | 2 +- 5 files changed, 127 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/commands/git.rs b/src-tauri/src/commands/git.rs index b6b36e40..d1028dd3 100644 --- a/src-tauri/src/commands/git.rs +++ b/src-tauri/src/commands/git.rs @@ -475,7 +475,7 @@ mod tests { assert_eq!(pull.status, "no_remote"); let push = git_push(vault.clone()).await.unwrap(); - assert_eq!(push.status, "error"); + assert_eq!(push.status, "no_remote"); let status = git_remote_status(vault.clone()).await.unwrap(); assert!(!status.has_remote); diff --git a/src-tauri/src/git/remote.rs b/src-tauri/src/git/remote.rs index 89234787..507067b6 100644 --- a/src-tauri/src/git/remote.rs +++ b/src-tauri/src/git/remote.rs @@ -18,10 +18,22 @@ pub struct GitPullResult { pub fn has_remote(vault_path: &str) -> Result { let vault = Path::new(vault_path); let output = git_command() - .args(["remote"]) + .args(["config", "--get-regexp", r"^remote\..*\.url$"]) .current_dir(vault) .output() - .map_err(|e| format!("Failed to run git remote: {}", e))?; + .map_err(|e| format!("Failed to inspect git remotes: {}", e))?; + + if !output.status.success() { + if output.status.code() == Some(1) { + return Ok(false); + } + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + return Err(if stderr.is_empty() { + "Failed to inspect git remotes".to_string() + } else { + stderr + }); + } Ok(!String::from_utf8_lossy(&output.stdout).trim().is_empty()) } @@ -181,7 +193,7 @@ fn current_branch(vault: &Path) -> Result { #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] pub struct GitPushResult { - pub status: String, // "ok" | "rejected" | "auth_error" | "network_error" | "error" + pub status: String, // "ok" | "rejected" | "auth_error" | "network_error" | "no_remote" | "error" pub message: String, } @@ -210,6 +222,10 @@ pub fn classify_push_error(stderr: &str) -> GitPushResult { ); } + if is_no_remote_push_error(&lower) { + return push_error("no_remote", "No remote configured"); + } + push_error( "error", format!("Push failed: {}", push_error_detail(stderr)), @@ -259,6 +275,18 @@ fn is_network_push_error(lower: &str) -> bool { ) } +fn is_no_remote_push_error(lower: &str) -> bool { + contains_any( + lower, + &[ + "no configured push destination", + "does not appear to be a git repository", + "no such remote", + "no upstream branch", + ], + ) +} + fn push_error_detail(stderr: &str) -> String { let hint_line = stderr .lines() @@ -283,6 +311,13 @@ fn push_error_detail(stderr: &str) -> String { pub fn git_push(vault_path: &str) -> Result { let vault = Path::new(vault_path); + if !has_remote(vault_path)? { + return Ok(GitPushResult { + status: "no_remote".to_string(), + message: "No remote configured".to_string(), + }); + } + let output = git_command() .args(["push"]) .current_dir(vault) @@ -331,6 +366,27 @@ mod tests { assert!(has_remote(vp).unwrap()); } + #[test] + fn test_has_remote_ignores_name_only_remote_without_url() { + let dir = setup_git_repo(); + let vault = dir.path(); + let vp = vault.to_str().unwrap(); + + git_command() + .args(["config", "remote.origin.prune", "true"]) + .current_dir(vault) + .output() + .unwrap(); + + let remote_names = git_command() + .args(["remote"]) + .current_dir(vault) + .output() + .unwrap(); + assert!(String::from_utf8_lossy(&remote_names.stdout).contains("origin")); + assert!(!has_remote(vp).unwrap()); + } + #[test] fn test_git_pull_no_remote_returns_no_remote() { let dir = setup_git_repo(); @@ -430,6 +486,14 @@ hint: have locally."#; assert!(result.message.contains("network")); } + #[test] + fn test_classify_push_error_no_remote() { + let stderr = "fatal: No configured push destination."; + let result = classify_push_error(stderr); + assert_eq!(result.status, "no_remote"); + assert!(result.message.contains("No remote")); + } + #[test] fn test_classify_push_error_unknown() { let stderr = "error: something unexpected happened\nhint: Try again later"; @@ -469,6 +533,19 @@ hint: have locally."#; assert_eq!(result.status, "ok"); } + #[test] + fn test_git_push_no_remote_returns_no_remote() { + let dir = setup_git_repo(); + let vault = dir.path(); + let vp = vault.to_str().unwrap(); + + fs::write(vault.join("note.md"), "# Note\n").unwrap(); + git_commit(vp, "initial").unwrap(); + + let result = git_push(vp).unwrap(); + assert_eq!(result.status, "no_remote"); + } + #[test] fn test_git_push_rejected_returns_rejected() { let (_bare, clone_a, clone_b) = setup_remote_pair(); diff --git a/src/hooks/useAutoSync.test.ts b/src/hooks/useAutoSync.test.ts index fea1f079..2e280b33 100644 --- a/src/hooks/useAutoSync.test.ts +++ b/src/hooks/useAutoSync.test.ts @@ -549,6 +549,40 @@ describe('useAutoSync', () => { }) }) + it('pullAndPush skips push for local-only vaults with no remote', async () => { + const { result } = renderSync() + await waitFor(() => { + expect(result.current.syncStatus).toBe('idle') + }) + + mockInvokeFn.mockClear() + mockInvokeFn.mockImplementation((cmd: string) => { + if (cmd === 'get_conflict_files') return Promise.resolve([]) + if (cmd === 'get_last_commit_info') return Promise.resolve(MOCK_COMMIT_INFO) + if (cmd === 'git_remote_status') return Promise.resolve({ branch: 'main', ahead: 0, behind: 0, hasRemote: false }) + if (cmd === 'git_pull') { + return Promise.resolve({ + status: 'no_remote', + message: 'No remote configured', + updatedFiles: [], + conflictFiles: [], + }) + } + if (cmd === 'git_push') return Promise.resolve({ status: 'ok', message: 'Pushed successfully' }) + return Promise.resolve(upToDate()) + }) + + await act(async () => { + result.current.pullAndPush() + }) + + await waitFor(() => { + expect(mockInvokeFn).toHaveBeenCalledWith('git_pull', { vaultPath: '/Users/luca/Laputa' }) + expect(mockInvokeFn).not.toHaveBeenCalledWith('git_push', { vaultPath: '/Users/luca/Laputa' }) + expect(result.current.syncStatus).toBe('idle') + }) + }) + it('surfaces pull conflicts and pull errors during recovery pushes', async () => { let mode: 'conflict' | 'error' = 'conflict' diff --git a/src/hooks/useAutoSync.ts b/src/hooks/useAutoSync.ts index 0eca2b6e..fb75b38b 100644 --- a/src/hooks/useAutoSync.ts +++ b/src/hooks/useAutoSync.ts @@ -521,7 +521,17 @@ export function useAutoSync({ }) } - const pushOutcomes = await Promise.all(pullVaultPaths.map(async (path) => ({ + const pushVaultPaths = pullOutcomes + .filter((outcome) => outcome.result.status !== 'no_remote') + .map((outcome) => outcome.vaultPath) + + if (pushVaultPaths.length === 0) { + clearConflictState(setSyncStatus, setConflictFiles, setConflictVaultPath) + void refreshRemoteStatus(pullVaultPaths) + return + } + + const pushOutcomes = await Promise.all(pushVaultPaths.map(async (path) => ({ result: await tauriCall('git_push', { vaultPath: path }), vaultPath: path, }))) diff --git a/src/types.ts b/src/types.ts index 1b6f6882..9cd02d16 100644 --- a/src/types.ts +++ b/src/types.ts @@ -141,7 +141,7 @@ export interface GitPullResult { } export interface GitPushResult { - status: 'ok' | 'rejected' | 'auth_error' | 'network_error' | 'error' + status: 'ok' | 'rejected' | 'auth_error' | 'network_error' | 'no_remote' | 'error' message: string } From ae7ee4064fcdc18e75e93ca57901681c7bf3d95a Mon Sep 17 00:00:00 2001 From: lucaronin Date: Tue, 26 May 2026 10:26:29 +0200 Subject: [PATCH 8/8] refactor: keep merged PRs above code health gate --- src-tauri/src/git/remote.rs | 38 +++++-- src/components/FolderTree.test.tsx | 104 ++++++++---------- src/components/FolderTree.tsx | 62 +++++++---- .../VaultMenu.applyMountedChange.test.ts | 87 +++++++-------- .../status-bar/vaultMenuMountedChange.ts | 22 +++- src/lib/aiTargets.ts | 20 +++- 6 files changed, 183 insertions(+), 150 deletions(-) diff --git a/src-tauri/src/git/remote.rs b/src-tauri/src/git/remote.rs index 507067b6..86dad2e6 100644 --- a/src-tauri/src/git/remote.rs +++ b/src-tauri/src/git/remote.rs @@ -4,6 +4,13 @@ use std::path::Path; use super::conflict::get_conflict_files; +const GIT_CONFIG_SUBCOMMAND: &str = "config"; +const GIT_CONFIG_GET_REGEXP: &str = "--get-regexp"; +const REMOTE_URL_CONFIG_PATTERN: &str = r"^remote\..*\.url$"; +const NO_REMOTE_STATUS: &str = "no_remote"; +const NO_REMOTE_MESSAGE: &str = "No remote configured"; +const GIT_INSPECT_REMOTES_ERROR: &str = "Failed to inspect git remotes"; + #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] pub struct GitPullResult { pub status: String, // "up_to_date" | "updated" | "conflict" | "no_remote" | "error" @@ -17,8 +24,16 @@ pub struct GitPullResult { /// Check whether the vault repo has at least one remote configured. pub fn has_remote(vault_path: &str) -> Result { let vault = Path::new(vault_path); + remote_url_config_exists(vault) +} + +fn remote_url_config_exists(vault: &Path) -> Result { let output = git_command() - .args(["config", "--get-regexp", r"^remote\..*\.url$"]) + .args([ + GIT_CONFIG_SUBCOMMAND, + GIT_CONFIG_GET_REGEXP, + REMOTE_URL_CONFIG_PATTERN, + ]) .current_dir(vault) .output() .map_err(|e| format!("Failed to inspect git remotes: {}", e))?; @@ -29,7 +44,7 @@ pub fn has_remote(vault_path: &str) -> Result { } let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); return Err(if stderr.is_empty() { - "Failed to inspect git remotes".to_string() + GIT_INSPECT_REMOTES_ERROR.to_string() } else { stderr }); @@ -45,8 +60,8 @@ pub fn git_pull(vault_path: &str) -> Result { if !has_remote(vault_path)? { return Ok(GitPullResult { - status: "no_remote".to_string(), - message: "No remote configured".to_string(), + status: NO_REMOTE_STATUS.to_string(), + message: NO_REMOTE_MESSAGE.to_string(), updated_files: vec![], conflict_files: vec![], }); @@ -313,8 +328,8 @@ pub fn git_push(vault_path: &str) -> Result { if !has_remote(vault_path)? { return Ok(GitPushResult { - status: "no_remote".to_string(), - message: "No remote configured".to_string(), + status: NO_REMOTE_STATUS.to_string(), + message: NO_REMOTE_MESSAGE.to_string(), }); } @@ -342,6 +357,11 @@ mod tests { use crate::git::tests::{setup_git_repo, setup_remote_pair}; use std::fs; + fn commit_default_note(vault_path: &Path) { + fs::write(vault_path.join("note.md"), "# Note\n").unwrap(); + git_commit(vault_path.to_str().unwrap(), "initial").unwrap(); + } + #[test] fn test_has_remote_returns_false_for_local_repo() { let dir = setup_git_repo(); @@ -527,8 +547,7 @@ hint: have locally."#; let (_bare, clone_a, _clone_b) = setup_remote_pair(); let vp_a = clone_a.path().to_str().unwrap(); - fs::write(clone_a.path().join("note.md"), "# Note\n").unwrap(); - git_commit(vp_a, "initial").unwrap(); + commit_default_note(clone_a.path()); let result = git_push(vp_a).unwrap(); assert_eq!(result.status, "ok"); } @@ -539,8 +558,7 @@ hint: have locally."#; let vault = dir.path(); let vp = vault.to_str().unwrap(); - fs::write(vault.join("note.md"), "# Note\n").unwrap(); - git_commit(vp, "initial").unwrap(); + commit_default_note(vault); let result = git_push(vp).unwrap(); assert_eq!(result.status, "no_remote"); diff --git a/src/components/FolderTree.test.tsx b/src/components/FolderTree.test.tsx index fbaf873a..b50fbd04 100644 --- a/src/components/FolderTree.test.tsx +++ b/src/components/FolderTree.test.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react' +import { useState, type ComponentProps } from 'react' import { act, fireEvent, render, screen, within } from '@testing-library/react' import { describe, it, expect, vi } from 'vitest' import { FolderTree } from './FolderTree' @@ -22,6 +22,30 @@ const mockFolders: FolderNode[] = [ const defaultSelection: SidebarSelection = { kind: 'filter', filter: 'all' } const vaultRootPath = '/Users/luca/Laputa' +function renderTree(props: Partial> = {}) { + const onSelect = props.onSelect ?? vi.fn() + render( + , + ) + return { onSelect } +} + +function clickFolderRow(path: string) { + fireEvent.click(screen.getByTestId(`folder-row:${path}`)) +} + +async function submitNewFolder(name: string) { + fireEvent.click(screen.getByTestId('create-folder-btn')) + const input = screen.getByTestId('new-folder-input') + fireEvent.change(input, { target: { value: name } }) + fireEvent.keyDown(input, { key: 'Enter' }) +} + describe('FolderTree', () => { it('renders nothing when folders is empty', () => { const { container } = render( @@ -153,32 +177,16 @@ describe('FolderTree', () => { }) it('selects the vault root with the root path attached', () => { - const onSelect = vi.fn() - render( - , - ) + const { onSelect } = renderTree({ vaultRootPath }) - fireEvent.click(screen.getByTestId('folder-row:')) + clickFolderRow('') expect(onSelect).toHaveBeenCalledWith({ kind: 'folder', path: '', rootPath: vaultRootPath }) }) it('selects child folders with the vault root path attached when the tree has a root', () => { - const onSelect = vi.fn() - render( - , - ) + const { onSelect } = renderTree({ vaultRootPath }) - fireEvent.click(screen.getByTestId('folder-row:areas')) + clickFolderRow('areas') expect(onSelect).toHaveBeenCalledWith({ kind: 'folder', path: 'areas', rootPath: vaultRootPath }) }) @@ -237,20 +245,13 @@ describe('FolderTree', () => { it('passes the selected folder as the parent when creating a new folder', async () => { const onCreateFolder = vi.fn().mockResolvedValue(true) - render( - , - ) + renderTree({ + selection: { kind: 'folder', path: 'projects', rootPath: vaultRootPath }, + onCreateFolder, + vaultRootPath, + }) - fireEvent.click(screen.getByTestId('create-folder-btn')) - const input = screen.getByTestId('new-folder-input') - fireEvent.change(input, { target: { value: 'laputa' } }) - fireEvent.keyDown(input, { key: 'Enter' }) + await submitNewFolder('laputa') await vi.waitFor(() => { expect(onCreateFolder).toHaveBeenCalledWith('laputa', { @@ -278,20 +279,14 @@ describe('FolderTree', () => { }, ] - render( - , - ) + renderTree({ + folders, + selection: { kind: 'folder', path: '', rootPath: otherVault }, + onCreateFolder, + vaultRootPath, + }) - fireEvent.click(screen.getByTestId('create-folder-btn')) - const input = screen.getByTestId('new-folder-input') - fireEvent.change(input, { target: { value: 'inbox' } }) - fireEvent.keyDown(input, { key: 'Enter' }) + await submitNewFolder('inbox') await vi.waitFor(() => { expect(onCreateFolder).toHaveBeenCalledWith('inbox', { @@ -303,20 +298,9 @@ describe('FolderTree', () => { it('omits parent context when nothing folder-like is selected', async () => { const onCreateFolder = vi.fn().mockResolvedValue(true) - render( - , - ) + renderTree({ onCreateFolder, vaultRootPath }) - fireEvent.click(screen.getByTestId('create-folder-btn')) - const input = screen.getByTestId('new-folder-input') - fireEvent.change(input, { target: { value: 'inbox' } }) - fireEvent.keyDown(input, { key: 'Enter' }) + await submitNewFolder('inbox') await vi.waitFor(() => { expect(onCreateFolder).toHaveBeenCalledWith('inbox', undefined) diff --git a/src/components/FolderTree.tsx b/src/components/FolderTree.tsx index b35b89c9..dabcffd1 100644 --- a/src/components/FolderTree.tsx +++ b/src/components/FolderTree.tsx @@ -94,6 +94,39 @@ function useDisplayedFolders(folders: FolderNode[], expanded: Record void + expandFolder: (key: string) => void + onCreateFolder?: (name: string, parent?: FolderCreationParent) => Promise | boolean + selection: SidebarSelection +}) { + return useCallback(async (value: string) => { + const nextName = value.trim() + if (!nextName || !onCreateFolder) { + closeCreateForm() + return true + } + + const parent = creationParentForSelection(selection) + const created = await onCreateFolder(nextName, parent) + if (!created) return created + + closeCreateForm() + if (parent?.path) expandFolder(folderNodeKey(parent)) + return created + }, [closeCreateForm, expandFolder, onCreateFolder, selection]) +} + export const FolderTree = memo(function FolderTree({ folders, selection, @@ -140,29 +173,12 @@ export const FolderTree = memo(function FolderTree({ onStartRenameFolder, }) - const handleCreateFolderSubmit = useCallback(async (value: string) => { - const nextName = value.trim() - if (!nextName || !onCreateFolder) { - closeCreateForm() - return true - } - - const parent: FolderCreationParent | undefined = selection.kind === 'folder' - ? { path: selection.path, rootPath: selection.rootPath } - : undefined - const created = await onCreateFolder(nextName, parent) - if (created) { - closeCreateForm() - // Ensure the parent folder is open so the freshly created child is visible. - // The selection→ancestor-expansion machinery deliberately doesn't expand the - // selected node itself, so we expand it explicitly here. Vault-root keys are - // open by default and don't need this. - if (parent?.path) { - expandFolder(folderNodeKey(parent)) - } - } - return created - }, [closeCreateForm, expandFolder, onCreateFolder, selection]) + const handleCreateFolderSubmit = useCreateFolderSubmit({ + closeCreateForm, + expandFolder, + onCreateFolder, + selection, + }) const handleCreateFolderClick = useCallback(() => { closeContextMenu() diff --git a/src/components/status-bar/VaultMenu.applyMountedChange.test.ts b/src/components/status-bar/VaultMenu.applyMountedChange.test.ts index b83aacdb..e93a8e7d 100644 --- a/src/components/status-bar/VaultMenu.applyMountedChange.test.ts +++ b/src/components/status-bar/VaultMenu.applyMountedChange.test.ts @@ -30,53 +30,48 @@ function callApply(overrides: ChangeOverrides) { } describe('applyMountedChange', () => { - it('reroutes both the default workspace and the active vault when the unmounted vault is both', () => { - const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ - defaultPath: '/a', - vaultPath: '/a', - path: '/a', - }) + it.each([ + { + name: 'both default and active vault', + request: { defaultPath: '/a', vaultPath: '/a', path: '/a' }, + expectedDefault: '/b', + expectedSwitch: '/b', + expectedIdentity: ['/a', { mounted: false }], + }, + { + name: 'active vault only', + request: { defaultPath: '/b', vaultPath: '/a', path: '/a' }, + expectedDefault: null, + expectedSwitch: '/b', + expectedIdentity: ['/a', { mounted: false }], + }, + { + name: 'default workspace only', + request: { defaultPath: '/a', vaultPath: '/b', path: '/a' }, + expectedDefault: '/b', + expectedSwitch: null, + expectedIdentity: ['/a', { mounted: false }], + }, + { + name: 'neither default nor active vault', + request: { + defaultPath: '/a', + vaultPath: '/a', + path: '/b', + includedVaults: [vault('/a'), vault('/b'), vault('/c')], + }, + expectedDefault: null, + expectedSwitch: null, + expectedIdentity: ['/b', { mounted: false }], + }, + ])('handles unmounting $name', ({ request, expectedDefault, expectedSwitch, expectedIdentity }) => { + const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply(request) - expect(onSetDefaultWorkspace).toHaveBeenCalledWith('/b') - expect(onSwitchVault).toHaveBeenCalledWith('/b') - expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/a', { mounted: false }) - }) - - it('reroutes the active vault when only the active vault is unmounted (default workspace differs)', () => { - const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ - defaultPath: '/b', - vaultPath: '/a', - path: '/a', - }) - - expect(onSetDefaultWorkspace).not.toHaveBeenCalled() - expect(onSwitchVault).toHaveBeenCalledWith('/b') - expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/a', { mounted: false }) - }) - - it('reroutes the default workspace when only the default is unmounted (active vault differs)', () => { - const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ - defaultPath: '/a', - vaultPath: '/b', - path: '/a', - }) - - expect(onSetDefaultWorkspace).toHaveBeenCalledWith('/b') - expect(onSwitchVault).not.toHaveBeenCalled() - expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/a', { mounted: false }) - }) - - it('does not reroute when the unmounted vault is neither default nor active', () => { - const { onSetDefaultWorkspace, onSwitchVault, onUpdateWorkspaceIdentity } = callApply({ - defaultPath: '/a', - vaultPath: '/a', - path: '/b', - includedVaults: [vault('/a'), vault('/b'), vault('/c')], - }) - - expect(onSetDefaultWorkspace).not.toHaveBeenCalled() - expect(onSwitchVault).not.toHaveBeenCalled() - expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith('/b', { mounted: false }) + if (expectedDefault) expect(onSetDefaultWorkspace).toHaveBeenCalledWith(expectedDefault) + else expect(onSetDefaultWorkspace).not.toHaveBeenCalled() + if (expectedSwitch) expect(onSwitchVault).toHaveBeenCalledWith(expectedSwitch) + else expect(onSwitchVault).not.toHaveBeenCalled() + expect(onUpdateWorkspaceIdentity).toHaveBeenCalledWith(...expectedIdentity) }) it('bails out without changing anything when no alternative mounted vault exists', () => { diff --git a/src/components/status-bar/vaultMenuMountedChange.ts b/src/components/status-bar/vaultMenuMountedChange.ts index a70fe92f..77023e18 100644 --- a/src/components/status-bar/vaultMenuMountedChange.ts +++ b/src/components/status-bar/vaultMenuMountedChange.ts @@ -19,6 +19,20 @@ function nextIncludedVaultPath(includedVaults: VaultOption[], currentPath: strin return includedVaults.find((vault) => vault.path !== currentPath)?.path ?? null } +function isCurrentPathUnmount(request: VaultMountChangeRequest): boolean { + if (request.mounted) return false + if (request.path === request.defaultPath) return true + return request.path === request.vaultPath +} + +function rerouteUnmountedPath(request: VaultMountChangeRequest): boolean { + const nextPath = nextIncludedVaultPath(request.includedVaults, request.path) + if (!nextPath) return false + if (request.path === request.defaultPath) request.callbacks.onSetDefaultWorkspace?.(nextPath) + if (request.path === request.vaultPath) request.callbacks.onSwitchVault(nextPath) + return true +} + export function applyMountedChange({ defaultPath, vaultPath, @@ -27,11 +41,7 @@ export function applyMountedChange({ path, callbacks, }: VaultMountChangeRequest): void { - if (!mounted && (path === defaultPath || path === vaultPath)) { - const nextPath = nextIncludedVaultPath(includedVaults, path) - if (!nextPath) return - if (path === defaultPath) callbacks.onSetDefaultWorkspace?.(nextPath) - if (path === vaultPath) callbacks.onSwitchVault(nextPath) - } + const request = { defaultPath, vaultPath, includedVaults, mounted, path, callbacks } + if (isCurrentPathUnmount(request) && !rerouteUnmountedPath(request)) return callbacks.onUpdateWorkspaceIdentity?.(path, { mounted }) } diff --git a/src/lib/aiTargets.ts b/src/lib/aiTargets.ts index 8b52e10f..b9d78182 100644 --- a/src/lib/aiTargets.ts +++ b/src/lib/aiTargets.ts @@ -124,24 +124,34 @@ export function agentTargets(): AiTarget[] { export function resolveAiTarget(settings: Settings): AiTarget { const providers = normalizeAiModelProviders(settings.ai_model_providers) - const targets = [...agentTargets(), ...configuredModelTargets(providers)] + const agents = agentTargets() + const targets = [...agents, ...configuredModelTargets(providers)] const storedLegacyAgent = normalizeStoredAiAgent(settings.default_ai_agent) const legacyAgent = storedLegacyAgent ?? DEFAULT_AI_AGENT const target = resolveStoredAiTarget(settings.default_ai_target, targets) if (target) { - if (target.kind === 'agent' && storedLegacyAgent && target.agent === DEFAULT_AI_AGENT && target.agent !== storedLegacyAgent) { - return agentTargets().find((candidate) => candidate.kind === 'agent' && candidate.agent === legacyAgent) ?? target - } + if (shouldPreferLegacyAgent(target, storedLegacyAgent)) return agentTargetFor(agents, legacyAgent) ?? target return target } - return agentTargets().find((candidate) => candidate.kind === 'agent' && candidate.agent === legacyAgent) ?? agentTargets()[0] + return agentTargetFor(agents, legacyAgent) ?? agents[0] } export function targetAgent(target: AiTarget): AiAgentId { return target.kind === 'agent' ? target.agent : DEFAULT_AI_AGENT } +function shouldPreferLegacyAgent(target: AiTarget, storedLegacyAgent: AiAgentId | null): boolean { + if (target.kind !== 'agent') return false + if (!storedLegacyAgent) return false + if (target.agent !== DEFAULT_AI_AGENT) return false + return target.agent !== storedLegacyAgent +} + +function agentTargetFor(agents: AiTarget[], agent: AiAgentId): AiTarget | undefined { + return agents.find((candidate) => candidate.kind === 'agent' && candidate.agent === agent) +} + function resolveStoredAiTarget(storedTarget: string | null | undefined, targets: AiTarget[]): AiTarget | null { const target = storedTarget?.trim() if (!target) return null