diff --git a/crates/maestro-cli/src/sessions.rs b/crates/maestro-cli/src/sessions.rs index 1a087dc..902a2de 100644 --- a/crates/maestro-cli/src/sessions.rs +++ b/crates/maestro-cli/src/sessions.rs @@ -153,6 +153,7 @@ mod tests { command: command.into(), env: HashMap::new(), metadata: HashMap::new(), + worktree: None, status: SessionStatus::Running, created_at: Utc.with_ymd_and_hms(2026, 5, 25, 12, 0, 0).unwrap(), foreground: None, diff --git a/crates/maestro-daemon/src/projects.rs b/crates/maestro-daemon/src/projects.rs index 68b49c4..aab747e 100644 --- a/crates/maestro-daemon/src/projects.rs +++ b/crates/maestro-daemon/src/projects.rs @@ -262,10 +262,16 @@ impl ProjectScanner { /// Cached snapshot, rescanning (off-thread) when the TTL has lapsed. pub async fn repos(&self) -> std::sync::Arc> { + self.repos_inner(false).await + } + + async fn repos_inner(&self, force: bool) -> std::sync::Arc> { let mut cache = self.cache.lock().await; - if let Some((at, repos)) = cache.as_ref() { - if at.elapsed() < self.ttl { - return repos.clone(); + if !force { + if let Some((at, repos)) = cache.as_ref() { + if at.elapsed() < self.ttl { + return repos.clone(); + } } } let roots = self.roots.clone(); @@ -281,9 +287,11 @@ impl ProjectScanner { repos } - /// Rescan now, ignoring the TTL. Called by the background timer. + /// Rescan now, ignoring the TTL. Called by the background timer — going + /// through `repos()` would hit the cache and make most ticks no-ops, + /// leaving requests to pay for the rescan instead (#54 review). async fn refresh(&self) { - self.repos().await; + self.repos_inner(true).await; } /// Spawn the slow background refresh task. Drop the returned handle to diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index ac54fb1..6b0d0af 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -290,6 +290,12 @@ impl SessionManager { command: req.command, env: req.env, metadata: req.metadata, + worktree: worktree_path.as_ref().zip(worktree_base_repo.as_ref()).map( + |(path, base_repo)| maestro_protocol::SessionWorktree { + path: path.clone(), + base_repo: base_repo.clone(), + }, + ), status: SessionStatus::Running, created_at: Utc::now(), // A freshly-spawned session is producing its initial output; the @@ -430,6 +436,7 @@ impl SessionManager { command: String::new(), env: Default::default(), metadata: Default::default(), + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), // Activity is unknown on adoption; the watcher fills it in. @@ -457,8 +464,11 @@ impl SessionManager { SessionRecord { session: session.clone(), fifo_path, - worktree_path: None, - worktree_base_repo: None, + // Restored from the sidecar: without it the recovered + // session cleans up nothing on kill, and the leftover + // directory blocks that branch forever (#54 review). + worktree_path: session.worktree.as_ref().map(|w| w.path.clone()), + worktree_base_repo: session.worktree.as_ref().map(|w| w.base_repo.clone()), bridge, }, ); diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 164021c..7a1450d 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -54,14 +54,20 @@ impl WorktreeManager { /// spawning the same branch name target one directory and the second /// fails with `PathExists`. fn target_path(&self, base_repo: &Path, branch: &str) -> PathBuf { - let slug = sanitize_branch_for_path(branch); + // An unusable branch name falls back to a digest rather than escaping + // the root; `run_add` surfaces git's own error for it anyway. + let slug = branch_relpath(branch).unwrap_or_else(|| PathBuf::from(short_hash(branch))); let main = main_checkout(base_repo); match &self.root_override { Some(root) => { - let repo = main + // Basename alone collides for two checkouts of the same name + // (`~/a/maestro` and `~/b/maestro`), so disambiguate with a + // digest of the full path (#54 review). + let base = main .file_name() .map(|n| n.to_string_lossy().into_owned()) .unwrap_or_else(|| "repo".into()); + let repo = format!("{base}-{}", short_hash(&main.to_string_lossy())); root.join(repo).join(slug) } None => main.join("worktrees").join(slug), @@ -180,26 +186,54 @@ fn nested_worktree(path: &Path) -> Option { .args(["worktree", "list", "--porcelain"]) .output() .ok()?; + // `worktree list` prints fully-resolved paths while ours comes from + // `--git-common-dir` and may still hold symlinks. Comparing the two forms + // directly makes the guard silently pass on any symlinked repo path — and + // this guard is what stands between a kill and deleting an inner + // worktree's uncommitted work (#54 review). + let needle = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); let text = String::from_utf8_lossy(&out.stdout); text.lines() .filter_map(|l| l.strip_prefix("worktree ")) .map(PathBuf::from) - .find(|p| p != path && p.starts_with(path)) + .find(|p| { + let real = p.canonicalize().unwrap_or_else(|_| p.clone()); + real != needle && real.starts_with(&needle) + }) } -fn sanitize_branch_for_path(branch: &str) -> String { - let mut out = String::with_capacity(branch.len()); - for c in branch.chars() { - if c.is_ascii_alphanumeric() || c == '-' || c == '_' { - out.push(c); - } else if c == '/' || c == '.' { - out.push('-'); - } +/// Short, stable digest used to disambiguate override roots. +fn short_hash(s: &str) -> String { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + s.hash(&mut h); + format!("{:08x}", h.finish() as u32) +} + +/// The relative directory a branch's worktree lives in. +/// +/// A git branch name is already a valid relative path — git stores it as +/// `.git/refs/heads/feat/example` — so it is used verbatim and `feat/example` +/// nests under `worktrees/feat/`. That keeps the path readable *and* keeps the +/// mapping injective: flattening `/` and `.` to `-` made `feat/x`, `feat.x` +/// and `feat-x` share one directory, and with deterministic paths plus +/// `PathExists` as a hard error the losers could never get a worktree at all +/// (#54 review). +/// +/// Git already forbids holding both `feat` and `feat/x`, so the directory +/// collision that nesting could cause cannot arise from valid refs. Only +/// traversal and absolute components are rejected. +fn branch_relpath(branch: &str) -> Option { + use std::path::Component; + let candidate = Path::new(branch); + if branch.is_empty() + || !candidate + .components() + .all(|c| matches!(c, Component::Normal(_))) + { + return None; } - if out.is_empty() { - out.push_str("wt"); - } - out + Some(candidate.to_path_buf()) } /// `Some(reason)` when deleting `path` would destroy work. diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index deb2cc7..15e9aa2 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -370,7 +370,7 @@ async fn kill_removes_worktree_even_when_spec_says_keep() { let fx = DaemonFixture::new(); let repo = TempDir::new().unwrap(); init_repo(repo.path()); - let wt_path = repo.path().join("worktrees").join("feat-keep"); + let wt_path = repo.path().join("worktrees").join("feat").join("keep"); let s = fx .manager @@ -401,7 +401,7 @@ async fn kill_leaves_worktree_with_unsaved_work_on_disk() { let fx = DaemonFixture::new(); let repo = TempDir::new().unwrap(); init_repo(repo.path()); - let wt_path = repo.path().join("worktrees").join("feat-guarded"); + let wt_path = repo.path().join("worktrees").join("feat").join("guarded"); let s = fx .manager diff --git a/crates/maestro-daemon/tests/sidecar.rs b/crates/maestro-daemon/tests/sidecar.rs index f270d1e..4997f1a 100644 --- a/crates/maestro-daemon/tests/sidecar.rs +++ b/crates/maestro-daemon/tests/sidecar.rs @@ -19,6 +19,7 @@ fn sample(id: Uuid) -> Session { command: "bash".into(), env: HashMap::from([("FOO".into(), "bar".into())]), metadata: HashMap::from([("name".into(), Value::String("maestro-impl".into()))]), + worktree: None, status: SessionStatus::Running, created_at: DateTime::parse_from_rfc3339("2026-05-24T12:00:00Z") .unwrap() diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index 50f63dd..cea834e 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -53,7 +53,7 @@ fn add_places_worktree_inside_the_base_repo_by_default() { let mgr = WorktreeManager::new(None); let path = mgr.add_new(&repo, "feat/example").expect("add worktree"); - assert_eq!(path, repo.join("worktrees").join("feat-example")); + assert_eq!(path, repo.join("worktrees").join("feat").join("example")); assert!(path.exists()); assert!(path.join("README").exists()); @@ -74,7 +74,24 @@ fn worktree_root_override_redirects_placement() { // repos spawning the same branch name don't target one directory. let repo_name = repo.file_name().unwrap(); let path = mgr.add_new(&repo, "feat/example").expect("add worktree"); - assert_eq!(path, wt_root.path().join(repo_name).join("feat-example")); + // The override root disambiguates same-named checkouts with a digest of + // the full repo path, and the branch keeps its own shape. + // /-/feat/example + let prefix = format!("{}-", repo_name.to_string_lossy()); + let repo_dir_name = path + .strip_prefix(wt_root.path()) + .unwrap() + .components() + .next() + .unwrap(); + assert!( + repo_dir_name + .as_os_str() + .to_string_lossy() + .starts_with(&prefix), + "{path:?}" + ); + assert!(path.ends_with("feat/example"), "{path:?}"); assert!(path.exists()); } @@ -86,7 +103,7 @@ fn existing_directory_at_target_path_is_an_error_carrying_the_path() { // A predictable path means collisions are possible; they must be // reported, never silently relocated. - std::fs::create_dir_all(repo.join("worktrees").join("feat-collide")).unwrap(); + std::fs::create_dir_all(repo.join("worktrees").join("feat").join("collide")).unwrap(); let mgr = WorktreeManager::new(None); let err = mgr.add_new(&repo, "feat/collide").unwrap_err(); @@ -94,7 +111,8 @@ fn existing_directory_at_target_path_is_an_error_carrying_the_path() { assert!( msg.contains( repo.join("worktrees") - .join("feat-collide") + .join("feat") + .join("collide") .to_str() .unwrap() ), @@ -312,10 +330,9 @@ fn placement_resolves_a_linked_worktree_to_the_main_checkout() { !inner.starts_with(&outer), "must not nest: {inner:?} inside {outer:?}" ); - assert_eq!( - inner.parent().and_then(|p| p.parent()), - Some(repo.canonicalize().unwrap().as_path()), - "placed against the main checkout" + assert!( + inner.starts_with(repo.canonicalize().unwrap().join("worktrees")), + "placed against the main checkout: {inner:?}" ); } @@ -363,3 +380,22 @@ fn override_root_keeps_repos_apart() { let b = mgr.add_new(b_dir.path(), "feat/shared").expect("repo b"); assert_ne!(a, b, "same branch name in two repos must not collide"); } + +/// Flattening `/` and `.` to `-` made these three share one directory, and a +/// deterministic path plus a hard `PathExists` meant the losers could never +/// get a worktree (#54 review). +#[test] +fn similar_branch_names_get_distinct_directories() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + let mgr = WorktreeManager::new(None); + let a = mgr.add_new(&repo, "feat/x").expect("feat/x"); + let b = mgr.add_new(&repo, "feat.x").expect("feat.x"); + let c = mgr.add_new(&repo, "feat-x").expect("feat-x"); + assert_ne!(a, b); + assert_ne!(b, c); + assert_ne!(a, c); + assert!(a.ends_with("feat/x"), "{a:?}"); +} diff --git a/crates/maestro-e2e/tests/e2e.rs b/crates/maestro-e2e/tests/e2e.rs index 6ad0b82..cbb213a 100644 --- a/crates/maestro-e2e/tests/e2e.rs +++ b/crates/maestro-e2e/tests/e2e.rs @@ -156,7 +156,7 @@ async fn kill_removes_clean_worktree() { assert!( worktrees .iter() - .any(|w| w.contains("worktrees/feature-e2e")), + .any(|w| w.contains("worktrees/feature/e2e")), "expected a worktree for feature/e2e under /worktrees, got: {worktrees:?}" ); @@ -186,7 +186,11 @@ async fn kill_spares_worktree_with_uncommitted_changes() { let base = TempDir::new().unwrap(); init_repo(base.path()); - let wt_path = base.path().join("worktrees").join("feature-unsaved"); + let wt_path = base + .path() + .join("worktrees") + .join("feature") + .join("unsaved"); let req = CreateSessionRequest { cwd: base.path().to_path_buf(), @@ -278,7 +282,7 @@ async fn spawn_new_branch_creates_worktree_and_runs_in_it() { let base = TempDir::new().unwrap(); init_repo(base.path()); - let wt_path = base.path().join("worktrees").join("feature-fresh"); + let wt_path = base.path().join("worktrees").join("feature").join("fresh"); let s = client .spawn_session(&worktree_req(base.path(), "feature/fresh", true)) @@ -343,13 +347,20 @@ async fn second_spawn_on_same_branch_is_rejected() { } // Exactly one registration, one directory, still usable. + // Count branch records only: the worktree path now contains the branch + // name too (`worktrees/feature/dup`), so a substring match over every line + // would count the same tree twice. let holders = git_worktree_list(base.path()) .iter() - .filter(|w| w.contains("feature/dup")) + .filter(|w| w.as_str() == "branch refs/heads/feature/dup") .count(); assert_eq!(holders, 1, "duplicate spawn must not register another tree"); assert!( - base.path().join("worktrees").join("feature-dup").is_dir(), + base.path() + .join("worktrees") + .join("feature") + .join("dup") + .is_dir(), "original worktree must be untouched" ); @@ -384,7 +395,7 @@ async fn spawn_from_linked_worktree_places_in_main_checkout() { .await .expect("spawn from linked worktree"); - let want = base.path().join("worktrees").join("feature-nested"); + let want = base.path().join("worktrees").join("feature").join("nested"); assert_eq!( s.cwd, want, "worktree must be placed under the main checkout" diff --git a/crates/maestro-protocol/src/lib.rs b/crates/maestro-protocol/src/lib.rs index 3bb1284..77979d8 100644 --- a/crates/maestro-protocol/src/lib.rs +++ b/crates/maestro-protocol/src/lib.rs @@ -53,6 +53,14 @@ pub struct Session { pub env: HashMap, #[serde(default)] pub metadata: HashMap, + /// The worktree this session owns, when it was spawned with one. + /// + /// Persisted rather than kept in memory only: after a daemon restart the + /// recovered session must still know what to clean up, and with + /// deterministic worktree paths a forgotten association leaves a directory + /// that blocks that branch from ever being spawned again (#54 review). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub worktree: Option, pub status: SessionStatus, pub created_at: DateTime, /// Current foreground command in the session's pane (e.g. `claude`, @@ -127,6 +135,14 @@ fn default_true() -> bool { true } +/// The worktree a running session owns, carried on [`Session`] so it survives +/// a daemon restart. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct SessionWorktree { + pub path: PathBuf, + pub base_repo: PathBuf, +} + /// HTTP `POST /api/sessions` body. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CreateSessionRequest { @@ -410,6 +426,7 @@ mod tests { command: "claude".into(), env: HashMap::from([("FOO".into(), "bar".into())]), metadata: HashMap::from([("name".into(), Value::String("maestro-impl".into()))]), + worktree: None, status: SessionStatus::Running, created_at: DateTime::parse_from_rfc3339("2026-05-24T12:00:00Z") .unwrap() diff --git a/crates/maestro-testing/src/lib.rs b/crates/maestro-testing/src/lib.rs index cce52c5..2706e83 100644 --- a/crates/maestro-testing/src/lib.rs +++ b/crates/maestro-testing/src/lib.rs @@ -36,6 +36,7 @@ pub fn session(id: Uuid, cwd: &str, command: &str) -> Session { command: command.into(), env: HashMap::new(), metadata: HashMap::new(), + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), foreground: None, diff --git a/crates/maestro-tui/src/app.rs b/crates/maestro-tui/src/app.rs index e4b3ac8..1676c5d 100644 --- a/crates/maestro-tui/src/app.rs +++ b/crates/maestro-tui/src/app.rs @@ -55,6 +55,14 @@ pub struct App { help: HelpOverlay, chord: ChordState, daemon: DaemonClient, + /// Results of draft data loads, delivered back to the event loop. + /// + /// These are network calls — `/api/projects` on a cold cache walks all of + /// `$HOME` — and awaiting them inside `perform` froze the whole TUI: no + /// keys, no frames, attached panes stalled, for up to the request timeout + /// (#67 review). They run as detached tasks and post here instead. + draft_tx: tokio::sync::mpsc::UnboundedSender, + draft_rx: tokio::sync::mpsc::UnboundedReceiver, /// Direct HTTP for the one endpoint [`DaemonClient`] doesn't cover yet: /// `GET /api/names` (#64). http: reqwest::Client, @@ -105,6 +113,7 @@ pub enum Effect { impl App { pub fn new(cfg: TuiConfig, catalog: CatalogStore, daemon: DaemonClient) -> Self { + let (draft_tx, draft_rx) = tokio::sync::mpsc::unbounded_channel(); let chord_timeout = cfg.chord_timeout; let help = HelpOverlay { lines: render_help_lines(&cfg), @@ -123,6 +132,8 @@ impl App { chord: ChordState::new(chord_timeout), daemon, http, + draft_tx, + draft_rx, region: RegionMap::default(), last_sent_size: HashMap::new(), clicks: ClickTracker::default(), @@ -144,6 +155,9 @@ impl App { return Ok(()); } } + Some(update) = self.draft_rx.recv() => { + self.apply_draft_update(update).await; + } _ = self.catalog.changed() => { // Render on next loop iteration. } @@ -204,13 +218,13 @@ impl App { Effect::Attach(id) => self.attach(id), Effect::Spawn(req) => self.submit_spawn(req).await, Effect::Kill(id) => self.submit_kill(id).await, - Effect::LoadDraft(tab_idx) => self.load_draft_data(tab_idx).await, + Effect::LoadDraft(tab_idx) => self.spawn_load_draft(tab_idx), Effect::FastForward => self.fast_forward_spawn().await, - Effect::RefillNames(tab_idx) => self.refill_names(tab_idx).await, + Effect::RefillNames(tab_idx) => self.spawn_refill_names(tab_idx), Effect::ProbeGitRoot { tab, seq, path } => { - self.probe_git_root(tab, seq, path).await + self.spawn_probe_git_root(tab, seq, path) } - Effect::LoadBranches(tab) => self.load_branches(tab).await, + Effect::LoadBranches(tab) => self.spawn_load_branches(tab), Effect::Forward { id, bytes } => { if let Some(handle) = self.emulators.get(id) { let _ = handle.tx.send(SessionInput::Bytes(bytes)).await; @@ -410,28 +424,130 @@ impl App { /// to the built-ins already in place), projects from `/api/projects`, and /// a first name batch. Applies only if that tab still shows the draft — /// a slow response must never clobber what replaced it. - async fn load_draft_data(&mut self, tab_idx: usize) { - let (info, projects) = tokio::join!(self.daemon.daemon_info(), self.daemon.list_projects()); - let names = self.fetch_names(crate::draft::NAME_BATCH_SIZE).await; + /// Kick off the draft's initial data load without blocking the loop. + fn spawn_load_draft(&self, tab: usize) { + let (daemon, http, base, tx) = self.loader_handles(); + tokio::spawn(async move { + let (info, projects) = tokio::join!(daemon.daemon_info(), daemon.list_projects()); + let names = fetch_names(&http, &base, crate::draft::NAME_BATCH_SIZE).await; + let _ = tx.send(DraftUpdate::Data { + tab, + info: info.ok(), + projects: projects.ok(), + names, + }); + }); + } + + fn spawn_refill_names(&self, tab: usize) { + let (_, http, base, tx) = self.loader_handles(); + tokio::spawn(async move { + let names = fetch_names(&http, &base, crate::draft::NAME_BATCH_SIZE).await; + let _ = tx.send(DraftUpdate::Names { tab, names }); + }); + } + + fn spawn_probe_git_root(&self, tab: usize, seq: u32, path: PathBuf) { + let (daemon, _, _, tx) = self.loader_handles(); + tokio::spawn(async move { + let root = daemon + .git_root(&path.to_string_lossy()) + .await + .ok() + .and_then(|g| g.root); + let _ = tx.send(DraftUpdate::GitRoot { tab, seq, root }); + }); + } + + fn spawn_load_branches(&self, tab: usize) { + let Some(cwd) = self + .state + .tabs + .get(tab) + .and_then(|t| t.pane.as_ref()) + .and_then(|p| match p { + PaneRef::Draft(d) => d.cwd(), + _ => None, + }) + else { + return; + }; + let (_, http, base, tx) = self.loader_handles(); + tokio::spawn(async move { + let branches = fetch_branches(&http, &base, &cwd).await; + let _ = tx.send(DraftUpdate::Branches { tab, cwd, branches }); + }); + } + + fn loader_handles( + &self, + ) -> ( + DaemonClient, + reqwest::Client, + String, + tokio::sync::mpsc::UnboundedSender, + ) { + ( + self.daemon.clone(), + self.http.clone(), + self.daemon.base().to_string(), + self.draft_tx.clone(), + ) + } + + /// Fold a finished background load into the draft that asked for it. + async fn apply_draft_update(&mut self, update: DraftUpdate) { + let tab_idx = match &update { + DraftUpdate::Data { tab, .. } + | DraftUpdate::Names { tab, .. } + | DraftUpdate::GitRoot { tab, .. } + | DraftUpdate::Branches { tab, .. } => *tab, + }; let Some(PaneRef::Draft(draft)) = self .state .tabs .get_mut(tab_idx) .and_then(|t| t.pane.as_mut()) else { + // The pane went away while the request was in flight. return; }; - if let Ok(info) = info { - draft.home_dir.clone_from(&info.home_dir); - if !info.presets.is_empty() { - draft.set_presets(info.presets); + let mut follow_up_branches = false; + match update { + DraftUpdate::Data { + info, + projects, + names, + .. + } => { + if let Some(info) = info { + draft.home_dir.clone_from(&info.home_dir); + if !info.presets.is_empty() { + draft.set_presets(info.presets); + } + } + if let Some(projects) = projects { + draft.set_projects(projects, Utc::now()); + } + if !names.is_empty() { + draft.seed_names(names); + } + } + DraftUpdate::Names { names, .. } => { + if !names.is_empty() { + draft.extend_names(names); + } + } + DraftUpdate::GitRoot { seq, root, .. } => { + draft.resolve_gitroot(seq, root); + follow_up_branches = draft.take_branch_load().is_some(); + } + DraftUpdate::Branches { cwd, branches, .. } => { + draft.set_branches(&cwd, branches); } } - if let Ok(projects) = projects { - draft.set_projects(projects, Utc::now()); - } - if !names.is_empty() { - draft.seed_names(names); + if follow_up_branches { + self.spawn_load_branches(tab_idx); } } @@ -475,88 +591,6 @@ impl App { draft.seed_names(names); } - /// Fetch a fresh name batch after a reroll drained the local queue. - async fn refill_names(&mut self, tab_idx: usize) { - let names = self.fetch_names(crate::draft::NAME_BATCH_SIZE).await; - let Some(PaneRef::Draft(draft)) = self - .state - .tabs - .get_mut(tab_idx) - .and_then(|t| t.pane.as_mut()) - else { - return; - }; - if !names.is_empty() { - draft.extend_names(names); - } - } - - /// Resolve the draft target's enclosing repository via `/api/gitroot` - /// (#65). A miss unticks the worktree toggle inside `resolve_gitroot`; - /// a hit may schedule a branch load (Existing mode), served inline. - async fn probe_git_root(&mut self, tab_idx: usize, seq: u32, path: PathBuf) { - let root = self - .daemon - .git_root(&path.to_string_lossy()) - .await - .ok() - .and_then(|g| g.root); - let Some(PaneRef::Draft(draft)) = self - .state - .tabs - .get_mut(tab_idx) - .and_then(|t| t.pane.as_mut()) - else { - return; - }; - draft.resolve_gitroot(seq, root); - if draft.take_branch_load().is_some() { - self.load_branches(tab_idx).await; - } - } - - /// One `/api/branches` batch for the draft's current cwd (#65). Same - /// reqwest route as [`Self::fetch_names`]: no typed client call exists. - async fn load_branches(&mut self, tab_idx: usize) { - let Some(path) = self - .state - .tabs - .get(tab_idx) - .and_then(|t| t.pane.as_ref()) - .and_then(|p| match p { - PaneRef::Draft(d) => d.cwd(), - _ => None, - }) - else { - return; - }; - let url = format!( - "{}{}", - maestro_protocol::routes::normalize_base(self.daemon.base()), - routes::BRANCHES - ); - let resp = self - .http - .get(url) - .query(&[(routes::PATH_QUERY, path.to_string_lossy().as_ref())]) - .send() - .await; - // On failure the picker stays empty; "new" mode still works. - let branches = match resp { - Ok(resp) => resp.json::>().await.unwrap_or_default(), - Err(_) => return, - }; - let Some(PaneRef::Draft(draft)) = self - .state - .tabs - .get_mut(tab_idx) - .and_then(|t| t.pane.as_mut()) - else { - return; - }; - draft.set_branches(&path, branches); - } - async fn submit_spawn(&mut self, req: CreateSessionRequest) { match self.daemon.create_session(&req).await { Ok(session) => { @@ -744,6 +778,76 @@ fn render_help_lines(cfg: &TuiConfig) -> Vec { lines } +/// A completed background load, applied on the event loop. +#[derive(Debug)] +enum DraftUpdate { + Data { + tab: usize, + info: Option, + projects: Option>, + names: Vec, + }, + Names { + tab: usize, + names: Vec, + }, + GitRoot { + tab: usize, + seq: u32, + root: Option, + }, + Branches { + tab: usize, + cwd: PathBuf, + branches: Vec, + }, +} + +/// `GET /api/names?count=N`. Free function: the spawned loaders have no `self`. +async fn fetch_names(http: &reqwest::Client, base: &str, count: usize) -> Vec { + let url = format!( + "{}{}", + maestro_protocol::routes::normalize_base(base), + routes::NAMES + ); + match http + .get(url) + .query(&[(routes::COUNT_QUERY, count)]) + .send() + .await + { + Ok(resp) => resp + .json::() + .await + .map(|batch| batch.names) + .unwrap_or_default(), + Err(_) => Vec::new(), + } +} + +/// `GET /api/branches?path=`. On failure the picker stays empty and "new +/// branch" mode still works. +async fn fetch_branches( + http: &reqwest::Client, + base: &str, + path: &std::path::Path, +) -> Vec { + let url = format!( + "{}{}", + maestro_protocol::routes::normalize_base(base), + routes::BRANCHES + ); + match http + .get(url) + .query(&[(routes::PATH_QUERY, path.to_string_lossy().as_ref())]) + .send() + .await + { + Ok(resp) => resp.json::>().await.unwrap_or_default(), + Err(_) => Vec::new(), + } +} + #[cfg(test)] mod tests { use super::*; @@ -830,6 +934,7 @@ mod tests { cwd: std::path::PathBuf::from("/"), env: Default::default(), metadata: Default::default(), + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), foreground: None, @@ -1220,7 +1325,15 @@ mod tests { let mut app = make_app_at(&base); let effects = app.dispatch(Action::NewDraftPane); assert!(matches!(effects.as_slice(), [Effect::LoadDraft(0)])); + // `perform` only spawns the load — it must not block the event loop on + // the network (#67 review). The result arrives on the draft channel, + // which the run loop drains; do that by hand here. app.perform(effects).await; + let update = tokio::time::timeout(Duration::from_secs(5), app.draft_rx.recv()) + .await + .expect("load completed") + .expect("channel open"); + app.apply_draft_update(update).await; let PaneRef::Draft(draft) = app.state.active_tab().pane.as_ref().unwrap() else { panic!("expected a draft pane"); @@ -1313,6 +1426,7 @@ mod tests { cwd: std::path::PathBuf::from("/"), env: Default::default(), metadata: Default::default(), + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), foreground: None, diff --git a/crates/maestro-tui/src/catalog.rs b/crates/maestro-tui/src/catalog.rs index 2190ee4..32115d6 100644 --- a/crates/maestro-tui/src/catalog.rs +++ b/crates/maestro-tui/src/catalog.rs @@ -188,6 +188,7 @@ mod tests { cwd: std::path::PathBuf::from("/tmp"), env: Default::default(), metadata: Default::default(), + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), foreground: None, diff --git a/crates/maestro-tui/src/draft.rs b/crates/maestro-tui/src/draft.rs index aab9bd8..b20b99c 100644 --- a/crates/maestro-tui/src/draft.rs +++ b/crates/maestro-tui/src/draft.rs @@ -382,7 +382,7 @@ impl DraftPane { self.branches_for = Some(cwd.to_path_buf()); self.branches = branches; self.selected_branch = (0..self.branches.len()) - .find(|&i| self.branches[i].worktree.is_none()) + .find(|&i| Self::branch_is_available(&self.branches[i])) .unwrap_or(0); } @@ -424,8 +424,17 @@ impl DraftPane { self.schedule_branch_load(); } + /// A branch is pickable only when nothing has it checked out. That means + /// no linked worktree *and* not the main checkout's HEAD — git refuses a + /// second checkout either way, and HEAD is commonly `main`, so treating it + /// as available made it the default pick and every spawn failed with + /// "already used by worktree" (#54 review). + fn branch_is_available(b: &BranchInfo) -> bool { + b.worktree.is_none() && !b.is_head + } + fn available_branch(&self, i: usize) -> bool { - self.branches.get(i).is_some_and(|b| b.worktree.is_none()) + self.branches.get(i).is_some_and(Self::branch_is_available) } /// Move the existing-branch highlight, skipping branches another worktree @@ -445,11 +454,11 @@ impl DraftPane { self.selected_branch = available[((cur as i32) + delta).rem_euclid(len) as usize]; } - /// The highlighted branch, only when no other worktree holds it. + /// The highlighted branch, only when nothing has it checked out. pub fn picked_branch(&self) -> Option<&BranchInfo> { self.branches .get(self.selected_branch) - .filter(|b| b.worktree.is_none()) + .filter(|b| Self::branch_is_available(b)) } pub fn is_custom(&self) -> bool { @@ -1781,15 +1790,16 @@ mod tests { ], ); assert_eq!( - draft.branches[draft.selected_branch].name, "main", - "selection lands on an available branch" + draft.branches[draft.selected_branch].name, "feature", + "selection skips `main`: the main checkout has it, so git would \ + refuse a second checkout" ); draft.focused_field = DraftField::BranchExisting; draft.on_input(&InputKey::Down); assert_eq!( draft.branches[draft.selected_branch].name, "feature", - "moving skips branches another worktree holds" + "the only pickable branch stays selected" ); let wt = draft.build_request().unwrap().worktree.unwrap(); diff --git a/crates/maestro-tui/src/sidebar.rs b/crates/maestro-tui/src/sidebar.rs index d17c12e..fa60e90 100644 --- a/crates/maestro-tui/src/sidebar.rs +++ b/crates/maestro-tui/src/sidebar.rs @@ -92,6 +92,7 @@ mod tests { cwd: std::path::PathBuf::from("/tmp"), env: Default::default(), metadata, + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), foreground: None, @@ -179,6 +180,7 @@ mod tests { cwd: std::path::PathBuf::from("/tmp"), env: Default::default(), metadata: HashMap::new(), + worktree: None, status: SessionStatus::Running, created_at: Utc::now(), foreground: None, diff --git a/web/src/fastForward.test.ts b/web/src/fastForward.test.ts index 4acd661..7e46408 100644 --- a/web/src/fastForward.test.ts +++ b/web/src/fastForward.test.ts @@ -85,9 +85,12 @@ describe("resolveFastForward (#67)", () => { }); }); - it("seeds worktree off when not even a project resolved", () => { + it("leaves the worktree default alone when not even a project resolved", () => { + // Not `false`: the form defaults the toggle on and a false would stick, + // so the fallback draft would disagree with a normally-opened draft and + // with the TUI's fast path. const seed = fastForwardSeed(resolveFastForward([], presets)); - expect(seed.worktreeEnabled).toBe(false); + expect(seed.worktreeEnabled).toBeUndefined(); expect(seed.cwd).toBe(""); }); }); diff --git a/web/src/fastForward.ts b/web/src/fastForward.ts index fe05dd5..8ac91fb 100644 --- a/web/src/fastForward.ts +++ b/web/src/fastForward.ts @@ -55,7 +55,11 @@ export function fastForwardSeed(res: FastForwardResolution) { cwd: res.project?.path ?? "", command: res.command ?? "", name: res.name ?? "", - worktreeEnabled: Boolean(res.project), + // Left undefined rather than false when nothing resolved: the form + // defaults the toggle on, and `false` would stick, so the fallback draft + // would disagree with both a normally-opened draft and the TUI's fast path + // (#67 review). + worktreeEnabled: res.project ? true : undefined, worktreeBranch: res.project && res.name ? res.name : undefined, }; diff --git a/web/src/main.ts b/web/src/main.ts index d393b84..00e664d 100644 --- a/web/src/main.ts +++ b/web/src/main.ts @@ -303,7 +303,12 @@ async function requestFastForward(): Promise { const req = buildFastForwardRequest(resolved); if (req) { try { - await createSession(req); + const session = await createSession(req); + // Mount it, the way the TUI's fast path attaches. Without this the + // keystroke creates a session and leaves the UI untouched, so it reads + // as a dead key (#67 review). + mv.openSession(session.id); + updateShellRoute(); return; } catch { // A failed POST degrades to the same pre-filled draft as an diff --git a/web/src/spawnForm.ts b/web/src/spawnForm.ts index b64fd0d..8e365c6 100644 --- a/web/src/spawnForm.ts +++ b/web/src/spawnForm.ts @@ -634,11 +634,14 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { return; } for (const b of branches) { - // A branch another worktree holds stays visible but unpickable — git - // would refuse to check it out twice; the path says who holds it. - const o = opt(b.worktree ? `${b.name} — held by ${b.worktree}` : b.name, b.name); - o.disabled = Boolean(b.worktree); - o.title = b.worktree ?? ""; + // A branch that is already checked out anywhere stays visible but + // unpickable — git refuses a second checkout. That includes the main + // checkout's own HEAD, which is commonly `main` and would otherwise be + // the default pick, making every existing-branch spawn fail (#54 review). + const held = b.worktree ?? (b.is_head ? "the main checkout" : null); + const o = opt(held ? `${b.name} — held by ${held}` : b.name, b.name); + o.disabled = held !== null; + o.title = held ?? ""; branchSelect.appendChild(o); } }