From be572c1692cd950ef7432702664936807a14275c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 30 Jul 2026 11:29:37 +0000 Subject: [PATCH] fix(files): address autosave migration and review regressions Seed omitted autoSaveEnabled from the hydrated client preference (including legacy localStorage) instead of resetting everyone to enabled. Restore SVG non-editable flags, treat clean draft saves as success, and throw again from disposed content-cache owners while keeping runtime-switch cache invalidation. Co-authored-by: Serhii Dziupin --- .../ui/src/components/views/FilesView.tsx | 11 +++++-- .../src/contexts/content-cache-owner.test.ts | 24 +++----------- .../ui/src/contexts/content-cache-owner.ts | 18 ++++++----- .../ui/src/lib/fileEditorAutosave.test.ts | 4 +-- packages/ui/src/lib/fileEditorAutosave.ts | 10 ++++-- packages/ui/src/lib/persistence.test.ts | 31 +++++++++++++++++-- packages/ui/src/lib/persistence.ts | 26 +++++++++++++--- 7 files changed, 85 insertions(+), 39 deletions(-) diff --git a/packages/ui/src/components/views/FilesView.tsx b/packages/ui/src/components/views/FilesView.tsx index d59b163a..fec12654 100644 --- a/packages/ui/src/components/views/FilesView.tsx +++ b/packages/ui/src/components/views/FilesView.tsx @@ -1638,6 +1638,11 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { return false; } + // Clean draft: treat as success so discard/save dialogs and Ctrl+S are not stranded. + if (!isDirty) { + return true; + } + setIsSaving(true); try { @@ -2351,12 +2356,14 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { const canCopy = Boolean(selectedFile && (!isSelectedImage || isSelectedSvg) && !isSelectedPdf && !isUnsupportedBinary && fileContent.length > 0); const canCopyPath = Boolean(selectedFile && displaySelectedPath.length > 0); - const canEdit = Boolean(selectedFile && !selectedFileIsOutsideWorkspace && !isSelectedBinary && files.writeFile && fileContent.length <= MAX_VIEW_CHARS); + // Keep image/SVG on the preview path: `isBinaryFile` excludes `.svg`, so binary + // alone would flip canEdit/isTextFile true and show a dead edit toggle + no-op Save. + const canEdit = Boolean(selectedFile && !selectedFileIsOutsideWorkspace && !isSelectedBinary && !isSelectedImage && files.writeFile && fileContent.length <= MAX_VIEW_CHARS); const isMarkdown = Boolean(selectedFile?.path && isMarkdownFile(selectedFile.path)); const isJson = Boolean(selectedFile?.path && isJsonFile(selectedFile.path)); const isHtml = Boolean(selectedFile?.path && isHtmlFile(selectedFile.path)); const isDrawio = Boolean(selectedFile?.path && isDrawioFile(selectedFile.path)); - const isTextFile = Boolean(selectedFile && !isSelectedBinary); + const isTextFile = Boolean(selectedFile && !isSelectedBinary && !isSelectedImage); const canUseShikiFileView = isTextFile && !isMarkdown && !isDrawio && !(isHtml && htmlViewMode === 'preview'); const isEditingFile = (isMarkdown && mdViewMode === 'edit') || (isHtml && htmlViewMode === 'edit') diff --git a/packages/ui/src/contexts/content-cache-owner.test.ts b/packages/ui/src/contexts/content-cache-owner.test.ts index 5bd7154a..a9095493 100644 --- a/packages/ui/src/contexts/content-cache-owner.test.ts +++ b/packages/ui/src/contexts/content-cache-owner.test.ts @@ -72,29 +72,15 @@ describe("content cache owner", () => { owner.dispose() }) - test("disposed owners still read through without throwing", async () => { - let reads = 0 + test("disposed owners throw on subsequent reads", async () => { const owner = createContentCachedFiles({ - readFile: async (path: string) => ({ path, content: `value-${++reads}` }), - statFile: async () => ({ isFile: true, isDirectory: false, size: 7, mtimeMs: 1 }), + readFile: async (path: string) => ({ path, content: "value" }), + statFile: async () => ({ isFile: true, isDirectory: false, size: 5, mtimeMs: 1 }), } as unknown as FilesAPI) owner.dispose() - expect((await owner.files.readFile!("notes.txt", { optional: true, directory: "/tmp/project" })).content).toBe("value-1") - expect(reads).toBe(1) - }) - - test("validateContextFileOpen succeeds against a disposed cached files API", async () => { - const { validateContextFileOpen } = await import("@/lib/contextFileOpenGuard") - const owner = createContentCachedFiles({ - listDirectory: async () => ({ directory: "/", entries: [] }), - readFile: async (path: string) => ({ path, content: "hello from notes\n" }), - } as unknown as FilesAPI) - - owner.dispose() - expect(await validateContextFileOpen(owner.files, "/tmp/project/notes.txt", { directory: "/tmp/project" })).toEqual({ - ok: true, - }) + await expect(owner.files.readFile!("notes.txt", { optional: true, directory: "/tmp/project" })) + .rejects.toThrow("File cache owner disposed") }) test("runtime endpoint changes clear cache but keep serving reads", async () => { diff --git a/packages/ui/src/contexts/content-cache-owner.ts b/packages/ui/src/contexts/content-cache-owner.ts index 065699f5..0a027ab4 100644 --- a/packages/ui/src/contexts/content-cache-owner.ts +++ b/packages/ui/src/contexts/content-cache-owner.ts @@ -5,6 +5,14 @@ const MAX_ENTRIES = 40; const MAX_BYTES = 20 * 1024 * 1024; type Entry = { content: string; path: string; sourcePath: string; size: number; mtimeMs: number; bytes: number }; +/** + * Content-cached `FilesAPI.readFile` wrapper. + * + * Lifecycle: `RuntimeAPIProvider` owns create/dispose in an effect so React + * Strict Mode remounts get a fresh owner. After `dispose()`, reads throw — + * callers must not keep using a torn-down owner. Runtime endpoint switches + * bump generation and clear the cache without deactivating the owner. + */ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; dispose: () => void } { const cache = new Map(); let totalBytes = 0; @@ -65,8 +73,7 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di const before = await files.statFile?.(path, options).catch(() => null); const result = await files.readFile!(path, options); const after = await files.statFile?.(path, options).catch(() => null); - // Disposed mid-read: still return the bytes we fetched; do not cache. - if (!active) return result; + if (!active) throw new Error('File cache owner disposed'); if (capturedGeneration !== generation) return cachedReadFile!(path, options); const stable = before && after && before.isFile && after.isFile && before.mtimeMs !== undefined && after.mtimeMs !== undefined @@ -77,17 +84,14 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di const cachedReadFile: FilesAPI['readFile'] = files.readFile ? async (path, options) => { await mutationBarrier; - // Disposed owners must keep serving reads. React Strict Mode can dispose a - // memoized owner that the provider still holds; throwing here surfaces as - // "Failed to open file" with no /api/fs/read request for text opens. - if (!active) return files.readFile!(path, options); + if (!active) throw new Error('File cache owner disposed'); const capturedGeneration = generation; if (options?.allowOutsideWorkspace) return files.readFile!(path, options); const key = cacheKey(path, options); const hit = cache.get(key); if (!hit) return readFresh(key, path, options, capturedGeneration); const latest = await files.statFile?.(path, options).catch(() => null); - if (!active) return files.readFile!(path, options); + if (!active) throw new Error('File cache owner disposed'); if (capturedGeneration !== generation) return cachedReadFile!(path, options); if (!latest || !metadataMatches(hit, latest)) { removeEntry(key); diff --git a/packages/ui/src/lib/fileEditorAutosave.test.ts b/packages/ui/src/lib/fileEditorAutosave.test.ts index 1b78c1da..66218ade 100644 --- a/packages/ui/src/lib/fileEditorAutosave.test.ts +++ b/packages/ui/src/lib/fileEditorAutosave.test.ts @@ -51,10 +51,10 @@ describe('shouldAllowFileDraftSave', () => { expect(shouldAllowFileDraftSave(ready)).toBe(true); }); - test('refuses incomplete load, binary, or clean draft', () => { + test('refuses incomplete load or binary; clean draft is a successful no-op', () => { expect(shouldAllowFileDraftSave({ ...ready, fileLoading: true })).toBe(false); expect(shouldAllowFileDraftSave({ ...ready, loadedFilePath: null })).toBe(false); expect(shouldAllowFileDraftSave({ ...ready, isNonEditableBinary: true })).toBe(false); - expect(shouldAllowFileDraftSave({ ...ready, isDirty: false })).toBe(false); + expect(shouldAllowFileDraftSave({ ...ready, isDirty: false })).toBe(true); }); }); diff --git a/packages/ui/src/lib/fileEditorAutosave.ts b/packages/ui/src/lib/fileEditorAutosave.ts index 6081877b..dbcdbe85 100644 --- a/packages/ui/src/lib/fileEditorAutosave.ts +++ b/packages/ui/src/lib/fileEditorAutosave.ts @@ -38,12 +38,18 @@ export type FileEditorSaveDraftGate = { }; /** - * Whether saveDraft may write. Refuses empty drafts against stale content and any binary target. + * Whether saveDraft may proceed. + * - Clean drafts return true ("nothing to save" is success) so callers like the + * unsaved-changes dialog and Ctrl+S do not treat a no-op as failure. + * - Incomplete loads and binary targets return false (refused). */ export function shouldAllowFileDraftSave(gate: FileEditorSaveDraftGate): boolean { - if (!gate.selectedFilePath || !gate.isDirty) { + if (!gate.selectedFilePath) { return false; } + if (!gate.isDirty) { + return true; + } if (gate.fileLoading || gate.loadedFilePath !== gate.selectedFilePath || gate.isNonEditableBinary) { return false; } diff --git a/packages/ui/src/lib/persistence.test.ts b/packages/ui/src/lib/persistence.test.ts index 8921969a..38057e9c 100644 --- a/packages/ui/src/lib/persistence.test.ts +++ b/packages/ui/src/lib/persistence.test.ts @@ -528,16 +528,43 @@ describe('updateDesktopSettings', () => { expect(saveCalls.some((changes) => changes.autoSaveEnabled === false)).toBe(true); }); - test('resets omitted autoSaveEnabled to the default enabled state', async () => { + test('seeds omitted autoSaveEnabled from the hydrated client preference', async () => { getWindow(); + invalidateSettingsCache(); useUIStore.getState().setAutoSaveEnabled(false); - registerSettingsApi(async () => ({}), async () => ({ + const saveCalls: Array> = []; + registerSettingsApi(async (changes) => { + saveCalls.push(changes); + return { ...changes } as SettingsPayload; + }, async () => ({ settings: { draftStartersCraftGoalAdded: true, draftStartersScheduleTaskAdded: true }, source: 'web', })); await syncDesktopSettings(); + await delay(500); + + expect(useUIStore.getState().autoSaveEnabled).toBe(false); + expect(saveCalls.some((changes) => changes.autoSaveEnabled === false)).toBe(true); + }); + + test('seeds default autoSaveEnabled when omitted and client still has the default', async () => { + getWindow(); + invalidateSettingsCache(); + useUIStore.getState().setAutoSaveEnabled(true); + const saveCalls: Array> = []; + registerSettingsApi(async (changes) => { + saveCalls.push(changes); + return { ...changes } as SettingsPayload; + }, async () => ({ + settings: { draftStartersCraftGoalAdded: true, draftStartersScheduleTaskAdded: true }, + source: 'web', + })); + + await syncDesktopSettings(); + await delay(500); expect(useUIStore.getState().autoSaveEnabled).toBe(true); + expect(saveCalls.some((changes) => changes.autoSaveEnabled === true)).toBe(true); }); }); diff --git a/packages/ui/src/lib/persistence.ts b/packages/ui/src/lib/persistence.ts index 4a3d846e..08cfd5fb 100644 --- a/packages/ui/src/lib/persistence.ts +++ b/packages/ui/src/lib/persistence.ts @@ -1728,6 +1728,12 @@ export const syncDesktopSettings = async (): Promise => { if (!isSettingsRuntimeContextCurrent(context)) return; const shouldPersistCraftGoalMigration = settings.draftStartersCraftGoalAdded !== true || settings.draftStartersScheduleTaskAdded !== true; + // `autoSaveEnabled` is new to the settings backend. Until the server has a + // value, materialize would invent the client default (true) and overwrite a + // deliberate legacy "off" preference migrated from + // `openchamber:files:auto-save-enabled`. Prefer the hydrated store value and + // seed the backend once so later omitted→default authority is correct. + const shouldSeedAutoSaveEnabled = typeof settings.autoSaveEnabled !== 'boolean'; const authoritativeSettings = materializeAuthoritativeUiSettings(settings); try { persistToLocalStorage(settings); @@ -1736,6 +1742,9 @@ export const syncDesktopSettings = async (): Promise => { } await waitForHydration(); if (!isSettingsRuntimeContextCurrent(context)) return; + if (shouldSeedAutoSaveEnabled) { + authoritativeSettings.autoSaveEnabled = useUIStore.getState().autoSaveEnabled; + } if (settings.draftStarters === undefined) { useUIStore.setState({ globalDraftStarters: null }); } @@ -1744,12 +1753,19 @@ export const syncDesktopSettings = async (): Promise => { } catch (error) { console.warn('applyDesktopUiPreferences failed:', error); } + const migrationPatch: Partial = {}; if (shouldPersistCraftGoalMigration) { - await updateDesktopSettings({ - ...(authoritativeSettings.draftStarters ? { draftStarters: authoritativeSettings.draftStarters } : {}), - draftStartersCraftGoalAdded: true, - draftStartersScheduleTaskAdded: true, - }); + if (authoritativeSettings.draftStarters) { + migrationPatch.draftStarters = authoritativeSettings.draftStarters; + } + migrationPatch.draftStartersCraftGoalAdded = true; + migrationPatch.draftStartersScheduleTaskAdded = true; + } + if (shouldSeedAutoSaveEnabled) { + migrationPatch.autoSaveEnabled = authoritativeSettings.autoSaveEnabled; + } + if (Object.keys(migrationPatch).length > 0) { + await updateDesktopSettings(migrationPatch); if (!isSettingsRuntimeContextCurrent(context)) return; }