From e356741435946ac3c10b88db5346f2e8883d8da0 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Tue, 4 Aug 2026 16:19:37 +0100 Subject: [PATCH] fix: stop the file-viewer reload loop on watched files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The watch callback forwarded every FS event, including the pure reads the viewer's own GET /api/file produces (IN_ACCESS / IN_CLOSE_NOWRITE on Linux): each silent reload re-triggered the watcher, looping at ~1 Hz. For PDFs every iteration minted a new blob URL and re-assigned iframe.src, which re-runs Chrome's whole PDF viewer (the flicker) and pushes a joint session-history entry (the back button buried under hundreds of blob: entries). - file_watch: forward an event only when the content version (mtime_ns, len) actually moved; drop Access events outright, stat-compare the rest. - viewer: render pdf/latex/svg previews in a keyed() iframe — a fresh element's first navigation replaces its history slot instead of pushing. --- src/frontend/api/file_watch.rs | 55 +++++++++++++++++++---- web/components/shared/file-viewer-base.js | 14 ++++-- web/index.html | 1 + web/mobile.html | 1 + 4 files changed, 59 insertions(+), 12 deletions(-) diff --git a/src/frontend/api/file_watch.rs b/src/frontend/api/file_watch.rs index 60b74ad..dbf8b9e 100644 --- a/src/frontend/api/file_watch.rs +++ b/src/frontend/api/file_watch.rs @@ -29,6 +29,13 @@ //! sharing). On disconnect every watcher is dropped and the OS resources are //! released automatically. //! +//! ## What counts as a change +//! +//! Only events that move the file's content version (mtime + len) are +//! forwarded; pure reads (`Access`) and metadata-only touches are dropped in +//! the watcher callback. Without that filter the viewer's own `GET /api/file` +//! read would re-trigger the watcher on every reload, looping forever. +//! //! ## LaTeX dependency-aware watching //! //! When subscribing to a `.tex` / `.latex` source, the server expands the @@ -59,7 +66,7 @@ use axum::{ response::IntoResponse, }; use core_api::user_fs::SharedFs; -use notify::{Config, RecommendedWatcher, RecursiveMode, Watcher}; +use notify::{Config, EventKind, RecommendedWatcher, RecursiveMode, Watcher}; use serde::Deserialize; use serde_json::json; use tokio::sync::mpsc; @@ -216,16 +223,34 @@ fn install_watcher( for path in paths_to_watch { let tx_for_cb = change_tx.clone(); let original_path = user_path.to_string(); + let path_for_cb = path.clone(); + // Content-version baseline for the stat dedup in the callback. + let mut last_stamp = content_stamp(&path); let mut watcher = RecommendedWatcher::new( move |res: notify::Result| { - // Any event on the watched path triggers a change notification. - // We don't inspect the event kind — reload on the client side - // re-reads the file and naturally handles create/modify/remove. - if res.is_ok() { - if tx_for_cb.send(original_path.clone()).is_err() { - // channel closed — receiver dropped (WS disconnected). - } + let Ok(event) = res else { return }; + // A pure read is never a change — and it matters doubly here, + // because the viewer's own `GET /api/file` produces exactly + // these events (IN_ACCESS / IN_CLOSE_NOWRITE on Linux): + // forwarding them made every client reload re-trigger the + // watcher, a self-sustaining reload loop. Real writes still + // surface as Modify events, so dropping Access loses nothing. + if matches!(event.kind, EventKind::Access(_)) { + return; + } + // Every other kind is answered with the one question that + // matters — did (mtime, len) actually move? This swallows + // metadata-only noise (atime, chmod) whatever kind the backend + // reports it as, without trusting per-platform kind mappings. + // A failed stat means the file is gone, and that IS a change. + let stamp = content_stamp(&path_for_cb); + if stamp == last_stamp { + return; + } + last_stamp = stamp; + if tx_for_cb.send(original_path.clone()).is_err() { + // channel closed — receiver dropped (WS disconnected). } }, Config::default(), @@ -248,6 +273,20 @@ fn install_watcher( Ok(()) } +/// Content version of a file as (mtime_ns, len) — the same inputs as the HTTP +/// `ETag` (`disk_etag` in `api/files.rs`). A pure read never moves it, any +/// write does. `None` when the file is missing or unreadable. +fn content_stamp(path: &Path) -> Option<(u128, u64)> { + let md = std::fs::metadata(path).ok()?; + let mtime_ns = md + .modified() + .ok()? + .duration_since(std::time::UNIX_EPOCH) + .ok()? + .as_nanos(); + Some((mtime_ns, md.len())) +} + /// True for `.tex` / `.latex` extensions — sources that trigger the /// dependency-aware watcher expansion. fn is_latex_path(path: &str) -> bool { diff --git a/web/components/shared/file-viewer-base.js b/web/components/shared/file-viewer-base.js index 5b452a0..b47d177 100644 --- a/web/components/shared/file-viewer-base.js +++ b/web/components/shared/file-viewer-base.js @@ -1,5 +1,6 @@ import { html, nothing } from 'lit'; import { unsafeHTML } from 'lit/directives/unsafe-html.js'; +import { keyed } from 'lit/directives/keyed.js'; import { LightElement, renderMarkdown } from '../../lib/base.js'; import { fileWatcher } from '../../lib/file-watcher.js'; import { t } from '../../lib/i18n.js'; @@ -553,12 +554,17 @@ export class FileViewerBase extends LightElement { return html`
${this._path}
`; } if (this._kind === 'pdf' && this._blobUrl) { - return html``; + // `keyed` re-creates the iframe element on every new blob URL: the first + // navigation of a fresh iframe replaces its history slot instead of + // pushing one — whereas re-assigning `src` on an existing iframe pushes + // a joint session-history entry each time (during the watch-reload loop + // that buried the back button under hundreds of blob: entries). + return keyed(this._blobUrl, html``); } if (this._kind === 'latex' && this._blobUrl) { // Successfully compiled server-side — render the resulting PDF the same - // way a native .pdf would be rendered. - return html``; + // way a native .pdf would be rendered (see the keyed() note above). + return keyed(this._blobUrl, html``); } if (this._kind === 'svg' && this._blobUrl) { // `allow-same-origin` (and nothing else) is required so the iframe can load @@ -566,7 +572,7 @@ export class FileViewerBase extends LightElement { // `allow-scripts` absent, any