From a1f65504b5b95b84261c76c266182bd0d73c66d8 Mon Sep 17 00:00:00 2001 From: Jonathan Lane Date: Tue, 28 Apr 2026 12:47:06 -0700 Subject: [PATCH 1/4] Quiet git stats in TUI footer --- nori-rs/tui/docs.md | 17 +- .../bottom_pane/chat_composer/rendering.rs | 5 +- nori-rs/tui/src/bottom_pane/footer.rs | 45 +++- ...r__tests__footer_with_untracked_alert.snap | 6 + nori-rs/tui/src/system_info.rs | 231 +++++------------- 5 files changed, 112 insertions(+), 192 deletions(-) create mode 100644 nori-rs/tui/src/bottom_pane/snapshots/nori_tui__bottom_pane__footer__tests__footer_with_untracked_alert.snap diff --git a/nori-rs/tui/docs.md b/nori-rs/tui/docs.md index b5f577f9b..c31ee7cb2 100644 --- a/nori-rs/tui/docs.md +++ b/nori-rs/tui/docs.md @@ -203,7 +203,8 @@ The `SystemInfo` struct collects environment data in a background thread to avoi |-------|--------| | `git_branch` | Git repository branch name | | `active_skillsets` | Active skillsets from `nori-skillsets list-active` (one name per line; returns all skillsets active for the current directory). Empty vec if the command is unavailable or fails. | -| `git_lines_added` / `git_lines_removed` | Git diff statistics relative to the merge-base with the default branch (PR-like stats) | +| `git_lines_added` / `git_lines_removed` | Git working tree statistics relative to `HEAD` for tracked files | +| `git_has_untracked` | Whether untracked, non-ignored files are present | | `is_worktree` | Whether CWD is a git worktree | | `worktree_name` | Last path component of CWD when parent directory is `.worktrees`; used to display the immutable worktree directory identifier in the footer | | `transcript_location` | Discovered transcript path and token usage when running within an agent environment | @@ -211,15 +212,11 @@ The `SystemInfo` struct collects environment data in a background thread to avoi The `transcript_location` field includes both `token_usage` (total tokens) and `token_breakdown` (detailed input/output/cached breakdown) which are displayed in the TUI footer when Nori runs as a nested agent inside Claude Code, Codex, or Gemini. -**Git Diff Base Resolution** (`system_info.rs: resolve_diff_base()`): - -The git diff stats are computed against the merge-base with the default branch, so they reflect what a PR would show rather than only uncommitted changes. The resolution order is: -1. `origin/HEAD` via `git symbolic-ref` -- detects the remote's default branch name -2. Falls back to checking if local `main` or `master` branches exist -3. Computes `git merge-base HEAD ` to find the common ancestor -4. Falls back to `HEAD` if no default branch can be resolved (shows only uncommitted changes) - -Untracked files (via `git ls-files --others --exclude-standard`) are also counted: their line counts are added to the insertion total. Binary files (non-UTF-8) are silently skipped. This means the statusline stats include new files that haven't been `git add`ed yet. +The footer git stats are intentionally scoped to uncommitted tracked-file +changes so the statusline stays compact in long-lived branches or repositories +with large histories. Untracked, non-ignored files render as a compact red `!` +alert instead of contributing line counts. The `/diff` command still produces a +PR-like diff when users ask for the full change context. Two collection methods are provided: - `collect_for_directory()` - Basic collection without first-message matching (test-only) diff --git a/nori-rs/tui/src/bottom_pane/chat_composer/rendering.rs b/nori-rs/tui/src/bottom_pane/chat_composer/rendering.rs index 92d6c646f..e43208449 100644 --- a/nori-rs/tui/src/bottom_pane/chat_composer/rendering.rs +++ b/nori-rs/tui/src/bottom_pane/chat_composer/rendering.rs @@ -38,6 +38,7 @@ impl ChatComposer { nori_version_source, git_lines_added, git_lines_removed, + git_has_untracked, ) = if let Some(ref info) = self.system_info { ( info.git_branch.clone(), @@ -46,9 +47,10 @@ impl ChatComposer { info.nori_version_source, info.git_lines_added, info.git_lines_removed, + info.git_has_untracked, ) } else { - (None, Vec::new(), None, None, None, None) + (None, Vec::new(), None, None, None, None, false) }; // Extract token breakdown and agent kind from transcript location @@ -110,6 +112,7 @@ impl ChatComposer { nori_version_source, git_lines_added, git_lines_removed, + git_has_untracked, is_worktree: self .system_info .as_ref() diff --git a/nori-rs/tui/src/bottom_pane/footer.rs b/nori-rs/tui/src/bottom_pane/footer.rs index 491968b3e..9ff32078b 100644 --- a/nori-rs/tui/src/bottom_pane/footer.rs +++ b/nori-rs/tui/src/bottom_pane/footer.rs @@ -36,6 +36,7 @@ pub(crate) struct FooterProps { pub(crate) nori_version_source: Option, pub(crate) git_lines_added: Option, pub(crate) git_lines_removed: Option, + pub(crate) git_has_untracked: bool, /// Whether the current directory is a git worktree (not the main repo). /// When true, the git branch indicator is shown in orange instead of yellow. pub(crate) is_worktree: bool, @@ -350,16 +351,27 @@ fn footer_segments(props: &FooterProps) -> Vec> { ])); } - // Add git stats if available and enabled: "+10 -3" (green for added, red for removed) - if config.is_enabled(FooterSegment::GitStats) - && let (Some(added), Some(removed)) = (props.git_lines_added, props.git_lines_removed) - && (added > 0 || removed > 0) - { - segments.push(Line::from(vec![ - Span::from(format!("+{added}")).green(), - Span::from(" ").dim(), - Span::from(format!("-{removed}")).red(), - ])); + // Add git stats if available and enabled: "+10 -3" plus "!" for untracked files. + if config.is_enabled(FooterSegment::GitStats) { + let mut spans = Vec::new(); + if let (Some(added), Some(removed)) = (props.git_lines_added, props.git_lines_removed) + && (added > 0 || removed > 0) + { + spans.extend([ + Span::from(format!("+{added}")).green(), + Span::from(" ").dim(), + Span::from(format!("-{removed}")).red(), + ]); + } + if props.git_has_untracked { + if !spans.is_empty() { + spans.push(Span::from(" ").dim()); + } + spans.push(Span::from("!").red().bold()); + } + if !spans.is_empty() { + segments.push(Line::from(spans)); + } } // Add context window info if available and enabled: "Context 27% (34K)". @@ -644,6 +656,7 @@ mod tests { nori_version_source: None, git_lines_added: None, git_lines_removed: None, + git_has_untracked: false, is_worktree: false, input_tokens: None, output_tokens: None, @@ -796,6 +809,18 @@ mod tests { ); } + #[test] + fn footer_with_untracked_alert() { + snapshot_footer( + "footer_with_untracked_alert", + FooterProps { + git_branch: Some("main".to_string()), + git_has_untracked: true, + ..default_props() + }, + ); + } + #[test] fn footer_with_no_nori_info() { snapshot_footer( diff --git a/nori-rs/tui/src/bottom_pane/snapshots/nori_tui__bottom_pane__footer__tests__footer_with_untracked_alert.snap b/nori-rs/tui/src/bottom_pane/snapshots/nori_tui__bottom_pane__footer__tests__footer_with_untracked_alert.snap new file mode 100644 index 000000000..92c70dad5 --- /dev/null +++ b/nori-rs/tui/src/bottom_pane/snapshots/nori_tui__bottom_pane__footer__tests__footer_with_untracked_alert.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/bottom_pane/footer.rs +assertion_line: 680 +expression: terminal.backend() +--- +" ⎇ main · ! · ? for shortcuts " diff --git a/nori-rs/tui/src/system_info.rs b/nori-rs/tui/src/system_info.rs index cfbbf1943..cc0b4e30d 100644 --- a/nori-rs/tui/src/system_info.rs +++ b/nori-rs/tui/src/system_info.rs @@ -34,6 +34,8 @@ pub(crate) struct SystemInfo { pub(crate) nori_version_source: Option, pub(crate) git_lines_added: Option, pub(crate) git_lines_removed: Option, + /// Whether there are untracked, non-ignored files in the current git repo. + pub(crate) git_has_untracked: bool, /// Whether the current directory is a git worktree (not the main repo) pub(crate) is_worktree: bool, /// The worktree directory name (last path component when parent is `.worktrees/`) @@ -135,6 +137,7 @@ impl SystemInfo { nori_version_source, git_lines_added, git_lines_removed, + git_has_untracked: has_untracked_files(dir), is_worktree, worktree_name: if is_worktree { dir.and_then(extract_worktree_name) @@ -308,101 +311,8 @@ fn get_nori_profile() -> Option { None } -/// Parse the output of `git symbolic-ref refs/remotes/origin/HEAD` to extract -/// the default branch name. Returns `None` if the output is malformed. -fn parse_origin_head(output: &str) -> Option { - let trimmed = output.trim(); - let suffix = trimmed.strip_prefix("ref: refs/remotes/origin/")?; - if suffix.is_empty() { - return None; - } - Some(suffix.to_string()) -} - -/// Count the number of lines in content (non-empty content). -fn count_lines_in_content(content: &str) -> i32 { - if content.is_empty() { - return 0; - } - content.lines().count() as i32 -} - -/// Resolve the diff base ref to compare against. -/// -/// Tries, in order: -/// 1. `origin/HEAD` (via `git symbolic-ref`) to find the remote default branch -/// 2. `main` branch exists -/// 3. `master` branch exists -/// 4. Falls back to `"HEAD"` (uncommitted changes only) -/// -/// Once a default branch is found, computes the merge-base with HEAD so that -/// the diff reflects what a PR would show. -fn resolve_diff_base(dir: Option<&std::path::Path>) -> String { - // Try origin/HEAD first - if let Some(default_branch) = get_origin_head(dir) - && let Some(merge_base) = get_merge_base(dir, &format!("origin/{default_branch}")) - { - return merge_base; - } - - // Try common default branch names - for branch in &["main", "master"] { - if branch_exists(dir, branch) - && let Some(merge_base) = get_merge_base(dir, branch) - { - return merge_base; - } - } - - // Fallback: diff against HEAD (uncommitted changes only) - "HEAD".to_string() -} - -fn get_origin_head(dir: Option<&std::path::Path>) -> Option { - let mut cmd = Command::new("git"); - cmd.args(["symbolic-ref", "refs/remotes/origin/HEAD"]); - if let Some(d) = dir { - cmd.current_dir(d); - } - let output = cmd.output().ok()?; - if !output.status.success() { - return None; - } - let stdout = String::from_utf8(output.stdout).ok()?; - parse_origin_head(&stdout) -} - -fn branch_exists(dir: Option<&std::path::Path>, branch: &str) -> bool { - let mut cmd = Command::new("git"); - cmd.args(["rev-parse", "--verify", branch]); - if let Some(d) = dir { - cmd.current_dir(d); - } - cmd.stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()); - cmd.status().is_ok_and(|s| s.success()) -} - -fn get_merge_base(dir: Option<&std::path::Path>, target: &str) -> Option { - let mut cmd = Command::new("git"); - cmd.args(["merge-base", "HEAD", target]); - if let Some(d) = dir { - cmd.current_dir(d); - } - let output = cmd.output().ok()?; - if !output.status.success() { - return None; - } - let sha = String::from_utf8(output.stdout).ok()?; - let sha = sha.trim(); - if sha.is_empty() { - return None; - } - Some(sha.to_string()) -} - -/// Count lines in untracked files (files not yet added to git). -fn count_untracked_lines(dir: Option<&std::path::Path>) -> i32 { +/// Detect untracked files (files not yet added to git and not ignored). +fn has_untracked_files(dir: Option<&std::path::Path>) -> bool { let mut cmd = Command::new("git"); cmd.args(["ls-files", "--others", "--exclude-standard"]); if let Some(d) = dir { @@ -411,34 +321,15 @@ fn count_untracked_lines(dir: Option<&std::path::Path>) -> i32 { let output = match cmd.output() { Ok(output) if output.status.success() => output, - _ => return 0, - }; - - let file_list = match String::from_utf8(output.stdout) { - Ok(s) => s, - Err(_) => return 0, + _ => return false, }; - let base_dir = dir - .map(std::path::PathBuf::from) - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()); - - let mut total_lines = 0; - for file in file_list.lines().map(str::trim).filter(|s| !s.is_empty()) { - let path = base_dir.join(file); - if let Ok(content) = std::fs::read_to_string(&path) { - total_lines += count_lines_in_content(&content); - } - // Skip binary files / files that can't be read as UTF-8 - } - total_lines + output.stdout.iter().any(|&byte| byte != b'\n') } fn get_git_stats(dir: Option<&std::path::Path>) -> (Option, Option) { - let diff_base = resolve_diff_base(dir); - let mut cmd = Command::new("git"); - cmd.args(["diff", &diff_base, "--shortstat"]); + cmd.args(["diff", "HEAD", "--shortstat"]); if let Some(d) = dir { cmd.current_dir(d); } @@ -457,17 +348,7 @@ fn get_git_stats(dir: Option<&std::path::Path>) -> (Option, Option) { Err(_) => return (None, None), }; - let (added, removed) = parse_git_shortstat(&stats); - - // Add untracked file lines to insertions - let untracked = count_untracked_lines(dir); - if untracked > 0 { - let added = Some(added.unwrap_or(0) + untracked); - let removed = Some(removed.unwrap_or(0)); - return (added, removed); - } - - (added, removed) + parse_git_shortstat(&stats) } fn parse_git_shortstat(output: &str) -> (Option, Option) { @@ -717,6 +598,31 @@ fn is_git_worktree(dir: Option<&std::path::Path>) -> bool { #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; + use std::path::Path; + use std::process::Stdio; + + fn run_git(dir: &Path, args: &[&str]) { + let status = Command::new("git") + .args(args) + .current_dir(dir) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .expect("git command should run"); + assert!(status.success(), "git {args:?} should succeed"); + } + + fn create_git_repo() -> tempfile::TempDir { + let dir = tempfile::TempDir::new().expect("temp dir should be created"); + run_git(dir.path(), &["init", "-b", "main"]); + run_git(dir.path(), &["config", "user.email", "test@example.com"]); + run_git(dir.path(), &["config", "user.name", "Test User"]); + std::fs::write(dir.path().join("file.txt"), "initial\n").expect("write file"); + run_git(dir.path(), &["add", "."]); + run_git(dir.path(), &["commit", "-m", "initial"]); + dir + } #[test] fn test_parse_nori_version_skillsets_format() { @@ -842,6 +748,32 @@ mod tests { assert_eq!(removed, None); } + #[test] + fn test_get_git_stats_ignores_committed_branch_diff() { + let dir = create_git_repo(); + run_git(dir.path(), &["switch", "-c", "feature"]); + std::fs::write(dir.path().join("file.txt"), "initial\nfeature\n") + .expect("write feature change"); + run_git(dir.path(), &["add", "."]); + run_git(dir.path(), &["commit", "-m", "feature change"]); + + let stats = get_git_stats(Some(dir.path())); + + assert_eq!(stats, (None, None)); + } + + #[test] + fn test_git_stats_do_not_count_untracked_file_lines() { + let dir = create_git_repo(); + std::fs::write(dir.path().join("untracked.txt"), "one\ntwo\nthree\n") + .expect("write untracked file"); + + let stats = get_git_stats(Some(dir.path())); + + assert_eq!(stats, (None, None)); + assert!(has_untracked_files(Some(dir.path()))); + } + #[test] fn test_parse_nori_version_with_program_name() { // Old format: "nori-ai 19.1.1" @@ -967,49 +899,6 @@ Filesystem 1024-blocks Used Available Capacity Mounted on assert_eq!(extract_worktree_name(path), None); } - #[test] - fn test_get_default_branch_prefers_origin_head() { - // When origin/HEAD points to a branch, use that branch name - assert_eq!( - parse_origin_head("ref: refs/remotes/origin/develop"), - Some("develop".to_string()) - ); - } - - #[test] - fn test_get_default_branch_parses_main() { - assert_eq!( - parse_origin_head("ref: refs/remotes/origin/main"), - Some("main".to_string()) - ); - } - - #[test] - fn test_get_default_branch_parses_master() { - assert_eq!( - parse_origin_head("ref: refs/remotes/origin/master"), - Some("master".to_string()) - ); - } - - #[test] - fn test_get_default_branch_handles_empty() { - assert_eq!(parse_origin_head(""), None); - } - - #[test] - fn test_get_default_branch_handles_malformed() { - assert_eq!(parse_origin_head("not a valid ref"), None); - } - - #[test] - fn test_count_lines_in_content() { - assert_eq!(count_lines_in_content("hello\nworld\n"), 2); - assert_eq!(count_lines_in_content("single line"), 1); - assert_eq!(count_lines_in_content(""), 0); - assert_eq!(count_lines_in_content("a\nb\nc"), 3); - } - #[test] fn test_parse_active_skillsets_multiple_lines() { let output = "amol\nrust-dev\n"; From 74d4d2db894ae79e3b84a83ccb9bc61eb79e49f8 Mon Sep 17 00:00:00 2001 From: Jonathan Lane Date: Thu, 30 Apr 2026 15:16:57 -0700 Subject: [PATCH 2/4] test(tui): update acp_tool_calls snapshots for quiet statusbar footer change Co-Authored-By: Claude Sonnet 4.6 --- .../acp_tool_calls__acp_generic_tool_call_resolved_name.snap | 2 +- .../snapshots/acp_tool_calls__acp_multi_call_exploring.snap | 2 +- .../snapshots/acp_tool_calls__acp_no_orphan_tool_cells.snap | 2 +- ..._tool_calls__acp_stuck_tool_calls_agent_message_renders.snap | 2 +- .../tests/snapshots/acp_tool_calls__acp_tool_call_echo.snap | 2 +- .../snapshots/acp_tool_calls__acp_tool_call_no_duplicates.snap | 2 +- .../tests/snapshots/acp_tool_calls__acp_tool_call_read.snap | 2 +- .../acp_tool_calls__acp_tool_calls_during_final_stream.snap | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_generic_tool_call_resolved_name.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_generic_tool_call_resolved_name.snap index 7812c755d..5fb05f42b 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_generic_tool_call_resolved_name.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_generic_tool_call_resolved_name.snap @@ -13,4 +13,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_multi_call_exploring.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_multi_call_exploring.snap index a8e2c7e9e..0ca2da9e5 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_multi_call_exploring.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_multi_call_exploring.snap @@ -22,4 +22,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_no_orphan_tool_cells.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_no_orphan_tool_cells.snap index f20f1f6a2..7103ebc0d 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_no_orphan_tool_cells.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_no_orphan_tool_cells.snap @@ -19,4 +19,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_stuck_tool_calls_agent_message_renders.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_stuck_tool_calls_agent_message_renders.snap index 9637c9e80..fbe8ecf9b 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_stuck_tool_calls_agent_message_renders.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_stuck_tool_calls_agent_message_renders.snap @@ -13,4 +13,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_echo.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_echo.snap index a8b012256..f98bc1aa2 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_echo.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_echo.snap @@ -15,4 +15,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_no_duplicates.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_no_duplicates.snap index b68bfc55a..6544df1f5 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_no_duplicates.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_no_duplicates.snap @@ -18,4 +18,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_read.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_read.snap index 5ec78e8be..a9154ecb2 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_read.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_read.snap @@ -15,4 +15,4 @@ expression: normalize_for_input_snapshot(session.screen_contents()) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts diff --git a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_calls_during_final_stream.snap b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_calls_during_final_stream.snap index 9487f478d..83ea477c5 100644 --- a/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_calls_during_final_stream.snap +++ b/nori-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_calls_during_final_stream.snap @@ -22,4 +22,4 @@ expression: normalize_for_input_snapshot(contents) › [DEFAULT_PROMPT] - ⎇ master · Approvals: Agent · ? for shortcuts + ⎇ master · ! · Approvals: Agent · ? for shortcuts From 0b96151fc60e1bd66f228c1ab23c9e0f83c8f64c Mon Sep 17 00:00:00 2001 From: Jonathan Lane Date: Thu, 30 Apr 2026 15:58:57 -0700 Subject: [PATCH 3/4] fix: diff syntax highlighting and relative line numbers --- nori-rs/Cargo.lock | 1 + nori-rs/acp/src/connection/sacp_connection.rs | 5 +- nori-rs/acp/src/translator.rs | 103 ++++++- nori-rs/core/Cargo.toml | 1 + nori-rs/core/src/util.rs | 56 ++++ nori-rs/tui/src/app/event_handling.rs | 12 +- .../tui/src/bottom_pane/approval_overlay.rs | 12 +- nori-rs/tui/src/client_tool_cell.rs | 20 +- nori-rs/tui/src/diff_render.rs | 260 ++++++++++++++---- nori-rs/tui/src/render/highlight.rs | 59 ++++ 10 files changed, 450 insertions(+), 79 deletions(-) diff --git a/nori-rs/Cargo.lock b/nori-rs/Cargo.lock index cd18412d5..d68cb9c19 100644 --- a/nori-rs/Cargo.lock +++ b/nori-rs/Cargo.lock @@ -942,6 +942,7 @@ dependencies = [ "core-foundation 0.9.4", "core_test_support", "ctor 0.5.0", + "diffy", "dirs", "dunce", "encoding_rs", diff --git a/nori-rs/acp/src/connection/sacp_connection.rs b/nori-rs/acp/src/connection/sacp_connection.rs index 9c75fb294..9b9928a35 100644 --- a/nori-rs/acp/src/connection/sacp_connection.rs +++ b/nori-rs/acp/src/connection/sacp_connection.rs @@ -219,8 +219,9 @@ impl SacpConnection { connection: ConnectionTo| { // Translate ACP permission request to Codex approval event. let event = if let Some(patch_event) = - translator::permission_request_to_patch_approval_event(&request) - { + translator::permission_request_to_patch_approval_event( + &request, &cwd, + ) { ApprovalEventType::Patch(patch_event) } else { let exec_event = translator::permission_request_to_approval_event( diff --git a/nori-rs/acp/src/translator.rs b/nori-rs/acp/src/translator.rs index 13ad2c34a..ce93478e5 100644 --- a/nori-rs/acp/src/translator.rs +++ b/nori-rs/acp/src/translator.rs @@ -705,6 +705,7 @@ pub fn is_patch_operation( pub fn tool_call_to_file_change( kind: Option<&acp::ToolKind>, raw_input: Option<&serde_json::Value>, + cwd: &std::path::Path, ) -> Option<(PathBuf, FileChange)> { let input = raw_input?; let file_path = extract_file_path(Some(input))?; @@ -737,8 +738,9 @@ pub fn tool_call_to_file_change( let old_string = input.get("old_string").and_then(|v| v.as_str())?; let new_string = input.get("new_string").and_then(|v| v.as_str())?; - // Generate unified diff using diffy - let unified_diff = diffy::create_patch(old_string, new_string).to_string(); + // Generate unified diff using diffy with file context if available + let unified_diff = + codex_core::util::create_patch_with_context(&path, cwd, old_string, new_string); Some(( path, @@ -755,6 +757,7 @@ pub fn tool_call_to_file_change( /// patch approval UI in the TUI instead of the generic exec approval. pub fn permission_request_to_patch_approval_event( request: &acp::RequestPermissionRequest, + cwd: &std::path::Path, ) -> Option { let kind = request.tool_call.fields.kind.as_ref(); let raw_input = request.tool_call.fields.raw_input.as_ref(); @@ -764,7 +767,7 @@ pub fn permission_request_to_patch_approval_event( return None; } - let (path, change) = tool_call_to_file_change(kind, raw_input)?; + let (path, change) = tool_call_to_file_change(kind, raw_input, cwd)?; let mut changes = HashMap::new(); changes.insert(path, change); @@ -1237,7 +1240,11 @@ mod tests { "new_string": "fn new() {\n println!(\"hello\");\n}" }); - let result = tool_call_to_file_change(Some(&acp::ToolKind::Edit), Some(&input)); + let result = tool_call_to_file_change( + Some(&acp::ToolKind::Edit), + Some(&input), + std::path::Path::new("."), + ); assert!(result.is_some()); let (path, change) = result.unwrap(); @@ -1258,6 +1265,70 @@ mod tests { } } + #[test] + fn test_tool_call_to_file_change_edit_with_context() { + let temp_dir = tempfile::tempdir().unwrap(); + let file_path = temp_dir.path().join("test.txt"); + let content = "line 1\nline 2\nline 3\nline 4\nline 5\n"; + std::fs::write(&file_path, content).unwrap(); + + let input = serde_json::json!({ + "path": file_path.to_str().unwrap(), + "old_string": "line 3\n", + "new_string": "line 3 modified\n" + }); + + // Use the temp dir as cwd + let result = + tool_call_to_file_change(Some(&acp::ToolKind::Edit), Some(&input), temp_dir.path()); + assert!(result.is_some()); + + let (_, change) = result.unwrap(); + if let FileChange::Update { unified_diff, .. } = change { + // The diff should show line 3, not line 1 + assert!( + unified_diff.contains("@@ -1,5 +1,5 @@") + || unified_diff.contains("@@ -1,4 +1,4 @@") + || unified_diff.contains("-line 3") + || unified_diff.contains("line 2") + ); + + // Wait, if it uses the whole file, it should have the correct line numbers. + // For a 5 line file, it might still show @@ -1,5 +1,5 @@ if the whole file is context. + // Let's use a larger file to be sure. + + let large_content = (1..=100).map(|i| format!("line {i}\n")).collect::(); + std::fs::write(&file_path, &large_content).unwrap(); + + let input2 = serde_json::json!({ + "path": file_path.to_str().unwrap(), + "old_string": "line 50\n", + "new_string": "line 50 modified\n" + }); + + let result2 = tool_call_to_file_change( + Some(&acp::ToolKind::Edit), + Some(&input2), + temp_dir.path(), + ); + let (_, change2) = result2.unwrap(); + if let FileChange::Update { unified_diff, .. } = change2 { + // The diff should contain line 50 + assert!(unified_diff.contains("-line 50")); + assert!(unified_diff.contains("+line 50 modified")); + // And the hunk header should NOT be @ -1,x +1,x + assert!(!unified_diff.contains("@@ -1,")); + // It should be adjusted to line 50 + assert!( + unified_diff.contains("@@ -50 +50 @@") + || unified_diff.contains("@@ -50,1 +50,1 @@") + ); + } + } else { + panic!("Expected FileChange::Update"); + } + } + #[test] fn test_tool_call_to_file_change_write() { let input = serde_json::json!({ @@ -1265,7 +1336,11 @@ mod tests { "content": "// New file\nfn main() {}\n" }); - let result = tool_call_to_file_change(Some(&acp::ToolKind::Edit), Some(&input)); + let result = tool_call_to_file_change( + Some(&acp::ToolKind::Edit), + Some(&input), + std::path::Path::new("."), + ); assert!(result.is_some()); let (path, change) = result.unwrap(); @@ -1286,7 +1361,11 @@ mod tests { "content": "// File to delete\n" }); - let result = tool_call_to_file_change(Some(&acp::ToolKind::Delete), Some(&input)); + let result = tool_call_to_file_change( + Some(&acp::ToolKind::Delete), + Some(&input), + std::path::Path::new("."), + ); assert!(result.is_some()); let (path, change) = result.unwrap(); @@ -1306,7 +1385,11 @@ mod tests { "content": "some content" }); - let result = tool_call_to_file_change(Some(&acp::ToolKind::Edit), Some(&input)); + let result = tool_call_to_file_change( + Some(&acp::ToolKind::Edit), + Some(&input), + std::path::Path::new("."), + ); assert!(result.is_none()); } @@ -1330,7 +1413,7 @@ mod tests { vec![], ); - let event = permission_request_to_patch_approval_event(&request); + let event = permission_request_to_patch_approval_event(&request, std::path::Path::new(".")); assert!(event.is_some()); let event = event.unwrap(); @@ -1365,7 +1448,7 @@ mod tests { vec![], ); - let event = permission_request_to_patch_approval_event(&request); + let event = permission_request_to_patch_approval_event(&request, std::path::Path::new(".")); assert!(event.is_none()); } @@ -1503,7 +1586,7 @@ mod tests { vec![], ); - let event = permission_request_to_patch_approval_event(&request); + let event = permission_request_to_patch_approval_event(&request, std::path::Path::new(".")); assert!(event.is_some()); let event = event.unwrap(); diff --git a/nori-rs/core/Cargo.toml b/nori-rs/core/Cargo.toml index bba732394..d2c6f2bec 100644 --- a/nori-rs/core/Cargo.toml +++ b/nori-rs/core/Cargo.toml @@ -32,6 +32,7 @@ codex-utils-pty = { workspace = true } codex-utils-readiness = { workspace = true } codex-utils-string = { workspace = true } codex-windows-sandbox = { package = "codex-windows-sandbox", path = "../windows-sandbox-rs" } +diffy = { workspace = true } dirs = { workspace = true } dunce = { workspace = true } env-flags = { workspace = true } diff --git a/nori-rs/core/src/util.rs b/nori-rs/core/src/util.rs index 06a55dcc4..39d1b4368 100644 --- a/nori-rs/core/src/util.rs +++ b/nori-rs/core/src/util.rs @@ -1,5 +1,61 @@ use tracing::debug; +pub fn create_patch_with_context( + path: &std::path::Path, + cwd: &std::path::Path, + old_text: &str, + new_text: &str, +) -> String { + let full_path = if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + }; + + let line_offset = if let Ok(file_content) = std::fs::read_to_string(&full_path) { + if let Some(offset) = file_content.find(old_text) { + Some(file_content[..offset].lines().count() + 1) + } else { + None + } + } else { + None + }; + + let patch = diffy::create_patch(old_text, new_text).to_string(); + if let Some(offset) = line_offset + && offset > 1 + { + return adjust_patch_line_numbers(&patch, offset); + } + patch +} + +fn adjust_patch_line_numbers(patch: &str, line_offset: usize) -> String { + let re = regex::Regex::new(r"^@@ -(\d+)(,?\d*) \+(\d+)(,?\d*) @@").unwrap(); + let mut result = String::new(); + for line in patch.lines() { + if let Some(caps) = re.captures(line) { + let old_start: usize = caps[1].parse().unwrap_or(1); + let new_start: usize = caps[3].parse().unwrap_or(1); + let old_rest = &caps[2]; + let new_rest = &caps[4]; + + let adjusted_old_start = old_start + line_offset - 1; + let adjusted_new_start = new_start + line_offset - 1; + + result.push_str(&format!( + "@@ -{}{}{} +{}{}{} @@\n", + adjusted_old_start, old_rest, "", adjusted_new_start, new_rest, "" + )); + } else { + result.push_str(line); + result.push('\n'); + } + } + result +} + pub(crate) fn try_parse_error_message(text: &str) -> String { debug!("Parsing server error response: {}", text); let json = serde_json::from_str::(text).unwrap_or_default(); diff --git a/nori-rs/tui/src/app/event_handling.rs b/nori-rs/tui/src/app/event_handling.rs index 936e5d07b..b729f0690 100644 --- a/nori-rs/tui/src/app/event_handling.rs +++ b/nori-rs/tui/src/app/event_handling.rs @@ -528,11 +528,15 @@ impl App { | nori_protocol::ToolKind::Delete | nori_protocol::ToolKind::Move ) { - let mut changes = - client_tool_cell::diff_changes_from_artifacts(&snapshot.artifacts); + let mut changes = client_tool_cell::diff_changes_from_artifacts( + &snapshot.artifacts, + &cwd, + ); if changes.is_empty() { - changes = - client_tool_cell::changes_from_invocation(&snapshot.invocation); + changes = client_tool_cell::changes_from_invocation( + &snapshot.invocation, + &cwd, + ); } if changes.is_empty() { None diff --git a/nori-rs/tui/src/bottom_pane/approval_overlay.rs b/nori-rs/tui/src/bottom_pane/approval_overlay.rs index b5d2dc4bc..ddda9c8cd 100644 --- a/nori-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/nori-rs/tui/src/bottom_pane/approval_overlay.rs @@ -473,11 +473,15 @@ impl From for ApprovalRequestState { // For edit-like tools, try to render a DiffSummary from the snapshot if is_edit_like { - let mut changes = - crate::client_tool_cell::diff_changes_from_artifacts(&snapshot.artifacts); + let mut changes = crate::client_tool_cell::diff_changes_from_artifacts( + &snapshot.artifacts, + &cwd, + ); if changes.is_empty() { - changes = - crate::client_tool_cell::changes_from_invocation(&snapshot.invocation); + changes = crate::client_tool_cell::changes_from_invocation( + &snapshot.invocation, + &cwd, + ); } if !changes.is_empty() { let header: Vec> = diff --git a/nori-rs/tui/src/client_tool_cell.rs b/nori-rs/tui/src/client_tool_cell.rs index b4fc53808..1d3e37d59 100644 --- a/nori-rs/tui/src/client_tool_cell.rs +++ b/nori-rs/tui/src/client_tool_cell.rs @@ -316,11 +316,11 @@ impl ClientToolCell { } // Render diff content from artifacts or invocation - let diff_changes = diff_changes_from_artifacts(&self.snapshot.artifacts); + let diff_changes = diff_changes_from_artifacts(&self.snapshot.artifacts, &self.cwd); let changes = if !diff_changes.is_empty() { diff_changes } else { - changes_from_invocation(&self.snapshot.invocation) + changes_from_invocation(&self.snapshot.invocation, &self.cwd) }; if !changes.is_empty() { @@ -658,6 +658,7 @@ fn extract_error_text(snapshot: &nori_protocol::ToolSnapshot) -> Option pub(crate) fn diff_changes_from_artifacts( artifacts: &[nori_protocol::Artifact], + cwd: &std::path::Path, ) -> std::collections::HashMap { let mut changes = std::collections::HashMap::new(); for artifact in artifacts { @@ -667,7 +668,12 @@ pub(crate) fn diff_changes_from_artifacts( content: change.new_text.clone(), }, Some(old_text) => codex_core::protocol::FileChange::Update { - unified_diff: diffy::create_patch(old_text, &change.new_text).to_string(), + unified_diff: codex_core::util::create_patch_with_context( + &change.path, + cwd, + old_text, + &change.new_text, + ), move_path: None, }, }; @@ -679,6 +685,7 @@ pub(crate) fn diff_changes_from_artifacts( pub(crate) fn changes_from_invocation( invocation: &Option, + cwd: &std::path::Path, ) -> std::collections::HashMap { let mut changes = std::collections::HashMap::new(); match invocation.as_ref() { @@ -689,7 +696,12 @@ pub(crate) fn changes_from_invocation( content: change.new_text.clone(), }, Some(old_text) => codex_core::protocol::FileChange::Update { - unified_diff: diffy::create_patch(old_text, &change.new_text).to_string(), + unified_diff: codex_core::util::create_patch_with_context( + &change.path, + cwd, + old_text, + &change.new_text, + ), move_path: None, }, }; diff --git a/nori-rs/tui/src/diff_render.rs b/nori-rs/tui/src/diff_render.rs index 3ec26e7ff..6bccf50b9 100644 --- a/nori-rs/tui/src/diff_render.rs +++ b/nori-rs/tui/src/diff_render.rs @@ -14,6 +14,7 @@ use std::path::PathBuf; use crate::exec_command::relativize_to_home; use crate::render::Insets; +use crate::render::highlight::highlight_code_to_lines_for_path; use crate::render::line_utils::prefix_lines; use crate::render::renderable::ColumnRenderable; use crate::render::renderable::InsetRenderable; @@ -126,6 +127,13 @@ enum DiffLineType { Context, } +#[derive(Clone, Copy)] +struct DiffLineLayout { + width: usize, + line_number_width: usize, + outer_pad: usize, +} + pub struct DiffSummary { changes: HashMap, cwd: PathBuf, @@ -140,13 +148,13 @@ impl DiffSummary { impl Renderable for FileChange { fn render(&self, area: Rect, buf: &mut Buffer) { let mut lines = vec![]; - render_change(self, &mut lines, area.width as usize); + render_change(self, &mut lines, area.width as usize, None); Paragraph::new(lines).render(area, buf); } fn desired_height(&self, width: u16) -> u16 { let mut lines = vec![]; - render_change(self, &mut lines, width as usize); + render_change(self, &mut lines, width as usize, None); lines.len() as u16 } } @@ -165,7 +173,10 @@ impl From for Box { rows.push(Box::new(path)); rows.push(Box::new(RtLine::from(""))); rows.push(Box::new(InsetRenderable::new( - Box::new(row.change) as Box, + Box::new(PathAwareFileChange { + path: row.path, + change: row.change, + }) as Box, Insets::tlbr(0, 2, 0, 0), ))); } @@ -174,6 +185,30 @@ impl From for Box { } } +struct PathAwareFileChange { + path: PathBuf, + change: FileChange, +} + +impl Renderable for PathAwareFileChange { + fn render(&self, area: Rect, buf: &mut Buffer) { + let mut lines = vec![]; + render_change( + &self.change, + &mut lines, + area.width as usize, + Some(&self.path), + ); + Paragraph::new(lines).render(area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + let mut lines = vec![]; + render_change(&self.change, &mut lines, width as usize, Some(&self.path)); + lines.len() as u16 + } +} + pub(crate) fn create_diff_summary( changes: &HashMap, cwd: &Path, @@ -292,6 +327,7 @@ fn render_changes_block(rows: Vec, wrap_cols: usize, cwd: &Path) -> Vec, wrap_cols: usize, cwd: &Path) -> Vec>, width: usize) { - render_change_with_ctx(change, out, width, 0, &DiffRenderStyleContext::new()); +fn render_change( + change: &FileChange, + out: &mut Vec>, + width: usize, + path: Option<&Path>, +) { + render_change_with_ctx(change, out, width, 0, &DiffRenderStyleContext::new(), path); } fn render_change_with_ctx( @@ -309,33 +350,40 @@ fn render_change_with_ctx( width: usize, outer_pad: usize, ctx: &DiffRenderStyleContext, + path: Option<&Path>, ) { match change { FileChange::Add { content } => { let line_number_width = line_number_width(content.lines().count()); for (i, raw) in content.lines().enumerate() { - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_maybe_path( i + 1, DiffLineType::Insert, raw, - width, - line_number_width, - outer_pad, + DiffLineLayout { + width, + line_number_width, + outer_pad, + }, ctx, + path, )); } } FileChange::Delete { content } => { let line_number_width = line_number_width(content.lines().count()); for (i, raw) in content.lines().enumerate() { - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_maybe_path( i + 1, DiffLineType::Delete, raw, - width, - line_number_width, - outer_pad, + DiffLineLayout { + width, + line_number_width, + outer_pad, + }, ctx, + path, )); } } @@ -379,40 +427,49 @@ fn render_change_with_ctx( match l { diffy::Line::Insert(text) => { let s = text.trim_end_matches('\n'); - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_maybe_path( new_ln, DiffLineType::Insert, s, - width, - line_number_width, - outer_pad, + DiffLineLayout { + width, + line_number_width, + outer_pad, + }, ctx, + path, )); new_ln += 1; } diffy::Line::Delete(text) => { let s = text.trim_end_matches('\n'); - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_maybe_path( old_ln, DiffLineType::Delete, s, - width, - line_number_width, - outer_pad, + DiffLineLayout { + width, + line_number_width, + outer_pad, + }, ctx, + path, )); old_ln += 1; } diffy::Line::Context(text) => { let s = text.trim_end_matches('\n'); - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_maybe_path( new_ln, DiffLineType::Context, s, - width, - line_number_width, - outer_pad, + DiffLineLayout { + width, + line_number_width, + outer_pad, + }, ctx, + path, )); old_ln += 1; new_ln += 1; @@ -461,9 +518,32 @@ fn push_wrapped_diff_line( line_number: usize, kind: DiffLineType, text: &str, - width: usize, - line_number_width: usize, - outer_pad: usize, + layout: DiffLineLayout, + ctx: &DiffRenderStyleContext, +) -> Vec> { + push_wrapped_diff_line_for_path(line_number, kind, text, Path::new(""), layout, ctx) +} + +fn push_wrapped_diff_line_maybe_path( + line_number: usize, + kind: DiffLineType, + text: &str, + layout: DiffLineLayout, + ctx: &DiffRenderStyleContext, + path: Option<&Path>, +) -> Vec> { + match path { + Some(path) => push_wrapped_diff_line_for_path(line_number, kind, text, path, layout, ctx), + None => push_wrapped_diff_line(line_number, kind, text, layout, ctx), + } +} + +fn push_wrapped_diff_line_for_path( + line_number: usize, + kind: DiffLineType, + text: &str, + path: &Path, + layout: DiffLineLayout, ctx: &DiffRenderStyleContext, ) -> Vec> { let ln_str = line_number.to_string(); @@ -471,7 +551,7 @@ fn push_wrapped_diff_line( // Reserve a fixed number of spaces (equal to the widest line number plus a // trailing spacer) so the sign column stays aligned across the diff block. - let gutter_width = line_number_width.max(1); + let gutter_width = layout.line_number_width.max(1); let prefix_cols = gutter_width + 1; let mut first = true; @@ -494,7 +574,7 @@ fn push_wrapped_diff_line( // Fit the content for the current terminal row: // compute how many columns are available after the prefix, then split // at a UTF-8 character boundary so this row's chunk fits exactly. - let available_content_cols = width.saturating_sub(prefix_cols + 1).max(1); + let available_content_cols = layout.width.saturating_sub(prefix_cols + 1).max(1); let split_at_byte_index = remaining_text .char_indices() .nth(available_content_cols) @@ -503,17 +583,18 @@ fn push_wrapped_diff_line( let (chunk, rest) = remaining_text.split_at(split_at_byte_index); remaining_text = rest; - let (gutter_span, content_span, used_cols) = if first { + let (gutter_span, sign_span, content_spans, used_cols) = if first { let gutter = format!("{ln_str:>gutter_width$} "); - let content = format!("{sign_char}{chunk}"); - let cols = gutter.len() + content.len(); + let sign = sign_char.to_string(); + let content_spans = highlighted_diff_content_spans(chunk, path, line_style); + let cols = gutter.len() + sign.len() + chunk.len(); first = false; - (gutter, content, cols) + (gutter, Some(sign), content_spans, cols) } else { let gutter = format!("{:gutter_width$} ", ""); - let content = chunk.to_string(); - let cols = gutter.len() + content.len(); - (gutter, content, cols) + let content_spans = highlighted_diff_content_spans(chunk, path, line_style); + let cols = gutter.len() + chunk.len(); + (gutter, None, content_spans, cols) }; // When a background tint is active, apply it to every span (including @@ -522,20 +603,26 @@ fn push_wrapped_diff_line( let line = if let Some(bg_style) = line_bg_style { let gutter_style = style_gutter().patch(bg_style); let content_style = line_style.patch(bg_style); - let mut spans = vec![ - RtSpan::styled(gutter_span, gutter_style), - RtSpan::styled(content_span, content_style), - ]; - let pad = (width + outer_pad).saturating_sub(used_cols); + let mut spans = vec![RtSpan::styled(gutter_span, gutter_style)]; + if let Some(sign) = sign_span { + spans.push(RtSpan::styled(sign, content_style)); + } + spans.extend(content_spans.into_iter().map(|span| { + let style = span.style.patch(bg_style); + RtSpan::styled(span.content.into_owned(), style) + })); + let pad = (layout.width + layout.outer_pad).saturating_sub(used_cols); if pad > 0 { spans.push(RtSpan::styled(" ".repeat(pad), bg_style)); } RtLine::from(spans).style(bg_style) } else { - RtLine::from(vec![ - RtSpan::styled(gutter_span, style_gutter()), - RtSpan::styled(content_span, line_style), - ]) + let mut spans = vec![RtSpan::styled(gutter_span, style_gutter())]; + if let Some(sign) = sign_span { + spans.push(RtSpan::styled(sign, line_style)); + } + spans.extend(content_spans); + RtLine::from(spans) }; lines.push(line); @@ -547,6 +634,31 @@ fn push_wrapped_diff_line( lines } +fn highlighted_diff_content_spans( + chunk: &str, + path: &Path, + fallback_style: Style, +) -> Vec> { + if chunk.is_empty() { + return vec![RtSpan::styled(String::new(), fallback_style)]; + } + + let highlighted = highlight_code_to_lines_for_path(chunk, path); + let Some(line) = highlighted.into_iter().next() else { + return vec![RtSpan::styled(chunk.to_string(), fallback_style)]; + }; + + let has_syntax_style = line + .spans + .iter() + .any(|span| span.style.fg.is_some() && span.style.fg != Some(Color::default())); + if has_syntax_style { + line.spans + } else { + vec![RtSpan::styled(chunk.to_string(), fallback_style)] + } +} + fn line_number_width(max_line_number: usize) -> usize { if max_line_number == 0 { 1 @@ -633,9 +745,11 @@ mod tests { 1, DiffLineType::Insert, long_line, - 80, - line_number_width(1), - 0, + DiffLineLayout { + width: 80, + line_number_width: line_number_width(1), + outer_pad: 0, + }, &ctx, ); @@ -895,9 +1009,11 @@ mod tests { 1, DiffLineType::Insert, "hello", - (total_width as usize) - prefix_width * 2, - 1, - prefix_width, + DiffLineLayout { + width: (total_width as usize) - prefix_width * 2, + line_number_width: 1, + outer_pad: prefix_width, + }, &ctx, ); @@ -933,9 +1049,11 @@ mod tests { 1, DiffLineType::Insert, "hello", - content_width, - 1, - outer_pad, + DiffLineLayout { + width: content_width, + line_number_width: 1, + outer_pad, + }, &ctx, ); assert_eq!(lines.len(), 1); @@ -955,4 +1073,36 @@ mod tests { "expected gutter span to have background" ); } + + #[test] + fn diff_insert_content_uses_syntax_highlighting_when_path_is_known() { + #[allow(clippy::disallowed_methods)] + let ctx = DiffRenderStyleContext { + add_bg: Some(Color::Rgb(33, 58, 43)), + del_bg: Some(Color::Rgb(74, 34, 29)), + }; + + let lines = push_wrapped_diff_line_for_path( + 1, + DiffLineType::Insert, + "fn main() { println!(\"hello\"); }", + Path::new("src/main.rs"), + DiffLineLayout { + width: 80, + line_number_width: 1, + outer_pad: 0, + }, + &ctx, + ); + + assert!( + lines[0].spans.iter().skip(2).any(|span| { + span.style.fg.is_some() + && span.style.fg != Some(Color::Green) + && span.style.bg == ctx.add_bg + }), + "expected inserted Rust content to use syntax colors over the add background, got {:?}", + lines[0].spans + ); + } } diff --git a/nori-rs/tui/src/render/highlight.rs b/nori-rs/tui/src/render/highlight.rs index 7f4aef7eb..6d4e9bac2 100644 --- a/nori-rs/tui/src/render/highlight.rs +++ b/nori-rs/tui/src/render/highlight.rs @@ -3,6 +3,7 @@ use ratatui::style::Modifier; use ratatui::style::Style; use ratatui::text::Line; use ratatui::text::Span; +use std::path::Path; use syntect::highlighting::FontStyle; use syntect::highlighting::Theme; use syntect::highlighting::ThemeSet; @@ -121,6 +122,55 @@ pub(crate) fn highlight_code_to_lines(code: &str, lang: &str) -> Vec Vec> { + if code.is_empty() { + return vec![Line::from("")]; + } + + if is_too_large(code) { + return plain_lines(code); + } + + let ss = syntax_set(); + let syntax = match ss.find_syntax_for_file(path) { + Ok(Some(s)) => s, + Ok(None) | Err(_) => return plain_lines(code), + }; + + let theme = current_theme(); + let mut highlighter = syntect::easy::HighlightLines::new(syntax, theme); + let mut result: Vec> = Vec::new(); + + for line_str in syntect::util::LinesWithEndings::from(code) { + let regions = match highlighter.highlight_line(line_str, ss) { + Ok(r) => r, + Err(_) => return plain_lines(code), + }; + + let spans: Vec> = regions + .into_iter() + .map(|(style, text)| { + let ratatui_style = syntect_style_to_ratatui(style); + let text = text.strip_suffix('\n').unwrap_or(text); + if text.is_empty() { + return Span::from("".to_string()); + } + Span::styled(text.to_string(), ratatui_style) + }) + .filter(|s| !s.content.is_empty()) + .collect(); + + result.push(Line::from(spans)); + } + + if result.is_empty() { + vec![Line::from("")] + } else { + result + } +} + /// Highlight a bash script into styled ratatui `Line`s. /// /// This is a convenience wrapper around [`highlight_code_to_lines`]. @@ -185,6 +235,15 @@ mod tests { ); } + #[test] + fn highlight_code_to_lines_for_path_rust() { + let lines = highlight_code_to_lines_for_path("fn main() {}", Path::new("src/main.rs")); + assert!( + has_non_default_fg(&lines), + "expected colored output for Rust path, got: {lines:?}" + ); + } + #[test] fn highlight_large_input_returns_plain() { // Create input that exceeds 512KB From 3e3ca8ddbf60cff021d62ab84212da007507ccad Mon Sep 17 00:00:00 2001 From: Jonathan Lane Date: Fri, 1 May 2026 14:46:21 -0700 Subject: [PATCH 4/4] fix clippy complaints --- nori-rs/core/src/util.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/nori-rs/core/src/util.rs b/nori-rs/core/src/util.rs index 39d1b4368..b8314091c 100644 --- a/nori-rs/core/src/util.rs +++ b/nori-rs/core/src/util.rs @@ -13,11 +13,9 @@ pub fn create_patch_with_context( }; let line_offset = if let Ok(file_content) = std::fs::read_to_string(&full_path) { - if let Some(offset) = file_content.find(old_text) { - Some(file_content[..offset].lines().count() + 1) - } else { - None - } + file_content + .find(old_text) + .map(|offset| file_content[..offset].lines().count() + 1) } else { None }; @@ -32,7 +30,9 @@ pub fn create_patch_with_context( } fn adjust_patch_line_numbers(patch: &str, line_offset: usize) -> String { - let re = regex::Regex::new(r"^@@ -(\d+)(,?\d*) \+(\d+)(,?\d*) @@").unwrap(); + let Ok(re) = regex::Regex::new(r"^@@ -(\d+)(,?\d*) \+(\d+)(,?\d*) @@") else { + return patch.to_string(); + }; let mut result = String::new(); for line in patch.lines() { if let Some(caps) = re.captures(line) { @@ -45,8 +45,7 @@ fn adjust_patch_line_numbers(patch: &str, line_offset: usize) -> String { let adjusted_new_start = new_start + line_offset - 1; result.push_str(&format!( - "@@ -{}{}{} +{}{}{} @@\n", - adjusted_old_start, old_rest, "", adjusted_new_start, new_rest, "" + "@@ -{adjusted_old_start}{old_rest} +{adjusted_new_start}{new_rest} @@\n", )); } else { result.push_str(line);