fix(maestro): fourth-review findings across daemon, tui and web
The existing-branch picker could never succeed in either client: both treated the main checkout's HEAD as available, and since that is usually `main` it became the default pick — git then refuses the second checkout. A TUI test had baked the bug in by asserting the selection lands on the `is_head` entry. Both pickers now treat "checked out anywhere" as held. Worktree paths were still not injective. Flattening `/` and `.` to `-` made `feat/x`, `feat.x` and `feat-x` share one directory, and the override root keyed on the repo basename alone, so `~/a/maestro` and `~/b/maestro` collided. A branch name is already a valid relative path — git stores it as refs/heads/feat/example — so it is used verbatim and nests under `worktrees/feat/`; the override root gains a digest of the full repo path. Git forbids holding both `feat` and `feat/x`, so the nesting cannot collide for valid refs. Also: - Recovered sessions dropped their worktree association, so after a restart a kill cleaned up nothing and the leftover directory blocked that branch forever. It is persisted on Session now. - The nesting guard compared git's resolved paths against an unresolved one, so it silently passed on any symlinked repo path — and that guard is what stands between a kill and an inner tree's uncommitted work. - Draft loads ran on the TUI event loop: /api/projects on a cold cache walks all of $HOME, freezing keys, frames and attached panes for up to the request timeout. They are detached tasks posting to a channel now. - Fast-forward in the web client created the session and never mounted it, so `N` looked like a dead key while the TUI attached. - The fast-forward fallback seed forced the worktree toggle off, against both a normal draft and the TUI. - The projects background refresh honoured the TTL, so two of every three ticks were no-ops and requests paid for the rescan instead.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -262,10 +262,16 @@ impl ProjectScanner {
|
||||
|
||||
/// Cached snapshot, rescanning (off-thread) when the TTL has lapsed.
|
||||
pub async fn repos(&self) -> std::sync::Arc<Vec<DiscoveredRepo>> {
|
||||
self.repos_inner(false).await
|
||||
}
|
||||
|
||||
async fn repos_inner(&self, force: bool) -> std::sync::Arc<Vec<DiscoveredRepo>> {
|
||||
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
|
||||
|
||||
@@ -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,
|
||||
},
|
||||
);
|
||||
|
||||
@@ -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<PathBuf> {
|
||||
.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<PathBuf> {
|
||||
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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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.
|
||||
// <root>/<repo basename>-<digest>/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:?}");
|
||||
}
|
||||
|
||||
@@ -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 <repo>/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"
|
||||
|
||||
@@ -53,6 +53,14 @@ pub struct Session {
|
||||
pub env: HashMap<String, String>,
|
||||
#[serde(default)]
|
||||
pub metadata: HashMap<String, Value>,
|
||||
/// 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<SessionWorktree>,
|
||||
pub status: SessionStatus,
|
||||
pub created_at: DateTime<Utc>,
|
||||
/// 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()
|
||||
|
||||
@@ -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,
|
||||
|
||||
+212
-98
@@ -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<DraftUpdate>,
|
||||
draft_rx: tokio::sync::mpsc::UnboundedReceiver<DraftUpdate>,
|
||||
/// 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<DraftUpdate>,
|
||||
) {
|
||||
(
|
||||
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::<Vec<BranchInfo>>().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<String> {
|
||||
lines
|
||||
}
|
||||
|
||||
/// A completed background load, applied on the event loop.
|
||||
#[derive(Debug)]
|
||||
enum DraftUpdate {
|
||||
Data {
|
||||
tab: usize,
|
||||
info: Option<maestro_protocol::DaemonInfo>,
|
||||
projects: Option<Vec<maestro_protocol::Project>>,
|
||||
names: Vec<String>,
|
||||
},
|
||||
Names {
|
||||
tab: usize,
|
||||
names: Vec<String>,
|
||||
},
|
||||
GitRoot {
|
||||
tab: usize,
|
||||
seq: u32,
|
||||
root: Option<PathBuf>,
|
||||
},
|
||||
Branches {
|
||||
tab: usize,
|
||||
cwd: PathBuf,
|
||||
branches: Vec<BranchInfo>,
|
||||
},
|
||||
}
|
||||
|
||||
/// `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<String> {
|
||||
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::<NameBatch>()
|
||||
.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<BranchInfo> {
|
||||
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::<Vec<BranchInfo>>().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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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("");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
+6
-1
@@ -303,7 +303,12 @@ async function requestFastForward(): Promise<void> {
|
||||
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
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user