From bf2291089ab4dae363a166c436dfe41d59866fde Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 22 Sep 2026 21:52:43 -0700 Subject: [PATCH] =?UTF-8?q?fix(viewer):=20final-review=20fixes=20=E2=80=94?= =?UTF-8?q?=20save=20base=20from=20written=20bytes,=20honest=20poll=20erro?= =?UTF-8?q?rs,=20retryable=20first=20read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - write.rs: a save's new base is sha256 of the bytes written; the script's post-mv hash comes back as disk_hash, and a mismatch (another writer landed after us) shows "Changed on disk" instead of being adopted (ledger M2). - write.rs: conflict:/gone:/read-only strings are constants with a pure saved_file() mapping and tests; app/src/viewer/ipcMessages.ts is the one TS copy and a cargo test checks it against the Rust originals. - write.rs: the comment now says the in-place `cat >` fallback follows a planted symlink, and why that is accepted (runs as claude). - poll.rs: a file deleted between `test -f` and `sha256sum` reads as gone. - viewerState/EditorPane: poll_failed carries its message; only the "Start the project before" refusal reads as Container not running, anything else gets its own banner and leaves Save enabled. - EditorPane: a failed first read shows Retry and is retried by the poll. - spec §1: refused OSC 8 targets keep the refusal card (Task 9 ruling). Co-Authored-By: Claude Opus 5.5 (1M context) --- app/src-tauri/src/commands/file_commands.rs | 13 +- .../src/commands/file_viewer_commands.rs | 35 +++- app/src-tauri/src/file_viewer/poll.rs | 93 +++++++++- app/src-tauri/src/file_viewer/write.rs | 174 ++++++++++++++++-- app/src/lib/tauri-commands.ts | 4 +- app/src/lib/types.ts | 8 + app/src/viewer/EditorPane.test.tsx | 92 ++++++++- app/src/viewer/EditorPane.tsx | 86 ++++++--- app/src/viewer/ipcMessages.ts | 21 +++ app/src/viewer/viewerState.test.ts | 37 +++- app/src/viewer/viewerState.ts | 37 +++- .../2026-09-22-terminal-file-viewer-design.md | 8 +- 12 files changed, 535 insertions(+), 73 deletions(-) create mode 100644 app/src/viewer/ipcMessages.ts diff --git a/app/src-tauri/src/commands/file_commands.rs b/app/src-tauri/src/commands/file_commands.rs index 6d1ab0c..53e4480 100644 --- a/app/src-tauri/src/commands/file_commands.rs +++ b/app/src-tauri/src/commands/file_commands.rs @@ -1448,6 +1448,14 @@ pub async fn create_container_directory( Ok(dest) } +/// Every "container is not running" refusal starts with this, so a caller (the file +/// viewer's poll, `app/src/viewer/ipcMessages.ts`) can tell it apart from any other failure. +pub(crate) const NOT_RUNNING_PREFIX: &str = "Start the project before"; + +pub(crate) fn not_running_message(action: &str, why: &str) -> String { + format!("{} {} — {}.", NOT_RUNNING_PREFIX, action, why) +} + /// Refuse, in a sentence, before a Docker error has to speak for us. /// /// Both file transfers and the backup run through `docker exec`, which needs a @@ -1468,10 +1476,7 @@ pub(crate) async fn require_running(container_id: &str, action: &str) -> Result< if running { return Ok(()); } - Err(format!( - "Start the project before {} — it runs inside the running container.", - action - )) + Err(not_running_message(action, "it runs inside the running container")) } /// Copy one regular file out of a container onto a host path the user chose in diff --git a/app/src-tauri/src/commands/file_viewer_commands.rs b/app/src-tauri/src/commands/file_viewer_commands.rs index f7ab869..2af9605 100644 --- a/app/src-tauri/src/commands/file_viewer_commands.rs +++ b/app/src-tauri/src/commands/file_viewer_commands.rs @@ -8,7 +8,7 @@ use serde::Serialize; use tauri::{AppHandle, Emitter, Manager, State}; use crate::commands::file_commands::{ - fetch_container_file, require_running, validate_container_write_path, MAX_READ_BYTES, + fetch_container_file, not_running_message, require_running, validate_container_write_path, MAX_READ_BYTES, }; use crate::file_viewer::is_viewer_label; use crate::file_viewer::poll::{poll_file, ViewerPoll}; @@ -17,7 +17,7 @@ use crate::file_viewer::registry::{ }; use crate::file_viewer::resolve::{candidate_paths, probe_candidates}; use crate::file_viewer::window::open_viewer_window; -use crate::file_viewer::write::{sha256_hex, write_file, MAX_WRITE_BYTES}; +use crate::file_viewer::write::{sha256_hex, write_file, SavedFile, MAX_WRITE_BYTES}; use crate::models::Project; use crate::AppState; @@ -114,9 +114,10 @@ fn project_of(state: &AppState, project_id: &str) -> Result { /// `action` completes "Start the project before …", e.g. "saving this file". async fn running_container_of(project: &Project, action: &str) -> Result { - let container_id = project.container_id.clone().ok_or_else(|| { - format!("Start the project before {} — files live in its container.", action) - })?; + let container_id = project + .container_id + .clone() + .ok_or_else(|| not_running_message(action, "files live in its container"))?; require_running(&container_id, action).await?; Ok(container_id) } @@ -273,8 +274,10 @@ pub async fn viewer_poll_file( } /// Errors from `write_file` pass through unchanged: the frontend matches the -/// `"conflict: "` and `"gone: "` prefixes, and anything else (a read-only file, -/// a full disk) is already a sentence it shows as is. +/// `write::CONFLICT_PREFIX`/`GONE_PREFIX` prefixes and `READ_ONLY_MESSAGE` (TS copies in +/// `app/src/viewer/ipcMessages.ts`), and anything else (a full disk) is already a +/// sentence it shows as is. Success is a `SavedFile`: the new base hash and the hash +/// the disk held right after the swap. #[tauri::command] pub async fn viewer_write_file( contents_base64: String, @@ -282,7 +285,7 @@ pub async fn viewer_write_file( window: tauri::Window, registry: State<'_, ViewerRegistry>, state: State<'_, AppState>, -) -> Result { +) -> Result { let (_label, target) = own_target(&window, ®istry)?; let path = resolved_path(&target)?; validate_container_write_path("File", &path)?; @@ -308,6 +311,22 @@ mod tests { assert!(require_viewer("file-viewer-").is_err()); } + /// Both "no container" refusals a viewer command can give start with the prefix + /// the viewer reads as "Container not running" (`ipcMessages.ts`). + #[test] + fn not_running_refusals_carry_the_shared_prefix() { + use crate::commands::file_commands::NOT_RUNNING_PREFIX; + let m = not_running_message("checking this file for changes", "files live in its container"); + assert_eq!(m, "Start the project before checking this file for changes — files live in its container."); + assert!(m.starts_with(NOT_RUNNING_PREFIX)); + } + + #[test] + fn a_saved_file_serialises_both_hashes() { + let json = serde_json::to_value(SavedFile { hash: "a".into(), disk_hash: "b".into() }).unwrap(); + assert_eq!(json, serde_json::json!({ "hash": "a", "disk_hash": "b" })); + } + #[test] fn the_title_is_basename_then_project() { assert_eq!(window_title("app/src/lib/urlRelay.ts", "Triple-C"), "urlRelay.ts — Triple-C"); diff --git a/app/src-tauri/src/file_viewer/poll.rs b/app/src-tauri/src/file_viewer/poll.rs index 6621d55..d7301cc 100644 --- a/app/src-tauri/src/file_viewer/poll.rs +++ b/app/src-tauri/src/file_viewer/poll.rs @@ -16,8 +16,12 @@ pub struct ViewerPoll { pub size: Option, } -pub const POLL_SCRIPT: &str = - r#"test -f "$1" || exit 4; sha256sum -- "$1" && stat -c %s -- "$1""#; +/// Exit 4 = gone. A failure after `test -f` passed is re-checked: if the file vanished +/// in between (deleted while being hashed), that is "gone", not an error (M6). +pub const POLL_SCRIPT: &str = r#"test -f "$1" || exit 4 +sha256sum -- "$1" && stat -c %s -- "$1" && exit 0 +test -f "$1" || exit 4 +exit 1"#; pub fn parse_poll_output(code: i64, stdout: &str) -> ViewerPoll { if code == 4 { @@ -90,6 +94,91 @@ mod tests { assert!(POLL_SCRIPT.contains("stat -c %s -- \"$1\"")); } + #[cfg(unix)] + fn run_poll_script(path_env: Option<&str>, target: &std::path::Path) -> (i64, String, String) { + let mut cmd = std::process::Command::new("sh"); + if let Some(p) = path_env { + cmd.env("PATH", p); + } + let out = cmd.arg("-c").arg(POLL_SCRIPT).arg("poll").arg(target).output().unwrap(); + ( + out.status.code().unwrap_or(-1) as i64, + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) + } + + #[cfg(unix)] + fn test_dir(name: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("tc-poll-{}-{}", name, uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[cfg(unix)] + #[test] + fn on_the_host_the_poll_script_reports_hash_size_and_gone() { + let dir = test_dir("plain"); + let target = dir.join("t.txt"); + std::fs::write(&target, b"hello\n").unwrap(); + let (code, stdout, stderr) = run_poll_script(None, &target); + assert_eq!(code, 0, "stderr={stderr}"); + let p = parse_poll_output(code, &stdout); + assert_eq!(p.hash.as_deref(), Some(super::super::write::sha256_hex(b"hello\n").as_str())); + assert_eq!(p.size, Some(6)); + + let (code, _, _) = run_poll_script(None, &dir.join("missing")); + assert_eq!(code, 4); + let _ = std::fs::remove_dir_all(&dir); + } + + /// M6: the file is deleted after `test -f` passed but before `sha256sum` read it + /// (a `sha256sum` shim on PATH deletes it and fails). That is "gone", not an error + /// the viewer would have to explain. + #[cfg(unix)] + #[test] + fn on_the_host_a_file_deleted_mid_poll_reads_as_gone() { + use std::os::unix::fs::PermissionsExt; + let dir = test_dir("race"); + let bin = dir.join("bin"); + std::fs::create_dir_all(&bin).unwrap(); + let shim = bin.join("sha256sum"); + std::fs::write(&shim, "#!/bin/sh\nrm -f -- \"$2\"\necho 'sha256sum: No such file or directory' >&2\nexit 1\n").unwrap(); + std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o755)).unwrap(); + let target = dir.join("t.txt"); + std::fs::write(&target, b"x").unwrap(); + let path = format!("{}:{}", bin.display(), std::env::var("PATH").unwrap_or_default()); + + let (code, stdout, stderr) = run_poll_script(Some(&path), &target); + + assert_eq!(code, 4, "stderr={stderr}"); + assert_eq!(parse_poll_output(code, &stdout), ViewerPoll { exists: false, hash: None, size: None }); + let _ = std::fs::remove_dir_all(&dir); + } + + /// A failure with the file still present stays a real error (exit 1), which + /// `poll_file` turns into "Could not check the file: …". + #[cfg(unix)] + #[test] + fn on_the_host_a_hash_failure_on_a_present_file_is_an_error() { + use std::os::unix::fs::PermissionsExt; + let dir = test_dir("fail"); + let bin = dir.join("bin"); + std::fs::create_dir_all(&bin).unwrap(); + let shim = bin.join("sha256sum"); + std::fs::write(&shim, "#!/bin/sh\necho 'sha256sum: Permission denied' >&2\nexit 1\n").unwrap(); + std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o755)).unwrap(); + let target = dir.join("t.txt"); + std::fs::write(&target, b"x").unwrap(); + let path = format!("{}:{}", bin.display(), std::env::var("PATH").unwrap_or_default()); + + let (code, _stdout, stderr) = run_poll_script(Some(&path), &target); + + assert_eq!(code, 1, "stderr={stderr}"); + assert!(stderr.contains("Permission denied")); + let _ = std::fs::remove_dir_all(&dir); + } + /// P15: a path containing a backslash makes GNU `sha256sum` prefix the whole /// line with `\`; that must not blind change detection by yielding `hash: None`. #[test] diff --git a/app/src-tauri/src/file_viewer/write.rs b/app/src-tauri/src/file_viewer/write.rs index 334e0cd..e256adb 100644 --- a/app/src-tauri/src/file_viewer/write.rs +++ b/app/src-tauri/src/file_viewer/write.rs @@ -7,6 +7,7 @@ //! A non-root process cannot `chown`, so the saved file is owned by the container user, //! as it would be after Claude Code edited it; mode is kept with `chmod --reference`. +use serde::Serialize; use sha2::{Digest, Sha256}; use crate::commands::file_commands::clip_container_text; @@ -27,7 +28,10 @@ pub fn is_sha256_hex(s: &str) -> bool { /// `$1` target, `$2` staged payload in /tmp, `$3` the hash the editor loaded from. /// Exit 1 = a step failed (unreadable target, a failed stage/replace, …), 3 = changed /// on disk, 4 = gone, 5 = the target is not writable by the container user; stdout on -/// success is `sha256sum` of the target. +/// success is `sha256sum` of the target *after* the write. That is not necessarily the +/// hash of what we wrote: another writer (Claude Code, on the same file) can land +/// between `mv` and `sha256sum`. `saved_file` therefore takes the save's base from the +/// bytes and only reports this one as what the disk held afterwards (M2). /// /// P15: `sha256sum -- "$target"` prefixes its whole line with `\` when the path /// contains a backslash or a newline, so `$actual` has that prefix stripped before @@ -59,6 +63,12 @@ actual=${actual%% *}; actual=${actual#\\} # would refuse. This is stricter than spec §5 step 3's literal "if the directory is # writable" branch, which never looks at the file's own permissions; the branch below # only ever chooses *how* to write, never *whether*. +# +# The rename branch replaces whatever is at "$target" (a symlink planted there after +# the window opened is replaced, not followed). The in-place `cat >` fallback, taken +# only for a writable file in a read-only directory, DOES follow such a symlink and +# writes through it. That is accepted: the write runs as `claude`, so it can reach +# nothing Claude Code in the same container cannot already write. [ -w "$target" ] || { echo "The file is read-only for the container user." >&2; exit 5; } dir=$(dirname -- "$target"); name=$(basename -- "$target") if [ -w "$dir" ]; then @@ -71,6 +81,16 @@ else fi sha256sum -- "$target""#; +/// A save refused because the file changed since its base hash. The frontend matches +/// this prefix; its copy lives in `app/src/viewer/ipcMessages.ts` (pinned by a test). +pub const CONFLICT_PREFIX: &str = "conflict:"; +/// A save refused because the file no longer exists; mirrored in `ipcMessages.ts`. +pub const GONE_PREFIX: &str = "gone:"; +/// The read-only refusal. The script echoes the same sentence (pinned by a test), but +/// the caller always gets this constant, whatever the script printed; mirrored in +/// `ipcMessages.ts`. +pub const READ_ONLY_MESSAGE: &str = "The file is read-only for the container user."; + /// I3: distinct from the generic failure code so the caller can hand back a specific, /// readable message instead of whatever the script's own diagnostic text says. const EXIT_READ_ONLY: i64 = 5; @@ -86,9 +106,7 @@ pub fn classify_write(code: i64, stdout: &str, stderr: &str) -> WriteOutcome { match code { 3 => WriteOutcome::Conflict, 4 => WriteOutcome::Gone, - EXIT_READ_ONLY => { - WriteOutcome::Failed("The file is read-only for the container user.".into()) - } + EXIT_READ_ONLY => WriteOutcome::Failed(READ_ONLY_MESSAGE.into()), 0 => match stdout .split_whitespace() .next() @@ -130,13 +148,36 @@ fn check_write_input(len: usize, base_hash: &str) -> Result<(), String> { Ok(()) } +/// What a successful save reports: `hash` is the new base, `sha256_hex` of the bytes +/// we wrote; `disk_hash` is what the container hashed right after the swap. They differ +/// only when another writer landed in between, and then the editor must show "Changed +/// on disk" rather than adopt the other writer's hash as its base (M2). +#[derive(Clone, Debug, Serialize, PartialEq, Eq)] +pub struct SavedFile { + pub hash: String, + pub disk_hash: String, +} + +/// `viewer_write_file`'s result, pure so the error-prefix contract has a unit test. +fn saved_file(outcome: WriteOutcome, bytes: &[u8]) -> Result { + match outcome { + WriteOutcome::Saved(disk_hash) => Ok(SavedFile { hash: sha256_hex(bytes), disk_hash }), + WriteOutcome::Conflict => Err(format!( + "{} the file changed on disk since it was loaded.", + CONFLICT_PREFIX + )), + WriteOutcome::Gone => Err(format!("{} the file no longer exists.", GONE_PREFIX)), + WriteOutcome::Failed(msg) => Err(format!("Could not save the file: {}", msg)), + } +} + pub async fn write_file( container_id: &str, exec_manager: &ExecSessionManager, target: &str, bytes: &[u8], base_hash: &str, -) -> Result { +) -> Result { check_write_input(bytes.len(), base_hash)?; let tmp_name = format!("triple-c-viewer-{}", uuid::Uuid::new_v4().simple()); let tmp_path = exec_manager @@ -145,14 +186,7 @@ pub async fn write_file( let cmd = write_command(target, &tmp_path, base_hash); let (stdout, stderr, code) = exec_oneshot_streams_as(container_id, "claude", cmd, Vec::new()).await?; - match classify_write(code, &stdout, &stderr) { - WriteOutcome::Saved(hash) => Ok(hash), - WriteOutcome::Conflict => { - Err("conflict: the file changed on disk since it was loaded.".into()) - } - WriteOutcome::Gone => Err("gone: the file no longer exists.".into()), - WriteOutcome::Failed(msg) => Err(format!("Could not save the file: {}", msg)), - } + saved_file(classify_write(code, &stdout, &stderr), bytes) } #[cfg(test)] @@ -193,6 +227,60 @@ mod tests { )); } + /// M2: the new base is the hash of the bytes we wrote, never the script's + /// post-`mv` hash, which may belong to a writer that landed after us. + #[test] + fn a_save_takes_its_base_from_the_written_bytes() { + let ours = sha256_hex(b"new\n"); + let same = saved_file(WriteOutcome::Saved(ours.clone()), b"new\n").unwrap(); + assert_eq!(same, SavedFile { hash: ours.clone(), disk_hash: ours.clone() }); + + let foreign = sha256_hex(b"someone else's\n"); + let raced = saved_file(WriteOutcome::Saved(foreign.clone()), b"new\n").unwrap(); + assert_eq!(raced.hash, ours, "the base must be what we wrote"); + assert_eq!(raced.disk_hash, foreign, "the foreign hash is reported, not adopted"); + } + + /// Important #4: the frontend matches these exact strings + /// (`app/src/viewer/ipcMessages.ts`), so pin them here too. + #[test] + fn save_errors_keep_the_prefix_contract() { + let conflict = saved_file(WriteOutcome::Conflict, b"").unwrap_err(); + assert!(conflict.starts_with("conflict:"), "{conflict}"); + assert_eq!(conflict, "conflict: the file changed on disk since it was loaded."); + + let gone = saved_file(WriteOutcome::Gone, b"").unwrap_err(); + assert!(gone.starts_with("gone:"), "{gone}"); + assert_eq!(gone, "gone: the file no longer exists."); + + let read_only = saved_file(classify_write(5, "", "whatever the script said"), b"").unwrap_err(); + assert_eq!(read_only, "Could not save the file: The file is read-only for the container user."); + assert!(!read_only.starts_with(CONFLICT_PREFIX) && !read_only.starts_with(GONE_PREFIX)); + + let other = saved_file(classify_write(1, "", "No space left on device"), b"").unwrap_err(); + assert_eq!(other, "Could not save the file: No space left on device"); + + // The script's own refusal text is the same sentence the caller is given. + assert!(WRITE_SCRIPT.contains(&format!("echo \"{}\" >&2; exit 5", READ_ONLY_MESSAGE))); + } + + /// The TypeScript side keeps one copy of each matched string; a change on either + /// side without the other fails here. + #[test] + fn the_frontend_copies_of_the_ipc_messages_match() { + let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../src/viewer/ipcMessages.ts"); + let ts = std::fs::read_to_string(&path).expect("app/src/viewer/ipcMessages.ts"); + for (name, value) in [ + ("CONFLICT_PREFIX", CONFLICT_PREFIX), + ("GONE_PREFIX", GONE_PREFIX), + ("READ_ONLY_MESSAGE", READ_ONLY_MESSAGE), + ("NOT_RUNNING_PREFIX", crate::commands::file_commands::NOT_RUNNING_PREFIX), + ] { + let line = format!("export const {} = \"{}\";", name, value); + assert!(ts.contains(&line), "ipcMessages.ts must contain `{line}`"); + } + } + /// P15: a target path with a backslash makes `sha256sum` prefix the line; /// the parsed hash must still be recognised as the saved hash. #[test] @@ -309,12 +397,72 @@ mod tests { assert_eq!(code, 0, "stdout={stdout} stderr={stderr}"); let new_hash = sha256_hex(b"new\n"); assert!(stdout.contains(&new_hash), "stdout={stdout}"); + // With no other writer, the reported disk hash is ours, so no conflict is shown. + let saved = saved_file(classify_write(code as i64, &stdout, &stderr), b"new\n").unwrap(); + assert_eq!(saved, SavedFile { hash: new_hash.clone(), disk_hash: new_hash.clone() }); assert_eq!(std::fs::read(&target).unwrap(), b"new\n"); assert!(!tmp.exists(), "the staged /tmp payload must be cleaned up"); let _ = std::fs::remove_dir_all(&dir); } + /// M2, for real: another writer lands between the script's `mv` and its final + /// `sha256sum` (simulated by a `sha256sum` shim on PATH that rewrites the target on + /// its second call). The save's base must still be the hash of our bytes, and the + /// foreign hash must come back as `disk_hash`, so the editor shows "Changed on disk". + #[cfg(unix)] + #[test] + fn on_the_host_a_write_that_lands_after_ours_is_reported_not_adopted() { + use std::os::unix::fs::PermissionsExt; + let real = std::process::Command::new("sh") + .args(["-c", "command -v sha256sum"]) + .output() + .expect("sh"); + let real = String::from_utf8_lossy(&real.stdout).trim().to_string(); + assert!(!real.is_empty(), "sha256sum must be on PATH"); + + let dir = unique_test_dir("race"); + let bin = dir.join("bin"); + std::fs::create_dir_all(&bin).unwrap(); + let mark = dir.join("called-once"); + let shim = bin.join("sha256sum"); + std::fs::write( + &shim, + format!( + "#!/bin/sh\nif [ -e '{mark}' ]; then printf 'theirs\\n' > \"$2\"; fi\n: > '{mark}'\nexec '{real}' \"$@\"\n", + mark = mark.display(), + real = real + ), + ) + .unwrap(); + std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let target = dir.join("t.txt"); + let tmp = dir.join("payload"); + std::fs::write(&target, b"old\n").unwrap(); + std::fs::write(&tmp, b"new\n").unwrap(); + let path = format!("{}:{}", bin.display(), std::env::var("PATH").unwrap_or_default()); + let out = std::process::Command::new("sh") + .env("PATH", path) + .arg("-c") + .arg(WRITE_SCRIPT) + .arg("save") + .arg(&target) + .arg(&tmp) + .arg(sha256_hex(b"old\n")) + .output() + .unwrap(); + let (stdout, stderr) = (String::from_utf8_lossy(&out.stdout), String::from_utf8_lossy(&out.stderr)); + assert_eq!(out.status.code(), Some(0), "stdout={stdout} stderr={stderr}"); + assert_eq!(std::fs::read(&target).unwrap(), b"theirs\n", "the shim's write landed last"); + + let saved = saved_file(classify_write(0, &stdout, &stderr), b"new\n").unwrap(); + assert_eq!(saved.hash, sha256_hex(b"new\n")); + assert_eq!(saved.disk_hash, sha256_hex(b"theirs\n")); + + let _ = std::fs::remove_dir_all(&dir); + } + #[cfg(unix)] #[test] fn on_the_host_a_stale_base_hash_conflicts_and_leaves_everything_untouched() { diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index 298e439..d286b7e 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -1,5 +1,5 @@ import { invoke } from "@tauri-apps/api/core"; -import type { Project, ProjectPath, ProjectRemovalReport, ProjectResetOutcome, ContainerInfo, AppSettings, SettingsImportPreview, SettingsImportOutcome, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome, Note, ViewerFile, ViewerPoll, ViewerState } from "./types"; +import type { Project, ProjectPath, ProjectRemovalReport, ProjectResetOutcome, ContainerInfo, AppSettings, SettingsImportPreview, SettingsImportOutcome, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome, Note, ViewerFile, ViewerPoll, ViewerSaved, ViewerState } from "./types"; // Docker export const checkDocker = () => invoke("check_docker"); @@ -429,6 +429,6 @@ export const viewerReadFile = (maxBytes: number) => invoke("viewer_read_file", { maxBytes }); export const viewerPollFile = () => invoke("viewer_poll_file"); export const viewerWriteFile = (contentsBase64: string, baseHash: string) => - invoke("viewer_write_file", { contentsBase64, baseHash }); + invoke("viewer_write_file", { contentsBase64, baseHash }); export const viewerChooseFile = (index: number) => invoke("viewer_choose_file", { index }); diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index f6fb9fa..54bf911 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -987,6 +987,14 @@ export interface ViewerFile { readonly_reason: string | null; } +/** A successful save (`write.rs`'s `SavedFile`). */ +export interface ViewerSaved { + /** SHA-256 of the bytes written: the editor's new base hash. */ + hash: string; + /** What the container hashed right after the swap; differs from `hash` only if another writer landed first. */ + disk_hash: string; +} + export interface ViewerPoll { exists: boolean; hash: string | null; diff --git a/app/src/viewer/EditorPane.test.tsx b/app/src/viewer/EditorPane.test.tsx index bc80659..ce39a1b 100644 --- a/app/src/viewer/EditorPane.test.tsx +++ b/app/src/viewer/EditorPane.test.tsx @@ -48,6 +48,8 @@ const typeInto = (from: number, to: number, insert: string) => { const bytesB64 = (bytes: number[]) => encodeBase64(new Uint8Array(bytes)); const utf8 = (s: string) => Array.from(new TextEncoder().encode(s)); const READ_ONLY = "Could not save the file: The file is read-only for the container user."; +const NOT_RUNNING = "Start the project before checking this file for changes — it runs inside the running container."; +const saved = (hash: string, diskHash = hash) => ({ hash, disk_hash: diskHash }); const poll = async (ms = 2100) => { await act(async () => { await vi.advanceTimersByTimeAsync(ms); }); }; describe("EditorPane", () => { @@ -65,7 +67,7 @@ describe("EditorPane", () => { Object.defineProperty(document, "visibilityState", { value: "visible", configurable: true }); commands.viewerReadFile.mockReset().mockResolvedValue(textFile("hello\n", H1)); commands.viewerPollFile.mockReset().mockResolvedValue({ exists: true, hash: H1, size: 6 }); - commands.viewerWriteFile.mockReset().mockResolvedValue(H2); + commands.viewerWriteFile.mockReset().mockResolvedValue(saved(H2)); windowApi.destroy.mockReset(); }); afterEach(() => vi.useRealTimers()); @@ -153,7 +155,7 @@ describe("EditorPane", () => { await clickSave(); const overwrite = await screen.findByRole("button", { name: /Overwrite on save/ }); await act(async () => { fireEvent.click(overwrite); }); - commands.viewerWriteFile.mockResolvedValue(H2); + commands.viewerWriteFile.mockResolvedValue(saved(H2)); await clickSave(); expect(commands.viewerWriteFile).toHaveBeenLastCalledWith(b64("hello\n"), H3); expect(await screen.findByText("Saved")).toBeInTheDocument(); @@ -194,16 +196,96 @@ describe("EditorPane", () => { expect(await screen.findByText("Could not save the file: disk full")).toBeInTheDocument(); }); - it("a failed poll shows Container not running and disables Save", async () => { + it("a poll refused because the container is down shows Container not running and disables Save", async () => { render(); await screen.findByText("/workspace/demo/notes.md"); edit(); - commands.viewerPollFile.mockRejectedValue("Container is not running."); + commands.viewerPollFile.mockRejectedValue(NOT_RUNNING); await poll(); expect(await screen.findByText(/until the project starts again/)).toBeInTheDocument(); + expect(screen.getByText("Container not running")).toBeInTheDocument(); expect(screen.getByRole("button", { name: /^save$/i })).toBeDisabled(); }); + it("any other poll failure says what failed, not that the container is down, and clears on a good poll", async () => { + render(); + await screen.findByText("/workspace/demo/notes.md"); + edit(); + commands.viewerPollFile.mockRejectedValue("Could not check the file: sha256sum: Permission denied"); + await poll(); + expect(await screen.findByRole("alert")).toHaveTextContent(/Could not check the file: sha256sum: Permission denied/); + expect(screen.getByText("Could not check for changes")).toBeInTheDocument(); + expect(screen.queryByText(/Container not running/)).toBeNull(); + // The write re-checks the hash itself, so saving stays possible. + expect(screen.getByRole("button", { name: /^save$/i })).toBeEnabled(); + commands.viewerPollFile.mockResolvedValue({ exists: true, hash: H1, size: 6 }); + await poll(2000); + expect(screen.queryByText(/Permission denied/)).toBeNull(); + expect(screen.getByText("Unsaved")).toBeInTheDocument(); + }); + + it("a save that another writer overtook shows Changed on disk instead of Saved", async () => { + render(); + await screen.findByText("Saved"); + edit(); + commands.viewerWriteFile.mockResolvedValue(saved(H2, H3)); + commands.viewerPollFile.mockResolvedValue({ exists: true, hash: H3, size: 7 }); + await clickSave(); + expect(await screen.findByText(/while you were editing/)).toBeInTheDocument(); + expect(screen.getByText("Changed on disk")).toBeInTheDocument(); + expect(screen.getByRole("button", { name: /^save$/i })).toBeDisabled(); + // The next poll sees the same foreign hash: the banner stays, nothing is reloaded over the buffer. + await poll(); + expect(screen.getByText(/while you were editing/)).toBeInTheDocument(); + expect(commands.viewerReadFile).toHaveBeenCalledTimes(1); + // Overwrite now saves against what is actually on disk. + await act(async () => { fireEvent.click(screen.getByRole("button", { name: /Overwrite on save/ })); }); + commands.viewerWriteFile.mockResolvedValue(saved(H2)); + await clickSave(); + expect(commands.viewerWriteFile).toHaveBeenLastCalledWith(b64("hello\n"), H3); + expect(await screen.findByText("Saved")).toBeInTheDocument(); + }); + + it("Save and close does not close when another writer overtook the save", async () => { + commands.viewerWriteFile.mockResolvedValue(saved(H2, H3)); + render(); + await screen.findByText("Saved"); + edit(); + await act(async () => { await windowApi.closeRequested?.({ preventDefault: () => {} }); }); + await act(async () => { fireEvent.click(await screen.findByRole("button", { name: "Save and close" })); }); + expect(windowApi.destroy).not.toHaveBeenCalled(); + expect(await screen.findByText(/while you were editing/)).toBeInTheDocument(); + }); + + it("a failed first read offers Retry, which loads the file", async () => { + commands.viewerReadFile.mockRejectedValueOnce(NOT_RUNNING.replace("checking this file for changes", "opening files")); + render(); + expect(await screen.findByText(/Start the project before opening files/)).toBeInTheDocument(); + const retry = screen.getByRole("button", { name: "Retry" }); + await act(async () => { fireEvent.click(retry); }); + expect(await screen.findByText("Saved")).toBeInTheDocument(); + expect(screen.getByTestId("code-editor")).toHaveTextContent("hello"); + expect(screen.queryByRole("button", { name: "Retry" })).toBeNull(); + }); + + it("a failed first read is retried by the poll until it succeeds", async () => { + commands.viewerReadFile.mockRejectedValueOnce("Docker is busy").mockRejectedValueOnce("Docker is still busy"); + render(); + expect(await screen.findByText("Docker is busy")).toBeInTheDocument(); + expect(commands.viewerReadFile).toHaveBeenCalledTimes(1); + await poll(); + expect(await screen.findByText("Docker is still busy")).toBeInTheDocument(); + expect(commands.viewerReadFile).toHaveBeenCalledTimes(2); + expect(commands.viewerPollFile).not.toHaveBeenCalled(); + await poll(2000); + expect(await screen.findByText("Saved")).toBeInTheDocument(); + expect(commands.viewerReadFile).toHaveBeenCalledTimes(3); + // Loaded: the poll is back to polling, not re-reading. + await poll(2000); + expect(commands.viewerPollFile).toHaveBeenCalled(); + expect(commands.viewerReadFile).toHaveBeenCalledTimes(3); + }); + it("closing with unsaved edits is intercepted", async () => { render(); await screen.findByText("/workspace/demo/notes.md"); @@ -249,7 +331,7 @@ describe("EditorPane", () => { render(); await screen.findByText("Saved"); commands.viewerPollFile.mockResolvedValue({ exists: true, hash: H2, size: 8 }); - commands.viewerReadFile.mockRejectedValueOnce("Container is not running.").mockResolvedValue(textFile("changed\n", H2)); + commands.viewerReadFile.mockRejectedValueOnce("Could not read the file: I/O error").mockResolvedValue(textFile("changed\n", H2)); await poll(); expect(screen.getByTestId("code-editor")).toHaveTextContent("hello"); await poll(2000); diff --git a/app/src/viewer/EditorPane.tsx b/app/src/viewer/EditorPane.tsx index 99664f7..ff35f18 100644 --- a/app/src/viewer/EditorPane.tsx +++ b/app/src/viewer/EditorPane.tsx @@ -7,6 +7,7 @@ import { decodeBase64, encodeBase64, imageMimeFor, previewLimit } from "../compo import { viewerPollFile, viewerReadFile, viewerWriteFile } from "../lib/tauri-commands"; import type { ViewerFile, ViewerLocation, ViewerState } from "../lib/types"; import { CodeEditor, type CodeEditorHandle } from "./CodeEditor"; +import { CONFLICT_PREFIX, GONE_PREFIX, READ_ONLY_MESSAGE } from "./ipcMessages"; import { classifyViewerFile, type Editability } from "./editability"; import { languageFor, wrapsLines } from "./languages"; import { decodeViewerText, encodeViewerText, type TextFormat } from "./textFormat"; @@ -34,7 +35,7 @@ type View = const errorText = (e: unknown) => (e instanceof Error ? e.message : String(e)); /** `write.rs`'s refusal to replace a file the container user may not write. */ -const isReadOnlyRefusal = (msg: string) => msg.includes("The file is read-only for the container user."); +const isReadOnlyRefusal = (msg: string) => msg.includes(READ_ONLY_MESSAGE); export default function EditorPane({ state }: { state: ViewerState }) { const path = state.state.kind === "resolved" ? state.state.container_path : ""; @@ -83,21 +84,36 @@ export default function EditorPane({ state }: { state: ViewerState }) { useEffect(() => () => { if (imageUrl.current) URL.revokeObjectURL(imageUrl.current); }, []); - useEffect(() => { - let cancelled = false; - void (async () => { - try { - const file = await viewerReadFile(previewLimit(path)); - if (cancelled) return; - show(file); - dispatch({ type: "loaded", hash: file.hash, truncated: file.truncated }); - } catch (e) { - if (!cancelled) setView({ kind: "error", message: errorText(e) }); - } - })(); - return () => { cancelled = true; }; + /** Bumped per initial-load attempt (and on unmount/path change); a stale attempt's result is dropped. */ + const loadGen = useRef(0); + + /** + * The initial read. Re-run by "Retry" and by the poll while the window shows + * a load error, so a window opened while the container was restarting + * recovers on its own instead of staying dead. + */ + const load = useCallback(async () => { + const gen = ++loadGen.current; + try { + const file = await viewerReadFile(previewLimit(path)); + if (loadGen.current !== gen) return; + show(file); + dispatch({ type: "loaded", hash: file.hash, truncated: file.truncated }); + } catch (e) { + if (loadGen.current === gen) setView({ kind: "error", message: errorText(e) }); + } }, [path, show]); + useEffect(() => { + void load(); + return () => { loadGen.current += 1; }; + }, [load]); + + const retryLoad = useCallback(() => { + setView({ kind: "loading" }); + void load(); + }, [load]); + // The language loads lazily and separately, so the text is on screen (and // polling runs) without waiting for a grammar chunk. useEffect(() => { @@ -122,20 +138,29 @@ export default function EditorPane({ state }: { state: ViewerState }) { }, [path, show]); // Poll (spec §5). A reload replaces the document only when the reducer says so. + // While the first read has failed, each tick retries that read instead. + // Always enabled, so a loading -> error flip does not fire an immediate extra read. useViewerPolling(POLL_MS, async () => { + if (view.kind === "loading") return; + if (view.kind === "error") { await load(); return; } // A poll that overlaps a save can carry the pre-save hash; skip it (M2). if (saving.current) return; const gen = saveGen.current; let poll; - try { poll = await viewerPollFile(); } catch { if (saveGen.current === gen) dispatch({ type: "poll_failed" }); return; } + try { + poll = await viewerPollFile(); + } catch (e) { + if (saveGen.current === gen) dispatch({ type: "poll_failed", message: errorText(e) }); + return; + } if (saveGen.current !== gen) return; const before = docRef.current; const after = reduceViewer(before, { type: "polled", poll }); dispatch({ type: "polled", poll }); if (pollEffect(before, after) === "reload") { - try { await reloadFromDisk(after.diskHash, true); } catch { dispatch({ type: "poll_failed" }); } + try { await reloadFromDisk(after.diskHash, true); } catch (e) { dispatch({ type: "poll_failed", message: errorText(e) }); } } - }, view.kind === "text" || view.kind === "image" || view.kind === "binary"); + }, true); const editable = view.kind === "text" && view.editability.editable; const saveEnabled = canSave(doc, editable); @@ -150,17 +175,20 @@ export default function EditorPane({ state }: { state: ViewerState }) { try { const bytes = encodeViewerText(handle.getDoc(), textFormat.current); const result = await viewerWriteFile(encodeBase64(bytes), baseHash).then( - (hash) => ({ ok: true as const, hash }), + (saved) => ({ ok: true as const, saved }), (e: unknown) => ({ ok: false as const, msg: errorText(e) }), ); saveGen.current += 1; if (result.ok) { - dispatch({ type: "saved", hash: result.hash }); + const { hash, disk_hash: diskHash } = result.saved; + dispatch({ type: "saved", hash, diskHash }); if (editGen.current !== gen) dispatch({ type: "edited" }); // typed while the save was in flight - else if (closingRef.current) await getCurrentWindow().destroy(); - } else if (result.msg.startsWith("conflict:")) { + // Another writer landed right after ours: the reducer shows "Changed on + // disk", and the window stays open so the user can decide. + else if (closingRef.current && diskHash === hash) await getCurrentWindow().destroy(); + } else if (result.msg.startsWith(CONFLICT_PREFIX)) { await adoptConflict(); - } else if (result.msg.startsWith("gone:")) { + } else if (result.msg.startsWith(GONE_PREFIX)) { dispatch({ type: "save_gone" }); } else if (isReadOnlyRefusal(result.msg)) { setSaveError({ text: READ_ONLY_SAVE }); @@ -181,8 +209,8 @@ export default function EditorPane({ state }: { state: ViewerState }) { let poll; try { poll = await viewerPollFile(); - } catch { - dispatch({ type: "poll_failed" }); + } catch (e) { + dispatch({ type: "poll_failed", message: errorText(e) }); dispatch({ type: "save_conflict" }); return; } @@ -230,6 +258,7 @@ export default function EditorPane({ state }: { state: ViewerState }) { const badge = useMemo((): { tone: StatusTone; label: string; detail?: string } | null => { if (view.kind === "loading" || view.kind === "error") return null; if (doc.containerDown) return { tone: "error", label: "Container not running" }; + if (doc.pollError) return { tone: "error", label: "Could not check for changes" }; if (doc.disk === "gone") return { tone: "error", label: "File no longer exists" }; if (!view.editability.editable) return { tone: "off", label: "Read-only", detail: view.editability.reason ?? undefined }; if (doc.disk === "changed") return { tone: "busy", label: "Changed on disk" }; @@ -251,6 +280,7 @@ export default function EditorPane({ state }: { state: ViewerState }) { {doc.containerDown && } + {doc.pollError && } {doc.disk === "gone" && } {doc.disk === "changed" && doc.doc === "dirty" && ( @@ -273,7 +303,13 @@ export default function EditorPane({ state }: { state: ViewerState }) {
{view.kind === "loading" &&

Loading…

} - {view.kind === "error" &&

{view.message}

} + {view.kind === "error" && ( +
+

{view.message}

+

The viewer retries every few seconds.

+ +
+ )} {view.kind === "binary" &&

{view.editability.reason}

} {view.kind === "image" && {path}} {view.kind === "text" && ( diff --git a/app/src/viewer/ipcMessages.ts b/app/src/viewer/ipcMessages.ts new file mode 100644 index 0000000..7aa301a --- /dev/null +++ b/app/src/viewer/ipcMessages.ts @@ -0,0 +1,21 @@ +/** + * The error strings the Rust side of the viewer produces and this side matches + * on. This file is the one TypeScript copy; the Rust originals are + * + * - `CONFLICT_PREFIX`, `GONE_PREFIX`, `READ_ONLY_MESSAGE` in + * `src-tauri/src/file_viewer/write.rs` (`viewer_write_file` errors), and + * - `NOT_RUNNING_PREFIX` in `src-tauri/src/commands/file_commands.rs` + * (`require_running` and the viewer's "no container" refusal). + * + * `write.rs`'s test `the_frontend_copies_of_the_ipc_messages_match` reads this + * file and fails if any literal here drifts from its Rust original. + */ + +/** A save refused because the file changed on disk since its base hash. */ +export const CONFLICT_PREFIX = "conflict:"; +/** A save refused because the file no longer exists. */ +export const GONE_PREFIX = "gone:"; +/** A save refused because the container user may not write the file. */ +export const READ_ONLY_MESSAGE = "The file is read-only for the container user."; +/** Any command refused because the project's container is not running. */ +export const NOT_RUNNING_PREFIX = "Start the project before"; diff --git a/app/src/viewer/viewerState.test.ts b/app/src/viewer/viewerState.test.ts index 5383619..2509e99 100644 --- a/app/src/viewer/viewerState.test.ts +++ b/app/src/viewer/viewerState.test.ts @@ -62,10 +62,22 @@ describe("reduceViewer", () => { }); it("a save clears dirty and aligns hashes; a conflict marks disk changed", () => { const s = reduceViewer(loaded(), { type: "edited" }); - expect(reduceViewer(s, { type: "saved", hash: H2 })).toMatchObject({ doc: "clean", disk: "same", baseHash: H2, diskHash: H2, overwrite: false }); + expect(reduceViewer(s, { type: "saved", hash: H2, diskHash: H2 })).toMatchObject({ doc: "clean", disk: "same", baseHash: H2, diskHash: H2, overwrite: false }); expect(reduceViewer(s, { type: "save_conflict" })).toMatchObject({ doc: "dirty", disk: "changed" }); expect(reduceViewer(s, { type: "save_gone" })).toMatchObject({ disk: "gone" }); }); + it("a save another writer overtook keeps our base but shows Changed on disk (M2)", () => { + const s = reduceViewer(loaded(), { type: "edited" }); + const raced = reduceViewer(s, { type: "saved", hash: H2, diskHash: H3 }); + expect(raced).toMatchObject({ doc: "dirty", disk: "changed", baseHash: H2, diskHash: H3, overwrite: false }); + expect(canSave(raced, true)).toBe(false); + // The next poll reporting that same foreign hash is quiet: the banner stays up. + const next = poll(raced, H3); + expect(next).toMatchObject({ disk: "changed", doc: "dirty" }); + expect(pollEffect(raced, next)).toBe("none"); + // Overwrite adopts what is on disk, not our own hash. + expect(reduceViewer(next, { type: "overwrite_on_save" })).toMatchObject({ baseHash: H3, disk: "same" }); + }); it("a gone file disables saving but keeps the buffer state", () => { const s = reduceViewer(loaded(), { type: "edited" }); const gone = poll(s, null, false); @@ -73,16 +85,33 @@ describe("reduceViewer", () => { expect(canSave(gone, true)).toBe(false); expect(pollEffect(s, gone)).toBe("banner"); }); - it("a failed poll flags the container down and a good one clears it", () => { + it("a poll refused as not running flags the container down and a good one clears it", () => { // Regression: start from a dirty doc, not a clean one -- otherwise // canSave(down, true) is false purely because doc !== "dirty", and the // assertion never actually exercises containerDown. const dirty = reduceViewer(loaded(), { type: "edited" }); - const down = reduceViewer(dirty, { type: "poll_failed" }); - expect(down.containerDown).toBe(true); + const down = reduceViewer(dirty, { + type: "poll_failed", + message: "Start the project before checking this file for changes — it runs inside the running container.", + }); + expect(down).toMatchObject({ containerDown: true, pollError: null }); expect(canSave(down, true)).toBe(false); expect(poll(down, H1).containerDown).toBe(false); }); + it("any other poll failure is kept as its own message, does not claim the container is down, and clears on a good poll", () => { + const dirty = reduceViewer(loaded(), { type: "edited" }); + const down = reduceViewer(dirty, { type: "poll_failed", message: "Start the project before checking this file for changes — files live in its container." }); + const failed = reduceViewer(down, { type: "poll_failed", message: "Could not check the file: Permission denied" }); + expect(failed).toMatchObject({ containerDown: false, pollError: "Could not check the file: Permission denied" }); + expect(canSave(failed, true)).toBe(true); + expect(poll(failed, H1)).toMatchObject({ pollError: null, containerDown: false }); + expect(poll(failed, null, false)).toMatchObject({ pollError: null, disk: "gone" }); + }); + it("a hash-less poll and a gone file reappearing both clear the flags; the reappeared file is same", () => { + const gone = poll(loaded(), null, false); + expect(poll(gone, H1)).toMatchObject({ disk: "same" }); + expect(poll(gone, null)).toMatchObject({ disk: "gone", containerDown: false, pollError: null }); + }); it("canSave needs dirty + editable + disk in sync", () => { expect(canSave(loaded(), true)).toBe(false); const dirty = reduceViewer(loaded(), { type: "edited" }); diff --git a/app/src/viewer/viewerState.ts b/app/src/viewer/viewerState.ts index b0b8a4b..405682c 100644 --- a/app/src/viewer/viewerState.ts +++ b/app/src/viewer/viewerState.ts @@ -14,6 +14,7 @@ * would re-download on every poll tick forever (spec Decision 2). */ import type { ViewerPoll } from "../lib/types"; +import { NOT_RUNNING_PREFIX } from "./ipcMessages"; export type DocStatus = "clean" | "dirty"; export type DiskStatus = "same" | "changed" | "gone"; @@ -25,7 +26,14 @@ export interface ViewerDocState { baseHash: string | null; /** Last known full-file hash on disk (null until known). */ diskHash: string | null; + /** The last poll was refused because the project's container is not running. */ containerDown: boolean; + /** + * The last poll failed for any other reason (an unreadable file, a Docker + * hiccup), with the backend's sentence. Changes on disk go unseen until a + * poll succeeds, but saving stays possible: the write re-checks the hash. + */ + pollError: string | null; /** Set for one render after a clean reload; UI shows "Reloaded". */ justReloaded: boolean; /** True when the user chose "Overwrite on save" after a disk change. */ @@ -36,10 +44,10 @@ export type ViewerAction = | { type: "loaded"; hash: string; truncated: boolean } | { type: "edited" } | { type: "polled"; poll: ViewerPoll } - | { type: "poll_failed" } + | { type: "poll_failed"; message: string } | { type: "reloaded"; hash: string; truncated: boolean; polledHash: string | null } | { type: "overwrite_on_save" } - | { type: "saved"; hash: string } + | { type: "saved"; hash: string; diskHash: string } | { type: "save_conflict" } | { type: "save_gone" }; @@ -49,6 +57,7 @@ export const initialViewerState: ViewerDocState = { baseHash: null, diskHash: null, containerDown: false, + pollError: null, justReloaded: false, overwrite: false, }; @@ -61,17 +70,22 @@ export function reduceViewer(state: ViewerDocState, action: ViewerAction): Viewe case "edited": return { ...s, doc: "dirty" }; case "polled": { - if (!action.poll.exists) return { ...s, disk: "gone", containerDown: false }; + const ok = { ...s, containerDown: false, pollError: null }; + if (!action.poll.exists) return { ...ok, disk: "gone" }; const hash = action.poll.hash; - if (hash === null) return { ...s, containerDown: false }; - if (s.diskHash === null) return { ...s, diskHash: hash, disk: s.disk === "gone" ? "same" : s.disk, containerDown: false }; - if (hash === s.diskHash) return { ...s, disk: s.disk === "gone" ? "same" : s.disk, containerDown: false }; + if (hash === null) return ok; + if (ok.diskHash === null) return { ...ok, diskHash: hash, disk: ok.disk === "gone" ? "same" : ok.disk }; + if (hash === ok.diskHash) return { ...ok, disk: ok.disk === "gone" ? "same" : ok.disk }; // Changed on disk. "Overwrite on save" adopted a base; a further change // on disk invalidates it again. - return { ...s, diskHash: hash, disk: "changed", overwrite: false, containerDown: false }; + return { ...ok, diskHash: hash, disk: "changed", overwrite: false }; } case "poll_failed": - return { ...s, containerDown: true }; + // Only the backend's "Start the project before …" refusal means the + // container is down; anything else is reported as what it says. + return action.message.startsWith(NOT_RUNNING_PREFIX) + ? { ...s, containerDown: true, pollError: null } + : { ...s, containerDown: false, pollError: action.message }; case "reloaded": return { ...s, @@ -85,6 +99,13 @@ export function reduceViewer(state: ViewerDocState, action: ViewerAction): Viewe case "overwrite_on_save": return { ...s, baseHash: s.diskHash, disk: "same", overwrite: true }; case "saved": + // The base is always the hash of the bytes written. If the disk already + // held something else right after the swap, another writer landed after + // us: the buffer is not what is on disk, so say "Changed on disk" (with + // Reload / Overwrite) rather than adopt the other writer's hash (M2). + if (action.diskHash !== action.hash) { + return { ...s, doc: "dirty", disk: "changed", baseHash: action.hash, diskHash: action.diskHash, overwrite: false }; + } return { ...s, doc: "clean", disk: "same", baseHash: action.hash, diskHash: action.hash, overwrite: false }; case "save_conflict": return { ...s, disk: "changed", overwrite: false }; diff --git a/docs/superpowers/specs/2026-09-22-terminal-file-viewer-design.md b/docs/superpowers/specs/2026-09-22-terminal-file-viewer-design.md index 5b79983..a7b8cea 100644 --- a/docs/superpowers/specs/2026-09-22-terminal-file-viewer-design.md +++ b/docs/superpowers/specs/2026-09-22-terminal-file-viewer-design.md @@ -115,7 +115,10 @@ AppManifest spec, `2026-09-22-app-manifest-lockdown-design.md`). `activate`/`hover`, including unparseable ones and `javascript:`; so the handler parses with `new URL()` itself: `file:` → `onOpenFile(pathname decoded, no line)`, `http(s):` → the existing `sanitizeRelayUrl` → `openUrlExternal` path unchanged, anything else → refused - (`console.warn`, no hover card). The hover card for a `file:` target shows the path and "Open + (`console.warn`, nothing opened). A refused target (`javascript:`, any other scheme, + unparseable text) keeps the existing refusal card ("This link will not be opened — it failed + the URL safety check"), which never echoes the target into the DOM; this is the ruled + behaviour (Task 9), not a new hover card. The hover card for a `file:` target shows the path and "Open in viewer". The `WebLinksAddon` comment that describes the old `allowNonHttpProtocols` behaviour is updated, not left stale. - Activation uses the same click gating as web links (`opensOnClick`: no selection drag, @@ -330,7 +333,8 @@ click, editable, CodeMirror 6, probe-roots resolution, 2 s polling, no autosave, Focus/title/unminimize are done from Rust, so no `core:window:allow-set-*` grants. 5. **OSC 8 with `allowNonHttpProtocols: true` receives unparseable and `javascript:` targets (§1).** The handler now parses and dispatches itself, and refuses everything but `file:` and - `http(s):` before drawing a hover card. The stale comment in the `WebLinksAddon` branch is + `http(s):` before drawing a file or web hover card; a refused target gets only the existing + refusal card, which never echoes the target. The stale comment in the `WebLinksAddon` branch is updated as part of the change. 6. **Wrapped-row joining is re-implemented (§1).** `WebLinksAddon`'s `LinkComputer` is not exported from the built package and `urlDetector.ts` never touches the buffer.