From b3edff66a9515773351d5ea8a941bc41805cdec6 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Sat, 4 Jul 2026 17:49:35 -0300 Subject: [PATCH] feat(tui): keep side conversations live --- codex-rs/tui/src/app/conversation_panes.rs | 5 + .../tui/src/app/conversation_panes_tests.rs | 6 + codex-rs/tui/src/app/event_dispatch.rs | 20 +- codex-rs/tui/src/app/session_lifecycle.rs | 1 + codex-rs/tui/src/app/side.rs | 395 ++++++++++++------ codex-rs/tui/src/app/tests.rs | 249 +++++++++-- codex-rs/tui/src/app/thread_routing.rs | 19 + .../tui/src/chatwidget/tests/constructor.rs | 9 + 8 files changed, 539 insertions(+), 165 deletions(-) diff --git a/codex-rs/tui/src/app/conversation_panes.rs b/codex-rs/tui/src/app/conversation_panes.rs index 2efa81d56f..742ac6fa30 100644 --- a/codex-rs/tui/src/app/conversation_panes.rs +++ b/codex-rs/tui/src/app/conversation_panes.rs @@ -8,6 +8,7 @@ use std::ops::Deref; use std::ops::DerefMut; use std::sync::Arc; use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use codex_protocol::ThreadId; use crossterm::event::KeyCode; @@ -169,6 +170,10 @@ impl ConversationPanes { if self.dispatch == Some(PaneSlot::Side) { self.dispatch = None; } + if let Some(side) = &self.side { + side.commit_anim_running + .store(/*val*/ false, Ordering::Release); + } self.side.take() } diff --git a/codex-rs/tui/src/app/conversation_panes_tests.rs b/codex-rs/tui/src/app/conversation_panes_tests.rs index b9e3a3b032..87b504b57f 100644 --- a/codex-rs/tui/src/app/conversation_panes_tests.rs +++ b/codex-rs/tui/src/app/conversation_panes_tests.rs @@ -75,11 +75,17 @@ async fn taking_focused_side_restores_parent_selection() { assert!(panes.install_side(side).is_ok()); assert!(panes.focus(PaneSlot::Side)); let side_origin = panes.conversation_origin().expect("side origin"); + panes + .by_slot(PaneSlot::Side) + .expect("side pane") + .commit_anim_running + .store(/*val*/ true, Ordering::Release); assert!(panes.dispatch_to(side_origin)); let removed = panes.take_side().expect("installed side pane"); assert_eq!(removed.origin(), Some(side_origin)); + assert!(!removed.commit_anim_running.load(Ordering::Acquire)); assert_eq!(panes.focused_slot(), PaneSlot::Parent); assert_eq!(panes.clear_dispatch(), None); assert!(panes.finish_dispatch(side_origin)); diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 6f6ea70921..57bec2b859 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -6,6 +6,7 @@ use super::conversation_events::normalize_conversation_event; use super::resize_reflow::trailing_run_start; use super::*; +use crate::app_event::ConversationOrigin; use crate::config_update::format_config_error; use crate::external_agent_config_migration_flow::ExternalAgentConfigMigrationFlowOutcome; #[cfg(target_os = "windows")] @@ -38,7 +39,9 @@ impl App { } else { None }; - let result = self.handle_event_inner(tui, app_server, event).await; + let result = self + .handle_event_inner(tui, app_server, conversation_origin, event) + .await; if let Some(previous_sender) = previous_sender { self.app_event_tx = previous_sender; } @@ -52,6 +55,7 @@ impl App { &mut self, tui: &mut tui::Tui, app_server: &mut AppServerSession, + conversation_origin: Option, event: AppEvent, ) -> Result { match event { @@ -356,6 +360,20 @@ impl App { self.chat_widget.on_commit_tick(); } AppEvent::Exit(mode) => { + if mode == ExitMode::ShutdownFirst + && conversation_origin.is_some_and(|origin| origin.pane == PaneSlot::Side) + && let Some(side_thread_id) = self + .chat_widget + .by_slot(PaneSlot::Side) + .and_then(|pane| pane.active_thread_id.or(pane.chat_widget.thread_id())) + { + if self.discard_side_thread(app_server, side_thread_id).await { + self.surface_pending_inactive_thread_interactive_requests() + .await?; + } + tui.frame_requester().schedule_frame(); + return Ok(AppRunControl::Continue); + } if mode == ExitMode::ShutdownFirst { self.show_shutdown_feedback(tui)?; } diff --git a/codex-rs/tui/src/app/session_lifecycle.rs b/codex-rs/tui/src/app/session_lifecycle.rs index 3f38aad8ac..e6c16470a4 100644 --- a/codex-rs/tui/src/app/session_lifecycle.rs +++ b/codex-rs/tui/src/app/session_lifecycle.rs @@ -482,6 +482,7 @@ impl App { } pub(super) fn reset_thread_event_state(&mut self) { + self.chat_widget.take_side(); self.abort_all_thread_event_listeners(); self.thread_event_channels.clear(); self.agent_navigation.clear(); diff --git a/codex-rs/tui/src/app/side.rs b/codex-rs/tui/src/app/side.rs index 9fbb8f8830..bc66fa6836 100644 --- a/codex-rs/tui/src/app/side.rs +++ b/codex-rs/tui/src/app/side.rs @@ -7,8 +7,12 @@ //! material only and steer the agent away from mutations unless the side conversation explicitly asks //! for them. +use super::conversation_panes::ConversationPaneInit; use super::*; +use crate::app_event::PaneSlot; +use crate::chatwidget::ChatWidget; use crate::chatwidget::InterruptedTurnNoticeMode; +use crate::file_search::FileSearchManager; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; @@ -218,55 +222,127 @@ impl SideThreadState { } } +fn clear_side_thread_ui(chat_widget: &mut ChatWidget) { + chat_widget.set_side_conversation_context_label(/*label*/ None); + chat_widget.set_side_conversation_active(/*active*/ false); + chat_widget.clear_thread_rename_block(); + chat_widget.set_interrupted_turn_notice_mode(InterruptedTurnNoticeMode::Default); +} + +fn configure_side_thread_ui( + chat_widget: &mut ChatWidget, + parent_is_main: bool, + parent_label: Option<&str>, + parent_status: Option, +) { + chat_widget.set_thread_rename_block_message(SIDE_RENAME_BLOCK_MESSAGE); + chat_widget.set_side_conversation_active(/*active*/ true); + chat_widget.set_interrupted_turn_notice_mode(InterruptedTurnNoticeMode::Suppress); + let mut label_parts = if parent_is_main { + vec!["from main thread".to_string()] + } else { + vec![format!( + "from parent thread ({})", + parent_label.unwrap_or("unknown") + )] + }; + if let Some(parent_status) = parent_status { + label_parts.push(parent_status.label(parent_is_main).to_string()); + } + label_parts.push("Ctrl+C to return".to_string()); + chat_widget + .set_side_conversation_context_label(Some(format!("Side {}", label_parts.join(" · ")))); +} + impl App { + fn installed_side_thread_id(&self) -> Option { + let pane = self.chat_widget.by_slot(PaneSlot::Side)?; + pane.active_thread_id.or(pane.thread_id()) + } + + fn focused_side_thread_id(&self) -> Option { + if self.chat_widget.focused_slot() != PaneSlot::Side { + return None; + } + self.installed_side_thread_id() + } + pub(super) fn sync_side_thread_ui(&mut self) { - let clear_side_ui = |chat_widget: &mut crate::chatwidget::ChatWidget| { - chat_widget.set_side_conversation_context_label(/*label*/ None); - chat_widget.set_side_conversation_active(/*active*/ false); - chat_widget.clear_thread_rename_block(); - chat_widget.set_interrupted_turn_notice_mode(InterruptedTurnNoticeMode::Default); - }; - let Some(active_thread_id) = self.current_displayed_thread_id() else { - clear_side_ui(&mut self.chat_widget); - return; - }; - let Some((parent_thread_id, parent_status)) = self - .side_threads - .get(&active_thread_id) - .map(|state| (state.parent_thread_id, state.parent_status)) + if let Some(parent) = self.chat_widget.by_slot_mut(PaneSlot::Parent) { + clear_side_thread_ui(&mut parent.chat_widget); + } + + let side_state = self + .installed_side_thread_id() + .and_then(|thread_id| self.side_threads.get(&thread_id)) + .cloned(); + let Some((parent_thread_id, parent_status)) = + side_state.map(|state| (state.parent_thread_id, state.parent_status)) else { - clear_side_ui(&mut self.chat_widget); + if let Some(side) = self.chat_widget.by_slot_mut(PaneSlot::Side) { + clear_side_thread_ui(&mut side.chat_widget); + } return; }; - self.chat_widget - .set_thread_rename_block_message(SIDE_RENAME_BLOCK_MESSAGE); - self.chat_widget - .set_side_conversation_active(/*active*/ true); - self.chat_widget - .set_interrupted_turn_notice_mode(InterruptedTurnNoticeMode::Suppress); - let mut label_parts = Vec::new(); let parent_is_main = self.primary_thread_id == Some(parent_thread_id); - if parent_is_main { - label_parts.push("from main thread".to_string()); - } else { - let parent_label = self.thread_label(parent_thread_id); - label_parts.push(format!("from parent thread ({parent_label})")); - } - if let Some(parent_status) = parent_status { - label_parts.push(parent_status.label(parent_is_main).to_string()); - } - label_parts.push("Ctrl+C to return".to_string()); - self.chat_widget - .set_side_conversation_context_label(Some(format!("Side {}", label_parts.join(" · ")))); + let parent_label = (!parent_is_main).then(|| self.thread_label(parent_thread_id)); + let Some(side) = self.chat_widget.by_slot_mut(PaneSlot::Side) else { + return; + }; + configure_side_thread_ui( + &mut side.chat_widget, + parent_is_main, + parent_label.as_deref(), + parent_status, + ); } pub(super) fn active_side_parent_thread_id(&self) -> Option { - self.current_displayed_thread_id() + self.installed_side_thread_id() .and_then(|thread_id| self.side_threads.get(&thread_id)) .map(|state| state.parent_thread_id) } + fn add_side_thread_error(&mut self, thread_id: ThreadId, message: String) { + if self.installed_side_thread_id() == Some(thread_id) + && let Some(side) = self.chat_widget.by_slot_mut(PaneSlot::Side) + { + side.add_error_message(message); + } else { + self.chat_widget.add_error_message(message); + } + } + + fn replay_side_thread_snapshot(&mut self, snapshot: ThreadEventSnapshot) -> bool { + let Some(side_origin) = self + .chat_widget + .by_slot(PaneSlot::Side) + .and_then(|pane| pane.origin()) + else { + return false; + }; + let previous_origin = self + .app_event_tx + .conversation_origin() + .filter(|origin| self.chat_widget.by_origin(*origin).is_some()); + if !self.chat_widget.dispatch_to(side_origin) { + return false; + } + let sender = self.chat_widget.conversation_event_sender(); + let previous_sender = std::mem::replace(&mut self.app_event_tx, sender); + self.replay_thread_snapshot(snapshot, /*resume_restored_queue*/ false); + self.app_event_tx = previous_sender; + if let Some(previous_origin) = previous_origin { + let restored = self.chat_widget.dispatch_to(previous_origin); + debug_assert!(restored); + } else { + let cleared = self.chat_widget.clear_dispatch(); + debug_assert_eq!(cleared, Some(PaneSlot::Side)); + } + true + } + pub(super) fn set_side_parent_status( &mut self, parent_thread_id: ThreadId, @@ -331,30 +407,38 @@ impl App { tui: &mut tui::Tui, app_server: &mut AppServerSession, ) -> bool { - if self.overlay.is_none() - && self.chat_widget.no_modal_or_popup_active() - && self.chat_widget.composer_is_empty() - && let Some(parent_thread_id) = self.active_side_parent_thread_id() - { - if self - .select_agent_thread_and_discard_side(tui, app_server, parent_thread_id) - .await - .is_err() - { - return false; - } - self.active_side_parent_thread_id().is_none() - } else { - false + let Some(side_thread_id) = self.focused_side_thread_id() else { + return false; + }; + let can_return = self.overlay.is_none() + && self + .chat_widget + .by_slot(PaneSlot::Side) + .is_some_and(|side| side.no_modal_or_popup_active() && side.composer_is_empty()); + if !can_return { + return false; } + + if self.discard_side_thread(app_server, side_thread_id).await { + if let Err(err) = self + .surface_pending_inactive_thread_interactive_requests() + .await + { + tracing::warn!(%err, "failed to surface pending requests after closing side pane"); + } + } else { + self.keep_side_thread_visible_after_cleanup_failure(tui, side_thread_id); + } + tui.frame_requester().schedule_frame(); + true } pub(super) fn side_thread_to_discard_after_switch( &self, target_thread_id: ThreadId, ) -> Option { - let side_thread_id = self.current_displayed_thread_id()?; - if target_thread_id == side_thread_id || !self.side_threads.contains_key(&side_thread_id) { + let side_thread_id = self.installed_side_thread_id()?; + if target_thread_id == side_thread_id { return None; } @@ -368,14 +452,14 @@ impl App { ) -> bool { if let Err(message) = self.interrupt_side_thread(app_server, thread_id).await { tracing::warn!("{message}"); - self.chat_widget.add_error_message(message); + self.add_side_thread_error(thread_id, message); return false; } if let Err(err) = app_server.thread_unsubscribe(thread_id).await { let message = format!("Failed to close side conversation {thread_id}; it is still open: {err}"); tracing::warn!("{message}"); - self.chat_widget.add_error_message(message); + self.add_side_thread_error(thread_id, message); return false; } self.discard_thread_local_state(thread_id).await; @@ -387,11 +471,15 @@ impl App { } pub(super) async fn discard_thread_local_state(&mut self, thread_id: ThreadId) { + let remove_side_pane = self.installed_side_thread_id() == Some(thread_id); self.abort_thread_event_listener(thread_id); self.thread_event_channels.remove(&thread_id); self.side_threads.remove(&thread_id); self.agent_navigation.remove(thread_id); - if self.chat_widget.active_thread_id == Some(thread_id) { + if remove_side_pane { + self.chat_widget.take_side(); + self.refresh_pending_thread_approvals().await; + } else if self.chat_widget.active_thread_id == Some(thread_id) { self.clear_active_thread().await; } else { self.refresh_pending_thread_approvals().await; @@ -415,18 +503,14 @@ impl App { }) } - async fn keep_side_thread_visible_after_cleanup_failure( + fn keep_side_thread_visible_after_cleanup_failure( &mut self, tui: &mut tui::Tui, - app_server: &mut AppServerSession, thread_id: ThreadId, ) { - if self.chat_widget.active_thread_id != Some(thread_id) - && let Err(err) = self.select_agent_thread(tui, app_server, thread_id).await - { - tracing::warn!( - "failed to restore side conversation after cleanup failure for {thread_id}: {err}" - ); + if self.installed_side_thread_id() == Some(thread_id) { + self.chat_widget.focus(PaneSlot::Side); + tui.frame_requester().schedule_frame(); } } @@ -439,12 +523,25 @@ impl App { if self.discard_side_thread(app_server, thread_id).await { true } else { - self.keep_side_thread_visible_after_cleanup_failure(tui, app_server, thread_id) - .await; + self.keep_side_thread_visible_after_cleanup_failure(tui, thread_id); false } } + async fn fail_side_start( + &mut self, + tui: &mut tui::Tui, + app_server: &mut AppServerSession, + thread_id: ThreadId, + user_message: Option, + message: String, + ) { + self.discard_side_thread_or_keep_visible(tui, app_server, thread_id) + .await; + self.restore_side_user_message(user_message); + self.add_side_thread_error(thread_id, message); + } + fn side_developer_instructions(existing_instructions: Option<&str>) -> String { match existing_instructions { Some(existing_instructions) if !existing_instructions.trim().is_empty() => { @@ -484,7 +581,7 @@ impl App { pub(super) fn side_start_block_message(&self) -> Option<&'static str> { if self.primary_thread_id.is_none() { Some(SIDE_MAIN_THREAD_UNAVAILABLE_MESSAGE) - } else if !self.side_threads.is_empty() { + } else if self.chat_widget.has_side() || !self.side_threads.is_empty() { Some(SIDE_ALREADY_OPEN_MESSAGE) } else { None @@ -508,8 +605,10 @@ impl App { user_message: Option, ) { if let Some(user_message) = user_message { - self.chat_widget - .restore_user_message_to_composer(user_message); + let slot = self.chat_widget.focused_slot(); + if let Some(pane) = self.chat_widget.by_slot_mut(slot) { + pane.restore_user_message_to_composer(user_message); + } } } @@ -530,24 +629,22 @@ impl App { app_server: &mut AppServerSession, thread_id: ThreadId, ) -> Result<()> { - let active_thread_id_before_switch = self.chat_widget.active_thread_id; - let side_thread_to_discard = self.side_thread_to_discard_after_switch(thread_id); - self.select_agent_thread(tui, app_server, thread_id).await?; - if self.chat_widget.active_thread_id == Some(thread_id) - && let Some(side_thread_id) = side_thread_to_discard - { - if self.discard_side_thread(app_server, side_thread_id).await { - self.surface_pending_inactive_thread_interactive_requests() - .await?; - } else if active_thread_id_before_switch == Some(side_thread_id) { - self.keep_side_thread_visible_after_cleanup_failure( - tui, - app_server, - side_thread_id, - ) - .await; + if self.installed_side_thread_id() == Some(thread_id) { + self.chat_widget.focus(PaneSlot::Side); + tui.frame_requester().schedule_frame(); + return Ok(()); + } + + if let Some(side_thread_id) = self.side_thread_to_discard_after_switch(thread_id) { + if !self.discard_side_thread(app_server, side_thread_id).await { + self.keep_side_thread_visible_after_cleanup_failure(tui, side_thread_id); + return Ok(()); } } + self.chat_widget.focus(PaneSlot::Parent); + self.select_agent_thread(tui, app_server, thread_id).await?; + self.surface_pending_inactive_thread_interactive_requests() + .await?; Ok(()) } @@ -574,7 +671,10 @@ impl App { .await; let fork_config = self.side_fork_config(); - match app_server.fork_thread(fork_config, parent_thread_id).await { + match app_server + .fork_thread(fork_config.clone(), parent_thread_id) + .await + { Ok(forked) => { let child_thread_id = forked.session.thread_id; let channel = self.ensure_thread_channel(child_thread_id); @@ -584,55 +684,98 @@ impl App { } self.side_threads .insert(child_thread_id, SideThreadState::new(parent_thread_id)); + + let init = self.chatwidget_init_for_forked_or_resumed_thread( + tui, + fork_config, + /*initial_user_message*/ None, + ); + let side_widget = ChatWidget::new_with_app_event_for_pane(init, PaneSlot::Side); + let file_search = FileSearchManager::new( + side_widget.config_ref().cwd.to_path_buf(), + side_widget.conversation_event_sender(), + ); + let owned_screen = if self.has_owned_screen() { + Self::owned_screen_for_behavior( + crate::AltScreenBehavior::Owned, + &side_widget, + self.keymap.pager.clone(), + ) + } else { + None + }; + if self + .chat_widget + .install_side(ConversationPaneInit { + chat_widget: side_widget, + file_search, + owned_screen, + }) + .is_err() + { + self.fail_side_start( + tui, + app_server, + child_thread_id, + user_message.take(), + format!("Failed to install side conversation {child_thread_id}."), + ) + .await; + return Ok(AppRunControl::Continue); + } + + let Some((receiver, snapshot)) = + self.activate_thread_for_replay(child_thread_id).await + else { + self.fail_side_start( + tui, + app_server, + child_thread_id, + user_message.take(), + format!("Failed to attach side conversation {child_thread_id}."), + ) + .await; + return Ok(AppRunControl::Continue); + }; + let Some(side) = self.chat_widget.by_slot_mut(PaneSlot::Side) else { + unreachable!("side pane was installed above"); + }; + side.attach_thread(child_thread_id, Some(receiver)); + if !self.replay_side_thread_snapshot(snapshot) { + self.fail_side_start( + tui, + app_server, + child_thread_id, + user_message.take(), + format!("Failed to initialize side conversation {child_thread_id}."), + ) + .await; + return Ok(AppRunControl::Continue); + } + self.chat_widget.focus(PaneSlot::Side); + self.sync_side_thread_ui(); + if let Err(err) = app_server .thread_inject_items(child_thread_id, vec![Self::side_boundary_prompt_item()]) .await { - self.discard_side_thread_or_keep_visible(tui, app_server, child_thread_id) - .await; - self.restore_side_user_message(user_message.take()); - self.chat_widget.add_error_message(format!( - "Failed to prepare side conversation {child_thread_id}: {err}" - )); + self.fail_side_start( + tui, + app_server, + child_thread_id, + user_message.take(), + format!("Failed to prepare side conversation {child_thread_id}: {err}"), + ) + .await; return Ok(AppRunControl::Continue); } - if let Err(err) = self - .select_agent_thread_and_discard_side(tui, app_server, child_thread_id) - .await + + if let Some(user_message) = user_message.take() + && let Some(side) = self.chat_widget.by_slot_mut(PaneSlot::Side) { - let discarded = self - .discard_side_thread_or_keep_visible(tui, app_server, child_thread_id) - .await; - if discarded - && self.chat_widget.active_thread_id != Some(parent_thread_id) - && let Err(restore_err) = self - .select_agent_thread(tui, app_server, parent_thread_id) - .await - { - tracing::warn!( - "failed to restore parent thread after side conversation switch failure: {restore_err}" - ); - } - self.restore_side_user_message(user_message.take()); - self.chat_widget.add_error_message(format!( - "Failed to switch into side conversation {child_thread_id}: {err}" - )); - return Ok(AppRunControl::Continue); - } - if self.chat_widget.active_thread_id == Some(child_thread_id) { - if let Some(user_message) = user_message.take() { - let _ = self - .chat_widget - .submit_user_message_as_plain_user_turn(user_message); - } - } else { - self.discard_side_thread_or_keep_visible(tui, app_server, child_thread_id) - .await; - self.restore_side_user_message(user_message.take()); - self.chat_widget.add_error_message(format!( - "Failed to switch into side conversation {child_thread_id}." - )); + let _ = side.submit_user_message_as_plain_user_turn(user_message); } + tui.frame_requester().schedule_frame(); } Err(err) => { self.restore_side_user_message(user_message.take()); diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 93a47f3a80..49598f8f59 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -12,6 +12,7 @@ use crate::app_backtrack::user_count; use crate::chatwidget::ChatWidgetInit; use crate::chatwidget::create_initial_user_message; +use crate::chatwidget::tests::constructor::make_chatwidget_for_pane_with_sender; use crate::chatwidget::tests::helpers::render_bottom_popup; use crate::chatwidget::tests::make_chatwidget_manual_with_sender; use crate::chatwidget::tests::set_chatgpt_auth; @@ -128,6 +129,49 @@ fn conversation_event_payload(event: AppEvent) -> AppEvent { } } +async fn install_test_side_pane( + app: &mut App, + parent_thread_id: ThreadId, + side_thread_id: ThreadId, +) { + app.chat_widget + .by_slot_mut(PaneSlot::Parent) + .expect("parent pane") + .attach_thread(parent_thread_id, /*receiver*/ None); + let side_widget = + make_chatwidget_for_pane_with_sender(PaneSlot::Side, app.app_event_tx.clone()).await; + let file_search = FileSearchManager::new( + side_widget.config_ref().cwd.to_path_buf(), + side_widget.conversation_event_sender(), + ); + let installed = app.chat_widget.install_side(ConversationPaneInit { + chat_widget: side_widget, + file_search, + owned_screen: None, + }); + assert!(installed.is_ok(), "side pane should install"); + app.chat_widget + .by_slot_mut(PaneSlot::Side) + .expect("side pane") + .attach_thread(side_thread_id, /*receiver*/ None); + app.side_threads + .insert(side_thread_id, SideThreadState::new(parent_thread_id)); + assert!(app.chat_widget.focus(PaneSlot::Side)); + app.sync_side_thread_ui(); +} + +async fn attach_test_pane_channel(app: &mut App, slot: PaneSlot, thread_id: ThreadId) { + app.set_thread_active(thread_id, /*active*/ true).await; + let receiver = app + .thread_event_channels + .get_mut(&thread_id) + .and_then(|channel| channel.receiver.take()); + app.chat_widget + .by_slot_mut(slot) + .expect("installed pane") + .attach_thread(thread_id, receiver); +} + async fn next_thread_settings_updated( app_server: &mut AppServerSession, thread_id: ThreadId, @@ -2294,7 +2338,7 @@ async fn inactive_thread_approval_bubbles_into_active_view() -> Result<()> { } #[tokio::test] -async fn side_defers_parent_approval_overlay_until_parent_replay() -> Result<()> { +async fn side_keeps_parent_approval_live_without_stealing_focus() -> Result<()> { let mut app = make_test_app().await; let parent_thread_id = ThreadId::from_string("00000000-0000-0000-0000-000000000011").expect("valid thread"); @@ -2302,9 +2346,6 @@ async fn side_defers_parent_approval_overlay_until_parent_replay() -> Result<()> ThreadId::from_string("00000000-0000-0000-0000-000000000022").expect("valid thread"); app.primary_thread_id = Some(parent_thread_id); - app.chat_widget.active_thread_id = Some(side_thread_id); - app.side_threads - .insert(side_thread_id, SideThreadState::new(parent_thread_id)); app.thread_event_channels.insert( parent_thread_id, ThreadEventChannel::new_with_session( @@ -2313,6 +2354,8 @@ async fn side_defers_parent_approval_overlay_until_parent_replay() -> Result<()> Vec::new(), ), ); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; + attach_test_pane_channel(&mut app, PaneSlot::Parent, parent_thread_id).await; app.enqueue_thread_request( parent_thread_id, @@ -2333,20 +2376,20 @@ async fn side_defers_parent_approval_overlay_until_parent_replay() -> Result<()> .and_then(|state| state.parent_status), Some(SideParentStatus::NeedsApproval) ); + let (slot, event) = app.chat_widget.recv_thread_event().await; + assert_eq!(slot, PaneSlot::Parent); + assert!(app.chat_widget.focus(PaneSlot::Parent)); + app.handle_thread_event_replay(event.expect("parent approval event")); + assert!(app.chat_widget.focus(PaneSlot::Side)); - let snapshot = { - let channel = app - .thread_event_channels - .get(&parent_thread_id) - .expect("parent thread channel"); - let store = channel.store.lock().await; - store.snapshot() - }; - app.side_threads.remove(&side_thread_id); - app.chat_widget.active_thread_id = Some(parent_thread_id); - app.replay_thread_snapshot(snapshot, /*resume_restored_queue*/ false); - - assert_eq!(app.chat_widget.has_active_view(), true); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Side); + assert!(!app.chat_widget.has_active_view()); + assert!( + app.chat_widget + .by_slot(PaneSlot::Parent) + .expect("parent pane") + .has_active_view() + ); Ok(()) } @@ -2409,9 +2452,7 @@ async fn side_defers_subagent_approval_overlay_until_side_exits() -> Result<()> ThreadId::from_string("00000000-0000-0000-0000-000000000033").expect("valid thread"); app.primary_thread_id = Some(main_thread_id); - app.chat_widget.active_thread_id = Some(side_thread_id); - app.side_threads - .insert(side_thread_id, SideThreadState::new(main_thread_id)); + install_test_side_pane(&mut app, main_thread_id, side_thread_id).await; app.thread_event_channels.insert( agent_thread_id, ThreadEventChannel::new_with_session( @@ -2449,8 +2490,7 @@ async fn side_defers_subagent_approval_overlay_until_side_exits() -> Result<()> &["Robie [explorer]".to_string()] ); - app.side_threads.remove(&side_thread_id); - app.chat_widget.active_thread_id = Some(main_thread_id); + app.discard_closed_side_thread(side_thread_id).await; app.surface_pending_inactive_thread_interactive_requests() .await?; @@ -3623,10 +3663,9 @@ async fn active_side_thread_renders_live_mcp_startup_notifications() { let parent_thread_id = ThreadId::new(); let side_thread_id = ThreadId::new(); app.primary_thread_id = Some(parent_thread_id); - app.side_threads - .insert(side_thread_id, SideThreadState::new(parent_thread_id)); app.ensure_thread_channel(side_thread_id); - app.activate_thread_channel(side_thread_id).await; + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; + attach_test_pane_channel(&mut app, PaneSlot::Side, side_thread_id).await; app.replay_thread_snapshot( ThreadEventSnapshot { session: Some(test_thread_session( @@ -3676,7 +3715,7 @@ async fn active_side_thread_renders_live_mcp_startup_notifications() { let mut rendered_cells = Vec::new(); while let Ok(event) = app_event_rx.try_recv() { - if let AppEvent::InsertHistoryCell(cell) = event { + if let AppEvent::InsertHistoryCell(cell) = conversation_event_payload(event) { rendered_cells.push(lines_to_single_string(&cell.display_lines(/*width*/ 120))); } } @@ -3704,14 +3743,111 @@ async fn side_restore_user_message_puts_inline_question_back_in_composer() { ); } +#[tokio::test] +async fn start_side_installs_live_child_without_replacing_parent() -> Result<()> { + Box::pin(async { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + let started = app_server + .start_thread(app.chat_widget.config_ref()) + .await?; + let parent_thread_id = started.session.thread_id; + app_server + .thread_inject_items(parent_thread_id, vec![App::side_boundary_prompt_item()]) + .await?; + app.enqueue_primary_thread_session(started.session, started.turns) + .await?; + app.chat_widget + .set_composer_text("parent draft".to_string(), Vec::new(), Vec::new()); + while app_event_rx.try_recv().is_ok() {} + let mut tui = crate::tui::test_support::make_test_tui()?; + + let control = app + .handle_start_side( + &mut tui, + &mut app_server, + parent_thread_id, + Some(crate::chatwidget::UserMessage::from("side question")), + ) + .await?; + + assert!(matches!(control, AppRunControl::Continue)); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Side); + let parent = app + .chat_widget + .by_slot(PaneSlot::Parent) + .expect("parent pane"); + assert_eq!(parent.active_thread_id, Some(parent_thread_id)); + assert!(parent.active_thread_rx.is_some()); + assert_eq!(parent.composer_text_with_pending(), "parent draft"); + let side = app.chat_widget.by_slot(PaneSlot::Side).expect("side pane"); + let side_thread_id = side.active_thread_id.expect("side thread id"); + assert_ne!(side_thread_id, parent_thread_id); + assert!(side.active_thread_rx.is_some()); + assert!(side.side_conversation_active()); + let side_origin = side.conversation_origin().expect("side origin"); + assert_eq!(side_origin.pane, PaneSlot::Side); + assert_eq!( + app.side_threads + .get(&side_thread_id) + .map(|state| state.parent_thread_id), + Some(parent_thread_id) + ); + for thread_id in [parent_thread_id, side_thread_id] { + let store = &app + .thread_event_channels + .get(&thread_id) + .expect("live thread channel") + .store; + assert!(store.lock().await.active); + } + + let mut submitted_to_side = false; + while let Ok(event) = app_event_rx.try_recv() { + if let AppEvent::ConversationOp { + target, + op: Op::UserTurn { items, .. }, + } = event + { + assert_eq!(target.thread_id, side_thread_id); + assert_eq!( + items, + vec![UserInput::Text { + text: "side question".to_string(), + text_elements: Vec::new(), + }] + ); + submitted_to_side = true; + } + } + assert!(submitted_to_side); + + let control = app + .handle_event( + &mut tui, + &mut app_server, + AppEvent::FromConversation { + target: side_origin, + event: Box::new(AppEvent::Exit(ExitMode::ShutdownFirst)), + }, + ) + .await?; + assert!(matches!(control, AppRunControl::Continue)); + assert!(!app.chat_widget.has_side()); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Parent); + assert_eq!(app.chat_widget.active_thread_id, Some(parent_thread_id)); + Ok(()) + }) + .await +} + #[tokio::test] async fn side_discard_selection_keeps_current_side_thread() { let mut app = make_test_app().await; let parent_thread_id = ThreadId::new(); let side_thread_id = ThreadId::new(); - app.chat_widget.active_thread_id = Some(side_thread_id); - app.side_threads - .insert(side_thread_id, SideThreadState::new(parent_thread_id)); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; assert_eq!( app.side_thread_to_discard_after_switch(side_thread_id), @@ -3733,8 +3869,8 @@ async fn discard_side_thread_removes_agent_navigation_entry() -> Result<()> { side_config.ephemeral = true; let started = app_server.start_thread(&side_config).await?; let side_thread_id = started.session.thread_id; - app.side_threads - .insert(side_thread_id, SideThreadState::new(ThreadId::new())); + let parent_thread_id = ThreadId::new(); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; app.agent_navigation.upsert( side_thread_id, Some("Side".to_string()), @@ -3749,6 +3885,8 @@ async fn discard_side_thread_removes_agent_navigation_entry() -> Result<()> { assert_eq!(app.agent_navigation.get(&side_thread_id), None); assert!(!app.side_threads.contains_key(&side_thread_id)); + assert!(!app.chat_widget.has_side()); + assert_eq!(app.chat_widget.active_thread_id, Some(parent_thread_id)); Ok(()) }) .await @@ -3762,9 +3900,7 @@ async fn discard_side_thread_keeps_local_state_when_server_close_fails() -> Resu crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; let parent_thread_id = ThreadId::new(); let side_thread_id = ThreadId::new(); - app.chat_widget.active_thread_id = Some(side_thread_id); - app.side_threads - .insert(side_thread_id, SideThreadState::new(parent_thread_id)); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; app.agent_navigation.upsert( side_thread_id, Some("Side".to_string()), @@ -3777,7 +3913,15 @@ async fn discard_side_thread_keeps_local_state_when_server_close_fails() -> Resu .await ); + assert!(app.chat_widget.has_side()); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Side); assert_eq!(app.chat_widget.active_thread_id, Some(side_thread_id)); + assert_eq!( + app.chat_widget + .by_slot(PaneSlot::Parent) + .and_then(|pane| pane.active_thread_id), + Some(parent_thread_id) + ); assert_eq!( app.side_threads .get(&side_thread_id) @@ -3795,9 +3939,7 @@ async fn discard_closed_side_thread_removes_local_state_without_server_rpc() { let mut app = make_test_app().await; let parent_thread_id = ThreadId::new(); let side_thread_id = ThreadId::new(); - app.chat_widget.active_thread_id = Some(side_thread_id); - app.side_threads - .insert(side_thread_id, SideThreadState::new(parent_thread_id)); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; app.thread_event_channels .insert(side_thread_id, ThreadEventChannel::new(/*capacity*/ 4)); app.agent_navigation.upsert( @@ -3809,12 +3951,43 @@ async fn discard_closed_side_thread_removes_local_state_without_server_rpc() { app.discard_closed_side_thread(side_thread_id).await; - assert_eq!(app.chat_widget.active_thread_id, None); + assert!(!app.chat_widget.has_side()); + assert_eq!(app.chat_widget.active_thread_id, Some(parent_thread_id)); assert!(!app.side_threads.contains_key(&side_thread_id)); assert!(!app.thread_event_channels.contains_key(&side_thread_id)); assert_eq!(app.agent_navigation.get(&side_thread_id), None); } +#[tokio::test] +async fn side_thread_closed_removes_only_side_and_keeps_parent_live() -> Result<()> { + let mut app = make_test_app().await; + let parent_thread_id = ThreadId::new(); + let side_thread_id = ThreadId::new(); + app.primary_thread_id = Some(parent_thread_id); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; + app.chat_widget + .by_slot_mut(PaneSlot::Parent) + .expect("parent pane") + .set_composer_text("parent draft".to_string(), Vec::new(), Vec::new()); + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + + app.handle_active_thread_event( + &mut tui, + &mut app_server, + ThreadBufferedEvent::Notification(thread_closed_notification(side_thread_id)), + ) + .await?; + + assert!(!app.chat_widget.has_side()); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Parent); + assert_eq!(app.chat_widget.active_thread_id, Some(parent_thread_id)); + assert_eq!(app.chat_widget.composer_text_with_pending(), "parent draft"); + assert!(!app.side_threads.contains_key(&side_thread_id)); + Ok(()) +} + #[tokio::test] async fn active_non_primary_shutdown_target_returns_none_for_non_shutdown_event() -> Result<()> { let mut app = make_test_app().await; diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 5ebf7ac6ca..3233590458 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -1574,6 +1574,25 @@ impl App { ) && self.pending_shutdown_exit_thread_id == self.chat_widget.active_thread_id; + if matches!( + &event, + ThreadBufferedEvent::Notification(ServerNotification::ThreadClosed(_)) + ) && let Some(side_thread_id) = self.chat_widget.active_thread_id + && self.side_threads.contains_key(&side_thread_id) + { + if pending_shutdown_exit_completed { + self.pending_shutdown_exit_thread_id = None; + } + self.mark_agent_picker_thread_closed(side_thread_id); + self.discard_closed_side_thread(side_thread_id).await; + self.chat_widget.add_info_message( + format!("Side conversation {side_thread_id} closed. Returned to parent."), + /*hint*/ None, + ); + tui.frame_requester().schedule_frame(); + return Ok(()); + } + // Processing order matters: // // 1. handle unexpected non-primary shutdown failover first; diff --git a/codex-rs/tui/src/chatwidget/tests/constructor.rs b/codex-rs/tui/src/chatwidget/tests/constructor.rs index 54d8f22087..c5c1ff93f6 100644 --- a/codex-rs/tui/src/chatwidget/tests/constructor.rs +++ b/codex-rs/tui/src/chatwidget/tests/constructor.rs @@ -43,6 +43,15 @@ pub(crate) async fn make_chatwidget_for_pane( (ChatWidget::new_with_app_event_for_pane(init, pane), rx) } +pub(crate) async fn make_chatwidget_for_pane_with_sender( + pane: PaneSlot, + app_event_tx: AppEventSender, +) -> ChatWidget { + let (mut init, _rx) = test_init().await; + init.app_event_tx = app_event_tx; + ChatWidget::new_with_app_event_for_pane(init, pane) +} + #[tokio::test] async fn app_event_constructor_defaults_to_parent_pane() { let (init, _rx) = test_init().await;