Put the layout store behind a boundary, and assert the words it puts on screen

Persistence here has two rules and neither was tested. Losing the layout must
never lose the session, so every failure falls back to the first-run workspace
and carries on; and the fallback must be visible, so each path returns a
status string the footer shows. Nine different strings, one of which is the
only notice a user gets that the arrangement they spent a morning on has been
dropped, and not one of them was asserted anywhere. The way to find out that
a corrupt snapshot reports "invalid layout ignored" was to corrupt one.

Ten tests now cover the branches a real machine reaches: nowhere to write, a
data directory that cannot be created, a database SQLite refuses to open, a
fresh database that is ready rather than restored, a round trip that restores
a panel created before the save, and both ways a snapshot can be unusable --
malformed JSON and well-formed JSON describing a workspace with no attached
panel, since parsing is not validation and only the second is easy to write by
accident.

workspace_database_path now reads through EnvSource rather than std::env, for
exactly the reason that trait was introduced in lumbridge-settings: the
workspace forbids unsafe, set_var is unsafe in Rust 2024, and a precedence
rule that cannot be exercised without mutating the process running the test is
a precedence rule that stays untested. The one behavioural consequence is that
a non-UTF-8 value in LUMBRIDGE_SPIKE_DB, XDG_DATA_HOME or HOME is now treated
as unset rather than used as a path; that is what every other setting in the
shell already does with such a value.

persist_panels stays a method, reduced to the one thing the shell owns: a
workspace with no store is not a save failure. It is memory-only, the footer
has said so since startup, and replacing that standing message with an error
every time a pane moved would say less, not more.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SPYebLiN2w4TqnHUYGdECq

Cargo.lock

@@ -3329,6 +3329,7 @@ dependencies = [
  "lumbridge-ui-fixture",
  "serde",
  "serde_json",
+ "tempfile",
 ]
 
 [[package]]

apps/lumbridge/Cargo.toml

@@ -20,5 +20,8 @@ lumbridge-ui-fixture = { path = "../../crates/lumbridge-ui-fixture" }
 serde = { version = "1.0.228", features = ["derive"] }
 serde_json = "1.0.149"
 
+[dev-dependencies]
+tempfile = "3.27.0"
+
 [lints]
 workspace = true

apps/lumbridge/src/main.rs

@@ -3,6 +3,7 @@ mod geometry;
 mod input;
 mod keymap;
 mod panel_registry;
+mod persistence;
 mod settings_view;
 mod sidebar;
 mod theme;
@@ -10,7 +11,6 @@ mod timing;
 mod usage_feed;
 
 use std::collections::BTreeMap;
-use std::path::PathBuf;
 use std::time::Duration;
 
 use gpui::{
@@ -52,7 +52,6 @@ use usage_feed::{UsageFeed, UsageSegment};
 
 const RUNTIME_POLL_INTERVAL: Duration = Duration::from_millis(16);
 const RUNTIME_DRAIN_LIMIT: usize = 64;
-const WORKSPACE_SNAPSHOT_ID: &str = "lumbridge-code-gpui-spike";
 /// The point size the terminal is painted at.
 const TERMINAL_FONT_SIZE: f32 = 13.0;
 /// Named rather than "monospace" so the measurement and the painting agree on a
@@ -499,67 +498,6 @@ fn dim_color(color: Rgba) -> Rgba {
     }
 }
 
-fn workspace_database_path() -> Option<PathBuf> {
-    if let Some(path) = std::env::var_os("LUMBRIDGE_SPIKE_DB") {
-        return Some(PathBuf::from(path));
-    }
-    if let Some(root) = std::env::var_os("XDG_DATA_HOME") {
-        return Some(PathBuf::from(root).join("lumbridge/lumbridge.db"));
-    }
-    std::env::var_os("HOME")
-        .map(PathBuf::from)
-        .map(|home| home.join(".local/share/lumbridge/lumbridge.db"))
-}
-
-fn load_panel_registry() -> (Option<Store>, PanelRegistry, String) {
-    let Some(path) = workspace_database_path() else {
-        return (
-            None,
-            PanelRegistry::first_run(),
-            "layout memory-only · no data directory".to_owned(),
-        );
-    };
-    if let Some(parent) = path.parent()
-        && let Err(error) = std::fs::create_dir_all(parent)
-    {
-        return (
-            None,
-            PanelRegistry::first_run(),
-            format!("layout memory-only · {error}"),
-        );
-    }
-    let store = match Store::open(&path) {
-        Ok(store) => store,
-        Err(error) => {
-            return (
-                None,
-                PanelRegistry::first_run(),
-                format!("layout memory-only · {error}"),
-            );
-        }
-    };
-    match store.workspace_snapshot(WORKSPACE_SNAPSHOT_ID) {
-        Ok(Some(json)) => match PanelRegistry::from_json(&json) {
-            Ok(panels) => (Some(store), panels, "layout restored · SQLite".to_owned()),
-            Err(error) => (
-                Some(store),
-                PanelRegistry::first_run(),
-                format!("invalid layout ignored · {error}"),
-            ),
-        },
-        Ok(None) => (
-            Some(store),
-            PanelRegistry::first_run(),
-            "layout ready · SQLite".to_owned(),
-        ),
-        Err(error) => (
-            Some(store),
-            PanelRegistry::first_run(),
-            format!("layout read failed · {error}"),
-        ),
-    }
-}
-
 impl LumbridgeShell {
     fn new(window: &mut Window, cx: &mut Context<Self>) -> Self {
         let root_focus = cx.focus_handle();
@@ -602,7 +540,7 @@ impl LumbridgeShell {
         })
         .detach();
 
-        let (store, panels, persistence_status) = load_panel_registry();
+        let (store, panels, persistence_status) = persistence::load_panel_registry(&ProcessEnv);
         let sidebar = SidebarState::default();
         let cell = CellMetrics::measure(cx);
         let terminal_dimensions = terminal_dimensions_for_window(
@@ -680,23 +618,19 @@ impl LumbridgeShell {
         }
     }
 
+    /// Writes the layout, or records why it was not written.
+    ///
+    /// The whole decision lives in [`persistence::persist_panels`]; what stays
+    /// here is the one thing the shell owns, which is that a workspace with no
+    /// store is not a failure to report. It is memory-only, the footer already
+    /// says so from startup, and overwriting that with a save error every time
+    /// a pane moved would replace an accurate standing message with a noisier
+    /// one that says less.
     fn persist_panels(&mut self) {
         let Some(store) = &self.store else {
             return;
         };
-        let result = self
+        self.persistence_status = persistence::persist_panels(store, &self.panels);
-            .panels
-            .to_json()
-            .map_err(|error| error.clone())
-            .and_then(|json| {
-                store
-                    .save_workspace_snapshot(WORKSPACE_SNAPSHOT_ID, &json)
-                    .map_err(|error| error.to_string())
-            });
-        self.persistence_status = match result {
-            Ok(()) => "layout saved · SQLite".to_owned(),
-            Err(error) => format!("layout save failed · {error}"),
-        };
     }
 
     fn dispatch(&mut self, action: ShellAction) -> ActionOutcome {

apps/lumbridge/src/persistence.rs

@@ -0,0 +1,317 @@
+//! Where the workspace layout is kept, and what the shell says when keeping it
+//! did not work.
+//!
+//! Two rules run through everything here. The first is that losing the layout
+//! must never lose the session: every failure below falls back to
+//! [`PanelRegistry::first_run`] and carries on, because a shell that refuses to
+//! open because a database file is unreadable has turned a cosmetic problem
+//! into a total one. The second is that the fallback must be *visible*. Each
+//! path returns a status string the footer shows, so `layout memory-only` and
+//! `layout saved · SQLite` are different words on screen; silently degrading to
+//! memory-only would leave a user rearranging panes every morning with no idea
+//! why they never stick.
+//!
+//! Those strings are the reason this file has tests at all. They are the entire
+//! user-visible contract of the persistence layer, and until now not one of
+//! them was asserted anywhere — the only way to find out that a broken database
+//! reports "layout read failed" rather than nothing was to break one.
+//!
+//! This module does IO and still has no renderer in it, which is what makes the
+//! tests possible: a `tempfile::TempDir` and a [`MapEnv`] reproduce every
+//! branch, including the ones that only happen on a machine with no writable
+//! data directory.
+//!
+//! The environment is read through [`EnvSource`] rather than `std::env`
+//! directly, for the reason that trait already documents in
+//! `lumbridge-settings`: the workspace forbids `unsafe`, `set_var` is unsafe in
+//! Rust 2024, and a precedence rule that cannot be tested without mutating the
+//! process running the test is a precedence rule nobody will test.
+
+use std::path::PathBuf;
+
+use lumbridge_settings::EnvSource;
+use lumbridge_storage::Store;
+
+use crate::panel_registry::PanelRegistry;
+
+/// The workspace this build reads and writes.
+///
+/// One workspace, named after the spike the shell grew out of. Changing the
+/// string orphans every layout already on disk, which is why it is a constant
+/// and not a format string over anything.
+const WORKSPACE_SNAPSHOT_ID: &str = "lumbridge-code-gpui-spike";
+
+/// Where the layout database lives, in order of who gets to decide.
+///
+/// `LUMBRIDGE_SPIKE_DB` first so a test or a second instance can be pointed at
+/// its own file without touching the real one; then `XDG_DATA_HOME`, which is
+/// the answer on a machine that has one; then `HOME` with the path XDG
+/// specifies as the default. `None` means there is nowhere to write, not that
+/// writing failed — the caller distinguishes the two in what it tells the user.
+pub(crate) fn workspace_database_path(env: &impl EnvSource) -> Option<PathBuf> {
+    if let Some(path) = env.get("LUMBRIDGE_SPIKE_DB") {
+        return Some(PathBuf::from(path));
+    }
+    if let Some(root) = env.get("XDG_DATA_HOME") {
+        return Some(PathBuf::from(root).join("lumbridge/lumbridge.db"));
+    }
+    env.get("HOME")
+        .map(PathBuf::from)
+        .map(|home| home.join(".local/share/lumbridge/lumbridge.db"))
+}
+
+pub(crate) fn load_panel_registry(env: &impl EnvSource) -> (Option<Store>, PanelRegistry, String) {
+    let Some(path) = workspace_database_path(env) else {
+        return (
+            None,
+            PanelRegistry::first_run(),
+            "layout memory-only · no data directory".to_owned(),
+        );
+    };
+    if let Some(parent) = path.parent()
+        && let Err(error) = std::fs::create_dir_all(parent)
+    {
+        return (
+            None,
+            PanelRegistry::first_run(),
+            format!("layout memory-only · {error}"),
+        );
+    }
+    let store = match Store::open(&path) {
+        Ok(store) => store,
+        Err(error) => {
+            return (
+                None,
+                PanelRegistry::first_run(),
+                format!("layout memory-only · {error}"),
+            );
+        }
+    };
+    match store.workspace_snapshot(WORKSPACE_SNAPSHOT_ID) {
+        Ok(Some(json)) => match PanelRegistry::from_json(&json) {
+            Ok(panels) => (Some(store), panels, "layout restored · SQLite".to_owned()),
+            Err(error) => (
+                Some(store),
+                PanelRegistry::first_run(),
+                format!("invalid layout ignored · {error}"),
+            ),
+        },
+        Ok(None) => (
+            Some(store),
+            PanelRegistry::first_run(),
+            "layout ready · SQLite".to_owned(),
+        ),
+        Err(error) => (
+            Some(store),
+            PanelRegistry::first_run(),
+            format!("layout read failed · {error}"),
+        ),
+    }
+}
+
+/// Writes the layout and reports what happened, in the footer's own words.
+pub(crate) fn persist_panels(store: &Store, panels: &PanelRegistry) -> String {
+    let result = panels
+        .to_json()
+        .map_err(|error| error.clone())
+        .and_then(|json| {
+            store
+                .save_workspace_snapshot(WORKSPACE_SNAPSHOT_ID, &json)
+                .map_err(|error| error.to_string())
+        });
+    match result {
+        Ok(()) => "layout saved · SQLite".to_owned(),
+        Err(error) => format!("layout save failed · {error}"),
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{
+        WORKSPACE_SNAPSHOT_ID, load_panel_registry, persist_panels, workspace_database_path,
+    };
+    use crate::panel_registry::PanelRegistry;
+    use lumbridge_settings::MapEnv;
+    use lumbridge_storage::Store;
+    use std::path::PathBuf;
+
+    /// An environment pointing the shell at a database inside `dir`.
+    fn spike_db(dir: &tempfile::TempDir) -> MapEnv {
+        let path = dir.path().join("workspace.db");
+        MapEnv::new([(
+            "LUMBRIDGE_SPIKE_DB",
+            path.to_str().expect("the tempdir path is UTF-8"),
+        )])
+    }
+
+    #[test]
+    fn the_override_outranks_xdg_and_xdg_outranks_home() {
+        // The order matters more than it looks: an instance told to use its own
+        // database must not quietly write to the real one, and a machine with
+        // XDG_DATA_HOME set has already answered the question HOME would only
+        // be guessing at.
+        assert_eq!(
+            workspace_database_path(&MapEnv::new([
+                ("LUMBRIDGE_SPIKE_DB", "/tmp/explicit.db"),
+                ("XDG_DATA_HOME", "/tmp/xdg"),
+                ("HOME", "/home/someone"),
+            ])),
+            Some(PathBuf::from("/tmp/explicit.db"))
+        );
+        assert_eq!(
+            workspace_database_path(&MapEnv::new([
+                ("XDG_DATA_HOME", "/tmp/xdg"),
+                ("HOME", "/home/someone"),
+            ])),
+            Some(PathBuf::from("/tmp/xdg/lumbridge/lumbridge.db"))
+        );
+        assert_eq!(
+            workspace_database_path(&MapEnv::new([("HOME", "/home/someone")])),
+            Some(PathBuf::from(
+                "/home/someone/.local/share/lumbridge/lumbridge.db"
+            ))
+        );
+    }
+
+    #[test]
+    fn no_data_directory_at_all_is_none_rather_than_a_relative_path() {
+        // Falling back to a bare "lumbridge.db" here would write into whatever
+        // directory the process happened to start in, which is a file nobody
+        // asked for in a place nobody will look.
+        assert_eq!(workspace_database_path(&MapEnv::default()), None);
+    }
+
+    #[test]
+    fn with_nowhere_to_write_the_shell_still_opens_and_says_it_is_memory_only() {
+        let (store, panels, status) = load_panel_registry(&MapEnv::default());
+        assert!(store.is_none());
+        assert_eq!(panels.attached_count(), 3, "the first-run workspace opens");
+        assert_eq!(status, "layout memory-only · no data directory");
+    }
+
+    #[test]
+    fn a_directory_that_cannot_be_created_degrades_to_memory_only() {
+        let dir = tempfile::tempdir().expect("tempdir");
+        let blocker = dir.path().join("not-a-directory");
+        std::fs::write(&blocker, b"").expect("write the blocking file");
+        let inside = blocker.join("workspace.db");
+        let env = MapEnv::new([(
+            "LUMBRIDGE_SPIKE_DB",
+            inside.to_str().expect("the tempdir path is UTF-8"),
+        )]);
+
+        let (store, _, status) = load_panel_registry(&env);
+        assert!(store.is_none());
+        assert!(
+            status.starts_with("layout memory-only · "),
+            "an unusable data directory must be named, not swallowed: {status}"
+        );
+    }
+
+    #[test]
+    fn a_database_that_will_not_open_degrades_to_memory_only() {
+        // The path exists and is a directory, so create_dir_all succeeds and
+        // SQLite is the one that refuses. This is the branch that separates
+        // "nowhere to write" from "somewhere that does not work".
+        let dir = tempfile::tempdir().expect("tempdir");
+        let occupied = dir.path().join("occupied");
+        std::fs::create_dir(&occupied).expect("create the occupying directory");
+        let env = MapEnv::new([(
+            "LUMBRIDGE_SPIKE_DB",
+            occupied.to_str().expect("the tempdir path is UTF-8"),
+        )]);
+
+        let (store, panels, status) = load_panel_registry(&env);
+        assert!(store.is_none());
+        assert_eq!(panels.attached_count(), 3);
+        assert!(
+            status.starts_with("layout memory-only · "),
+            "a database that will not open must be named: {status}"
+        );
+        assert_ne!(status, "layout memory-only · no data directory");
+    }
+
+    #[test]
+    fn a_fresh_database_is_ready_rather_than_restored() {
+        // "restored" is a claim about the user's own layout. Saying it over a
+        // database with nothing in it would be a lie the very first time the
+        // app is opened.
+        let dir = tempfile::tempdir().expect("tempdir");
+        let (store, _, status) = load_panel_registry(&spike_db(&dir));
+        assert!(store.is_some());
+        assert_eq!(status, "layout ready · SQLite");
+    }
+
+    #[test]
+    fn a_saved_layout_comes_back_and_says_it_was_restored() {
+        let dir = tempfile::tempdir().expect("tempdir");
+        let env = spike_db(&dir);
+
+        let (store, mut panels, _) = load_panel_registry(&env);
+        let store = store.expect("the store opened");
+        let created = panels.create(crate::panel_registry::PanelKind::Terminal);
+        assert_eq!(persist_panels(&store, &panels), "layout saved · SQLite");
+        drop(store);
+
+        let (_, restored, status) = load_panel_registry(&env);
+        assert_eq!(status, "layout restored · SQLite");
+        assert!(
+            restored.panel(created).is_some(),
+            "the panel created before the save must survive it"
+        );
+    }
+
+    #[test]
+    fn an_unreadable_layout_is_ignored_by_name_and_the_workspace_still_opens() {
+        // The failure this guards is the worst one available: a snapshot the
+        // shell cannot parse must not stop the shell starting, and it must not
+        // be discarded quietly either, because the status line is the only
+        // notice a user gets that their arrangement was dropped.
+        let dir = tempfile::tempdir().expect("tempdir");
+        let env = spike_db(&dir);
+        let path = workspace_database_path(&env).expect("the override names a path");
+        let store = Store::open(&path).expect("the store opened");
+        store
+            .save_workspace_snapshot(WORKSPACE_SNAPSHOT_ID, "{ not json at all")
+            .expect("the snapshot row was written");
+        drop(store);
+
+        let (store, panels, status) = load_panel_registry(&env);
+        assert!(
+            store.is_some(),
+            "a corrupt snapshot is not a reason to stop persisting"
+        );
+        assert!(
+            status.starts_with("invalid layout ignored · "),
+            "the reason must reach the footer: {status}"
+        );
+        assert_eq!(
+            panels.attached_count(),
+            PanelRegistry::first_run().attached_count()
+        );
+    }
+
+    #[test]
+    fn a_structurally_valid_but_impossible_layout_is_refused_the_same_way() {
+        // Parsing is not validation. A snapshot whose selected panel is not
+        // attached is well-formed JSON and an unopenable workspace, and it must
+        // land in the same fallback rather than reaching the shell.
+        let dir = tempfile::tempdir().expect("tempdir");
+        let env = spike_db(&dir);
+        let path = workspace_database_path(&env).expect("the override names a path");
+        let store = Store::open(&path).expect("the store opened");
+        store
+            .save_workspace_snapshot(
+                WORKSPACE_SNAPSHOT_ID,
+                r#"{"schema_version":1,"panels":[],"selected":1,"next_id":2}"#,
+            )
+            .expect("the snapshot row was written");
+        drop(store);
+
+        let (_, _, status) = load_panel_registry(&env);
+        assert!(
+            status.starts_with("invalid layout ignored · "),
+            "validation failures share the fallback with parse failures: {status}"
+        );
+    }
+}