diff --git a/packages/ui/src/contexts/RuntimeAPIProvider.tsx b/packages/ui/src/contexts/RuntimeAPIProvider.tsx index f91a4abe..46ec8376 100644 --- a/packages/ui/src/contexts/RuntimeAPIProvider.tsx +++ b/packages/ui/src/contexts/RuntimeAPIProvider.tsx @@ -1,17 +1,32 @@ import React, { type JSX, type ReactNode } from 'react'; import { RuntimeAPIContext } from '@/contexts/runtimeAPIContext'; -import type { RuntimeAPIs } from '@/lib/api/types'; +import type { FilesAPI, RuntimeAPIs } from '@/lib/api/types'; import { createContentCachedFiles } from '@/contexts/content-cache-owner'; +type ContentCachedFiles = ReturnType; + export function RuntimeAPIProvider({ apis, children }: { apis: RuntimeAPIs; children: ReactNode }): JSX.Element { - const cachedFiles = React.useMemo(() => createContentCachedFiles(apis.files), [apis.files]); - React.useEffect(() => () => cachedFiles.dispose(), [cachedFiles]); + // Effect-owned lifecycle: React Strict Mode dispose+remount must create a fresh + // owner. useMemo + dispose reused a dead owner and broke text-file opens + // (binaries skipped the pre-read, so they still appeared to work). + const [cachedOwner, setCachedOwner] = React.useState(null); + + React.useEffect(() => { + const owner = createContentCachedFiles(apis.files); + setCachedOwner(owner); + return () => { + owner.dispose(); + setCachedOwner((current) => (current === owner ? null : current)); + }; + }, [apis.files]); + + const files: FilesAPI = cachedOwner?.files ?? apis.files; const cachedApis = React.useMemo( () => ({ ...apis, - files: cachedFiles.files, + files, }), - [apis, cachedFiles], + [apis, files], ); return {children}; } diff --git a/packages/ui/src/contexts/content-cache-owner.test.ts b/packages/ui/src/contexts/content-cache-owner.test.ts index 31547064..5bd7154a 100644 --- a/packages/ui/src/contexts/content-cache-owner.test.ts +++ b/packages/ui/src/contexts/content-cache-owner.test.ts @@ -71,4 +71,68 @@ describe("content cache owner", () => { expect(second.content).toBe("/b-2") owner.dispose() }) + + test("disposed owners still read through without throwing", async () => { + let reads = 0 + const owner = createContentCachedFiles({ + readFile: async (path: string) => ({ path, content: `value-${++reads}` }), + statFile: async () => ({ isFile: true, isDirectory: false, size: 7, 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, + }) + }) + + test("runtime endpoint changes clear cache but keep serving reads", async () => { + const originalWindow = globalThis.window + const events = new EventTarget() + Object.defineProperty(globalThis, "window", { + configurable: true, + value: { + addEventListener: events.addEventListener.bind(events), + removeEventListener: events.removeEventListener.bind(events), + dispatchEvent: events.dispatchEvent.bind(events), + }, + }) + + try { + let reads = 0 + const owner = createContentCachedFiles({ + readFile: async (path: string) => ({ path, content: `value-${++reads}` }), + statFile: async () => ({ isFile: true, isDirectory: false, size: 7, mtimeMs: 1 }), + } as unknown as FilesAPI) + + expect((await owner.files.readFile!("notes.txt")).content).toBe("value-1") + window.dispatchEvent(new CustomEvent("openchamber:runtime-endpoint-will-change", { + detail: { + apiBaseUrl: "http://127.0.0.1:3902", + previousApiBaseUrl: "http://127.0.0.1:3901", + runtimeKey: "url:http://127.0.0.1:3902", + previousRuntimeKey: "url:http://127.0.0.1:3901", + }, + })) + expect((await owner.files.readFile!("notes.txt")).content).toBe("value-2") + expect(reads).toBe(2) + owner.dispose() + } finally { + Object.defineProperty(globalThis, "window", { + configurable: true, + value: originalWindow, + }) + } + }) }) diff --git a/packages/ui/src/contexts/content-cache-owner.ts b/packages/ui/src/contexts/content-cache-owner.ts index 34b8e8aa..065699f5 100644 --- a/packages/ui/src/contexts/content-cache-owner.ts +++ b/packages/ui/src/contexts/content-cache-owner.ts @@ -32,6 +32,10 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di && cached.mtimeMs === latest.mtimeMs && cached.size === latest.size ); + const clearCache = () => { + cache.clear(); + totalBytes = 0; + }; const cacheResult = ( key: string, sourcePath: string, @@ -61,7 +65,8 @@ 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); - if (!active) throw new Error('File read invalidated by runtime change'); + // Disposed mid-read: still return the bytes we fetched; do not cache. + 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 @@ -72,14 +77,17 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di const cachedReadFile: FilesAPI['readFile'] = files.readFile ? async (path, options) => { await mutationBarrier; - if (!active) throw new Error('File cache owner disposed'); + // 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); 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) throw new Error('File read invalidated by runtime change'); + if (!active) return files.readFile!(path, options); if (capturedGeneration !== generation) return cachedReadFile!(path, options); if (!latest || !metadataMatches(hit, latest)) { removeEntry(key); @@ -116,18 +124,18 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di }; const unsubscribeRuntime = subscribeRuntimeEndpointWillChange((detail) => { if (detail.runtimeKey === detail.previousRuntimeKey) return; - active = false; + // Invalidate cached content for the previous runtime, but keep serving reads. + // `apis.files` is typically stable across endpoint switches, so permanently + // deactivating this owner would break every subsequent text-file open. generation += 1; - cache.clear(); - totalBytes = 0; + clearCache(); }); return { files: cachedFiles, dispose: () => { active = false; generation += 1; - cache.clear(); - totalBytes = 0; + clearCache(); unsubscribeRuntime(); }, };