From 3056eb29e070de406e06cecec6b8e9ccac0c561a Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 24 Aug 2026 16:34:08 +1000 Subject: [PATCH 1/6] fix(chat): refresh artifacts changed on disk Poll open artifact fingerprints while visible, reload stable text and image changes without flicker, and retain last-good content behind an explicit divergence warning when disk reads fail. Adapted from Brandon Sherman's format-patch attached to BOT-1675. Signed-off-by: Matt Toohey --- src-tauri/src/commands/system.rs | 71 ++++- src-tauri/src/lib.rs | 1 + src/features/chat/ui/ArtifactViewer.tsx | 279 ++++++++++++++++-- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 89 +++++- src/shared/api/system.ts | 9 + src/shared/i18n/locales/en/chat.json | 2 + src/shared/i18n/locales/es/chat.json | 2 + 7 files changed, 421 insertions(+), 32 deletions(-) diff --git a/src-tauri/src/commands/system.rs b/src-tauri/src/commands/system.rs index fce4e91dc..3f82f0c9d 100644 --- a/src-tauri/src/commands/system.rs +++ b/src-tauri/src/commands/system.rs @@ -13,7 +13,7 @@ use std::io::{self, Write}; use std::path::{Component, Path, PathBuf}; use std::process::Command; use std::sync::{Arc, Condvar, Mutex, OnceLock}; -use std::time::{Duration, Instant}; +use std::time::{Duration, Instant, UNIX_EPOCH}; const DEFAULT_FILE_MENTION_LIMIT: usize = 12; const MAX_FILE_MENTION_LIMIT: usize = 32; @@ -844,6 +844,51 @@ pub struct TextFilePayload { pub mime_type: Option, } +#[derive(Serialize, Clone, Debug, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct FileStatPayload { + /// Decimal strings preserve exact identity across the JSON/JavaScript + /// boundary, including nanosecond timestamp precision and large files. + pub byte_size: String, + pub modified_at_ns: String, +} + +/// Return the size and modification time used by open artifact viewers to +/// detect writes that do not appear in the main ACP session's tool events. +#[tauri::command] +pub fn stat_file(path: String) -> Result { + let target = Path::new(&path); + let metadata = fs::metadata(target) + .map_err(|error| format!("Failed to inspect '{}': {}", target.display(), error))?; + if !metadata.is_file() { + return Err(format!("Path is not a file: {}", target.display())); + } + + let modified_at_ns = metadata + .modified() + .map_err(|error| { + format!( + "Failed to read modification time for '{}': {}", + target.display(), + error + ) + })? + .duration_since(UNIX_EPOCH) + .map_err(|error| { + format!( + "Invalid modification time for '{}': {}", + target.display(), + error + ) + })? + .as_nanos(); + + Ok(FileStatPayload { + byte_size: metadata.len().to_string(), + modified_at_ns: modified_at_ns.to_string(), + }) +} + fn looks_binary(bytes: &[u8]) -> bool { bytes .iter() @@ -1994,8 +2039,9 @@ mod tests { get_or_build_file_mention_index_from_cache, inspect_attachment_path, inspect_attachment_paths, normalize_attachment_paths, normalize_roots, open_in_chrome_with, read_directory_entries, read_image_attachment, read_text_file, - search_file_mentions_blocking, write_agent_image_atomically, write_sibling_then_replace, - FileMentionIndexCache, MAX_IMAGE_ATTACHMENT_BYTES, MAX_TEXT_FILE_BYTES, + search_file_mentions_blocking, stat_file, write_agent_image_atomically, + write_sibling_then_replace, FileMentionIndexCache, MAX_IMAGE_ATTACHMENT_BYTES, + MAX_TEXT_FILE_BYTES, }; use base64::Engine; use std::fs; @@ -2848,6 +2894,25 @@ mod tests { assert!(!payload.base64.is_empty()); } + #[test] + fn stat_file_returns_size_and_modified_time() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("notes.md"); + fs::write(&path, "hello").expect("write"); + + let payload = stat_file(path.to_string_lossy().into_owned()).expect("stat file"); + assert_eq!(payload.byte_size, "5"); + assert!(payload.modified_at_ns.parse::().expect("timestamp") > 0); + } + + #[test] + fn stat_file_rejects_directories() { + let dir = tempdir().expect("tempdir"); + let error = stat_file(dir.path().to_string_lossy().into_owned()) + .expect_err("directory should error"); + assert!(error.contains("not a file"), "unexpected error: {error}"); + } + #[test] fn read_text_file_returns_utf8_contents() { let dir = tempdir().expect("tempdir"); diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index b01ce27d8..7f9fd8554 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -606,6 +606,7 @@ pub fn run() { commands::system::search_file_mentions, commands::system::read_image_attachment, commands::system::read_text_file, + commands::system::stat_file, commands::terminal::start_terminal, commands::terminal::write_terminal, commands::terminal::resize_terminal, diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index ef5ef718c..4121f2c26 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -7,7 +7,7 @@ import { ImageIcon, XIcon, } from "lucide-react"; -import { useEffect, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { Artifact, @@ -28,7 +28,7 @@ import { } from "@/shared/ui/dropdown-menu"; import { Spinner } from "@/shared/ui/spinner"; import { ToggleGroup, ToggleGroupItem } from "@/shared/ui/toggle-group"; -import { readTextFile } from "@/shared/api/system"; +import { readTextFile, statFile } from "@/shared/api/system"; import { revealInFileManager } from "@/shared/lib/fileManager"; import { getPlatform } from "@/shared/lib/platform"; import { useArtifactActionsContext } from "@/features/chat/hooks/ArtifactPolicyContext"; @@ -47,12 +47,29 @@ interface ArtifactViewerProps { } type MarkdownView = "preview" | "raw"; +type DiskStatus = "current" | "checking" | "diverged"; interface TextState { status: "loading" | "loaded" | "error"; contents: string; } +interface FileFingerprint { + byteSize: string; + modifiedAtNs: string; +} + +const ARTIFACT_POLL_INTERVAL_MS = 1_500; + +function sameFingerprint( + left: FileFingerprint, + right: FileFingerprint, +): boolean { + return ( + left.byteSize === right.byteSize && left.modifiedAtNs === right.modifiedAtNs + ); +} + export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { const { t } = useTranslation(["chat", "common"]); const { openResolvedPath } = useArtifactActionsContext(); @@ -65,6 +82,29 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { status: "loading", contents: "", }); + const textStateRef = useRef(textState); + const displayedPathRef = useRef(artifact.resolvedPath); + const fingerprintRef = useRef(null); + const [diskStatus, setDiskStatus] = useState("checking"); + const diskStatusRef = useRef(diskStatus); + const [imageDiskRevision, setImageDiskRevision] = useState(0); + const imageDiskRevisionRef = useRef(0); + const [retryRevision, setRetryRevision] = useState(0); + const renderedTextState: TextState = + displayedPathRef.current === artifact.resolvedPath + ? textState + : { status: "loading", contents: "" }; + const contentReadRevision = artifact.revision; + const refreshGenerationRef = useRef(0); + + const updateTextState = useCallback((next: TextState) => { + textStateRef.current = next; + setTextState(next); + }, []); + const updateDiskStatus = useCallback((next: DiskStatus) => { + diskStatusRef.current = next; + setDiskStatus(next); + }, []); // Escape closes the viewer — but only when nothing closer to the event // already handled it (open menus, dialogs, transcript search, etc.). @@ -78,27 +118,168 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { return () => window.removeEventListener("keydown", handleKeyDown); }, [onClose]); - // Load text contents for markdown. Images render straight from the path. + // Establish both the rendered content and the disk fingerprint. Re-reads of + // the same path retain last-good content while loading so tool-triggered and + // manual refreshes do not flash a spinner or reset the scroll container. useEffect(() => { - if (viewMode === "image") return; let cancelled = false; - setTextState({ status: "loading", contents: "" }); - void readTextFile(artifact.resolvedPath) - .then((payload) => { - if (cancelled) return; - setTextState({ status: "loaded", contents: payload.contents }); - }) - .catch(() => { - if (cancelled) return; - setTextState({ status: "error", contents: "" }); - }); + const refreshGeneration = ++refreshGenerationRef.current; + const isCurrentRefresh = () => + !cancelled && refreshGeneration === refreshGenerationRef.current; + const pathChanged = displayedPathRef.current !== artifact.resolvedPath; + if (pathChanged) { + displayedPathRef.current = artifact.resolvedPath; + fingerprintRef.current = null; + updateTextState({ status: "loading", contents: "" }); + imageDiskRevisionRef.current = 0; + setImageDiskRevision(0); + } else if ( + viewMode !== "image" && + textStateRef.current.status !== "loaded" + ) { + updateTextState({ status: "loading", contents: "" }); + } + updateDiskStatus("checking"); + + // Reading this value makes the ACP-driven revision an explicit input to + // this request even though only its change, not its numeric value, matters. + void contentReadRevision; + void (async () => { + try { + const before = await statFile(artifact.resolvedPath); + if (!isCurrentRefresh()) return; + + if (viewMode === "image") { + fingerprintRef.current = before; + if (retryRevision > 0) { + imageDiskRevisionRef.current += 1; + setImageDiskRevision(imageDiskRevisionRef.current); + } + updateDiskStatus("current"); + return; + } + + const payload = await readTextFile(artifact.resolvedPath); + const after = await statFile(artifact.resolvedPath); + if (!isCurrentRefresh()) return; + if (!sameFingerprint(before, after)) { + updateDiskStatus("diverged"); + return; + } + + fingerprintRef.current = after; + if ( + textStateRef.current.contents !== payload.contents || + textStateRef.current.status !== "loaded" + ) { + updateTextState({ status: "loaded", contents: payload.contents }); + } + updateDiskStatus("current"); + } catch { + if (!isCurrentRefresh()) return; + if (textStateRef.current.status === "loaded") { + updateDiskStatus("diverged"); + } else { + updateTextState({ status: "error", contents: "" }); + updateDiskStatus("diverged"); + } + } + })(); + return () => { cancelled = true; + refreshGenerationRef.current += 1; + }; + }, [ + artifact.resolvedPath, + contentReadRevision, + retryRevision, + updateDiskStatus, + updateTextState, + viewMode, + ]); + + // Tool events cannot account for shell writes, delegated subagents, or + // external editors. Poll the one open file while this document is visible, + // including an immediate check on return from the background. + useEffect(() => { + let cancelled = false; + let checkInFlight = false; + + const checkForDiskChange = async () => { + if (document.visibilityState === "hidden" || checkInFlight) { + return; + } + const refreshGeneration = ++refreshGenerationRef.current; + const isCurrentRefresh = () => + !cancelled && refreshGeneration === refreshGenerationRef.current; + checkInFlight = true; + try { + const fingerprint = await statFile(artifact.resolvedPath); + if (!isCurrentRefresh()) return; + const previous = fingerprintRef.current; + if ( + previous && + sameFingerprint(previous, fingerprint) && + diskStatusRef.current !== "diverged" + ) { + updateDiskStatus("current"); + return; + } + // A diverged view always retries the content/decode even when stat has + // returned to the last fingerprint, so transient failures self-heal. + + if (viewMode === "image") { + const candidateRevision = imageDiskRevisionRef.current + 1; + await preloadArtifactImage(artifact.resolvedPath, candidateRevision); + if (!isCurrentRefresh()) return; + fingerprintRef.current = fingerprint; + imageDiskRevisionRef.current = candidateRevision; + setImageDiskRevision(candidateRevision); + updateDiskStatus("current"); + return; + } + + const payload = await readTextFile(artifact.resolvedPath); + if (!isCurrentRefresh()) return; + const confirmedFingerprint = await statFile(artifact.resolvedPath); + if ( + !isCurrentRefresh() || + !sameFingerprint(fingerprint, confirmedFingerprint) + ) { + updateDiskStatus("diverged"); + return; + } + fingerprintRef.current = confirmedFingerprint; + if (textStateRef.current.contents !== payload.contents) { + updateTextState({ status: "loaded", contents: payload.contents }); + } + updateDiskStatus("current"); + } catch { + if (isCurrentRefresh()) updateDiskStatus("diverged"); + } finally { + checkInFlight = false; + } }; - // Depend on the artifact object, not just the path: the store creates a - // fresh object (with a bumped revision) when the same path is re-opened - // after the agent re-edits it, and the contents must be re-read then. - }, [artifact, viewMode]); + + const handleVisibilityChange = () => { + if (document.visibilityState !== "hidden") { + void checkForDiskChange(); + } + }; + const intervalId = window.setInterval( + () => void checkForDiskChange(), + ARTIFACT_POLL_INTERVAL_MS, + ); + document.addEventListener("visibilitychange", handleVisibilityChange); + + return () => { + cancelled = true; + refreshGenerationRef.current += 1; + window.clearInterval(intervalId); + document.removeEventListener("visibilitychange", handleVisibilityChange); + }; + }, [artifact.resolvedPath, updateDiskStatus, updateTextState, viewMode]); return ( @@ -181,13 +362,34 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { + {diskStatus === "diverged" ? ( +
+ {t("artifactViewer.diskDiverged")} + +
+ ) : null} +
{viewMode === "image" ? ( - + updateDiskStatus("diverged")} + /> ) : ( { void openResolvedPath(artifact.resolvedPath).catch(() => {}); }} @@ -198,22 +400,45 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { ); } -function ImageBody({ artifact }: { artifact: OpenArtifact }) { +function artifactImageSrc(path: string, revision: number): string { + const assetSrc = convertFileSrc(path, "asset"); + return revision > 0 ? `${assetSrc}?rev=${revision}` : assetSrc; +} + +function preloadArtifactImage(path: string, revision: number): Promise { + return new Promise((resolve, reject) => { + const image = new Image(); + image.onload = () => resolve(); + image.onerror = () => reject(new Error("Artifact image failed to load")); + image.src = artifactImageSrc(path, revision); + }); +} + +function ImageBody({ + artifact, + diskRevision, + onLoadError, +}: { + artifact: OpenArtifact; + diskRevision: number; + onLoadError: () => void; +}) { const { t } = useTranslation("chat"); const src = useMemo(() => { - const assetSrc = convertFileSrc(artifact.resolvedPath, "asset"); - // Re-opening the same path (agent re-edited the open image) must bypass + // Re-opening or detecting an external write to the same path must bypass // the webview's cache for the unchanged asset URL. - return artifact.revision > 0 - ? `${assetSrc}?rev=${artifact.revision}` - : assetSrc; - }, [artifact.resolvedPath, artifact.revision]); + return artifactImageSrc( + artifact.resolvedPath, + artifact.revision + diskRevision, + ); + }, [artifact.resolvedPath, artifact.revision, diskRevision]); return (
{t("artifactViewer.imageAlt",
); diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index 9c58eb3de..6d3d652df 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -1,11 +1,12 @@ -import { render, screen, waitFor } from "@testing-library/react"; +import { act, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ArtifactViewer } from "../ArtifactViewer"; const mockOpenResolvedPath = vi.fn().mockResolvedValue(undefined); const mockRevealInFileManager = vi.fn().mockResolvedValue(undefined); const mockReadTextFile = vi.fn(); +const mockStatFile = vi.fn(); vi.mock("@/features/chat/hooks/ArtifactPolicyContext", () => ({ useArtifactActionsContext: () => ({ @@ -22,6 +23,7 @@ vi.mock("@/shared/lib/fileManager", () => ({ vi.mock("@/shared/api/system", () => ({ readTextFile: (path: string) => mockReadTextFile(path), + statFile: (path: string) => mockStatFile(path), })); // jsdom has no Tauri internals, so the real asset-URL converter throws. @@ -49,6 +51,12 @@ describe("ArtifactViewer header actions", () => { mockRevealInFileManager.mockClear(); mockReadTextFile.mockReset(); mockReadTextFile.mockResolvedValue({ contents: "# Title\n\nBody copy." }); + mockStatFile.mockReset(); + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); + }); + + afterEach(() => { + vi.useRealTimers(); }); it("reveals the file in the OS file manager from the file actions menu", async () => { @@ -116,4 +124,81 @@ describe("ArtifactViewer header actions", () => { expect(heading.className).not.toMatch(/\buppercase\b/); expect(heading.textContent).toBe("api_KEY and Path"); }); + + it("polls the open file and swaps in externally changed text", async () => { + vi.useFakeTimers(); + let changed = false; + mockReadTextFile.mockImplementation(async () => ({ + contents: changed ? "# Updated externally" : "# Original", + })); + mockStatFile.mockImplementation(async () => + changed + ? { byteSize: "20", modifiedAtNs: "2" } + : { byteSize: "10", modifiedAtNs: "1" }, + ); + + render(); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + expect( + screen.getByRole("heading", { name: "Original" }), + ).toBeInTheDocument(); + + changed = true; + await act(async () => { + vi.advanceTimersByTime(1_500); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect( + screen.getByRole("heading", { name: "Updated externally" }), + ).toBeInTheDocument(); + expect(screen.queryByText(/out of date/i)).not.toBeInTheDocument(); + }); + + it("keeps last-good content visible and marks it stale when a changed file cannot be read", async () => { + vi.useFakeTimers(); + let changed = false; + mockReadTextFile.mockImplementation(async () => { + if (changed) throw new Error("mid-write"); + return { contents: "# Last good copy" }; + }); + mockStatFile.mockImplementation(async () => + changed + ? { byteSize: "20", modifiedAtNs: "2" } + : { byteSize: "16", modifiedAtNs: "1" }, + ); + + render(); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + changed = true; + await act(async () => { + vi.advanceTimersByTime(1_500); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect( + screen.getByRole("heading", { name: "Last good copy" }), + ).toBeInTheDocument(); + expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + expect(screen.getByRole("button", { name: /reload/i })).toBeInTheDocument(); + + // A later unchanged stat must not silently clear the warning: the viewer + // still has the old contents until a read succeeds. + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "2" }); + await act(async () => { + vi.advanceTimersByTime(1_500); + await Promise.resolve(); + await Promise.resolve(); + }); + expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + }); }); diff --git a/src/shared/api/system.ts b/src/shared/api/system.ts index 1531136d8..54ec98b2c 100644 --- a/src/shared/api/system.ts +++ b/src/shared/api/system.ts @@ -169,3 +169,12 @@ export interface TextFilePayload { export async function readTextFile(path: string): Promise { return invoke("read_text_file", { path }); } + +export interface FileStatPayload { + byteSize: string; + modifiedAtNs: string; +} + +export async function statFile(path: string): Promise { + return invoke("stat_file", { path }); +} diff --git a/src/shared/i18n/locales/en/chat.json b/src/shared/i18n/locales/en/chat.json index 94e42e672..9137a1df4 100644 --- a/src/shared/i18n/locales/en/chat.json +++ b/src/shared/i18n/locales/en/chat.json @@ -689,6 +689,8 @@ "viewCode": "Code", "loading": "Loading file…", "loadError": "Couldn't load this file.", + "diskDiverged": "This preview may be out of date because the file changed or is unavailable on disk.", + "reload": "Reload", "imageAlt": "Preview of {{filename}}" }, "artifactChips": { diff --git a/src/shared/i18n/locales/es/chat.json b/src/shared/i18n/locales/es/chat.json index ac77f93c5..7cc6183f0 100644 --- a/src/shared/i18n/locales/es/chat.json +++ b/src/shared/i18n/locales/es/chat.json @@ -685,6 +685,8 @@ "viewCode": "Código", "loading": "Cargando archivo…", "loadError": "No se pudo cargar este archivo.", + "diskDiverged": "Esta vista previa puede estar desactualizada porque el archivo cambió o no está disponible en el disco.", + "reload": "Volver a cargar", "imageAlt": "Vista previa de {{filename}}" }, "artifactChips": { From 408548855aaf653efa421b93c6958687f33b1755 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 24 Aug 2026 17:10:07 +1000 Subject: [PATCH 2/6] fix(chat): harden artifact refresh recovery Track cross-platform change times and signed pre-epoch mtimes, serialize forced refreshes against polling, and only accept image fingerprints after the rendered cache-busted source decodes. Cover metadata-preserving rewrites, refresh races, image URL validation, and empty-file recovery. Signed-off-by: Matt Toohey --- src-tauri/Cargo.toml | 1 + src-tauri/src/commands/system.rs | 116 +++++++++-- src/features/chat/ui/ArtifactViewer.tsx | 172 +++++++++++----- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 187 +++++++++++++++++- src/shared/api/system.ts | 1 + 5 files changed, 409 insertions(+), 68 deletions(-) diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 4c57c6e2d..df94e574c 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -103,6 +103,7 @@ zip = { version = "2", default-features = false, features = ["deflate"] } windows-sys = { version = "0.59", features = [ "Win32_Foundation", "Win32_Globalization", + "Win32_Storage_FileSystem", "Win32_System_Com", "Win32_System_Threading", "Win32_UI_Shell", diff --git a/src-tauri/src/commands/system.rs b/src-tauri/src/commands/system.rs index 3f82f0c9d..938f6332a 100644 --- a/src-tauri/src/commands/system.rs +++ b/src-tauri/src/commands/system.rs @@ -13,7 +13,7 @@ use std::io::{self, Write}; use std::path::{Component, Path, PathBuf}; use std::process::Command; use std::sync::{Arc, Condvar, Mutex, OnceLock}; -use std::time::{Duration, Instant, UNIX_EPOCH}; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; const DEFAULT_FILE_MENTION_LIMIT: usize = 12; const MAX_FILE_MENTION_LIMIT: usize = 32; @@ -851,10 +851,59 @@ pub struct FileStatPayload { /// boundary, including nanosecond timestamp precision and large files. pub byte_size: String, pub modified_at_ns: String, + /// Change time catches same-size rewrites whose modification time was + /// restored. It is available on Unix and Windows; other platforms omit it. + #[serde(skip_serializing_if = "Option::is_none")] + pub changed_at_ns: Option, } -/// Return the size and modification time used by open artifact viewers to -/// detect writes that do not appear in the main ACP session's tool events. +fn signed_unix_timestamp_ns(time: SystemTime) -> String { + match time.duration_since(UNIX_EPOCH) { + Ok(duration) => duration.as_nanos().to_string(), + Err(error) => format!("-{}", error.duration().as_nanos()), + } +} + +#[cfg(windows)] +fn windows_file_change_time_ns(path: &Path) -> Result { + use std::fs::File; + use std::mem::{size_of, zeroed}; + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Storage::FileSystem::{ + FileBasicInfo, GetFileInformationByHandleEx, FILE_BASIC_INFO, + }; + + let file = File::open(path).map_err(|error| { + format!( + "Failed to open '{}' for change time: {}", + path.display(), + error + ) + })?; + let mut info: FILE_BASIC_INFO = unsafe { zeroed() }; + let succeeded = unsafe { + GetFileInformationByHandleEx( + file.as_raw_handle(), + FileBasicInfo, + (&raw mut info).cast(), + size_of::() as u32, + ) + }; + if succeeded == 0 { + return Err(format!( + "Failed to read change time for '{}': {}", + path.display(), + io::Error::last_os_error() + )); + } + + // Windows reports signed 100ns ticks from 1601. It is an opaque token for + // equality comparisons, so preserving that epoch avoids lossy conversion. + Ok((i128::from(info.ChangeTime) * 100).to_string()) +} + +/// Return the metadata identity used by open artifact viewers to detect writes +/// that do not appear in the main ACP session's tool events. #[tauri::command] pub fn stat_file(path: String) -> Result { let target = Path::new(&path); @@ -866,26 +915,31 @@ pub fn stat_file(path: String) -> Result { let modified_at_ns = metadata .modified() + .map(signed_unix_timestamp_ns) .map_err(|error| { format!( "Failed to read modification time for '{}': {}", target.display(), error ) - })? - .duration_since(UNIX_EPOCH) - .map_err(|error| { - format!( - "Invalid modification time for '{}': {}", - target.display(), - error - ) - })? - .as_nanos(); + })?; + + #[cfg(unix)] + let changed_at_ns = { + use std::os::unix::fs::MetadataExt; + let nanoseconds = + i128::from(metadata.ctime()) * 1_000_000_000 + i128::from(metadata.ctime_nsec()); + Some(nanoseconds.to_string()) + }; + #[cfg(windows)] + let changed_at_ns = Some(windows_file_change_time_ns(target)?); + #[cfg(not(any(unix, windows)))] + let changed_at_ns = None; Ok(FileStatPayload { byte_size: metadata.len().to_string(), - modified_at_ns: modified_at_ns.to_string(), + modified_at_ns, + changed_at_ns, }) } @@ -2039,9 +2093,9 @@ mod tests { get_or_build_file_mention_index_from_cache, inspect_attachment_path, inspect_attachment_paths, normalize_attachment_paths, normalize_roots, open_in_chrome_with, read_directory_entries, read_image_attachment, read_text_file, - search_file_mentions_blocking, stat_file, write_agent_image_atomically, - write_sibling_then_replace, FileMentionIndexCache, MAX_IMAGE_ATTACHMENT_BYTES, - MAX_TEXT_FILE_BYTES, + search_file_mentions_blocking, signed_unix_timestamp_ns, stat_file, + write_agent_image_atomically, write_sibling_then_replace, FileMentionIndexCache, + MAX_IMAGE_ATTACHMENT_BYTES, MAX_TEXT_FILE_BYTES, }; use base64::Engine; use std::fs; @@ -2056,7 +2110,7 @@ mod tests { Arc, Barrier, Mutex, }; use std::thread; - use std::time::Duration; + use std::time::{Duration, UNIX_EPOCH}; use tempfile::tempdir; /// Create a temp dir with `git init` so the ignore crate picks up `.gitignore`. @@ -2895,14 +2949,36 @@ mod tests { } #[test] - fn stat_file_returns_size_and_modified_time() { + fn stat_file_returns_size_and_metadata_times() { let dir = tempdir().expect("tempdir"); let path = dir.path().join("notes.md"); fs::write(&path, "hello").expect("write"); let payload = stat_file(path.to_string_lossy().into_owned()).expect("stat file"); assert_eq!(payload.byte_size, "5"); - assert!(payload.modified_at_ns.parse::().expect("timestamp") > 0); + assert!(payload.modified_at_ns.parse::().expect("timestamp") > 0); + #[cfg(unix)] + assert!(payload.changed_at_ns.is_some()); + } + + #[test] + fn serializes_pre_epoch_times_as_signed_nanoseconds() { + let timestamp = UNIX_EPOCH - Duration::from_nanos(42); + assert_eq!(signed_unix_timestamp_ns(timestamp), "-42"); + } + + #[test] + fn stat_file_accepts_pre_epoch_modification_times() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("old-notes.md"); + fs::write(&path, "hello").expect("write"); + let file = fs::File::open(&path).expect("open"); + let old_timestamp = UNIX_EPOCH - Duration::from_secs(1); + file.set_times(fs::FileTimes::new().set_modified(old_timestamp)) + .expect("set pre-epoch mtime"); + + let payload = stat_file(path.to_string_lossy().into_owned()).expect("stat old file"); + assert_eq!(payload.modified_at_ns, "-1000000000"); } #[test] diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 4121f2c26..7af9c3049 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -57,6 +57,7 @@ interface TextState { interface FileFingerprint { byteSize: string; modifiedAtNs: string; + changedAtNs?: string; } const ARTIFACT_POLL_INTERVAL_MS = 1_500; @@ -66,7 +67,9 @@ function sameFingerprint( right: FileFingerprint, ): boolean { return ( - left.byteSize === right.byteSize && left.modifiedAtNs === right.modifiedAtNs + left.byteSize === right.byteSize && + left.modifiedAtNs === right.modifiedAtNs && + left.changedAtNs === right.changedAtNs ); } @@ -90,12 +93,28 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { const [imageDiskRevision, setImageDiskRevision] = useState(0); const imageDiskRevisionRef = useRef(0); const [retryRevision, setRetryRevision] = useState(0); + const consumedRetryRevisionRef = useRef(0); const renderedTextState: TextState = displayedPathRef.current === artifact.resolvedPath ? textState : { status: "loading", contents: "" }; const contentReadRevision = artifact.revision; - const refreshGenerationRef = useRef(0); + const forcedRefreshGenerationRef = useRef(0); + const forcedRefreshInFlightRef = useRef(false); + const pollGenerationRef = useRef(0); + const loadedImageSrcRef = useRef(null); + const pendingImageRef = useRef<{ + src: string; + fingerprint: FileFingerprint; + } | null>(null); + const imageSrc = useMemo( + () => + artifactImageSrc( + artifact.resolvedPath, + artifact.revision + imageDiskRevision, + ), + [artifact.resolvedPath, artifact.revision, imageDiskRevision], + ); const updateTextState = useCallback((next: TextState) => { textStateRef.current = next; @@ -123,13 +142,25 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { // manual refreshes do not flash a spinner or reset the scroll container. useEffect(() => { let cancelled = false; - const refreshGeneration = ++refreshGenerationRef.current; + const refreshGeneration = ++forcedRefreshGenerationRef.current; + // A forced ACP/manual refresh supersedes a poll already in flight. Polls + // never supersede forced work; they pause until it completes. + pollGenerationRef.current += 1; + forcedRefreshInFlightRef.current = true; const isCurrentRefresh = () => - !cancelled && refreshGeneration === refreshGenerationRef.current; + !cancelled && refreshGeneration === forcedRefreshGenerationRef.current; + const finishRefresh = () => { + if (refreshGeneration === forcedRefreshGenerationRef.current) { + forcedRefreshInFlightRef.current = false; + } + }; const pathChanged = displayedPathRef.current !== artifact.resolvedPath; if (pathChanged) { displayedPathRef.current = artifact.resolvedPath; fingerprintRef.current = null; + pendingImageRef.current = null; + loadedImageSrcRef.current = null; + consumedRetryRevisionRef.current = retryRevision; updateTextState({ status: "loading", contents: "" }); imageDiskRevisionRef.current = 0; setImageDiskRevision(0); @@ -139,7 +170,9 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { ) { updateTextState({ status: "loading", contents: "" }); } - updateDiskStatus("checking"); + if (pathChanged || diskStatusRef.current !== "diverged") { + updateDiskStatus("checking"); + } // Reading this value makes the ACP-driven revision an explicit input to // this request even though only its change, not its numeric value, matters. @@ -150,12 +183,28 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { if (!isCurrentRefresh()) return; if (viewMode === "image") { - fingerprintRef.current = before; - if (retryRevision > 0) { - imageDiskRevisionRef.current += 1; - setImageDiskRevision(imageDiskRevisionRef.current); + const shouldBustImageCache = + retryRevision !== consumedRetryRevisionRef.current; + const candidateDiskRevision = shouldBustImageCache + ? imageDiskRevisionRef.current + 1 + : imageDiskRevisionRef.current; + const candidateSrc = artifactImageSrc( + artifact.resolvedPath, + artifact.revision + candidateDiskRevision, + ); + if (shouldBustImageCache) { + await preloadArtifactImage(candidateSrc); + if (!isCurrentRefresh()) return; + imageDiskRevisionRef.current = candidateDiskRevision; + consumedRetryRevisionRef.current = retryRevision; + setImageDiskRevision(candidateDiskRevision); + } + pendingImageRef.current = { src: candidateSrc, fingerprint: before }; + if (loadedImageSrcRef.current === candidateSrc) { + fingerprintRef.current = before; + pendingImageRef.current = null; + updateDiskStatus("current"); } - updateDiskStatus("current"); return; } @@ -183,15 +232,21 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { updateTextState({ status: "error", contents: "" }); updateDiskStatus("diverged"); } + } finally { + finishRefresh(); } })(); return () => { cancelled = true; - refreshGenerationRef.current += 1; + if (refreshGeneration === forcedRefreshGenerationRef.current) { + forcedRefreshGenerationRef.current += 1; + forcedRefreshInFlightRef.current = false; + } }; }, [ artifact.resolvedPath, + artifact.revision, contentReadRevision, retryRevision, updateDiskStatus, @@ -207,16 +262,22 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { let checkInFlight = false; const checkForDiskChange = async () => { - if (document.visibilityState === "hidden" || checkInFlight) { + if ( + document.visibilityState === "hidden" || + checkInFlight || + forcedRefreshInFlightRef.current + ) { return; } - const refreshGeneration = ++refreshGenerationRef.current; - const isCurrentRefresh = () => - !cancelled && refreshGeneration === refreshGenerationRef.current; + const pollGeneration = ++pollGenerationRef.current; + const isCurrentPoll = () => + !cancelled && + pollGeneration === pollGenerationRef.current && + !forcedRefreshInFlightRef.current; checkInFlight = true; try { const fingerprint = await statFile(artifact.resolvedPath); - if (!isCurrentRefresh()) return; + if (!isCurrentPoll()) return; const previous = fingerprintRef.current; if ( previous && @@ -230,33 +291,39 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { // returned to the last fingerprint, so transient failures self-heal. if (viewMode === "image") { - const candidateRevision = imageDiskRevisionRef.current + 1; - await preloadArtifactImage(artifact.resolvedPath, candidateRevision); - if (!isCurrentRefresh()) return; - fingerprintRef.current = fingerprint; - imageDiskRevisionRef.current = candidateRevision; - setImageDiskRevision(candidateRevision); - updateDiskStatus("current"); + const candidateDiskRevision = imageDiskRevisionRef.current + 1; + const candidateSrc = artifactImageSrc( + artifact.resolvedPath, + artifact.revision + candidateDiskRevision, + ); + await preloadArtifactImage(candidateSrc); + if (!isCurrentPoll()) return; + pendingImageRef.current = { src: candidateSrc, fingerprint }; + imageDiskRevisionRef.current = candidateDiskRevision; + setImageDiskRevision(candidateDiskRevision); return; } const payload = await readTextFile(artifact.resolvedPath); - if (!isCurrentRefresh()) return; + if (!isCurrentPoll()) return; const confirmedFingerprint = await statFile(artifact.resolvedPath); if ( - !isCurrentRefresh() || + !isCurrentPoll() || !sameFingerprint(fingerprint, confirmedFingerprint) ) { updateDiskStatus("diverged"); return; } fingerprintRef.current = confirmedFingerprint; - if (textStateRef.current.contents !== payload.contents) { + if ( + textStateRef.current.contents !== payload.contents || + textStateRef.current.status !== "loaded" + ) { updateTextState({ status: "loaded", contents: payload.contents }); } updateDiskStatus("current"); } catch { - if (isCurrentRefresh()) updateDiskStatus("diverged"); + if (isCurrentPoll()) updateDiskStatus("diverged"); } finally { checkInFlight = false; } @@ -275,11 +342,17 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { return () => { cancelled = true; - refreshGenerationRef.current += 1; + pollGenerationRef.current += 1; window.clearInterval(intervalId); document.removeEventListener("visibilitychange", handleVisibilityChange); }; - }, [artifact.resolvedPath, updateDiskStatus, updateTextState, viewMode]); + }, [ + artifact.resolvedPath, + artifact.revision, + updateDiskStatus, + updateTextState, + viewMode, + ]); return ( @@ -383,8 +456,22 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { {viewMode === "image" ? ( updateDiskStatus("diverged")} + src={imageSrc} + onLoad={(loadedSrc) => { + if (loadedSrc !== imageSrc) return; + loadedImageSrcRef.current = loadedSrc; + const pending = pendingImageRef.current; + if (pending?.src !== loadedSrc) return; + fingerprintRef.current = pending.fingerprint; + pendingImageRef.current = null; + updateDiskStatus("current"); + }} + onLoadError={(failedSrc) => { + if (failedSrc !== imageSrc) return; + loadedImageSrcRef.current = null; + pendingImageRef.current = null; + updateDiskStatus("diverged"); + }} /> ) : ( 0 ? `${assetSrc}?rev=${revision}` : assetSrc; } -function preloadArtifactImage(path: string, revision: number): Promise { +function preloadArtifactImage(src: string): Promise { return new Promise((resolve, reject) => { const image = new Image(); image.onload = () => resolve(); image.onerror = () => reject(new Error("Artifact image failed to load")); - image.src = artifactImageSrc(path, revision); + image.src = src; }); } function ImageBody({ artifact, - diskRevision, + src, + onLoad, onLoadError, }: { artifact: OpenArtifact; - diskRevision: number; - onLoadError: () => void; + src: string; + onLoad: (src: string) => void; + onLoadError: (src: string) => void; }) { const { t } = useTranslation("chat"); - const src = useMemo(() => { - // Re-opening or detecting an external write to the same path must bypass - // the webview's cache for the unchanged asset URL. - return artifactImageSrc( - artifact.resolvedPath, - artifact.revision + diskRevision, - ); - }, [artifact.resolvedPath, artifact.revision, diskRevision]); return (
{t("artifactViewer.imageAlt", onLoad(src)} + onError={() => onLoadError(src)} />
); diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index 6d3d652df..f4a35c317 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -1,4 +1,10 @@ -import { act, render, screen, waitFor } from "@testing-library/react"; +import { + act, + fireEvent, + render, + screen, + waitFor, +} from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ArtifactViewer } from "../ArtifactViewer"; @@ -31,14 +37,30 @@ vi.mock("@tauri-apps/api/core", () => ({ convertFileSrc: (path: string) => `asset://localhost/${path}`, })); -function artifact(path = "/p/report.md") { +function artifact(path = "/p/report.md", revision = 0) { return { resolvedPath: path, filename: path.split("/").pop() ?? path, - revision: 0, + revision, }; } +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} + +async function flushAsyncWork() { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); +} + async function openFileActionsMenu() { const user = userEvent.setup(); await user.click(screen.getByRole("button", { name: /file actions/i })); @@ -57,6 +79,7 @@ describe("ArtifactViewer header actions", () => { afterEach(() => { vi.useRealTimers(); + vi.unstubAllGlobals(); }); it("reveals the file in the OS file manager from the file actions menu", async () => { @@ -159,6 +182,31 @@ describe("ArtifactViewer header actions", () => { expect(screen.queryByText(/out of date/i)).not.toBeInTheDocument(); }); + it("detects same-size same-mtime rewrites from change time", async () => { + vi.useFakeTimers(); + let changed = false; + mockReadTextFile.mockImplementation(async () => ({ + contents: changed ? "# Second" : "# First!", + })); + mockStatFile.mockImplementation(async () => ({ + byteSize: "8", + modifiedAtNs: "1", + changedAtNs: changed ? "2" : "1", + })); + + render(); + await act(flushAsyncWork); + expect(screen.getByRole("heading", { name: "First!" })).toBeInTheDocument(); + + changed = true; + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + + expect(screen.getByRole("heading", { name: "Second" })).toBeInTheDocument(); + }); + it("keeps last-good content visible and marks it stale when a changed file cannot be read", async () => { vi.useFakeTimers(); let changed = false; @@ -201,4 +249,137 @@ describe("ArtifactViewer header actions", () => { }); expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); }); + + it("recovers an initially failed empty text file to loaded state", async () => { + vi.useFakeTimers(); + let available = false; + mockReadTextFile.mockImplementation(async () => { + if (!available) throw new Error("temporarily unavailable"); + return { contents: "" }; + }); + + render(); + await act(flushAsyncWork); + expect(screen.getByText(/couldn't load/i)).toBeInTheDocument(); + + available = true; + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + + expect(screen.queryByText(/couldn't load/i)).not.toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("does not let polling cancel an ACP-forced text reread", async () => { + vi.useFakeTimers(); + const forcedRead = deferred<{ contents: string }>(); + mockReadTextFile + .mockResolvedValueOnce({ contents: "# Original" }) + .mockReturnValueOnce(forcedRead.promise); + + const { rerender } = render( + , + ); + await act(flushAsyncWork); + + rerender( + , + ); + await act(async () => { + await Promise.resolve(); + vi.advanceTimersByTime(1_500); + await Promise.resolve(); + }); + forcedRead.resolve({ contents: "# Forced refresh" }); + await act(flushAsyncWork); + + expect( + screen.getByRole("heading", { name: "Forced refresh" }), + ).toBeInTheDocument(); + }); + + it("commits image status only after the rendered cache-busted URL decodes", async () => { + vi.useFakeTimers(); + const initialStat = deferred<{ + byteSize: string; + modifiedAtNs: string; + }>(); + mockStatFile + .mockReturnValueOnce(initialStat.promise) + .mockResolvedValue({ byteSize: "20", modifiedAtNs: "2" }); + const { rerender } = render( + , + ); + + const image = screen.getByRole("img"); + expect(image).toHaveAttribute("src", "asset://localhost//p/shot.png?rev=4"); + fireEvent.error(image); + initialStat.resolve({ byteSize: "20", modifiedAtNs: "1" }); + await act(flushAsyncWork); + // A late successful stat must not overwrite the earlier decode failure. + expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + + rerender( + , + ); + await act(flushAsyncWork); + const refreshedImage = screen.getByRole("img"); + expect(refreshedImage).toHaveAttribute( + "src", + "asset://localhost//p/shot.png?rev=5", + ); + expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + + fireEvent.load(refreshedImage); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("preloads and renders the same image URL after a polled change", async () => { + vi.useFakeTimers(); + let changed = false; + mockStatFile.mockImplementation(async () => ({ + byteSize: "20", + modifiedAtNs: changed ? "2" : "1", + })); + const preloadedSources: string[] = []; + class PreloadImage { + onload: (() => void) | null = null; + + set src(value: string) { + preloadedSources.push(value); + queueMicrotask(() => this.onload?.()); + } + } + vi.stubGlobal("Image", PreloadImage); + + render( + , + ); + await act(flushAsyncWork); + fireEvent.load(screen.getByRole("img")); + + changed = true; + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + + const expectedSrc = "asset://localhost//p/shot.png?rev=5"; + expect(preloadedSources).toEqual([expectedSrc]); + expect(screen.getByRole("img")).toHaveAttribute("src", expectedSrc); + + fireEvent.load(screen.getByRole("img")); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); }); diff --git a/src/shared/api/system.ts b/src/shared/api/system.ts index 54ec98b2c..b64e8c4cf 100644 --- a/src/shared/api/system.ts +++ b/src/shared/api/system.ts @@ -173,6 +173,7 @@ export async function readTextFile(path: string): Promise { export interface FileStatPayload { byteSize: string; modifiedAtNs: string; + changedAtNs?: string; } export async function statFile(path: string): Promise { From ca403f306860915f3973edab2c2b5bb5ebb1d7e4 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 24 Aug 2026 17:27:45 +1000 Subject: [PATCH 3/6] fix(chat): stabilize artifact image polling Recheck image fingerprints after asynchronous decode before accepting cache-busted content, and run polled metadata inspection on Tokio's blocking pool. Add focused coverage for decode-time file changes and async metadata execution. Signed-off-by: Matt Toohey --- src-tauri/src/commands/system.rs | 46 +++++++++++++----- src/features/chat/ui/ArtifactViewer.tsx | 27 ++++++++++- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 47 +++++++++++++++++++ 3 files changed, 106 insertions(+), 14 deletions(-) diff --git a/src-tauri/src/commands/system.rs b/src-tauri/src/commands/system.rs index 938f6332a..4a03c9f3d 100644 --- a/src-tauri/src/commands/system.rs +++ b/src-tauri/src/commands/system.rs @@ -902,10 +902,7 @@ fn windows_file_change_time_ns(path: &Path) -> Result { Ok((i128::from(info.ChangeTime) * 100).to_string()) } -/// Return the metadata identity used by open artifact viewers to detect writes -/// that do not appear in the main ACP session's tool events. -#[tauri::command] -pub fn stat_file(path: String) -> Result { +fn stat_file_blocking(path: String) -> Result { let target = Path::new(&path); let metadata = fs::metadata(target) .map_err(|error| format!("Failed to inspect '{}': {}", target.display(), error))?; @@ -943,6 +940,24 @@ pub fn stat_file(path: String) -> Result { }) } +async fn stat_file_with(path: String, operation: F) -> Result +where + F: FnOnce(String) -> Result + Send + 'static, +{ + tokio::task::spawn_blocking(move || operation(path)) + .await + .map_err(|error| format!("Failed to inspect file metadata: {error}"))? +} + +/// Return the metadata identity used by open artifact viewers to detect writes +/// that do not appear in the main ACP session's tool events. Filesystem metadata +/// calls are blocking and may wait on remote or removable filesystems, so keep +/// them off Tauri's async command thread. +#[tauri::command] +pub async fn stat_file(path: String) -> Result { + stat_file_with(path, stat_file_blocking).await +} + fn looks_binary(bytes: &[u8]) -> bool { bytes .iter() @@ -2093,9 +2108,9 @@ mod tests { get_or_build_file_mention_index_from_cache, inspect_attachment_path, inspect_attachment_paths, normalize_attachment_paths, normalize_roots, open_in_chrome_with, read_directory_entries, read_image_attachment, read_text_file, - search_file_mentions_blocking, signed_unix_timestamp_ns, stat_file, - write_agent_image_atomically, write_sibling_then_replace, FileMentionIndexCache, - MAX_IMAGE_ATTACHMENT_BYTES, MAX_TEXT_FILE_BYTES, + search_file_mentions_blocking, signed_unix_timestamp_ns, stat_file_blocking, + stat_file_with, write_agent_image_atomically, write_sibling_then_replace, + FileMentionIndexCache, MAX_IMAGE_ATTACHMENT_BYTES, MAX_TEXT_FILE_BYTES, }; use base64::Engine; use std::fs; @@ -2948,13 +2963,19 @@ mod tests { assert!(!payload.base64.is_empty()); } - #[test] - fn stat_file_returns_size_and_metadata_times() { + #[tokio::test(flavor = "current_thread")] + async fn stat_file_async_command_moves_metadata_work_off_the_runtime_thread() { let dir = tempdir().expect("tempdir"); let path = dir.path().join("notes.md"); fs::write(&path, "hello").expect("write"); + let runtime_thread = std::thread::current().id(); - let payload = stat_file(path.to_string_lossy().into_owned()).expect("stat file"); + let payload = stat_file_with(path.to_string_lossy().into_owned(), move |path| { + assert_ne!(std::thread::current().id(), runtime_thread); + stat_file_blocking(path) + }) + .await + .expect("stat file"); assert_eq!(payload.byte_size, "5"); assert!(payload.modified_at_ns.parse::().expect("timestamp") > 0); #[cfg(unix)] @@ -2977,14 +2998,15 @@ mod tests { file.set_times(fs::FileTimes::new().set_modified(old_timestamp)) .expect("set pre-epoch mtime"); - let payload = stat_file(path.to_string_lossy().into_owned()).expect("stat old file"); + let payload = + stat_file_blocking(path.to_string_lossy().into_owned()).expect("stat old file"); assert_eq!(payload.modified_at_ns, "-1000000000"); } #[test] fn stat_file_rejects_directories() { let dir = tempdir().expect("tempdir"); - let error = stat_file(dir.path().to_string_lossy().into_owned()) + let error = stat_file_blocking(dir.path().to_string_lossy().into_owned()) .expect_err("directory should error"); assert!(error.contains("not a file"), "unexpected error: {error}"); } diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 7af9c3049..0bc7f56d4 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -192,14 +192,26 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { artifact.resolvedPath, artifact.revision + candidateDiskRevision, ); + let confirmedFingerprint = before; if (shouldBustImageCache) { await preloadArtifactImage(candidateSrc); if (!isCurrentRefresh()) return; + confirmedFingerprint = await statFile(artifact.resolvedPath); + if ( + !isCurrentRefresh() || + !sameFingerprint(before, confirmedFingerprint) + ) { + updateDiskStatus("diverged"); + return; + } imageDiskRevisionRef.current = candidateDiskRevision; consumedRetryRevisionRef.current = retryRevision; setImageDiskRevision(candidateDiskRevision); } - pendingImageRef.current = { src: candidateSrc, fingerprint: before }; + pendingImageRef.current = { + src: candidateSrc, + fingerprint: confirmedFingerprint, + }; if (loadedImageSrcRef.current === candidateSrc) { fingerprintRef.current = before; pendingImageRef.current = null; @@ -298,7 +310,18 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { ); await preloadArtifactImage(candidateSrc); if (!isCurrentPoll()) return; - pendingImageRef.current = { src: candidateSrc, fingerprint }; + const confirmedFingerprint = await statFile(artifact.resolvedPath); + if ( + !isCurrentPoll() || + !sameFingerprint(fingerprint, confirmedFingerprint) + ) { + updateDiskStatus("diverged"); + return; + } + pendingImageRef.current = { + src: candidateSrc, + fingerprint: confirmedFingerprint, + }; imageDiskRevisionRef.current = candidateDiskRevision; setImageDiskRevision(candidateDiskRevision); return; diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index f4a35c317..c7589916c 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -342,6 +342,53 @@ describe("ArtifactViewer header actions", () => { expect(screen.queryByRole("status")).not.toBeInTheDocument(); }); + it("rejects a preloaded image when its fingerprint changes during decode", async () => { + vi.useFakeTimers(); + let version = "1"; + mockStatFile.mockImplementation(async () => ({ + byteSize: "20", + modifiedAtNs: version, + })); + let finishPreload: (() => void) | undefined; + class PreloadImage { + onload: (() => void) | null = null; + + set src(_value: string) { + finishPreload = () => this.onload?.(); + } + } + vi.stubGlobal("Image", PreloadImage); + + render( + , + ); + await act(flushAsyncWork); + const renderedImage = screen.getByRole("img"); + fireEvent.load(renderedImage); + + version = "2"; + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + expect(finishPreload).toBeDefined(); + + version = "3"; + await act(async () => { + finishPreload?.(); + await flushAsyncWork(); + }); + + expect(renderedImage).toHaveAttribute( + "src", + "asset://localhost//p/shot.png?rev=4", + ); + expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + }); + it("preloads and renders the same image URL after a polled change", async () => { vi.useFakeTimers(); let changed = false; From da02f7fb65ffbf8d819c8f622a8009fe7ab5d8ea Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 24 Aug 2026 19:43:31 +1000 Subject: [PATCH 4/6] fix(chat): reduce background artifact polling Poll visible artifacts every ten seconds while Berd is unfocused, check immediately when focus returns, and restore the foreground interval. Cover background timing and focus recovery. Signed-off-by: Matt Toohey --- src/features/chat/ui/ArtifactViewer.tsx | 47 +++++++++-- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 81 +++++++++++++++++++ 2 files changed, 120 insertions(+), 8 deletions(-) diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 0bc7f56d4..3f6cb9889 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -60,7 +60,8 @@ interface FileFingerprint { changedAtNs?: string; } -const ARTIFACT_POLL_INTERVAL_MS = 1_500; +const FOREGROUND_ARTIFACT_POLL_INTERVAL_MS = 1_500; +const BACKGROUND_ARTIFACT_POLL_INTERVAL_MS = 10_000; function sameFingerprint( left: FileFingerprint, @@ -268,10 +269,12 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { // Tool events cannot account for shell writes, delegated subagents, or // external editors. Poll the one open file while this document is visible, - // including an immediate check on return from the background. + // slowing down when the app is not focused and checking immediately when it + // returns to the foreground. useEffect(() => { let cancelled = false; let checkInFlight = false; + let pollTimerId: number | null = null; const checkForDiskChange = async () => { if ( @@ -352,21 +355,49 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { } }; + const clearPollTimer = () => { + if (pollTimerId !== null) { + window.clearTimeout(pollTimerId); + pollTimerId = null; + } + }; + const scheduleNextPoll = () => { + clearPollTimer(); + if (cancelled || document.visibilityState === "hidden") return; + + const interval = document.hasFocus() + ? FOREGROUND_ARTIFACT_POLL_INTERVAL_MS + : BACKGROUND_ARTIFACT_POLL_INTERVAL_MS; + pollTimerId = window.setTimeout(() => { + pollTimerId = null; + void checkForDiskChange().finally(scheduleNextPoll); + }, interval); + }; + const handleFocus = () => { + clearPollTimer(); + void checkForDiskChange().finally(scheduleNextPoll); + }; + const handleBlur = () => { + scheduleNextPoll(); + }; const handleVisibilityChange = () => { + clearPollTimer(); if (document.visibilityState !== "hidden") { - void checkForDiskChange(); + void checkForDiskChange().finally(scheduleNextPoll); } }; - const intervalId = window.setInterval( - () => void checkForDiskChange(), - ARTIFACT_POLL_INTERVAL_MS, - ); + + scheduleNextPoll(); + window.addEventListener("focus", handleFocus); + window.addEventListener("blur", handleBlur); document.addEventListener("visibilitychange", handleVisibilityChange); return () => { cancelled = true; pollGenerationRef.current += 1; - window.clearInterval(intervalId); + clearPollTimer(); + window.removeEventListener("focus", handleFocus); + window.removeEventListener("blur", handleBlur); document.removeEventListener("visibilitychange", handleVisibilityChange); }; }, [ diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index c7589916c..20a25a022 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -69,6 +69,7 @@ async function openFileActionsMenu() { describe("ArtifactViewer header actions", () => { beforeEach(() => { + vi.spyOn(document, "hasFocus").mockReturnValue(true); mockOpenResolvedPath.mockClear(); mockRevealInFileManager.mockClear(); mockReadTextFile.mockReset(); @@ -182,6 +183,86 @@ describe("ArtifactViewer header actions", () => { expect(screen.queryByText(/out of date/i)).not.toBeInTheDocument(); }); + it("slows polling to ten seconds while the app is not foregrounded", async () => { + vi.useFakeTimers(); + vi.mocked(document.hasFocus).mockReturnValue(false); + let changed = false; + mockReadTextFile.mockImplementation(async () => ({ + contents: changed ? "# Background update" : "# Original", + })); + mockStatFile.mockImplementation(async () => + changed + ? { byteSize: "20", modifiedAtNs: "2" } + : { byteSize: "10", modifiedAtNs: "1" }, + ); + + render(); + await act(flushAsyncWork); + expect( + screen.getByRole("heading", { name: "Original" }), + ).toBeInTheDocument(); + + changed = true; + await act(async () => { + vi.advanceTimersByTime(9_999); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Original" }), + ).toBeInTheDocument(); + + await act(async () => { + vi.advanceTimersByTime(1); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Background update" }), + ).toBeInTheDocument(); + }); + + it("checks immediately on focus and restores foreground polling", async () => { + vi.useFakeTimers(); + vi.mocked(document.hasFocus).mockReturnValue(false); + let version = 0; + mockReadTextFile.mockImplementation(async () => ({ + contents: `# Version ${version}`, + })); + mockStatFile.mockImplementation(async () => ({ + byteSize: String(10 + version), + modifiedAtNs: String(version), + })); + + render(); + await act(flushAsyncWork); + + version = 1; + vi.mocked(document.hasFocus).mockReturnValue(true); + await act(async () => { + window.dispatchEvent(new Event("focus")); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Version 1" }), + ).toBeInTheDocument(); + + version = 2; + await act(async () => { + vi.advanceTimersByTime(1_499); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Version 1" }), + ).toBeInTheDocument(); + + await act(async () => { + vi.advanceTimersByTime(1); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Version 2" }), + ).toBeInTheDocument(); + }); + it("detects same-size same-mtime rewrites from change time", async () => { vi.useFakeTimers(); let changed = false; From 599ec747d89bc07ee2b8da0afa0b38938daec6e9 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Tue, 25 Aug 2026 20:10:39 +1000 Subject: [PATCH 5/6] fix(chat): add stale-view grace period, dimming, and deleted-file handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** fix **User Impact:** The artifact viewer no longer flashes a stale warning for routine one-off file races, visibly dims content that really is stale, and tells users when the file was deleted instead of offering a reload that cannot succeed. **Problem:** The viewer flagged the view as diverged on the very first failed poll cycle, including torn writes where the file was simply mid-rewrite, so agents rewriting files tripped the warning constantly. The single warning message also conflated "the file changed but can't be read" with "the file is gone", and offered a pointless Reload button for deleted files. **Solution:** Polling failures now get a two-strike grace period: one failed cycle changes nothing, two consecutive failures flag the view. Torn-write fingerprint mismatches neither flag nor strike — the next cycle retries against the settled file. User-initiated reloads bypass the grace period so the user gets an immediate answer, and any successful cycle resets the streak. The Rust `stat_file` command now returns a structured error distinguishing missing files from other failures, the warning strip shows deletion-specific copy without a Reload button when the file is gone, and diverged content dims to 60% opacity so the strip clearly describes it. ## Verification - `just check` - `just test` (6,900 passed, 1 skipped) - `just tauri-check` - `just tauri-test` - `just clippy` - `just tauri-fmt-check`
File changes **src-tauri/src/commands/system.rs** Return a structured `FileStatError { kind, message }` from `stat_file` with a `missing`/`other` kind so deletion survives the IPC boundary; update the stat tests and pin the serialized discriminant. **src/shared/api/system.ts** Add the `FileStatErrorKind`/`FileStatError` types and the `fileStatErrorKind()` narrowing helper for command rejections. **src/features/chat/ui/ArtifactViewer.tsx** Add the two-strike divergence grace period, treat torn-write fingerprint mismatches as no verdict, flag failed user reloads immediately, dim diverged content, and split the warning strip into deleted (no Reload) and unreadable (with Reload) cases. **src/features/chat/ui/__tests__/ArtifactViewer.test.tsx** Cover the grace period, torn-write healing, deleted vs unreadable copy and Reload visibility, immediate user-reload flagging, dimming, and strike reset on recovery. **src/shared/i18n/locales/en/chat.json** / **src/shared/i18n/locales/es/chat.json** Replace `artifactViewer.diskDiverged` with `fileDeleted` and `fileUnreadable` in both locales.
Co-authored-by: goose --- src-tauri/src/commands/system.rs | 91 ++++++- src/features/chat/ui/ArtifactViewer.tsx | 153 ++++++++--- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 251 ++++++++++++++++-- src/shared/api/system.ts | 24 ++ src/shared/i18n/locales/en/chat.json | 3 +- src/shared/i18n/locales/es/chat.json | 3 +- 6 files changed, 458 insertions(+), 67 deletions(-) diff --git a/src-tauri/src/commands/system.rs b/src-tauri/src/commands/system.rs index 4a03c9f3d..264a9055d 100644 --- a/src-tauri/src/commands/system.rs +++ b/src-tauri/src/commands/system.rs @@ -857,6 +857,42 @@ pub struct FileStatPayload { pub changed_at_ns: Option, } +#[derive(Serialize, Clone, Copy, Debug, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum FileStatErrorKind { + /// The path does not exist. Deleted artifacts get distinct messaging in + /// the viewer, so this case must survive the IPC boundary. + Missing, + /// Any other metadata failure (permissions, transient I/O, not a file). + Other, +} + +/// Structured `stat_file` failure. Tauri serializes the command's `Err` +/// payload into the JavaScript rejection value, so the renderer can +/// distinguish a deleted file from other metadata failures. +#[derive(Serialize, Clone, Debug, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct FileStatError { + pub kind: FileStatErrorKind, + pub message: String, +} + +impl FileStatError { + fn missing(message: String) -> Self { + Self { + kind: FileStatErrorKind::Missing, + message, + } + } + + fn other(message: String) -> Self { + Self { + kind: FileStatErrorKind::Other, + message, + } + } +} + fn signed_unix_timestamp_ns(time: SystemTime) -> String { match time.duration_since(UNIX_EPOCH) { Ok(duration) => duration.as_nanos().to_string(), @@ -902,23 +938,32 @@ fn windows_file_change_time_ns(path: &Path) -> Result { Ok((i128::from(info.ChangeTime) * 100).to_string()) } -fn stat_file_blocking(path: String) -> Result { +fn stat_file_blocking(path: String) -> Result { let target = Path::new(&path); - let metadata = fs::metadata(target) - .map_err(|error| format!("Failed to inspect '{}': {}", target.display(), error))?; + let metadata = fs::metadata(target).map_err(|error| { + let message = format!("Failed to inspect '{}': {}", target.display(), error); + if error.kind() == io::ErrorKind::NotFound { + FileStatError::missing(message) + } else { + FileStatError::other(message) + } + })?; if !metadata.is_file() { - return Err(format!("Path is not a file: {}", target.display())); + return Err(FileStatError::other(format!( + "Path is not a file: {}", + target.display() + ))); } let modified_at_ns = metadata .modified() .map(signed_unix_timestamp_ns) .map_err(|error| { - format!( + FileStatError::other(format!( "Failed to read modification time for '{}': {}", target.display(), error - ) + )) })?; #[cfg(unix)] @@ -929,7 +974,7 @@ fn stat_file_blocking(path: String) -> Result { Some(nanoseconds.to_string()) }; #[cfg(windows)] - let changed_at_ns = Some(windows_file_change_time_ns(target)?); + let changed_at_ns = Some(windows_file_change_time_ns(target).map_err(FileStatError::other)?); #[cfg(not(any(unix, windows)))] let changed_at_ns = None; @@ -940,13 +985,15 @@ fn stat_file_blocking(path: String) -> Result { }) } -async fn stat_file_with(path: String, operation: F) -> Result +async fn stat_file_with(path: String, operation: F) -> Result where - F: FnOnce(String) -> Result + Send + 'static, + F: FnOnce(String) -> Result + Send + 'static, { tokio::task::spawn_blocking(move || operation(path)) .await - .map_err(|error| format!("Failed to inspect file metadata: {error}"))? + .map_err(|error| { + FileStatError::other(format!("Failed to inspect file metadata: {error}")) + })? } /// Return the metadata identity used by open artifact viewers to detect writes @@ -954,7 +1001,7 @@ where /// calls are blocking and may wait on remote or removable filesystems, so keep /// them off Tauri's async command thread. #[tauri::command] -pub async fn stat_file(path: String) -> Result { +pub async fn stat_file(path: String) -> Result { stat_file_with(path, stat_file_blocking).await } @@ -2110,7 +2157,7 @@ mod tests { read_directory_entries, read_image_attachment, read_text_file, search_file_mentions_blocking, signed_unix_timestamp_ns, stat_file_blocking, stat_file_with, write_agent_image_atomically, write_sibling_then_replace, - FileMentionIndexCache, MAX_IMAGE_ATTACHMENT_BYTES, MAX_TEXT_FILE_BYTES, + FileMentionIndexCache, FileStatErrorKind, MAX_IMAGE_ATTACHMENT_BYTES, MAX_TEXT_FILE_BYTES, }; use base64::Engine; use std::fs; @@ -3008,7 +3055,25 @@ mod tests { let dir = tempdir().expect("tempdir"); let error = stat_file_blocking(dir.path().to_string_lossy().into_owned()) .expect_err("directory should error"); - assert!(error.contains("not a file"), "unexpected error: {error}"); + assert_eq!(error.kind, FileStatErrorKind::Other); + assert!( + error.message.contains("not a file"), + "unexpected error: {}", + error.message + ); + } + + #[test] + fn stat_file_reports_missing_files_with_the_missing_kind() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("deleted.md"); + let error = stat_file_blocking(path.to_string_lossy().into_owned()) + .expect_err("missing file should error"); + assert_eq!(error.kind, FileStatErrorKind::Missing); + // The kind must cross the IPC boundary as the exact discriminant the + // renderer matches on. + let serialized = serde_json::to_value(&error).expect("serialize"); + assert_eq!(serialized["kind"], "missing"); } #[test] diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 3f6cb9889..676e714ae 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -28,7 +28,13 @@ import { } from "@/shared/ui/dropdown-menu"; import { Spinner } from "@/shared/ui/spinner"; import { ToggleGroup, ToggleGroupItem } from "@/shared/ui/toggle-group"; -import { readTextFile, statFile } from "@/shared/api/system"; +import { + fileStatErrorKind, + readTextFile, + statFile, + type FileStatErrorKind, +} from "@/shared/api/system"; +import { cn } from "@/shared/lib/cn"; import { revealInFileManager } from "@/shared/lib/fileManager"; import { getPlatform } from "@/shared/lib/platform"; import { useArtifactActionsContext } from "@/features/chat/hooks/ArtifactPolicyContext"; @@ -62,6 +68,10 @@ interface FileFingerprint { const FOREGROUND_ARTIFACT_POLL_INTERVAL_MS = 1_500; const BACKGROUND_ARTIFACT_POLL_INTERVAL_MS = 10_000; +// Consecutive failed poll cycles tolerated before the stale warning shows. A +// single failure is routinely a file mid-rewrite or a transient I/O hiccup; +// two in a row is a real divergence worth surfacing. +const DIVERGENCE_STRIKE_THRESHOLD = 2; function sameFingerprint( left: FileFingerprint, @@ -91,6 +101,8 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { const fingerprintRef = useRef(null); const [diskStatus, setDiskStatus] = useState("checking"); const diskStatusRef = useRef(diskStatus); + const [divergedKind, setDivergedKind] = useState("other"); + const divergenceStrikesRef = useRef(0); const [imageDiskRevision, setImageDiskRevision] = useState(0); const imageDiskRevisionRef = useRef(0); const [retryRevision, setRetryRevision] = useState(0); @@ -124,7 +136,29 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { const updateDiskStatus = useCallback((next: DiskStatus) => { diskStatusRef.current = next; setDiskStatus(next); + // Any non-diverged outcome ends the current failure streak, so recovery + // both clears the warning and re-arms the full grace period. + if (next !== "diverged") divergenceStrikesRef.current = 0; }, []); + const flagDiverged = useCallback( + (kind: FileStatErrorKind) => { + setDivergedKind(kind); + updateDiskStatus("diverged"); + }, + [updateDiskStatus], + ); + // Polling failures get a grace period: one failed cycle is routinely a file + // mid-rewrite or a transient I/O error, so only consecutive failures flag + // the view as diverged. User-initiated reloads bypass this via flagDiverged. + const recordDivergenceStrike = useCallback( + (kind: FileStatErrorKind) => { + divergenceStrikesRef.current += 1; + if (divergenceStrikesRef.current >= DIVERGENCE_STRIKE_THRESHOLD) { + flagDiverged(kind); + } + }, + [flagDiverged], + ); // Escape closes the viewer — but only when nothing closer to the event // already handled it (open menus, dialogs, transcript search, etc.). @@ -174,6 +208,11 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { if (pathChanged || diskStatusRef.current !== "diverged") { updateDiskStatus("checking"); } + // An unconsumed retry revision means the user pressed Reload. That intent + // matters on failure: the user asked "is the file back?", so the answer is + // immediate rather than smoothed over by the polling grace period. + const userReloadRequested = + retryRevision !== consumedRetryRevisionRef.current; // Reading this value makes the ACP-driven revision an explicit input to // this request even though only its change, not its numeric value, matters. @@ -184,8 +223,7 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { if (!isCurrentRefresh()) return; if (viewMode === "image") { - const shouldBustImageCache = - retryRevision !== consumedRetryRevisionRef.current; + const shouldBustImageCache = userReloadRequested; const candidateDiskRevision = shouldBustImageCache ? imageDiskRevisionRef.current + 1 : imageDiskRevisionRef.current; @@ -198,11 +236,11 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { await preloadArtifactImage(candidateSrc); if (!isCurrentRefresh()) return; confirmedFingerprint = await statFile(artifact.resolvedPath); - if ( - !isCurrentRefresh() || - !sameFingerprint(before, confirmedFingerprint) - ) { - updateDiskStatus("diverged"); + if (!isCurrentRefresh()) return; + if (!sameFingerprint(before, confirmedFingerprint)) { + // Torn write: the file changed while the image was decoding. + // Leave the current state alone and let the next cycle retry + // against the settled file. return; } imageDiskRevisionRef.current = candidateDiskRevision; @@ -225,7 +263,9 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { const after = await statFile(artifact.resolvedPath); if (!isCurrentRefresh()) return; if (!sameFingerprint(before, after)) { - updateDiskStatus("diverged"); + // Torn write: the file changed underneath the read, so neither the + // fetched contents nor a divergence verdict is trustworthy. Keep the + // current state and let the next poll cycle retry the settled file. return; } @@ -236,14 +276,30 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { ) { updateTextState({ status: "loaded", contents: payload.contents }); } + consumedRetryRevisionRef.current = retryRevision; updateDiskStatus("current"); - } catch { + } catch (error) { if (!isCurrentRefresh()) return; - if (textStateRef.current.status === "loaded") { - updateDiskStatus("diverged"); - } else { + const kind = fileStatErrorKind(error); + const hasLastGoodView = textStateRef.current.status === "loaded"; + if (!hasLastGoodView) { updateTextState({ status: "error", contents: "" }); - updateDiskStatus("diverged"); + } + if (userReloadRequested) { + // The user explicitly asked whether the file is back, so answer + // immediately instead of smoothing the failure over with the + // polling grace period. + consumedRetryRevisionRef.current = retryRevision; + flagDiverged(kind); + } else if (hasLastGoodView) { + // ACP-driven re-open of already-rendered content: tool writes + // routinely race the re-read, so give the failure the same grace + // as a polling failure. + recordDivergenceStrike(kind); + } else { + // Initial load (or path change) failure: there is no last-good view + // to protect, so the error state and warning show immediately. + flagDiverged(kind); } } finally { finishRefresh(); @@ -261,6 +317,8 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { artifact.resolvedPath, artifact.revision, contentReadRevision, + flagDiverged, + recordDivergenceStrike, retryRevision, updateDiskStatus, updateTextState, @@ -314,11 +372,11 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { await preloadArtifactImage(candidateSrc); if (!isCurrentPoll()) return; const confirmedFingerprint = await statFile(artifact.resolvedPath); - if ( - !isCurrentPoll() || - !sameFingerprint(fingerprint, confirmedFingerprint) - ) { - updateDiskStatus("diverged"); + if (!isCurrentPoll()) return; + if (!sameFingerprint(fingerprint, confirmedFingerprint)) { + // Torn write: the file changed while the image was decoding, so + // the decoded bytes are already stale. Neither flag nor strike — + // the next cycle retries against the settled file. return; } pendingImageRef.current = { @@ -333,11 +391,11 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { const payload = await readTextFile(artifact.resolvedPath); if (!isCurrentPoll()) return; const confirmedFingerprint = await statFile(artifact.resolvedPath); - if ( - !isCurrentPoll() || - !sameFingerprint(fingerprint, confirmedFingerprint) - ) { - updateDiskStatus("diverged"); + if (!isCurrentPoll()) return; + if (!sameFingerprint(fingerprint, confirmedFingerprint)) { + // Torn write: the fingerprint moved during the read, so the fetched + // contents describe no settled file version. Neither flag nor + // strike — the next cycle retries. return; } fingerprintRef.current = confirmedFingerprint; @@ -348,8 +406,8 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { updateTextState({ status: "loaded", contents: payload.contents }); } updateDiskStatus("current"); - } catch { - if (isCurrentPoll()) updateDiskStatus("diverged"); + } catch (error) { + if (isCurrentPoll()) recordDivergenceStrike(fileStatErrorKind(error)); } finally { checkInFlight = false; } @@ -403,6 +461,7 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { }, [ artifact.resolvedPath, artifact.revision, + recordDivergenceStrike, updateDiskStatus, updateTextState, viewMode, @@ -494,19 +553,36 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { role="status" className="flex items-center justify-between gap-3 border-b border-border bg-muted/60 px-4 py-2 text-xs text-muted-foreground" > - {t("artifactViewer.diskDiverged")} - + + {divergedKind === "missing" + ? t("artifactViewer.fileDeleted") + : t("artifactViewer.fileUnreadable")} + + {/* A deleted file has nothing to reload; the strip is the whole + answer. Polling keeps retrying, so if the file reappears the + view heals without user action. */} + {divergedKind !== "missing" ? ( + + ) : null}
) : null} -
+ {/* Dim the stale body while diverged so the warning strip reads as + describing the content, not competing with it. "checking" stays at + full opacity — routine polls must not flicker the view. */} +
{viewMode === "image" ? ( ) : ( diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index 20a25a022..60e8854ff 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -27,14 +27,21 @@ vi.mock("@/shared/lib/fileManager", () => ({ revealInFileManager: (path: string) => mockRevealInFileManager(path), })); -vi.mock("@/shared/api/system", () => ({ - readTextFile: (path: string) => mockReadTextFile(path), - statFile: (path: string) => mockStatFile(path), -})); +// Keep the real `fileStatErrorKind` narrowing helper so these tests exercise +// the same "missing" vs "other" classification the app ships. +vi.mock("@/shared/api/system", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readTextFile: (path: string) => mockReadTextFile(path), + statFile: (path: string) => mockStatFile(path), + }; +}); // jsdom has no Tauri internals, so the real asset-URL converter throws. vi.mock("@tauri-apps/api/core", () => ({ convertFileSrc: (path: string) => `asset://localhost/${path}`, + invoke: vi.fn(), })); function artifact(path = "/p/report.md", revision = 0) { @@ -180,7 +187,7 @@ describe("ArtifactViewer header actions", () => { expect( screen.getByRole("heading", { name: "Updated externally" }), ).toBeInTheDocument(); - expect(screen.queryByText(/out of date/i)).not.toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); }); it("slows polling to ten seconds while the app is not foregrounded", async () => { @@ -307,17 +314,22 @@ describe("ArtifactViewer header actions", () => { await Promise.resolve(); }); + // Two consecutive failed cycles: the first is inside the grace period. changed = true; await act(async () => { vi.advanceTimersByTime(1_500); - await Promise.resolve(); - await Promise.resolve(); + await flushAsyncWork(); + }); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); }); expect( screen.getByRole("heading", { name: "Last good copy" }), ).toBeInTheDocument(); - expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + expect(screen.getByRole("status")).toHaveTextContent(/can't be read/i); expect(screen.getByRole("button", { name: /reload/i })).toBeInTheDocument(); // A later unchanged stat must not silently clear the warning: the viewer @@ -325,10 +337,9 @@ describe("ArtifactViewer header actions", () => { mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "2" }); await act(async () => { vi.advanceTimersByTime(1_500); - await Promise.resolve(); - await Promise.resolve(); + await flushAsyncWork(); }); - expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + expect(screen.getByRole("status")).toHaveTextContent(/can't be read/i); }); it("recovers an initially failed empty text file to loaded state", async () => { @@ -403,7 +414,9 @@ describe("ArtifactViewer header actions", () => { initialStat.resolve({ byteSize: "20", modifiedAtNs: "1" }); await act(flushAsyncWork); // A late successful stat must not overwrite the earlier decode failure. - expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + // A rendered decode failure is already visibly broken, so it flags + // immediately with the unreadable copy — no grace period. + expect(screen.getByRole("status")).toHaveTextContent(/can't be read/i); rerender( { "src", "asset://localhost//p/shot.png?rev=5", ); - expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + expect(screen.getByRole("status")).toHaveTextContent(/can't be read/i); fireEvent.load(refreshedImage); expect(screen.queryByRole("status")).not.toBeInTheDocument(); }); - it("rejects a preloaded image when its fingerprint changes during decode", async () => { + it("rejects a preloaded image whose fingerprint changed during decode and heals next cycle", async () => { vi.useFakeTimers(); let version = "1"; mockStatFile.mockImplementation(async () => ({ @@ -463,11 +476,31 @@ describe("ArtifactViewer header actions", () => { await flushAsyncWork(); }); + // Torn write: the decoded bytes belong to no settled file version. The + // rendered image is untouched and no warning appears — the view is not + // wrong, only mid-transition. expect(renderedImage).toHaveAttribute( "src", "asset://localhost//p/shot.png?rev=4", ); - expect(screen.getByRole("status")).toHaveTextContent(/out of date/i); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + + // The next cycle sees a settled file and swaps the fresh image in. + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + await act(async () => { + finishPreload?.(); + await flushAsyncWork(); + }); + const refreshedImage = screen.getByRole("img"); + expect(refreshedImage).toHaveAttribute( + "src", + "asset://localhost//p/shot.png?rev=5", + ); + fireEvent.load(refreshedImage); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); }); it("preloads and renders the same image URL after a polled change", async () => { @@ -511,3 +544,191 @@ describe("ArtifactViewer header actions", () => { expect(screen.queryByRole("status")).not.toBeInTheDocument(); }); }); + +describe("ArtifactViewer divergence grace period", () => { + beforeEach(() => { + vi.spyOn(document, "hasFocus").mockReturnValue(true); + vi.useFakeTimers(); + mockReadTextFile.mockReset(); + mockReadTextFile.mockResolvedValue({ contents: "# Loaded fine" }); + mockStatFile.mockReset(); + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + function contentBody() { + // The scroll container wrapping the markdown/raw/image body is what dims. + return screen + .getByRole("heading", { name: "Loaded fine" }) + .closest(".overflow-auto") as HTMLElement; + } + + async function renderLoadedViewer() { + render(); + await act(flushAsyncWork); + expect( + screen.getByRole("heading", { name: "Loaded fine" }), + ).toBeInTheDocument(); + } + + async function advancePollCycle() { + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + } + + it("keeps the view clean through a single transient stat failure", async () => { + await renderLoadedViewer(); + + mockStatFile.mockRejectedValueOnce({ + kind: "other", + message: "transient I/O", + }); + await advancePollCycle(); + + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + expect(contentBody().className).not.toMatch(/\bopacity-60\b/); + + // The next cycle succeeds, so the streak resets and no warning ever shows. + await advancePollCycle(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + expect(contentBody().className).not.toMatch(/\bopacity-60\b/); + }); + + it("shows the warning strip and dims the body after two consecutive failures", async () => { + await renderLoadedViewer(); + + mockStatFile.mockRejectedValue({ kind: "other", message: "io error" }); + await advancePollCycle(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + await advancePollCycle(); + + expect(screen.getByRole("status")).toHaveTextContent( + "File changed but can't be read.", + ); + expect(contentBody().className).toMatch(/\bopacity-60\b/); + }); + + it("treats a torn-write fingerprint mismatch as no verdict and heals next cycle", async () => { + let phase: "settled" | "torn" | "updated" = "settled"; + let statCalls = 0; + mockStatFile.mockImplementation(async () => { + statCalls += 1; + if (phase === "settled") return { byteSize: "20", modifiedAtNs: "1" }; + if (phase === "torn") { + // The confirm stat (even call) disagrees with the cycle's first stat. + return statCalls % 2 === 0 + ? { byteSize: "30", modifiedAtNs: "3" } + : { byteSize: "25", modifiedAtNs: "2" }; + } + return { byteSize: "30", modifiedAtNs: "3" }; + }); + mockReadTextFile.mockImplementation(async () => ({ + contents: phase === "settled" ? "# Loaded fine" : "# Settled rewrite", + })); + await renderLoadedViewer(); + + phase = "torn"; + statCalls = 0; + await advancePollCycle(); + // No flag, no strike consumed toward the threshold: torn reads carry no + // information about whether the view is actually stale. + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + expect(contentBody().className).not.toMatch(/\bopacity-60\b/); + + phase = "updated"; + await advancePollCycle(); + expect( + screen.getByRole("heading", { name: "Settled rewrite" }), + ).toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("reports a deleted file without offering a pointless reload", async () => { + await renderLoadedViewer(); + + mockStatFile.mockRejectedValue({ + kind: "missing", + message: "no such file", + }); + await advancePollCycle(); + await advancePollCycle(); + + expect(screen.getByRole("status")).toHaveTextContent( + "File deleted from disk.", + ); + expect( + screen.queryByRole("button", { name: /reload/i }), + ).not.toBeInTheDocument(); + expect(contentBody().className).toMatch(/\bopacity-60\b/); + }); + + it("reports an unreadable file with a reload action", async () => { + await renderLoadedViewer(); + + mockStatFile.mockRejectedValue({ kind: "other", message: "EACCES" }); + await advancePollCycle(); + await advancePollCycle(); + + expect(screen.getByRole("status")).toHaveTextContent( + "File changed but can't be read.", + ); + expect(screen.getByRole("button", { name: /reload/i })).toBeInTheDocument(); + }); + + it("flags a failed user-initiated reload immediately, bypassing the grace period", async () => { + // Fail the initial load so the strip shows the "other" copy with a Reload + // button while the strike counter sits at zero. A reload failure inside a + // grace period would then be strike one of two and change nothing; the + // bypass instead answers the user on the very first failure. + mockStatFile.mockRejectedValueOnce({ kind: "other", message: "EACCES" }); + render(); + await act(flushAsyncWork); + expect(screen.getByRole("status")).toHaveTextContent( + "File changed but can't be read.", + ); + + // The user presses Reload; by now the file has been deleted outright. + mockStatFile.mockRejectedValue({ + kind: "missing", + message: "no such file", + }); + fireEvent.click(screen.getByRole("button", { name: /reload/i })); + await act(flushAsyncWork); + + // One failure, immediate verdict: the strip re-describes the divergence + // as a deletion and drops the now-pointless Reload button. + expect(screen.getByRole("status")).toHaveTextContent( + "File deleted from disk.", + ); + expect( + screen.queryByRole("button", { name: /reload/i }), + ).not.toBeInTheDocument(); + }); + + it("clears the strip and dim on recovery and re-arms the full grace period", async () => { + await renderLoadedViewer(); + + mockStatFile.mockRejectedValue({ kind: "other", message: "io error" }); + await advancePollCycle(); + await advancePollCycle(); + expect(screen.getByRole("status")).toBeInTheDocument(); + expect(contentBody().className).toMatch(/\bopacity-60\b/); + + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); + await advancePollCycle(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + expect(contentBody().className).not.toMatch(/\bopacity-60\b/); + + // Strikes reset on recovery: a later single failure is back inside the + // grace period rather than continuing the old streak. + mockStatFile.mockRejectedValueOnce({ kind: "other", message: "blip" }); + await advancePollCycle(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); +}); diff --git a/src/shared/api/system.ts b/src/shared/api/system.ts index b64e8c4cf..266313840 100644 --- a/src/shared/api/system.ts +++ b/src/shared/api/system.ts @@ -176,6 +176,30 @@ export interface FileStatPayload { changedAtNs?: string; } +export type FileStatErrorKind = "missing" | "other"; + +export interface FileStatError { + kind: FileStatErrorKind; + message: string; +} + +/** + * Narrow a `statFile` rejection to its structured kind. The Tauri command + * rejects with the serialized `FileStatError`; anything else (IPC failures, + * mocked rejections) counts as "other". + */ +export function fileStatErrorKind(error: unknown): FileStatErrorKind { + if ( + typeof error === "object" && + error !== null && + "kind" in error && + (error as { kind: unknown }).kind === "missing" + ) { + return "missing"; + } + return "other"; +} + export async function statFile(path: string): Promise { return invoke("stat_file", { path }); } diff --git a/src/shared/i18n/locales/en/chat.json b/src/shared/i18n/locales/en/chat.json index 9137a1df4..aa5775f70 100644 --- a/src/shared/i18n/locales/en/chat.json +++ b/src/shared/i18n/locales/en/chat.json @@ -689,7 +689,8 @@ "viewCode": "Code", "loading": "Loading file…", "loadError": "Couldn't load this file.", - "diskDiverged": "This preview may be out of date because the file changed or is unavailable on disk.", + "fileDeleted": "File deleted from disk.", + "fileUnreadable": "File changed but can't be read.", "reload": "Reload", "imageAlt": "Preview of {{filename}}" }, diff --git a/src/shared/i18n/locales/es/chat.json b/src/shared/i18n/locales/es/chat.json index 7cc6183f0..da05f2ef1 100644 --- a/src/shared/i18n/locales/es/chat.json +++ b/src/shared/i18n/locales/es/chat.json @@ -685,7 +685,8 @@ "viewCode": "Código", "loading": "Cargando archivo…", "loadError": "No se pudo cargar este archivo.", - "diskDiverged": "Esta vista previa puede estar desactualizada porque el archivo cambió o no está disponible en el disco.", + "fileDeleted": "Archivo eliminado del disco.", + "fileUnreadable": "El archivo cambió pero no se puede leer.", "reload": "Volver a cargar", "imageAlt": "Vista previa de {{filename}}" }, From d97bde3b0ca2867e44d7a5b37c8bfda9f329c726 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Wed, 26 Aug 2026 12:59:39 +1000 Subject: [PATCH 6/6] fix(chat): bound artifact freshness checks with a presentation timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** fix **User Impact:** A hung filesystem operation (stalled network mount, yanked removable media) can no longer wedge the artifact viewer into a permanent spinner or silently kill freshness polling. After ten seconds the hang surfaces through the same error state and stale-warning strip as any other read failure, and polling keeps running so the view heals on its own once the filesystem recovers. **Problem:** Every stat/read/image-decode await in the freshness machinery was unbounded. The Tauri filesystem commands run under spawn_blocking precisely because these syscalls can hang indefinitely, and a never-settling invoke had three failure modes: an initial load spun forever with no error; a hung forced refresh left `forcedRefreshInFlightRef` stuck true, suppressing every future poll; and a hung poll never cleared `checkInFlight` or reached `scheduleNextPoll`, silently ending polling. No warning, no recovery. **Solution:** A `withPresentationTimeout()` helper bounds each stat/read/decode step at `PRESENTATION_TIMEOUT_MS` (10s — comfortably beyond any healthy local operation), rejecting with a distinguishable `PresentationTimeoutError`. The rejection flows into the existing catch/finally blocks, so timeouts get exactly the established failure semantics: initial loads show the error state and flag immediately, polls against last-good content consume divergence strikes, and user-initiated reloads flag without grace. Crucially the finally blocks now run on timeout, clearing `forcedRefreshInFlightRef`/`checkInFlight` and rescheduling the next poll. A late settlement of the timed-out promise only re-settles the already-rejected wrapper — a spec-level no-op — so stale bytes can never overwrite newer state; the existing generation guards remain as an independent second line of defense. ## Verification - `just check` - `just test` (6,904 passed, 1 skipped)
File changes **src/features/chat/ui/ArtifactViewer.tsx** Add `PRESENTATION_TIMEOUT_MS`, `PresentationTimeoutError`, and the `withPresentationTimeout()` wrapper; apply it to every stat, text read, and image preload in both the forced-refresh effect and the poll effect. **src/features/chat/ui/__tests__/ArtifactViewer.test.tsx** Cover a hung initial load timing out into the error state with polling proceeding, hung reads consuming the two-strike grace period, polling continuing after a timed-out cycle, and a late settlement of a timed-out read staying inert after newer content lands.
Co-authored-by: goose --- src/features/chat/ui/ArtifactViewer.tsx | 81 +++++++-- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 155 ++++++++++++++++++ 2 files changed, 226 insertions(+), 10 deletions(-) diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 676e714ae..5f4012bf9 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -72,6 +72,51 @@ const BACKGROUND_ARTIFACT_POLL_INTERVAL_MS = 10_000; // single failure is routinely a file mid-rewrite or a transient I/O hiccup; // two in a row is a real divergence worth surfacing. const DIVERGENCE_STRIKE_THRESHOLD = 2; +// Upper bound on any single stat/read/decode step of a freshness cycle. These +// awaits cross IPC into filesystem calls that can hang indefinitely (stalled +// network mounts, yanked removable media — the Rust side uses spawn_blocking +// for exactly this reason), and an unbounded await here wedges the viewer: a +// stuck forced refresh suppresses all polling via forcedRefreshInFlightRef, +// and a stuck poll never reaches scheduleNextPoll. Ten seconds is comfortably +// beyond any healthy local operation while still funneling a genuine hang +// into the existing failure paths (error state, divergence strikes) before +// the viewer reads as dead. +const PRESENTATION_TIMEOUT_MS = 10_000; + +class PresentationTimeoutError extends Error { + constructor() { + super("Artifact presentation timed out"); + this.name = "PresentationTimeoutError"; + } +} + +// Bounds one stat/read/decode step. On timeout the wrapper rejects with +// PresentationTimeoutError so the caller's catch/finally blocks run — clearing +// the in-flight flags and letting polling continue. A later settlement of the +// underlying promise only re-settles the already-rejected wrapper, which is a +// no-op per the Promise spec: the awaiting effect code never resumes, so a +// timed-out operation cannot deliver stale bytes over newer state. (The +// generation guards remain in place as an independent second line of +// defense.) Attaching the rejection handler here also keeps a late failure of +// the underlying promise from surfacing as an unhandled rejection. +function withPresentationTimeout(promise: Promise): Promise { + return new Promise((resolve, reject) => { + const timerId = window.setTimeout( + () => reject(new PresentationTimeoutError()), + PRESENTATION_TIMEOUT_MS, + ); + promise.then( + (value) => { + window.clearTimeout(timerId); + resolve(value); + }, + (error: unknown) => { + window.clearTimeout(timerId); + reject(error); + }, + ); + }); +} function sameFingerprint( left: FileFingerprint, @@ -219,7 +264,9 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { void contentReadRevision; void (async () => { try { - const before = await statFile(artifact.resolvedPath); + const before = await withPresentationTimeout( + statFile(artifact.resolvedPath), + ); if (!isCurrentRefresh()) return; if (viewMode === "image") { @@ -233,9 +280,11 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { ); let confirmedFingerprint = before; if (shouldBustImageCache) { - await preloadArtifactImage(candidateSrc); + await withPresentationTimeout(preloadArtifactImage(candidateSrc)); if (!isCurrentRefresh()) return; - confirmedFingerprint = await statFile(artifact.resolvedPath); + confirmedFingerprint = await withPresentationTimeout( + statFile(artifact.resolvedPath), + ); if (!isCurrentRefresh()) return; if (!sameFingerprint(before, confirmedFingerprint)) { // Torn write: the file changed while the image was decoding. @@ -259,8 +308,12 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { return; } - const payload = await readTextFile(artifact.resolvedPath); - const after = await statFile(artifact.resolvedPath); + const payload = await withPresentationTimeout( + readTextFile(artifact.resolvedPath), + ); + const after = await withPresentationTimeout( + statFile(artifact.resolvedPath), + ); if (!isCurrentRefresh()) return; if (!sameFingerprint(before, after)) { // Torn write: the file changed underneath the read, so neither the @@ -349,7 +402,9 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { !forcedRefreshInFlightRef.current; checkInFlight = true; try { - const fingerprint = await statFile(artifact.resolvedPath); + const fingerprint = await withPresentationTimeout( + statFile(artifact.resolvedPath), + ); if (!isCurrentPoll()) return; const previous = fingerprintRef.current; if ( @@ -369,9 +424,11 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { artifact.resolvedPath, artifact.revision + candidateDiskRevision, ); - await preloadArtifactImage(candidateSrc); + await withPresentationTimeout(preloadArtifactImage(candidateSrc)); if (!isCurrentPoll()) return; - const confirmedFingerprint = await statFile(artifact.resolvedPath); + const confirmedFingerprint = await withPresentationTimeout( + statFile(artifact.resolvedPath), + ); if (!isCurrentPoll()) return; if (!sameFingerprint(fingerprint, confirmedFingerprint)) { // Torn write: the file changed while the image was decoding, so @@ -388,9 +445,13 @@ export function ArtifactViewer({ artifact, onClose }: ArtifactViewerProps) { return; } - const payload = await readTextFile(artifact.resolvedPath); + const payload = await withPresentationTimeout( + readTextFile(artifact.resolvedPath), + ); if (!isCurrentPoll()) return; - const confirmedFingerprint = await statFile(artifact.resolvedPath); + const confirmedFingerprint = await withPresentationTimeout( + statFile(artifact.resolvedPath), + ); if (!isCurrentPoll()) return; if (!sameFingerprint(fingerprint, confirmedFingerprint)) { // Torn write: the fingerprint moved during the read, so the fetched diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index 60e8854ff..fd1fb67bc 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -732,3 +732,158 @@ describe("ArtifactViewer divergence grace period", () => { expect(screen.queryByRole("status")).not.toBeInTheDocument(); }); }); + +describe("ArtifactViewer presentation timeout", () => { + // Mirrors PRESENTATION_TIMEOUT_MS in ArtifactViewer.tsx: the bound on any + // single stat/read/decode step before it is treated as a failure. + const PRESENTATION_TIMEOUT_MS = 10_000; + + beforeEach(() => { + vi.spyOn(document, "hasFocus").mockReturnValue(true); + vi.useFakeTimers(); + mockReadTextFile.mockReset(); + mockReadTextFile.mockResolvedValue({ contents: "# Loaded fine" }); + mockStatFile.mockReset(); + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + function contentBody() { + return screen + .getByRole("heading", { name: "Loaded fine" }) + .closest(".overflow-auto") as HTMLElement; + } + + async function renderLoadedViewer() { + render(); + await act(flushAsyncWork); + expect( + screen.getByRole("heading", { name: "Loaded fine" }), + ).toBeInTheDocument(); + } + + // One poll cycle where the read hangs: the poll timer fires, stat resolves, + // the read never settles, and the presentation timeout converts the hang + // into an ordinary cycle failure. + async function advanceHangingPollCycle() { + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + vi.advanceTimersByTime(PRESENTATION_TIMEOUT_MS); + await flushAsyncWork(); + }); + } + + it("times out a hung initial load into the error state and lets polling proceed", async () => { + const hungStat = deferred<{ byteSize: string; modifiedAtNs: string }>(); + mockStatFile + .mockReturnValueOnce(hungStat.promise) + .mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); + + render(); + await act(flushAsyncWork); + // Still hung: the loading spinner is up and nothing has been flagged yet. + // (Query by strip copy — the spinner itself carries role="status".) + expect(screen.queryByText(/can't be read/i)).not.toBeInTheDocument(); + expect(screen.getByLabelText(/loading file/i)).toBeInTheDocument(); + + await act(async () => { + vi.advanceTimersByTime(PRESENTATION_TIMEOUT_MS); + await flushAsyncWork(); + }); + // No last-good content exists, so the timeout shows the error state and + // flags immediately — same as any other initial-load failure. + expect(screen.getByText(/couldn't load/i)).toBeInTheDocument(); + expect(screen.getByRole("status")).toHaveTextContent(/can't be read/i); + + // The timed-out forced refresh must have cleared its in-flight flag: + // the next poll cycle runs, reads the now-healthy file, and heals. + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Loaded fine" }), + ).toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("gives hung reads the same two-strike grace period as failed reads", async () => { + await renderLoadedViewer(); + + // The file changed on disk, but every re-read hangs forever. + const hungRead = deferred<{ contents: string }>(); + mockStatFile.mockResolvedValue({ byteSize: "21", modifiedAtNs: "2" }); + mockReadTextFile.mockReturnValue(hungRead.promise); + + await advanceHangingPollCycle(); + // First timed-out cycle is inside the grace period: last-good content + // stays clean. + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + expect(contentBody().className).not.toMatch(/\bopacity-60\b/); + + await advanceHangingPollCycle(); + // Second consecutive timeout: warning strip plus dimmed last-good body. + expect(screen.getByRole("status")).toHaveTextContent(/can't be read/i); + expect(contentBody().className).toMatch(/\bopacity-60\b/); + }); + + it("keeps polling after a timed-out cycle instead of wedging", async () => { + await renderLoadedViewer(); + + // One cycle hangs; the file itself has settled at a new version. + const hungRead = deferred<{ contents: string }>(); + mockStatFile.mockResolvedValue({ byteSize: "21", modifiedAtNs: "2" }); + mockReadTextFile + .mockReturnValueOnce(hungRead.promise) + .mockResolvedValue({ contents: "# Fresh copy" }); + + await advanceHangingPollCycle(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + + // checkInFlight cleared and scheduleNextPoll ran: the very next cycle + // reads the settled file and swaps the fresh contents in. + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Fresh copy" }), + ).toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("ignores a late settlement of a timed-out read once newer content landed", async () => { + await renderLoadedViewer(); + + const hungRead = deferred<{ contents: string }>(); + mockStatFile.mockResolvedValue({ byteSize: "21", modifiedAtNs: "2" }); + mockReadTextFile + .mockReturnValueOnce(hungRead.promise) + .mockResolvedValue({ contents: "# Newer copy" }); + + await advanceHangingPollCycle(); + await act(async () => { + vi.advanceTimersByTime(1_500); + await flushAsyncWork(); + }); + expect( + screen.getByRole("heading", { name: "Newer copy" }), + ).toBeInTheDocument(); + + // The hung read finally settles, long after its cycle was abandoned. The + // timed-out wrapper already rejected, so these stale bytes must be inert. + hungRead.resolve({ contents: "# Stale bytes" }); + await act(flushAsyncWork); + + expect( + screen.getByRole("heading", { name: "Newer copy" }), + ).toBeInTheDocument(); + expect(screen.queryByText("Stale bytes")).not.toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); +});