From 716f17c76f203c3a121086dc24618fe13fb3573e Mon Sep 17 00:00:00 2001 From: Daniels-Main Date: Sun, 6 Sep 2026 15:43:06 +0200 Subject: [PATCH 01/20] fix: honor Git hooks for every commit and amend --- PRD.md | 2 +- README.md | 3 +- ROADMAP.md | 9 + TASKS.md | 5 +- crates/strand-core/src/commit.rs | 179 +++++++++--------- crates/strand-core/src/git_output.rs | 59 ++++++ crates/strand-core/src/lib.rs | 1 + crates/strand-tauri/src/commands.rs | 6 +- ...-identity-signing-validation-2026-09-06.md | 35 ++++ docs/learnings.md | 8 +- ui/src/demo/dispatch.ts | 2 +- ui/src/lib/types.ts | 1 + ui/src/stores/commitDrafts.ts | 25 +++ ui/src/stores/repo.test.ts | 32 ++++ ui/src/stores/repo.ts | 27 ++- ui/src/styles/features.css | 5 + ui/src/views/LocalChanges.tsx | 26 +-- website/docs/everyday-git.md | 6 + website/docs/settings.md | 2 +- 19 files changed, 318 insertions(+), 115 deletions(-) create mode 100644 crates/strand-core/src/git_output.rs create mode 100644 docs/hooks-identity-signing-validation-2026-09-06.md create mode 100644 ui/src/stores/commitDrafts.ts diff --git a/PRD.md b/PRD.md index 91716aa7..764ac5f2 100644 --- a/PRD.md +++ b/PRD.md @@ -58,7 +58,7 @@ It will be built as a **Tauri 2** application with a **Rust** git backend and a | App shell | **Tauri 2** | True cross-platform, small bundles (~10MB vs Electron's ~100MB+), native webview per OS, signed installers built-in. | | Backend language | **Rust** | Speed, safety, and a great Git ecosystem (`gix`, `git2`). Pairs natively with Tauri. | | Git engine (read) | **`gix` (gitoxide)** | Pure-Rust, modern, dramatically faster than libgit2 for log/diff/status on large repos. | -| Git engine (write) | **`git2` (libgit2)** + shell-out to `git` | Use `git2` for commit/branch/merge/rebase where stable. Shell out to the user's `git` binary for ops that need it: interactive rebase, GPG signing, Git LFS, Git-flow, hooks. This is what Sublime Merge and Tower do — it's the right call. | +| Git engine (write) | **`git2` (libgit2)** + shell-out to `git` | Use `git2` for index/branch writes where stable. Commit/amend always use system Git for hook parity. Shell out to the user's `git` binary for ops that need it: interactive rebase, GPG signing, Git LFS, Git-flow, hooks. This is what Sublime Merge and Tower do — it's the right call. | | Frontend | **React + TypeScript** | Required by `@pierre/diffs` and `@pierre/trees`. | | Diff & code rendering | **`@pierre/diffs`** | Split/stacked diffs, merge conflict UI, line selection, annotations, Shiki themes. Covers most of §6.3. | | File tree | **`@pierre/trees`** | Virtualized (handles 100k+ files), Git status badges built in, drag-and-drop, search, keyboard nav, accessible. Covers §6.5. | diff --git a/README.md b/README.md index 40ac8df7..938aa5af 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,8 @@ the resolved app appearance automatically. (fetch-first for remote bases) and copies gitignored setup files listed in `.worktreeinclude` (`.env`, local settings) so agents can run out of the box. Stale entries whose directories are already gone prune immediately. -- **Everyday Git** — stage, unstage, or recoverably discard whole change +- **Everyday Git** — hook-aware signed/unsigned commit and amend with checkout + session drafts and bounded output; stage, unstage, or recoverably discard whole change blocks or individually selected lines inline in the diff; bulk tree actions include every selected file and every changed file beneath selected folders; initialize a repository with an initial branch, optional diff --git a/ROADMAP.md b/ROADMAP.md index f048b0bf..64995455 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2810,6 +2810,15 @@ GitHub/Azure review, Workbench and performance work retain their own status. --- +**Commit hook parity shipped (2026-09-06, F01):** Signed and unsigned commit/amend +now run system Git’s applicable hooks and honor custom hooksPath, rejection +and rewritten messages. Checkout session drafts survive rejection/navigation; +expandable output retains bounded diagnostics. Native Ctrl+Enter flows and +core/store regressions passed; the loaded-host no-hook cost is recorded in +`docs/hooks-identity-signing-validation-2026-09-06.md`. Index/status paths are +unchanged; the commit-policy exception is explicit in PRD and learnings. + + ## Cross-cutting tracks (run in parallel with all milestones) **Performance audit kick (2026-09-06):** Rechecked `main` at `8e83c8c` on diff --git a/TASKS.md b/TASKS.md index 4c7910bf..4325929d 100644 --- a/TASKS.md +++ b/TASKS.md @@ -89,10 +89,11 @@ Detailed comparison and sequencing: [`docs/git-client-1.0-audit.md`](./docs/git- (`docs/git-client-feature-audit-2026-09-06.md`: 19 missing/partial feature families, code evidence, priorities, fallbacks, and acceptance criteria). Priorities below are current recommendations, not historical PRD release gates. -- ☐ **F01 / P1 — Hook parity for unsigned commit/amend.** Resolve the recorded +- ☑ **F01 / P1 — Hook parity for unsigned commit/amend.** Resolve the recorded git2 commit-policy versus Git-hook contract tension; honor `core.hooksPath`, rejection and message rewriting, preserve drafts and bounded diagnostics, - and measure the no-hook path (`commit.rs`; signed commits already use Git). + and measure the no-hook path (`Repo::commit`, bounded `git_output`, checkout + `commitDrafts`; evidence in `docs/hooks-identity-signing-validation-2026-09-06.md`). - ☐ **F02 / P1 — Effective repository identity and scoped overrides.** Show the current author/committer identity, set/remove repo-local name/email without changing global/conditional config, and verify linked worktrees. diff --git a/crates/strand-core/src/commit.rs b/crates/strand-core/src/commit.rs index d99081c7..56f63da8 100644 --- a/crates/strand-core/src/commit.rs +++ b/crates/strand-core/src/commit.rs @@ -12,94 +12,32 @@ pub struct CommitOutcome { pub oid: String, /// Whether this commit was an amend of the previous HEAD. pub amended: bool, + /// Bounded stdout and stderr, including successful hook diagnostics. + pub output: String, } -/// Whether the repo's effective config asks for signed commits -/// (`commit.gpgSign = true`). Read through a snapshot for a consistent merged -/// view (system + global + local), like [`gitconfig`](crate::gitconfig); git2 -/// config keys are case-insensitive, so the lowercase lookup matches any -/// spelling. -fn signing_enabled(repo: &git2::Repository) -> bool { - repo.config() - .and_then(|mut c| c.snapshot()) - .and_then(|s| s.get_bool("commit.gpgsign")) - .unwrap_or(false) -} - -/// Write the current index as a new commit on HEAD. -/// -/// Two paths: when `commit.gpgSign` is off (the default) we commit in-process -/// via git2; when it's on we shell out to the user's `git` instead, because -/// git2 never signs — the shell-out picks up the user's gpg/ssh signing -/// config, `gpg.format`, and key lookup for free. +/// Commit/amend always use system Git: it owns hooks (including hooksPath), +/// identity, merge parents, signing and message rewrites. Index edits stay on +/// git2. This deliberately supersedes the old unsigned git2 fast path. impl Repo { pub fn commit(&self, subject: &str, body: Option<&str>, amend: bool) -> Result { - let repo = self.git2()?; - let message = match body.map(str::trim).filter(|b| !b.is_empty()) { Some(b) => format!("{}\n\n{}\n", subject.trim(), b), None => format!("{}\n", subject.trim()), }; - - let oid = if signing_enabled(repo) { - self.commit_via_git(&message, amend)?; - repo.head()?.peel_to_commit()?.id() - } else { - let sig = repo.signature()?; - let mut index = repo.index()?; - let tree_oid = index.write_tree()?; - let tree = repo.find_tree(tree_oid)?; - - if amend { - let head = repo.head()?; - let head_commit = head.peel_to_commit()?; - // Author `None` keeps the original author (git2 reuses the - // existing field), matching real `git commit --amend` and the - // shell-out path; only the committer is the current user. - head_commit.amend( - Some("HEAD"), - None, - Some(&sig), - None, - Some(&message), - Some(&tree), - )? - } else { - // Parent list: HEAD if it exists; empty for the initial commit. - let parents: Vec = match repo.head() { - Ok(h) => vec![h.peel_to_commit()?], - Err(_) => Vec::new(), - }; - let parent_refs: Vec<&git2::Commit> = parents.iter().collect(); - repo.commit(Some("HEAD"), &sig, &sig, &message, &tree, &parent_refs)? - } - }; - - Ok(CommitOutcome { - oid: oid.to_string(), - amended: amend, - }) + let output = self.commit_via_git(&message, amend)?; + let oid = self.git2()?.head()?.peel_to_commit()?.id().to_string(); + Ok(CommitOutcome { oid, amended: amend, output }) } - /// Commit the staged index by shelling out to the user's `git` — the - /// signing path, since git2 cannot sign. The message goes through a temp - /// file (`-F`, with `--cleanup=verbatim`: the file is built by us and - /// already exact, so verbatim keeps `#` lines AND byte parity with the - /// git2 path, which never cleans) to dodge platform quoting. Unlike the - /// git2 path this runs the user's hooks (pre-commit / commit-msg) — that - /// matches plain `git commit` and is the same accepted trust boundary the - /// other shell-out ops have (PRD §10). - fn commit_via_git(&self, message: &str, amend: bool) -> Result<()> { + fn commit_via_git(&self, message: &str, amend: bool) -> Result { let file = temp_message_file(message)?; let file_arg = file.to_string_lossy().into_owned(); let mut args = vec!["commit", "-F", file_arg.as_str(), "--cleanup=verbatim"]; - if amend { - args.push("--amend"); - } + if amend { args.push("--amend"); } let res = run_git(&self.path, &args); - // Best effort, on the error path too — a leak here is only temp litter. let _ = std::fs::remove_file(&file); - res.map(|_| ()) + res } } @@ -141,7 +79,7 @@ fn temp_message_file(message: &str) -> Result { /// (not a `Repo` method) so it doesn't collide with `stash`'s same-named /// helper on the same type. fn run_git(cwd: &Path, args: &[&str]) -> Result { - let out = crate::git_command() + let out = crate::git_output::capture(crate::git_command() .current_dir(cwd) .env("GIT_TERMINAL_PROMPT", "0") // Detach stdin so git can never block reading from a TTY/pipe we don't @@ -149,8 +87,8 @@ fn run_git(cwd: &Path, args: &[&str]) -> Result { .stdin(std::process::Stdio::null()) // Neutralize repo-local config that would run code as a side effect. .args(crate::GIT_SAFE_CONFIG) - .args(args) - .output() + .env("GIT_EDITOR", ":") + .args(args)) .map_err(|e| Error::Other(format!("spawn git failed: {e}")))?; if !out.status.success() { let stdout = String::from_utf8_lossy(&out.stdout); @@ -162,7 +100,7 @@ fn run_git(cwd: &Path, args: &[&str]) -> Result { combined })); } - Ok(String::from_utf8_lossy(&out.stdout).trim().to_string()) + Ok(format!("{}{}", String::from_utf8_lossy(&out.stdout), String::from_utf8_lossy(&out.stderr)).trim().to_string()) } #[cfg(test)] @@ -185,6 +123,8 @@ mod tests { git(&dir, &["init", "-q", "-b", "main"]); git(&dir, &["config", "user.name", "Test"]); git(&dir, &["config", "user.email", "test@example.com"]); + git(&dir, &["config", "commit.gpgsign", "false"]); + git(&dir, &["config", "core.hooksPath", ".git/hooks"]); (Repo::discover(dir.to_str().unwrap()).unwrap(), dir) } @@ -204,18 +144,85 @@ mod tests { git(dir, &["add", file]); } + fn hook(dir: &Path, name: &str, script: &str) { + std::fs::create_dir_all(dir).unwrap(); + let path = dir.join(name); + std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); + #[cfg(unix)] { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + } + #[test] - fn signing_enabled_defaults_off_and_follows_config() { + fn hooks_reject_rewrite_and_run_after_commit_and_amend() { let (repo, dir) = scratch_repo(); - let g2 = repo.git2().unwrap(); - assert!(!signing_enabled(&g2), "unset ⇒ off"); - - git(&dir, &["config", "commit.gpgsign", "true"]); - assert!(signing_enabled(&repo.git2().unwrap())); + stage(&dir, "a.txt", "a\n"); + let hooks = dir.join("custom hooks"); + git(&dir, &["config", "core.hooksPath", "custom hooks"]); + hook(&hooks, "pre-commit", "echo policy-rejected >&2; exit 1"); + let error = repo.commit("draft", Some("body"), false).unwrap_err().to_string(); + assert!(error.contains("policy-rejected")); + assert!(repo.git2().unwrap().head().is_err()); + assert_eq!(git(&dir, &["diff", "--cached", "--name-only"]), "a.txt"); + hook(&hooks, "pre-commit", "echo pre-commit-ok"); + hook(&hooks, "prepare-commit-msg", "echo prepared >> \"$1\""); + hook(&hooks, "commit-msg", "echo rewritten >> \"$1\""); + hook(&hooks, "post-commit", "echo post-commit-ok >&2"); + hook(&hooks, "post-rewrite", "read old new; echo post-rewrite-$1-$old-$new >&2"); + let outcome = repo.commit("draft", Some("body"), false).unwrap(); + assert!(outcome.output.contains("pre-commit-ok")); + assert!(outcome.output.contains("post-commit-ok")); + assert_eq!(git(&dir, &["log", "-1", "--format=%B"]), "draft\n\nbody\nprepared\nrewritten"); + hook(&hooks, "commit-msg", "echo message-rejected >&2; exit 1"); + assert!(repo.commit("amend draft", None, true).unwrap_err().to_string().contains("message-rejected")); + assert_eq!(git(&dir, &["rev-parse", "HEAD"]), outcome.oid); + hook(&hooks, "commit-msg", "echo amended >> \"$1\""); + let amended = repo.commit("amend draft", None, true).unwrap(); + assert!(amended.output.contains(&format!("post-rewrite-amend-{}-{}", outcome.oid, amended.oid))); + assert_eq!(git(&dir, &["rev-list", "--count", "HEAD"]), "1"); + let _ = std::fs::remove_dir_all(dir); + } - git(&dir, &["config", "commit.gpgsign", "false"]); - assert!(!signing_enabled(&repo.git2().unwrap())); + #[test] + fn hook_output_is_bounded_and_preserves_final_failure() { + let (repo, dir) = scratch_repo(); + stage(&dir, "a.txt", "a\n"); + hook(&dir.join(".git/hooks"), "pre-commit", "i=0; while [ $i -lt 3000 ]; do echo verbose-hook-output; echo verbose-stderr >&2; i=$((i+1)); done; echo final-rejection >&2; exit 1"); + let error = repo.commit("draft", None, false).unwrap_err().to_string(); + assert!(error.len() < 34 * 1024); + assert!(error.contains("output truncated")); + assert!(error.ends_with("final-rejection")); + let _ = std::fs::remove_dir_all(dir); + } + #[test] + #[ignore = "manual no-hook latency measurement"] + fn measure_no_hook_commit_path() { + let (repo, dir) = scratch_repo(); + let mut samples = Vec::new(); + for i in 0..25 { + stage(&dir, "a.txt", &format!("{i}\n")); + let start = std::time::Instant::now(); + repo.commit("measurement", None, false).unwrap(); + samples.push(start.elapsed().as_secs_f64() * 1000.0); + } + let mut previous = Vec::new(); + for i in 0..25 { + stage(&dir, "a.txt", &format!("old-{i}\n")); + let start = std::time::Instant::now(); + let g2 = repo.git2().unwrap(); + let sig = g2.signature().unwrap(); + let tree_oid = g2.index().unwrap().write_tree().unwrap(); + let tree = g2.find_tree(tree_oid).unwrap(); + let parent = g2.head().unwrap().peel_to_commit().unwrap(); + g2.commit(Some("HEAD"), &sig, &sig, "measurement", &tree, &[&parent]).unwrap(); + previous.push(start.elapsed().as_secs_f64() * 1000.0); + } + previous.sort_by(f64::total_cmp); + println!("previous git2 path: median {:.2}ms, p95 {:.2}ms (25 iterations)", previous[12], previous[23]); + samples.sort_by(f64::total_cmp); + println!("no-hook Git commit: median {:.2}ms, p95 {:.2}ms (25 iterations)", samples[12], samples[23]); let _ = std::fs::remove_dir_all(dir); } @@ -273,7 +280,7 @@ mod tests { stage(&dir, "a.txt", "a\n"); repo.commit("original", None, false).unwrap(); - // Same parity check for the git2 path: a different configured user + // A different configured user // amends, the original author survives, the committer updates. git(&dir, &["config", "user.name", "Other"]); git(&dir, &["config", "user.email", "other@example.com"]); diff --git a/crates/strand-core/src/git_output.rs b/crates/strand-core/src/git_output.rs new file mode 100644 index 00000000..5adb7b7c --- /dev/null +++ b/crates/strand-core/src/git_output.rs @@ -0,0 +1,59 @@ +//! Bounded pipe capture for user-triggered Git commands. Drain both pipes to +//! EOF even after the limit, retaining the start and final diagnostics. + +use std::io::Read; +use std::process::{Command, Output, Stdio}; + +const HALF_LIMIT: usize = 8 * 1024; + +fn drain(mut pipe: impl Read) -> std::io::Result> { + let mut head = Vec::new(); + let mut tail = Vec::new(); + let mut total = 0; + let mut buffer = [0; 8192]; + loop { + let n = pipe.read(&mut buffer)?; + if n == 0 { break; } + total += n; + let split = n.min(HALF_LIMIT - head.len()); + head.extend_from_slice(&buffer[..split]); + tail.extend_from_slice(&buffer[split..n]); + if tail.len() > HALF_LIMIT { + tail.drain(..tail.len() - HALF_LIMIT); + } + } + if total > HALF_LIMIT * 2 { + head.extend_from_slice(b"\n[output truncated; final diagnostics follow]\n"); + } + head.extend(tail); + Ok(head) +} + +pub(crate) fn capture(command: &mut Command) -> crate::Result { + let mut child = command.stdin(Stdio::null()) + .stdout(Stdio::piped()).stderr(Stdio::piped()).spawn()?; + let stdout = child.stdout.take().expect("piped stdout"); + let stderr = child.stderr.take().expect("piped stderr"); + let reader = std::thread::spawn(move || drain(stdout)); + let stderr = drain(stderr); + let status = child.wait()?; + let stdout = reader.join().map_err(|_| crate::Error::Other("Git output reader failed".into()))??; + Ok(Output { status, stdout, stderr: stderr? }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn retains_start_and_end_without_unbounded_allocation() { + let mut bytes = b"start\n".to_vec(); + bytes.extend(vec![b'x'; 1024 * 1024]); + bytes.extend_from_slice(b"\nfinal error"); + let output = drain(bytes.as_slice()).unwrap(); + assert!(output.len() < 17 * 1024); + assert!(output.starts_with(b"start\n")); + assert!(output.ends_with(b"\nfinal error")); + assert!(String::from_utf8(output).unwrap().contains("output truncated")); + } +} diff --git a/crates/strand-core/src/lib.rs b/crates/strand-core/src/lib.rs index e2dc385a..8c48a803 100644 --- a/crates/strand-core/src/lib.rs +++ b/crates/strand-core/src/lib.rs @@ -31,6 +31,7 @@ pub mod maintenance; pub mod conflict; pub mod external; pub mod gitconfig; +mod git_output; pub mod history; pub mod ignore; pub mod stash; diff --git a/crates/strand-tauri/src/commands.rs b/crates/strand-tauri/src/commands.rs index e0ec2642..23fed03a 100644 --- a/crates/strand-tauri/src/commands.rs +++ b/crates/strand-tauri/src/commands.rs @@ -899,13 +899,15 @@ pub fn repo_apply_patch(path: String, patch: String, target: String) -> CmdResul } #[tauri::command(async)] -pub fn repo_commit( +pub async fn repo_commit( path: String, subject: String, body: Option, amend: bool, ) -> CmdResult { - Ok(Repo::discover(&path)?.commit(&subject, body.as_deref(), amend)?) + run_blocking("commit", move || { + Ok(Repo::discover(&path)?.commit(&subject, body.as_deref(), amend)?) + }).await } // Network commands run on a blocking thread (they shell out to `git`, which diff --git a/docs/hooks-identity-signing-validation-2026-09-06.md b/docs/hooks-identity-signing-validation-2026-09-06.md new file mode 100644 index 00000000..50be4ec0 --- /dev/null +++ b/docs/hooks-identity-signing-validation-2026-09-06.md @@ -0,0 +1,35 @@ +# F01–F03 validation — 2026-09-06 + +Base: `263ebe6` (PR #114). Windows, system Git, isolated worktree and fixtures. + +## F01 — commit hooks + +Commit and amend always use system Git. This explicitly supersedes the old +unsigned git2 path documented in learnings; index operations remain on git2. +Git owns hook lookup, `core.hooksPath`, rejection, message rewrites, signing, +merge parents and amend attribution. The command runs on the blocking pool. +Stdout and stderr are drained concurrently, keeping each stream’s first/last +8 KiB with an explicit truncation marker. Drafts are checkout-keyed and survive +failed operations and view/repository changes during the session. + +Evidence: +- Core fixtures: custom hooksPath, rejecting pre-commit and commit-msg, + prepare-commit-msg/commit-msg rewriting, post-commit output, amend post-rewrite + old/new OIDs, preserved index/HEAD on rejection, bounded verbose output, + attribution on amend and missing SSH signing key failure. +- Store/shortcut tests: 12 passed, including rejected-hook index refresh, + completed commit with failed refresh, and repository-switch response handling. +- Frontend TypeScript: passed. +- Isolated native WebView2: Ctrl+Enter rejection kept subject/body; switching + to Commits and back retained the draft; retry ran message rewriting and + exposed successful hook output. Verified the resulting commit message with + Git. Screenshots retained under `target/verify-f010203/f01-*.png` (local only). +- Manual 25-iteration debug measurement, while other tasks were compiling: + system Git median **529.01 ms**, p95 **797.32 ms**; former git2 algorithm median + **24.50 ms**, p95 **97.98 ms**. This is an explicit correctness cost on commit, + not a status/staging hot-path change or an idle performance certification. + Reproduce with `cargo test -p strand-core measure_no_hook_commit_path -- + --ignored --nocapture`. Both measurements exclude staging. + +Git contracts: [hooks](https://git-scm.com/docs/githooks), +[commit](https://git-scm.com/docs/git-commit). diff --git a/docs/learnings.md b/docs/learnings.md index e50db001..ef97422c 100644 --- a/docs/learnings.md +++ b/docs/learnings.md @@ -359,7 +359,13 @@ the existing pattern better than a forced shared abstraction. behaviour matters more than staying pure-git2: **conflicts** (git leaves markers + the in-progress state on disk), **GPG/SSH signing**, and **hooks** — none of which git2's `merge`/`cherrypick`/`revert` do for free, and git2 has no rebase -driver. Index/commit ops still use git2. After any history op, the store refresh +driver. Index operations still use git2; commit/amend always use system Git +(F01, 2026-09-06), including unsigned commits, so Git owns hook discovery, +rejection, message rewriting, merge parents and effective identity. No hook +existence shortcut: conditional/worktree config and installed hooks can change +between operations. Capture bounded stdout/stderr, preserve checkout drafts on +failure, and do not report post-success refresh errors as commit failures. +After any history op, the store refresh tail is meta + local-changes + log + refs (`refreshAfterHistoryOp`), and a paused op is detected via `Repo::operation_in_progress` reading `.git/` markers (`rebase-merge`/`rebase-apply`, `CHERRY_PICK_HEAD`, `REVERT_HEAD`, `MERGE_HEAD`, diff --git a/ui/src/demo/dispatch.ts b/ui/src/demo/dispatch.ts index 4298b70f..4157cbd2 100644 --- a/ui/src/demo/dispatch.ts +++ b/ui/src/demo/dispatch.ts @@ -209,7 +209,7 @@ export const handlers: Record = { repo_apply_patch: (a) => repo.applyPatchTo(wtOf(a), str(a.patch), a.target as 'index' | 'index_reverse' | 'workdir_reverse' | 'workdir'), repo_commit: (a) => { const c = repo.commitIndex(wtOf(a), str(a.subject), a.body == null ? null : str(a.body), Boolean(a.amend)); - return { oid: c.hash, amended: Boolean(a.amend) }; + return { oid: c.hash, amended: Boolean(a.amend), output: 'Demo commit created.' }; }, // ---- branches / tags / remotes ----------------------------------------- diff --git a/ui/src/lib/types.ts b/ui/src/lib/types.ts index c27be6a0..48e49034 100644 --- a/ui/src/lib/types.ts +++ b/ui/src/lib/types.ts @@ -162,6 +162,7 @@ export interface ReviewNote { export interface CommitOutcome { oid: string; amended: boolean; + output: string; } export interface UpstreamRef { diff --git a/ui/src/stores/commitDrafts.ts b/ui/src/stores/commitDrafts.ts new file mode 100644 index 00000000..85d07793 --- /dev/null +++ b/ui/src/stores/commitDrafts.ts @@ -0,0 +1,25 @@ +import { create } from 'zustand'; + +interface CommitDraft { + subject: string; + body: string; + amend: boolean; + submitting: boolean; + output: string; + error: string | null; +} + +export const emptyCommitDraft: CommitDraft = { + subject: '', body: '', amend: false, submitting: false, output: '', error: null, +}; + +/** Session drafts belong to a checkout, including while a hook is running. */ +export const useCommitDrafts = create<{ + drafts: Record; + patch(path: string, patch: Partial): void; +}>((set) => ({ + drafts: {}, + patch: (path, patch) => set((s) => ({ + drafts: { ...s.drafts, [path]: { ...(s.drafts[path] ?? emptyCommitDraft), ...patch } }, + })), +})); diff --git a/ui/src/stores/repo.test.ts b/ui/src/stores/repo.test.ts index 5d203f72..3a5fdf52 100644 --- a/ui/src/stores/repo.test.ts +++ b/ui/src/stores/repo.test.ts @@ -144,3 +144,35 @@ describe('AI review notes', () => { }]); }); }); + + +describe('commit outcome boundary', () => { + it('propagates a hook rejection and refreshes its index changes', async () => { + const refresh = vi.fn(async () => {}); + const failure = { message: 'commit-msg rejected' }; + vi.spyOn(tauri, 'repoCommit').mockRejectedValue(failure); + useRepo.setState({ activePath: '/repo', refreshLocalChanges: refresh }); + await expect(useRepo.getState().commit('draft', 'body', true)).rejects.toBe(failure); + expect(refresh).toHaveBeenCalledOnce(); + }); + + it('keeps a completed commit successful if refresh fails', async () => { + const outcome = { oid: 'abc', amended: false, output: 'hook accepted' }; + vi.spyOn(tauri, 'repoCommit').mockResolvedValue(outcome); + const refresh = vi.fn(async () => { throw new Error('refresh failed'); }); + useRepo.setState({ activePath: '/repo', refreshLocalChanges: refresh, + refreshLog: refresh, refreshStashes: refresh, refreshMeta: refresh, refreshRefs: refresh }); + await expect(useRepo.getState().commit('draft', null, false)).resolves.toEqual(outcome); + }); + + it('does not refresh a different checkout after a slow hook completes', async () => { + const refresh = vi.fn(async () => {}); + vi.spyOn(tauri, 'repoCommit').mockImplementation(async () => { + useRepo.setState({ activePath: '/other' }); + return { oid: 'abc', amended: false, output: '' }; + }); + useRepo.setState({ activePath: '/repo', refreshLocalChanges: refresh, refreshLog: refresh }); + await useRepo.getState().commit('draft', null, false); + expect(refresh).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/src/stores/repo.ts b/ui/src/stores/repo.ts index 44ec9ae4..844ad528 100644 --- a/ui/src/stores/repo.ts +++ b/ui/src/stores/repo.ts @@ -23,6 +23,7 @@ import type { BaseBranch, CodeReviewFinding, Commit, + CommitOutcome, BranchPushRequest, CommitSearchMode, FileDiff, @@ -431,7 +432,7 @@ export interface RepoState { loadRepoDiffMode(): Promise; stageAll(): Promise; unstageAll(): Promise; - commit(subject: string, body: string | null, amend: boolean): Promise; + commit(subject: string, body: string | null, amend: boolean): Promise; /** Re-read RepoMeta (branch, ahead/behind) for the active tab. */ refreshMeta(): Promise; @@ -1799,13 +1800,23 @@ export const useRepo = create((set, get) => ({ }, async commit(subject, body, amend) { const path = get().activePath; - if (!path) return; - await tauri.repoCommit(path, subject, body, amend); - await Promise.all([ - get().refreshLocalChanges(), - get().refreshLog(), - get().refreshStashes(), - ]); + if (!path) throw new Error('No repository selected.'); + let outcome: CommitOutcome; + try { + outcome = await tauri.repoCommit(path, subject, body, amend); + } catch (error) { + // Hooks can edit the index/worktree even when they reject the commit. + if (get().activePath === path) await get().refreshLocalChanges().catch(() => {}); + throw error; + } + if (get().activePath === path) { + // A failed refresh cannot turn a completed commit into a retryable failure. + await Promise.allSettled([ + get().refreshLocalChanges(), get().refreshLog(), get().refreshStashes(), + get().refreshMeta(), get().refreshRefs(), + ]); + } + return outcome; }, async refreshMeta() { diff --git a/ui/src/styles/features.css b/ui/src/styles/features.css index 03aa8eaa..52a72821 100644 --- a/ui/src/styles/features.css +++ b/ui/src/styles/features.css @@ -9542,3 +9542,8 @@ select.clone-input { .plugin-heroi-select-thinking, .plugin-heroi-select-permission { display: none; } } + +/* Git hook transcripts stay selectable and bounded in the commit form. */ +.cb-output, .cb-error { max-height: 160px; overflow: auto; white-space: pre-wrap; overflow-wrap: anywhere; } +.cb-output { font-size: var(--type-ui-sm); color: var(--text-2); } +.cb-output pre { white-space: pre-wrap; margin: 6px 0; } diff --git a/ui/src/views/LocalChanges.tsx b/ui/src/views/LocalChanges.tsx index e3d1d269..815fe685 100644 --- a/ui/src/views/LocalChanges.tsx +++ b/ui/src/views/LocalChanges.tsx @@ -1,3 +1,4 @@ +import { emptyCommitDraft, useCommitDrafts } from '../stores/commitDrafts'; import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { Panel, PanelGroup, PanelResizeHandle } from 'react-resizable-panels'; import { @@ -1667,12 +1668,15 @@ function CommitBar({ canCommit, hasChanges }: { canCommit: boolean; hasChanges: const openaiCli = useSettings((s) => s.openaiCli); const anthropicCli = useSettings((s) => s.anthropicCli); const platform = useSettings((s) => s.platform); - const [subject, setSubject] = useState(''); - const [body, setBody] = useState(''); - const [amend, setAmend] = useState(false); - const [submitting, setSubmitting] = useState(false); + const draftPath = activePath ?? ''; + const draft = useCommitDrafts((s) => s.drafts[draftPath] ?? emptyCommitDraft); + const patchDraft = useCommitDrafts((s) => s.patch); + const { subject, body, amend, submitting, output, error: commitError } = draft; + const setSubject = (subject: string) => patchDraft(draftPath, { subject }); + const setBody = (body: string) => patchDraft(draftPath, { body }); + const setAmend = (amend: boolean) => patchDraft(draftPath, { amend }); + const setCommitError = (error: string | null) => patchDraft(draftPath, { error }); const [suggesting, setSuggesting] = useState(false); - const [commitError, setCommitError] = useState(null); const [sensitivePrompt, setSensitivePrompt] = useState<{ fingerprint: string; files: AiSensitiveFile[]; @@ -1789,18 +1793,15 @@ function CommitBar({ canCommit, hasChanges }: { canCommit: boolean; hasChanges: const trimmed = subject.trim(); if (!trimmed || submitting) return; if (!canCommit && !amend) return; - setSubmitting(true); - setCommitError(null); + patchDraft(draftPath, { submitting: true, error: null, output: '' }); try { - await commit(trimmed, body.trim() || null, amend); - setSubject(''); - setBody(''); - setAmend(false); + const result = await commit(trimmed, body.trim() || null, amend); + patchDraft(draftPath, { subject: '', body: '', amend: false, output: result.output }); } catch (e) { console.error('commit failed', e); setCommitError(`Commit failed: ${gitErrorHint(e)}`); } finally { - setSubmitting(false); + patchDraft(draftPath, { submitting: false }); } } @@ -1904,6 +1905,7 @@ function CommitBar({ canCommit, hasChanges }: { canCommit: boolean; hasChanges: )} + {output &&
Commit output
{output}
} {commitError && (
{commitError} diff --git a/website/docs/everyday-git.md b/website/docs/everyday-git.md index cf6223f9..d49a2a4a 100644 --- a/website/docs/everyday-git.md +++ b/website/docs/everyday-git.md @@ -2,6 +2,12 @@ Strand is a complete daily-driver Git client alongside its review features. This page covers staging and committing in Local Changes, syncing with remotes, the sidebar's branch/tag/stash/remote/submodule sections, history operations like cherry-pick and interactive rebase, and conflict resolution. +Commit and amend run system Git’s hooks, including a configured `core.hooksPath`. +A rejecting hook leaves your subject, body and amend selection in the checkout’s +session draft. Drafts also survive view/repository switches. Failures show Git’s +diagnostics; successful commits offer expandable **Commit output**. Output keeps +the first and last 8 KiB of each stream when a hook is verbose. + ## Local Changes (`Mod+2`) Local Changes is a pure staging workspace: an Unstaged pane and a Staged pane (hierarchical file trees with status badges), a diff pane, and the commit form. diff --git a/website/docs/settings.md b/website/docs/settings.md index c3aa8e84..85a29033 100644 --- a/website/docs/settings.md +++ b/website/docs/settings.md @@ -44,7 +44,7 @@ Below the rebindable list, a **Context shortcuts** card documents the fixed, sur - **Global identity** — Name and Email inputs written to your global git config (`~/.gitconfig`) with an explicit **Save identity** button. This is the author identity for new commits everywhere, not just in Strand. - **Default clone & open folder** — a path with **Choose…** and **Clear** buttons. This is where the clone dialog and the open-repository picker start. -Everything else about git — credentials, SSH keys, commit signing — is inherited from your existing git setup: network operations (push, pull, fetch, clone) go through your system `git`, and when `commit.gpgSign` is on, commits do too — picking up your signing config and running your `pre-commit` / `commit-msg` hooks, just like plain `git commit`. Unsigned commits (the default) are written in-process and do not run commit hooks. There is nothing to configure in Strand for those. +Everything else about git — credentials, SSH keys, commit signing — is inherited from your existing git setup: network operations (push, pull, fetch, clone) go through your system `git`, and every commit/amend does too. Signed and unsigned commits honor hooks (including `core.hooksPath`), rejecting policies and message rewrites. A rejection preserves your checkout’s draft; expandable commit output retains bounded hook diagnostics. Signing continues to use your existing Git config and agents. ## Hosting From ecaedde580c39562cc8d8bf0134f2503709733f0 Mon Sep 17 00:00:00 2001 From: Daniels-Main Date: Sun, 6 Sep 2026 15:49:56 +0200 Subject: [PATCH 02/20] fix(hosted): paginate GitHub inbox and review connections --- README.md | 2 +- ROADMAP.md | 6 + TASKS.md | 8 +- crates/strand-tauri/src/commands.rs | 14 + crates/strand-tauri/src/main.rs | 3 + crates/strand-tauri/src/pull_requests.rs | 192 ++---- .../strand-tauri/src/pull_requests/pages.rs | 578 ++++++++++++++++++ docs/learnings.md | 9 + ui/src/App.tsx | 14 + ui/src/lib/pullRequestPages.test.ts | 55 ++ ui/src/lib/pullRequestPages.ts | 40 ++ ui/src/lib/tauri.ts | 7 + ui/src/lib/types.ts | 22 + ui/src/styles/features.css | 2 + ui/src/views/PullRequestDataLoader.tsx | 52 ++ ui/src/views/PullRequestInboxLoader.tsx | 47 ++ ui/src/views/PullRequests.tsx | 21 +- website/docs/pull-requests.md | 14 +- 18 files changed, 926 insertions(+), 160 deletions(-) create mode 100644 crates/strand-tauri/src/pull_requests/pages.rs create mode 100644 ui/src/lib/pullRequestPages.test.ts create mode 100644 ui/src/lib/pullRequestPages.ts create mode 100644 ui/src/views/PullRequestDataLoader.tsx create mode 100644 ui/src/views/PullRequestInboxLoader.tsx diff --git a/README.md b/README.md index 40ac8df7..f7bfa4e8 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,7 @@ the resolved app appearance automatically. Codex or Claude Code subscription can inspect that exact review set for possible defects. Findings stay pending until you explicitly add selected ones as severity-labelled notes; AI review never edits repository files. -- **Hosted pull requests** — browse the latest 100 GitHub or Azure DevOps PRs +- **Hosted pull requests** — browse GitHub PRs with cursor pages or the latest 100 Azure DevOps PRs for the active repository, with the active PR for your checked-out branch opening and being followed automatically even before the PR view is opened. Create a PR or draft for the checked-out branch from the toolbar or command diff --git a/ROADMAP.md b/ROADMAP.md index f048b0bf..252bb204 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2808,6 +2808,12 @@ implementation rows while the July audit is labeled historical. This is a planning update, not a claim that these features shipped; existing local Git, GitHub/Azure review, Workbench and performance work retain their own status. +**Hosted GitHub pagination shipped (2026-09-06, F06):** Inbox and activated +review data now use cursor pages, including independent thread replies and +checks. Partial counts, retry/cancel controls and palette actions preserve +loaded content and drafts. Page appends deduplicate provider IDs and reject +head changes; inbox navigation remains shallow and hosted patches remain lazy. + --- ## Cross-cutting tracks (run in parallel with all milestones) diff --git a/TASKS.md b/TASKS.md index 4c7910bf..c506e2e6 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1867,16 +1867,18 @@ tree: watch the agent work, review fast, accept or reject safely. batched submission use GitHub's atomic review payload or Azure's bounded latest-iteration/change-tracking resolver (`azure_review_coordinates`, `azure_server_review_coordinates`). - - ☐ **F06 / P1 — Complete large-PR pagination.** Paginate GitHub inbox, + - ☑ **F06 / P1 — Complete large-PR pagination.** Paginate GitHub inbox, reviews, threads/replies and check contexts beyond the current bounded queries; expose partial/error states, deduplicate pages and test 101+ - entries while keeping initial queries shallow. + entries while keeping initial queries shallow (`pull_requests::pages`, + `PullRequestDataLoader`, `PullRequestInboxLoader`; 34 Rust / 38 frontend + tests and WebView2 101-row/review, failure, cancellation and stale-head pass). - ☑ Batched review submission: pending comments plus Comment / Approve / Request changes, summary preview, exact-head stale guard, and draft preservation when a provider write fails (`pullRequestReview` drafts, `PullRequestChanges` review composer, `repo_pull_request_submit_review`). - ☑ Searchable repository PR inbox (`filterPullRequests`, `.pr-inbox-*`): - All, Authored, and Completed filter the shallow latest-100 list locally; + All, Authored, and Completed filter the loaded shallow inbox pages locally; search covers number/title/author/source/target branches; provider-account identity drives Authored without hiding All when identity lookup fails; selection, j/k/arrows/Home/End/Enter, focus restoration, and palette search diff --git a/crates/strand-tauri/src/commands.rs b/crates/strand-tauri/src/commands.rs index e0ec2642..95268ed2 100644 --- a/crates/strand-tauri/src/commands.rs +++ b/crates/strand-tauri/src/commands.rs @@ -392,6 +392,20 @@ pub async fn repo_pull_requests(path: String) -> CmdResult { .await } +/// One bounded provider page; cancellation never affects provider writes. +#[tauri::command(async)] +pub async fn repo_pull_request_inbox_page(path: String, cursor: Option, request_id: String) -> CmdResult { + run_blocking("pull request inbox page", move || pull_requests::pages::inbox(&path, cursor.as_deref(), &request_id).map_err(|message| CmdError { message })).await +} + +#[tauri::command(async)] +pub async fn repo_pull_request_data_page(path: String, id: u64, expected_head: String, request: pull_requests::pages::Cursor, request_id: String) -> CmdResult { + run_blocking("pull request data page", move || pull_requests::pages::read(&path, id, &expected_head, request, &request_id).map_err(|message| CmdError { message })).await +} + +#[tauri::command] +pub fn repo_pull_request_cancel_read(request_id: String) { pull_requests::pages::cancel(&request_id); } + /// Active pull request for one checked-out branch. This targeted query lets /// automatic following work without loading the full hosted-PR workspace. #[tauri::command(async)] diff --git a/crates/strand-tauri/src/main.rs b/crates/strand-tauri/src/main.rs index 0b3fad89..3a9a8815 100644 --- a/crates/strand-tauri/src/main.rs +++ b/crates/strand-tauri/src/main.rs @@ -187,6 +187,9 @@ fn main() { commands::azdo_profile_clear_pat, commands::azdo_profile_test, commands::repo_pull_requests, + commands::repo_pull_request_inbox_page, + commands::repo_pull_request_data_page, + commands::repo_pull_request_cancel_read, commands::repo_pull_request_for_branch, commands::repo_pull_request_create, commands::repo_pull_request_activity, diff --git a/crates/strand-tauri/src/pull_requests.rs b/crates/strand-tauri/src/pull_requests.rs index 9f08edc7..329ee911 100644 --- a/crates/strand-tauri/src/pull_requests.rs +++ b/crates/strand-tauri/src/pull_requests.rs @@ -26,6 +26,8 @@ use uuid::Uuid; use crate::ai::bin::{base_command, resolve_cli}; use crate::azdo_helper; +pub mod pages; + const COMMAND_TIMEOUT: Duration = Duration::from_secs(30); const MAX_COMMENT_BYTES: usize = 65_536; const MAX_THREAD_ID_BYTES: usize = 512; @@ -43,8 +45,8 @@ const GITHUB_LIST_FIELDS: &str = concat!( ); const GITHUB_DETAIL_FIELDS: &str = concat!( "number,title,state,isDraft,author,headRefName,baseRefName,createdAt,updatedAt,", - "closedAt,mergedAt,url,body,mergeStateStatus,reviewDecision,comments,commits,additions,deletions,", - "changedFiles,reviewRequests,latestReviews,labels,statusCheckRollup,headRefOid" + "closedAt,mergedAt,url,body,mergeStateStatus,reviewDecision,additions,deletions,", + "changedFiles,reviewRequests,labels,headRefOid" ); const GITHUB_ACTIVITY_QUERY: &str = r#"query($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { @@ -57,6 +59,7 @@ const GITHUB_ACTIVITY_QUERY: &str = r#"query($owner: String!, $repo: String!, $n } statusCheckRollup { contexts(first: 100) { + pageInfo { hasNextPage endCursor } nodes { __typename ... on CheckRun { databaseId name status conclusion } @@ -67,44 +70,6 @@ const GITHUB_ACTIVITY_QUERY: &str = r#"query($owner: String!, $repo: String!, $n } } }"#; -const GITHUB_REVIEW_THREADS_QUERY: &str = r#"query($owner: String!, $repo: String!, $number: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $number) { - viewerCanUpdate - reviews(last: 100) { - nodes { - id - body - state - submittedAt - url - viewerCanUpdate - viewerDidAuthor - author { login avatarUrl } - } - } - reviewThreads(first: 100) { - nodes { - id - isResolved - isOutdated - viewerCanReply - viewerCanResolve - viewerCanUnresolve - path - line - startLine - originalLine - originalStartLine - diffSide - comments(first: 100) { - nodes { id body createdAt url author { login avatarUrl } } - } - } - } - } - } -}"#; const GITHUB_REVIEW_UPDATE_MUTATION: &str = r#"mutation($reviewId: ID!, $body: String!) { updatePullRequestReview(input: { pullRequestReviewId: $reviewId, body: $body }) { pullRequestReview { id } @@ -210,6 +175,7 @@ pub struct PullRequestReview { #[derive(Debug, Clone, Serialize)] pub struct PullRequestCheck { + pub id: String, pub name: String, pub status: String, } @@ -269,6 +235,7 @@ struct AzureDiscussion { #[derive(Debug, Clone, Serialize)] pub struct PullRequest { + pub data_pages: Vec, pub id: u64, pub title: String, pub state: String, @@ -303,6 +270,8 @@ pub struct PullRequest { #[derive(Debug, Clone, Serialize)] pub struct PullRequestList { + pub next_cursor: Option, + pub total_count: Option, pub repository: PullRequestRepository, pub pull_requests: Vec, } @@ -911,49 +880,8 @@ fn host_for_path(path: &str) -> Result<(String, HostRepo)> { } fn list_github(cwd: &str, remote: String, owner: String, repo: String) -> Result { - let slug = format!("{owner}/{repo}"); - // Keep the list query shallow. Asking GraphQL to expand nested comments, - // commits, reviews, and checks across 100 PRs can exceed GitHub's 500k - // possible-node cap even for a modest repository. Rich fields load only - // for the selected PR via `detail_github`. - let (output, viewer) = thread::scope(|scope| { - let viewer = scope.spawn(|| github_viewer(cwd)); - let output = run_command( - cwd, - "gh", - &[ - "pr", - "list", - "--repo", - &slug, - "--state", - "all", - "--limit", - "100", - "--json", - GITHUB_LIST_FIELDS, - ], - &[("GH_PROMPT_DISABLED", "1")], - ); - let viewer = viewer.join().ok().and_then(Result::ok); - (output, viewer) - }); - let output = output?; - let values: Vec = serde_json::from_slice(&output) - .map_err(|e| format!("GitHub CLI returned invalid JSON: {e}"))?; - let pull_requests = values - .iter() - .filter_map(|value| parse_github_pr(value, viewer.as_deref())) - .collect(); - Ok(PullRequestList { - repository: PullRequestRepository { - provider: PullRequestProvider::GitHub, - remote, - label: slug, - viewer, - }, - pull_requests, - }) + let _ = (remote, owner, repo); + pages::inbox(cwd, None, &Uuid::new_v4().to_string()) } fn for_branch_github( @@ -1000,16 +928,6 @@ fn for_branch_github( })) } -fn github_viewer(cwd: &str) -> Result { - let output = run_command( - cwd, - "gh", - &["api", "user", "--jq", ".login"], - &[("GH_PROMPT_DISABLED", "1")], - )?; - non_empty_text(&output, "GitHub CLI returned no signed-in account") -} - #[allow(clippy::too_many_arguments)] fn create_github( cwd: &str, @@ -1107,8 +1025,10 @@ fn activity_github( let pull_request = value .pointer("/data/repository/pullRequest") .ok_or_else(|| format!("GitHub returned no activity data for PR #{id}"))?; + let mut pull_request = pull_request.clone(); + pages::activity_checks(cwd, &owner, &repo, id, &mut pull_request)?; parse_github_activity( - pull_request, + &pull_request, PullRequestRepository { provider: PullRequestProvider::GitHub, remote, @@ -1139,57 +1059,10 @@ fn detail_github(cwd: &str, owner: String, repo: String, id: u64) -> Result Result<(Vec, Vec, bool)> { - let query = format!("query={GITHUB_REVIEW_THREADS_QUERY}"); - let owner = format!("owner={owner}"); - let repo = format!("repo={repo}"); - let number = format!("number={id}"); - let output = run_command( - cwd, - "gh", - &[ - "api", "graphql", "-f", &query, "-F", &owner, "-F", &repo, "-F", &number, - ], - &[("GH_PROMPT_DISABLED", "1")], - )?; - let value: Value = serde_json::from_slice(&output) - .map_err(|error| format!("GitHub CLI returned invalid review-thread JSON: {error}"))?; - Ok(( - parse_github_review_threads(&value), - parse_github_reviews(&value), - parse_github_can_mark_ready(&value), - )) -} - fn diff_github(cwd: &str, owner: String, repo: String, id: u64) -> Result { let slug = format!("{owner}/{repo}"); let id = id.to_string(); @@ -1579,6 +1452,8 @@ fn list_azure( }) .collect(); Ok(PullRequestList { + next_cursor: None, + total_count: None, repository: PullRequestRepository { provider: PullRequestProvider::AzureDevOps, remote, @@ -1624,6 +1499,8 @@ fn list_azure_server( }) .collect(); Ok(PullRequestList { + next_cursor: None, + total_count: None, repository: PullRequestRepository { provider: PullRequestProvider::AzureDevOps, remote, @@ -1745,6 +1622,7 @@ fn detail_azure_server( pull_request.checks = checks .iter() .map(|check| PullRequestCheck { + id: check.id.clone(), name: check.name.clone(), status: check.status.clone(), }) @@ -2418,6 +2296,7 @@ fn detail_azure( pull_request.checks = checks .iter() .map(|check| PullRequestCheck { + id: check.id.clone(), name: check.name.clone(), status: check.status.clone(), }) @@ -3343,6 +3222,20 @@ fn run_command_input( envs: &[(&str, &str)], stdin_data: Option<&[u8]>, ) -> Result> { + run_command_input_cancellable(cwd, program, args, envs, stdin_data, None) +} + +fn run_command_input_cancellable( + cwd: &str, + program: &str, + args: &[&str], + envs: &[(&str, &str)], + stdin_data: Option<&[u8]>, + cancelled: Option<&std::sync::atomic::AtomicBool>, +) -> Result> { + if cancelled.is_some_and(|flag| flag.load(std::sync::atomic::Ordering::Relaxed)) { + return Err("Read cancelled".into()); + } // Resolve strictly through PATH before setting the untrusted repository as // cwd. On Windows, CreateProcess otherwise searches cwd and could execute // a repository-owned `gh.exe`/`az.exe`. Reuse the AI CLI resolver so batch @@ -3414,7 +3307,7 @@ fn run_command_input( return Err(format!("{program} wait failed: {error}")); } } - if started.elapsed() >= COMMAND_TIMEOUT { + if started.elapsed() >= COMMAND_TIMEOUT || cancelled.is_some_and(|flag| flag.load(std::sync::atomic::Ordering::Relaxed)) { let _ = child.kill(); let _ = child.wait(); let _ = stdout_reader.join(); @@ -3422,7 +3315,7 @@ fn run_command_input( if let Some(writer) = stdin_writer.take() { let _ = writer.join(); } - return Err(format!("{program} timed out after 30 seconds")); + return Err(if cancelled.is_some_and(|flag| flag.load(std::sync::atomic::Ordering::Relaxed)) { "Read cancelled".into() } else { format!("{program} timed out after 30 seconds") }); } thread::sleep(Duration::from_millis(25)); }; @@ -3916,7 +3809,7 @@ fn parse_github_activity( comments, reviews, checks, - checks_complete: true, + checks_complete: value.pointer("/statusCheckRollup/contexts/pageInfo/hasNextPage").and_then(Value::as_bool) == Some(false) || value.get("statusCheckRollup") == Some(&Value::Null), }) } @@ -4042,6 +3935,7 @@ fn parse_github_pr(value: &Value, viewer: Option<&str>) -> Option { .iter() .filter_map(|check| { Some(PullRequestCheck { + id: text(check.get("id")).unwrap_or_default(), name: text(check.get("name")).or_else(|| text(check.get("context")))?, status: text(check.get("conclusion")) .filter(|status| !status.is_empty()) @@ -4051,6 +3945,7 @@ fn parse_github_pr(value: &Value, viewer: Option<&str>) -> Option { }) .collect(); Some(PullRequest { + data_pages: Vec::new(), id, title: text(value.get("title")).unwrap_or_default(), state: text(value.get("state")) @@ -4370,6 +4265,7 @@ fn parse_azure_pr( "review required".into() }; Some(PullRequest { + data_pages: Vec::new(), id, title: text(value.get("title")).unwrap_or_default(), state: text(value.get("status")) @@ -5016,7 +4912,7 @@ mod tests { } for nested in ["comments", "commits", "latestReviews", "statusCheckRollup"] { assert!(!GITHUB_LIST_FIELDS.contains(nested)); - assert!(GITHUB_DETAIL_FIELDS.contains(nested)); + assert!(!GITHUB_DETAIL_FIELDS.contains(nested)); } assert_eq!( auth_hint( @@ -5117,7 +5013,7 @@ mod tests { "viewerCanResolve", "viewerCanUnresolve", ] { - assert!(GITHUB_REVIEW_THREADS_QUERY.contains(field)); + assert!(pages::review_query_contract().contains(field)); } } @@ -5140,7 +5036,7 @@ mod tests { "reviewThreads": { "nodes": [{ "comments": { "nodes": [ { "id": "PRRC_1", "author": { "login": "linus" } } ] } }] }, - "statusCheckRollup": { "contexts": { "nodes": [ + "statusCheckRollup": { "contexts": { "pageInfo": {"hasNextPage":false}, "nodes": [ { "__typename": "CheckRun", "databaseId": 99, "name": "CI", "status": "COMPLETED", "conclusion": "FAILURE" }, { "__typename": "StatusContext", "id": "SC_1", "context": "lint", "state": "SUCCESS" } ] } } diff --git a/crates/strand-tauri/src/pull_requests/pages.rs b/crates/strand-tauri/src/pull_requests/pages.rs new file mode 100644 index 00000000..bf07d56c --- /dev/null +++ b/crates/strand-tauri/src/pull_requests/pages.rs @@ -0,0 +1,578 @@ +//! Explicit, bounded GitHub connection pages. No traversal on inbox focus. +use super::*; +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, Mutex, OnceLock, +}; + +static READS: OnceLock>>> = OnceLock::new(); + +pub struct ReadGuard { + id: String, + pub cancelled: Arc, +} +impl ReadGuard { + pub fn new(id: &str) -> Result { + Uuid::parse_str(id).map_err(|_| "Invalid read request ID".to_string())?; + let cancelled = Arc::new(AtomicBool::new(false)); + let mut reads = READS + .get_or_init(Default::default) + .lock() + .map_err(|_| "Read lock failed")?; + if reads.contains_key(id) { + return Err("Read request already active".into()); + } + reads.insert(id.into(), cancelled.clone()); + Ok(Self { + id: id.into(), + cancelled, + }) + } +} +impl Drop for ReadGuard { + fn drop(&mut self) { + if let Ok(mut reads) = READS.get_or_init(Default::default).lock() { + reads.remove(&self.id); + } + } +} +pub fn cancel(id: &str) { + if let Ok(reads) = READS.get_or_init(Default::default).lock() { + if let Some(cancelled) = reads.get(id) { + cancelled.store(true, Ordering::Relaxed); + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum Kind { + Comments, + Commits, + Reviews, + Threads, + Replies, + Checks, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Cursor { + pub kind: Kind, + pub thread_id: Option, + pub cursor: Option, + pub total: Option, + pub error: Option, +} + +#[derive(Debug, Clone, Serialize)] +pub struct Page { + pub source_commit: String, + pub request: Cursor, + pub pending: Vec, + pub comments: Vec, + pub commits: Vec, + pub reviews: Vec, + pub review_threads: Vec, + pub checks: Vec, +} + +const INFO: &str = "totalCount pageInfo { hasNextPage endCursor }"; +const COMMENT: &str = "id body createdAt url author { login avatarUrl }"; +const REVIEW: &str = + "id body state submittedAt url viewerCanUpdate viewerDidAuthor author { login avatarUrl }"; +const CHECK: &str = "__typename ... on CheckRun { id databaseId name status conclusion } ... on StatusContext { id context state }"; +const COMMIT: &str = + "commit { oid messageHeadline committedDate url author { name avatarUrl user { login } } }"; + +fn connection(kind: Kind, after: &str) -> String { + let (name, fields) = match kind { + Kind::Comments | Kind::Replies => ("comments", COMMENT.to_string()), + Kind::Reviews => ("reviews", REVIEW.to_string()), + Kind::Commits => ("commits", COMMIT.to_string()), + Kind::Checks => ("contexts", CHECK.to_string()), + // Only the root comment per thread; replies have their own connection. + Kind::Threads => ("reviewThreads", format!("id isResolved isOutdated viewerCanReply viewerCanResolve viewerCanUnresolve path line startLine originalLine originalStartLine diffSide comments(first: 1) {{ {INFO} nodes {{ {COMMENT} }} }}")), + }; + let field = format!("{name}(first: 50, after: {after}) {{ {INFO} nodes {{ {fields} }} }}"); + if kind == Kind::Checks { + format!("statusCheckRollup {{ {field} }}") + } else { + field + } +} + +#[cfg(test)] +pub fn review_query_contract() -> String { + format!( + "viewerCanUpdate {} {}", + connection(Kind::Threads, "null"), + connection(Kind::Reviews, "null") + ) +} + +/// Background snapshots keep bodies/patches out, but never truncate checks. +pub fn activity_checks( + cwd: &str, + owner: &str, + repo: &str, + id: u64, + value: &mut Value, +) -> Result<()> { + if value.get("statusCheckRollup") == Some(&Value::Null) { + return Ok(()); + } + let mut cursor = next_cursor(&value["statusCheckRollup"]["contexts"], None)?; + let head = text(value.get("headRefOid")).ok_or("Missing GitHub head")?; + let mut seen = std::collections::HashSet::new(); + while let Some(after) = cursor { + if !seen.insert(after.clone()) { + return Err("Repeated GitHub check cursor".into()); + } + let field = connection(Kind::Checks, "$cursor"); + let query_text = format!("query($owner: String!, $repo: String!, $number: Int!, $cursor: String) {{ repository(owner: $owner, name: $repo) {{ pullRequest(number: $number) {{ headRefOid {field} }} }} }}"); + let next = query( + cwd, + &query_text, + serde_json::json!({"owner":owner,"repo":repo,"number":id,"cursor":after}), + None, + )?; + let pr = &next["data"]["repository"]["pullRequest"]; + ensure_review_head(pr["headRefOid"].as_str().unwrap_or_default(), &head)?; + let contexts = &pr["statusCheckRollup"]["contexts"]; + cursor = next_cursor(contexts, Some(&after))?; + value["statusCheckRollup"]["contexts"]["nodes"] + .as_array_mut() + .ok_or("Missing check nodes")? + .extend(array(contexts, "nodes").iter().cloned()); + value["statusCheckRollup"]["contexts"]["pageInfo"] = contexts["pageInfo"].clone(); + } + if let Some(nodes) = value["statusCheckRollup"]["contexts"]["nodes"].as_array_mut() { + let mut ids = std::collections::HashSet::new(); + nodes.retain(|c| parse_github_activity_check(c).is_some_and(|c| ids.insert(c.id))); + } + Ok(()) +} + +pub fn query( + cwd: &str, + query: &str, + variables: Value, + cancelled: Option<&AtomicBool>, +) -> Result { + let input = + serde_json::to_vec(&github_graphql_payload(query, variables)).map_err(|e| e.to_string())?; + let output = run_command_input_cancellable( + cwd, + "gh", + &["api", "graphql", "--method", "POST", "--input", "-"], + &[("GH_PROMPT_DISABLED", "1")], + Some(&input), + cancelled, + )?; + let value: Value = + serde_json::from_slice(&output).map_err(|e| format!("Invalid GitHub page: {e}"))?; + if let Some(errors) = value + .get("errors") + .and_then(Value::as_array) + .filter(|v| !v.is_empty()) + { + return Err(format!( + "GitHub page failed: {}", + errors + .iter() + .filter_map(|e| e.get("message").and_then(Value::as_str)) + .collect::>() + .join("; ") + )); + } + Ok(value) +} + +pub fn inbox(path: &str, cursor: Option<&str>, request_id: &str) -> Result { + let guard = ReadGuard::new(request_id)?; + let (remote, host) = host_for_path(path)?; + let HostRepo::GitHub { owner, repo } = host else { + if cursor.is_some() { + return Err("Inbox pagination is unavailable for this provider".into()); + } + return list(path); + }; + let value = query( + path, + r#"query($owner: String!, $repo: String!, $cursor: String) { + viewer { login } + repository(owner: $owner, name: $repo) { + pullRequests(first: 100, after: $cursor, orderBy: {field: CREATED_AT, direction: DESC}) { + totalCount pageInfo { hasNextPage endCursor } + nodes { number title state isDraft author { login } headRefName headRefOid baseRefName createdAt updatedAt closedAt mergedAt url reviewDecision additions deletions changedFiles } + } + } + }"#, + serde_json::json!({"owner":owner,"repo":repo,"cursor":cursor}), + Some(&guard.cancelled), + )?; + let connection = value + .pointer("/data/repository/pullRequests") + .ok_or("Missing GitHub inbox")?; + let next_cursor = next_cursor(connection, cursor)?; + let viewer = text(value.pointer("/data/viewer/login")); + let mut seen = std::collections::HashSet::new(); + Ok(PullRequestList { + repository: PullRequestRepository { + provider: PullRequestProvider::GitHub, + remote, + label: format!("{owner}/{repo}"), + viewer: viewer.clone(), + }, + pull_requests: array(connection, "nodes") + .iter() + .filter_map(|v| parse_github_pr(v, viewer.as_deref())) + .filter(|pr| seen.insert(pr.id)) + .collect(), + next_cursor, + total_count: connection.get("totalCount").and_then(Value::as_u64), + }) +} + +fn next_cursor(value: &Value, previous: Option<&str>) -> Result> { + match value + .pointer("/pageInfo/hasNextPage") + .and_then(Value::as_bool) + { + Some(false) => Ok(None), + Some(true) => text(value.pointer("/pageInfo/endCursor")) + .filter(|cursor| !cursor.is_empty() && Some(cursor.as_str()) != previous) + .map(Some) + .ok_or_else(|| "GitHub returned a missing or repeated cursor; refresh to retry".into()), + None => Err("GitHub did not report whether this connection is complete".into()), + } +} + +const KINDS: [Kind; 5] = [ + Kind::Comments, + Kind::Commits, + Kind::Reviews, + Kind::Threads, + Kind::Checks, +]; + +pub fn initial(cwd: &str, owner: &str, repo: &str, pr: &mut PullRequest) { + let fields = KINDS + .iter() + .map(|kind| connection(*kind, "null")) + .collect::>() + .join(" "); + let query_text = format!("query($owner: String!, $repo: String!, $number: Int!) {{ repository(owner: $owner, name: $repo) {{ pullRequest(number: $number) {{ headRefOid viewerCanUpdate {fields} }} }} }}"); + let result = query( + cwd, + &query_text, + serde_json::json!({"owner":owner,"repo":repo,"number":pr.id}), + None, + ); + if let Ok(value) = &result { + pr.can_mark_ready = pr.is_draft && parse_github_can_mark_ready(value); + } + for kind in KINDS { + let request = Cursor { + kind, + thread_id: None, + cursor: None, + total: None, + error: None, + }; + let page = result + .as_ref() + .map_err(Clone::clone) + .and_then(|value| parse_page(value, request.clone(), &pr.source_commit)); + match page { + Ok(page) => { + pr.comments.extend(page.comments); + pr.commits.extend(page.commits); + pr.reviews.extend(page.reviews); + pr.review_threads.extend(page.review_threads); + pr.checks.extend(page.checks); + pr.data_pages.extend(page.pending); + } + Err(error) => pr.data_pages.push(Cursor { + error: Some(error), + ..request + }), + } + } + pr.comments.extend( + pr.review_threads + .iter() + .flat_map(|t| t.comments.iter().cloned()), + ); + let mut seen = std::collections::HashSet::new(); + pr.comments.retain(|c| seen.insert(c.id.clone())); + pr.comment_count = pr.comments.len(); + pr.commit_count = pr.commits.len(); + pr.checks_complete = !pr.data_pages.iter().any(|p| p.kind == Kind::Checks); +} + +pub fn read( + path: &str, + id: u64, + expected_head: &str, + request: Cursor, + request_id: &str, +) -> Result { + validate_commit(expected_head)?; + let guard = ReadGuard::new(request_id)?; + let (_, host) = host_for_path(path)?; + let HostRepo::GitHub { owner, repo } = host else { + return Err("Connection pages are unavailable for this provider".into()); + }; + let field = connection(request.kind, "$cursor"); + let selection = if request.kind == Kind::Replies { + validate_thread_id(request.thread_id.as_deref().unwrap_or_default())?; + format!("node(id: $threadId) {{ ... on PullRequestReviewThread {{ pullRequest {{ number repository {{ nameWithOwner }} }} {field} }} }}") + } else { + String::new() + }; + let thread_variable = if request.kind == Kind::Replies { + ", $threadId: ID!" + } else { + "" + }; + let pr_field = if request.kind == Kind::Replies { + "" + } else { + &field + }; + let query_text = format!("query($owner: String!, $repo: String!, $number: Int!, $cursor: String{thread_variable}) {{ repository(owner: $owner, name: $repo) {{ pullRequest(number: $number) {{ headRefOid viewerCanUpdate {pr_field} }} }} {selection} }}"); + let value = query( + path, + &query_text, + serde_json::json!({"owner":owner,"repo":repo,"number":id,"cursor":request.cursor,"threadId":request.thread_id}), + Some(&guard.cancelled), + )?; + if request.kind == Kind::Replies + && (value + .pointer("/data/node/pullRequest/number") + .and_then(Value::as_u64) + != Some(id) + || text(value.pointer("/data/node/pullRequest/repository/nameWithOwner")).as_deref() + != Some(format!("{owner}/{repo}").as_str())) + { + return Err("Thread does not belong to this pull request".into()); + } + parse_page(&value, request, expected_head) +} + +fn parse_page(value: &Value, request: Cursor, expected_head: &str) -> Result { + let pr = value + .pointer("/data/repository/pullRequest") + .ok_or("Missing GitHub pull request")?; + let head = text(pr.get("headRefOid")).ok_or("Missing GitHub head")?; + ensure_review_head(&head, expected_head)?; + let key = match request.kind { + Kind::Comments => "/comments", + Kind::Commits => "/commits", + Kind::Reviews => "/reviews", + Kind::Threads => "/reviewThreads", + Kind::Checks => "/statusCheckRollup/contexts", + Kind::Replies => "", + }; + let empty_checks = + serde_json::json!({"nodes":[],"pageInfo":{"hasNextPage":false},"totalCount":0}); + let connection = if request.kind == Kind::Replies { + value.pointer("/data/node/comments") + } else if request.kind == Kind::Checks && pr.get("statusCheckRollup") == Some(&Value::Null) { + Some(&empty_checks) + } else { + pr.pointer(key) + } + .ok_or("Missing GitHub connection; loaded data is incomplete")?; + let mut pending = Vec::new(); + if let Some(cursor) = next_cursor(connection, request.cursor.as_deref())? { + pending.push(Cursor { + cursor: Some(cursor), + total: connection.get("totalCount").and_then(Value::as_u64), + error: None, + ..request.clone() + }); + } + let mut page = Page { + source_commit: head, + request: request.clone(), + pending, + comments: vec![], + commits: vec![], + reviews: vec![], + review_threads: vec![], + checks: vec![], + }; + match request.kind { + Kind::Reviews => page.reviews = parse_github_reviews(value), + Kind::Threads => { + page.review_threads = parse_github_review_threads(value); + for thread in array(connection, "nodes") { + if let Some(cursor) = next_cursor(&thread["comments"], None)? { + page.pending.push(Cursor { + kind: Kind::Replies, + thread_id: text(thread.get("id")), + cursor: Some(cursor), + total: thread + .pointer("/comments/totalCount") + .and_then(Value::as_u64), + error: None, + }); + } + } + } + Kind::Comments | Kind::Replies => { + let mock = serde_json::json!({"number":1,"comments":connection["nodes"]}); + page.comments = parse_github_pr(&mock, None).unwrap().comments; + } + Kind::Checks => { + page.checks = array(connection, "nodes") + .iter() + .filter_map(parse_github_activity_check) + .map(|c| PullRequestCheck { + id: c.id, + name: c.name, + status: c.status, + }) + .collect() + } + Kind::Commits => { + page.commits = array(connection, "nodes") + .iter() + .filter_map(|node| { + let c = &node["commit"]; + Some(PullRequestCommit { + id: text(c.get("oid"))?, + title: text(c.get("messageHeadline")).unwrap_or_default(), + author: text(c.pointer("/author/name")).unwrap_or_else(|| "unknown".into()), + avatar_url: text(c.pointer("/author/avatarUrl")), + committed_at: text(c.get("committedDate")).unwrap_or_default(), + url: text(c.get("url")), + }) + }) + .collect() + } + } + Ok(page) +} + +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn missing_or_repeated_cursor_is_an_error() { + assert!(next_cursor(&serde_json::json!({}), None).is_err()); + assert!(next_cursor( + &serde_json::json!({"pageInfo":{"hasNextPage":true,"endCursor":"same"}}), + Some("same") + ) + .is_err()); + assert_eq!( + next_cursor(&serde_json::json!({"pageInfo":{"hasNextPage":false}}), None).unwrap(), + None + ); + } + #[test] + fn cancellation_is_scoped_and_removed() { + let id = Uuid::new_v4().to_string(); + let guard = ReadGuard::new(&id).unwrap(); + assert!(ReadGuard::new(&id).is_err()); + cancel(&id); + assert!(guard.cancelled.load(Ordering::Relaxed)); + drop(guard); + assert!(!ReadGuard::new(&id) + .unwrap() + .cancelled + .load(Ordering::Relaxed)); + } + #[test] + fn thread_pages_keep_nested_reads_shallow() { + let query = connection(Kind::Threads, "$cursor"); + assert!(query.contains("comments(first: 1)")); + assert!(query.contains("after: $cursor")); + } + + #[test] + fn all_connections_traverse_101_entries_and_reject_force_push() { + let head = "a".repeat(40); + for kind in [ + Kind::Comments, + Kind::Reviews, + Kind::Threads, + Kind::Replies, + Kind::Checks, + Kind::Commits, + ] { + let mut count = 0; + let mut cursor = None; + for (start, end) in [(0, 50), (50, 100), (100, 101)] { + let nodes = (start..end).map(|i| serde_json::json!({ + "id":format!("node-{i}"),"body":"feedback","state":"APPROVED","author":{"login":"reviewer"}, + "path":"file.rs","line":i+1,"diffSide":"RIGHT", "name":format!("check-{i}"),"status":"SUCCESS", + "comments":{"nodes":[{"id":format!("comment-{i}"),"body":"feedback"}],"pageInfo":{"hasNextPage":false}}, + "commit":{"oid":format!("{i:040x}"),"messageHeadline":"Commit"} + })).collect::>(); + let connection = serde_json::json!({"nodes":nodes,"totalCount":101,"pageInfo":{"hasNextPage":end<101,"endCursor":end.to_string()}}); + let mut value = + serde_json::json!({"data":{"repository":{"pullRequest":{"headRefOid":head}}}}); + let pr = &mut value["data"]["repository"]["pullRequest"]; + match kind { + Kind::Comments => pr["comments"] = connection, + Kind::Commits => pr["commits"] = connection, + Kind::Reviews => pr["reviews"] = connection, + Kind::Threads => pr["reviewThreads"] = connection, + Kind::Checks => { + pr["statusCheckRollup"] = serde_json::json!({"contexts":connection}) + } + Kind::Replies => { + value["data"]["node"] = serde_json::json!({"comments":connection}) + } + } + let request = Cursor { + kind, + thread_id: None, + cursor, + total: None, + error: None, + }; + assert!(parse_page(&value, request.clone(), &"b".repeat(40)).is_err()); + let page = parse_page(&value, request, &head).unwrap(); + count += page.comments.len() + + page.commits.len() + + page.reviews.len() + + page.review_threads.len() + + page.checks.len(); + cursor = page.pending.first().and_then(|p| p.cursor.clone()); + } + assert_eq!(count, 101, "{kind:?}"); + assert!(cursor.is_none()); + } + } + + #[test] + fn cancellation_terminates_an_active_read() { + let cancelled = Arc::new(AtomicBool::new(false)); + let signal = cancelled.clone(); + let worker = thread::spawn(move || { + thread::sleep(Duration::from_millis(250)); + signal.store(true, Ordering::Relaxed); + }); + let start = Instant::now(); + #[cfg(windows)] + let result = run_command_input_cancellable( + ".", + "powershell", + &["-NoProfile", "-Command", "Start-Sleep -Seconds 20"], + &[], + None, + Some(&cancelled), + ); + #[cfg(not(windows))] + let result = + run_command_input_cancellable(".", "sleep", &["20"], &[], None, Some(&cancelled)); + worker.join().unwrap(); + assert!(result.unwrap_err().contains("cancelled")); + assert!(start.elapsed() < Duration::from_secs(5)); + } +} diff --git a/docs/learnings.md b/docs/learnings.md index e50db001..894d2772 100644 --- a/docs/learnings.md +++ b/docs/learnings.md @@ -2494,3 +2494,12 @@ Pierre reads `navigator.userAgent` during module evaluation; Node 22's built-in `navigator` hid a failure on CI's Node 20. Stub browser globals and restore them after the test, while retaining real integration assertions. Reproduce this class of failure locally with `--no-experimental-global-navigator`. + +## Hosted connection pages carry completeness and reviewed heads (2026-09-06) + +GitHub connection continuations carry their opaque cursor and the activated +head SHA. Reject missing/repeated cursors and head mismatches; deduplicate by +provider ID when appending, and keep already loaded data on failures. A thread +page fetches only its root comment; replies have independent cursors. Counts +remain explicitly partial until their connections are exhausted. Background +check snapshots traverse check pages without patch or comment-body reads. diff --git a/ui/src/App.tsx b/ui/src/App.tsx index ea335854..2fedd5f0 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -1720,6 +1720,20 @@ export function App() { }, } satisfies PaletteAction] : []), ...(view === 'pull-requests' ? [ + { + id: 'pull-request-load-more', + label: 'Pull Requests: load next data page', + group: 'Actions', + keywords: 'pr pagination inbox reviews threads replies checks more partial', + run: () => window.dispatchEvent(new CustomEvent('strand:pull-request-load-more')), + } satisfies PaletteAction, + { + id: 'pull-request-cancel-read', + label: 'Pull Requests: cancel loading page', + group: 'Actions', + keywords: 'pr stop pagination', + run: () => window.dispatchEvent(new CustomEvent('strand:pull-request-cancel-read')), + } satisfies PaletteAction, { id: 'pull-request-search', label: 'Pull Requests: search…', diff --git a/ui/src/lib/pullRequestPages.test.ts b/ui/src/lib/pullRequestPages.test.ts new file mode 100644 index 00000000..d227af4e --- /dev/null +++ b/ui/src/lib/pullRequestPages.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from 'vitest'; +import { appendPullRequestPage, incompleteLabel, uniqueBy } from './pullRequestPages'; +import type { PullRequest, PullRequestDataPage, PullRequestPageCursor } from './types'; + +const cursor = (kind: PullRequestPageCursor['kind'], after: string | null): PullRequestPageCursor => + ({ kind, cursor: after, thread_id: null, total: 101, error: null }); +const pr = (kind: PullRequestPageCursor['kind']): PullRequest => ({ + source_commit: 'a'.repeat(40), data_pages: [cursor(kind, null)], + comments: [], commits: [], reviews: [], checks: [], review_threads: [], checks_complete: false, +} as unknown as PullRequest); +const page = (request: PullRequestPageCursor): PullRequestDataPage => ({ + request, source_commit: 'a'.repeat(40), pending: [], comments: [], commits: [], reviews: [], checks: [], review_threads: [], +}); + +describe('provider pages', () => { + it.each(['comments', 'commits', 'reviews', 'checks', 'threads'] as const)('loads 101 %s with overlapping pages exactly once', (kind) => { + let current = pr(kind); + const field = kind === 'threads' ? 'review_threads' : kind; + for (const [start, end, after, next] of [[0, 50, null, '50'], [49, 100, '50', '100'], [99, 101, '100', null]] as const) { + const incoming = page(cursor(kind, after)); + incoming.pending = next ? [cursor(kind, next)] : []; + Object.assign(incoming, { [field]: Array.from({ length: end - start }, (_, i) => ({ id: `${start + i}`, comments: [] })) }); + current = appendPullRequestPage(current, incoming); + expect(appendPullRequestPage(current, incoming)).toBe(current); + } + expect(current[field]).toHaveLength(101); + expect(incompleteLabel(current)).toBe(''); + expect(current.checks_complete).toBe(true); + }); + it('deduplicates inbox rows without losing existing selection objects', () => { + const rows = Array.from({ length: 100 }, (_, id) => ({ id })); + expect(uniqueBy(rows, [{ id: 99 }, { id: 100 }], (p) => p.id)).toHaveLength(101); + }); + it('rejects an old head and preserves prior data on invalid cursor', () => { + const current = pr('reviews'); + const incoming = page(cursor('reviews', null)); + incoming.source_commit = 'b'.repeat(40); + expect(appendPullRequestPage(current, incoming)).toBe(current); + incoming.source_commit = current.source_commit; + incoming.request.cursor = 'unexpected'; + expect(appendPullRequestPage(current, incoming)).toBe(current); + expect(incompleteLabel(current)).toContain('Counts show loaded items'); + }); + it('loads 101 replies into the original thread and timeline with file coordinates intact', () => { + let current = pr('replies'); + current.data_pages![0].thread_id = 'thread'; + current.review_threads = [{ id: 'thread', path: 'src/a.ts', comments: [] }] as unknown as PullRequest['review_threads']; + const incoming = page(current.data_pages![0]); + incoming.comments = Array.from({ length: 101 }, (_, i) => ({ id: `${i}`, path: null })) as PullRequest['comments']; + current = appendPullRequestPage(current, incoming); + expect(current.review_threads[0].comments).toHaveLength(101); + expect(current.comments).toHaveLength(101); + expect(current.comments[100].path).toBe('src/a.ts'); + }); +}); diff --git a/ui/src/lib/pullRequestPages.ts b/ui/src/lib/pullRequestPages.ts new file mode 100644 index 00000000..357b0b42 --- /dev/null +++ b/ui/src/lib/pullRequestPages.ts @@ -0,0 +1,40 @@ +import type { PullRequest, PullRequestDataPage, PullRequestPageCursor } from './types'; + +export const pageKey = (page: PullRequestPageCursor) => `${page.kind}:${page.thread_id ?? ''}`; + +export function uniqueBy(old: readonly T[], incoming: readonly T[], key: (item: T) => string | number): T[] { + const result = new Map(old.map((item) => [key(item), item])); + for (const item of incoming) result.set(key(item), item); + return [...result.values()]; +} + +/** Reject both stale heads and out-of-order/replayed cursor responses. */ +export function appendPullRequestPage(pr: PullRequest, page: PullRequestDataPage): PullRequest { + const pending = pr.data_pages ?? []; + if (pr.source_commit !== page.source_commit || !pending.some((p) => + pageKey(p) === pageKey(page.request) && p.cursor === page.request.cursor)) return pr; + const data_pages = uniqueBy(pending.filter((p) => pageKey(p) !== pageKey(page.request)), page.pending, pageKey); + let review_threads = uniqueBy(pr.review_threads, page.review_threads, (t) => t.id); + if (page.request.kind === 'replies') { + review_threads = review_threads.map((thread) => thread.id === page.request.thread_id ? { + ...thread, comments: uniqueBy(thread.comments, page.comments.map((c) => ({ ...c, path: thread.path })), (c) => c.id), + } : thread); + } + const comments = uniqueBy(pr.comments, [ + ...(page.request.kind === 'comments' ? page.comments : []), + ...review_threads.flatMap((thread) => thread.comments), + ], (c) => c.id); + const commits = uniqueBy(pr.commits, page.commits, (c) => c.id); + return { + ...pr, data_pages, review_threads, comments, commits, + comment_count: comments.length, commit_count: commits.length, + reviews: uniqueBy(pr.reviews, page.reviews, (r) => r.id), + checks: uniqueBy(pr.checks, page.checks, (c) => c.id || c.name), + checks_complete: !data_pages.some((p) => p.kind === 'checks'), + }; +} + +export function incompleteLabel(pr: PullRequest): string { + const kinds = [...new Set((pr.data_pages ?? []).map((p) => p.kind))]; + return kinds.length ? `Partial data: ${kinds.join(', ')}. Counts show loaded items.` : ''; +} diff --git a/ui/src/lib/tauri.ts b/ui/src/lib/tauri.ts index e7356543..e32b9465 100644 --- a/ui/src/lib/tauri.ts +++ b/ui/src/lib/tauri.ts @@ -40,6 +40,8 @@ import type { Progress, PullMode, PullRequest, + PullRequestPageCursor, + PullRequestDataPage, PullRequestActivitySnapshot, PullRequestBranchMatch, PullRequestCheckoutPreparation, @@ -195,6 +197,11 @@ export const tauri = { repoCommitExportPatch: (path: string, oids: string[], destination: string) => invoke('repo_commit_export_patch', { path, oids, destination }), repoRefs: (path: string) => invoke('repo_refs', { path }), + repoPullRequestInboxPage: (path: string, cursor: string | null, requestId: string) => + invoke('repo_pull_request_inbox_page', { path, cursor, requestId }), + repoPullRequestDataPage: (path: string, id: number, expectedHead: string, request: PullRequestPageCursor, requestId: string) => + invoke('repo_pull_request_data_page', { path, id, expectedHead, request, requestId }), + repoPullRequestCancelRead: (requestId: string) => invoke('repo_pull_request_cancel_read', { requestId }), repoPullRequests: (path: string) => invoke('repo_pull_requests', { path }), repoPullRequestForBranch: (path: string, branch: string) => invoke('repo_pull_request_for_branch', { path, branch }), diff --git a/ui/src/lib/types.ts b/ui/src/lib/types.ts index c27be6a0..55695103 100644 --- a/ui/src/lib/types.ts +++ b/ui/src/lib/types.ts @@ -275,6 +275,7 @@ export interface PullRequestReviewer { } export interface PullRequestCheck { + id?: string; name: string; status: string; } @@ -338,6 +339,7 @@ export interface PullRequestReview { } export interface PullRequest { + data_pages?: PullRequestPageCursor[]; id: number; title: string; state: string; @@ -373,6 +375,8 @@ export interface PullRequest { } export interface PullRequestList { + next_cursor?: string | null; + total_count?: number | null; repository: PullRequestRepository; pull_requests: PullRequest[]; } @@ -927,3 +931,21 @@ export type AiGenerationOutcome = coverage: AiInputCoverage; provider: AiProvider; }; + +export interface PullRequestPageCursor { + kind: 'comments' | 'commits' | 'reviews' | 'threads' | 'replies' | 'checks'; + thread_id: string | null; + cursor: string | null; + total: number | null; + error: string | null; +} +export interface PullRequestDataPage { + source_commit: string; + request: PullRequestPageCursor; + pending: PullRequestPageCursor[]; + comments: PullRequestComment[]; + commits: PullRequestCommit[]; + reviews: PullRequestReview[]; + review_threads: PullRequestReviewThread[]; + checks: PullRequestCheck[]; +} diff --git a/ui/src/styles/features.css b/ui/src/styles/features.css index 03aa8eaa..98d896b9 100644 --- a/ui/src/styles/features.css +++ b/ui/src/styles/features.css @@ -9542,3 +9542,5 @@ select.clone-input { .plugin-heroi-select-thinking, .plugin-heroi-select-permission { display: none; } } + +.pr-data-status { display: flex; flex-wrap: wrap; gap: 8px; align-items: center; padding: 8px 16px; flex-shrink: 0; color: var(--text-dim); font-size: 12px; border-bottom: 1px solid var(--border); } diff --git a/ui/src/views/PullRequestDataLoader.tsx b/ui/src/views/PullRequestDataLoader.tsx new file mode 100644 index 00000000..96f5be58 --- /dev/null +++ b/ui/src/views/PullRequestDataLoader.tsx @@ -0,0 +1,52 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { incompleteLabel } from '../lib/pullRequestPages'; +import { errMessage, tauri } from '../lib/tauri'; +import type { PullRequest, PullRequestDataPage } from '../lib/types'; + +export function PullRequestDataLoader({ path, pr, onPage }: { + path: string; pr: PullRequest; onPage: (page: PullRequestDataPage) => void; +}) { + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + const request = useRef(null); + const stop = useCallback(() => { + if (request.current) void tauri.repoPullRequestCancelRead(request.current).catch(() => {}); + request.current = null; + setBusy(false); + }, []); + useEffect(() => { setError(null); return stop; }, [path, pr.id, pr.source_commit, stop]); + const next = pr.data_pages?.[0]; + const load = useCallback(async () => { + if (!next || request.current) return; + const id = crypto.randomUUID(); + request.current = id; + setBusy(true); + setError(null); + try { + const page = await tauri.repoPullRequestDataPage(path, pr.id, pr.source_commit, next, id); + if (request.current === id) onPage(page); + } catch (caught) { + if (request.current === id) setError(errMessage(caught)); + } finally { + if (request.current === id) { request.current = null; setBusy(false); } + } + }, [next, onPage, path, pr.id, pr.source_commit]); + useEffect(() => { + const run = () => { void load(); }; + window.addEventListener('strand:pull-request-load-more', run); + window.addEventListener('strand:pull-request-cancel-read', stop); + return () => { + window.removeEventListener('strand:pull-request-load-more', run); + window.removeEventListener('strand:pull-request-cancel-read', stop); + }; + }, [load, stop]); + if (!next) return null; + return
+ {incompleteLabel(pr)} + + {busy && } + {(error || next.error) && {error || next.error} · Retry loading or refresh.} +
; +} diff --git a/ui/src/views/PullRequestInboxLoader.tsx b/ui/src/views/PullRequestInboxLoader.tsx new file mode 100644 index 00000000..fce65b5c --- /dev/null +++ b/ui/src/views/PullRequestInboxLoader.tsx @@ -0,0 +1,47 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { errMessage, tauri } from '../lib/tauri'; +import type { PullRequestList } from '../lib/types'; + +export function PullRequestInboxLoader({ path, data, onPage }: { + path: string; data: PullRequestList; onPage: (page: PullRequestList) => void; +}) { + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + const request = useRef(null); + const stop = useCallback(() => { + if (request.current) void tauri.repoPullRequestCancelRead(request.current).catch(() => {}); + request.current = null; + setBusy(false); + }, []); + useEffect(() => stop, [path, stop]); + const load = useCallback(async () => { + if (!data.next_cursor || request.current) return; + const id = crypto.randomUUID(); + request.current = id; + setBusy(true); setError(null); + try { + const page = await tauri.repoPullRequestInboxPage(path, data.next_cursor, id); + if (request.current === id) onPage(page); + } catch (caught) { + if (request.current === id) setError(errMessage(caught)); + } finally { + if (request.current === id) { request.current = null; setBusy(false); } + } + }, [data.next_cursor, onPage, path]); + useEffect(() => { + const run = () => { void load(); }; + window.addEventListener('strand:pull-request-load-more', run); + window.addEventListener('strand:pull-request-cancel-read', stop); + return () => { + window.removeEventListener('strand:pull-request-load-more', run); + window.removeEventListener('strand:pull-request-cancel-read', stop); + }; + }, [load, stop]); + return
+ {data.pull_requests.length} loaded{data.total_count != null ? ` of ${data.total_count}` : ''} + {data.next_cursor ? ' · Partial inbox; filters search loaded items.' : ''} + {data.next_cursor && } + {busy && } + {error && {error} · Retry loading.} +
; +} diff --git a/ui/src/views/PullRequests.tsx b/ui/src/views/PullRequests.tsx index 01b9ac0a..cd930d1f 100644 --- a/ui/src/views/PullRequests.tsx +++ b/ui/src/views/PullRequests.tsx @@ -52,6 +52,7 @@ import type { PullRequestCheck, PullRequestComment, PullRequestCreateOutcome, + PullRequestDataPage, PullRequestList, PullRequestPendingComment, PullRequestReview, @@ -62,6 +63,9 @@ import type { import { useRepo } from '../stores/repo'; import { usePullRequests } from '../stores/pullRequests'; import { useSettings } from '../stores/settings'; +import { PullRequestDataLoader } from './PullRequestDataLoader'; +import { PullRequestInboxLoader } from './PullRequestInboxLoader'; +import { appendPullRequestPage, uniqueBy } from '../lib/pullRequestPages'; import { PullRequestMergeControl } from './PullRequestMergeControl'; import { PullRequestCreateDialog } from './PullRequestCreateDialog'; @@ -423,7 +427,7 @@ function PullRequestSummary({
- Checks {pr.checks.length} + Checks {pr.checks.length}{!pr.checks_complete ? '+' : ''}
{pr.checks.length > 0 ? (
    @@ -436,7 +440,7 @@ function PullRequestSummary({
- Reviews {(pr.reviews ?? []).length} + Reviews {(pr.reviews ?? []).length}{pr.data_pages?.some((p) => p.kind === 'reviews') ? '+' : ''}
{(pr.reviews ?? []).length > 0 ? (pr.reviews ?? []).map((review) => ( void; + onPage: (page: PullRequestDataPage) => void; onUpdated: (next: PullRequest) => void; onToast: (message: string, kind?: 'success' | 'error') => void; followed: boolean; @@ -2062,6 +2068,7 @@ function PullRequestDetails({
)}
+ {lifecycleMenu && ( '}`; const shouldAutoOpen = autoOpenedContext.current !== autoOpenContext; - setData(next); + setData((current) => current ? { ...next, pull_requests: uniqueBy(next.pull_requests, current.pull_requests.filter((item) => !next.pull_requests.some((pr) => pr.id === item.id)), (pr) => pr.id) } : next); setSelectedId((selected) => - !shouldAutoOpen && next.pull_requests.some((pr) => pr.id === selected) + !shouldAutoOpen && selected != null ? selected : preferredId); - setOpenedId((opened) => - next.pull_requests.some((pr) => pr.id === opened) ? opened : null); if (shouldAutoOpen) { autoOpenedContext.current = autoOpenContext; setOpenedId(branchPullRequest?.id ?? null); @@ -2528,7 +2533,7 @@ export function PullRequests({ ) : data ? (
@@ -2543,6 +2548,7 @@ export function PullRequests({ } /> + {path && setData((current) => current ? { ...page, pull_requests: uniqueBy(current.pull_requests, page.pull_requests, (pr) => pr.id) } : page)} />}