From eb731bbaada19afd25b6cb9d7c9e5a2533f2e28f Mon Sep 17 00:00:00 2001 From: lucaronin Date: Fri, 8 May 2026 17:37:04 +0200 Subject: [PATCH] fix: guard inspector property mutations --- src/components/Editor.tsx | 13 +-- src/components/EditorRightPanel.tsx | 7 +- src/components/Inspector.test.tsx | 13 +++ src/components/Inspector.tsx | 7 +- .../inspector/useInspectorPropertyActions.ts | 39 ++++++-- src/hooks/frontmatterOps.ts | 4 + src/hooks/useNoteActions.hook.test.ts | 16 ++++ src/hooks/useNoteActions.ts | 95 +++++++++++++------ tests/smoke/save-before-note-switch.spec.ts | 30 ++++++ 9 files changed, 172 insertions(+), 52 deletions(-) diff --git a/src/components/Editor.tsx b/src/components/Editor.tsx index 761ec4bd..9c315ff8 100644 --- a/src/components/Editor.tsx +++ b/src/components/Editor.tsx @@ -11,6 +11,7 @@ import { RUNTIME_STYLE_NONCE } from '../lib/runtimeStyleNonce' import type { VaultEntry, GitCommit, NoteWidthMode, NoteStatus } from '../types' import type { NoteListItem } from '../utils/ai-context' import type { FrontmatterValue } from './Inspector' +import type { FrontmatterOpOptions } from '../hooks/frontmatterOps' import { ResizeHandle } from './ResizeHandle' import { useDiffMode, type CommitDiffRequest } from '../hooks/useDiffMode' import { useEditorFocus } from '../hooks/useEditorFocus' @@ -66,9 +67,9 @@ interface EditorProps { inspectorEntry: VaultEntry | null inspectorContent: string | null gitHistory: GitCommit[] - onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise - onDeleteProperty?: (path: string, key: string) => Promise - onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise + onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise + onDeleteProperty?: (path: string, key: string, options?: FrontmatterOpOptions) => Promise + onAddProperty?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void @@ -437,9 +438,9 @@ function EditorLayout({ noteList?: NoteListItem[] noteListFilter?: { type: string | null; query: string } handleViewCommitDiff: (commitHash: string) => Promise - onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise - onDeleteProperty?: (path: string, key: string) => Promise - onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise + onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise + onDeleteProperty?: (path: string, key: string, options?: FrontmatterOpOptions) => Promise + onAddProperty?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void diff --git a/src/components/EditorRightPanel.tsx b/src/components/EditorRightPanel.tsx index 3fbd3b78..01acce7e 100644 --- a/src/components/EditorRightPanel.tsx +++ b/src/components/EditorRightPanel.tsx @@ -6,6 +6,7 @@ import type { AppLocale } from '../lib/i18n' import type { VaultEntry, GitCommit } from '../types' import type { NoteListItem } from '../utils/ai-context' import { Inspector, type FrontmatterValue } from './Inspector' +import type { FrontmatterOpOptions } from '../hooks/frontmatterOps' import { AiPanelView } from './AiPanel' import { useAiPanelController } from './useAiPanelController' import { NEW_AI_CHAT_EVENT } from '../utils/aiPromptBridge' @@ -34,9 +35,9 @@ interface EditorRightPanelProps { onToggleTableOfContents?: () => void onNavigateWikilink: (target: string) => void onViewCommitDiff: (commitHash: string) => Promise - onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise - onDeleteProperty?: (path: string, key: string) => Promise - onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise + onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise + onDeleteProperty?: (path: string, key: string, options?: FrontmatterOpOptions) => Promise + onAddProperty?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void diff --git a/src/components/Inspector.test.tsx b/src/components/Inspector.test.tsx index bbd21835..d6b71611 100644 --- a/src/components/Inspector.test.tsx +++ b/src/components/Inspector.test.tsx @@ -187,6 +187,19 @@ Status: Evergreened expect(btn).toBeDisabled() }) + it('guards delete property callbacks against stale active notes', () => { + const onDeleteProperty = vi.fn().mockResolvedValue(undefined) + renderSelectedInspector({ onDeleteProperty }) + + fireEvent.click(screen.getAllByTitle('Delete property')[0]) + + expect(onDeleteProperty).toHaveBeenCalledWith( + mockEntry.path, + 'Status', + { requireActivePath: mockEntry.path }, + ) + }) + it('shows cadence when present', () => { // Cadence is now read from frontmatter in content (already in mockContent) render() diff --git a/src/components/Inspector.tsx b/src/components/Inspector.tsx index cda8cacc..39bf5a34 100644 --- a/src/components/Inspector.tsx +++ b/src/components/Inspector.tsx @@ -4,6 +4,7 @@ import { cn } from '@/lib/utils' import { Separator } from './ui/separator' import { parseFrontmatter, detectFrontmatterState, detectFrontmatterWarnings } from '../utils/frontmatter' import { DynamicPropertiesPanel } from './DynamicPropertiesPanel' +import type { FrontmatterOpOptions } from '../hooks/frontmatterOps' import { DynamicRelationshipsPanel, BacklinksPanel, @@ -30,9 +31,9 @@ interface InspectorProps { vaultPath?: string onNavigate: (target: string) => void onViewCommitDiff?: (commitHash: string) => void - onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise - onDeleteProperty?: (path: string, key: string) => Promise - onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise + onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise + onDeleteProperty?: (path: string, key: string, options?: FrontmatterOpOptions) => Promise + onAddProperty?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise onCreateAndOpenNote?: (title: string) => Promise onInitializeProperties?: (path: string) => void diff --git a/src/components/inspector/useInspectorPropertyActions.ts b/src/components/inspector/useInspectorPropertyActions.ts index 3306cb90..2d2fd96e 100644 --- a/src/components/inspector/useInspectorPropertyActions.ts +++ b/src/components/inspector/useInspectorPropertyActions.ts @@ -1,22 +1,43 @@ import { useMemo } from 'react' import type { VaultEntry } from '../../types' +import type { FrontmatterOpOptions } from '../../hooks/frontmatterOps' type FrontmatterValue = string | number | boolean | string[] | null interface InspectorPropertyActionsConfig { entry: VaultEntry | null - onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue) => Promise - onDeleteProperty?: (path: string, key: string) => Promise - onAddProperty?: (path: string, key: string, value: FrontmatterValue) => Promise + onUpdateFrontmatter?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise + onDeleteProperty?: (path: string, key: string, options?: FrontmatterOpOptions) => Promise + onAddProperty?: (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => Promise onCreateMissingType?: (path: string, missingType: string, nextTypeName: string) => Promise } -function bindEntryAction( +function activeEntryOptions(entry: VaultEntry): FrontmatterOpOptions { + return { requireActivePath: entry.path } +} + +function bindUpdateAction( entry: VaultEntry | null, - action: ((path: string, ...args: TArgs) => TResult) | undefined, + action: InspectorPropertyActionsConfig['onUpdateFrontmatter'], ) { if (!entry || !action) return undefined - return (...args: TArgs) => action(entry.path, ...args) + return (key: string, value: FrontmatterValue) => action(entry.path, key, value, activeEntryOptions(entry)) +} + +function bindDeleteAction( + entry: VaultEntry | null, + action: InspectorPropertyActionsConfig['onDeleteProperty'], +) { + if (!entry || !action) return undefined + return (key: string) => action(entry.path, key, activeEntryOptions(entry)) +} + +function bindAddAction( + entry: VaultEntry | null, + action: InspectorPropertyActionsConfig['onAddProperty'], +) { + if (!entry || !action) return undefined + return (key: string, value: FrontmatterValue) => action(entry.path, key, value, activeEntryOptions(entry)) } function bindMissingTypeAction( @@ -36,15 +57,15 @@ export function useInspectorPropertyActions({ onCreateMissingType, }: InspectorPropertyActionsConfig) { const handleUpdateProperty = useMemo( - () => bindEntryAction<[string, FrontmatterValue], Promise>(entry, onUpdateFrontmatter), + () => bindUpdateAction(entry, onUpdateFrontmatter), [entry, onUpdateFrontmatter], ) const handleDeleteProperty = useMemo( - () => bindEntryAction<[string], Promise>(entry, onDeleteProperty), + () => bindDeleteAction(entry, onDeleteProperty), [entry, onDeleteProperty], ) const handleAddProperty = useMemo( - () => bindEntryAction<[string, FrontmatterValue], Promise>(entry, onAddProperty), + () => bindAddAction(entry, onAddProperty), [entry, onAddProperty], ) const handleCreateMissingType = useMemo( diff --git a/src/hooks/frontmatterOps.ts b/src/hooks/frontmatterOps.ts index 585bfdd1..fa27c0aa 100644 --- a/src/hooks/frontmatterOps.ts +++ b/src/hooks/frontmatterOps.ts @@ -259,6 +259,8 @@ async function executeFrontmatterOp( export interface FrontmatterOpOptions { /** Suppress toast feedback (caller manages its own toast). */ silent?: boolean + /** Require this note to still be active before applying UI-side mutation state. */ + requireActivePath?: VaultPath } export interface FrontmatterApplyCallbacks { @@ -266,6 +268,7 @@ export interface FrontmatterApplyCallbacks { updateEntry: (path: VaultPath, patch: Partial) => void toast: (message: ToastMessage) => void getEntry?: (path: VaultPath) => VaultEntry | undefined + shouldApply?: (path: VaultPath) => boolean } export interface FrontmatterRunRequest { @@ -350,6 +353,7 @@ export async function runFrontmatterAndApply(request: FrontmatterRunRequest): Pr const { op, path, key, value, callbacks, options } = request try { const newContent = await executeFrontmatterOp(op, path, key, value) + if (callbacks.shouldApply && !callbacks.shouldApply(path)) return undefined callbacks.updateTab(path, newContent) applyEntryPatch(path, callbacks, frontmatterToEntryPatch(op, key, value)) notifyFrontmatterSuccess(op, callbacks, options) diff --git a/src/hooks/useNoteActions.hook.test.ts b/src/hooks/useNoteActions.hook.test.ts index 3553e4fa..00e481c8 100644 --- a/src/hooks/useNoteActions.hook.test.ts +++ b/src/hooks/useNoteActions.hook.test.ts @@ -226,6 +226,22 @@ describe('useNoteActions hook', () => { expect(setToastMessage).toHaveBeenCalledWith('Property deleted') }) + it('ignores guarded inspector property deletes after the active note changes', async () => { + const noteA = makeEntry({ path: '/vault/note-a.md', filename: 'note-a.md', title: 'Note A' }) + const noteB = makeEntry({ path: '/vault/note-b.md', filename: 'note-b.md', title: 'Note B' }) + const { result } = renderHook(() => useNoteActions(makeConfig([noteA, noteB]))) + + act(() => { + result.current.handleSwitchTab(noteB.path) + }) + await act(async () => { + await result.current.handleDeleteProperty(noteA.path, 'status', { requireActivePath: noteA.path }) + }) + + expect(updateEntry).not.toHaveBeenCalled() + expect(setToastMessage).not.toHaveBeenCalled() + }) + it('handleCreateNoteImmediate creates note with timestamp-based title', async () => { const createdEntry = await createImmediateEntry() expect(createdEntry.title).toBe('Untitled Note 1700000000') diff --git a/src/hooks/useNoteActions.ts b/src/hooks/useNoteActions.ts index 753ae251..eb3b3b66 100644 --- a/src/hooks/useNoteActions.ts +++ b/src/hooks/useNoteActions.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect } from 'react' +import { useCallback, useEffect, type MutableRefObject } from 'react' import type { VaultEntry } from '../types' import type { FrontmatterValue } from '../components/Inspector' import { useTabManagement } from './useTabManagement' @@ -163,6 +163,16 @@ async function flushBeforeNoteMutation( } } +function activePathGuardAllowsMutation( + path: string, + activeTabPathRef: MutableRefObject, + options?: FrontmatterOpOptions, +): boolean { + const requiredPath = options?.requireActivePath + if (!requiredPath) return true + return notePathsMatch(path, requiredPath) && notePathsMatch(activeTabPathRef.current, requiredPath) +} + async function maybeRenameAfterFrontmatterUpdate({ path, key, @@ -183,16 +193,18 @@ interface UpdateFrontmatterAndMaybeRenameParams { key: string options?: FrontmatterOpOptions path: string - runFrontmatterOp: ( - op: 'update' | 'delete', - path: string, - key: string, - value?: FrontmatterValue, - options?: FrontmatterOpOptions, - ) => Promise + runFrontmatterOp: RunFrontmatterOp value: FrontmatterValue } +type RunFrontmatterOp = ( + op: 'update' | 'delete', + path: string, + key: string, + value?: FrontmatterValue, + options?: FrontmatterOpOptions, +) => Promise + async function updateFrontmatterAndMaybeRename({ config, deps, @@ -202,8 +214,10 @@ async function updateFrontmatterAndMaybeRename({ runFrontmatterOp, value, }: UpdateFrontmatterAndMaybeRenameParams): Promise { + if (!activePathGuardAllowsMutation(path, deps.activeTabPathRef, options)) return const canFlush = await flushBeforeNoteMutation(path, config.flushBeforeNoteMutation) if (!canFlush) return + if (!activePathGuardAllowsMutation(path, deps.activeTabPathRef, options)) return const newContent = await runFrontmatterOp('update', path, key, value, options) if (!applyFrontmatterCallbacks({ config, path, newContent })) return @@ -288,13 +302,7 @@ function useFrontmatterActionHandlers({ handleSwitchTab: (path: string) => void setToastMessage: (msg: string | null) => void updateTabContent: (path: string, newContent: string) => void - runFrontmatterOp: ( - op: 'update' | 'delete', - path: string, - key: string, - value?: FrontmatterValue, - options?: FrontmatterOpOptions, - ) => Promise + runFrontmatterOp: RunFrontmatterOp }) { const handleUpdateFrontmatter = useCallback(async ( path: string, @@ -325,22 +333,26 @@ function useFrontmatterActionHandlers({ }, [activeTabPathRef, config, handleSwitchTab, renameTabsRef, runFrontmatterOp, setTabs, setToastMessage, updateTabContent]) const handleDeleteProperty = useCallback(async (path: string, key: string, options?: FrontmatterOpOptions) => { + if (!activePathGuardAllowsMutation(path, activeTabPathRef, options)) return const canFlush = await flushBeforeNoteMutation(path, config.flushBeforeNoteMutation) if (!canFlush) return + if (!activePathGuardAllowsMutation(path, activeTabPathRef, options)) return const newContent = await runFrontmatterOp('delete', path, key, undefined, options) if (!applyFrontmatterCallbacks({ config, path, newContent })) return await notifyFrontmatterPersisted(config, key) - }, [config, runFrontmatterOp]) + }, [activeTabPathRef, config, runFrontmatterOp]) - const handleAddProperty = useCallback(async (path: string, key: string, value: FrontmatterValue) => { + const handleAddProperty = useCallback(async (path: string, key: string, value: FrontmatterValue, options?: FrontmatterOpOptions) => { + if (!activePathGuardAllowsMutation(path, activeTabPathRef, options)) return const canFlush = await flushBeforeNoteMutation(path, config.flushBeforeNoteMutation) if (!canFlush) return + if (!activePathGuardAllowsMutation(path, activeTabPathRef, options)) return - const newContent = await runFrontmatterOp('update', path, key, value) + const newContent = await runFrontmatterOp('update', path, key, value, options) if (!applyFrontmatterCallbacks({ config, path, newContent })) return await notifyFrontmatterPersisted(config, key) - }, [config, runFrontmatterOp]) + }, [activeTabPathRef, config, runFrontmatterOp]) return { handleUpdateFrontmatter, @@ -349,6 +361,38 @@ function useFrontmatterActionHandlers({ } } +function useFrontmatterRunner({ + activeTabPathRef, + entries, + setToastMessage, + updateEntry, + updateTabContent, +}: { + activeTabPathRef: MutableRefObject + entries: VaultEntry[] + setToastMessage: NoteActionsConfig['setToastMessage'] + updateEntry: NoteActionsConfig['updateEntry'] + updateTabContent: (path: string, newContent: string) => void +}): RunFrontmatterOp { + return useCallback( + (op, path, key, value, options) => runFrontmatterAndApply({ + op, + path, + key, + value, + callbacks: { + updateTab: updateTabContent, + updateEntry, + toast: setToastMessage, + getEntry: (p) => findByNotePath(entries, p), + shouldApply: (p) => activePathGuardAllowsMutation(p, activeTabPathRef, options), + }, + options, + }), + [activeTabPathRef, entries, setToastMessage, updateEntry, updateTabContent], + ) +} + export function useNoteActions(config: NoteActionsConfig) { const { entries, setToastMessage, updateEntry } = config const tabMgmt = useTabManagement(buildTabManagementOptions(config)) @@ -379,18 +423,7 @@ export function useNoteActions(config: NoteActionsConfig) { [entries, handleSelectNote, tabMgmt.activeTabPath, tabMgmt.tabs], ) - const runFrontmatterOp = useCallback( - (op: 'update' | 'delete', path: string, key: string, value?: FrontmatterValue, options?: FrontmatterOpOptions) => - runFrontmatterAndApply({ - op, - path, - key, - value, - callbacks: { updateTab: updateTabContent, updateEntry, toast: setToastMessage, getEntry: (p) => findByNotePath(entries, p) }, - options, - }), - [updateTabContent, updateEntry, setToastMessage, entries], - ) + const runFrontmatterOp = useFrontmatterRunner({ activeTabPathRef, entries, setToastMessage, updateEntry, updateTabContent }) const frontmatterActions = useFrontmatterActionHandlers({ config, renameTabsRef: rename.tabsRef, diff --git a/tests/smoke/save-before-note-switch.spec.ts b/tests/smoke/save-before-note-switch.spec.ts index 7fc99aa2..a14c72cc 100644 --- a/tests/smoke/save-before-note-switch.spec.ts +++ b/tests/smoke/save-before-note-switch.spec.ts @@ -21,6 +21,13 @@ async function openRawMode(page: Page) { await expect(page.locator('.cm-content')).toBeVisible({ timeout: 5_000 }) } +async function openPropertiesPanel(page: Page) { + const openPanelButton = page.getByRole('button', { name: 'Open the properties panel' }) + if (await openPanelButton.count()) { + await openPanelButton.click() + } +} + async function getRawEditorContent(page: Page): Promise { return page.evaluate(() => { const el = document.querySelector('.cm-content') @@ -81,3 +88,26 @@ test('@smoke switching notes persists unsaved raw edits without waiting for the { timeout: 450, intervals: [50, 100, 100, 100, 100] }, ).toContain(appendedText) }) + +test('@smoke deleting a property after switching notes keeps the current note editable', async ({ page }) => { + const alphaPath = path.join(tempVaultDir, 'project', 'alpha-project.md') + + await openNote(page, 'Note B') + await openPropertiesPanel(page) + await expect(page.getByTestId('editable-property').filter({ hasText: 'Status' })).toBeVisible() + + await openNote(page, 'Alpha Project') + await expect(page.getByTestId('breadcrumb-filename-trigger')).toContainText('alpha-project', { timeout: 5_000 }) + + const ownerRow = page.getByTestId('editable-property').filter({ hasText: 'Owner' }) + await expect(ownerRow).toBeVisible() + await ownerRow.hover() + await ownerRow.getByTitle('Delete property').click({ force: true }) + + await expect(ownerRow).toHaveCount(0) + await expect(page.getByTestId('breadcrumb-filename-trigger')).toContainText('alpha-project') + await expect.poll( + () => fs.readFileSync(alphaPath, 'utf8'), + { timeout: 1_000, intervals: [50, 100, 200, 300, 350] }, + ).not.toContain('Owner:') +})