From 2f5912c2874754e109cc6257752a4f7729d0088a Mon Sep 17 00:00:00 2001 From: jwcrystal <121911854+jwcrystal@users.noreply.github.com> Date: Tue, 21 Apr 2026 23:04:24 +0800 Subject: [PATCH] fix(files): refresh open file content after external changes (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(files): refresh open file content after external edits Previously, opening a file in the Files view and then editing it externally (e.g. via CLI or another editor) would show stale content. Even closing and reopening the file returned cached content — a full page reload was required. Root causes: 1. The in-memory readFile cache used path-only hits, with no metadata validation. External edits were invisible until the cache was evicted. 2. No polling mechanism existed to detect external changes to the open file. Fix: - Add mtimeMs to statFile across all runtimes (web, VS Code, desktop). - Cache layer (RuntimeAPIProvider): validate cache hits against current stat metadata (mtimeMs + size). On miss, use stat→read→stat to avoid TOCTOU. - UI layer (FilesView): poll the open file every 2s; on detected change, set loadedFilePath=null to trigger the existing load effect once (no double reload). Skip polling when tab is hidden or editor has unsaved changes. - After save, refresh the stat ref so the next poll doesn't see a spurious change from the save itself. Addresses review feedback from PR #827 (double reload + TOCTOU). * fix(files): address P2 review findings - readFreshFile retry now uses stat→read→stat to maintain TOCTOU protection during the retry path (not just the initial read). - Replace isDirty in polling effect deps with isDirtyRef to avoid unnecessary interval teardown/restart on every edit/save cycle. --- .../ui/src/components/views/FilesView.tsx | 97 ++++++++++++++++++- .../ui/src/contexts/RuntimeAPIProvider.tsx | 90 ++++++++++++++--- packages/ui/src/lib/api/types.ts | 2 +- packages/vscode/src/bridge-fs-runtime.ts | 1 + packages/vscode/webview/api/files.ts | 5 +- packages/web/server/lib/fs/routes.js | 2 +- packages/web/src/api/files.ts | 3 +- 7 files changed, 179 insertions(+), 21 deletions(-) diff --git a/packages/ui/src/components/views/FilesView.tsx b/packages/ui/src/components/views/FilesView.tsx index 7ab196e2..dab7ca74 100644 --- a/packages/ui/src/components/views/FilesView.tsx +++ b/packages/ui/src/components/views/FilesView.tsx @@ -92,6 +92,12 @@ type FileNode = { relativePath?: string; }; +type FileStatSnapshot = { + path: string; + size: number; + mtimeMs?: number; +}; + type SelectedLineRange = { start: number; end: number; @@ -623,6 +629,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { const [draftContent, setDraftContent] = React.useState(''); const [isSaving, setIsSaving] = React.useState(false); const autoSaveTimerRef = React.useRef | null>(null); + const lastLoadedFileStatRef = React.useRef(null); const [autoSaveStatus, setAutoSaveStatus] = React.useState<'idle' | 'saved'>('idle'); const [confirmDiscardOpen, setConfirmDiscardOpen] = React.useState(false); @@ -1213,6 +1220,18 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { return response.text(); }, [files]); + const readFileStat = React.useCallback(async (path: string): Promise => { + if (files.statFile) { + const result = await files.statFile(path); + return { + path: result.path, + size: result.size, + mtimeMs: result.mtimeMs, + }; + } + return null; + }, [files]); + const displayedContent = React.useMemo(() => { return fileContent.length > MAX_VIEW_CHARS ? `${fileContent.slice(0, MAX_VIEW_CHARS)}\n\n… truncated …` @@ -1240,6 +1259,14 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { return; } setFileContent(draftContent); + // Refresh stat after write so polling doesn't see a stale metadata change. + void readFileStat(selectedFile.path) + .then((stat) => { + if (stat) { + lastLoadedFileStatRef.current = stat; + } + }) + .catch(() => {}); }) .catch((error) => { toast.error(error instanceof Error ? error.message : 'Save failed'); @@ -1247,7 +1274,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { .finally(() => { setIsSaving(false); }); - }, [draftContent, files, isDirty, selectedFile]); + }, [draftContent, files, isDirty, readFileStat, selectedFile]); React.useEffect(() => { if (!isDirty) { @@ -1371,6 +1398,13 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { ? `${content.slice(0, MAX_VIEW_CHARS)}\n\n… truncated …` : content); setLoadedFilePath(node.path); + void readFileStat(node.path) + .then((stat) => { + if (stat) { + lastLoadedFileStatRef.current = stat; + } + }) + .catch(() => {}); }) .catch((error) => { if (isDirectoryReadError(error)) { @@ -1381,6 +1415,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { setFileContent(''); setDraftContent(''); setLoadedFilePath(null); + lastLoadedFileStatRef.current = null; if (searchQuery.trim().length > 0) { setSearchQuery(''); } @@ -1404,11 +1439,12 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { setFileContent(''); setDraftContent(''); setFileError(error instanceof Error ? error.message : 'Failed to read file'); + lastLoadedFileStatRef.current = null; }) .finally(() => { setFileLoading(false); }); - }, [expandPaths, isMobile, loadDirectory, readFile, root, runtime.isDesktop, searchQuery, setSelectedPath]); + }, [expandPaths, isMobile, loadDirectory, readFile, readFileStat, root, runtime.isDesktop, searchQuery, setSelectedPath]); const ensurePathVisible = React.useCallback(async (targetPath: string, includeTarget: boolean) => { if (!root) { @@ -1483,6 +1519,63 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { void loadSelectedFile(selectedFile); }, [loadSelectedFile, loadedFilePath, selectedFile]); + // Sync isDirty to a ref so the polling interval can read the latest value + // without isDirty in its dependency array (avoids interval restart on every edit/save). + const isDirtyRef = React.useRef(isDirty); + isDirtyRef.current = isDirty; + + // Poll open file for external changes. + // When a change is detected, reset loadedFilePath so the effect above + // triggers a single reload — no double-load. + React.useEffect(() => { + if (!selectedFile?.path || loadedFilePath !== selectedFile.path) { + return; + } + + let cancelled = false; + const interval = window.setInterval(() => { + if (document.hidden) { + return; + } + + void readFileStat(selectedFile.path) + .then((latestStat) => { + if (cancelled || !latestStat) { + return; + } + + const previousStat = lastLoadedFileStatRef.current; + if (!previousStat || previousStat.path !== selectedFile.path) { + lastLoadedFileStatRef.current = latestStat; + return; + } + + const changedByMtime = latestStat.mtimeMs !== undefined + && previousStat.mtimeMs !== undefined + && latestStat.mtimeMs !== previousStat.mtimeMs; + const changedBySize = latestStat.size !== previousStat.size; + + if (!changedByMtime && !changedBySize) { + return; + } + + if (isDirtyRef.current) { + return; + } + + lastLoadedFileStatRef.current = latestStat; + // Reset loadedFilePath so the effect above triggers a single reload. + setLoadedFilePath(null); + }) + .catch(() => {}); + }, 2000); + + return () => { + cancelled = true; + window.clearInterval(interval); + }; + }, [loadedFilePath, readFileStat, selectedFile?.path]); + const discardAndContinue = React.useCallback(() => { const nextFile = pendingSelectFileRef.current; const nextTab = pendingTabRef.current; diff --git a/packages/ui/src/contexts/RuntimeAPIProvider.tsx b/packages/ui/src/contexts/RuntimeAPIProvider.tsx index 8badbaba..01c80fdb 100644 --- a/packages/ui/src/contexts/RuntimeAPIProvider.tsx +++ b/packages/ui/src/contexts/RuntimeAPIProvider.tsx @@ -11,28 +11,90 @@ import { /** Wrap a FilesAPI with an in-memory LRU content cache. */ function withContentCache(files: FilesAPI): FilesAPI { - const cache = new Map(); + const cache = new Map(); + + /** Whether cached metadata still matches the file on disk. */ + const statMatches = ( + cached: { size?: number; mtimeMs?: number }, + latest: { isFile: boolean; size: number; mtimeMs?: number }, + ): boolean => { + if (!latest.isFile) return false; + // If mtimeMs is available on both sides, it is the strongest signal. + if (cached.mtimeMs !== undefined && latest.mtimeMs !== undefined) { + return cached.mtimeMs === latest.mtimeMs && cached.size === latest.size; + } + return cached.size === latest.size; + }; + + const syncCacheEntry = ( + path: string, + result: { content: string; path: string }, + stat?: { isFile: boolean; size: number; mtimeMs?: number } | null, + ): { content: string; path: string } => { + const bytes = approxStringBytes(result.content); + cache.set(path, { + ...result, + size: stat?.isFile ? stat.size : undefined, + mtimeMs: stat?.isFile ? stat.mtimeMs : undefined, + }); + setContentBytes(path, bytes); + + const keep = new Set(); + evictContentLru(keep, (evictPath) => { + cache.delete(evictPath); + }); + + return result; + }; + + const readFreshFile = async (path: string): Promise<{ content: string; path: string }> => { + // stat → read → stat to avoid TOCTOU: + // if the file changes between read and either stat, metadata won't match and we retry. + const statBefore = await files.statFile?.(path).catch(() => null); + + const result = await files.readFile!(path); + + const statAfter = await files.statFile?.(path).catch(() => null); + + // If both stats are available and agree, the read was atomic with respect to file changes. + if (statBefore && statAfter && statBefore.isFile && statAfter.isFile) { + if (statBefore.size === statAfter.size && statBefore.mtimeMs === statAfter.mtimeMs) { + return syncCacheEntry(path, result, statAfter); + } + // File changed during read — discard and re-read once. + const retryStatBefore = await files.statFile?.(path).catch(() => null); + const retry = await files.readFile!(path); + const retryStat = await files.statFile?.(path).catch(() => null); + // Accept retry only if file was stable across the read. + if (retryStatBefore && retryStat && retryStatBefore.isFile && retryStat.isFile + && retryStatBefore.size === retryStat.size && retryStatBefore.mtimeMs === retryStat.mtimeMs) { + return syncCacheEntry(path, retry, retryStat); + } + // Best-effort: file was still changing, cache what we got. Next hit will re-validate. + return syncCacheEntry(path, retry, retryStat); + } + + return syncCacheEntry(path, result, statAfter ?? statBefore); + }; const cachedReadFile: FilesAPI['readFile'] = files.readFile ? async (path: string) => { const hit = cache.get(path); if (hit) { + // Validate cached entry is still fresh + if (files.statFile) { + const latest = await files.statFile(path).catch(() => null); + if (latest && !statMatches(hit, latest)) { + cache.delete(path); + removeContentBytes(path); + return readFreshFile(path); + } + } touchContentLru(path); - return hit; + return { content: hit.content, path: hit.path }; } - const result = await files.readFile!(path); - const bytes = approxStringBytes(result.content); - cache.set(path, result); - setContentBytes(path, bytes); - - // Evict if over limits - const keep = new Set(); - evictContentLru(keep, (evictPath) => { - cache.delete(evictPath); - }); - - return result; + return readFreshFile(path); } : undefined; diff --git a/packages/ui/src/lib/api/types.ts b/packages/ui/src/lib/api/types.ts index 0876f509..631f4950 100644 --- a/packages/ui/src/lib/api/types.ts +++ b/packages/ui/src/lib/api/types.ts @@ -513,7 +513,7 @@ export interface FilesAPI { listDirectory(path: string, options?: ListDirectoryOptions): Promise; search(payload: FileSearchQuery): Promise; createDirectory(path: string): Promise<{ success: boolean; path: string }>; - statFile?(path: string): Promise<{ path: string; isFile: boolean; size: number }>; + statFile?(path: string): Promise<{ path: string; isFile: boolean; size: number; mtimeMs?: number }>; readFile?(path: string): Promise<{ content: string; path: string }>; readFileBinary?(path: string): Promise<{ dataUrl: string; path: string }>; writeFile?(path: string, content: string): Promise<{ success: boolean; path: string }>; diff --git a/packages/vscode/src/bridge-fs-runtime.ts b/packages/vscode/src/bridge-fs-runtime.ts index 5f0ab712..b9ae8708 100644 --- a/packages/vscode/src/bridge-fs-runtime.ts +++ b/packages/vscode/src/bridge-fs-runtime.ts @@ -205,6 +205,7 @@ export async function handleFsBridgeMessage( path: deps.normalizeFsPath(resolution.resolvedPath), isFile: true, size: stats.size, + mtimeMs: stats.mtimeMs, }, }; } catch (error) { diff --git a/packages/vscode/webview/api/files.ts b/packages/vscode/webview/api/files.ts index d93562f9..f778fd0b 100644 --- a/packages/vscode/webview/api/files.ts +++ b/packages/vscode/webview/api/files.ts @@ -71,13 +71,14 @@ export const createVSCodeFilesAPI = (): FilesAPI => ({ }; }, - async statFile(path: string): Promise<{ path: string; isFile: boolean; size: number }> { + async statFile(path: string): Promise<{ path: string; isFile: boolean; size: number; mtimeMs?: number }> { const target = normalizePath(path); - const data = await sendBridgeMessage<{ path?: string; isFile?: boolean; size?: number }>('api:fs:stat', { path: target }); + const data = await sendBridgeMessage<{ path?: string; isFile?: boolean; size?: number; mtimeMs?: number }>('api:fs:stat', { path: target }); return { path: typeof data?.path === 'string' ? normalizePath(data.path) : target, isFile: Boolean(data?.isFile), size: typeof data?.size === 'number' ? data.size : 0, + mtimeMs: typeof data?.mtimeMs === 'number' ? data.mtimeMs : undefined, }; }, diff --git a/packages/web/server/lib/fs/routes.js b/packages/web/server/lib/fs/routes.js index 2fafb5cc..3e1f08de 100644 --- a/packages/web/server/lib/fs/routes.js +++ b/packages/web/server/lib/fs/routes.js @@ -317,7 +317,7 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(400).json({ error: 'Specified path is not a file' }); } - return res.json({ path: canonicalPath, isFile: true, size: stats.size }); + return res.json({ path: canonicalPath, isFile: true, size: stats.size, mtimeMs: stats.mtimeMs }); } catch (error) { const err = error; if (err && typeof err === 'object' && err.code === 'ENOENT') { diff --git a/packages/web/src/api/files.ts b/packages/web/src/api/files.ts index a4f5e81f..f3bb20d5 100644 --- a/packages/web/src/api/files.ts +++ b/packages/web/src/api/files.ts @@ -110,7 +110,7 @@ export const createWebFilesAPI = (): FilesAPI => ({ }; }, - async statFile(path: string): Promise<{ path: string; isFile: boolean; size: number }> { + async statFile(path: string): Promise<{ path: string; isFile: boolean; size: number; mtimeMs?: number }> { const target = normalizePath(path); const response = await fetch(`/api/fs/stat?path=${encodeURIComponent(target)}`); @@ -124,6 +124,7 @@ export const createWebFilesAPI = (): FilesAPI => ({ path: typeof (result as { path?: string }).path === 'string' ? normalizePath((result as { path: string }).path) : target, isFile: Boolean((result as { isFile?: boolean }).isFile), size: typeof (result as { size?: number }).size === 'number' ? (result as { size: number }).size : 0, + mtimeMs: typeof (result as { mtimeMs?: number }).mtimeMs === 'number' ? (result as { mtimeMs: number }).mtimeMs : undefined, }; },