From d32e05d4e51f38811683f1629a522c4ac5010325 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Mon, 9 Feb 2026 01:01:46 -0300 Subject: [PATCH] fix(tui): treat tabs as visible width, preserve theme warning on resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Tabs in syntax-highlighted diff spans now count as 4 columns instead of zero, so tab-indented code wraps correctly in the diff view. - Move theme warning injection to after the last possible config reload (session resume/fork) so the ⚠ banner is not silently discarded. --- codex-rs/tui/src/diff_render.rs | 21 +++++++++-- codex-rs/tui/src/lib.rs | 62 +++++++++++++++++++++++++++++---- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 62a557746d..8d9ab7d575 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -14,6 +14,9 @@ use std::path::PathBuf; use unicode_width::UnicodeWidthChar; +/// Display width of a tab character in columns. +const TAB_WIDTH: usize = 4; + use crate::exec_command::relativize_to_home; use crate::render::Insets; use crate::render::highlight::exceeds_highlight_limits; @@ -596,7 +599,8 @@ fn wrap_styled_spans(spans: &[RtSpan<'static>], max_cols: usize) -> Vec max_cols && byte_end > 0 { // Adding this character would exceed the line width and we // already have some content — break here. @@ -618,7 +622,7 @@ fn wrap_styled_spans(spans: &[RtSpan<'static>], max_cols: usize) -> Vec= 2, + "tab + 5 chars should exceed 8 cols and wrap, got {} line(s): {result:?}", + result.len() + ); + } + #[test] fn large_update_diff_skips_highlighting() { // Build a patch large enough to exceed MAX_HIGHLIGHT_LINES (10_000). diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index b11f93f92f..fb64abeb1f 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -433,7 +433,7 @@ pub async fn run_main( async fn run_ratatui_app( cli: Cli, - mut initial_config: Config, + initial_config: Config, overrides: ConfigOverrides, cli_kv_overrides: Vec<(String, toml::Value)>, mut cloud_requirements: CloudRequirementsLoader, @@ -443,12 +443,12 @@ async fn run_ratatui_app( // Configure syntax highlighting theme before any rendering can occur. // Surface resolution failures as a startup warning (⚠ banner in chat). - if let Some(warning) = crate::render::highlight::set_theme_override( + // Keep the warning separate so we can re-inject it after onboarding + // reloads config (which would otherwise discard it). + let theme_warning = crate::render::highlight::set_theme_override( initial_config.tui_theme.clone(), find_codex_home().ok(), - ) { - initial_config.startup_warnings.push(warning); - } + ); tooltips::announcement::prewarm(); @@ -554,6 +554,7 @@ async fn run_ratatui_app( } else { initial_config }; + let mut missing_session_exit = |id_str: &str, action: &str| { error!("Error finding conversation path: {id_str}"); restore(); @@ -702,7 +703,7 @@ async fn run_ratatui_app( None => None, }; - let config = match &session_selection { + let mut config = match &session_selection { resume_picker::SessionSelection::Resume(_) | resume_picker::SessionSelection::Fork(_) => { load_config_or_exit_with_fallback_cwd( cli_kv_overrides.clone(), @@ -714,6 +715,14 @@ async fn run_ratatui_app( } _ => config, }; + + // Inject theme warning after the last possible config reload (onboarding + // at line ~520 and resume/fork just above can both replace the config, + // discarding any previously pushed warnings). + if let Some(w) = theme_warning { + config.startup_warnings.push(w); + } + set_default_client_residency_requirement(config.enforce_residency.value()); let active_profile = config.active_profile.clone(); let should_show_trust_screen = should_show_trust_screen(&config); @@ -1195,6 +1204,47 @@ trust_level = "untrusted" Ok(()) } + /// Regression: theme warning must survive config reloads. + /// + /// `run_ratatui_app` reloads config twice — once during onboarding and + /// once on session resume/fork. Each reload produces a fresh `Config` + /// with an empty `startup_warnings`. The theme warning (captured before + /// any reload) must be injected *after the last reload* so it is never + /// discarded. This test exercises that exact pattern. + #[tokio::test] + async fn theme_warning_survives_config_reload() -> std::io::Result<()> { + let temp_dir = TempDir::new()?; + + // Capture a theme warning before any config reload (mirrors the real + // call to `set_theme_override` at the top of `run_ratatui_app`). + let theme_warning: Option = Some("unknown syntax theme \"bogus\"".into()); + + // First config — stands in for `initial_config` after onboarding. + let mut config = build_config(&temp_dir).await?; + config.startup_warnings.push("other warning".into()); + + // Simulate the resume/fork config reload: a fresh config with no + // startup_warnings (just like `load_config_or_exit_with_fallback_cwd` + // returns). + let mut config = build_config(&temp_dir).await?; + assert!( + config.startup_warnings.is_empty(), + "freshly loaded config should have no warnings" + ); + + // Re-inject after the last reload — the pattern used in run_ratatui_app. + if let Some(w) = theme_warning { + config.startup_warnings.push(w); + } + + assert_eq!(config.startup_warnings.len(), 1); + assert!( + config.startup_warnings[0].contains("bogus"), + "theme warning should be present after config reload" + ); + Ok(()) + } + #[tokio::test] async fn read_session_cwd_falls_back_to_session_meta() -> std::io::Result<()> { let temp_dir = TempDir::new()?;