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 <makeittech@users.noreply.github.com>

packages/ui/src/components/views/FilesView.tsx

@@ -1638,6 +1638,11 @@ export const FilesView: React.FC<FilesViewProps> = ({ 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<FilesViewProps> = ({ 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')

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 () => {
+  test("disposed owners throw on subsequent reads", async () => {
-    let reads = 0
     const owner = createContentCachedFiles({
-      readFile: async (path: string) => ({ path, content: `value-${++reads}` }),
+      readFile: async (path: string) => ({ path, content: "value" }),
-      statFile: async () => ({ isFile: true, isDirectory: false, size: 7, mtimeMs: 1 }),
+      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")
+    await expect(owner.files.readFile!("notes.txt", { optional: true, directory: "/tmp/project" }))
-    expect(reads).toBe(1)
+      .rejects.toThrow("File cache owner disposed")
-  })
-
-  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,
-    })
   })
 
   test("runtime endpoint changes clear cache but keep serving reads", async () => {

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<string, Entry>();
   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) throw new Error('File cache owner disposed');
-    if (!active) return result;
     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
+        if (!active) throw new Error('File cache owner disposed');
-        // 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);
         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);

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);
   });
 });

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;
   }

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<Partial<SettingsPayload>> = [];
+    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<Partial<SettingsPayload>> = [];
+    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);
   });
 });

packages/ui/src/lib/persistence.ts

@@ -1728,6 +1728,12 @@ export const syncDesktopSettings = async (): Promise<void> => {
     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<void> => {
     }
     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<void> => {
     } catch (error) {
       console.warn('applyDesktopUiPreferences failed:', error);
     }
+    const migrationPatch: Partial<DesktopSettings> = {};
     if (shouldPersistCraftGoalMigration) {
-      await updateDesktopSettings({
+      if (authoritativeSettings.draftStarters) {
-        ...(authoritativeSettings.draftStarters ? { draftStarters: authoritativeSettings.draftStarters } : {}),
+        migrationPatch.draftStarters = authoritativeSettings.draftStarters;
-        draftStartersCraftGoalAdded: true,
+      }
-        draftStartersScheduleTaskAdded: true,
+      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;
     }