From d713d0638e1f9fb1b06092f4e87fd42be87d531e Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 13 Aug 2026 16:15:31 +1000 Subject: [PATCH 1/6] feat(branches): move a branch to a different project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a "Move to Project…" item to the branch card's `…` menu. It opens a searchable project picker and re-parents the branch — with its notes, commits, reviews, sessions and images — into the chosen project. No schema change is needed: every branch-scoped table references the branch through `branch_id` and follows it for free. Only three rows carry a `project_id` of their own, so `store::move_branch_to_project` rewrites them in one transaction — the branch, its `project_repos` row, its `workdirs` row — plus `images.project_id` and the `sessions.working_dir` snapshots rooted under the old worktree. `UPDATE`s only: the `AFTER DELETE` triggers GC sessions, so delete-and-reinsert would destroy transcripts. The `project_repos` row is N:1, so it travels only when the moved branch is the last one on it; otherwise a clone lands in the destination and the siblings keep theirs. A branch with a NULL `project_repo_id` gets a row materialized rather than carried across, where the `resolve_branch_repo_slug` fallback would resolve to the *destination's* primary repo. Both projects then re-elect a primary and re-sync their denormalized `github_repo`, following `remove_project_repo`. On disk, the worktree relocates through a new `git worktree move` wrapper — a plain rename would leave the gitfile and the repo's `gitdir` pointer dangling — from wherever `workdirs.path` says it is, so legacy-layout worktrees move correctly. A failed transaction moves it back. Image files follow per-entry and tolerantly; the diff cache is dropped and rebuilds lazily. Preconditions are checked before anything mutates: both projects must be local (remote branches share one Blox workspace per project), the destination must not already have the branch's repo + subpath, and no session may be running on the branch. The dialog states the first two as a disabled Move button with the reason, using the same NULL-vs-empty subpath key as `idx_project_repos_unique`, and waits on the target's lazily-hydrated repos before trusting the duplicate check. Verified with `just ci`: fmt, clippy, svelte-check, 728 Rust and 644 frontend tests. Co-Authored-By: Claude Opus 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/branches.rs | 510 ++++++++++++++++ apps/staged/src-tauri/src/git/mod.rs | 11 +- apps/staged/src-tauri/src/git/worktree.rs | 55 ++ apps/staged/src-tauri/src/lib.rs | 1 + .../staged/src-tauri/src/store/branch_move.rs | 574 ++++++++++++++++++ apps/staged/src-tauri/src/store/images.rs | 15 + apps/staged/src-tauri/src/store/mod.rs | 2 + apps/staged/src-tauri/src/web_server.rs | 14 + apps/staged/src/lib/commands.ts | 18 + .../lib/features/branches/BranchCard.svelte | 3 + .../branches/BranchCardActionsBar.svelte | 16 + .../features/branches/MoveBranchDialog.svelte | 311 ++++++++++ .../branches/moveBranchTarget.test.ts | 165 +++++ .../lib/features/branches/moveBranchTarget.ts | 94 +++ .../lib/features/projects/ProjectHome.svelte | 24 + .../features/projects/ProjectSection.svelte | 3 + 16 files changed, 1811 insertions(+), 5 deletions(-) create mode 100644 apps/staged/src-tauri/src/store/branch_move.rs create mode 100644 apps/staged/src/lib/features/branches/MoveBranchDialog.svelte create mode 100644 apps/staged/src/lib/features/branches/moveBranchTarget.test.ts create mode 100644 apps/staged/src/lib/features/branches/moveBranchTarget.ts diff --git a/apps/staged/src-tauri/src/branches.rs b/apps/staged/src-tauri/src/branches.rs index 1bca3742b..d23988cfe 100644 --- a/apps/staged/src-tauri/src/branches.rs +++ b/apps/staged/src-tauri/src/branches.rs @@ -2353,6 +2353,320 @@ pub async fn delete_branch( store.delete_branch(&branch_id).map_err(|e| e.to_string()) } +/// Move a branch into another project, taking its notes, commits, reviews, +/// sessions and images with it. +#[tauri::command(rename_all = "camelCase")] +pub async fn move_branch( + store: tauri::State<'_, Mutex>>>, + executor: tauri::State<'_, Arc>, + registry: tauri::State<'_, Arc>, + branch_id: String, + target_project_id: String, +) -> Result { + let store = get_store(&store)?; + move_branch_impl(&store, &executor, ®istry, &branch_id, &target_project_id).await +} + +/// The `(github_repo, subpath)` identity of a repo inside a project, matching +/// `idx_project_repos_unique`'s `(project_id, github_repo, COALESCE(subpath, +/// ''))`: a NULL subpath and an empty one are the same repo. +fn repo_subpath_key(github_repo: &str, subpath: Option<&str>) -> String { + format!("{github_repo}\u{0}{}", subpath.unwrap_or("")) +} + +fn describe_repo(github_repo: &str, subpath: Option<&str>) -> String { + match subpath.filter(|s| !s.is_empty()) { + Some(subpath) => format!("{github_repo} ({subpath})"), + None => github_repo.to_string(), + } +} + +/// A validated branch move: what to rewrite, and where the worktree has to end +/// up, resolved against the database before anything is mutated. +#[derive(Debug)] +struct BranchMovePlan { + source_project: store::Project, + /// The branch's repo slug, resolved through `project_repos` or the source + /// project's primary. Names the local clone the worktree belongs to. + github_repo: String, + mv: store::BranchMove, +} + +/// Check every precondition on a move and resolve what it will rewrite. +/// +/// Split out from [`move_branch_impl`] because this is the whole of the move's +/// decision-making — which `project_repos` row travels, which is cloned, what +/// counts as a destination that already has the repo — and it needs nothing but +/// the store to run. +fn plan_branch_move( + store: &Arc, + branch_id: &str, + target_project_id: &str, +) -> Result { + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + if branch.project_id == target_project_id { + return Err("This branch is already in that project".to_string()); + } + + let source_project = store + .get_project(&branch.project_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Project not found: {}", branch.project_id))?; + let target_project = store + .get_project(target_project_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Project not found: {target_project_id}"))?; + + // Remote branches of a project share one Blox workspace, so moving one out + // would mean cross-workspace surgery on a filesystem we don't own. + for project in [&source_project, &target_project] { + if project.location == store::ProjectLocation::Remote { + return Err(format!( + "Project '{}' runs on a remote workspace; branches can only move between local projects", + project.name + )); + } + } + if branch.branch_type == store::BranchType::Remote { + return Err("Remote branches can't be moved between projects".to_string()); + } + + // The move pulls the worktree out from under anything running in it. + if store + .has_running_session_for_branch(branch_id) + .map_err(|e| e.to_string())? + { + return Err("A session is running on this branch — wait for the running session to finish before moving it".to_string()); + } + + let source_repo = match &branch.project_repo_id { + Some(repo_id) => store.get_project_repo(repo_id).map_err(|e| e.to_string())?, + None => None, + }; + // Mirrors `resolve_branch_repo_slug`: a branch with no repo link falls back + // to its project's primary repo. + let (github_repo, subpath) = match &source_repo { + Some(repo) => (repo.github_repo.clone(), repo.subpath.clone()), + None => ( + project_primary_repo(&source_project)?.to_string(), + source_project.subpath.clone(), + ), + }; + + let key = repo_subpath_key(&github_repo, subpath.as_deref()); + if store + .list_project_repos(target_project_id) + .map_err(|e| e.to_string())? + .iter() + .any(|repo| repo_subpath_key(&repo.github_repo, repo.subpath.as_deref()) == key) + { + return Err(format!( + "'{}' already has {} attached", + target_project.name, + describe_repo(&github_repo, subpath.as_deref()) + )); + } + + // A `project_repos` row can be shared by several branches, so it only + // travels when this branch is the last one on it. + let placement = match &source_repo { + Some(repo) => { + let shared = store + .list_branches_for_project(&source_project.id) + .map_err(|e| e.to_string())? + .into_iter() + .any(|b| { + b.id != branch.id && b.project_repo_id.as_deref() == Some(repo.id.as_str()) + }); + if shared { + let mut clone = store::ProjectRepo::new( + target_project_id, + &repo.github_repo, + &repo.branch_name, + repo.subpath.clone(), + ); + clone.reason = repo.reason.clone(); + clone.head_repo = repo.head_repo.clone(); + store::RepoPlacement::Clone(clone) + } else { + store::RepoPlacement::Reparent { + repo_id: repo.id.clone(), + } + } + } + // Legacy branch with no repo link: materialize a row rather than carry + // the NULL across, which `resolve_branch_repo_slug` would resolve to the + // *destination's* primary repo — a wrong-repo read. + None => store::RepoPlacement::Clone(store::ProjectRepo::new( + target_project_id, + &github_repo, + &branch.branch_name, + subpath.clone(), + )), + }; + + // `workdirs.path` is the source of truth for where the worktree actually + // is — some branches still sit in the legacy non-project-scoped layout. + let workdir = match store + .get_workdir_for_branch(branch_id) + .map_err(|e| e.to_string())? + { + Some(wd) => { + let new_path = git::project_worktree_path_for( + target_project_id, + &github_repo, + &branch.branch_name, + ) + .map_err(|e| e.to_string())?; + Some(store::WorkdirMove { + workdir_id: wd.id, + old_path: wd.path, + new_path: new_path.to_string_lossy().to_string(), + }) + } + None => None, + }; + + Ok(BranchMovePlan { + mv: store::BranchMove { + branch_id: branch_id.to_string(), + source_project_id: source_project.id.clone(), + target_project_id: target_project_id.to_string(), + repo: placement, + workdir, + }, + source_project, + github_repo, + }) +} + +/// The body of [`move_branch`], shared with the web router. +/// +/// The move runs in three stages that have to agree with each other: the +/// worktree relocation on disk, the re-parent transaction, and the image files. +/// A failed transaction puts the worktree back. +pub(crate) async fn move_branch_impl( + store: &Arc, + executor: &ActionExecutor, + registry: &ActionRegistry, + branch_id: &str, + target_project_id: &str, +) -> Result { + let BranchMovePlan { + source_project, + github_repo, + mv, + } = plan_branch_move(store, branch_id, target_project_id)?; + + crate::actions::commands::stop_actions_for_branches(executor, registry, &[branch_id]); + + let repo_path = crate::paths::repos_dir() + .map(|d| d.join(&github_repo)) + .ok_or("Cannot determine clone path")?; + let workdir_move = mv.workdir.clone(); + + // A branch whose setup never finished has nothing on disk to relocate. + let mut moved_on_disk = false; + if let Some(wd) = &workdir_move { + let old_path = PathBuf::from(&wd.old_path); + let new_path = PathBuf::from(&wd.new_path); + if old_path.exists() { + let repo_path = repo_path.clone(); + tauri::async_runtime::spawn_blocking(move || { + git::move_worktree(&repo_path, &old_path, &new_path) + }) + .await + .map_err(|e| format!("Failed to move worktree: {e}"))? + .map_err(|e| format!("Failed to move worktree: {e}"))?; + moved_on_disk = true; + } + } + + if let Err(e) = store.move_branch_to_project(&mv) { + // Disk and database have to agree, so undo the half that landed. + if let (true, Some(wd)) = (moved_on_disk, &workdir_move) { + if let Err(revert) = + git::move_worktree(&repo_path, Path::new(&wd.new_path), Path::new(&wd.old_path)) + { + log::warn!( + "Failed to move worktree for branch {branch_id} back to {}: {revert}", + wd.old_path + ); + } + } + return Err(e.to_string()); + } + + move_branch_image_files(store, branch_id, &source_project.id, target_project_id); + + // Cheaper to drop than to move, and it rebuilds on the next diff read. + if let Err(e) = crate::diff_cache::delete_branch_cache( + source_project.location, + &source_project.id, + branch_id, + ) { + log::warn!("Failed to clear diff cache for moved branch {branch_id}: {e}"); + } + + let updated = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + let workdir = store + .get_workdir_for_branch(branch_id) + .map_err(|e| e.to_string())? + .map(|w| w.path); + Ok(to_branch_with_workdir(updated, workdir)) +} + +/// Relocate a moved branch's image files into the destination project's +/// `images/` directory. +/// +/// Tolerant per entry, like [`crate::paths::migrate_directory_contents`]: a file +/// that won't move costs one broken attachment, not the whole move — and the +/// rows already point at the destination by the time this runs. +fn move_branch_image_files( + store: &Arc, + branch_id: &str, + source_project_id: &str, + target_project_id: &str, +) { + let images = match store.list_all_images_for_branch(branch_id) { + Ok(images) => images, + Err(e) => { + log::warn!("Cannot list images for moved branch {branch_id}: {e}"); + return; + } + }; + + for image in images { + let from = store::images::image_file_path(source_project_id, &image.id, &image.filename); + let to = store::images::image_file_path(target_project_id, &image.id, &image.filename); + let (Ok(from), Ok(to)) = (from, to) else { + continue; + }; + if !from.exists() { + continue; + } + if let Some(parent) = to.parent() { + if let Err(e) = std::fs::create_dir_all(parent) { + log::warn!("Cannot create image directory {}: {e}", parent.display()); + continue; + } + } + if let Err(e) = std::fs::rename(&from, &to) { + log::warn!( + "Failed to move image {} -> {}: {e}", + from.display(), + to.display() + ); + } + } +} + #[tauri::command(rename_all = "camelCase")] pub async fn rename_branch( store: tauri::State<'_, Mutex>>>, @@ -2991,4 +3305,200 @@ mod tests { "expected missing remote ref error, got: {err}" ); } + + // ── plan_branch_move ──────────────────────────────────────────────────── + + struct MoveFixture { + store: Arc, + source: store::Project, + target: store::Project, + repo: store::ProjectRepo, + branch: store::Branch, + } + + fn move_fixture() -> MoveFixture { + let store = Arc::new(Store::in_memory().unwrap()); + let source = store::Project::named("source").with_primary_repo("acme/widgets"); + let target = store::Project::named("target"); + store.create_project(&source).unwrap(); + store.create_project(&target).unwrap(); + + let repo = store::ProjectRepo::new(&source.id, "acme/widgets", "feature", None).primary(); + store.create_project_repo(&repo).unwrap(); + let branch = + store::Branch::new(&source.id, "feature", "origin/main").with_project_repo(&repo.id); + store.create_branch(&branch).unwrap(); + + MoveFixture { + store, + source, + target, + repo, + branch, + } + } + + #[test] + fn plan_move_carries_a_sole_branch_repo_row_across() { + let f = move_fixture(); + + let plan = plan_branch_move(&f.store, &f.branch.id, &f.target.id).unwrap(); + + match &plan.mv.repo { + store::RepoPlacement::Reparent { repo_id } => assert_eq!(repo_id, &f.repo.id), + other => panic!("expected the repo row to travel, got {other:?}"), + } + assert_eq!(plan.github_repo, "acme/widgets"); + // Nothing on disk yet, so there is no worktree to relocate. + assert!(plan.mv.workdir.is_none()); + } + + #[test] + fn plan_move_clones_a_repo_row_a_sibling_branch_shares() { + let f = move_fixture(); + let sibling = + store::Branch::new(&f.source.id, "other", "origin/main").with_project_repo(&f.repo.id); + f.store.create_branch(&sibling).unwrap(); + + let plan = plan_branch_move(&f.store, &f.branch.id, &f.target.id).unwrap(); + + match plan.mv.repo { + store::RepoPlacement::Clone(repo) => { + assert_ne!(repo.id, f.repo.id); + assert_eq!(repo.github_repo, "acme/widgets"); + assert_eq!(repo.project_id, f.target.id); + } + other => panic!("expected a cloned repo row, got {other:?}"), + } + } + + /// A NULL `project_repo_id` would resolve to the *destination's* primary + /// repo after the move, so the plan materializes a row instead. + #[test] + fn plan_move_materializes_a_row_for_a_branch_with_no_repo_link() { + let f = move_fixture(); + let legacy = store::Branch::new(&f.source.id, "legacy", "origin/main"); + f.store.create_branch(&legacy).unwrap(); + + let plan = plan_branch_move(&f.store, &legacy.id, &f.target.id).unwrap(); + + match plan.mv.repo { + store::RepoPlacement::Clone(repo) => { + assert_eq!(repo.github_repo, "acme/widgets"); + assert_eq!(repo.branch_name, "legacy"); + } + other => panic!("expected a materialized repo row, got {other:?}"), + } + } + + #[test] + fn plan_move_relocates_the_worktree_into_the_destination_project() { + let f = move_fixture(); + let workdir = store::Workdir::new(&f.source.id, "/wt/old/path").with_branch(&f.branch.id); + f.store.create_workdir(&workdir).unwrap(); + + let plan = plan_branch_move(&f.store, &f.branch.id, &f.target.id).unwrap(); + + let wd = plan.mv.workdir.expect("worktree should be relocated"); + assert_eq!(wd.workdir_id, workdir.id); + // The old path comes from the row, not a recomputed one — legacy-layout + // worktrees move from wherever they actually are. + assert_eq!(wd.old_path, "/wt/old/path"); + let expected = git::project_worktree_path_for(&f.target.id, "acme/widgets", "feature") + .unwrap() + .to_string_lossy() + .to_string(); + assert_eq!(wd.new_path, expected); + } + + #[test] + fn plan_move_rejects_a_remote_project_on_either_end() { + let f = move_fixture(); + let mut remote = store::Project::named("remote-target"); + remote.location = store::ProjectLocation::Remote; + f.store.create_project(&remote).unwrap(); + + let err = plan_branch_move(&f.store, &f.branch.id, &remote.id).unwrap_err(); + assert!(err.contains("remote workspace"), "unexpected error: {err}"); + + // …and the same when the branch is leaving a remote project. + let remote_repo = + store::ProjectRepo::new(&remote.id, "acme/other", "feature", None).primary(); + f.store.create_project_repo(&remote_repo).unwrap(); + let remote_branch = store::Branch::new(&remote.id, "feature", "origin/main") + .with_project_repo(&remote_repo.id); + f.store.create_branch(&remote_branch).unwrap(); + + let err = plan_branch_move(&f.store, &remote_branch.id, &f.target.id).unwrap_err(); + assert!(err.contains("remote workspace"), "unexpected error: {err}"); + } + + #[test] + fn plan_move_rejects_a_branch_with_a_session_running_on_it() { + let f = move_fixture(); + let session = store::Session::new_running("work", Path::new("/wt/old/path")) + .with_branch(&f.branch.id); + f.store.create_session(&session).unwrap(); + + let err = plan_branch_move(&f.store, &f.branch.id, &f.target.id).unwrap_err(); + + assert!( + err.contains("session is running"), + "unexpected error: {err}" + ); + } + + #[test] + fn plan_move_rejects_a_destination_that_already_has_the_repo() { + let f = move_fixture(); + f.store + .create_project_repo(&store::ProjectRepo::new( + &f.target.id, + "acme/widgets", + "main", + None, + )) + .unwrap(); + + let err = plan_branch_move(&f.store, &f.branch.id, &f.target.id).unwrap_err(); + + assert!( + err.contains("already has acme/widgets"), + "unexpected error: {err}" + ); + } + + /// `idx_project_repos_unique` coalesces the subpath, so a NULL subpath and + /// an empty one are the same repo — the check has to agree. + #[test] + fn plan_move_treats_a_null_and_an_empty_subpath_as_the_same_repo() { + let f = move_fixture(); + f.store + .create_project_repo(&store::ProjectRepo::new( + &f.target.id, + "acme/widgets", + "main", + Some(String::new()), + )) + .unwrap(); + + let err = plan_branch_move(&f.store, &f.branch.id, &f.target.id).unwrap_err(); + + assert!( + err.contains("already has acme/widgets"), + "unexpected error: {err}" + ); + } + + #[test] + fn plan_move_rejects_a_move_into_the_branchs_own_project() { + let f = move_fixture(); + + let err = plan_branch_move(&f.store, &f.branch.id, &f.source.id).unwrap_err(); + + assert!( + err.contains("already in that project"), + "unexpected error: {err}" + ); + } } diff --git a/apps/staged/src-tauri/src/git/mod.rs b/apps/staged/src-tauri/src/git/mod.rs index 23d61166c..a0e8888de 100644 --- a/apps/staged/src-tauri/src/git/mod.rs +++ b/apps/staged/src-tauri/src/git/mod.rs @@ -50,9 +50,10 @@ pub use worktree::{ create_worktree_for_existing_branch_at_path, create_worktree_from_pr, create_worktree_from_pr_at_path, discard_worktree_changes, fetch_pr_head_sha, get_commits_since_base, get_full_commit_log, get_head_sha, get_parent_commit, - has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_branch_commit_line, - parse_worktree_status_paths, project_worktree_path_for, project_worktree_root_for, - remote_branch_exists, remove_worktree, reset_to_commit, set_upstream_to_origin, switch_branch, - update_branch_from_pr, worktree_path_for, BranchCommitFields, CommitInfo, UpdateFromPrResult, - WorktreeChangePaths, BRANCH_COMMIT_LOG_FORMAT, + has_unpushed_commits, list_worktree_change_paths, list_worktrees, move_worktree, + parse_branch_commit_line, parse_worktree_status_paths, project_worktree_path_for, + project_worktree_root_for, remote_branch_exists, remove_worktree, reset_to_commit, + set_upstream_to_origin, switch_branch, update_branch_from_pr, worktree_path_for, + BranchCommitFields, CommitInfo, UpdateFromPrResult, WorktreeChangePaths, + BRANCH_COMMIT_LOG_FORMAT, }; diff --git a/apps/staged/src-tauri/src/git/worktree.rs b/apps/staged/src-tauri/src/git/worktree.rs index a7cf975ad..f1440ae16 100644 --- a/apps/staged/src-tauri/src/git/worktree.rs +++ b/apps/staged/src-tauri/src/git/worktree.rs @@ -184,6 +184,28 @@ pub fn create_worktree_for_existing_branch_at_path( Ok(worktree_path.to_path_buf()) } +/// Move an existing worktree to a new path. +/// +/// Goes through `git worktree move` rather than `fs::rename` because a +/// worktree is two pointers, not one directory: the `.git` gitfile inside the +/// worktree names the repo's admin directory, and that admin directory's +/// `gitdir` file names the worktree. A rename updates neither, leaving both +/// dangling; `git worktree move` rewrites both. +pub fn move_worktree(repo: &Path, from: &Path, to: &Path) -> Result<(), GitError> { + ensure_worktree_parent_exists(to)?; + ensure_worktree_absent(to)?; + + let from_str = from + .to_str() + .ok_or_else(|| GitError::InvalidPath(from.display().to_string()))?; + let to_str = to + .to_str() + .ok_or_else(|| GitError::InvalidPath(to.display().to_string()))?; + + cli::run(repo, &["worktree", "move", from_str, to_str])?; + Ok(()) +} + /// Remove a worktree and its associated branch. /// /// Removes the worktree directory, git worktree reference, and the local git branch. @@ -961,6 +983,39 @@ mod tests { assert!(paths.reset_required); } + /// The reason this goes through `git worktree move`: after the move the + /// worktree still has to be a working tree at its new path, which requires + /// both the gitfile and the repo's `gitdir` pointer to have been rewritten. + #[test] + fn move_worktree_leaves_a_working_tree_at_the_new_path() { + let repo = crate::test_utils::TempGitRepo::new(); + repo.write_file("tracked.txt", "base\n"); + repo.commit("base"); + + repo.run_git(&["branch", "feature"]); + + let name = repo.path().file_name().unwrap().to_str().unwrap(); + let parent = repo.path().parent().unwrap(); + let from = parent.join(format!("{name}-wt-from")); + let to = parent.join(format!("{name}-wt-to")); + create_worktree_for_existing_branch_at_path(repo.path(), "feature", &from).unwrap(); + + move_worktree(repo.path(), &from, &to).unwrap(); + + assert!(!from.exists()); + assert!(to.join("tracked.txt").exists()); + // A dangling gitfile or gitdir pointer fails both of these. + assert!(cli::run(&to, &["status", "--porcelain"]).is_ok()); + assert_eq!( + cli::run(&to, &["rev-parse", "--abbrev-ref", "HEAD"]) + .unwrap() + .trim(), + "feature" + ); + + let _ = std::fs::remove_dir_all(&to); + } + #[test] fn discard_worktree_changes_resets_tracked_and_removes_new_files() { let repo = crate::test_utils::TempGitRepo::new(); diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index cf460f89a..1e7668b5b 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -2229,6 +2229,7 @@ pub fn run() { branches::resume_workspace, branches::delete_branch, branches::rename_branch, + branches::move_branch, branches::get_blox_env, branches::get_workspace_info, branches::poll_workspace_status, diff --git a/apps/staged/src-tauri/src/store/branch_move.rs b/apps/staged/src-tauri/src/store/branch_move.rs new file mode 100644 index 000000000..b5d5a9fc2 --- /dev/null +++ b/apps/staged/src-tauri/src/store/branch_move.rs @@ -0,0 +1,574 @@ +//! Re-parenting a branch into a different project. +//! +//! Everything else a branch owns — commits, notes, reviews, sessions and their +//! messages, comments, reviewed files — hangs off `branch_id` and follows the +//! branch for free. Only four things carry a `project_id` of their own and have +//! to be rewritten in step with it: the branch row, its `project_repos` row, +//! its `workdirs` row, and its images. That rewrite is what lives here, in one +//! transaction, as `UPDATE`s and one `INSERT` only — the `AFTER DELETE` +//! triggers on the artifact tables garbage-collect sessions, so a +//! delete-and-reinsert would take transcripts with it. + +use rusqlite::{params, OptionalExtension}; + +use super::models::ProjectRepo; +use super::{now_timestamp, Store, StoreError}; + +/// Which `project_repos` row the moved branch points at once it lands. +/// +/// The relationship is N:1 — sibling branches can share one row — so the row +/// only travels when the moved branch is the last one on it. +#[derive(Debug, Clone)] +pub enum RepoPlacement { + /// Carry the branch's own row into the destination project. + Reparent { repo_id: String }, + /// Insert this clone in the destination and leave the source row for the + /// siblings still pointing at it. + Clone(ProjectRepo), +} + +impl RepoPlacement { + fn repo_id(&self) -> &str { + match self { + Self::Reparent { repo_id } => repo_id, + Self::Clone(repo) => &repo.id, + } + } +} + +/// The worktree relocation half of a move: which `workdirs` row to rewrite and +/// where to. Absent for a branch whose worktree was never set up. +#[derive(Debug, Clone)] +pub struct WorkdirMove { + pub workdir_id: String, + pub old_path: String, + pub new_path: String, +} + +/// Everything [`Store::move_branch_to_project`] rewrites, resolved by the +/// caller before any of it is applied. +#[derive(Debug, Clone)] +pub struct BranchMove { + pub branch_id: String, + pub source_project_id: String, + pub target_project_id: String, + pub repo: RepoPlacement, + pub workdir: Option, +} + +impl Store { + /// Move a branch into another project, rewriting every `project_id` that + /// points at the old one. + /// + /// The `branches(project_id, project_repo_id, branch_name)` and + /// `workdirs(project_id, path)` unique indexes are the backstop against a + /// destination that already holds this branch, so violations surface as + /// user-readable errors rather than raw SQLite text. + pub fn move_branch_to_project(&self, mv: &BranchMove) -> Result<(), StoreError> { + let mut conn = self.conn.lock().unwrap(); + let tx = conn.transaction()?; + let now = now_timestamp(); + + match &mv.repo { + RepoPlacement::Reparent { repo_id } => { + // Primary is re-elected per project below, so hand the row over + // unelected and let the destination decide. + tx.execute( + "UPDATE project_repos SET project_id = ?1, is_primary = 0, updated_at = ?2 + WHERE id = ?3", + params![mv.target_project_id, now, repo_id], + ) + .map_err(|e| duplicate_repo_error(&e))?; + } + RepoPlacement::Clone(repo) => { + tx.execute( + "INSERT INTO project_repos (id, project_id, github_repo, branch_name, subpath, + is_primary, reason, head_repo, created_at, updated_at) + VALUES (?1, ?2, ?3, ?4, ?5, 0, ?6, ?7, ?8, ?9)", + params![ + repo.id, + mv.target_project_id, + repo.github_repo, + repo.branch_name, + repo.subpath, + repo.reason, + repo.head_repo, + repo.created_at, + now, + ], + ) + .map_err(|e| duplicate_repo_error(&e))?; + } + } + + tx.execute( + "UPDATE branches SET project_id = ?1, project_repo_id = ?2, updated_at = ?3 + WHERE id = ?4", + params![mv.target_project_id, mv.repo.repo_id(), now, mv.branch_id], + ) + .map_err(|e| duplicate_branch_error(&e))?; + + if let Some(wd) = &mv.workdir { + tx.execute( + "UPDATE workdirs SET project_id = ?1, path = ?2, updated_at = ?3 WHERE id = ?4", + params![mv.target_project_id, wd.new_path, now, wd.workdir_id], + ) + .map_err(|e| duplicate_workdir_error(&e))?; + + // `sessions.working_dir` is an absolute-path snapshot taken when the + // session started, so it has to be rewritten by hand. Matching on + // the old worktree path rather than on the branch's artifacts is + // both narrower and wider in the right ways: a path under this + // worktree belongs to this branch and nothing else, and it catches + // sessions whose link to the branch runs through something this + // query would otherwise have to enumerate. The suffix is preserved + // so a session rooted at a repo subpath keeps it. + let old_prefix = format!("{}/", wd.old_path); + tx.execute( + "UPDATE sessions + SET working_dir = ?1 || substr(working_dir, ?2), updated_at = ?3 + WHERE working_dir = ?4 OR instr(working_dir, ?5) = 1", + params![ + wd.new_path, + wd.old_path.chars().count() as i64 + 1, + now, + wd.old_path, + old_prefix, + ], + )?; + } + + // Branch-scoped images only: an image with no `branch_id` belongs to a + // project note and stays where it is. + tx.execute( + "UPDATE images SET project_id = ?1 WHERE branch_id = ?2", + params![mv.target_project_id, mv.branch_id], + )?; + + elect_primary_repo(&tx, &mv.source_project_id, now)?; + elect_primary_repo(&tx, &mv.target_project_id, now)?; + + tx.commit()?; + Ok(()) + } +} + +/// Make sure a project has exactly one primary repo and that +/// `projects.github_repo`/`subpath` still denormalize it. +/// +/// Same shape as `remove_project_repo`'s re-election: the sitting primary keeps +/// the job, a project that just lost its primary promotes its oldest remaining +/// repo, and a project with no repos left denormalizes to NULL. +fn elect_primary_repo( + tx: &rusqlite::Transaction<'_>, + project_id: &str, + now: i64, +) -> Result<(), StoreError> { + let primary: Option<(String, String, Option)> = tx + .query_row( + "SELECT id, github_repo, subpath FROM project_repos + WHERE project_id = ?1 AND is_primary = 1 + ORDER BY created_at ASC LIMIT 1", + params![project_id], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ) + .optional()?; + + let elected = match primary { + Some(existing) => Some(existing), + None => { + let next: Option<(String, String, Option)> = tx + .query_row( + "SELECT id, github_repo, subpath FROM project_repos + WHERE project_id = ?1 ORDER BY created_at ASC LIMIT 1", + params![project_id], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ) + .optional()?; + if let Some((id, _, _)) = &next { + tx.execute( + "UPDATE project_repos SET is_primary = 1, updated_at = ?1 WHERE id = ?2", + params![now, id], + )?; + } + next + } + }; + + match elected { + Some((_, github_repo, subpath)) => tx.execute( + "UPDATE projects SET github_repo = ?1, subpath = ?2, updated_at = ?3 WHERE id = ?4", + params![github_repo, subpath, now, project_id], + )?, + None => tx.execute( + "UPDATE projects SET github_repo = NULL, subpath = NULL, updated_at = ?1 WHERE id = ?2", + params![now, project_id], + )?, + }; + Ok(()) +} + +fn is_unique_violation(err: &rusqlite::Error, needles: &[&str]) -> bool { + match err { + rusqlite::Error::SqliteFailure(_, Some(msg)) => { + needles.iter().any(|needle| msg.contains(needle)) + } + _ => false, + } +} + +fn duplicate_repo_error(err: &rusqlite::Error) -> StoreError { + if is_unique_violation( + err, + &["idx_project_repos_unique", "project_repos.github_repo"], + ) { + return StoreError( + "The destination project already has this repository attached.".to_string(), + ); + } + StoreError(err.to_string()) +} + +fn duplicate_branch_error(err: &rusqlite::Error) -> StoreError { + if is_unique_violation(err, &["branches.branch_name"]) { + return StoreError( + "The destination project already tracks a branch with this name for this repository." + .to_string(), + ); + } + StoreError(err.to_string()) +} + +fn duplicate_workdir_error(err: &rusqlite::Error) -> StoreError { + if is_unique_violation(err, &["workdirs.path"]) { + return StoreError( + "The destination project already has a worktree at that path.".to_string(), + ); + } + StoreError(err.to_string()) +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::*; + use crate::store::models::{Branch, Image, Note, Project, Session, Workdir}; + + struct Fixture { + store: Store, + source: Project, + target: Project, + repo: ProjectRepo, + branch: Branch, + } + + /// A branch on its own `project_repos` row in `source`, plus an empty + /// `target` project to move it into. + fn fixture() -> Fixture { + let store = Store::in_memory().unwrap(); + let source = Project::named("source").with_primary_repo("acme/widgets"); + let target = Project::named("target"); + store.create_project(&source).unwrap(); + store.create_project(&target).unwrap(); + + let repo = ProjectRepo::new(&source.id, "acme/widgets", "feature", None).primary(); + store.create_project_repo(&repo).unwrap(); + + let branch = Branch::new(&source.id, "feature", "origin/main").with_project_repo(&repo.id); + store.create_branch(&branch).unwrap(); + + Fixture { + store, + source, + target, + repo, + branch, + } + } + + fn reparent(f: &Fixture, workdir: Option) -> BranchMove { + BranchMove { + branch_id: f.branch.id.clone(), + source_project_id: f.source.id.clone(), + target_project_id: f.target.id.clone(), + repo: RepoPlacement::Reparent { + repo_id: f.repo.id.clone(), + }, + workdir, + } + } + + #[test] + fn carries_the_branch_its_repo_row_and_its_images() { + let f = fixture(); + let image = Image::new( + Some(&f.branch.id), + &f.source.id, + "shot.png", + "image/png", + 12, + false, + ); + f.store.create_image(&image).unwrap(); + // A project-note image has no branch and must not follow. + let project_image = Image::new(None, &f.source.id, "note.png", "image/png", 12, false); + f.store.create_image(&project_image).unwrap(); + + f.store.move_branch_to_project(&reparent(&f, None)).unwrap(); + + let moved = f.store.get_branch(&f.branch.id).unwrap().unwrap(); + assert_eq!(moved.project_id, f.target.id); + assert_eq!(moved.project_repo_id.as_deref(), Some(f.repo.id.as_str())); + assert_eq!( + f.store + .get_project_repo(&f.repo.id) + .unwrap() + .unwrap() + .project_id, + f.target.id + ); + assert_eq!( + f.store.get_image(&image.id).unwrap().unwrap().project_id, + f.target.id + ); + assert_eq!( + f.store + .get_image(&project_image.id) + .unwrap() + .unwrap() + .project_id, + f.source.id + ); + } + + /// The point of moving via `UPDATE` rather than delete-and-reinsert: the + /// `AFTER DELETE` triggers would have taken the session transcript with it. + #[test] + fn keeps_the_branch_artifacts_and_their_sessions() { + let f = fixture(); + let session = Session::new_running("do the thing", Path::new("/tmp/wt")); + f.store.create_session(&session).unwrap(); + let note = Note::new(&f.branch.id, "Findings", "body").with_session(&session.id); + f.store.create_note(¬e).unwrap(); + + f.store.move_branch_to_project(&reparent(&f, None)).unwrap(); + + assert_eq!( + f.store.list_notes_for_branch(&f.branch.id).unwrap().len(), + 1 + ); + assert!(f.store.get_session(&session.id).unwrap().is_some()); + // The session resolves to the destination through the branch join. + assert_eq!( + f.store + .get_project_id_for_session(&session.id) + .unwrap() + .as_deref(), + Some(f.target.id.as_str()) + ); + } + + #[test] + fn rewrites_the_workdir_and_the_session_working_dirs_under_it() { + let f = fixture(); + let workdir = Workdir::new(&f.source.id, "/wt/source/acme-widgets--feature") + .with_branch(&f.branch.id); + f.store.create_workdir(&workdir).unwrap(); + + let at_root = Session::new_running("root", Path::new("/wt/source/acme-widgets--feature")); + let at_subpath = Session::new_running( + "sub", + Path::new("/wt/source/acme-widgets--feature/apps/web"), + ); + // A sibling path that merely shares the prefix's characters must not move. + let elsewhere = + Session::new_running("other", Path::new("/wt/source/acme-widgets--feature-two")); + for session in [&at_root, &at_subpath, &elsewhere] { + f.store.create_session(session).unwrap(); + } + + f.store + .move_branch_to_project(&reparent( + &f, + Some(WorkdirMove { + workdir_id: workdir.id.clone(), + old_path: workdir.path.clone(), + new_path: "/wt/target/acme-widgets--feature".to_string(), + }), + )) + .unwrap(); + + let moved = f.store.get_workdir(&workdir.id).unwrap().unwrap(); + assert_eq!(moved.project_id, f.target.id); + assert_eq!(moved.path, "/wt/target/acme-widgets--feature"); + + let working_dir = |id: &str| f.store.get_session(id).unwrap().unwrap().working_dir; + assert_eq!(working_dir(&at_root.id), "/wt/target/acme-widgets--feature"); + assert_eq!( + working_dir(&at_subpath.id), + "/wt/target/acme-widgets--feature/apps/web" + ); + assert_eq!( + working_dir(&elsewhere.id), + "/wt/source/acme-widgets--feature-two" + ); + } + + #[test] + fn clones_a_repo_row_that_sibling_branches_still_need() { + let f = fixture(); + let sibling = + Branch::new(&f.source.id, "other", "origin/main").with_project_repo(&f.repo.id); + f.store.create_branch(&sibling).unwrap(); + + let clone = ProjectRepo::new(&f.target.id, "acme/widgets", "feature", None); + f.store + .move_branch_to_project(&BranchMove { + repo: RepoPlacement::Clone(clone.clone()), + ..reparent(&f, None) + }) + .unwrap(); + + // The source row stays behind for the sibling. + let source_repo = f.store.get_project_repo(&f.repo.id).unwrap().unwrap(); + assert_eq!(source_repo.project_id, f.source.id); + assert_eq!( + f.store + .get_branch(&sibling.id) + .unwrap() + .unwrap() + .project_repo_id, + Some(f.repo.id.clone()) + ); + // …and the moved branch points at the clone in the destination. + let moved = f.store.get_branch(&f.branch.id).unwrap().unwrap(); + assert_eq!(moved.project_repo_id.as_deref(), Some(clone.id.as_str())); + assert_eq!( + f.store + .get_project_repo(&clone.id) + .unwrap() + .unwrap() + .project_id, + f.target.id + ); + } + + #[test] + fn promotes_the_arriving_repo_when_the_destination_had_none() { + let f = fixture(); + + f.store.move_branch_to_project(&reparent(&f, None)).unwrap(); + + let target_primary = f + .store + .get_primary_project_repo(&f.target.id) + .unwrap() + .unwrap(); + assert_eq!(target_primary.id, f.repo.id); + assert_eq!( + f.store + .get_project(&f.target.id) + .unwrap() + .unwrap() + .github_repo + .as_deref(), + Some("acme/widgets") + ); + // The source lost its only repo, so it denormalizes to NULL. + let source = f.store.get_project(&f.source.id).unwrap().unwrap(); + assert!(source.github_repo.is_none()); + assert!(f + .store + .get_primary_project_repo(&f.source.id) + .unwrap() + .is_none()); + } + + #[test] + fn leaves_the_destinations_own_primary_in_place() { + let f = fixture(); + let existing = ProjectRepo::new(&f.target.id, "acme/other", "main", None).primary(); + f.store.create_project_repo(&existing).unwrap(); + + f.store.move_branch_to_project(&reparent(&f, None)).unwrap(); + + assert_eq!( + f.store + .get_primary_project_repo(&f.target.id) + .unwrap() + .unwrap() + .id, + existing.id + ); + assert!( + !f.store + .get_project_repo(&f.repo.id) + .unwrap() + .unwrap() + .is_primary + ); + } + + #[test] + fn re_elects_the_sources_next_repo_when_the_primary_leaves() { + let f = fixture(); + let remaining = ProjectRepo::new(&f.source.id, "acme/other", "main", None); + f.store.create_project_repo(&remaining).unwrap(); + + f.store.move_branch_to_project(&reparent(&f, None)).unwrap(); + + assert_eq!( + f.store + .get_primary_project_repo(&f.source.id) + .unwrap() + .unwrap() + .id, + remaining.id + ); + assert_eq!( + f.store + .get_project(&f.source.id) + .unwrap() + .unwrap() + .github_repo + .as_deref(), + Some("acme/other") + ); + } + + /// The unique indexes are the backstop inside the transaction, and a + /// violation has to leave the branch where it was. + #[test] + fn rolls_back_when_the_destination_already_has_the_repo() { + let f = fixture(); + let conflict = ProjectRepo::new(&f.target.id, "acme/widgets", "main", None); + f.store.create_project_repo(&conflict).unwrap(); + + let err = f + .store + .move_branch_to_project(&reparent(&f, None)) + .unwrap_err(); + + assert!( + err.to_string().contains("already has this repository"), + "unexpected error: {err}" + ); + assert_eq!( + f.store + .get_branch(&f.branch.id) + .unwrap() + .unwrap() + .project_id, + f.source.id + ); + assert_eq!( + f.store + .get_project_repo(&f.repo.id) + .unwrap() + .unwrap() + .project_id, + f.source.id + ); + } +} diff --git a/apps/staged/src-tauri/src/store/images.rs b/apps/staged/src-tauri/src/store/images.rs index 0c59a1cd0..bb279b66c 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -53,6 +53,21 @@ impl Store { rows.collect::, _>>().map_err(Into::into) } + /// Every image row attached to a branch, session-scoped ones included. + /// + /// [`Self::list_images_for_branch`] is the timeline view and stops at + /// branch-level attachments; this is the whole set, for callers that have to + /// account for the files on disk (moving a branch between projects). + pub fn list_all_images_for_branch(&self, branch_id: &str) -> Result, StoreError> { + let conn = self.conn.lock().unwrap(); + let mut stmt = conn.prepare( + "SELECT id, branch_id, project_id, session_id, filename, mime_type, size_bytes, created_at + FROM images WHERE branch_id = ?1 ORDER BY created_at ASC", + )?; + let rows = stmt.query_map(params![branch_id], Self::row_to_image)?; + rows.collect::, _>>().map_err(Into::into) + } + /// Return a filename that is unique among images on the given branch (or project if no branch). /// If `filename` is already taken, appends ` 2`, ` 3`, … before the extension /// (e.g. `Screenshot.png` → `Screenshot 2.png`). diff --git a/apps/staged/src-tauri/src/store/mod.rs b/apps/staged/src-tauri/src/store/mod.rs index 024b1e5b6..d1de4d8ba 100644 --- a/apps/staged/src-tauri/src/store/mod.rs +++ b/apps/staged/src-tauri/src/store/mod.rs @@ -14,6 +14,7 @@ pub mod models; mod actions; +mod branch_move; mod branches; mod commits; pub mod images; @@ -37,6 +38,7 @@ mod migration_tests; mod tests; // Re-export all model types for backwards compatibility. +pub use branch_move::{BranchMove, RepoPlacement, WorkdirMove}; pub use models::*; pub use repo_badges::{fallback_short_name, next_hue}; diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 55265eec9..2a655977f 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -1316,6 +1316,20 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { + let store = get_store(store_mutex)?; + let branch_id: String = arg(&args, "branchId")?; + let target_project_id: String = arg(&args, "targetProjectId")?; + let moved = crate::branches::move_branch_impl( + &store, + action_executor, + action_registry, + &branch_id, + &target_project_id, + ) + .await?; + Ok(serde_json::to_value(moved).unwrap()) + } "get_blox_env" => Ok(serde_json::to_value(std::env::var("BLOX_ENV").ok()).unwrap()), // ===================================================================== // Workspace / Blox commands (Tier 3) diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index 1f50234d6..c7cf3f9cb 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -353,6 +353,24 @@ export function renameBranch(branchId: string, branchName: string): Promise { + const branch = await invokeCommand('move_branch', { branchId, targetProjectId }); + await Promise.all([ + invalidateCacheByCommand('list_projects'), + invalidateCacheByCommand('list_branches_for_project'), + invalidateCacheByCommand('list_project_repos'), + ]); + return branch; +} + /** Return the BLOX_ENV environment variable value, or null if unset. */ export function getBloxEnv(): Promise { return invokeCommand('get_blox_env'); diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index eb4aada94..649e1ecd2 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -116,6 +116,7 @@ workspaceError?: string; onDelete?: () => void; onRename?: (branchName: string) => void | Promise; + onMove?: (targetProjectId: string) => void | Promise; onRetryWorktree?: () => void; } @@ -128,6 +129,7 @@ workspaceError, onDelete, onRename, + onMove, onRetryWorktree, }: Props = $props(); @@ -1927,6 +1929,7 @@ {remoteWorkspaceStatus} {onDelete} {onRename} + {onMove} onNoteCreated={() => loadTimeline()} onRebaseBranch={() => startBranchCommandPipeline('rebase')} onSquashCommits={() => startBranchCommandPipeline('squash')} diff --git a/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte b/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte index 7ded99ea8..9984e99fa 100644 --- a/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte @@ -12,6 +12,7 @@ import { onMount, onDestroy } from 'svelte'; import GitBranch from '@lucide/svelte/icons/git-branch'; import Copy from '@lucide/svelte/icons/copy'; + import FolderInput from '@lucide/svelte/icons/folder-input'; import ExternalLink from '@lucide/svelte/icons/external-link'; import Trash2 from '@lucide/svelte/icons/trash-2'; import MoreVertical from '@lucide/svelte/icons/more-vertical'; @@ -31,6 +32,7 @@ import { agentState } from '../agents/agent.svelte'; import * as DropdownMenu from '$lib/components/ui/dropdown-menu'; import RenameBranchDialog from './RenameBranchDialog.svelte'; + import MoveBranchDialog from './MoveBranchDialog.svelte'; interface Props { branch: Branch; @@ -41,6 +43,7 @@ remoteWorkspaceStatus: string | null; onDelete?: () => void; onRename?: (branchName: string) => void | Promise; + onMove?: (targetProjectId: string) => void | Promise; onNoteCreated?: () => void; onRebaseBranch?: () => void; onSquashCommits?: () => void; @@ -59,6 +62,7 @@ remoteWorkspaceStatus, onDelete, onRename, + onMove, onNoteCreated, onRebaseBranch, onSquashCommits, @@ -95,6 +99,7 @@ }); let renameDialogOpen = $state(false); + let moveDialogOpen = $state(false); // More menu state let openerApps = $state([]); @@ -147,6 +152,10 @@ renameDialogOpen = true; } + function handleMoveFromMenu() { + moveDialogOpen = true; + } + const terminalAppIds = new Set([ 'terminal', 'warp', @@ -272,6 +281,11 @@ Rename Branch + {#if isLocal} + + Move to Project… + + {/if} onRebaseBranch?.()}> Rebase Branch @@ -290,6 +304,8 @@ + + + + + + + { + if (nextOpen) { + open = true; + } else { + requestClose(); + } + }} +> + + + Move to Project + + {branch.branchName} moves with its notes, commits, reviews and sessions. + + + +
+
+ + +
+ +
+ {#each filtered as project (project.id)} + {@const repos = projectsDataStore.reposByProject.get(project.id) ?? []} + + {:else} +

+ {#if query.trim()} + No projects matching "{query.trim()}" + {:else} + No other projects yet. + {/if} +

+ {/each} +
+ + {#if error} + + {:else if invalidReason} + + {/if} + + + + + +
+ + + + diff --git a/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts b/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts new file mode 100644 index 000000000..0e88a8386 --- /dev/null +++ b/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, it } from 'vitest'; +import { + branchRepoIdentity, + filterMoveTargets, + isMoveTargetChecking, + moveTargetInvalidReason, + repoKey, +} from './moveBranchTarget'; +import type { Project, ProjectRepo } from '../../types'; + +function project(overrides: Partial & { id: string; name: string }): Project { + return { + githubRepo: null, + location: 'local', + subpath: null, + createdAt: 0, + updatedAt: 0, + ...overrides, + }; +} + +function repo(overrides: Partial & { id: string; githubRepo: string }): ProjectRepo { + return { + projectId: 'p', + branchName: 'main', + subpath: null, + isPrimary: false, + reason: null, + headRepo: null, + createdAt: 0, + updatedAt: 0, + ...overrides, + }; +} + +describe('repoKey', () => { + it('treats a null and an empty subpath as the same repo, like the unique index', () => { + expect(repoKey('acme/widgets', null)).toBe(repoKey('acme/widgets', '')); + expect(repoKey('acme/widgets', 'apps/web')).not.toBe(repoKey('acme/widgets', null)); + }); +}); + +describe('branchRepoIdentity', () => { + it('prefers the branch’s own repo row', () => { + const identity = branchRepoIdentity( + repo({ id: 'r1', githubRepo: 'acme/widgets', subpath: 'apps/web' }), + project({ id: 'p1', name: 'Source', githubRepo: 'acme/other' }) + ); + + expect(identity).toEqual({ githubRepo: 'acme/widgets', subpath: 'apps/web' }); + }); + + it('falls back to the source project’s primary repo for a branch with no row', () => { + const identity = branchRepoIdentity( + null, + project({ id: 'p1', name: 'Source', githubRepo: 'acme/widgets', subpath: 'apps/web' }) + ); + + expect(identity).toEqual({ githubRepo: 'acme/widgets', subpath: 'apps/web' }); + }); + + it('has no identity when neither the row nor the project names a repo', () => { + expect(branchRepoIdentity(null, project({ id: 'p1', name: 'Empty' }))).toBeNull(); + }); +}); + +describe('filterMoveTargets', () => { + const alpha = project({ id: 'a', name: 'Alpha' }); + const beta = project({ id: 'b', name: 'Beta' }); + const repos = new Map([ + ['a', [repo({ id: 'r1', githubRepo: 'acme/widgets' })]], + ['b', [repo({ id: 'r2', githubRepo: 'other/gadgets', subpath: 'apps/web' })]], + ]); + + it('returns every candidate for an empty query', () => { + expect(filterMoveTargets([alpha, beta], repos, ' ')).toEqual([alpha, beta]); + }); + + it('matches on the project name', () => { + expect(filterMoveTargets([alpha, beta], repos, 'bet')).toEqual([beta]); + }); + + it('matches on an attached repo path, including its subpath', () => { + expect(filterMoveTargets([alpha, beta], repos, 'widgets')).toEqual([alpha]); + expect(filterMoveTargets([alpha, beta], repos, 'gadgets/apps/web')).toEqual([beta]); + }); + + it('matches nothing when neither the name nor a repo matches', () => { + expect(filterMoveTargets([alpha, beta], repos, 'zzz')).toEqual([]); + }); +}); + +describe('moveTargetInvalidReason', () => { + const branchRepo = { githubRepo: 'acme/widgets', subpath: null }; + + it('rejects a remote project, repos fetched or not', () => { + const remote = project({ id: 'r', name: 'Remote', location: 'remote' }); + + expect(moveTargetInvalidReason(remote, branchRepo, undefined)).toBe( + "Remote projects can't receive branches." + ); + expect(moveTargetInvalidReason(remote, branchRepo, [])).toBe( + "Remote projects can't receive branches." + ); + }); + + it('rejects a project that already has the branch’s repo', () => { + const target = project({ id: 't', name: 'Target' }); + + expect( + moveTargetInvalidReason(target, branchRepo, [repo({ id: 'r1', githubRepo: 'acme/widgets' })]) + ).toBe('Target already has acme/widgets attached.'); + }); + + it('names the subpath when the branch’s repo has one', () => { + const target = project({ id: 't', name: 'Target' }); + + expect( + moveTargetInvalidReason(target, { githubRepo: 'acme/widgets', subpath: 'apps/web' }, [ + repo({ id: 'r1', githubRepo: 'acme/widgets', subpath: 'apps/web' }), + ]) + ).toBe('Target already has acme/widgets (apps/web) attached.'); + }); + + it('counts an empty destination subpath as the same repo as a null one', () => { + const target = project({ id: 't', name: 'Target' }); + + expect( + moveTargetInvalidReason(target, branchRepo, [ + repo({ id: 'r1', githubRepo: 'acme/widgets', subpath: '' }), + ]) + ).toBe('Target already has acme/widgets attached.'); + }); + + it('accepts a project holding the same repo at a different subpath', () => { + const target = project({ id: 't', name: 'Target' }); + + expect( + moveTargetInvalidReason(target, branchRepo, [ + repo({ id: 'r1', githubRepo: 'acme/widgets', subpath: 'apps/web' }), + ]) + ).toBeNull(); + }); + + it('accepts a project with no overlapping repo', () => { + const target = project({ id: 't', name: 'Target' }); + + expect( + moveTargetInvalidReason(target, branchRepo, [repo({ id: 'r1', githubRepo: 'other/gadgets' })]) + ).toBeNull(); + }); +}); + +describe('isMoveTargetChecking', () => { + it('waits on a local project whose repos have not been fetched', () => { + expect(isMoveTargetChecking(project({ id: 't', name: 'Target' }), undefined)).toBe(true); + expect(isMoveTargetChecking(project({ id: 't', name: 'Target' }), [])).toBe(false); + }); + + it('does not wait on a remote project — it is rejected either way', () => { + expect( + isMoveTargetChecking(project({ id: 'r', name: 'Remote', location: 'remote' }), undefined) + ).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/features/branches/moveBranchTarget.ts b/apps/staged/src/lib/features/branches/moveBranchTarget.ts new file mode 100644 index 000000000..f2578071c --- /dev/null +++ b/apps/staged/src/lib/features/branches/moveBranchTarget.ts @@ -0,0 +1,94 @@ +/** + * Which projects a branch can move to, and why a chosen one can't take it. + * + * MoveBranchDialog's list and its disabled Move button both read from here, so + * the rules the backend enforces have exactly one frontend statement. + */ +import { matchesRepoSearch } from '../../shared/repoSearch'; +import { projectDisplayName } from '../../shared/utils'; +import type { Project, ProjectRepo } from '../../types'; + +/** The repo a branch travels with: `githubRepo` plus its optional subpath. */ +export interface BranchRepo { + githubRepo: string; + subpath: string | null; +} + +/** + * A repo's identity inside a project, keyed the way the backend's + * `idx_project_repos_unique` does — `(github_repo, COALESCE(subpath, ''))`, so a + * NULL subpath and an empty one are the same repo. + */ +export function repoKey(githubRepo: string, subpath: string | null | undefined): string { + return `${githubRepo}\x00${subpath ?? ''}`; +} + +/** + * The repo the branch moves with: its own `project_repos` row when it has one, + * else its project's denormalized primary — mirroring the backend's + * `resolve_branch_repo_slug` fallback. + */ +export function branchRepoIdentity( + repo: ProjectRepo | null | undefined, + sourceProject: Project | null | undefined +): BranchRepo | null { + if (repo) { + return { githubRepo: repo.githubRepo, subpath: repo.subpath }; + } + if (sourceProject?.githubRepo) { + return { githubRepo: sourceProject.githubRepo, subpath: sourceProject.subpath }; + } + return null; +} + +export function describeBranchRepo(repo: BranchRepo): string { + return repo.subpath ? `${repo.githubRepo} (${repo.subpath})` : repo.githubRepo; +} + +/** Match projects on their name or on any attached repo path. */ +export function filterMoveTargets( + candidates: Project[], + reposByProject: Map, + query: string +): Project[] { + const trimmed = query.trim(); + if (!trimmed) return candidates; + const lower = trimmed.toLowerCase(); + return candidates.filter((project) => { + if (projectDisplayName(project).toLowerCase().includes(lower)) return true; + return (reposByProject.get(project.id) ?? []).some((repo) => + matchesRepoSearch(repo.githubRepo, repo.subpath, trimmed) + ); + }); +} + +/** + * Why `target` can't receive the branch, or `null` when it can. + * + * `targetRepos` is `undefined` for a project whose repos haven't been fetched: + * the duplicate check would be a guess, so this returns `null` and callers gate + * on [`isMoveTargetChecking`] instead of letting the move through. + */ +export function moveTargetInvalidReason( + target: Project, + branchRepo: BranchRepo | null, + targetRepos: ProjectRepo[] | undefined +): string | null { + // Remote projects share one Blox workspace across their branches, so a move + // in would mean cross-workspace surgery on a filesystem we don't own. + if (target.location === 'remote') return "Remote projects can't receive branches."; + if (!targetRepos || !branchRepo) return null; + const key = repoKey(branchRepo.githubRepo, branchRepo.subpath); + if (targetRepos.some((repo) => repoKey(repo.githubRepo, repo.subpath) === key)) { + return `${projectDisplayName(target)} already has ${describeBranchRepo(branchRepo)} attached.`; + } + return null; +} + +/** Whether the target's repos still have to land before the move can be judged. */ +export function isMoveTargetChecking( + target: Project, + targetRepos: ProjectRepo[] | undefined +): boolean { + return target.location !== 'remote' && targetRepos === undefined; +} diff --git a/apps/staged/src/lib/features/projects/ProjectHome.svelte b/apps/staged/src/lib/features/projects/ProjectHome.svelte index 3eaa89653..99c8172a8 100644 --- a/apps/staged/src/lib/features/projects/ProjectHome.svelte +++ b/apps/staged/src/lib/features/projects/ProjectHome.svelte @@ -696,6 +696,28 @@ throw e; } } + + async function handleMoveBranch( + branchId: string, + sourceProjectId: string, + targetProjectId: string + ) { + try { + await commands.moveBranch(branchId, targetProjectId); + // A move reshapes both ends: the branch leaves one project's list and + // joins the other's, and each project's repos may have changed with it. + await Promise.all([ + projectsDataStore.refreshProject(sourceProjectId), + projectsDataStore.refreshProject(targetProjectId), + ]); + commands.invalidateBranchTimeline(branchId); + const target = projectsDataStore.projects.find((p) => p.id === targetProjectId); + toast.success(`Branch moved to ${target ? projectDisplayName(target) : 'the project'}`); + } catch (e) { + console.error('Failed to move branch:', e); + throw e; + } + } handleDeleteBranchRequest(branchId, project)} onRenameBranch={(branchId, branchName) => handleRenameBranch(branchId, project.id, branchName)} + onMoveBranch={(branchId, targetProjectId) => + handleMoveBranch(branchId, project.id, targetProjectId)} onProjectTitleElement={selectedProjectId ? setProjectTitleElement : undefined} onRepoSelected={(selection) => handleRepoSelected(project.id, selection)} onRetryWorktree={(branchId) => setupBranchWorktree(branchId, project.id)} diff --git a/apps/staged/src/lib/features/projects/ProjectSection.svelte b/apps/staged/src/lib/features/projects/ProjectSection.svelte index d95e2b46a..971d2267c 100644 --- a/apps/staged/src/lib/features/projects/ProjectSection.svelte +++ b/apps/staged/src/lib/features/projects/ProjectSection.svelte @@ -59,6 +59,7 @@ workspaceErrors?: Map; onDeleteBranch?: (branchId: string) => void; onRenameBranch?: (branchId: string, branchName: string) => void | Promise; + onMoveBranch?: (branchId: string, targetProjectId: string) => void | Promise; onProjectTitleElement?: (element: HTMLHeadingElement | null) => void; onRepoSelected?: (selection: RepoPickerSelection) => void | Promise; onRetryWorktree?: (branchId: string) => void; @@ -74,6 +75,7 @@ workspaceErrors = new Map(), onDeleteBranch, onRenameBranch, + onMoveBranch, onProjectTitleElement, onRepoSelected, onRetryWorktree, @@ -532,6 +534,7 @@ workspaceError={workspaceErrors.get(branch.id)} onDelete={() => onDeleteBranch?.(branch.id)} onRename={(branchName) => onRenameBranch?.(branch.id, branchName)} + onMove={(targetProjectId) => onMoveBranch?.(branch.id, targetProjectId)} onRetryWorktree={() => onRetryWorktree?.(branch.id)} /> {/each} From 93a0514e2e421a20d1fb7771da6bbfab981a0375 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 13 Aug 2026 16:36:05 +1000 Subject: [PATCH 2/6] fix(branches): serialize a branch move and fix its picker's arrow keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the review on de6de4f. The "no session is running on this branch" precondition was checked at plan time with nothing held through the mutation, so a session starting between the check and the `git worktree move` — or a second move for the same branch dispatched from the web router — raced the rename. Planning now happens twice: a pre-flight that refuses an impossible move before any of the branch's actions are stopped for it, then again inside `apply_branch_move`, which holds the branch's session launch lock across the re-check, the rename and the transaction. That is the lock `prs` already takes around its queue-or-start decisions, and holding it means the whole mutation runs synchronously on a blocking thread rather than around an await. When the transaction fails and the worktree can't be moved back, the returned error now says where the worktree actually is: reporting only the store error read as "nothing changed" while the tree sat where neither project expects it. The crash window between the rename and the commit is documented rather than closed — `new_path` is derived rather than stored, so re-running the move finds the worktree already sitting where the transaction is about to record it, and completes. `is_unique_violation` matched on SQLite's message text alone, so a future error that merely mentioned `idx_project_repos_unique` would have been rewritten into a confident "the destination already has this repository"; it now requires `ErrorCode::ConstraintViolation` first, with a test that an unrelated failure naming the index keeps its own words. In the dialog, `highlightedIndex` is derived from the selection instead of tracked beside it, and the clamping moves into a tested helper. Arrowing down a long list, narrowing it with a query and then pressing ArrowUp left the index past the end of the new list, where `selectProject` received `undefined` and threw; clicking a row likewise left the index stale for the next arrow press. Verified with `just ci`: fmt, clippy, svelte-check, 729 Rust and 648 frontend tests. Co-Authored-By: Claude Opus 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/branches.rs | 118 ++++++++++++------ .../staged/src-tauri/src/store/branch_move.rs | 29 ++++- .../features/branches/MoveBranchDialog.svelte | 22 ++-- .../branches/moveBranchTarget.test.ts | 25 ++++ .../lib/features/branches/moveBranchTarget.ts | 13 ++ 5 files changed, 158 insertions(+), 49 deletions(-) diff --git a/apps/staged/src-tauri/src/branches.rs b/apps/staged/src-tauri/src/branches.rs index d23988cfe..097b4faa2 100644 --- a/apps/staged/src-tauri/src/branches.rs +++ b/apps/staged/src-tauri/src/branches.rs @@ -2435,6 +2435,9 @@ fn plan_branch_move( } // The move pulls the worktree out from under anything running in it. + // [`apply_branch_move`] plans again under the branch's session launch lock, + // which is what keeps a session from starting between this check and the + // rename that acts on it. if store .has_running_session_for_branch(branch_id) .map_err(|e| e.to_string())? @@ -2547,7 +2550,8 @@ fn plan_branch_move( /// /// The move runs in three stages that have to agree with each other: the /// worktree relocation on disk, the re-parent transaction, and the image files. -/// A failed transaction puts the worktree back. +/// The first two go together in [`apply_branch_move`], under the branch's +/// session launch lock; a failed transaction puts the worktree back. pub(crate) async fn move_branch_impl( store: &Arc, executor: &ActionExecutor, @@ -2555,39 +2559,94 @@ pub(crate) async fn move_branch_impl( branch_id: &str, target_project_id: &str, ) -> Result { + // Pre-flight, so an impossible move is refused before anything the branch is + // running gets stopped for it. [`apply_branch_move`] plans again under the + // launch lock, which is what makes the answer safe to act on. + plan_branch_move(store, branch_id, target_project_id)?; + + crate::actions::commands::stop_actions_for_branches(executor, registry, &[branch_id]); + + let source_project = { + let store = Arc::clone(store); + let branch_id = branch_id.to_string(); + let target_project_id = target_project_id.to_string(); + tauri::async_runtime::spawn_blocking(move || { + apply_branch_move(&store, &branch_id, &target_project_id) + }) + .await + .map_err(|e| format!("Failed to move branch: {e}"))?? + }; + + move_branch_image_files(store, branch_id, &source_project.id, target_project_id); + + // Cheaper to drop than to move, and it rebuilds on the next diff read. + if let Err(e) = crate::diff_cache::delete_branch_cache( + source_project.location, + &source_project.id, + branch_id, + ) { + log::warn!("Failed to clear diff cache for moved branch {branch_id}: {e}"); + } + + let updated = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + let workdir = store + .get_workdir_for_branch(branch_id) + .map_err(|e| e.to_string())? + .map(|w| w.path); + Ok(to_branch_with_workdir(updated, workdir)) +} + +/// Plan the move and apply it: the worktree on disk first, then the transaction +/// that re-parents the branch, with the rename undone if the transaction fails. +/// Returns the source project, which the caller still needs to relocate the +/// branch's images and drop its diff cache. +/// +/// Synchronous and holding the branch's session launch lock throughout, the way +/// [`crate::prs`] holds it around its queue-or-start decisions: the plan's "no +/// session is running on this branch" precondition is only worth acting on for +/// as long as no session can start underneath it, and two moves of the same +/// branch — the Tauri command and the web router are separate entry points — +/// would otherwise race the same rename. The plan is resolved in here for that +/// reason rather than handed in from outside the lock. +fn apply_branch_move( + store: &Arc, + branch_id: &str, + target_project_id: &str, +) -> Result { + let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); + let _guard = launch_lock.lock().unwrap(); + let BranchMovePlan { source_project, github_repo, mv, } = plan_branch_move(store, branch_id, target_project_id)?; - crate::actions::commands::stop_actions_for_branches(executor, registry, &[branch_id]); - let repo_path = crate::paths::repos_dir() .map(|d| d.join(&github_repo)) .ok_or("Cannot determine clone path")?; - let workdir_move = mv.workdir.clone(); - // A branch whose setup never finished has nothing on disk to relocate. + // A branch whose setup never finished has nothing on disk to relocate — and + // neither does one whose move was interrupted between this rename and the + // commit below, because `new_path` is derived rather than stored: re-running + // the move finds the worktree already sitting where the transaction is about + // to record it, and completes. let mut moved_on_disk = false; - if let Some(wd) = &workdir_move { - let old_path = PathBuf::from(&wd.old_path); - let new_path = PathBuf::from(&wd.new_path); + if let Some(wd) = &mv.workdir { + let old_path = Path::new(&wd.old_path); if old_path.exists() { - let repo_path = repo_path.clone(); - tauri::async_runtime::spawn_blocking(move || { - git::move_worktree(&repo_path, &old_path, &new_path) - }) - .await - .map_err(|e| format!("Failed to move worktree: {e}"))? - .map_err(|e| format!("Failed to move worktree: {e}"))?; + git::move_worktree(&repo_path, old_path, Path::new(&wd.new_path)) + .map_err(|e| format!("Failed to move worktree: {e}"))?; moved_on_disk = true; } } if let Err(e) = store.move_branch_to_project(&mv) { // Disk and database have to agree, so undo the half that landed. - if let (true, Some(wd)) = (moved_on_disk, &workdir_move) { + if let (true, Some(wd)) = (moved_on_disk, &mv.workdir) { if let Err(revert) = git::move_worktree(&repo_path, Path::new(&wd.new_path), Path::new(&wd.old_path)) { @@ -2595,31 +2654,18 @@ pub(crate) async fn move_branch_impl( "Failed to move worktree for branch {branch_id} back to {}: {revert}", wd.old_path ); + // Reporting only the store error would read as "nothing changed" + // while the worktree sits where neither project expects it. + return Err(format!( + "{e} The worktree could not be moved back either, so it is now at {} instead of {}: {revert}", + wd.new_path, wd.old_path + )); } } return Err(e.to_string()); } - move_branch_image_files(store, branch_id, &source_project.id, target_project_id); - - // Cheaper to drop than to move, and it rebuilds on the next diff read. - if let Err(e) = crate::diff_cache::delete_branch_cache( - source_project.location, - &source_project.id, - branch_id, - ) { - log::warn!("Failed to clear diff cache for moved branch {branch_id}: {e}"); - } - - let updated = store - .get_branch(branch_id) - .map_err(|e| e.to_string())? - .ok_or_else(|| format!("Branch not found: {branch_id}"))?; - let workdir = store - .get_workdir_for_branch(branch_id) - .map_err(|e| e.to_string())? - .map(|w| w.path); - Ok(to_branch_with_workdir(updated, workdir)) + Ok(source_project) } /// Relocate a moved branch's image files into the destination project's diff --git a/apps/staged/src-tauri/src/store/branch_move.rs b/apps/staged/src-tauri/src/store/branch_move.rs index b5d5a9fc2..d27ab5a91 100644 --- a/apps/staged/src-tauri/src/store/branch_move.rs +++ b/apps/staged/src-tauri/src/store/branch_move.rs @@ -208,9 +208,16 @@ fn elect_primary_repo( Ok(()) } +/// Whether `err` is a constraint violation naming one of `needles`. +/// +/// The index or column name only comes back as message text, so the code has to +/// be checked too: without it any future error that happens to mention +/// `project_repos` would be rewritten into a confidently wrong explanation. fn is_unique_violation(err: &rusqlite::Error, needles: &[&str]) -> bool { match err { - rusqlite::Error::SqliteFailure(_, Some(msg)) => { + rusqlite::Error::SqliteFailure(inner, Some(msg)) + if inner.code == rusqlite::ErrorCode::ConstraintViolation => + { needles.iter().any(|needle| msg.contains(needle)) } _ => false, @@ -571,4 +578,24 @@ mod tests { f.source.id ); } + + /// Only a constraint violation may be rewritten into the friendly message: + /// the index name is matched in the error's text, so an unrelated failure + /// that happens to mention it has to keep its own words. + #[test] + fn explains_only_constraint_violations() { + let busy = rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_BUSY), + Some("database is locked writing idx_project_repos_unique".to_string()), + ); + let violation = rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_CONSTRAINT_UNIQUE), + Some("UNIQUE constraint failed: index 'idx_project_repos_unique'".to_string()), + ); + + assert!(duplicate_repo_error(&busy).to_string().contains("locked")); + assert!(duplicate_repo_error(&violation) + .to_string() + .contains("already has this repository")); + } } diff --git a/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte b/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte index 34b662ba7..0431b1841 100644 --- a/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte +++ b/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte @@ -28,6 +28,7 @@ filterMoveTargets, isMoveTargetChecking, moveTargetInvalidReason, + nextMoveTargetIndex, } from './moveBranchTarget'; import type { Branch, Project, ProjectRepo } from '../../types'; @@ -43,7 +44,6 @@ const searchId = `move-branch-search-${++inputCounter}`; let query = $state(''); let selectedId = $state(null); - let highlightedIndex = $state(-1); let error = $state(null); let moving = $state(false); let searchElement = $state(null); @@ -53,7 +53,6 @@ if (open && !wasOpen) { query = ''; selectedId = null; - highlightedIndex = -1; error = null; moving = false; // Repos hydrate lazily, so the duplicate-repo check can't be trusted @@ -72,6 +71,9 @@ let candidates = $derived(projectsDataStore.projects.filter((p) => p.id !== branch.projectId)); let filtered = $derived(filterMoveTargets(candidates, projectsDataStore.reposByProject, query)); let selected = $derived(filtered.find((p) => p.id === selectedId) ?? null); + // Derived, not tracked: a stale cursor could outlive the row it named — the + // query narrowing the list, or a click moving the selection out from under it. + let highlightedIndex = $derived(filtered.findIndex((p) => p.id === selectedId)); let selectedRepos = $derived( selected ? projectsDataStore.reposByProject.get(selected.id) : undefined ); @@ -97,16 +99,12 @@ } function handleSearchKeydown(e: KeyboardEvent) { - if (filtered.length === 0) return; - if (e.key === 'ArrowDown') { - e.preventDefault(); - highlightedIndex = Math.min(highlightedIndex + 1, filtered.length - 1); - selectProject(filtered[highlightedIndex]); - } else if (e.key === 'ArrowUp') { - e.preventDefault(); - highlightedIndex = Math.max(highlightedIndex - 1, 0); - selectProject(filtered[highlightedIndex]); - } + const delta = e.key === 'ArrowDown' ? 1 : e.key === 'ArrowUp' ? -1 : 0; + if (delta === 0) return; + e.preventDefault(); + const next = nextMoveTargetIndex(highlightedIndex, delta, filtered.length); + if (next < 0) return; + selectProject(filtered[next]); } async function handleSubmit(e: SubmitEvent) { diff --git a/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts b/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts index 0e88a8386..9615f0eb2 100644 --- a/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts +++ b/apps/staged/src/lib/features/branches/moveBranchTarget.test.ts @@ -4,6 +4,7 @@ import { filterMoveTargets, isMoveTargetChecking, moveTargetInvalidReason, + nextMoveTargetIndex, repoKey, } from './moveBranchTarget'; import type { Project, ProjectRepo } from '../../types'; @@ -151,6 +152,30 @@ describe('moveTargetInvalidReason', () => { }); }); +describe('nextMoveTargetIndex', () => { + it('starts at the first row when nothing is selected', () => { + expect(nextMoveTargetIndex(-1, 1, 3)).toBe(0); + expect(nextMoveTargetIndex(-1, -1, 3)).toBe(0); + }); + + it('steps within the list and stops at both ends', () => { + expect(nextMoveTargetIndex(0, 1, 3)).toBe(1); + expect(nextMoveTargetIndex(2, 1, 3)).toBe(2); + expect(nextMoveTargetIndex(1, -1, 3)).toBe(0); + expect(nextMoveTargetIndex(0, -1, 3)).toBe(0); + }); + + it('clamps a cursor left past the end by a narrowing query', () => { + expect(nextMoveTargetIndex(7, -1, 2)).toBe(1); + expect(nextMoveTargetIndex(7, 1, 2)).toBe(1); + }); + + it('selects nothing when the query matched nothing', () => { + expect(nextMoveTargetIndex(-1, 1, 0)).toBe(-1); + expect(nextMoveTargetIndex(3, -1, 0)).toBe(-1); + }); +}); + describe('isMoveTargetChecking', () => { it('waits on a local project whose repos have not been fetched', () => { expect(isMoveTargetChecking(project({ id: 't', name: 'Target' }), undefined)).toBe(true); diff --git a/apps/staged/src/lib/features/branches/moveBranchTarget.ts b/apps/staged/src/lib/features/branches/moveBranchTarget.ts index f2578071c..7f788418f 100644 --- a/apps/staged/src/lib/features/branches/moveBranchTarget.ts +++ b/apps/staged/src/lib/features/branches/moveBranchTarget.ts @@ -85,6 +85,19 @@ export function moveTargetInvalidReason( return null; } +/** + * The row an arrow key moves the keyboard cursor to, clamped into the list. + * + * `current` is derived from the selection rather than tracked alongside it, so + * it is `-1` while nothing is selected and can name a row the current query has + * filtered away; both have to land back inside the list rather than index off + * the end of it. Returns `-1` for an empty list, which selects nothing. + */ +export function nextMoveTargetIndex(current: number, delta: number, length: number): number { + if (length === 0) return -1; + return Math.min(Math.max(current + delta, 0), length - 1); +} + /** Whether the target's repos still have to land before the move can be judged. */ export function isMoveTargetChecking( target: Project, From 55cfa10345c2b5ad6b5cf466a3a2f7386a6e0a8c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 13 Aug 2026 17:01:27 +1000 Subject: [PATCH 3/6] style(branches): restyle the move dialog to match the session modals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The move-to-project dialog used the stock shadcn dialog chrome — stacked header, default footer, transparent input — while every other modal (new session, note, session) shares a house style. It now follows NewSessionModal: a flush header bar with the title and a ghost X close button over a border, an 18px-padded body, and an outline Cancel next to an accent Move button. The search field and the project list sit on var(--bg-primary) — white in light mode, the same surface as the session prompt editor — with a var(--border-muted) border, instead of disappearing into the gray card. The input's focus ring is swapped for the editor's border-emphasis treatment, and on mobile the list stretches to fill the full-screen dialog rather than capping at the desktop height. Verified with prettier, svelte-check (0 errors) and 648 frontend tests. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- .../features/branches/MoveBranchDialog.svelte | 101 +++++++++++++++--- 1 file changed, 86 insertions(+), 15 deletions(-) diff --git a/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte b/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte index 0431b1841..07a5031bf 100644 --- a/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte +++ b/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte @@ -16,6 +16,7 @@ import { tick } from 'svelte'; import Cloud from '@lucide/svelte/icons/cloud'; import Search from '@lucide/svelte/icons/search'; + import X from '@lucide/svelte/icons/x'; import * as Dialog from '$lib/components/ui/dialog'; import { Button } from '$lib/components/ui/button'; import { Input } from '$lib/components/ui/input'; @@ -134,15 +135,32 @@ } }} > - - - Move to Project - + +
+ + Move to Project + + +
+ +
+ {branch.branchName} moves with its notes, commits, reviews and sessions. - -
@@ -200,11 +218,22 @@ {/if} - - - - +
From 1b0b72e36a5a28737b4ceb723ed604f53833afbc Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 13 Aug 2026 17:33:22 +1000 Subject: [PATCH 4/6] style(branches): share the sidebar's project row with the move picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The move dialog's project rows showed a plain name over full repo-path labels, so the same projects read differently than they do in the sidebar, which introduces each one with a state icon and colored repo badges. The sidebar row's identity block — the PR-status or cloud icon on the left, the bold name, and the badge/activity meta line with its repo-count fallback — now lives in a shared ProjectRowContent component that both the sidebar and MoveBranchDialog render. It derives everything from the shared stores given only the project, so a picker row shows the same live state the sidebar does. The dialog needed nothing new to feed it: opening it already hydrates every project's branches and repos for the duplicate-repo check, and hydration is also what materializes the repo badges. The sidebar's active-row brightening reached its meta text through scoped descendant selectors, which can't cross into the child, and a :global svg override would tie with the child's status-icon colors at equal specificity, leaving the winner to stylesheet order. The meta color is now a --project-row-meta-color custom property the child reads, and the stroke override names the two places that still need it — the row's status spinners and the All Repos icon — where its higher specificity wins deterministically. Verified with prettier, svelte-check (0 errors, 0 warnings) and 648 frontend tests. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- .../features/branches/MoveBranchDialog.svelte | 38 +-- .../projects/ProjectRowContent.svelte | 227 ++++++++++++++++++ .../features/projects/ProjectsSidebar.svelte | 202 +--------------- 3 files changed, 238 insertions(+), 229 deletions(-) create mode 100644 apps/staged/src/lib/features/projects/ProjectRowContent.svelte diff --git a/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte b/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte index 07a5031bf..0be964c41 100644 --- a/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte +++ b/apps/staged/src/lib/features/branches/MoveBranchDialog.svelte @@ -14,16 +14,14 @@ + +
+ {#if project.location === 'remote'} + + {:else if prStatus === 'merged'} + + {:else if prStatus === 'checks_failing'} + + {:else if prStatus === 'open'} + + {:else if prStatus === 'closed'} + + {:else if prStatus === 'conflict'} + + {:else if projectHasCodeChanges(branches)} + + {:else} + + {/if} +
+ {projectDisplayName(project)} +
+ {#if badges.length > 0} + + {#each badges as badge} + + {/each} + + {#if activity} + · + {activity} + {/if} + {:else} + {projectSubtitle(repoCount, sessionTypes, status.runActionPhase)} + {/if} +
+
+
+ + diff --git a/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte b/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte index 307d241af..c32756da0 100644 --- a/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte +++ b/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte @@ -4,29 +4,14 @@ import { fade } from 'svelte/transition'; import House from '@lucide/svelte/icons/house'; import Plus from '@lucide/svelte/icons/plus'; - import Cloud from '@lucide/svelte/icons/cloud'; - import GitPullRequest from '@lucide/svelte/icons/git-pull-request'; - import GitPullRequestClosed from '@lucide/svelte/icons/git-pull-request-closed'; - import GitPullRequestDraft from '@lucide/svelte/icons/git-pull-request-draft'; import GitBranch from '@lucide/svelte/icons/git-branch'; - import Sprout from '@lucide/svelte/icons/sprout'; import FolderGit2 from '@lucide/svelte/icons/folder-git-2'; import Mail from '@lucide/svelte/icons/mail'; import Trash2 from '@lucide/svelte/icons/trash-2'; - import type { Project, WorkspaceStatus, RepoHomeItem } from '../../types'; + import type { RepoHomeItem } from '../../types'; import { goHome, navigation, selectProject, showAllRepos } from '../layout/navigation.svelte'; - import { - projectDisplayName, - aggregateProjectPrStatus, - projectHasCodeChanges, - projectSubtitle, - projectActivity, - } from '../../shared/utils'; - import RepoBadge from '../../shared/RepoBadge.svelte'; - import { repoBadgeStore } from '../../stores/repoBadges.svelte'; import { projectsDataStore } from '../../stores/projectsData.svelte'; import { projectRunActionsStore } from '../../stores/projectRunActions.svelte'; - import { projectStateStore } from '../../stores/projectState.svelte'; import Spinner from '../../shared/Spinner.svelte'; import SineWave from '../../shared/SineWave.svelte'; import StagedIcon from '../../shared/StagedIcon.svelte'; @@ -42,6 +27,7 @@ } from './projectsSidebarState.svelte'; import { viewport, watchViewport } from '../../shared/viewport.svelte'; import RepoCard from './RepoCard.svelte'; + import ProjectRowContent from './ProjectRowContent.svelte'; import SidebarFilterRow from './SidebarFilterRow.svelte'; import { projectFiltersStore } from './projectFilters.svelte'; import * as commands from '../../api/commands'; @@ -61,8 +47,6 @@ // state (width, scroll, drag) lives here or in projectsSidebarState. let projects = $derived(projectsDataStore.projects); let projectBranches = $derived(projectsDataStore.branchesByProject); - let reposByProject = $derived(projectsDataStore.reposByProject); - let repoCountsByProject = $derived(projectsDataStore.repoCountsByProject); let deletingProjectNames = $derived(projectsDataStore.deletingProjectNames); let loading = $derived(projectsDataStore.loading || !projectsDataStore.loaded); let error = $derived(projectsDataStore.error); @@ -287,37 +271,6 @@ } } - function repoCountForProject(project: Project): number { - return repoCountsByProject.get(project.id) ?? (project.githubRepo ? 1 : 0); - } - - function getProjectPrStatus( - projectId: string - ): 'merged' | 'open' | 'closed' | 'checks_failing' | 'conflict' | null { - const branches = projectBranches.get(projectId) || []; - return aggregateProjectPrStatus(branches); - } - - function getProjectWorkspaceStatus(projectId: string): WorkspaceStatus | null { - const branches = projectBranches.get(projectId) || []; - return branches.find((b) => b.workspaceStatus)?.workspaceStatus ?? null; - } - - function cloudStatusClass(status: WorkspaceStatus | null): string { - switch (status) { - case 'running': - return 'cloud-running'; - case 'starting': - return 'cloud-starting'; - case 'error': - return 'cloud-error'; - case 'stopped': - case 'suspended': - default: - return 'cloud-inactive'; - } - } - let resizing = $state(false); let resizeStartX = 0; let resizeStartWidth = SIDEBAR_DEFAULT_WIDTH; @@ -515,17 +468,6 @@ deletingProjectNames, projectBranches.get(project.id) || [] )} - {@const repoCount = repoCountForProject(project)} - {@const prStatus = getProjectPrStatus(project.id)} - {@const workspaceStatus = - project.location === 'remote' ? getProjectWorkspaceStatus(project.id) : null} - {@const sessionTypes = projectStateStore.getRunningSessionTypes(project.id)} - {@const repos = reposByProject.get(project.id) ?? []} - {@const badges = repos - .map((r) => repoBadgeStore.lookup(r.githubRepo, r.subpath)) - .filter((b): b is NonNullable => Boolean(b)) - .sort((a, b) => a.shortName.localeCompare(b.shortName))} - {@const activity = projectActivity(sessionTypes, status.runActionPhase)}