From 3056eb29e070de406e06cecec6b8e9ccac0c561a Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 24 Aug 2026 16:34:08 +1000 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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;