diff --git a/apps/staged/src-tauri/src/git/cli.rs b/apps/staged/src-tauri/src/git/cli.rs index cb0566d3..761175e2 100644 --- a/apps/staged/src-tauri/src/git/cli.rs +++ b/apps/staged/src-tauri/src/git/cli.rs @@ -2,6 +2,8 @@ use std::path::Path; use std::process::Command; use thiserror::Error; +use super::strip_git_env; + #[derive(Error, Debug)] pub enum GitError { #[error("git not found - is git installed?")] @@ -20,24 +22,6 @@ pub enum GitError { InvalidPath(String), } -const GIT_LOCAL_ENV_VARS: &[&str] = &[ - "GIT_ALTERNATE_OBJECT_DIRECTORIES", - "GIT_CONFIG", - "GIT_CONFIG_PARAMETERS", - "GIT_CONFIG_COUNT", - "GIT_OBJECT_DIRECTORY", - "GIT_DIR", - "GIT_WORK_TREE", - "GIT_IMPLICIT_WORK_TREE", - "GIT_GRAFT_FILE", - "GIT_INDEX_FILE", - "GIT_NO_REPLACE_OBJECTS", - "GIT_REPLACE_REF_BASE", - "GIT_PREFIX", - "GIT_SHALLOW_FILE", - "GIT_COMMON_DIR", -]; - /// Run a git command and return stdout as a string pub fn run(repo: &Path, args: &[&str]) -> Result { let repo_str = repo @@ -46,9 +30,7 @@ pub fn run(repo: &Path, args: &[&str]) -> Result { let mut command = Command::new("git"); command.args(["-C", repo_str]).args(args); - for key in GIT_LOCAL_ENV_VARS { - command.env_remove(key); - } + strip_git_env(&mut command); let output = command.output().map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { diff --git a/apps/staged/src-tauri/src/git/env.rs b/apps/staged/src-tauri/src/git/env.rs new file mode 100644 index 00000000..7240dd6d --- /dev/null +++ b/apps/staged/src-tauri/src/git/env.rs @@ -0,0 +1,15 @@ +use std::ffi::OsStr; +use std::process::Command; + +/// Remove inherited git-specific environment from a command before running git. +pub(crate) fn strip_git_env(command: &mut Command) { + for (key, _) in std::env::vars_os() { + if starts_with_git_prefix(&key) { + command.env_remove(key); + } + } +} + +fn starts_with_git_prefix(key: &OsStr) -> bool { + key.to_string_lossy().starts_with("GIT_") +} diff --git a/apps/staged/src-tauri/src/git/mod.rs b/apps/staged/src-tauri/src/git/mod.rs index edf62666..a6f17c76 100644 --- a/apps/staged/src-tauri/src/git/mod.rs +++ b/apps/staged/src-tauri/src/git/mod.rs @@ -1,6 +1,7 @@ mod cli; mod commit; mod diff; +mod env; mod files; pub mod github; mod refs; @@ -10,6 +11,7 @@ mod worktree; pub use cli::GitError; pub use commit::commit; pub use diff::{get_file_diff, get_unified_diff, list_diff_files}; +pub(crate) use env::strip_git_env; pub use files::{get_file_at_ref, search_files}; pub use github::{ check_github_auth, check_monorepo_modules, create_pull_request, detect_default_branch_for_repo, diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index 766aaeac..7d3d3ff3 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -26,6 +26,9 @@ pub(crate) mod terminal_output; pub mod timeline; pub mod util_commands; +#[cfg(test)] +pub mod test_utils; + use serde::Serialize; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, Ordering}; diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 65c13f97..60f57ab9 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -2215,6 +2215,7 @@ pub fn emit_session_running( #[cfg(test)] mod tests { use super::*; + use crate::git::strip_git_env; #[cfg(unix)] #[tokio::test] @@ -2293,25 +2294,7 @@ mod tests { fn run_git(dir: &std::path::Path, args: &[&str]) -> String { let mut command = std::process::Command::new("git"); command.args(args).current_dir(dir); - for key in [ - "GIT_ALTERNATE_OBJECT_DIRECTORIES", - "GIT_CONFIG", - "GIT_CONFIG_PARAMETERS", - "GIT_CONFIG_COUNT", - "GIT_OBJECT_DIRECTORY", - "GIT_DIR", - "GIT_WORK_TREE", - "GIT_IMPLICIT_WORK_TREE", - "GIT_GRAFT_FILE", - "GIT_INDEX_FILE", - "GIT_NO_REPLACE_OBJECTS", - "GIT_REPLACE_REF_BASE", - "GIT_PREFIX", - "GIT_SHALLOW_FILE", - "GIT_COMMON_DIR", - ] { - command.env_remove(key); - } + strip_git_env(&mut command); let output = command.output().unwrap(); assert!( output.status.success(), diff --git a/apps/staged/src-tauri/src/test_utils.rs b/apps/staged/src-tauri/src/test_utils.rs new file mode 100644 index 00000000..4fe6a9fe --- /dev/null +++ b/apps/staged/src-tauri/src/test_utils.rs @@ -0,0 +1,68 @@ +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; +use uuid::Uuid; + +use crate::git::strip_git_env; + +/// A temporary git repository for use in tests. +/// +/// Creates a fresh git repo in a temp directory and cleans it up on drop. +pub struct TempGitRepo { + path: PathBuf, +} + +impl TempGitRepo { + pub fn new() -> Self { + let path = std::env::temp_dir().join(format!("staged-test-{}", Uuid::new_v4())); + fs::create_dir_all(&path).unwrap(); + + let repo = Self { path }; + repo.run_git(&["init", "--initial-branch=main"]); + repo.run_git(&["config", "user.email", "test@example.com"]); + repo.run_git(&["config", "user.name", "Test"]); + repo + } + + pub fn path(&self) -> &Path { + &self.path + } + + pub fn write_file(&self, name: &str, content: &str) { + fs::write(self.path.join(name), content).unwrap(); + } + + pub fn commit(&self, message: &str) -> String { + self.run_git(&["add", "."]); + self.run_git(&["commit", "-m", message]); + self.run_git(&["rev-parse", "HEAD"]).trim().to_string() + } + + pub fn run_git(&self, args: &[&str]) -> String { + let mut command = Command::new("git"); + command + .arg("-c") + .arg("core.hooksPath=/dev/null") + .arg("-C") + .arg(&self.path) + .args(args); + strip_git_env(&mut command); + + let output = command.output().unwrap(); + + assert!( + output.status.success(), + "git {:?} failed: {}", + args, + String::from_utf8_lossy(&output.stderr) + ); + + String::from_utf8(output.stdout).unwrap() + } +} + +impl Drop for TempGitRepo { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.path); + } +} diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index 5ccb505d..f995d6b2 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -2,11 +2,12 @@ use crate::git; use crate::session_runner; -use crate::store::Store; +use crate::store::{CommentAuthor, Review, Store}; use crate::{ blox, branches, BranchTimeline, CommitTimelineItem, ImageTimelineItem, NoteTimelineItem, ReviewTimelineItem, }; +use std::collections::HashSet; use std::path::Path; use std::sync::{Arc, Mutex}; @@ -146,6 +147,12 @@ fn build_branch_timeline(store: &Arc, branch_id: &str) -> Result = commits + .iter() + .filter(|c| !c.sha.is_empty()) + .map(|c| c.sha.as_str()) + .collect(); + // Get notes let db_notes = store .list_notes_for_branch(branch_id) @@ -175,6 +182,7 @@ fn build_branch_timeline(store: &Arc, branch_id: &str) -> Result = db_reviews .into_iter() + .filter(|r| review_is_visible_in_timeline(r, &visible_shas)) .map(|r| { let resolved = store.resolve_session_status(r.session_id.as_deref()); let comment_count = r.comments.len(); @@ -225,6 +233,15 @@ fn build_branch_timeline(store: &Arc, branch_id: &str) -> Result) -> bool { + review.commit_sha.is_empty() + || visible_shas.contains(review.commit_sha.as_str()) + || review + .comments + .iter() + .any(|comment| comment.author == CommentAuthor::User) +} + #[tauri::command] pub async fn get_branch_timeline( store: tauri::State<'_, Mutex>>>, @@ -447,3 +464,109 @@ pub async fn delete_commit( .await .map_err(|e| format!("Delete task failed: {e}"))? } + +#[cfg(test)] +mod tests { + use super::*; + use crate::git::Span; + use crate::store::models::{Branch, Comment, Project, ReviewScope, Workdir}; + use crate::test_utils::TempGitRepo; + + fn repo_with_visible_and_stale_commit() -> (TempGitRepo, String, String) { + let repo = TempGitRepo::new(); + repo.write_file("file.txt", "base\n"); + repo.commit("base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", "main"]); + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("file.txt", "base\nvisible\n"); + let visible_sha = repo.commit("visible change"); + repo.write_file("file.txt", "base\nvisible\nstale\n"); + let stale_sha = repo.commit("stale change"); + repo.run_git(&["reset", "--hard", &visible_sha]); + + (repo, visible_sha, stale_sha) + } + + fn store_with_branch(repo: &TempGitRepo) -> (Arc, Branch) { + let store = Arc::new(Store::in_memory().unwrap()); + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + let workdir = + Workdir::new(&project.id, repo.path().to_str().unwrap()).with_branch(&branch.id); + store.create_workdir(&workdir).unwrap(); + + (store, branch) + } + + #[test] + fn build_branch_timeline_hides_reviews_for_commits_missing_from_git_history() { + let (repo, visible_sha, stale_sha) = repo_with_visible_and_stale_commit(); + let (store, branch) = store_with_branch(&repo); + + let visible_review = Review::new(&branch.id, &visible_sha, ReviewScope::Commit); + let stale_review = Review::new(&branch.id, &stale_sha, ReviewScope::Commit); + store.create_review(&visible_review).unwrap(); + store.create_review(&stale_review).unwrap(); + + let timeline = build_branch_timeline(&store, &branch.id).unwrap(); + + assert_eq!(timeline.commits.len(), 1); + assert_eq!(timeline.commits[0].sha, visible_sha); + assert_eq!(timeline.reviews.len(), 1); + assert_eq!(timeline.reviews[0].id, visible_review.id); + } + + #[test] + fn build_branch_timeline_hides_stale_reviews_with_only_agent_comments() { + let (repo, _visible_sha, stale_sha) = repo_with_visible_and_stale_commit(); + let (store, branch) = store_with_branch(&repo); + + let stale_review = Review::new(&branch.id, &stale_sha, ReviewScope::Commit); + let agent_comment = Comment::new("file.txt", Span::new(2, 3), "Agent note") + .with_author(CommentAuthor::Agent); + store.create_review(&stale_review).unwrap(); + store.add_comment(&stale_review.id, &agent_comment).unwrap(); + + let timeline = build_branch_timeline(&store, &branch.id).unwrap(); + + assert_eq!(timeline.commits.len(), 1); + assert!(timeline.reviews.is_empty()); + } + + #[test] + fn build_branch_timeline_keeps_stale_reviews_with_user_comments() { + let (repo, _visible_sha, stale_sha) = repo_with_visible_and_stale_commit(); + let (store, branch) = store_with_branch(&repo); + + let stale_review = Review::new(&branch.id, &stale_sha, ReviewScope::Commit); + let user_comment = Comment::new("file.txt", Span::new(2, 3), "Please follow up"); + store.create_review(&stale_review).unwrap(); + store.add_comment(&stale_review.id, &user_comment).unwrap(); + + let timeline = build_branch_timeline(&store, &branch.id).unwrap(); + + assert_eq!(timeline.commits.len(), 1); + assert_eq!(timeline.reviews.len(), 1); + assert_eq!(timeline.reviews[0].id, stale_review.id); + assert_eq!(timeline.reviews[0].comment_count, 1); + } + + #[test] + fn build_branch_timeline_hides_stale_reviews_with_deleted_user_comments() { + let (repo, _visible_sha, stale_sha) = repo_with_visible_and_stale_commit(); + let (store, branch) = store_with_branch(&repo); + + let stale_review = Review::new(&branch.id, &stale_sha, ReviewScope::Commit); + let user_comment = Comment::new("file.txt", Span::new(2, 3), "Please follow up"); + store.create_review(&stale_review).unwrap(); + store.add_comment(&stale_review.id, &user_comment).unwrap(); + store.delete_comment(&user_comment.id).unwrap(); + + let timeline = build_branch_timeline(&store, &branch.id).unwrap(); + + assert_eq!(timeline.commits.len(), 1); + assert!(timeline.reviews.is_empty()); + } +}