From 7ea24e3c507b039479235da6470f5493b6163d87 Mon Sep 17 00:00:00 2001 From: Ibrahim Khan <2005ibrahimkhan@gmail.com> Date: Sat, 5 Sep 2026 09:31:35 -0600 Subject: [PATCH] fix(files): stop file viewer reload loop from sub-ms mtime jitter (#1489) (#2297) * fix(files): guard file polling races - Ignore sub-millisecond mtime jitter on a same-size file so an unchanged open file no longer loops through reload and flickers. - Swap externally changed text content into the open editor in place instead of clearing the loaded path and showing the load spinner. - Read content only after metadata changed, confirm it with a second read, and skip the swap when the file is unchanged, the buffer is dirty, or a newer local write landed. - Keep the stat baseline unchanged when a poll cannot observe content so a failed read is retried rather than treated as unchanged. - Fall back to a full reload for images, PDFs, binaries, and files above the content-poll byte limit, and when a poll returns binary content. - Serialize polls and dispose the poller on unmount, file switch, and directory change. - Add a `fresh` file read option that bypasses the content cache and the HTTP cache. * fix(files): invalidate stale polls after diagram saves --------- Co-authored-by: Bohdan Triapitsyn --- .../ui/src/components/views/FilesView.tsx | 117 +++++++--- .../views/fileContentPoller.test.ts | 200 ++++++++++++++++++ .../src/components/views/fileContentPoller.ts | 43 ++++ .../components/views/fileStatChange.test.ts | 17 ++ .../ui/src/components/views/fileStatChange.ts | 16 ++ .../src/contexts/content-cache-owner.test.ts | 19 ++ .../ui/src/contexts/content-cache-owner.ts | 4 + packages/ui/src/lib/api/types.ts | 1 + packages/web/src/api/files.test.ts | 9 + packages/web/src/api/files.ts | 2 +- 10 files changed, 397 insertions(+), 31 deletions(-) create mode 100644 packages/ui/src/components/views/fileContentPoller.test.ts create mode 100644 packages/ui/src/components/views/fileContentPoller.ts create mode 100644 packages/ui/src/components/views/fileStatChange.test.ts create mode 100644 packages/ui/src/components/views/fileStatChange.ts diff --git a/packages/ui/src/components/views/FilesView.tsx b/packages/ui/src/components/views/FilesView.tsx index 20060b51..f82aa4d3 100644 --- a/packages/ui/src/components/views/FilesView.tsx +++ b/packages/ui/src/components/views/FilesView.tsx @@ -26,6 +26,8 @@ import { CodeMirrorEditor } from '@/components/ui/CodeMirrorEditor'; import { GoToLineDialog } from './GoToLineDialog'; import { MarkdownPreviewSearch } from './MarkdownPreviewSearch'; import { PreviewToggleButton } from './PreviewToggleButton'; +import { createFileContentPoller } from './fileContentPoller'; +import { hasFileStatChanged } from './fileStatChange'; import { JsonTreeView } from '@/components/ui/JsonTreeView'; import { SimpleMarkdownRenderer } from '@/components/chat/MarkdownRenderer'; import { languageByExtension, loadLanguageByExtension } from '@/lib/codemirror/languageByExtension'; @@ -314,6 +316,7 @@ const isFileMissingError = (error: unknown): boolean => { }; const MAX_VIEW_CHARS = 200_000; +const MAX_CONTENT_POLL_BYTES = 200_000; type FileLineEnding = '\n' | '\r\n'; // Fast cache key for pierre's line/highlight caches: content-derived (not a @@ -931,6 +934,8 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { const pendingDrawioPreviewFrameRef = React.useRef(null); const diagramEditorRef = React.useRef>(null); const lastLoadedFileStatRef = React.useRef(null); + const lastLoadedFileContentRef = React.useRef(''); + const lastLoadedFileRevisionRef = React.useRef(0); const activeFileLoadIdRef = React.useRef(0); const loadingFilePathRef = React.useRef(null); const [fileContentRevision, setFileContentRevision] = React.useState(0); @@ -1573,10 +1578,16 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { }; }, [currentDirectory, debouncedSearchQuery, searchFiles, showHidden, showGitignored]); - const readFile = React.useCallback(async (path: string): Promise => { + // `fresh` bypasses the content cache and HTTP cache so external-change polling + // compares against the file on disk rather than a cached copy. + const readFile = React.useCallback(async (path: string, cacheOptions?: { fresh?: boolean }): Promise => { const options = await resolveFileReadOptions(path); if (files.readFile) { - const result = await files.readFile(path, { ...options, directory: root || undefined }); + const result = await files.readFile(path, { + ...options, + directory: root || undefined, + ...cacheOptions, + }); return result.content ?? ''; } @@ -1590,7 +1601,10 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { if (root) { params.set('directory', root); } - const response = await runtimeFetch(`/api/fs/read?${params.toString()}`); + const response = await runtimeFetch( + `/api/fs/read?${params.toString()}`, + cacheOptions?.fresh ? { cache: 'no-store' } : undefined, + ); if (!response.ok) { const error = await response.json().catch(() => ({ error: response.statusText })); throw new Error((error as { error?: string }).error || t('filesView.error.readFileFailed')); @@ -1690,6 +1704,8 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { return false; } setFileContent(draftContent); + lastLoadedFileContentRef.current = contentToWrite; + lastLoadedFileRevisionRef.current += 1; if (root && isPathWithinRoot(selectedFile.path, root)) { const relativePath = getDisplayPath(root, selectedFile.path); if (relativePath) { @@ -1819,6 +1835,19 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { }, }); + const applyLoadedTextContent = React.useCallback((content: string) => { + const editorContent = normalizeEditorLineEndings(content); + lastLoadedFileContentRef.current = content; + lastLoadedFileRevisionRef.current += 1; + setLoadedFileLineEnding(detectFileLineEnding(content)); + setFileContent(editorContent); + diagramXmlRef.current = editorContent; + diagramSavedXmlRef.current = editorContent; + setDraftContent(editorContent.length > MAX_VIEW_CHARS + ? `${editorContent.slice(0, MAX_VIEW_CHARS)}\n\n… truncated …` + : editorContent); + }, []); + const loadSelectedFile = React.useCallback(async (node: FileNode) => { const loadId = activeFileLoadIdRef.current + 1; activeFileLoadIdRef.current = loadId; @@ -1896,14 +1925,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { setLoadedFilePath(node.path); return; } - const editorContent = normalizeEditorLineEndings(content); - setLoadedFileLineEnding(detectFileLineEnding(content)); - setFileContent(editorContent); - diagramXmlRef.current = editorContent; - diagramSavedXmlRef.current = editorContent; - setDraftContent(editorContent.length > MAX_VIEW_CHARS - ? `${editorContent.slice(0, MAX_VIEW_CHARS)}\n\n… truncated …` - : editorContent); + applyLoadedTextContent(content); setLoadedFilePath(node.path); void readFileStat(node.path) .then((stat) => { @@ -1970,7 +1992,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { setFileLoading(false); } }); - }, [expandPaths, isMobile, loadDirectory, readFile, readFileStat, removeOpenPathsByPrefix, resolveFileReadOptions, root, runtime.isDesktop, searchQuery, setSelectedPath, t]); + }, [applyLoadedTextContent, expandPaths, isMobile, loadDirectory, readFile, readFileStat, removeOpenPathsByPrefix, resolveFileReadOptions, root, runtime.isDesktop, searchQuery, setSelectedPath, t]); const ensurePathVisible = React.useCallback(async (targetPath: string, includeTarget: boolean) => { if (!root) { @@ -2082,38 +2104,71 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { setFileContentRevision((revision) => revision + 1); }), [selectedFile?.path]); - // Poll open file for external changes. - // When a change is detected, reset loadedFilePath so the effect above - // triggers a single reload — no double-load. + // Poll open file for external changes. Metadata is compared first so an + // unchanged file never reads content, and a changed text file swaps content + // in place; only other files fall back to a full reload. React.useEffect(() => { if (!selectedFile?.path || loadedFilePath !== selectedFile.path) { return; } + const selectedPath = selectedFile.path; + // draw.io preview edits live in the XML refs, not the draft buffer, so an + // in-place content swap has to treat them as unsaved too. + const hasUnsavedChanges = () => isDirtyRef.current || ( + isDrawioFile(selectedPath) && diagramXmlRef.current !== diagramSavedXmlRef.current + ); + // Same exclusions as `isTextFile`: `isBinaryFile` covers PDFs, `isImageFile` covers SVG. + const contentPoller = !isBinaryFile(selectedPath) && !isImageFile(selectedPath) && !contentDetectedBinary + ? createFileContentPoller({ + readContent: () => readFile(selectedPath, { fresh: true }), + getLoadedContent: () => lastLoadedFileContentRef.current, + getLoadedRevision: () => lastLoadedFileRevisionRef.current, + isDirty: hasUnsavedChanges, + applyContent: (content) => { + // An external write can turn a text file binary; reload so the + // binary guards run instead of pasting binary into the editor. + if (looksLikeBinaryText(content)) { + setLoadedFilePath(null); + return; + } + applyLoadedTextContent(content); + }, + maxBytes: MAX_CONTENT_POLL_BYTES, + }) + : null; + let cancelled = false; + let polling = false; const interval = window.setInterval(() => { - if (document.hidden) { + if (document.hidden || polling) { return; } - void readFileStat(selectedFile.path) - .then((latestStat) => { + polling = true; + void readFileStat(selectedPath) + .then(async (latestStat) => { if (cancelled || !latestStat) { return; } const previousStat = lastLoadedFileStatRef.current; - if (!previousStat || previousStat.path !== selectedFile.path) { + if (!previousStat || previousStat.path !== selectedPath) { lastLoadedFileStatRef.current = latestStat; return; } - const changedByMtime = latestStat.mtimeMs !== undefined - && previousStat.mtimeMs !== undefined - && latestStat.mtimeMs !== previousStat.mtimeMs; - const changedBySize = latestStat.size !== previousStat.size; + if (!hasFileStatChanged(previousStat, latestStat)) { + return; + } - if (!changedByMtime && !changedBySize) { + if (contentPoller && latestStat.size <= MAX_CONTENT_POLL_BYTES) { + // Only an observed read retires the change; a dirty buffer or a + // failed read leaves the baseline so the next tick retries. + const observed = await contentPoller.poll(latestStat.size); + if (observed && !cancelled) { + lastLoadedFileStatRef.current = latestStat; + } return; } @@ -2125,14 +2180,18 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { // Reset loadedFilePath so the effect above triggers a single reload. setLoadedFilePath(null); }) - .catch(() => {}); + .catch(() => {}) + .finally(() => { + polling = false; + }); }, 2000); return () => { cancelled = true; + contentPoller?.dispose(); window.clearInterval(interval); }; - }, [loadedFilePath, readFileStat, selectedFile?.path]); + }, [applyLoadedTextContent, contentDetectedBinary, loadedFilePath, readFile, readFileStat, selectedFile?.path]); const discardAndContinue = React.useCallback(() => { const nextFile = pendingSelectFileRef.current; @@ -2595,15 +2654,13 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { return false; } - diagramXmlRef.current = xml; - diagramSavedXmlRef.current = xml; - setDraftContent(xml); + applyLoadedTextContent(xml); const stat = await readFileStat(path).catch(() => null); if (stat) { lastLoadedFileStatRef.current = stat; } return true; - }, [files, readFileStat, t]); + }, [applyLoadedTextContent, files, readFileStat, t]); React.useEffect(() => { return () => { diff --git a/packages/ui/src/components/views/fileContentPoller.test.ts b/packages/ui/src/components/views/fileContentPoller.test.ts new file mode 100644 index 00000000..04122d17 --- /dev/null +++ b/packages/ui/src/components/views/fileContentPoller.test.ts @@ -0,0 +1,200 @@ +import { describe, expect, test } from 'bun:test'; + +import { createFileContentPoller } from './fileContentPoller'; + +const MAX_BYTES = 200_000; + +const deferred = () => { + let resolve!: (value: T) => void; + const promise = new Promise((done) => { resolve = done; }); + return { promise, resolve }; +}; + +describe('createFileContentPoller', () => { + test('does not reload unchanged content across repeated metadata false positives (issue #1489)', async () => { + let reads = 0; + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: async () => { + reads += 1; + return 'same content'; + }, + getLoadedContent: () => 'same content', + getLoadedRevision: () => 0, + isDirty: () => false, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + expect(await poller.poll(12)).toBe(true); + expect(await poller.poll(12)).toBe(true); + + expect(reads).toBe(2); + expect(applied).toEqual([]); + }); + + test('reports a failed read as unobserved instead of unchanged', async () => { + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: async () => { throw new Error('read failed'); }, + getLoadedContent: () => 'before', + getLoadedRevision: () => 0, + isDirty: () => false, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + expect(await poller.poll(6)).toBe(false); + expect(applied).toEqual([]); + }); + + test('reports a dirty buffer as unobserved', async () => { + let reads = 0; + const poller = createFileContentPoller({ + readContent: async () => { + reads += 1; + return 'external edit'; + }, + getLoadedContent: () => 'before', + getLoadedRevision: () => 0, + isDirty: () => true, + applyContent: () => undefined, + maxBytes: MAX_BYTES, + }); + + expect(await poller.poll(6)).toBe(false); + expect(reads).toBe(0); + }); + + test('reloads a same-size edit even when metadata cannot distinguish it', async () => { + let reads = 0; + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: async () => { + reads += 1; + return 'abd'; + }, + getLoadedContent: () => 'abc', + getLoadedRevision: () => 0, + isDirty: () => false, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + expect(await poller.poll(3)).toBe(true); + + expect(reads).toBe(2); + expect(applied).toEqual(['abd']); + }); + + test('does not overwrite a buffer that becomes dirty while polling', async () => { + const read = deferred(); + let dirty = false; + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: () => read.promise, + getLoadedContent: () => 'before', + getLoadedRevision: () => 0, + isDirty: () => dirty, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + const polling = poller.poll(20); + dirty = true; + read.resolve('external edit'); + await polling; + + expect(applied).toEqual([]); + }); + + test('does not apply a transient read that changes during polling', async () => { + const reads = ['partial', 'settled', 'settled', 'settled']; + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: async () => reads.shift() ?? 'settled', + getLoadedContent: () => 'before', + getLoadedRevision: () => 0, + isDirty: () => false, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + await poller.poll(7); + expect(applied).toEqual([]); + + expect(await poller.poll(7)).toBe(true); + expect(applied).toEqual(['settled']); + }); + + test('allows only one read in flight and ignores a disposed poll', async () => { + const read = deferred(); + let reads = 0; + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: () => { + reads += 1; + return read.promise; + }, + getLoadedContent: () => 'before', + getLoadedRevision: () => 0, + isDirty: () => false, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + const first = poller.poll(5); + await poller.poll(5); + poller.dispose(); + read.resolve('after'); + await first; + + expect(reads).toBe(1); + expect(applied).toEqual([]); + }); + + test('does not overwrite after an ABA save during the confirmation read', async () => { + const firstRead = deferred(); + const secondRead = deferred(); + let reads = 0; + let loadedRevision = 0; + const applied: string[] = []; + const poller = createFileContentPoller({ + readContent: () => ++reads === 1 ? firstRead.promise : secondRead.promise, + getLoadedContent: () => 'before', + getLoadedRevision: () => loadedRevision, + isDirty: () => false, + applyContent: (content) => applied.push(content), + maxBytes: MAX_BYTES, + }); + + const polling = poller.poll(20); + firstRead.resolve('external edit'); + await Promise.resolve(); + loadedRevision += 1; + secondRead.resolve('external edit'); + await polling; + + expect(reads).toBe(2); + expect(applied).toEqual([]); + }); + + test('does not read content above the polling byte limit', async () => { + let reads = 0; + const poller = createFileContentPoller({ + readContent: async () => { + reads += 1; + return 'content'; + }, + getLoadedContent: () => 'before', + getLoadedRevision: () => 0, + isDirty: () => false, + applyContent: () => undefined, + maxBytes: MAX_BYTES, + }); + + expect(await poller.poll(MAX_BYTES + 1)).toBe(false); + + expect(reads).toBe(0); + }); +}); diff --git a/packages/ui/src/components/views/fileContentPoller.ts b/packages/ui/src/components/views/fileContentPoller.ts new file mode 100644 index 00000000..1608a604 --- /dev/null +++ b/packages/ui/src/components/views/fileContentPoller.ts @@ -0,0 +1,43 @@ +type FileContentPollerOptions = { + readContent: () => Promise; + getLoadedContent: () => string; + getLoadedRevision: () => number; + isDirty: () => boolean; + applyContent: (content: string) => void; + maxBytes: number; +}; + +export const createFileContentPoller = (options: FileContentPollerOptions) => { + let active = true; + let polling = false; + + return { + /** Resolves true only when the poll observed the file's current content. */ + poll: async (size: number): Promise => { + if (!active || polling || options.isDirty() || size > options.maxBytes) return false; + polling = true; + const loadedContent = options.getLoadedContent(); + const loadedRevision = options.getLoadedRevision(); + try { + const content = await options.readContent(); + if (!active || options.isDirty() || loadedRevision !== options.getLoadedRevision()) return false; + if (content === loadedContent) return true; + + const confirmedContent = await options.readContent(); + if (!active || options.isDirty() || confirmedContent !== content || loadedRevision !== options.getLoadedRevision()) { + return false; + } + options.applyContent(content); + return true; + } catch { + // A failed read is not proof the file is unchanged; the next poll retries. + return false; + } finally { + polling = false; + } + }, + dispose: () => { + active = false; + }, + }; +}; diff --git a/packages/ui/src/components/views/fileStatChange.test.ts b/packages/ui/src/components/views/fileStatChange.test.ts new file mode 100644 index 00000000..a7ad5d28 --- /dev/null +++ b/packages/ui/src/components/views/fileStatChange.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, test } from 'bun:test'; + +import { hasFileStatChanged } from './fileStatChange'; + +describe('hasFileStatChanged', () => { + test('ignores sub-millisecond mtime jitter on an unchanged file (issue #1489)', () => { + expect(hasFileStatChanged( + { size: 100, mtimeMs: 1700000000123.456 }, + { size: 100, mtimeMs: 1700000000123.4561 }, + )).toBe(false); + }); + + test('detects size changes and meaningful mtime changes', () => { + expect(hasFileStatChanged({ size: 100, mtimeMs: 1 }, { size: 101, mtimeMs: 1 })).toBe(true); + expect(hasFileStatChanged({ size: 100, mtimeMs: 1 }, { size: 100, mtimeMs: 2 })).toBe(true); + }); +}); diff --git a/packages/ui/src/components/views/fileStatChange.ts b/packages/ui/src/components/views/fileStatChange.ts new file mode 100644 index 00000000..49eaed9c --- /dev/null +++ b/packages/ui/src/components/views/fileStatChange.ts @@ -0,0 +1,16 @@ +type FileStatChangeInput = { + size: number; + mtimeMs?: number; +}; + +// Some filesystems report sub-millisecond mtime jitter for unchanged files. +const MIN_MTIME_CHANGE_MS = 1; + +export const hasFileStatChanged = ( + previous: FileStatChangeInput, + latest: FileStatChangeInput, +): boolean => latest.size !== previous.size || ( + latest.mtimeMs !== undefined + && previous.mtimeMs !== undefined + && Math.abs(latest.mtimeMs - previous.mtimeMs) >= MIN_MTIME_CHANGE_MS +); diff --git a/packages/ui/src/contexts/content-cache-owner.test.ts b/packages/ui/src/contexts/content-cache-owner.test.ts index a9095493..5d86c4ca 100644 --- a/packages/ui/src/contexts/content-cache-owner.test.ts +++ b/packages/ui/src/contexts/content-cache-owner.test.ts @@ -36,6 +36,25 @@ describe("content cache owner", () => { owner.dispose() }) + test("bypasses metadata cache for an authoritative read", async () => { + let content = "old" + let reads = 0 + const owner = createContentCachedFiles({ + readFile: async (path: string) => { + reads += 1 + return { path, content } + }, + statFile: async () => ({ isFile: true, isDirectory: false, size: 3, mtimeMs: 1 }), + } as unknown as FilesAPI) + + await owner.files.readFile!("file.ts") + content = "new" + expect((await owner.files.readFile!("file.ts")).content).toBe("old") + expect((await owner.files.readFile!("file.ts", { fresh: true })).content).toBe("new") + expect(reads).toBe(2) + owner.dispose() + }) + test("retries a read that overlaps a write", async () => { const firstRead = deferred<{ path: string; content: string }>() let content = "old" diff --git a/packages/ui/src/contexts/content-cache-owner.ts b/packages/ui/src/contexts/content-cache-owner.ts index 0a027ab4..c29f65a3 100644 --- a/packages/ui/src/contexts/content-cache-owner.ts +++ b/packages/ui/src/contexts/content-cache-owner.ts @@ -88,6 +88,10 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di const capturedGeneration = generation; if (options?.allowOutsideWorkspace) return files.readFile!(path, options); const key = cacheKey(path, options); + if (options?.fresh) { + removeEntry(key); + return files.readFile!(path, options); + } const hit = cache.get(key); if (!hit) return readFresh(key, path, options, capturedGeneration); const latest = await files.statFile?.(path, options).catch(() => null); diff --git a/packages/ui/src/lib/api/types.ts b/packages/ui/src/lib/api/types.ts index 61b31ba5..7fb80447 100644 --- a/packages/ui/src/lib/api/types.ts +++ b/packages/ui/src/lib/api/types.ts @@ -655,6 +655,7 @@ interface FileReadOptions { outsideFileGrant?: string; optional?: boolean; directory?: string; + fresh?: boolean; } export interface FilesAPI { diff --git a/packages/web/src/api/files.test.ts b/packages/web/src/api/files.test.ts index f353acfa..2a8e3b43 100644 --- a/packages/web/src/api/files.test.ts +++ b/packages/web/src/api/files.test.ts @@ -77,6 +77,15 @@ describe('createWebFilesAPI', () => { cache: 'default', headers: { 'x-opencode-directory': '/worktree-a' }, }); + + runtimeFetchMock.mockResolvedValueOnce(new Response('fresh content')); + await api.readFile?.('/worktree-b/file.txt', { directory: '/worktree-a', fresh: true }); + + expect(runtimeFetchMock).toHaveBeenLastCalledWith('/api/fs/read', { + query: new URLSearchParams({ path: '/worktree-b/file.txt' }), + cache: 'no-store', + headers: { 'x-opencode-directory': '/worktree-a' }, + }); }); it('sends the workspace directory header for downloads', async () => { diff --git a/packages/web/src/api/files.ts b/packages/web/src/api/files.ts index 696c6f60..e5528588 100644 --- a/packages/web/src/api/files.ts +++ b/packages/web/src/api/files.ts @@ -197,7 +197,7 @@ export const createWebFilesAPI = ({ getDirectory }: WebFilesAPIOptions): FilesAP } const response = await runtimeFetch('/api/fs/read', { query: params, - cache: options?.optional ? 'no-store' : 'default', + cache: options?.optional || options?.fresh ? 'no-store' : 'default', headers: directoryHeaders(getDirectory, options?.directory), });