diff --git a/app/src-tauri/src/browser_view/commands.rs b/app/src-tauri/src/browser_view/commands.rs index 0b3a58e..15a0639 100644 --- a/app/src-tauri/src/browser_view/commands.rs +++ b/app/src-tauri/src/browser_view/commands.rs @@ -15,6 +15,12 @@ use crate::AppState; /// non-`Running` status carrying an explanation rather than an error, so the /// pane always has something specific to say. This is host-side only — no /// container recreation is involved either way. +/// +/// Either way the choice is persisted, so it survives an app restart. This is +/// the only caller allowed to write `false`: every other path to +/// [`BrowserViewManager::stop`](crate::browser_view::BrowserViewManager::stop) +/// is a teardown rather than the user changing their mind. Enabling persists +/// inside `start`, which is the single funnel for it. #[tauri::command] pub async fn set_browser_view_enabled( project_id: String, @@ -23,9 +29,15 @@ pub async fn set_browser_view_enabled( state: State<'_, AppState>, ) -> Result { if !enabled { + // Persist first, then tear down: the supervisor's own teardown emit + // reads this flag back out of the store, and reading it mid-stop would + // announce a view that is going away as still enabled. + state + .projects_store + .set_browser_view_enabled(&project_id, false)?; // Awaits the supervisor, so the host port is released before we return. manager().stop(&project_id).await; - return Ok(manager().status(&project_id).await); + return Ok(manager().status(&project_id, false).await); } let container_id = running_container(&state, &project_id, "opening the browser view").await?; @@ -40,10 +52,17 @@ pub async fn set_browser_view_enabled( .await } -/// Current status. Cheap: reads in-process state only, never the container. +/// Current status. Cheap: the session map in this process plus the stored flag, +/// never the container. +/// +/// The two are independent on purpose — this is what the pane reads on mount, +/// and after an app restart the honest answer is "enabled, nothing running". #[tauri::command] -pub async fn get_browser_view_status(project_id: String) -> Result { - Ok(manager().status(&project_id).await) +pub async fn get_browser_view_status( + project_id: String, + state: State<'_, AppState>, +) -> Result { + Ok(manager().status(&project_id, enabled_for(&state, &project_id)).await) } /// Probe the container for Playwright without starting anything. @@ -110,7 +129,9 @@ pub async fn open_browser_view_popout( app_handle: AppHandle, state: State<'_, AppState>, ) -> Result<(), String> { - let status = manager().status(&project_id).await; + let status = manager() + .status(&project_id, enabled_for(&state, &project_id)) + .await; let (BrowserViewState::Running, Some(url)) = (status.state, status.url.as_deref()) else { return Err( "The browser view isn't running. Start it before opening it in its own window." @@ -209,7 +230,9 @@ pub async fn open_page_in_container_browser( // the user to go and press Start in the Browser tab themselves — and from // the terminal's URL prompt, with no indication that was even needed. // Asking for a page *is* asking to watch it, so the viewer comes up too. - let status = manager().status(&project_id).await; + let status = manager() + .status(&project_id, enabled_for(&state, &project_id)) + .await; if status.state != BrowserViewState::Running { crate::commands::project_commands::emit_progress( &app_handle, @@ -229,7 +252,9 @@ pub async fn open_page_in_container_browser( // From the terminal there is no pane on screen to fill, so the page needs a // window of its own or it lands somewhere the user isn't looking. if show_window { - let status = manager().status(&project_id).await; + let status = manager() + .status(&project_id, enabled_for(&state, &project_id)) + .await; if let Some(url) = status.url.as_deref() { let name = state .projects_store @@ -311,6 +336,20 @@ pub async fn get_browser_view_match_window(project_id: String) -> Result, project_id: &str) -> bool { + state + .projects_store + .get(project_id) + .is_some_and(|p| p.browser_view_enabled) +} + /// The project's container, or a sentence saying why there isn't one. /// /// Every command here needs a *running* container, and every one of them used diff --git a/app/src-tauri/src/browser_view/mod.rs b/app/src-tauri/src/browser_view/mod.rs index b816749..dd35dcb 100644 --- a/app/src-tauri/src/browser_view/mod.rs +++ b/app/src-tauri/src/browser_view/mod.rs @@ -34,14 +34,22 @@ //! //! ## Lifecycle //! -//! Off by default and per-project opt-in, exactly like `auth_bridge_enabled`. +//! Off by default and per-project opt-in. The opt-in itself is +//! [`Project::browser_view_enabled`](crate::models::Project), persisted like +//! `auth_bridge_enabled` and read from the store on demand rather than cached +//! here — so the pane comes back the way it was left. What does *not* persist +//! is the session: nothing starts a viewer on app start, so a project left +//! enabled reports `enabled: true` with a state of `Off` until the pane asks +//! for one. That is deliberate, and the reason the flag and the session are +//! separate ideas — see [`BrowserViewManager::status`]. +//! //! One supervisor task per session owns the proxy and the viewer process, and it //! is the only thing that tears them down, so every way a session can end funnels //! through one code path: //! //! | Trigger | Path | //! |---|---| -//! | Turned off in the UI | `set_browser_view_enabled(false)` → [`BrowserViewManager::stop`] | +//! | Turned off in the UI | `set_browser_view_enabled(false)` → persist `false`, then [`BrowserViewManager::stop`] | //! | Container stopped, by the UI or otherwise | supervisor's `is_container_running` check | //! | Project deleted | supervisor's `store.get()` check | //! | Container rebuilt | old container stops → supervisor exits; the new one is not auto-started | @@ -59,7 +67,10 @@ //! orphan is reachable on container loopback only: the host-side port dies with //! the app, and [`crate::auth_bridge::RESERVED_CONTAINER_PORTS`] is a constant //! precisely so the bridge will not mirror an orphan the next time the app -//! starts. The next [`BrowserViewManager::start`] reclaims it. +//! starts. The next [`BrowserViewManager::start`] reclaims it — and since the +//! opt-in is now durable, the restarted app says `enabled` with nothing running, +//! which is exactly the state that invites the user to press the button that +//! reclaims it. Nothing reclaims it on its own, because nothing auto-starts. pub mod commands; pub mod detect; @@ -134,7 +145,10 @@ pub enum BrowserViewState { #[derive(Debug, Clone, Serialize)] pub struct BrowserViewStatus { - /// The per-project opt-in. Off by default. + /// The per-project opt-in, read from the persisted project record. Off by + /// default, and true without a `Running` state whenever the view is turned + /// on but has nothing up — a stopped container, or an app that has just + /// restarted and does not auto-start viewers. pub enabled: bool, pub state: BrowserViewState, /// Fully-formed, token-bearing URL for the pane's iframe. Loopback only. @@ -201,17 +215,20 @@ struct Session { type SessionMap = Arc>>; +/// Live sessions, and nothing else. +/// +/// The per-project opt-in deliberately is **not** a field here. It lives on +/// the project record as +/// [`browser_view_enabled`](crate::models::Project::browser_view_enabled) and +/// is read from [`ProjectsStore`] at each use, exactly as +/// [`crate::auth_bridge::AuthBridgeManager`] treats `auth_bridge_enabled`: +/// one copy, durable across a restart, and impossible to get out of step with +/// what the Config tab shows. A cached copy here was the previous design and +/// its only observable behaviour was forgetting the user's choice on every +/// app start. #[derive(Default)] pub struct BrowserViewManager { sessions: SessionMap, - /// The per-project opt-in. - /// - /// NOTE: in memory only, so it does not survive an app restart. The durable - /// home for this is a `browser_view_enabled: bool` field on - /// `models::Project` (see the report) — `models/project.rs` is out of scope - /// for this change, so the flag lives here and the wiring is otherwise - /// identical to `auth_bridge_enabled`. - enabled: Mutex>, next_epoch: AtomicU64, } @@ -226,22 +243,15 @@ pub fn manager() -> &'static Arc { } impl BrowserViewManager { - pub async fn is_enabled(&self, project_id: &str) -> bool { - self.enabled.lock().await.contains(project_id) - } - - async fn set_enabled(&self, project_id: &str, enabled: bool) { - let mut set = self.enabled.lock().await; - if enabled { - set.insert(project_id.to_string()); - } else { - set.remove(project_id); - } - } - /// Current status without touching the container. - pub async fn status(&self, project_id: &str) -> BrowserViewStatus { - let enabled = self.is_enabled(project_id).await; + /// + /// `enabled` is passed in rather than looked up, the way + /// [`crate::auth_bridge::AuthBridgeManager::status`] takes it: the flag is + /// the caller's to read from the store, and keeping it out of here is what + /// stops a second copy of it appearing. A project whose view is enabled but + /// whose container is stopped — or whose app has just restarted — reports + /// `enabled: true` with a state of `Off`, which is the honest answer. + pub async fn status(&self, project_id: &str, enabled: bool) -> BrowserViewStatus { match self.sessions.lock().await.get(project_id) { Some(session) => BrowserViewStatus { enabled, @@ -261,6 +271,14 @@ impl BrowserViewManager { /// /// Idempotent: a call while a live session exists returns that session's /// status untouched, so re-opening the tab does not restart the dashboard. + /// + /// This is the single funnel for turning the view **on**, so it is also + /// where the durable flag is written — both call sites (the toggle and + /// `open_page_in_container_browser`, which opens a page and then shows it) + /// mean "on", and neither can forget. The **off** direction is not + /// symmetric and must not be: [`Self::stop`] is reached by teardown paths + /// that are not the user changing their mind, so the command owns that + /// write. See [`Self::stop`]. pub async fn start( &self, project_id: String, @@ -268,7 +286,7 @@ impl BrowserViewManager { app: AppHandle, store: Arc, ) -> Result { - self.set_enabled(&project_id, true).await; + store.set_browser_view_enabled(&project_id, true)?; // Bind the answer before acting on it: `status()` takes the same lock, // and this mutex is not reentrant. @@ -279,7 +297,7 @@ impl BrowserViewManager { .get(&project_id) .is_some_and(|s| !s.supervisor.is_finished()); if already_live { - return Ok(self.status(&project_id).await); + return Ok(self.status(&project_id, true).await); } let detection = detect::detect(&container_id).await?; @@ -364,14 +382,21 @@ impl BrowserViewManager { }, ); - let status = self.status(&project_id).await; + let status = self.status(&project_id, true).await; emit(&app, &project_id, &status); Ok(status) } /// Stop one project's view and wait until its host port has been released. + /// + /// Tears the *session* down and deliberately leaves the durable flag alone. + /// Most callers are not the user turning the feature off — a migration + /// removes the container out from under a running view + /// (`migration_commands`), and the container can stop for any other reason + /// — and persisting `false` for those would quietly opt the project out of + /// a feature it never asked to lose. `set_browser_view_enabled(false)` is + /// the one caller that means it, and it writes the flag itself first. pub async fn stop(&self, project_id: &str) { - self.set_enabled(project_id, false).await; // Remove under the lock, then release it before awaiting: the // supervisor takes the same lock to deregister itself on exit. let session = self.sessions.lock().await.remove(project_id); @@ -483,7 +508,12 @@ async fn supervise( // longer exists. The session owns it, and this is where the session ends. let _ = popout::close(&app, &project_id); - let enabled = manager().is_enabled(&project_id).await; + // Straight from the store, like the auth bridge's own teardown emit: the + // session is over, but the project may well still be opted in — a stopped + // container is not a changed mind, and the pane has to show the difference. + let enabled = store + .get(&project_id) + .is_some_and(|p| p.browser_view_enabled); emit(&app, &project_id, &BrowserViewStatus::off(enabled)); } @@ -915,6 +945,25 @@ mod tests { assert!(s.url.is_none()); } + #[tokio::test] + async fn the_opt_in_and_the_live_session_are_separate_answers() { + let manager = BrowserViewManager::default(); + + // Exactly what the pane reads on mount after an app restart of a + // project that was left enabled: the durable flag says on, and nothing + // auto-starts, so the state is honestly `Off`. The old in-memory flag + // could not express this — it came back `false` and the pane silently + // showed the feature as never having been turned on. + let status = manager.status("p1", true).await; + assert!(status.enabled); + assert_eq!(status.state, BrowserViewState::Off); + assert!(status.url.is_none()); + + // The flag belongs to the caller, read from the store. The manager + // keeps no copy, so it has nothing to contradict it with. + assert!(!manager.status("p1", false).await.enabled); + } + #[test] fn an_unavailable_status_keeps_the_detail_the_user_needs() { let mut d = PlaywrightDetection::default(); diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index ac93bfe..25912c4 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -1100,6 +1100,15 @@ pub async fn update_project( project.container_id = stored.container_id; project.status = stored.status; + // `browser_view_enabled` is owned by `set_browser_view_enabled` and is + // restored here rather than taken from the payload, exactly like + // `container_id` and `status` above. The Config tab has no control for it + // — the Browser tab's toggle is the only way it ever changes — so the + // project object the frontend round-trips carries whatever it was told at + // load time and would silently undo a toggle made since. `auth_bridge_enabled` + // is different and does arrive through this save: the Config tab edits it, + // which is why the reconcile below follows whatever was just persisted. + project.browser_view_enabled = stored.browser_view_enabled; project.created_at = stored.created_at; project.updated_at = chrono::Utc::now().to_rfc3339(); diff --git a/app/src-tauri/src/models/project.rs b/app/src-tauri/src/models/project.rs index 5c22e15..70905fd 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -132,6 +132,26 @@ fn default_use_shared_auth_token() -> bool { true } +/// `auth_bridge_enabled` defaults to **on**, and the default is what makes +/// `claude login` work at all. +/// +/// The login flow binds a *random* ephemeral loopback port inside the +/// container and then sends the host's browser to `127.0.0.1:`. +/// On the host nothing is listening there, so the callback lands on a closed +/// port and the CLI waits for a redirect that can never arrive. The bridge +/// mirrors the container's loopback listeners onto the same host port, which +/// is the only thing that closes that loop — so off-by-default made a hang the +/// out-of-the-box experience. +/// +/// Returning `true` from a `#[serde(default)]` helper (rather than flipping the +/// constructor alone) is deliberate: existing `projects.json` records were +/// written before this field existed, or while it was off, and an absent key is +/// what the default is read for. A project that wants the old behaviour turns +/// the toggle off, which persists an explicit `false`. +fn default_auth_bridge_enabled() -> bool { + true +} + /// How much autonomy Claude Code is granted inside the container. /// /// Maps onto Claude Code CLI flags — see [`PermissionMode::cli_args`], which is @@ -336,17 +356,30 @@ pub struct Project { pub sandbox_mode_enabled: bool, #[serde(default)] pub mission_control_enabled: bool, - /// Opt in to the auth bridge: while the container runs, its loopback - /// listeners are mirrored onto the host's loopback so browser OAuth - /// callbacks (`claude login`, `fly login`, `aws sso login`) can reach them. + /// The auth bridge: while the container runs, its loopback listeners are + /// mirrored onto the host's loopback so browser OAuth callbacks + /// (`claude login`, `fly login`, `aws sso login`) can reach them. /// Purely host-side — it deliberately has no container-recreation label, /// because toggling it changes nothing about the container itself. - #[serde(default)] + /// + /// **On by default**, and opt-*out* rather than opt-in — see + /// [`default_auth_bridge_enabled`] for why the default is the feature. + #[serde(default = "default_auth_bridge_enabled")] pub auth_bridge_enabled: bool, /// Opt in to the browser-view pane, which watches and takes over the /// browser Claude drives with Playwright inside the container. Purely /// host-side like `auth_bridge_enabled`, so it likewise has no /// container-recreation label. + /// + /// This is the *durable* home of the flag: `BrowserViewManager` reads it + /// rather than keeping its own copy, so the pane comes back the way it was + /// left. Off by default, and unlike the auth bridge it stays that way — a + /// view costs a container exec, a Node daemon and a host port, and a + /// container without Playwright cannot serve one at all. + /// + /// Durable does **not** mean auto-started: nothing brings a viewer up on + /// app start, so a project left enabled reports `enabled` with a state of + /// `Off` until the pane (or `open_page_in_container_browser`) asks for one. #[serde(default)] pub browser_view_enabled: bool, /// Grant the container what a VPN client needs to build a tunnel: @@ -639,7 +672,7 @@ impl Project { allow_docker_access: false, sandbox_mode_enabled: false, mission_control_enabled: false, - auth_bridge_enabled: false, + auth_bridge_enabled: default_auth_bridge_enabled(), browser_view_enabled: false, vpn_support_enabled: false, use_shared_auth_token: default_use_shared_auth_token(), @@ -885,4 +918,69 @@ mod tests { let round_tripped: ClaudeCodeSettings = serde_json::from_str(&json).unwrap(); assert_eq!(round_tripped, partial); } + + // ── The host-side per-project toggles ───────────────────────────────── + + #[test] + fn a_project_stored_before_the_auth_bridge_existed_gets_it_turned_on() { + // The whole point of the serde default: `MAIN_SHAPE_PROJECT` is a real + // record written by a shipped binary and has no `auth_bridge_enabled` + // key at all. Without this, every existing project keeps hanging on + // `claude login` until its owner finds the toggle. + assert!(!MAIN_SHAPE_PROJECT.contains("auth_bridge_enabled")); + let project: Project = serde_json::from_str(MAIN_SHAPE_PROJECT).unwrap(); + assert!(project.auth_bridge_enabled); + + // The browser view is the other way round and must stay so: it costs a + // Node daemon, a container exec loop and a host port, and most + // containers have no Playwright to serve it with. + assert!(!project.browser_view_enabled); + } + + #[test] + fn turning_the_auth_bridge_off_survives_the_default() { + // Opt-out has to be expressible, or the toggle does nothing across a + // restart. An explicit `false` in the file beats the default. + let json = r#"{ "auth_bridge_enabled": false }"#; + #[derive(Deserialize)] + struct JustTheFlag { + #[serde(default = "default_auth_bridge_enabled")] + auth_bridge_enabled: bool, + } + let parsed: JustTheFlag = serde_json::from_str(json).unwrap(); + assert!(!parsed.auth_bridge_enabled); + + // And a saved project always writes the key, so the choice is pinned + // rather than re-defaulted on the next load. + let mut p = Project::new("demo".to_string(), Vec::new()); + p.auth_bridge_enabled = false; + let round_tripped: Project = + serde_json::from_str(&serde_json::to_string(&p).unwrap()).unwrap(); + assert!(!round_tripped.auth_bridge_enabled); + } + + #[test] + fn a_new_project_starts_with_the_bridge_on_and_the_view_off() { + let p = Project::new("demo".to_string(), Vec::new()); + assert!(p.auth_bridge_enabled); + assert!(!p.browser_view_enabled); + } + + #[test] + fn the_path_migration_never_writes_the_flags_and_so_cannot_defeat_the_default() { + // `ProjectsStore::new` runs every record through this before + // deserialising. If it inserted either key — even as `false` — the + // serde default above would never be consulted for an existing project + // and this change would be a no-op on exactly the projects it is for. + let legacy = serde_json::json!({ + "id": "p1", + "name": "demo", + "path": "/home/u/demo", + }); + let migrated = Project::migrate_from_value(legacy); + let obj = migrated.as_object().unwrap(); + assert!(obj.contains_key("paths"), "the migration should still do its own job"); + assert!(!obj.contains_key("auth_bridge_enabled")); + assert!(!obj.contains_key("browser_view_enabled")); + } } diff --git a/app/src-tauri/src/storage/projects_store.rs b/app/src-tauri/src/storage/projects_store.rs index e3ed311..338da50 100644 --- a/app/src-tauri/src/storage/projects_store.rs +++ b/app/src-tauri/src/storage/projects_store.rs @@ -241,6 +241,21 @@ impl ProjectsStore { } } + /// Granular setter for the browser view's opt-in, for the same reason + /// [`Self::set_auth_bridge_enabled`] has one: the pane toggles this while + /// the Config tab may be holding an older copy of the whole record. + pub fn set_browser_view_enabled(&self, project_id: &str, enabled: bool) -> Result<(), String> { + let mut projects = self.lock(); + if let Some(p) = projects.iter_mut().find(|p| p.id == project_id) { + p.browser_view_enabled = enabled; + p.updated_at = chrono::Utc::now().to_rfc3339(); + self.save(&projects)?; + Ok(()) + } else { + Err(format!("Project {} not found", project_id)) + } + } + pub fn set_container_id(&self, project_id: &str, container_id: Option) -> Result<(), String> { let mut projects = self.lock(); if let Some(p) = projects.iter_mut().find(|p| p.id == project_id) { @@ -338,4 +353,61 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + + /// A store over a temp file. `new()` insists on `dirs::data_dir()`, which + /// is the real user's; the fields are right here, so the granular setters + /// can be exercised against a directory the test owns. + fn store_over(dir: &Path, projects: Vec) -> ProjectsStore { + ProjectsStore { + projects: Mutex::new(projects), + file_path: dir.join("projects.json"), + } + } + + #[test] + fn the_browser_view_flag_is_written_to_disk_and_read_back() { + // The point of the whole exercise: before this the flag lived in a + // `HashSet` in `BrowserViewManager` and an app restart forgot it. + let dir = temp_dir("browser-view"); + let project = Project::new("demo".to_string(), Vec::new()); + let id = project.id.clone(); + let store = store_over(&dir, vec![project]); + + assert!(!store.get(&id).unwrap().browser_view_enabled); + store.set_browser_view_enabled(&id, true).unwrap(); + assert!(store.get(&id).unwrap().browser_view_enabled); + + // Durable, not merely in memory — this is what a restart reads. + let on_disk: Vec = + serde_json::from_str(&fs::read_to_string(dir.join("projects.json")).unwrap()).unwrap(); + assert!(on_disk[0].browser_view_enabled); + + store.set_browser_view_enabled(&id, false).unwrap(); + assert!(!store.get(&id).unwrap().browser_view_enabled); + + assert!(store.set_browser_view_enabled("no-such-project", true).is_err()); + + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn a_granular_toggle_leaves_every_other_field_alone() { + // Why these setters exist at all: the Config tab can be holding an + // older copy of the whole record while the pane flips one flag. + let dir = temp_dir("granular"); + let mut project = Project::new("demo".to_string(), Vec::new()); + project.claude_instructions = Some("keep me".to_string()); + let id = project.id.clone(); + let store = store_over(&dir, vec![project]); + + store.set_browser_view_enabled(&id, true).unwrap(); + store.set_auth_bridge_enabled(&id, false).unwrap(); + + let saved = store.get(&id).unwrap(); + assert_eq!(saved.claude_instructions.as_deref(), Some("keep me")); + assert!(saved.browser_view_enabled); + assert!(!saved.auth_bridge_enabled); + + fs::remove_dir_all(&dir).ok(); + } }