diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index bc2d9c2..00e3a93 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -247,6 +247,12 @@ pub fn run() { pr::pr_merge, pr::merge_base_into_branch, pr::gh_username, + // PRs tab (list / review / checkout / return) + pr::pr_list, + pr::pr_checkout, + pr::pr_checkout_return, + pr::pr_review_enter, + pr::pr_review_restore, // Connections (Task #10) connections::connections_scan, // Search (Task #15) @@ -300,8 +306,6 @@ pub fn run() { #[cfg(target_os = "macos")] warp_term::term_native_selection_text, #[cfg(target_os = "macos")] - warp_term::term_native_selection_scrolled, - #[cfg(target_os = "macos")] warp_term::term_native_set_viewport, #[cfg(target_os = "macos")] warp_term::term_native_link_at, diff --git a/src-tauri/src/pr.rs b/src-tauri/src/pr.rs index 4d93978..dd0e710 100644 --- a/src-tauri/src/pr.rs +++ b/src-tauri/src/pr.rs @@ -30,6 +30,43 @@ pub struct PrStatus { pub mergeable: Option, } +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PrListItem { + pub number: u64, + pub title: String, + pub url: String, + pub head_ref_name: String, + pub base_ref_name: String, + pub author: String, + pub additions: u64, + pub deletions: u64, + pub changed_files: u64, + /// One of MERGEABLE, CONFLICTING, UNKNOWN. + pub mergeable: String, + pub is_draft: bool, + pub updated_at: String, +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PrCheckoutResult { + pub original_branch: String, + pub stashed: bool, + /// Branch head at checkout time — what `pr_checkout_return` needs + /// to undo the review-state soft reset. None when review state + /// could not be established (the branch is checked out normally). + pub head_sha: Option, +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PrReturnResult { + /// False when a stash was expected but the tagged stash was gone — + /// the branch switch happened, but nothing was restored. + pub stash_restored: bool, +} + #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ConflictResult { @@ -576,6 +613,360 @@ pub async fn pr_status(cwd: String, branch: String) -> Result }) } +/// List every open PR on the repo via `gh pr list`. Drives the right +/// panel's PRs tab. Author is flattened to the login string; draft PRs +/// are included (the tab renders a Draft chip rather than hiding them). +#[tauri::command] +pub async fn pr_list(cwd: String) -> Result, String> { + if !Path::new(&cwd).exists() { + return Err(format!("cwd does not exist: {cwd}")); + } + let out = Command::new("gh") + .args([ + "pr", + "list", + "--state", + "open", + "--limit", + "50", + "--json", + "number,title,url,headRefName,baseRefName,author,additions,deletions,changedFiles,mergeable,isDraft,updatedAt", + ]) + .current_dir(&cwd) + .output() + .await + .map_err(|e| format!("spawn gh: {e}. Is GitHub CLI installed? `brew install gh`"))?; + if !out.status.success() { + return Err(format!( + "gh pr list failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + )); + } + #[derive(Deserialize)] + struct AuthorJson { + login: String, + } + #[derive(Deserialize)] + #[serde(rename_all = "camelCase")] + struct RowJson { + number: u64, + title: String, + url: String, + head_ref_name: String, + base_ref_name: String, + author: Option, + additions: u64, + deletions: u64, + changed_files: u64, + mergeable: String, + is_draft: bool, + updated_at: String, + } + let rows: Vec = serde_json::from_slice(&out.stdout) + .map_err(|e| format!("parse gh json: {e}"))?; + Ok(rows + .into_iter() + .map(|r| PrListItem { + number: r.number, + title: r.title, + url: r.url, + head_ref_name: r.head_ref_name, + base_ref_name: r.base_ref_name, + author: r.author.map(|a| a.login).unwrap_or_default(), + additions: r.additions, + deletions: r.deletions, + changed_files: r.changed_files, + mergeable: r.mergeable, + is_draft: r.is_draft, + updated_at: r.updated_at, + }) + .collect()) +} + +/// Put the checked-out PR branch into "review state": HEAD soft-reset +/// to the merge-base with `base`, so the PR's entire diff shows up as +/// staged changes and flows through the app's normal changes UI (the +/// Changes tab, the all-changes view, per-file diffs) instead of a +/// bespoke PR-diff surface. Returns the branch head sha needed to undo +/// this later. +/// +/// Best-effort by design: any failure past resolving HEAD leaves the +/// branch checked out normally and still returns the sha — the +/// checkout already succeeded, and the caller must be able to offer +/// the way back regardless. +async fn enter_review_state(cwd: &str, base: &str) -> Option { + let head_sha = run_git_checked(cwd, &["rev-parse", "HEAD"]) + .await + .ok()? + .trim() + .to_string(); + if head_sha.is_empty() || base.is_empty() { + return if head_sha.is_empty() { None } else { Some(head_sha) }; + } + // origin/ may be stale or absent locally; refresh best-effort. + // Non-interactive and timeboxed — a fetch that wants credentials or + // a dead network must degrade to "merge-base against the local ref", + // never hang the checkout. + let mut fetch = Command::new("git"); + fetch + .args(["fetch", "origin", base]) + .current_dir(cwd) + .stdin(std::process::Stdio::null()); + for (k, v) in NON_INTERACTIVE_GIT_ENV { + fetch.env(k, v); + } + let _ = tokio::time::timeout(std::time::Duration::from_secs(20), fetch.output()).await; + let origin_base = format!("origin/{base}"); + let merge_base = match run_git_checked(cwd, &["merge-base", &origin_base, "HEAD"]).await { + Ok(s) => s.trim().to_string(), + Err(_) => match run_git_checked(cwd, &["merge-base", base, "HEAD"]).await { + Ok(s) => s.trim().to_string(), + Err(_) => return Some(head_sha), + }, + }; + if !merge_base.is_empty() && merge_base != head_sha { + let _ = run_git_checked(cwd, &["reset", "--soft", &merge_base]).await; + } + Some(head_sha) +} + +/// Undo `enter_review_state`: move the branch ref forward to the +/// recorded head. Soft, so any review edits stay in the index/worktree +/// as diffs against the real head — lossless. Only acts when HEAD is +/// an ancestor of `head_sha` (the soft reset is still in effect); if +/// the user or an agent committed on top, resetting would drop those +/// commits from the branch, so we leave everything alone and report +/// false. +async fn restore_review_state(cwd: &str, head_sha: &str) -> Result { + let head = run_git_checked(cwd, &["rev-parse", "HEAD"]) + .await? + .trim() + .to_string(); + if head == head_sha { + return Ok(false); + } + let ancestor = Command::new("git") + .args(["merge-base", "--is-ancestor", "HEAD", head_sha]) + .current_dir(cwd) + .output() + .await + .map_err(|e| format!("spawn git: {e}"))? + .status + .success(); + if !ancestor { + return Ok(false); + } + run_git_checked(cwd, &["reset", "--soft", head_sha]).await?; + Ok(true) +} + +/// Re-establish review state on an already-checked-out PR branch — +/// used after a clean base merge so the (now conflict-free) PR diff +/// shows as staged changes again. Returns the new head sha to persist. +#[tauri::command] +pub async fn pr_review_enter(cwd: String, base: String) -> Result, String> { + if !Path::new(&cwd).exists() { + return Err(format!("cwd does not exist: {cwd}")); + } + Ok(enter_review_state(&cwd, &base).await) +} + +/// Leave review state (branch ref back on the real head) without +/// switching branches — the step before any operation that needs the +/// branch in its true shape, e.g. merging the base in. +#[tauri::command] +pub async fn pr_review_restore(cwd: String, head_sha: String) -> Result { + if !Path::new(&cwd).exists() { + return Err(format!("cwd does not exist: {cwd}")); + } + if head_sha.is_empty() { + return Ok(false); + } + restore_review_state(&cwd, &head_sha).await +} + +/// Message marker on the auto-stash created by `pr_checkout`, suffixed +/// with the branch it was taken from so `pr_checkout_return` can find +/// exactly this stash even if the user made others in between. +const PR_STASH_MARKER: &str = "goonware-pr-checkout"; + +/// Find the stash ref (e.g. `stash@{1}`) whose message carries our +/// marker for `branch`. Newest match wins. +async fn find_pr_stash(cwd: &str, branch: &str) -> Option { + let raw = run_git(cwd, &["stash", "list", "--format=%gd\u{1f}%gs"]) + .await + .ok()?; + let needle = format!("{PR_STASH_MARKER}:{branch}"); + for line in raw.lines() { + let Some((gd, gs)) = line.split_once('\u{1f}') else { + continue; + }; + // %gs renders as "On : " — compare the message + // exactly, so returning to `feat` can never pop a stash tagged + // for `feature` (or any user stash that merely mentions the + // marker). Branch names can't contain spaces, so the first + // ": " is always git's own separator. + let msg = gs.split_once(": ").map_or(gs, |(_, m)| m); + if msg == needle { + return Some(gd.to_string()); + } + } + None +} + +/// Turn the worktree into a review environment for a PR: stash any +/// uncommitted work (tagged so we can restore it later), `gh pr +/// checkout `, then soft-reset to the merge-base with `base` +/// so the PR's whole diff appears as staged changes in the normal +/// changes UI. Returns what the caller needs to offer a "go back" +/// button — the original branch, whether a stash was made, and the PR +/// branch's real head sha. +/// +/// If the checkout itself fails the stash is popped back immediately so +/// the user's work never silently disappears into the stash list. +#[tauri::command] +pub async fn pr_checkout( + cwd: String, + number: u64, + base: String, + head: String, +) -> Result { + if !Path::new(&cwd).exists() { + return Err(format!("cwd does not exist: {cwd}")); + } + let original_branch = run_git_checked(&cwd, &["symbolic-ref", "--short", "HEAD"]) + .await + .map_err(|_| "could not determine current branch (detached HEAD?)".to_string())? + .trim() + .to_string(); + if original_branch.is_empty() { + return Err("could not determine current branch (detached HEAD?)".into()); + } + + // Reviewing the PR that IS this worktree's branch would stash the + // user's own work-in-progress and soft-reset their working branch + // in place — and when the worktree hosts a running dev build, that + // rewrites the app's own sources mid-flight. There is nothing to + // check out; refuse up front. + if !head.is_empty() && head == original_branch { + return Err(format!( + "PR #{number}'s branch (`{head}`) is what this worktree is already on — \ + this worktree IS that PR. Review it from a different branch or worktree." + )); + } + + // A tagged stash for this branch means an earlier review session + // never finished (e.g. the app died mid-checkout). Stacking a + // second stash under the same tag would make the eventual pop + // ambiguous — refuse and point at the recovery path instead. + if find_pr_stash(&cwd, &original_branch).await.is_some() { + return Err(format!( + "A previous PR review left stashed work on `{original_branch}` \ + (see `git stash list`). Pop or drop that stash, then retry." + )); + } + + let porcelain = run_git_checked(&cwd, &["status", "--porcelain"]).await?; + let dirty = porcelain.lines().any(|l| !l.trim().is_empty()); + let mut stashed = false; + if dirty { + let msg = format!("{PR_STASH_MARKER}:{original_branch}"); + run_git_checked(&cwd, &["stash", "push", "-u", "-m", &msg]).await?; + stashed = true; + } + + let num = number.to_string(); + let out = Command::new("gh") + .args(["pr", "checkout", &num]) + .current_dir(&cwd) + .output() + .await + .map_err(|e| format!("spawn gh: {e}. Is GitHub CLI installed? `brew install gh`"))?; + if !out.status.success() { + let mut msg = format!( + "gh pr checkout failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + ); + // Roll the stash back so a failed checkout is a clean no-op. + // gh isn't atomic — it can die after already switching branches + // — so make sure the pop lands on the original branch, and say + // so when the stash couldn't be restored rather than dropping + // the pop error on the floor. + if stashed { + let head = run_git_checked(&cwd, &["symbolic-ref", "--short", "HEAD"]) + .await + .map(|s| s.trim().to_string()) + .unwrap_or_default(); + let on_original = head == original_branch + || run_git_checked(&cwd, &["checkout", &original_branch]) + .await + .is_ok(); + let popped = match find_pr_stash(&cwd, &original_branch).await { + Some(stash_ref) if on_original => { + run_git_checked(&cwd, &["stash", "pop", &stash_ref]) + .await + .is_ok() + } + _ => false, + }; + if !popped { + msg.push_str( + " — your uncommitted work is still saved in the stash list (`git stash list`)", + ); + } + } + return Err(msg); + } + // Best-effort past this point — the checkout succeeded, so the + // session record (and its way back) must reach the caller even if + // review state couldn't be established. + let head_sha = enter_review_state(&cwd, &base).await; + Ok(PrCheckoutResult { + original_branch, + stashed, + head_sha, + }) +} + +/// Undo `pr_checkout`: put the PR branch back on its real head (the +/// review-state soft reset would otherwise carry the whole PR diff +/// onto the original branch as staged changes), switch back, and pop +/// the tagged stash if one was made. Fails loudly if the PR branch has +/// review edits (git refuses the checkout) — the user decides whether +/// to commit or discard those, we never do it for them. +/// +/// `stash_restored` is false when `stashed` was set but the tagged +/// stash no longer exists (e.g. the user popped it by hand) — the +/// caller must not claim the work was restored. +#[tauri::command] +pub async fn pr_checkout_return( + cwd: String, + branch: String, + stashed: bool, + head_sha: Option, +) -> Result { + if !Path::new(&cwd).exists() { + return Err(format!("cwd does not exist: {cwd}")); + } + if branch.is_empty() { + return Err("no branch to return to".into()); + } + // A failed restore must abort the return: checking out with the + // PR's diff still staged would splat it onto the original branch. + if let Some(sha) = head_sha.as_deref().filter(|s| !s.is_empty()) { + restore_review_state(&cwd, sha).await?; + } + run_git_checked(&cwd, &["checkout", &branch]).await?; + let mut stash_restored = false; + if stashed { + if let Some(stash_ref) = find_pr_stash(&cwd, &branch).await { + run_git_checked(&cwd, &["stash", "pop", &stash_ref]).await?; + stash_restored = true; + } + } + Ok(PrReturnResult { stash_restored }) +} + /// Merge a PR via `gh pr merge` — server-side merge using the user's /// existing GitHub auth. Defaults to `--merge` (merge commit). Pass /// `"squash"` or `"rebase"` to override. Branch is deleted on remote @@ -1208,4 +1599,272 @@ mod tests { .expect_err("missing cwd should error"); assert!(!err.is_empty()); } + + // ---- pr_checkout_return stash round-trip ----------------------------- + + #[tokio::test] + async fn checkout_return_restores_branch_and_pops_tagged_stash() { + // Simulate the state pr_checkout leaves behind: work stashed + // with the marker message on `feature/mine`, HEAD moved to a + // "PR branch". Return must land back on the original branch + // with the stashed file restored — and leave an unrelated, + // newer stash untouched. + let (clone, _bare) = build_repo_with_bare_remote("feature/mine"); + let cwd = clone.path().to_str().unwrap(); + + std::fs::write(clone.path().join("wip.txt"), "my wip\n").unwrap(); + run_sync( + clone.path(), + &[ + "stash", + "push", + "-u", + "-m", + &format!("{PR_STASH_MARKER}:feature/mine"), + ], + ); + run_sync(clone.path(), &["checkout", "-b", "pr-branch"]); + + // A second, unrelated stash made "later" (stash@{0}) must survive. + std::fs::write(clone.path().join("other.txt"), "other\n").unwrap(); + run_sync(clone.path(), &["stash", "push", "-u", "-m", "user stash"]); + + let result = pr_checkout_return(cwd.to_string(), "feature/mine".to_string(), true, None) + .await + .expect("return succeeds"); + assert!(result.stash_restored, "tagged stash should be reported restored"); + + let branch = run_sync(clone.path(), &["symbolic-ref", "--short", "HEAD"]); + assert_eq!(branch.trim(), "feature/mine"); + assert!( + clone.path().join("wip.txt").exists(), + "stashed wip should be restored" + ); + let stashes = run_sync(clone.path(), &["stash", "list"]); + assert!( + stashes.contains("user stash"), + "unrelated stash must survive: {stashes}" + ); + assert!( + !stashes.contains(PR_STASH_MARKER), + "tagged stash should be consumed: {stashes}" + ); + } + + #[tokio::test] + async fn checkout_return_without_stash_just_switches_branch() { + let (clone, _bare) = build_repo_with_bare_remote("feature/clean"); + let cwd = clone.path().to_str().unwrap(); + run_sync(clone.path(), &["checkout", "-b", "pr-branch"]); + + let result = pr_checkout_return(cwd.to_string(), "feature/clean".to_string(), false, None) + .await + .expect("return succeeds"); + assert!(!result.stash_restored, "no stash existed to restore"); + + let branch = run_sync(clone.path(), &["symbolic-ref", "--short", "HEAD"]); + assert_eq!(branch.trim(), "feature/clean"); + } + + #[tokio::test] + async fn checkout_return_ignores_stash_tagged_for_prefixed_branch() { + // Marker matching is exact: a stash tagged for `feature/mine-2` + // must not be popped when returning to `feature/mine`. + let (clone, _bare) = build_repo_with_bare_remote("feature/mine"); + let cwd = clone.path().to_str().unwrap(); + + std::fs::write(clone.path().join("wip.txt"), "other branch wip\n").unwrap(); + run_sync( + clone.path(), + &[ + "stash", + "push", + "-u", + "-m", + &format!("{PR_STASH_MARKER}:feature/mine-2"), + ], + ); + run_sync(clone.path(), &["checkout", "-b", "pr-branch"]); + + let result = pr_checkout_return(cwd.to_string(), "feature/mine".to_string(), true, None) + .await + .expect("return succeeds"); + assert!( + !result.stash_restored, + "stash tagged for a prefixed branch name must not match" + ); + let stashes = run_sync(clone.path(), &["stash", "list"]); + assert!( + stashes.contains("feature/mine-2"), + "the other branch's stash must survive: {stashes}" + ); + } + + // ---- pr_checkout guards ---------------------------------------------- + + #[tokio::test] + async fn checkout_refuses_the_worktrees_own_branch() { + // Reviewing the PR whose head IS the current branch would stash + // the user's WIP and soft-reset their own working branch — + // refuse before touching anything. + let (clone, _bare) = build_repo_with_bare_remote("feature/mine"); + let cwd = clone.path().to_str().unwrap(); + std::fs::write(clone.path().join("wip.txt"), "wip\n").unwrap(); + + let err = pr_checkout( + cwd.to_string(), + 7, + "main".to_string(), + "feature/mine".to_string(), + ) + .await + .expect_err("same-branch checkout must refuse"); + assert!(err.contains("already on"), "unexpected error: {err}"); + + // Nothing was stashed or moved. + assert!(clone.path().join("wip.txt").exists()); + let stashes = run_sync(clone.path(), &["stash", "list"]); + assert!(stashes.trim().is_empty(), "no stash expected: {stashes}"); + } + + #[tokio::test] + async fn checkout_refuses_when_a_tagged_stash_already_exists() { + // A leftover tagged stash means a previous session never + // finished — a second checkout must not stack another stash + // under the same tag. + let (clone, _bare) = build_repo_with_bare_remote("feature/mine"); + let cwd = clone.path().to_str().unwrap(); + std::fs::write(clone.path().join("old-wip.txt"), "stranded\n").unwrap(); + run_sync( + clone.path(), + &[ + "stash", + "push", + "-u", + "-m", + &format!("{PR_STASH_MARKER}:feature/mine"), + ], + ); + + let err = pr_checkout( + cwd.to_string(), + 7, + "main".to_string(), + "someone-elses-branch".to_string(), + ) + .await + .expect_err("existing tagged stash must refuse"); + assert!(err.contains("stash"), "unexpected error: {err}"); + + // The stranded stash is untouched — exactly one, same tag. + let stashes = run_sync(clone.path(), &["stash", "list"]); + assert_eq!( + stashes.matches(PR_STASH_MARKER).count(), + 1, + "stash list changed: {stashes}" + ); + } + + // ---- review state (soft reset to merge-base) -------------------------- + + /// Repo with main pushed to a bare remote plus a two-commit + /// "PR branch" checked out. Returns (clone, bare, pr_head_sha). + fn build_pr_branch_repo() -> (TempDir, TempDir, String) { + let (clone, bare) = build_repo_on_main("main"); + run_sync(clone.path(), &["checkout", "-b", "pr-branch"]); + std::fs::write(clone.path().join("a.txt"), "one\n").unwrap(); + run_sync(clone.path(), &["add", "a.txt"]); + run_sync(clone.path(), &["commit", "-m", "pr commit 1"]); + std::fs::write(clone.path().join("b.txt"), "two\n").unwrap(); + run_sync(clone.path(), &["add", "b.txt"]); + run_sync(clone.path(), &["commit", "-m", "pr commit 2"]); + let head = run_sync(clone.path(), &["rev-parse", "HEAD"]).trim().to_string(); + (clone, bare, head) + } + + #[tokio::test] + async fn enter_review_state_stages_pr_diff_and_restore_undoes_it() { + let (clone, _bare, pr_head) = build_pr_branch_repo(); + let cwd = clone.path().to_str().unwrap(); + + let returned = enter_review_state(cwd, "main").await; + assert_eq!(returned.as_deref(), Some(pr_head.as_str())); + + // HEAD sits on the merge-base; the PR's files are staged. + let mb = run_sync(clone.path(), &["merge-base", "origin/main", &pr_head]); + let head_now = run_sync(clone.path(), &["rev-parse", "HEAD"]); + assert_eq!(head_now.trim(), mb.trim()); + let staged = run_sync(clone.path(), &["diff", "--cached", "--name-only"]); + assert!(staged.contains("a.txt") && staged.contains("b.txt"), + "PR files should show as staged: {staged}"); + + // Restore: branch ref back on the real head, tree clean. + let did = restore_review_state(cwd, &pr_head).await.expect("restore"); + assert!(did, "restore should have acted"); + let head_after = run_sync(clone.path(), &["rev-parse", "HEAD"]); + assert_eq!(head_after.trim(), pr_head); + let porcelain = run_sync(clone.path(), &["status", "--porcelain"]); + assert!(porcelain.trim().is_empty(), "expected clean tree: {porcelain}"); + } + + #[tokio::test] + async fn restore_review_state_refuses_when_commits_were_made_on_top() { + let (clone, _bare, pr_head) = build_pr_branch_repo(); + let cwd = clone.path().to_str().unwrap(); + enter_review_state(cwd, "main").await.expect("enter"); + + // Simulate a takeover: commit the staged review diff as one + // new commit. HEAD is no longer an ancestor of the PR head. + run_sync(clone.path(), &["commit", "-m", "review takeover"]); + let new_head = run_sync(clone.path(), &["rev-parse", "HEAD"]).trim().to_string(); + + let did = restore_review_state(cwd, &pr_head).await.expect("restore call"); + assert!(!did, "must not reset past user commits"); + let head_after = run_sync(clone.path(), &["rev-parse", "HEAD"]); + assert_eq!(head_after.trim(), new_head, "takeover commit must survive"); + } + + #[tokio::test] + async fn checkout_return_with_head_sha_restores_branch_before_switching() { + let (clone, _bare, pr_head) = build_pr_branch_repo(); + let cwd = clone.path().to_str().unwrap(); + enter_review_state(cwd, "main").await.expect("enter"); + + let result = pr_checkout_return( + cwd.to_string(), + "main".to_string(), + false, + Some(pr_head.clone()), + ) + .await + .expect("return succeeds"); + assert!(!result.stash_restored); + + // Back on main with a clean tree — the PR diff did NOT come along. + let branch = run_sync(clone.path(), &["symbolic-ref", "--short", "HEAD"]); + assert_eq!(branch.trim(), "main"); + let porcelain = run_sync(clone.path(), &["status", "--porcelain"]); + assert!(porcelain.trim().is_empty(), "expected clean tree: {porcelain}"); + // And the PR branch still points at its real head. + let pr_ref = run_sync(clone.path(), &["rev-parse", "pr-branch"]); + assert_eq!(pr_ref.trim(), pr_head); + } + + #[tokio::test] + async fn review_edits_survive_restore_as_uncommitted_changes() { + let (clone, _bare, pr_head) = build_pr_branch_repo(); + let cwd = clone.path().to_str().unwrap(); + enter_review_state(cwd, "main").await.expect("enter"); + + // Edit a PR file during review. + std::fs::write(clone.path().join("a.txt"), "one edited\n").unwrap(); + + let did = restore_review_state(cwd, &pr_head).await.expect("restore"); + assert!(did); + // The edit remains, now as a plain uncommitted diff vs the head. + let porcelain = run_sync(clone.path(), &["status", "--porcelain"]); + assert!(porcelain.contains("a.txt"), "edit must survive: {porcelain}"); + let content = std::fs::read_to_string(clone.path().join("a.txt")).unwrap(); + assert_eq!(content, "one edited\n"); + } } diff --git a/src-tauri/src/warp_term.rs b/src-tauri/src/warp_term.rs index f531058..dd615c7 100644 --- a/src-tauri/src/warp_term.rs +++ b/src-tauri/src/warp_term.rs @@ -371,16 +371,22 @@ impl Pane { } } -/// The panes: index 0 = main column, 1 = right-panel side terminal. Created -/// lazily before attach so the sinks and the view share the same `Pane`s. -static PANES: OnceLock<[Pane; 2]> = OnceLock::new(); -fn panes() -> &'static [Pane; 2] { - PANES.get_or_init(|| [Pane::new(), Pane::new()]) +/// The panes: index 0 = main column (or the LEFT half of a main-column +/// split), 1 = right-panel side terminal, 2 = the RIGHT half of a +/// main-column split. Created lazily before attach so the sinks and the +/// view share the same `Pane`s. +static PANES: OnceLock<[Pane; 3]> = OnceLock::new(); +fn panes() -> &'static [Pane; 3] { + PANES.get_or_init(|| [Pane::new(), Pane::new(), Pane::new()]) } -/// Resolve a React pane key to its `Pane`. Anything but "side" is the main pane -/// (so a missing/legacy key maps safely to main). +/// Resolve a React pane key to its `Pane`. Anything but "side" / "main2" is +/// the main pane (so a missing/legacy key maps safely to main). fn pane(key: &str) -> &'static Pane { - &panes()[if key == "side" { 1 } else { 0 }] + &panes()[match key { + "side" => 1, + "main2" => 2, + _ => 0, + }] } /// The pane currently mirroring `pty_id`, if any (frame/block sink routing). fn pane_for_pty(pty_id: &str) -> Option<&'static Pane> { @@ -871,6 +877,87 @@ fn is_zsh_eol_marker(row: &RowSnapshot) -> bool { seen } +/// Concatenated glyph text of a row with trailing blanks trimmed — the identity +/// we match on when measuring how far an alt-screen app scrolled. We compare +/// TEXT (not the full styled spans) so a pager re-coloring a line (e.g. moving +/// its highlighted current line, or a cursor landing on it) doesn't defeat the +/// match. +fn row_text(row: &RowSnapshot) -> String { + let mut s = String::new(); + for sp in &row.spans { + s.push_str(&sp.text); + } + s.trim_end().to_string() +} + +/// Measure how many rows an alt-screen app scrolled between two consecutive grid +/// snapshots, by content matching. Returns `k` such that the new grid shows, at +/// row `i`, what the old grid had at row `i + k`: +/// - `k > 0` → content moved UP by k rows (scrolled toward newer / down) +/// - `k < 0` → content moved DOWN by k rows (scrolled toward older / up) +/// - `0` → no clear scroll (partial repaint, spinner tick, cursor blink, +/// a page swap, or genuinely nothing moved) +/// +/// This is the source of truth for gluing a selection to alt-screen text, and it +/// replaces the old "assume the app scrolls one row per wheel notch" guess — +/// which drifts on any app that scrolls several rows per notch (vim's default, +/// most pagers). Because it reads the app's ACTUAL response it's correct +/// regardless of the app's wheel-to-rows ratio. +/// +/// Deliberately conservative: it reports a non-zero shift only when a clear +/// majority of the NON-BLANK rows line up at exactly one offset AND that offset +/// explains more rows than staying put (k = 0). A one-line spinner update or a +/// single streamed character leaves k = 0 unbeaten, so a completed selection is +/// never nudged by a non-scroll repaint — the exact "jumpy / stuck selection" +/// artifact a naive always-shift approach produces. +fn detect_scroll_shift(old: &[RowSnapshot], new: &[RowSnapshot]) -> i32 { + let n = old.len().min(new.len()); + if n < 4 { + return 0; // too little signal to be confident + } + let o: Vec = old.iter().take(n).map(row_text).collect(); + let e: Vec = new.iter().take(n).map(row_text).collect(); + + // Count indices where new[i] equals old[i + k], ignoring blank rows (a blank + // line matches every other blank line and would inflate every offset). + let score = |k: i32| -> usize { + let mut c = 0usize; + for i in 0..n { + let j = i as i32 + k; + if j < 0 || j as usize >= n { + continue; + } + if !e[i].is_empty() && e[i] == o[j as usize] { + c += 1; + } + } + c + }; + + let base = score(0); // non-blank rows still in place + let mut best_k = 0i32; + let mut best = base; + let range = (n as i32) - 1; + for k in -range..=range { + if k == 0 { + continue; + } + let s = score(k); + if s > best { + best = s; + best_k = k; + } + } + + // Require the winning offset to be genuinely dominant: it must line up a solid + // block of rows and clearly beat the in-place score, else treat it as noise. + if best_k != 0 && best >= 3 && best > base + 1 { + best_k + } else { + 0 + } +} + /// How many leading rows of the live grid to render: trims trailing blank rows /// (keeping through the cursor row) so an idle / just-finished shell screen sits /// compactly above the input instead of padding the transcript with blanks. @@ -1197,6 +1284,34 @@ fn build_pane_column(p: &'static Pane, mono: FamilyId) -> Box { }, grid, ); + // Clip a wide alt grid to the pane. The narrow side pane pins a wide + // PTY (PAN_MIN_COLS) for shell layout, and an alt-screen TUI paints + // rows at that full grid width — ConstrainedBox only bounds layout, + // it doesn't clip painting — so without this the rows draw straight + // past the pane's right edge, over the neighbouring React panels + // ("terminal pokes out the side"). Same horizontal ClippedScrollable + // as the shell transcript below, gated on real overflow. + let grid_px = g.n_cols as f32 * CELL_ADVANCE; + let h_overflow = pane_w > 1.0 && grid_px > pane_w + 2.0; + let clipped: Box = if h_overflow { + let max_hscroll = (grid_px - pane_w).max(0.0); + let hx = p.hscroll.scroll_start().as_f32().clamp(0.0, max_hscroll); + p.hscroll.scroll_to(Pixels::new(hx)); + let bounded = ConstrainedBox::new(Box::new(selectable)).with_width(grid_px); + Box::new( + ClippedScrollable::horizontal( + p.hscroll.clone(), + Box::new(bounded), + ScrollbarWidth::None, + Fill::None, + Fill::None, + Fill::None, + ) + .with_overlayed_scrollbar(), + ) + } else { + Box::new(selectable) + }; // Bound the full grid to the pane's height at its surface offset. // `pin_region`'s ConstrainedBox gives this MainAxisSize::Max grid a FINITE // height; nesting it raw under the side pane's column instead left it in @@ -1205,9 +1320,9 @@ fn build_pane_column(p: &'static Pane, mono: FamilyId) -> Box { // (rect not reported yet — only the topmost pane, briefly) returns the raw // grid, which the Stack bounds. if pane_h > 1.0 { - pin_region(Box::new(selectable), surface_offset_y, pane_h) + pin_region(clipped, surface_offset_y, pane_h) } else { - Box::new(selectable) + clipped } } else { // Shell transcript AND inline agents (claude/codex): closed blocks @@ -1483,62 +1598,82 @@ impl View for TerminalRootView { } fn render(&self, _: &AppContext) -> Box { - let ps = panes(); - let main = &ps[0]; - let side = &ps[1]; - - let main_col = build_pane_column(main, self.mono); - let side_on = side.active(); - let column: Box = if side_on { - // Both panes placed (the right-panel split is open). The surface - // covers the combined bounding box; lay the panes side-by-side by - // width (they share the AppShell row's top + height), with the gap - // between them (the React divider) left black. - // - // NOTE: the MAIN pane keeps its rect even when it's showing a - // non-terminal tab (editor/diff) — see `term_native_detach` / - // `term_surface_set_rect`, which detach the pty + clear the grid but - // DON'T zero the rect. So `main.rect()` is the real main-column box - // here, the gap stays ~0 (not `side.x`), and the combined surface - // size is unchanged from the both-terminals layout. That's what keeps - // the side terminal in place: the surface never resizes on a main - // tab switch, it just paints the main column empty/black behind the - // opaque editor DOM. (A detached main renders an empty grid, hidden.) - let (mx, my, mw, mh) = main.rect(); - let (sx, sy, sw, sh) = side.rect(); - let gap = (sx - (mx + mw)).max(0.0); - // Combined surface height. BOTH columns must be height-bounded to it: - // each pane's column is a MainAxisSize::Max flex (it fills the surface, - // with a lower pane's content pushed down by surface_offset_y), and a - // Max flex PANICS under an unbounded/infinite max constraint — which - // is what a width-only ConstrainedBox left the side column with, so a - // claude/alt-screen full grid in the side pane aborted the app - // (flex/mod.rs "can't be rendered in an infinite max constraint"). - let ch = ((my + mh).max(sy + sh) - my.min(sy)).max(1.0); - let side_col = build_pane_column(side, self.mono); + // Every placed pane (non-trivial rect), laid out left-to-right by + // reported x: main (or the split's left half), the split's right + // half (main2), and the right-panel side terminal — any subset of + // which can be present. The surface covers the combined bounding + // box; the gaps between panes (React dividers) stay black. + // + // NOTE: the MAIN pane keeps its rect even when it's showing a + // non-terminal tab (editor/diff) — see `term_native_detach` / + // `term_surface_set_rect`, which detach the pty + clear the grid but + // DON'T zero the rect. So `main.rect()` is the real main-column box + // here and the combined surface size is unchanged when a main tab + // switch lands on an editor. That's what keeps the other terminals + // in place: the surface never resizes on a main tab switch, it just + // paints the main column empty/black behind the opaque editor DOM. + // (A detached main renders an empty grid, hidden.) + let mut placed: Vec<&'static Pane> = + panes().iter().filter(|p| p.active()).collect(); + placed.sort_by(|a, b| { + a.rect() + .0 + .partial_cmp(&b.rect().0) + .unwrap_or(std::cmp::Ordering::Equal) + }); + + let column: Box = if placed.len() <= 1 { + // Single (or no) placed pane — degenerate to the plain column, + // exactly the pre-split single-pane layout. `pane("main")` keeps + // the startup path (no rect reported yet) painting the main grid. + let p = placed.first().copied().unwrap_or(pane("main")); + build_pane_column(p, self.mono) + } else { + // Combined surface height. EVERY column must be height-bounded to + // it: each pane's column is a MainAxisSize::Max flex (it fills the + // surface, with a lower pane's content pushed down by + // surface_offset_y), and a Max flex PANICS under an unbounded/ + // infinite max constraint — which is what a width-only + // ConstrainedBox left the side column with, so a claude/alt-screen + // full grid in the side pane aborted the app (flex/mod.rs "can't + // be rendered in an infinite max constraint"). + let mut top = f32::MAX; + let mut bot = 0.0f32; + for p in &placed { + let (_, y, _, h) = p.rect(); + top = top.min(y); + bot = bot.max(y + h); + } + let ch = (bot - top).max(1.0); let mut kids: Vec> = Vec::new(); - kids.push(Box::new( - ConstrainedBox::new(main_col) - .with_width(mw.max(1.0)) - .with_height(ch), - )); - if gap > 0.5 { + for (i, p) in placed.iter().enumerate() { + let (x, _, w, _) = p.rect(); + // Clamp each column so it can't overlap the next pane — a + // stale retained main rect (e.g. full-width from before a + // split opened) must not push its neighbours off the surface. + let w = match placed.get(i + 1) { + Some(n) => w.min((n.rect().0 - x).max(1.0)), + None => w, + }; kids.push(Box::new( - ConstrainedBox::new(Rect::new().finish()).with_width(gap), + ConstrainedBox::new(build_pane_column(p, self.mono)) + .with_width(w.max(1.0)) + .with_height(ch), )); + if let Some(n) = placed.get(i + 1) { + let gap = n.rect().0 - (x + w); + if gap > 0.5 { + kids.push(Box::new( + ConstrainedBox::new(Rect::new().finish()).with_width(gap), + )); + } + } } - kids.push(Box::new( - ConstrainedBox::new(side_col) - .with_width(sw.max(1.0)) - .with_height(ch), - )); Flex::row() .with_main_axis_size(MainAxisSize::Max) .with_cross_axis_alignment(CrossAxisAlignment::Stretch) .with_children(kids) .finish() - } else { - main_col }; Stack::new() @@ -1580,7 +1715,26 @@ pub fn attach(app: &tauri::AppHandle) { // Route the frame to whichever pane mirrors this pty (main or side). if let Some(p) = pane_for_pty(pty_id) { let mut g = p.grid.lock().unwrap_or_else(|e| e.into_inner()); + // Alt-screen scroll-glue: an alt-screen app (git log / less / man / + // vim / htop) repaints its grid in place when it scrolls, so a + // completed selection anchored to fixed grid coordinates would slide + // off the text it was started on. Measure how far the grid ACTUALLY + // scrolled (content match — robust to the app's rows-per-wheel-notch, + // which the old React-side line-count guess got wrong) and shift the + // selection to track it. Gated to when something is actually selected + // AND the app owns the screen: the normal shell/inline-agent + // transcript glues for free (its SelectableArea lives inside the + // ClippedScrollable, whose scroll translation already moves the + // selection with the content), so we must NOT double-shift it here. + let detect = frame.alt_screen && p.sel.has_selection(); + let old_rows = if detect { g.rows.clone() } else { Vec::new() }; g.apply_frame(frame); + if detect { + let k = detect_scroll_shift(&old_rows, &g.rows); + if k != 0 { + p.sel.shift_relative_y(-(k as f32) * LINE_PX); + } + } } let _ = app_for_sink.run_on_main_thread(|| { warpui::platform::poke_embedded_redraw(); @@ -1775,8 +1929,11 @@ pub fn term_surface_set_rect(pane_key: String, x: f64, y: f64, width: f64, heigh // is what stops the right-panel side terminal from blanking / shrinking: the // shared GPU surface is fragile to resize, and a zeroed main both collapsed // the box AND broke the side's side-by-side gap math. The side pane is NOT - // retained — collapsing the right panel SHOULD shrink the surface. - let is_main = pane_key != "side"; + // retained — collapsing the right panel SHOULD shrink the surface. Neither + // is main2 (the split's right half): closing the split zero-reports it, + // and retaining would leave a stale column painting over the re-widened + // main pane. + let is_main = pane_key != "side" && pane_key != "main2"; if is_main && zero { // Drop the stale report; keep the prior rect. } else if let Ok(mut r) = p.rect.lock() { @@ -1801,13 +1958,21 @@ pub fn term_native_attach( let p = pane(&pane_key); // Stop mirroring this pane's previous pty, then mirror the new one. let prev = p.pty_id(); - if !prev.is_empty() && prev != id { - crate::term::clear_native_pty(&prev); - } if let Ok(mut g) = p.pty.lock() { g.clear(); g.push_str(&id); } + // Unregister the previous pty ONLY if no other pane mirrors it now. + // When two terminals swap halves (main ⇄ main2) the two attach calls + // land back-to-back, and the second pane's "previous" pty is exactly + // the one the first pane just claimed — clearing it unconditionally + // froze that pane (frames stopped reaching the sink). + if !prev.is_empty() + && prev != id + && panes().iter().all(|q| q.pty_id() != prev) + { + crate::term::clear_native_pty(&prev); + } crate::term::set_native_pty(&id); { let mut g = p.grid.lock().unwrap_or_else(|e| e.into_inner()); @@ -1858,13 +2023,16 @@ pub fn term_native_attach( pub fn term_native_detach(pane_key: String) { let p = pane(&pane_key); let prev = p.pty_id(); - if !prev.is_empty() { - crate::term::clear_native_pty(&prev); - } if let Ok(mut g) = p.pty.lock() { g.clear(); } - if pane_key == "side" { + // Same other-pane guard as `term_native_attach`: during a half-swap the + // pty this pane is letting go of may have just been claimed by another + // pane — don't yank its frames. + if !prev.is_empty() && panes().iter().all(|q| q.pty_id() != prev) { + crate::term::clear_native_pty(&prev); + } + if pane_key == "side" || pane_key == "main2" { if let Ok(mut r) = p.rect.lock() { *r = (0.0, 0.0, 0.0, 0.0); } @@ -1980,24 +2148,6 @@ pub fn term_native_mouse( } } -/// Tauri command: the alt-screen agent's content scrolled by `delta_lines` -/// (the same signed line count just sent through `term_native_wheel`; -/// positive = toward newer/down). The app repaints its grid in place, so a -/// selection anchored to grid coordinates would highlight whatever text -/// scrolled under it. Shift the stored selection bounds by the distance the -/// content moved (scrolling up by N lines moves the text DOWN N rows → -/// +N·LINE_PX) so the highlight stays glued to the text it was started on, -/// Warp-style. Best-effort: assumes the app scrolls one row per wheel line -/// (true for claude/codex and every pager we route here). -#[tauri::command] -pub fn term_native_selection_scrolled(pane_key: String, delta_lines: i32) { - let p = pane(&pane_key); - p.sel.shift_relative_y(-(delta_lines as f32) * LINE_PX); - if let Some(app) = APP_HANDLE.get() { - let _ = app.run_on_main_thread(|| warpui::platform::poke_embedded_redraw()); - } -} - /// Tauri command: the latest selected transcript text (cached by the /// `SelectableArea` selection handler), or `None` if nothing is selected. React /// reads this on Cmd+C in the shell and writes it to the clipboard via the @@ -2054,6 +2204,125 @@ pub fn term_native_set_viewport(pane_key: String, top: f64, height: f64) { } } +#[cfg(test)] +mod scroll_shift_tests { + use super::*; + + fn plain_span(text: &str) -> Span { + Span { + text: text.to_string(), + fg: "var(--text-primary)".into(), + bg: "var(--surface-0)".into(), + bold: false, + italic: false, + underline: false, + inverse: false, + dim: false, + strikeout: false, + link: None, + } + } + fn rows(lines: &[&str]) -> Vec { + lines + .iter() + .map(|l| RowSnapshot { + spans: vec![plain_span(l)], + }) + .collect() + } + + #[test] + fn scroll_down_shifts_content_up() { + // Ten distinct rows; the app scrolls DOWN by 3 (content moves up 3, three + // fresh rows appear at the bottom). new[i] == old[i+3] for the retained + // rows, so the measured shift is +3. + let old = rows(&[ + "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7", "r8", "r9", + ]); + let new = rows(&[ + "r3", "r4", "r5", "r6", "r7", "r8", "r9", "n7", "n8", "n9", + ]); + assert_eq!(detect_scroll_shift(&old, &new), 3); + } + + #[test] + fn scroll_up_shifts_content_down() { + // The app scrolls UP by 2 (content moves down 2, two older rows appear at + // the top). new[i] == old[i-2] → measured shift is -2. + let old = rows(&[ + "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7", "r8", "r9", + ]); + let new = rows(&[ + "p0", "p1", "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7", + ]); + assert_eq!(detect_scroll_shift(&old, &new), -2); + } + + #[test] + fn identical_grids_report_no_scroll() { + let g = rows(&["a", "b", "c", "d", "e", "f", "g", "h"]); + assert_eq!(detect_scroll_shift(&g, &g), 0); + } + + #[test] + fn spinner_tick_reports_no_scroll() { + // Only one row changes (a spinner glyph / streamed char). Staying put + // explains far more rows than any shift, so no shift is reported — this + // is what keeps a completed selection from jumping on a non-scroll frame. + let old = rows(&["a", "b", "c", "d", "e", "f", "g", "loading |"]); + let new = rows(&["a", "b", "c", "d", "e", "f", "g", "loading /"]); + assert_eq!(detect_scroll_shift(&old, &new), 0); + } + + #[test] + fn full_page_swap_reports_no_scroll() { + // A page jump replaces every row with unrelated content — no offset lines + // anything up, so we conservatively report no scroll rather than guess. + let old = rows(&["a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7"]); + let new = rows(&["z0", "z1", "z2", "z3", "z4", "z5", "z6", "z7"]); + assert_eq!(detect_scroll_shift(&old, &new), 0); + } + + #[test] + fn blank_rows_do_not_inflate_a_false_shift() { + // A grid that is mostly blank with a couple of content rows must not + // report a shift just because the blank rows "match" at every offset. + let old = rows(&["", "", "hello", "world", "", "", "", ""]); + let new = rows(&["", "", "hello", "world", "", "", "", ""]); + assert_eq!(detect_scroll_shift(&old, &new), 0); + } + + #[test] + fn ignores_color_only_changes() { + // A pager re-coloring its current line (same text, different style) is not + // a scroll. row_text compares glyphs only, so this stays at 0. + let old = vec![ + RowSnapshot { spans: vec![plain_span("line one")] }, + RowSnapshot { spans: vec![plain_span("line two")] }, + RowSnapshot { spans: vec![plain_span("line three")] }, + RowSnapshot { spans: vec![plain_span("line four")] }, + RowSnapshot { spans: vec![plain_span("line five")] }, + ]; + let mut recolored = plain_span("line three"); + recolored.inverse = true; + let new = vec![ + RowSnapshot { spans: vec![plain_span("line one")] }, + RowSnapshot { spans: vec![plain_span("line two")] }, + RowSnapshot { spans: vec![recolored] }, + RowSnapshot { spans: vec![plain_span("line four")] }, + RowSnapshot { spans: vec![plain_span("line five")] }, + ]; + assert_eq!(detect_scroll_shift(&old, &new), 0); + } + + #[test] + fn tiny_grids_bail_out() { + let old = rows(&["a", "b", "c"]); + let new = rows(&["b", "c", "d"]); + assert_eq!(detect_scroll_shift(&old, &new), 0); + } +} + #[cfg(test)] mod link_tests { use super::*; diff --git a/src/design/cm6-theme.ts b/src/design/cm6-theme.ts index ee41d7d..ec05f18 100644 --- a/src/design/cm6-theme.ts +++ b/src/design/cm6-theme.ts @@ -40,6 +40,10 @@ const COLORS = { stateWarning: "var(--state-warning)", stateError: "var(--state-error)", stateInfoMuted: "color-mix(in oklch, var(--surface-1), var(--state-info) 18%)", + + syntaxVariable: "var(--syntax-variable)", + syntaxProperty: "var(--syntax-property)", + syntaxFunction: "var(--syntax-function)", }; export const cm6Theme = EditorView.theme( @@ -134,15 +138,18 @@ export const cm6Highlight = HighlightStyle.define([ { tag: t.paren, color: COLORS.textSecondary }, { tag: t.separator, color: COLORS.textTertiary }, - // Identifiers — primary text, no special color - { tag: t.variableName, color: COLORS.textPrimary }, - { tag: t.propertyName, color: COLORS.textPrimary }, + // Identifiers — soft light-blue. Lezer tags bare variables and + // function calls alike as `variableName`, so this is what gives the + // bulk of the code color instead of a wall of near-white text. + { tag: t.variableName, color: COLORS.syntaxVariable }, + { tag: t.definition(t.variableName), color: COLORS.syntaxVariable }, + { tag: t.propertyName, color: COLORS.syntaxProperty }, { tag: t.attributeName, color: COLORS.stateInfo }, - // Functions — medium weight, primary color - { tag: t.function(t.variableName), color: COLORS.textPrimary, fontWeight: "500" }, - { tag: t.function(t.propertyName), color: COLORS.textPrimary, fontWeight: "500" }, - { tag: t.macroName, color: COLORS.textPrimary, fontWeight: "500" }, + // Functions — warm gold, medium weight + { tag: t.function(t.variableName), color: COLORS.syntaxFunction, fontWeight: "500" }, + { tag: t.function(t.propertyName), color: COLORS.syntaxFunction, fontWeight: "500" }, + { tag: t.macroName, color: COLORS.syntaxFunction, fontWeight: "500" }, // Keywords — info blue, distinct from accent { tag: t.keyword, color: COLORS.stateInfo, fontWeight: "500" }, diff --git a/src/design/tokens.css b/src/design/tokens.css index 8caf752..af1730e 100644 --- a/src/design/tokens.css +++ b/src/design/tokens.css @@ -109,6 +109,19 @@ --diff-change-bg: oklch(32% 0.13 85 / 0.40); --diff-change-fg: oklch(86% 0.18 85); + /* Syntax palette — shared by the editor theme (cm6-theme.ts) and the + diff highlighter (diff-highlight.tsx), which mirror each other. + Identifiers — variables, function calls, and properties — are the + bulk of any code diff, and Lezer tags them all as plain + `variableName`/`propertyName`. Left uncolored they fall back to + near-white --text-primary, so a diff reads as a wall of white. + A soft light-blue for identifiers and a warm gold for functions + give code real color while staying calm against the cool-dark + surface. */ + --syntax-variable: oklch(83% 0.055 245); + --syntax-property: oklch(83% 0.055 245); + --syntax-function: oklch(87% 0.11 92); + /* Modal backdrop */ --backdrop: oklch(0% 0 0 / 0.55); @@ -215,7 +228,7 @@ --right-width: 372px; /* files / changes / checks / memory + secondary terminal */ --rail-width: 48px; - --tab-height: 36px; /* 2-line tab (title + summary) at a fixed height; shared by sidebar + right-panel top bars */ + --tab-height: 30px; /* single-line tab at a fixed height; shared by sidebar + right-panel top bars */ --pane-header-height: 28px; --filter-bar-height: 32px; --section-header-height: 24px; diff --git a/src/git/AllChangesView.tsx b/src/git/AllChangesView.tsx index 534723a..cafdbd0 100644 --- a/src/git/AllChangesView.tsx +++ b/src/git/AllChangesView.tsx @@ -4,6 +4,7 @@ import { CaretDown } from "@phosphor-icons/react"; import { git } from "@/lib/git"; import { DiffBody } from "./DiffView"; import { DiffAskOverlay, reconstructDiffContext } from "./DiffAsk"; +import { DiffFixBar, DiffFixProvider } from "./DiffFix"; import { parseUnifiedDiff, type DiffLine } from "./diff-parse"; /** @@ -76,6 +77,7 @@ export function AllChangesView({ projectPath }: { projectPath: string }) { }, [sections]); return ( + )} + + ); } diff --git a/src/git/DiffFix.test.ts b/src/git/DiffFix.test.ts new file mode 100644 index 0000000..841499d --- /dev/null +++ b/src/git/DiffFix.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from "bun:test"; +import { composeFixPrompt, type HunkRef } from "./DiffFix"; + +const hunk = (over: Partial): HunkRef => ({ + id: "src/foo.ts#0", + file: "src/foo.ts", + label: "", + snippet: "@@ -1,2 +1,2 @@\n-old\n+new", + ...over, +}); + +describe("composeFixPrompt", () => { + it("pairs each request with its diff snippet inside a fenced block", () => { + const prompt = composeFixPrompt([ + { ref: hunk({ label: "fn handleClick" }), text: "rename to onSelect" }, + ]); + expect(prompt).toContain("### Change 1 — src/foo.ts (in fn handleClick)"); + expect(prompt).toContain("```diff\n@@ -1,2 +1,2 @@\n-old\n+new\n```"); + expect(prompt).toContain("Requested: rename to onSelect"); + expect(prompt).toContain("Make these edits now."); + }); + + it("omits the scope suffix when the hunk has no label", () => { + const prompt = composeFixPrompt([{ ref: hunk({}), text: "handle null" }]); + expect(prompt).toContain("### Change 1 — src/foo.ts\n"); + expect(prompt).not.toContain("(in "); + }); + + it("numbers multiple changes in order", () => { + const prompt = composeFixPrompt([ + { ref: hunk({ id: "a#0", file: "a.ts" }), text: "first" }, + { ref: hunk({ id: "b#0", file: "b.ts" }), text: "second" }, + ]); + expect(prompt).toContain("### Change 1 — a.ts"); + expect(prompt).toContain("### Change 2 — b.ts"); + expect(prompt.indexOf("Change 1")).toBeLessThan(prompt.indexOf("Change 2")); + }); + + it("trims surrounding whitespace from the user's request", () => { + const prompt = composeFixPrompt([ + { ref: hunk({}), text: " spaced out \n" }, + ]); + expect(prompt).toContain("Requested: spaced out"); + }); +}); diff --git a/src/git/DiffFix.tsx b/src/git/DiffFix.tsx new file mode 100644 index 0000000..56ac3de --- /dev/null +++ b/src/git/DiffFix.tsx @@ -0,0 +1,552 @@ +import { + createContext, + useContext, + useEffect, + useMemo, + useRef, + useState, + type ReactNode, +} from "react"; +import { invoke } from "@tauri-apps/api/core"; +import { AnimatePresence, motion } from "motion/react"; +import { MagicWand, Minus, NotePencil, Plus, X } from "@phosphor-icons/react"; +import { useAppDispatch, useAppState } from "@/state/AppState"; +import { useToast } from "@/primitives/Toast"; +import type { AppState, TerminalTab, Worktree } from "@/state/types"; + +/** + * "Fix this hunk" — a review-and-delegate layer over the working diff. + * + * Each edited block in a working-tree diff (a contiguous run of `+`/`-` + * lines) grows a `+` button in the left gutter; clicking it opens an + * inline box under that block where the user types what they want + * changed about that part of the code. Comments accumulate across blocks + * and files, and a floating "Fix" bar composes them into one prompt — + * each request paired with the exact diff snippet it refers to — and + * pastes it straight into the worktree's agent terminal, which then goes + * and implements them. + * + * The plumbing is a React context: `DiffFixProvider` owns the comment + * state and the target worktree's cwd; `DiffBody` reads the context to + * decide whether to render `+` buttons at all (so historical commit + * diffs, which mount without a provider, stay read-only); `DiffFixBar` + * reads it to build and send the prompt. + */ + +/** A single hunk the user can attach a change request to. */ +export interface HunkRef { + /** Stable within one diff render: `${file}#${ordinal}`. */ + id: string; + file: string; + /** Enclosing scope git tacked onto the `@@` header, if any. */ + label: string; + /** The hunk rebuilt as a unified-diff snippet, handed to the agent. */ + snippet: string; +} + +interface DiffFixApi { + isOpen: (id: string) => boolean; + hasText: (id: string) => boolean; + /** Open a box for this hunk (idempotent — keeps existing text). */ + openBox: (ref: HunkRef) => void; + /** Close a box and drop its text. */ + closeBox: (id: string) => void; + getText: (id: string) => string; + setText: (id: string, text: string) => void; + /** Open boxes with non-empty text, in file+hunk order. */ + entries: () => Array<{ ref: HunkRef; text: string }>; + /** Count of non-empty comments — drives the Fix bar. */ + count: number; + clear: () => void; + /** Worktree checkout dir this diff belongs to. */ + cwd: string; +} + +const DiffFixContext = createContext(null); + +/** Diff-fix API for the enclosing view, or null when there is no + * provider (e.g. a historical commit diff — read-only, no `+`). */ +export function useDiffFix(): DiffFixApi | null { + return useContext(DiffFixContext); +} + +export function DiffFixProvider({ + cwd, + children, +}: { + cwd: string; + children: ReactNode; +}) { + // `open` maps hunk id → its ref (so we can rebuild the prompt without + // re-deriving snippets); `text` maps hunk id → the user's request. + const [open, setOpen] = useState>({}); + const [text, setText] = useState>({}); + + const api = useMemo(() => { + const orderKey = (ref: HunkRef) => { + const hash = ref.id.lastIndexOf("#"); + const ord = hash >= 0 ? Number(ref.id.slice(hash + 1)) : 0; + return [ref.file, Number.isFinite(ord) ? ord : 0] as const; + }; + const activeRefs = Object.values(open).filter( + (ref) => (text[ref.id]?.trim().length ?? 0) > 0, + ); + return { + isOpen: (id) => id in open, + hasText: (id) => id in open && (text[id]?.trim().length ?? 0) > 0, + openBox: (ref) => + setOpen((o) => (o[ref.id] ? o : { ...o, [ref.id]: ref })), + closeBox: (id) => { + setOpen((o) => { + if (!(id in o)) return o; + const { [id]: _drop, ...rest } = o; + return rest; + }); + setText((t) => { + if (!(id in t)) return t; + const { [id]: _drop, ...rest } = t; + return rest; + }); + }, + getText: (id) => text[id] ?? "", + setText: (id, value) => setText((s) => ({ ...s, [id]: value })), + entries: () => + activeRefs + .slice() + .sort((a, b) => { + const [fa, oa] = orderKey(a); + const [fb, ob] = orderKey(b); + return fa === fb ? oa - ob : fa < fb ? -1 : 1; + }) + .map((ref) => ({ ref, text: text[ref.id] ?? "" })), + count: activeRefs.length, + clear: () => { + setOpen({}); + setText({}); + }, + cwd, + }; + }, [open, text, cwd]); + + return ( + {children} + ); +} + +/* ------------------------------------------------------------------ + `+` button — sits in the left gutter beside each change block. + ------------------------------------------------------------------ */ + +export function HunkAddButton({ + hunkRef, + fix, +}: { + hunkRef: HunkRef; + fix: DiffFixApi; +}) { + const active = fix.isOpen(hunkRef.id); + const commented = fix.hasText(hunkRef.id); + const lit = active || commented; + const [hover, setHover] = useState(false); + return ( + + ); +} + +/* ------------------------------------------------------------------ + Inline comment box — rendered in its own row under the hunk. + ------------------------------------------------------------------ */ + +export function HunkCommentBox({ + hunkRef, + fix, +}: { + hunkRef: HunkRef; + fix: DiffFixApi; +}) { + const ref = useRef(null); + const value = fix.getText(hunkRef.id); + + // Focus on mount; auto-grow to fit content. + useEffect(() => { + const el = ref.current; + if (!el) return; + el.focus(); + el.setSelectionRange(el.value.length, el.value.length); + }, []); + useEffect(() => { + const el = ref.current; + if (!el) return; + el.style.height = "auto"; + el.style.height = `${Math.min(el.scrollHeight, 160)}px`; + }, [value]); + + return ( + + +