diff --git a/codex-rs/core/src/git_info.rs b/codex-rs/core/src/git_info.rs index cb7e304618..65e796e693 100644 --- a/codex-rs/core/src/git_info.rs +++ b/codex-rs/core/src/git_info.rs @@ -492,24 +492,23 @@ pub fn resolve_root_git_project_for_trust(cwd: &Path) -> Option { /// Returns a list of local git branches. /// Includes the default branch at the beginning of the list, if it exists. -pub async fn local_git_branches(cwd: &PathBuf) -> Vec { - let out = std::process::Command::new("git") - .args(["branch", "--format=%(refname:short)"]) - .current_dir(cwd) - .output(); - - let mut branches: Vec = match out { - Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout) +pub async fn local_git_branches(cwd: &Path) -> Vec { + let mut branches: Vec = if let Some(out) = + run_git_command_with_timeout(&["branch", "--format=%(refname:short)"], cwd).await + && out.status.success() + { + String::from_utf8_lossy(&out.stdout) .lines() .map(|s| s.trim().to_string()) .filter(|s| !s.is_empty()) - .collect(), - _ => Vec::new(), + .collect() + } else { + Vec::new() }; branches.sort_unstable(); - if let Some(base) = get_default_branch_local(cwd.as_path()).await + if let Some(base) = get_default_branch_local(cwd).await && let Some(pos) = branches.iter().position(|name| name == &base) { let base_branch = branches.remove(pos); @@ -520,17 +519,11 @@ pub async fn local_git_branches(cwd: &PathBuf) -> Vec { } /// Returns the current checked out branch name. -pub fn current_branch_name(cwd: &PathBuf) -> Option { - let out = std::process::Command::new("git") - .args(["branch", "--show-current"]) - .current_dir(cwd) - .output() - .ok()?; - +pub async fn current_branch_name(cwd: &Path) -> Option { + let out = run_git_command_with_timeout(&["branch", "--show-current"], cwd).await?; if !out.status.success() { return None; } - String::from_utf8(out.stdout) .ok() .map(|s| s.trim().to_string()) diff --git a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs index 6c40c95986..73867daeba 100644 --- a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs +++ b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs @@ -12,7 +12,7 @@ use ratatui::widgets::StatefulWidgetRef; use ratatui::widgets::Widget; use std::cell::RefCell; -use crate::chatwidget::STANDARD_POPUP_HINT_LINE; +use super::popup_consts::STANDARD_POPUP_HINT_LINE; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; @@ -184,7 +184,7 @@ impl BottomPaneView for CustomPromptView { let hint_y = hint_blank_y.saturating_add(1); if hint_y < area.y.saturating_add(area.height) { - Paragraph::new(STANDARD_POPUP_HINT_LINE.to_string()).render( + Paragraph::new(STANDARD_POPUP_HINT_LINE).render( Rect { x: area.x, y: hint_y, diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 160de768e0..4fd6f8d27a 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -28,10 +28,21 @@ pub(crate) struct SelectionItem { pub description: Option, pub is_current: bool, pub actions: Vec, - pub dimiss_on_select: bool, + pub dismiss_on_select: bool, pub search_value: Option, } +#[derive(Default)] +pub(crate) struct SelectionViewParams { + pub title: String, + pub subtitle: Option, + pub footer_hint: Option, + pub items: Vec, + pub is_searchable: bool, + pub search_placeholder: Option, + pub empty_message: Option, +} + pub(crate) struct ListSelectionView { title: String, subtitle: Option, @@ -57,32 +68,23 @@ impl ListSelectionView { para.render(area, buf); } - pub fn new( - title: String, - subtitle: Option, - footer_hint: Option, - items: Vec, - is_searchable: bool, - search_placeholder: Option, - empty_message: Option, - app_event_tx: AppEventSender, - ) -> Self { + pub fn new(params: SelectionViewParams, app_event_tx: AppEventSender) -> Self { let mut s = Self { - title, - subtitle, - footer_hint, - items, + title: params.title, + subtitle: params.subtitle, + footer_hint: params.footer_hint, + items: params.items, state: ScrollState::new(), complete: false, app_event_tx, - is_searchable, + is_searchable: params.is_searchable, search_query: String::new(), - search_placeholder: if is_searchable { - search_placeholder + search_placeholder: if params.is_searchable { + params.search_placeholder } else { None }, - empty_message, + empty_message: params.empty_message, filtered_indices: Vec::new(), }; s.apply_filter(); @@ -119,10 +121,8 @@ impl ListSelectionView { search_value.to_lowercase().contains(&query_lower) } else { let mut matches = item.name.to_lowercase().contains(&query_lower); - if !matches { - if let Some(desc) = &item.description { - matches = desc.to_lowercase().contains(&query_lower); - } + if !matches && let Some(desc) = &item.description { + matches = desc.to_lowercase().contains(&query_lower); } matches }; @@ -156,6 +156,33 @@ impl ListSelectionView { self.state.ensure_visible(len, visible); } + fn build_rows(&self) -> Vec { + self.filtered_indices + .iter() + .enumerate() + .filter_map(|(visible_idx, actual_idx)| { + self.items.get(*actual_idx).map(|item| { + let is_selected = self.state.selected_idx == Some(visible_idx); + let prefix = if is_selected { '>' } else { ' ' }; + let name = item.name.as_str(); + let name_with_marker = if item.is_current { + format!("{name} (current)") + } else { + item.name.clone() + }; + let n = visible_idx + 1; + let display_name = format!("{prefix} {n}. {name_with_marker}"); + GenericDisplayRow { + name: display_name, + match_indices: None, + is_current: item.is_current, + description: item.description.clone(), + } + }) + }) + .collect() + } + fn move_up(&mut self) { let len = self.visible_len(); self.state.move_up_wrap(len); @@ -178,7 +205,7 @@ impl ListSelectionView { for act in &item.actions { act(&self.app_event_tx); } - if item.dimiss_on_select { + if item.dismiss_on_select { self.complete = true; } } else { @@ -242,30 +269,7 @@ impl BottomPaneView for ListSelectionView { fn desired_height(&self, width: u16) -> u16 { // Measure wrapped height for up to MAX_POPUP_ROWS items at the given width. // Build the same display rows used by the renderer so wrapping math matches. - let rows: Vec = self - .filtered_indices - .iter() - .enumerate() - .filter_map(|(visible_idx, actual_idx)| { - self.items.get(*actual_idx).map(|item| { - let is_selected = self.state.selected_idx == Some(visible_idx); - let prefix = if is_selected { '>' } else { ' ' }; - let name_with_marker = if item.is_current { - format!("{} (current)", item.name) - } else { - item.name.clone() - }; - let display_name = - format!("{} {}. {}", prefix, visible_idx + 1, name_with_marker); - GenericDisplayRow { - name: display_name, - match_indices: None, - is_current: item.is_current, - description: item.description.clone(), - } - }) - }) - .collect(); + let rows = self.build_rows(); let rows_height = measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, width); @@ -356,30 +360,7 @@ impl BottomPaneView for ListSelectionView { .saturating_sub(footer_reserved), }; - let rows: Vec = self - .filtered_indices - .iter() - .enumerate() - .filter_map(|(visible_idx, actual_idx)| { - self.items.get(*actual_idx).map(|item| { - let is_selected = self.state.selected_idx == Some(visible_idx); - let prefix = if is_selected { '>' } else { ' ' }; - let name_with_marker = if item.is_current { - format!("{} (current)", item.name) - } else { - item.name.clone() - }; - let display_name = - format!("{} {}. {}", prefix, visible_idx + 1, name_with_marker); - GenericDisplayRow { - name: display_name, - match_indices: None, - is_current: item.is_current, - description: item.description.clone(), - } - }) - }) - .collect(); + let rows = self.build_rows(); if rows_area.height > 0 { render_rows( rows_area, @@ -410,7 +391,7 @@ mod tests { use super::BottomPaneView; use super::*; use crate::app_event::AppEvent; - use crate::chatwidget::STANDARD_POPUP_HINT_LINE; + use crate::bottom_pane::popup_consts::STANDARD_POPUP_HINT_LINE; use insta::assert_snapshot; use ratatui::layout::Rect; use tokio::sync::mpsc::unbounded_channel; @@ -424,7 +405,7 @@ mod tests { description: Some("Codex can read files".to_string()), is_current: true, actions: vec![], - dimiss_on_select: true, + dismiss_on_select: true, search_value: None, }, SelectionItem { @@ -432,18 +413,18 @@ mod tests { description: Some("Codex can edit files".to_string()), is_current: false, actions: vec![], - dimiss_on_select: true, + dismiss_on_select: true, search_value: None, }, ]; ListSelectionView::new( - "Select Approval Mode".to_string(), - subtitle.map(str::to_string), - Some(STANDARD_POPUP_HINT_LINE.to_string()), - items, - false, - None, - None, + SelectionViewParams { + title: "Select Approval Mode".to_string(), + subtitle: subtitle.map(str::to_string), + footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), + items, + ..Default::default() + }, tx, ) } @@ -496,17 +477,19 @@ mod tests { description: Some("Codex can read files".to_string()), is_current: false, actions: vec![], - dimiss_on_select: true, + dismiss_on_select: true, search_value: None, }]; let mut view = ListSelectionView::new( - "Select Approval Mode".to_string(), - None, - Some(STANDARD_POPUP_HINT_LINE.to_string()), - items, - true, - Some("Type to search branches".to_string()), - Some("no matches".to_string()), + SelectionViewParams { + title: "Select Approval Mode".to_string(), + footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), + items, + is_searchable: true, + search_placeholder: Some("Type to search branches".to_string()), + empty_message: Some("no matches".to_string()), + ..Default::default() + }, tx, ); view.set_search_query("filters".to_string()); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 870755fc5e..a77e25ff3d 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -24,8 +24,9 @@ mod command_popup; pub mod custom_prompt_view; mod file_search_popup; mod list_selection_view; +pub(crate) use list_selection_view::SelectionViewParams; mod paste_burst; -mod popup_consts; +pub mod popup_consts; mod scroll_state; mod selection_popup_common; mod textarea; @@ -184,7 +185,7 @@ impl BottomPane { self.view_stack.clear(); self.on_active_view_complete(); } else { - let idx = reinsertion_index.min(self.view_stack.len()); + let idx = reinsertion_index; self.view_stack.insert(idx, view); } self.request_redraw(); @@ -350,26 +351,8 @@ impl BottomPane { } /// Show a generic list selection view with the provided items. - pub(crate) fn show_selection_view( - &mut self, - title: String, - subtitle: Option, - footer_hint: Option, - items: Vec, - is_searchable: bool, - search_placeholder: Option, - empty_message: Option, - ) { - let view = list_selection_view::ListSelectionView::new( - title, - subtitle, - footer_hint, - items, - is_searchable, - search_placeholder, - empty_message, - self.app_event_tx.clone(), - ); + pub(crate) fn show_selection_view(&mut self, params: list_selection_view::SelectionViewParams) { + let view = list_selection_view::ListSelectionView::new(params, self.app_event_tx.clone()); self.push_view(Box::new(view)); } diff --git a/codex-rs/tui/src/bottom_pane/popup_consts.rs b/codex-rs/tui/src/bottom_pane/popup_consts.rs index 5f447d735c..5147b2ee5f 100644 --- a/codex-rs/tui/src/bottom_pane/popup_consts.rs +++ b/codex-rs/tui/src/bottom_pane/popup_consts.rs @@ -3,3 +3,6 @@ /// Maximum number of rows any popup should attempt to display. /// Keep this consistent across all popups for a uniform feel. pub(crate) const MAX_POPUP_ROWS: usize = 8; + +/// Standard footer hint text used by popups. +pub(crate) const STANDARD_POPUP_HINT_LINE: &str = "Press Enter to confirm or Esc to go back"; diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 01b036bad5..5ee684f700 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -64,7 +64,9 @@ use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::InputResult; use crate::bottom_pane::SelectionAction; use crate::bottom_pane::SelectionItem; +use crate::bottom_pane::SelectionViewParams; use crate::bottom_pane::custom_prompt_view::CustomPromptView; +use crate::bottom_pane::popup_consts::STANDARD_POPUP_HINT_LINE; use crate::clipboard_paste::paste_image_to_temp_png; use crate::diff_render::display_path_for; use crate::get_git_diff::get_git_diff; @@ -99,8 +101,7 @@ use codex_core::protocol::AskForApproval; use codex_core::protocol::SandboxPolicy; use codex_core::protocol_config_types::ReasoningEffort as ReasoningEffortConfig; use codex_file_search::FileMatch; - -pub(crate) const STANDARD_POPUP_HINT_LINE: &str = "Press Enter to confirm or Esc to go back"; +use std::path::Path; // Track information about an in-flight exec command. struct RunningCommand { @@ -1343,20 +1344,20 @@ impl ChatWidget { description, is_current, actions, - dimiss_on_select: true, + dismiss_on_select: true, search_value: None, }); } - self.bottom_pane.show_selection_view( - "Select model and reasoning level".to_string(), - Some("Switch between OpenAI models for this and future Codex CLI session".to_string()), - Some(STANDARD_POPUP_HINT_LINE.to_string()), + self.bottom_pane.show_selection_view(SelectionViewParams { + title: "Select model and reasoning level".to_string(), + subtitle: Some( + "Switch between OpenAI models for this and future Codex CLI session".to_string(), + ), + footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, - false, - None, - None, - ); + ..Default::default() + }); } /// Open a popup to choose the approvals mode (ask for approval policy + sandbox policy). @@ -1389,20 +1390,17 @@ impl ChatWidget { description, is_current, actions, - dimiss_on_select: true, + dismiss_on_select: true, search_value: None, }); } - self.bottom_pane.show_selection_view( - "Select Approval Mode".to_string(), - None, - Some(STANDARD_POPUP_HINT_LINE.to_string()), + self.bottom_pane.show_selection_view(SelectionViewParams { + title: "Select Approval Mode".to_string(), + footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, - false, - None, - None, - ); + ..Default::default() + }); } /// Set the approval policy in the widget's config copy. @@ -1528,7 +1526,7 @@ impl ChatWidget { })); }, )], - dimiss_on_select: true, + dismiss_on_select: true, search_value: None, }); @@ -1542,7 +1540,7 @@ impl ChatWidget { tx.send(AppEvent::OpenReviewBranchPicker(cwd.clone())); } })], - dimiss_on_select: false, + dismiss_on_select: false, search_value: None, }); @@ -1553,25 +1551,23 @@ impl ChatWidget { actions: vec![Box::new(move |tx| { tx.send(AppEvent::OpenReviewCustomPrompt); })], - dimiss_on_select: false, + dismiss_on_select: false, search_value: None, }); - self.bottom_pane.show_selection_view( - "Select a review preset".into(), - None, - Some(STANDARD_POPUP_HINT_LINE.to_string()), + self.bottom_pane.show_selection_view(SelectionViewParams { + title: "Select a review preset".into(), + footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, - false, - None, - None, - ); + ..Default::default() + }); } - pub(crate) async fn show_review_branch_picker(&mut self, cwd: &PathBuf) { + pub(crate) async fn show_review_branch_picker(&mut self, cwd: &Path) { let branches = local_git_branches(cwd).await; - let current_branch = - current_branch_name(cwd).unwrap_or_else(|| "(detached HEAD)".to_string()); + let current_branch = current_branch_name(cwd) + .await + .unwrap_or_else(|| "(detached HEAD)".to_string()); let mut items: Vec = Vec::with_capacity(branches.len()); for option in branches { @@ -1590,20 +1586,20 @@ impl ChatWidget { }, })); })], - dimiss_on_select: true, + dismiss_on_select: true, search_value: Some(option), }); } - self.bottom_pane.show_selection_view( - "Select a base branch".to_string(), - None, - Some(STANDARD_POPUP_HINT_LINE.to_string()), + self.bottom_pane.show_selection_view(SelectionViewParams { + title: "Select a base branch".to_string(), + footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, - true, - Some("Type to search branches".to_string()), - Some("no matches".to_string()), - ); + is_searchable: true, + search_placeholder: Some("Type to search branches".to_string()), + empty_message: Some("no matches".to_string()), + ..Default::default() + }); } pub(crate) fn show_review_custom_prompt(&mut self) {