From ee2149a9b42ff487910514d1236349efcd00a641 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 21 Sep 2026 00:06:55 +0000 Subject: [PATCH] Preserve transcript position when opening settings pickers (#46910) ## Why Opening a settings picker while reading older messages should preserve the reading position. Settings errors and confirmations still need to be visible. ## What changed - Keep the transcript anchored when opening `/model`, `/theme`, `/keymap`, `/memories`, `/title`, and `/statusline` without arguments. - Resume transcript following for settings actions that report confirmations or errors, including save failures and unavailable commands. - Keep inline commands and commands with arguments following their output. ## Testing Add regression tests for picker open/close behavior, settings save failures, confirmation output, and `/model` before startup completes. Include snapshots of the preserved reading position and a visible theme save error. GitOrigin-RevId: 0680e6414d43b8b5d2a3ab9a2df187abfb727d49 --- .../tui/src/app/backend_banner_fallback.rs | 2 + codex-rs/tui/src/app/config_persistence.rs | 4 + codex-rs/tui/src/app/event_dispatch.rs | 15 ++ codex-rs/tui/src/app/experimental_features.rs | 22 +- .../app/tests/local_command_scroll_tests.rs | 249 +++++++++++++++++- codex-rs/tui/src/chatwidget/input_flow.rs | 31 ++- 6 files changed, 304 insertions(+), 19 deletions(-) diff --git a/codex-rs/tui/src/app/backend_banner_fallback.rs b/codex-rs/tui/src/app/backend_banner_fallback.rs index 96f355cdc8..eef5bb0349 100644 --- a/codex-rs/tui/src/app/backend_banner_fallback.rs +++ b/codex-rs/tui/src/app/backend_banner_fallback.rs @@ -1,6 +1,7 @@ //! Apply backend-authorized fallback and task-local Reserve settings without saving defaults. use super::App; +use crate::app_event::AppEvent; use crate::app_server_session::AppServerSession; use crate::chatwidget::AutomaticModelSwitchReason; use crate::model_catalog::LUNA_RESERVE_MODEL; @@ -25,6 +26,7 @@ impl App { { return; } + self.app_event_tx.send(AppEvent::FollowTranscript); let mut mode = self.chat_widget.effective_collaboration_mode(); mode.settings.reasoning_effort = effort.clone(); let params = ThreadSettingsUpdateParams { diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index 89d0e4fb38..b6cb2f1160 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -851,6 +851,7 @@ impl App { Ok(response) => response, Err(err) => { tracing::error!(error = %err, "failed to persist memory settings"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget .add_error_message(format!("Failed to save memory settings: {err}")); return false; @@ -862,6 +863,7 @@ impl App { message, "memory settings config write was overridden by effective config" ); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_error_message(format!( "Memory setting changes were saved but not applied: {message}" )); @@ -912,6 +914,7 @@ impl App { if let Err(err) = app_server.thread_memory_mode_set(thread_id, mode).await { tracing::error!(error = %err, %thread_id, "failed to update thread memory mode"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_error_message(format!( "Saved memory settings, but failed to update the current thread: {err}" )); @@ -922,6 +925,7 @@ impl App { &mut self, app_server: &mut AppServerSession, ) { + self.app_event_tx.send(AppEvent::FollowTranscript); if let Err(err) = app_server.memory_reset().await { tracing::error!(error = %err, "failed to reset memories"); self.chat_widget diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 7f1503567d..d565daa743 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -1962,6 +1962,7 @@ impl App { self.chat_widget.open_advanced_reasoning_popup(model); } AppEvent::ApplyAdvancedReasoning { model, effort } => { + self.app_event_tx.send(AppEvent::FollowTranscript); if self .active_thread_model_setting_update_params(model.clone()) .is_some_and(|params| params.permissions.is_some()) @@ -2199,6 +2200,7 @@ impl App { } } AppEvent::PersistModelSelection { model, effort } => { + self.app_event_tx.send(AppEvent::FollowTranscript); match self.persist_model_defaults( app_server.request_handle(), crate::config_update::build_model_selection_edits( @@ -2234,6 +2236,7 @@ impl App { } } AppEvent::SelectSessionModel { model, effort } => { + self.app_event_tx.send(AppEvent::FollowTranscript); self.select_session_model(app_server, model, effort).await; } AppEvent::CyberModelAutoReviewNotice => { @@ -2449,6 +2452,7 @@ impl App { } } AppEvent::PersistPlanModeReasoningEffort(effort) => { + self.app_event_tx.send(AppEvent::FollowTranscript); let key_path = "plan_mode_reasoning_effort"; let edit = if let Some(effort) = effort { crate::config_update::replace_config_value( @@ -2899,6 +2903,7 @@ impl App { Err(err) => { let error = format_config_error(&err); tracing::error!(error = %error, "failed to persist status line settings; keeping previous selection"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_error_message(format!( "Failed to save status line settings: {error}" )); @@ -2938,6 +2943,7 @@ impl App { } Err(err) => { tracing::error!(error = %err, "failed to persist terminal title items; keeping previous selection"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.revert_terminal_title_setup_preview(); self.chat_widget.add_error_message(format!( "Failed to save terminal title items: {err}" @@ -2977,6 +2983,7 @@ impl App { self.restore_runtime_theme_from_config(); self.refresh_status_line(); tracing::error!(error = %err, "failed to persist theme selection"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget .add_error_message(format!("Failed to save theme: {err}")); } @@ -3113,6 +3120,7 @@ impl App { ) { Ok(outcome) => outcome, Err(err) => { + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_error_message(err); return; } @@ -3124,6 +3132,7 @@ impl App { message, } => (*keymap_config, bindings, message), crate::keymap_setup::KeymapEditOutcome::Unchanged { message } => { + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_info_message(message, /*hint*/ None); return; } @@ -3161,10 +3170,12 @@ impl App { self.sync_side_thread_ui(); self.chat_widget .return_to_keymap_picker(&context, &action, &runtime_keymap); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_info_message(message, /*hint*/ None); } Err(err) => { tracing::error!(error = %err, "failed to persist keymap binding"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget .add_error_message(format!("Failed to save shortcut: {err}")); } @@ -3186,6 +3197,7 @@ impl App { ) { Ok(keymap_config) => keymap_config, Err(err) => { + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_error_message(err); return; } @@ -3194,6 +3206,7 @@ impl App { let runtime_keymap = match RuntimeKeymap::from_config(&keymap_config) { Ok(runtime_keymap) => runtime_keymap, Err(err) => { + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget .add_error_message(format!("Failed to refresh shortcuts: {err}")); return; @@ -3215,6 +3228,7 @@ impl App { self.sync_side_thread_ui(); self.chat_widget .return_to_keymap_picker(&context, &action, &runtime_keymap); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget.add_info_message( format!("Removed custom shortcut for `{context}.{action}`."), /*hint*/ None, @@ -3222,6 +3236,7 @@ impl App { } Err(err) => { tracing::error!(error = %err, "failed to clear keymap binding"); + self.app_event_tx.send(AppEvent::FollowTranscript); self.chat_widget .add_error_message(format!("Failed to remove shortcut: {err}")); } diff --git a/codex-rs/tui/src/app/experimental_features.rs b/codex-rs/tui/src/app/experimental_features.rs index 814959dc11..b89689948c 100644 --- a/codex-rs/tui/src/app/experimental_features.rs +++ b/codex-rs/tui/src/app/experimental_features.rs @@ -40,14 +40,20 @@ impl App { "{label} setting saved on the server for new threads. This thread is unchanged. Project or task settings may override it." ))) } - Ok(response) => Box::new(history_cell::new_error_event(format!( - "{label} setting was saved but is overridden: {}", - overridden_write_message(&response) - ))), - Err(err) => Box::new(history_cell::new_error_event(format!( - "Failed to save {label} setting: {}", - crate::config_update::format_config_error(&err) - ))), + Ok(response) => { + self.transcript_view.jump_to_latest(); + Box::new(history_cell::new_error_event(format!( + "{label} setting was saved but is overridden: {}", + overridden_write_message(&response) + ))) + } + Err(err) => { + self.transcript_view.jump_to_latest(); + Box::new(history_cell::new_error_event(format!( + "Failed to save {label} setting: {}", + crate::config_update::format_config_error(&err) + ))) + } }; self.insert_history_cell(tui, notice); } diff --git a/codex-rs/tui/src/app/tests/local_command_scroll_tests.rs b/codex-rs/tui/src/app/tests/local_command_scroll_tests.rs index 138f0042d8..45ee9e7cf9 100644 --- a/codex-rs/tui/src/app/tests/local_command_scroll_tests.rs +++ b/codex-rs/tui/src/app/tests/local_command_scroll_tests.rs @@ -47,6 +47,244 @@ fn submit_local_command(app: &mut App, command: &str) { assert_eq!(app.chat_widget.composer_text_with_pending(), ""); } +#[tokio::test] +async fn settings_pickers_preserve_the_reading_anchor_through_open_and_close() -> Result<()> { + let (mut app, mut events, _op_rx) = make_test_app_with_channels().await; + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut server = start_config_write_test_app_server(&app).await?; + tui.set_owned_screen(/*owned*/ true)?; + app.chat_widget.handle_thread_session(test_thread_session( + ThreadId::new(), + app.config.cwd.to_path_buf(), + )); + app.sync_tui_theme_selection("base16-ocean-dark".to_string()); + hold_older_history(&mut app, &mut tui); + let held = transcript_buffer(&mut app); + + for (command, close) in [ + ("/model", KeyCode::Esc), + ("/theme", KeyCode::Enter), + ("/keymap", KeyCode::Esc), + ("/memories", KeyCode::Esc), + ("/title", KeyCode::Esc), + ("/statusline", KeyCode::Esc), + ] { + while events.try_recv().is_ok() {} + submit_local_command(&mut app, command); + while let Ok(event) = events.try_recv() { + app.handle_event(&mut tui, &mut server, event).await?; + } + assert!(app.chat_widget.has_active_view(), "{command}"); + assert!(!app.transcript_view.is_following(), "{command} opened"); + assert_eq!(transcript_buffer(&mut app), held, "{command} opened"); + + app.handle_tui_event(&mut tui, &mut server, TuiEvent::Key(close.into())) + .await?; + let mut selected_theme = None; + while let Ok(event) = events.try_recv() { + if let AppEvent::SyntaxThemeSelected { name } = &event { + selected_theme = Some(name.clone()); + } + app.handle_event(&mut tui, &mut server, event).await?; + } + if command == "/theme" { + assert_eq!(selected_theme, Some("base16-ocean-dark".to_string())); + } + assert!(!app.chat_widget.has_active_view(), "{command}"); + assert!(!app.transcript_view.is_following(), "{command} closed"); + assert_eq!(transcript_buffer(&mut app), held, "{command} closed"); + } + + let visible = transcript_buffer(&mut app) + .content + .chunks(/*chunk_size*/ 80) + .map(|row| { + row.iter() + .map(ratatui::buffer::Cell::symbol) + .collect::() + .trim_end() + .to_owned() + }) + .collect::>() + .join("\n"); + insta::assert_snapshot!(visible, @" + older 0 + older 1 + older 2 + "); + tui.set_owned_screen(/*owned*/ false)?; + server.shutdown().await?; + Ok(()) +} + +#[tokio::test] +async fn settings_picker_save_failures_reveal_their_errors() -> Result<()> { + let (mut app, mut events, _op_rx) = make_test_app_with_channels().await; + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut server = start_config_write_test_app_server(&app).await?; + tui.set_owned_screen(/*owned*/ true)?; + app.sync_tui_theme_selection("base16-ocean-dark".to_string()); + // Break writes only after startup, as can happen when the config is edited externally. + std::fs::write(app.local_settings.user_config_path.as_path(), "[broken")?; + + for event in [ + AppEvent::SyntaxThemeSelected { + name: "base16-ocean-dark".into(), + }, + AppEvent::StatusLineSetup { + items: vec![], + use_theme_colors: false, + }, + AppEvent::TerminalTitleSetup { items: vec![] }, + AppEvent::PersistModelSelection { + model: "gpt-5.5".into(), + effort: Some(ReasoningEffortConfig::High), + }, + AppEvent::ApplyAdvancedReasoning { + model: "gpt-5.5".into(), + effort: ReasoningEffortConfig::Ultra, + }, + AppEvent::PersistPlanModeReasoningEffort(Some(ReasoningEffortConfig::High)), + AppEvent::UpdateMemorySettings { + use_memories: true, + generate_memories: true, + }, + AppEvent::EnableFeatureForNewThreads(Feature::MemoryTool), + AppEvent::KeymapCaptured { + context: "global".into(), + action: "find_transcript".into(), + key: "f12".into(), + intent: crate::app_event::KeymapEditIntent::ReplaceAll, + }, + AppEvent::KeymapCleared { + context: "global".into(), + action: "find_transcript".into(), + }, + ] { + hold_older_history(&mut app, &mut tui); + while events.try_recv().is_ok() {} + let selection = format!("{event:?}"); + let is_theme = matches!(event, AppEvent::SyntaxThemeSelected { .. }); + if is_theme { + submit_local_command(&mut app, "/theme"); + app.handle_tui_event(&mut tui, &mut server, TuiEvent::Key(KeyCode::Enter.into())) + .await?; + } else { + app.handle_event(&mut tui, &mut server, event).await?; + } + while let Ok(event) = events.try_recv() { + app.handle_event(&mut tui, &mut server, event).await?; + } + assert!(app.transcript_view.is_following(), "{selection}"); + let error = app.transcript_cells.last().expect("save error"); + assert!( + lines_to_single_string(&error.display_lines(/*width*/ 80)).contains("Failed to"), + "{selection}" + ); + if is_theme { + let area = Rect::new( + /*x*/ 0, /*y*/ 0, /*width*/ 80, /*height*/ 8, + ); + let mut buffer = Buffer::empty(area); + app.transcript_view + .render(area, &mut buffer, &app.transcript_cells); + let visible = buffer + .content + .chunks(/*chunk_size*/ 80) + .map(|row| { + row.iter() + .map(ratatui::buffer::Cell::symbol) + .collect::() + .trim_end() + .to_owned() + }) + .collect::>() + .join("\n"); + insta::assert_snapshot!(visible, @" + older 4 + older 5 + + ■ Failed to save theme: TOML parse error at line 1, column 8 + | + 1 | [broken + | ^ + unclosed table, expected `]` + "); + } + } + tui.set_owned_screen(/*owned*/ false)?; + server.shutdown().await?; + Ok(()) +} + +#[tokio::test] +async fn settings_confirmations_reveal_their_history_output() -> Result<()> { + let (mut app, mut events, _op_rx) = make_test_app_with_channels().await; + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut server = start_config_write_test_app_server(&app).await?; + tui.set_owned_screen(/*owned*/ true)?; + // A stale thread id makes the active-thread update fail independently of config writes. + let thread_id = ThreadId::new(); + app.active_thread_id = Some(thread_id); + app.chat_widget + .handle_thread_session(test_thread_session(thread_id, app.config.cwd.to_path_buf())); + for (event, output) in [ + ( + AppEvent::SelectSessionModel { + model: "gpt-5.5".into(), + effort: Some(ReasoningEffortConfig::High), + }, + "Failed to update thread settings", + ), + ( + AppEvent::UpdateLunaReserveReasoning { + thread_id, + effort: Some(ReasoningEffortConfig::High), + }, + "Failed to update thread settings", + ), + (AppEvent::ResetMemories, "Reset local memories."), + ( + AppEvent::KeymapCaptured { + context: "global".into(), + action: "find_transcript".into(), + key: "f12".into(), + intent: crate::app_event::KeymapEditIntent::ReplaceAll, + }, + "global.find_transcript", + ), + ( + AppEvent::KeymapCleared { + context: "global".into(), + action: "find_transcript".into(), + }, + "Removed custom shortcut", + ), + ] { + hold_older_history(&mut app, &mut tui); + while events.try_recv().is_ok() {} + if matches!(event, AppEvent::UpdateLunaReserveReasoning { .. }) { + app.chat_widget + .set_model(crate::model_catalog::LUNA_RESERVE_MODEL); + } + let previous_cells = app.transcript_cells.len(); + app.handle_event(&mut tui, &mut server, event).await?; + while let Ok(event) = events.try_recv() { + app.handle_event(&mut tui, &mut server, event).await?; + } + assert!(app.transcript_view.is_following()); + let messages = app.transcript_cells[previous_cells..] + .iter() + .map(|cell| lines_to_single_string(&cell.display_lines(/*width*/ 80))) + .collect::>() + .join("\n"); + assert!(messages.contains(output), "expected {output:?}: {messages}"); + } + tui.set_owned_screen(/*owned*/ false)?; + server.shutdown().await?; + Ok(()) +} + #[tokio::test] async fn local_command_and_inline_error_reveal_their_history_output() -> Result<()> { let (mut app, mut events, _op_rx) = make_test_app_with_channels().await; @@ -55,21 +293,22 @@ async fn local_command_and_inline_error_reveal_their_history_output() -> Result< tui.set_owned_screen(/*owned*/ true)?; for (command, output) in [ + ( + "/model", + "Model selection is disabled until startup completes.", + ), ("/status", "/status"), ("/keymap invalid", "Usage: /keymap [debug]"), ] { hold_older_history(&mut app, &mut tui); while events.try_recv().is_ok() {} submit_local_command(&mut app, command); - let follow = events.try_recv().expect("command follow event"); - assert_matches!(&follow, AppEvent::FollowTranscript); - app.handle_event(&mut tui, &mut server, follow).await?; let mut inserted = false; while let Ok(event) = events.try_recv() { - if matches!(&event, AppEvent::InsertHistoryCell(_)) { - app.handle_event(&mut tui, &mut server, event).await?; + if matches!(event, AppEvent::InsertHistoryCell(_)) { inserted = true; } + app.handle_event(&mut tui, &mut server, event).await?; } assert!(inserted, "{command} should emit local history output"); assert!(app.transcript_view.is_following()); diff --git a/codex-rs/tui/src/chatwidget/input_flow.rs b/codex-rs/tui/src/chatwidget/input_flow.rs index e1f3369fb7..a6b5cdd9a5 100644 --- a/codex-rs/tui/src/chatwidget/input_flow.rs +++ b/codex-rs/tui/src/chatwidget/input_flow.rs @@ -3,7 +3,8 @@ //! The queue data itself lives in `input_queue`; this module owns the app-level //! effects around taking composer input, submitting user turns, draining queued //! follow-ups, and restoring draft state across interrupts or thread switches. -//! Composer submissions resume transcript following before dispatch or startup queueing. +//! Composer submissions resume transcript following before dispatch or startup queueing, +//! except for reversible settings pickers that preserve the reading position. use super::*; use crate::bottom_pane::prompt_args::parse_slash_name; @@ -22,12 +23,26 @@ impl ChatWidget { input_result: InputResult, had_modal_or_popup: bool, ) { - if matches!( - &input_result, + let follow_transcript = match &input_result { + // Opening settings is not a request to leave the current reading anchor. + // Inline commands still follow so their output (including usage errors) is visible. + InputResult::Command( + SlashCommand::Model + | SlashCommand::Keymap + | SlashCommand::Memories + | SlashCommand::Title + | SlashCommand::Statusline + | SlashCommand::Theme, + ) => false, InputResult::Command(_) - | InputResult::ServiceTierCommand(_) - | InputResult::CommandWithArgs(..) - ) { + | InputResult::ServiceTierCommand(_) + | InputResult::CommandWithArgs(..) => true, + InputResult::Submitted { .. } + | InputResult::Queued { .. } + | InputResult::ParentOwnedInputBlocked + | InputResult::None => false, + }; + if follow_transcript { self.app_event_tx.send(AppEvent::FollowTranscript); } match input_result { @@ -80,6 +95,10 @@ impl ChatWidget { } InputResult::Command(cmd) => { self.handle_slash_command_dispatch(cmd); + // A settings command can instead report why it is unavailable. + if !follow_transcript && self.bottom_pane.no_modal_or_popup_active() { + self.app_event_tx.send(AppEvent::FollowTranscript); + } } InputResult::ServiceTierCommand(command) => { self.handle_service_tier_command_dispatch(command);