From 71c3bd3a7939f63f765ed4364eeefed8a9823564 Mon Sep 17 00:00:00 2001 From: easong-openai Date: Mon, 4 Aug 2025 19:35:23 -0700 Subject: [PATCH] feedback --- codex-rs/core/src/conversation_history.rs | 69 +++++++++++++---------- codex-rs/core/src/models.rs | 17 +++--- codex-rs/tui/src/bottom_pane/mod.rs | 56 ++++++------------ codex-rs/tui/src/chatwidget.rs | 8 +-- codex-rs/tui/src/history_cell.rs | 2 - codex-rs/tui/src/lib.rs | 6 -- codex-rs/tui/src/live_wrap.rs | 48 +++++++++++----- codex-rs/tui/tests/vt100_history.rs | 21 +------ 8 files changed, 103 insertions(+), 124 deletions(-) diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index c1b13f388a..1d55b125bc 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -177,17 +177,16 @@ mod tests { h.record_items([&a1, &a2]); let items = h.contents(); - assert_eq!(items.len(), 1, "adjacent assistant messages should merge"); - if let ResponseItem::Message { role, content, .. } = &items[0] { - assert_eq!(role, "assistant"); - let text = match &content[0] { - ContentItem::OutputText { text } => text, - _ => panic!("expected OutputText"), - }; - assert_eq!(text, "Hello, world!"); - } else { - panic!("expected Message"); - } + assert_eq!( + items, + vec![ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: "Hello, world!".to_string() + }] + }] + ); } #[test] @@ -201,17 +200,16 @@ mod tests { h.record_items([&final_msg]); let items = h.contents(); - assert_eq!(items.len(), 1); - if let ResponseItem::Message { role, content, .. } = &items[0] { - assert_eq!(role, "assistant"); - let text = match &content[0] { - ContentItem::OutputText { text } => text, - _ => panic!("expected OutputText"), - }; - assert_eq!(text, "Hello, world!"); - } else { - panic!("expected Message"); - } + assert_eq!( + items, + vec![ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: "Hello, world!".to_string() + }] + }] + ); } #[test] @@ -233,13 +231,24 @@ mod tests { h.record_items([&u, &a]); let items = h.contents(); - assert_eq!(items.len(), 2); - match (&items[0], &items[1]) { - (ResponseItem::Message { role: r0, .. }, ResponseItem::Message { role: r1, .. }) => { - assert_eq!(r0, "user"); - assert_eq!(r1, "assistant"); - } - _ => panic!("expected two Message items"), - } + assert_eq!( + items, + vec![ + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::OutputText { + text: "hi".to_string() + }] + }, + ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: "hello".to_string() + }] + } + ] + ); } } diff --git a/codex-rs/core/src/models.rs b/codex-rs/core/src/models.rs index 166404915a..91bfb3bc8c 100644 --- a/codex-rs/core/src/models.rs +++ b/codex-rs/core/src/models.rs @@ -9,7 +9,7 @@ use serde::ser::Serializer; use crate::protocol::InputItem; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum ResponseInputItem { Message { @@ -26,7 +26,7 @@ pub enum ResponseInputItem { }, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum ContentItem { InputText { text: String }, @@ -34,7 +34,7 @@ pub enum ContentItem { OutputText { text: String }, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum ResponseItem { Message { @@ -107,7 +107,7 @@ impl From for ResponseItem { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(rename_all = "snake_case")] pub enum LocalShellStatus { Completed, @@ -115,13 +115,13 @@ pub enum LocalShellStatus { Incomplete, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum LocalShellAction { Exec(LocalShellExecAction), } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct LocalShellExecAction { pub command: Vec, pub timeout_ms: Option, @@ -130,7 +130,7 @@ pub struct LocalShellExecAction { pub user: Option, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum ReasoningItemReasoningSummary { SummaryText { text: String }, @@ -185,10 +185,9 @@ pub struct ShellToolCallParams { pub timeout_ms: Option, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub struct FunctionCallOutputPayload { pub content: String, - #[expect(dead_code)] pub success: Option, } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 6c29d76605..34161e02d8 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -9,6 +9,7 @@ use codex_file_search::FileMatch; use crossterm::event::KeyEvent; use ratatui::buffer::Buffer; use ratatui::layout::Rect; +use ratatui::text::Line; use ratatui::widgets::WidgetRef; mod approval_modal_view; @@ -30,6 +31,7 @@ pub(crate) enum CancellationEvent { pub(crate) use chat_composer::ChatComposer; pub(crate) use chat_composer::InputResult; +use crate::status_indicator_widget::StatusIndicatorWidget; use approval_modal_view::ApprovalModalView; use status_indicator_view::StatusIndicatorView; @@ -50,7 +52,7 @@ pub(crate) struct BottomPane<'a> { /// Optional live, multi‑line status/"live cell" rendered directly above /// the composer while a task is running. Unlike `active_view`, this does /// not replace the composer; it augments it. - live_status: Option, + live_status: Option, /// Optional transient ring shown above the composer. This is a rendering-only /// container used during development before we wire it to ChatWidget events. @@ -100,24 +102,22 @@ impl BottomPane<'_> { .map(|r| r.desired_height(width)) .unwrap_or(0); - if let Some(view) = self.active_view.as_ref() { + let view_height = if let Some(view) = self.active_view.as_ref() { // Add a single blank spacer line between live ring and status view when active. let spacer = if self.live_ring.is_some() && self.status_view_active { 1 } else { 0 }; - overlay_status_h - .saturating_add(ring_h) - .saturating_add(spacer) - .saturating_add(view.desired_height(width)) - .saturating_add(Self::BOTTOM_PAD_LINES) + spacer + view.desired_height(width) } else { - overlay_status_h - .saturating_add(ring_h) - .saturating_add(self.composer.desired_height(width)) - .saturating_add(Self::BOTTOM_PAD_LINES) - } + self.composer.desired_height(width) + }; + + overlay_status_h + .saturating_add(ring_h) + .saturating_add(view_height) + .saturating_add(Self::BOTTOM_PAD_LINES) } pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { @@ -206,10 +206,7 @@ impl BottomPane<'_> { // present an overlay above the composer. if !handled_by_view { if self.live_status.is_none() { - self.live_status = - Some(crate::status_indicator_widget::StatusIndicatorWidget::new( - self.app_event_tx.clone(), - )); + self.live_status = Some(StatusIndicatorWidget::new(self.app_event_tx.clone())); } if let Some(status) = &mut self.live_status { status.update_text(text); @@ -256,10 +253,8 @@ impl BottomPane<'_> { if let Some(mut view) = self.active_view.take() { if !view.should_hide_when_task_is_done() { self.active_view = Some(view); - self.status_view_active = false; - } else { - self.status_view_active = false; } + self.status_view_active = false; } } } @@ -337,11 +332,7 @@ impl BottomPane<'_> { } /// Set the rows and cap for the transient live ring overlay. - pub(crate) fn set_live_ring_rows( - &mut self, - max_rows: u16, - rows: Vec>, - ) { + pub(crate) fn set_live_ring_rows(&mut self, max_rows: u16, rows: Vec>) { let mut w = live_ring_widget::LiveRingWidget::new(); w.set_max_rows(max_rows); w.set_rows(rows); @@ -390,7 +381,7 @@ impl WidgetRef for &BottomPane<'_> { } } - if let Some(ov) = &self.active_view { + if let Some(view) = &self.active_view { if y_offset < area.height { // Reserve bottom padding lines; keep at least 1 line for the view. let avail = area.height - y_offset; @@ -401,7 +392,7 @@ impl WidgetRef for &BottomPane<'_> { width: area.width, height: avail - pad, }; - ov.render(view_rect, buf); + view.render(view_rect, buf); } } else if y_offset < area.height { let composer_rect = Rect { @@ -485,18 +476,7 @@ mod tests { } lines.push(s.trim_end().to_string()); } - assert!( - lines[0].contains("two"), - "top row should be 'two': {lines:?}" - ); - assert!( - lines[1].contains("three"), - "middle row should be 'three': {lines:?}" - ); - assert!( - lines[2].contains("four"), - "bottom row should be 'four': {lines:?}" - ); + assert_eq!(lines, vec!["two", "three", "four"]); } #[test] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index a26af28382..f63810b62a 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -250,11 +250,10 @@ impl ChatWidget<'_> { self.request_redraw(); } - EventMsg::AgentMessage(AgentMessageEvent { message }) => { + EventMsg::AgentMessage(AgentMessageEvent { message: _ }) => { // Final assistant answer: commit all remaining rows and close with // a blank line. Use the final text if provided, otherwise rely on // streamed deltas already in the builder. - let _ = message; // Already streamed via deltas in most providers. self.finalize_stream(StreamKind::Answer); self.request_redraw(); } @@ -272,9 +271,8 @@ impl ChatWidget<'_> { self.stream_push_and_maybe_commit(&delta); self.request_redraw(); } - EventMsg::AgentReasoning(AgentReasoningEvent { text }) => { + EventMsg::AgentReasoning(AgentReasoningEvent { text: _ }) => { // Final reasoning: commit remaining rows and close with a blank. - let _ = text; // Deltas carried the content; finalize below. self.finalize_stream(StreamKind::Reasoning); self.request_redraw(); } @@ -597,7 +595,7 @@ impl ChatWidget<'_> { lines.push(ratatui::text::Line::from(r.text)); } // Close the block with a blank line for readability. - lines.push(ratatui::text::Line::from(String::new())); + lines.push(ratatui::text::Line::from("")); self.app_event_tx.send(AppEvent::InsertHistory(lines)); } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index df3c3d44c9..17f0e683c0 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -223,8 +223,6 @@ impl HistoryCell { } } - // Removed unused new_agent_message and new_agent_reasoning constructors. - pub(crate) fn new_active_exec_command(command: Vec) -> Self { let command_escaped = strip_bash_lc_and_escape(&command); diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index f5f85c63b0..c619ce8ff0 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -25,19 +25,13 @@ mod bottom_pane; mod chatwidget; mod citation_regex; mod cli; -#[cfg(feature = "vt100-tests")] pub mod custom_terminal; -#[cfg(not(feature = "vt100-tests"))] -mod custom_terminal; mod exec_command; mod file_search; mod get_git_diff; mod git_warning_screen; mod history_cell; -#[cfg(feature = "vt100-tests")] pub mod insert_history; -#[cfg(not(feature = "vt100-tests"))] -mod insert_history; pub mod live_wrap; mod log_layer; mod markdown; diff --git a/codex-rs/tui/src/live_wrap.rs b/codex-rs/tui/src/live_wrap.rs index 9a06a587ce..97c36e8bf5 100644 --- a/codex-rs/tui/src/live_wrap.rs +++ b/codex-rs/tui/src/live_wrap.rs @@ -182,15 +182,15 @@ impl RowBuilder { } } -/// Take a prefix of `s` whose visible width is at most `max_cols`. +/// Take a prefix of `text` whose visible width is at most `max_cols`. /// Returns (prefix, suffix, prefix_width). -pub fn take_prefix_by_width(s: &str, max_cols: usize) -> (String, &str, usize) { - if max_cols == 0 || s.is_empty() { - return (String::new(), s, 0); +pub fn take_prefix_by_width(text: &str, max_cols: usize) -> (String, &str, usize) { + if max_cols == 0 || text.is_empty() { + return (String::new(), text, 0); } let mut cols = 0usize; let mut end_idx = 0usize; - for (i, ch) in s.char_indices() { + for (i, ch) in text.char_indices() { let ch_width = UnicodeWidthChar::width(ch).unwrap_or(0); if cols.saturating_add(ch_width) > max_cols { break; @@ -201,24 +201,34 @@ pub fn take_prefix_by_width(s: &str, max_cols: usize) -> (String, &str, usize) { break; } } - let prefix = s[..end_idx].to_string(); - let suffix = &s[end_idx..]; + let prefix = text[..end_idx].to_string(); + let suffix = &text[end_idx..]; (prefix, suffix, cols) } #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; #[test] fn rows_do_not_exceed_width_ascii() { let mut rb = RowBuilder::new(10); rb.push_fragment("hello world this is a test"); - let rows = rb.rows(); - assert!(!rows.is_empty()); - for r in rows { - assert!(r.width() <= 10, "row exceeds width: {r:?}"); - } + let rows = rb.rows().to_vec(); + assert_eq!( + rows, + vec![ + Row { + text: "hello worl".to_string(), + explicit_break: false + }, + Row { + text: "d this is ".to_string(), + explicit_break: false + } + ] + ); } #[test] @@ -226,9 +236,17 @@ mod tests { // πŸ˜€ is width 2; δ½ /ε₯½ are width 2. let mut rb = RowBuilder::new(6); rb.push_fragment("πŸ˜€πŸ˜€ δ½ ε₯½"); - for r in rb.rows() { - assert!(r.width() <= 6, "row exceeds width: {r:?}"); - } + let rows = rb.rows().to_vec(); + // At width 6, we expect the first row to fit exactly two emojis and a space + // (2 + 2 + 1 = 5) plus one more column for the first CJK char (2 would overflow), + // so only the two emojis and the space fit; the rest remains buffered. + assert_eq!( + rows, + vec![Row { + text: "πŸ˜€πŸ˜€ ".to_string(), + explicit_break: false + }] + ); } #[test] diff --git a/codex-rs/tui/tests/vt100_history.rs b/codex-rs/tui/tests/vt100_history.rs index 97fc32ed62..11ee044041 100644 --- a/codex-rs/tui/tests/vt100_history.rs +++ b/codex-rs/tui/tests/vt100_history.rs @@ -137,25 +137,8 @@ fn hist_003_emoji_and_cjk() { let text = String::from("πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€ δ½ ε₯½δΈ–η•Œ"); let lines = vec![Line::from(text.clone())]; let buf = scenario.run_insert(lines); - let mut parser = vt100::Parser::new(6, 20, 0); - parser.process(&buf); - let screen = parser.screen(); - - // Reconstruct string by concatenating non-space cells; ensure all emojis and CJK are present. - let mut reconstructed = String::new(); - for row in 0..6 { - for col in 0..20 { - if let Some(cell) = screen.cell(row, col) { - let cont = cell.contents(); - if let Some(ch) = cont.chars().next() { - if ch != ' ' { - reconstructed.push(ch); - } - } - } - } - } - + let rows = scenario.screen_rows_from_bytes(&buf); + let reconstructed: String = rows.join("").chars().filter(|c| *c != ' ').collect(); for ch in text.chars().filter(|c| !c.is_whitespace()) { assert!( reconstructed.contains(ch),