From 7e5588699da9a89b63f0db988b9209ece31d7fba Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 20 Apr 2026 14:31:37 +0100 Subject: [PATCH] chore: drop review prompt from TUI UX (#18659) Due to the app-server rebase of the TUI, the review prompt was leaked into the transcript on the TUI This is not a security issue but it was bad UX. This PR fixes this --- codex-rs/tui/src/chatwidget.rs | 126 +++++++----------- codex-rs/tui/src/chatwidget/realtime.rs | 1 - .../tui/src/chatwidget/tests/review_mode.rs | 74 ++++++++++ 3 files changed, 123 insertions(+), 78 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c5fc25c54..8fb926ba8 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -134,6 +134,7 @@ use codex_protocol::config_types::Settings; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::items::AgentMessageContent; use codex_protocol::items::AgentMessageItem; +use codex_protocol::items::UserMessageItem; use codex_protocol::models::MessagePhase; use codex_protocol::models::local_image_label_text; use codex_protocol::parse_command::ParsedCommand; @@ -167,7 +168,6 @@ use codex_protocol::protocol::DeprecationNoticeEvent; use codex_protocol::protocol::ErrorEvent; #[cfg(test)] use codex_protocol::protocol::Event; -#[cfg(test)] use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::ExecCommandBeginEvent; @@ -6005,54 +6005,14 @@ impl ChatWidget { let replay_kind = render_source.replay_kind(); match item { ThreadItem::UserMessage { id, content } => { - let user_message = codex_protocol::items::UserMessageItem { + let user_message = UserMessageItem { id, content: content .into_iter() .map(codex_app_server_protocol::UserInput::into_core) .collect(), }; - let codex_protocol::protocol::EventMsg::UserMessage(event) = - user_message.as_legacy_event() - else { - unreachable!("user message item should convert to a user message event"); - }; - if from_replay { - self.on_user_message_event(event); - } else { - let rendered = Self::rendered_user_message_event_from_event(&event); - let compare_key = - Self::pending_steer_compare_key_from_items(&user_message.content); - if self - .pending_steers - .front() - .is_some_and(|pending| pending.compare_key == compare_key) - { - if let Some(pending) = self.pending_steers.pop_front() { - self.refresh_pending_input_preview(); - let pending_event = UserMessageEvent { - message: pending.user_message.text, - images: Some(pending.user_message.remote_image_urls), - local_images: pending - .user_message - .local_images - .into_iter() - .map(|image| image.path) - .collect(), - text_elements: pending.user_message.text_elements, - }; - self.on_user_message_event(pending_event); - } else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) - { - tracing::warn!( - "pending steer matched compare key but queue was empty when rendering committed user message" - ); - self.on_user_message_event(event); - } - } else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) { - self.on_user_message_event(event); - } - } + self.on_committed_user_message(&user_message, from_replay); } ThreadItem::AgentMessage { id, @@ -7168,40 +7128,7 @@ impl ChatWidget { EventMsg::ItemCompleted(event) => { let item = event.item; if !from_replay && let codex_protocol::items::TurnItem::UserMessage(item) = &item { - let EventMsg::UserMessage(event) = item.as_legacy_event() else { - unreachable!("user message item should convert to a legacy user message"); - }; - let rendered = Self::rendered_user_message_event_from_event(&event); - let compare_key = Self::pending_steer_compare_key_from_item(item); - if self - .pending_steers - .front() - .is_some_and(|pending| pending.compare_key == compare_key) - { - if let Some(pending) = self.pending_steers.pop_front() { - self.refresh_pending_input_preview(); - let pending_event = UserMessageEvent { - message: pending.user_message.text, - images: Some(pending.user_message.remote_image_urls), - local_images: pending - .user_message - .local_images - .into_iter() - .map(|image| image.path) - .collect(), - text_elements: pending.user_message.text_elements, - }; - self.on_user_message_event(pending_event); - } else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) - { - tracing::warn!( - "pending steer matched compare key but queue was empty when rendering committed user message" - ); - self.on_user_message_event(event); - } - } else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) { - self.on_user_message_event(event); - } + self.on_committed_user_message(item, from_replay); } if let codex_protocol::items::TurnItem::Plan(plan_item) = &item { self.on_plan_item_completed(plan_item.text.clone()); @@ -7286,6 +7213,51 @@ impl ChatWidget { self.exit_review_mode_after_item(); } + fn on_committed_user_message(&mut self, item: &UserMessageItem, from_replay: bool) { + let EventMsg::UserMessage(event) = item.as_legacy_event() else { + unreachable!("user message item should convert to a legacy user message"); + }; + if from_replay { + if !self.is_review_mode { + self.on_user_message_event(event); + } + return; + } + + let rendered = Self::rendered_user_message_event_from_event(&event); + let compare_key = Self::pending_steer_compare_key_from_item(item); + if self + .pending_steers + .front() + .is_some_and(|pending| pending.compare_key == compare_key) + { + if let Some(pending) = self.pending_steers.pop_front() { + self.refresh_pending_input_preview(); + let pending_event = UserMessageEvent { + message: pending.user_message.text, + images: Some(pending.user_message.remote_image_urls), + local_images: pending + .user_message + .local_images + .into_iter() + .map(|image| image.path) + .collect(), + text_elements: pending.user_message.text_elements, + }; + self.on_user_message_event(pending_event); + } else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) { + tracing::warn!( + "pending steer matched compare key but queue was empty when rendering committed user message" + ); + self.on_user_message_event(event); + } + } else if !self.is_review_mode + && self.last_rendered_user_message_event.as_ref() != Some(&rendered) + { + self.on_user_message_event(event); + } + } + fn on_user_message_event(&mut self, event: UserMessageEvent) { self.last_rendered_user_message_event = Some(Self::rendered_user_message_event_from_event(&event)); diff --git a/codex-rs/tui/src/chatwidget/realtime.rs b/codex-rs/tui/src/chatwidget/realtime.rs index 6357361f8..f09e3e9b3 100644 --- a/codex-rs/tui/src/chatwidget/realtime.rs +++ b/codex-rs/tui/src/chatwidget/realtime.rs @@ -132,7 +132,6 @@ impl ChatWidget { } } - #[cfg(test)] pub(super) fn pending_steer_compare_key_from_item( item: &codex_protocol::items::UserMessageItem, ) -> PendingSteerCompareKey { diff --git a/codex-rs/tui/src/chatwidget/tests/review_mode.rs b/codex-rs/tui/src/chatwidget/tests/review_mode.rs index 91b942324..2de9c7eb9 100644 --- a/codex-rs/tui/src/chatwidget/tests/review_mode.rs +++ b/codex-rs/tui/src/chatwidget/tests/review_mode.rs @@ -146,6 +146,80 @@ async fn entered_review_mode_defaults_to_current_changes_banner() { assert!(chat.is_review_mode); } +#[tokio::test] +async fn live_core_review_prompt_item_is_not_rendered() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(Event { + id: "review-start".into(), + msg: EventMsg::EnteredReviewMode(ReviewRequest { + target: ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + user_facing_hint: Some("changes against 'main'".to_string()), + }), + }); + let cells = drain_insert_history(&mut rx); + assert_eq!(cells.len(), 1); + assert!(lines_to_single_string(&cells[0]).contains("Code review started")); + + complete_user_message( + &mut chat, + "review-prompt", + "Review the code changes against the base branch 'main'.", + ); + + assert!(drain_insert_history(&mut rx).is_empty()); +} + +#[tokio::test] +async fn live_app_server_review_prompt_item_is_not_rendered() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + + let review_mode_item = AppServerThreadItem::EnteredReviewMode { + id: "review-start".to_string(), + review: "changes against 'main'".to_string(), + }; + chat.handle_server_notification( + ServerNotification::ItemStarted(ItemStartedNotification { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item: review_mode_item.clone(), + }), + /*replay_kind*/ None, + ); + let cells = drain_insert_history(&mut rx); + assert_eq!(cells.len(), 1); + assert!(lines_to_single_string(&cells[0]).contains("Code review started")); + + chat.handle_server_notification( + ServerNotification::ItemCompleted(ItemCompletedNotification { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item: review_mode_item, + }), + /*replay_kind*/ None, + ); + assert!(drain_insert_history(&mut rx).is_empty()); + + chat.handle_server_notification( + ServerNotification::ItemCompleted(ItemCompletedNotification { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item: AppServerThreadItem::UserMessage { + id: "review-prompt".to_string(), + content: vec![AppServerUserInput::Text { + text: "Review the code changes against the base branch 'main'.".to_string(), + text_elements: Vec::new(), + }], + }, + }), + /*replay_kind*/ None, + ); + + assert!(drain_insert_history(&mut rx).is_empty()); +} + #[tokio::test] async fn steer_rejection_queues_review_follow_up_before_existing_queued_messages() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;