From d3078b9adcfd3c7b44be3a68da5ce3bb216f18e1 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 15 Aug 2025 15:32:41 -0700 Subject: [PATCH] Show progress indicator for /diff command (#2245) ## Summary - Show a temporary Working on diff state in the bottom pan - Add `DiffResult` app event and dispatch git diff asynchronously ## Testing - `just fmt` - `just fix` *(fails: `let` expressions in this position are unstable)* - `cargo test --all-features` *(fails: `let` expressions in this position are unstable)* ------ https://chatgpt.com/codex/tasks/task_i_689a839f32b88321840a893551d5fbef --- codex-rs/tui/src/app.rs | 38 ++++++----- codex-rs/tui/src/app_event.rs | 3 + .../tui/src/bottom_pane/bottom_pane_view.rs | 4 ++ codex-rs/tui/src/bottom_pane/mod.rs | 13 ++++ .../src/bottom_pane/status_indicator_view.rs | 4 ++ codex-rs/tui/src/chatwidget.rs | 11 +++- codex-rs/tui/src/get_git_diff.rs | 63 ++++++++++--------- 7 files changed, 89 insertions(+), 47 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index ee054e03..3d12ad54 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -345,6 +345,11 @@ impl App<'_> { AppState::Chat { widget } => widget.submit_op(op), AppState::Onboarding { .. } => {} }, + AppEvent::DiffResult(text) => { + if let AppState::Chat { widget } = &mut self.app_state { + widget.add_diff_output(text); + } + } AppEvent::DispatchCommand(command) => match command { SlashCommand::New => { // User accepted – switch to chat view. @@ -382,25 +387,24 @@ impl App<'_> { break; } SlashCommand::Diff => { - let (is_git_repo, diff_text) = match get_git_diff() { - Ok(v) => v, - Err(e) => { - let msg = format!("Failed to compute diff: {e}"); - if let AppState::Chat { widget } = &mut self.app_state { - widget.add_diff_output(msg); - } - continue; - } - }; - if let AppState::Chat { widget } = &mut self.app_state { - let text = if is_git_repo { - diff_text - } else { - "`/diff` — _not inside a git repository_".to_string() - }; - widget.add_diff_output(text); + widget.add_diff_in_progress(); } + + let tx = self.app_event_tx.clone(); + tokio::spawn(async move { + let text = match get_git_diff().await { + Ok((is_git_repo, diff_text)) => { + if is_git_repo { + diff_text + } else { + "`/diff` — _not inside a git repository_".to_string() + } + } + Err(e) => format!("Failed to compute diff: {e}"), + }; + tx.send(AppEvent::DiffResult(text)); + }); } SlashCommand::Mention => { if let AppState::Chat { widget } = &mut self.app_state { diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 6b999984..1afffd75 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -51,6 +51,9 @@ pub(crate) enum AppEvent { matches: Vec, }, + /// Result of computing a `/diff` command. + DiffResult(String), + InsertHistory(Vec>), StartCommitAnimation, diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index c86dad31..31a32140 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -41,4 +41,8 @@ pub(crate) trait BottomPaneView<'a> { ) -> Option { Some(request) } + + /// Optional hook for views that expose a live status line. Views that do not + /// support this can ignore the call. + fn update_status_text(&mut self, _text: String) {} } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index b2da8d28..85ed59de 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -210,6 +210,19 @@ impl BottomPane<'_> { } } + /// Update the live status text shown while a task is running. + /// If a modal view is active (i.e., not the status indicator), this is a no‑op. + pub(crate) fn update_status_text(&mut self, text: String) { + if !self.is_task_running || !self.status_view_active { + return; + } + if let Some(mut view) = self.active_view.take() { + view.update_status_text(text); + self.active_view = Some(view); + self.request_redraw(); + } + } + pub(crate) fn composer_is_empty(&self) -> bool { self.composer.is_empty() } diff --git a/codex-rs/tui/src/bottom_pane/status_indicator_view.rs b/codex-rs/tui/src/bottom_pane/status_indicator_view.rs index b0f64a97..ccfd240b 100644 --- a/codex-rs/tui/src/bottom_pane/status_indicator_view.rs +++ b/codex-rs/tui/src/bottom_pane/status_indicator_view.rs @@ -43,4 +43,8 @@ impl BottomPaneView<'_> for StatusIndicatorView { self.view.interrupt(); } } + + fn update_status_text(&mut self, text: String) { + self.update_text(text); + } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 637d50bd..b7bd380b 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -659,8 +659,17 @@ impl ChatWidget<'_> { self.app_event_tx.send(AppEvent::RequestRedraw); } + pub(crate) fn add_diff_in_progress(&mut self) { + self.bottom_pane.set_task_running(true); + self.bottom_pane + .update_status_text("computing diff".to_string()); + self.request_redraw(); + } + pub(crate) fn add_diff_output(&mut self, diff_output: String) { - self.add_to_history(&history_cell::new_diff_output(diff_output.clone())); + self.bottom_pane.set_task_running(false); + self.add_to_history(&history_cell::new_diff_output(diff_output)); + self.mark_needs_redraw(); } pub(crate) fn add_status_output(&mut self) { diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index e6ba9f4d..78ab53d9 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -7,24 +7,26 @@ use std::io; use std::path::Path; -use std::process::Command; use std::process::Stdio; +use tokio::process::Command; /// Return value of [`get_git_diff`]. /// /// * `bool` – Whether the current working directory is inside a Git repo. /// * `String` – The concatenated diff (may be empty). -pub(crate) fn get_git_diff() -> io::Result<(bool, String)> { +pub(crate) async fn get_git_diff() -> io::Result<(bool, String)> { // First check if we are inside a Git repository. - if !inside_git_repo()? { + if !inside_git_repo().await? { return Ok((false, String::new())); } - // 1. Diff for tracked files. - let tracked_diff = run_git_capture_diff(&["diff", "--color"])?; - - // 2. Determine untracked files. - let untracked_output = run_git_capture_stdout(&["ls-files", "--others", "--exclude-standard"])?; + // Run tracked diff and untracked file listing in parallel. + let (tracked_diff_res, untracked_output_res) = tokio::join!( + run_git_capture_diff(&["diff", "--color"]), + run_git_capture_stdout(&["ls-files", "--others", "--exclude-standard"]), + ); + let tracked_diff = tracked_diff_res?; + let untracked_output = untracked_output_res?; let mut untracked_diff = String::new(); let null_device: &Path = if cfg!(windows) { @@ -33,26 +35,26 @@ pub(crate) fn get_git_diff() -> io::Result<(bool, String)> { Path::new("/dev/null") }; + let null_path = null_device.to_str().unwrap_or("/dev/null").to_string(); + let mut join_set: tokio::task::JoinSet> = tokio::task::JoinSet::new(); for file in untracked_output .split('\n') .map(str::trim) .filter(|s| !s.is_empty()) { - // Use `git diff --no-index` to generate a diff against the null device. - let args = [ - "diff", - "--color", - "--no-index", - "--", - null_device.to_str().unwrap_or("/dev/null"), - file, - ]; - - match run_git_capture_diff(&args) { - Ok(diff) => untracked_diff.push_str(&diff), - // If the file disappeared between ls-files and diff we ignore the error. - Err(err) if err.kind() == io::ErrorKind::NotFound => {} - Err(err) => return Err(err), + let null_path = null_path.clone(); + let file = file.to_string(); + join_set.spawn(async move { + let args = ["diff", "--color", "--no-index", "--", &null_path, &file]; + run_git_capture_diff(&args).await + }); + } + while let Some(res) = join_set.join_next().await { + match res { + Ok(Ok(diff)) => untracked_diff.push_str(&diff), + Ok(Err(err)) if err.kind() == io::ErrorKind::NotFound => {} + Ok(Err(err)) => return Err(err), + Err(_) => {} } } @@ -61,12 +63,13 @@ pub(crate) fn get_git_diff() -> io::Result<(bool, String)> { /// Helper that executes `git` with the given `args` and returns `stdout` as a /// UTF-8 string. Any non-zero exit status is considered an *error*. -fn run_git_capture_stdout(args: &[&str]) -> io::Result { +async fn run_git_capture_stdout(args: &[&str]) -> io::Result { let output = Command::new("git") .args(args) .stdout(Stdio::piped()) .stderr(Stdio::null()) - .output()?; + .output() + .await?; if output.status.success() { Ok(String::from_utf8_lossy(&output.stdout).into_owned()) @@ -80,12 +83,13 @@ fn run_git_capture_stdout(args: &[&str]) -> io::Result { /// Like [`run_git_capture_stdout`] but treats exit status 1 as success and /// returns stdout. Git returns 1 for diffs when differences are present. -fn run_git_capture_diff(args: &[&str]) -> io::Result { +async fn run_git_capture_diff(args: &[&str]) -> io::Result { let output = Command::new("git") .args(args) .stdout(Stdio::piped()) .stderr(Stdio::null()) - .output()?; + .output() + .await?; if output.status.success() || output.status.code() == Some(1) { Ok(String::from_utf8_lossy(&output.stdout).into_owned()) @@ -98,12 +102,13 @@ fn run_git_capture_diff(args: &[&str]) -> io::Result { } /// Determine if the current directory is inside a Git repository. -fn inside_git_repo() -> io::Result { +async fn inside_git_repo() -> io::Result { let status = Command::new("git") .args(["rev-parse", "--is-inside-work-tree"]) .stdout(Stdio::null()) .stderr(Stdio::null()) - .status(); + .status() + .await; match status { Ok(s) if s.success() => Ok(true),