mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
fix(tui): treat tabs as visible width, preserve theme warning on resume
- 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.
This commit is contained in:
@@ -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<Vec<RtSp
|
||||
let mut chars_col = 0;
|
||||
|
||||
for ch in remaining.chars() {
|
||||
let w = ch.width().unwrap_or(0);
|
||||
// Tabs have no Unicode width; treat them as TAB_WIDTH columns.
|
||||
let w = ch.width().unwrap_or(if ch == '\t' { TAB_WIDTH } else { 0 });
|
||||
if col + chars_col + w > 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<Vec<RtSp
|
||||
};
|
||||
let ch_len = ch.len_utf8();
|
||||
current_line.push(RtSpan::styled(remaining[..ch_len].to_string(), style));
|
||||
col = ch.width().unwrap_or(1);
|
||||
col = ch.width().unwrap_or(if ch == '\t' { TAB_WIDTH } else { 1 });
|
||||
remaining = &remaining[ch_len..];
|
||||
continue;
|
||||
}
|
||||
@@ -1058,6 +1062,19 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn wrap_styled_spans_tabs_have_visible_width() {
|
||||
// A tab should count as TAB_WIDTH columns, not zero.
|
||||
// With max_cols=8, a tab (4 cols) + "abcde" (5 cols) = 9 cols → must wrap.
|
||||
let spans = vec![RtSpan::raw("\tabcde")];
|
||||
let result = wrap_styled_spans(&spans, 8);
|
||||
assert!(
|
||||
result.len() >= 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).
|
||||
|
||||
+56
-6
@@ -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<String> = 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()?;
|
||||
|
||||
Reference in New Issue
Block a user