From 5bce369c4d24d8b408260969ff4832d0819dced4 Mon Sep 17 00:00:00 2001 From: ae Date: Mon, 18 Aug 2025 08:26:29 -0700 Subject: [PATCH] fix: clean up styles & colors and define in styles.md (#2401) New style guide: # Headers, primary, and secondary text - **Headers:** Use `bold`. For markdown with various header levels, leave in the `#` signs. - **Primary text:** Default. - **Secondary text:** Use `dim`. # Foreground colors - **Default:** Most of the time, just use the default foreground color. `reset` can help get it back. - **Selection:** Use ANSI `blue`. (Ed & AE want to make this cyan too, but we'll do that in a followup since it's riskier in different themes.) - **User input tips and status indicators:** Use ANSI `cyan`. - **Success and additions:** Use ANSI `green`. - **Errors, failures and deletions:** Use ANSI `red`. - **Codex:** Use ANSI `magenta`. # Avoid - Avoid custom colors because there's no guarantee that they'll contrast well or look good on various terminal color themes. - Avoid ANSI `black`, `white`, `yellow` as foreground colors because the terminal theme will do a better job. (Use `reset` if you need to in order to get those.) The exception is if you need contrast rendering over a manually colored background. (There are some rules to try to catch this in `clippy.toml`.) # Testing Tested in a variety of light and dark color themes in Terminal, iTerm2, and Ghostty. --- AGENTS.md | 4 +++ codex-rs/clippy.toml | 9 ++++- .../src/bottom_pane/selection_popup_common.rs | 2 +- codex-rs/tui/src/colors.rs | 4 --- codex-rs/tui/src/history_cell.rs | 12 +++---- codex-rs/tui/src/lib.rs | 2 +- codex-rs/tui/src/onboarding/auth.rs | 36 ++++++------------- .../tui/src/onboarding/trust_directory.rs | 10 +----- codex-rs/tui/src/shimmer.rs | 11 ++++-- codex-rs/tui/styles.md | 22 ++++++++++++ 10 files changed, 62 insertions(+), 50 deletions(-) delete mode 100644 codex-rs/tui/src/colors.rs create mode 100644 codex-rs/tui/styles.md diff --git a/AGENTS.md b/AGENTS.md index b2f21c7c..f1744a87 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,10 @@ Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` director 1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. 2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. +## TUI style conventions + +See `codex-rs/tui/styles.md`. + ## TUI code conventions - Use concise styling helpers from ratatui’s Stylize trait. diff --git a/codex-rs/clippy.toml b/codex-rs/clippy.toml index 9e614bec..5a6ff7f0 100644 --- a/codex-rs/clippy.toml +++ b/codex-rs/clippy.toml @@ -1,2 +1,9 @@ allow-expect-in-tests = true -allow-unwrap-in-tests = true \ No newline at end of file +allow-unwrap-in-tests = true +disallowed-methods = [ + { path = "ratatui::style::Color::Rgb", reason = "Use ANSI colors, which work better in various terminal themes." }, + { path = "ratatui::style::Color::Indexed", reason = "Use ANSI colors, which work better in various terminal themes." }, + { path = "ratatui::style::Stylize::white", reason = "Avoid hardcoding white; prefer default fg or dim/bold. Exception: Disable this rule if rendering over a hardcoded ANSI background." }, + { path = "ratatui::style::Stylize::black", reason = "Avoid hardcoding black; prefer default fg or dim/bold. Exception: Disable this rule if rendering over a hardcoded ANSI background." }, + { path = "ratatui::style::Stylize::yellow", reason = "Avoid yellow; prefer other colors in `tui/styles.md`." }, +] diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 0a0574c6..1a032313 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -101,7 +101,7 @@ pub(crate) fn render_rows( if Some(i) == state.selected_idx { cell = cell.style( Style::default() - .fg(Color::Yellow) + .fg(Color::Blue) .add_modifier(Modifier::BOLD), ); } else if *is_current { diff --git a/codex-rs/tui/src/colors.rs b/codex-rs/tui/src/colors.rs deleted file mode 100644 index 0ba386df..00000000 --- a/codex-rs/tui/src/colors.rs +++ /dev/null @@ -1,4 +0,0 @@ -use ratatui::style::Color; - -pub(crate) const LIGHT_BLUE: Color = Color::Rgb(134, 238, 255); -pub(crate) const SUCCESS_GREEN: Color = Color::Rgb(169, 230, 158); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index faf01c5e..f6c69306 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1,4 +1,3 @@ -use crate::colors::LIGHT_BLUE; use crate::diff_render::create_diff_summary; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; @@ -252,12 +251,13 @@ fn new_parsed_command( lines.push(Line::from(spans)); } Some(o) if o.exit_code == 0 => { - lines.push(Line::from("✓ Completed".green().bold())); + lines.push(Line::from(vec!["✓".green(), " Completed".into()])); } Some(o) => { - lines.push(Line::from( - format!("✗ Failed (exit {})", o.exit_code).red().bold(), - )); + lines.push(Line::from(vec![ + "✗".red(), + format!(" Failed (exit {})", o.exit_code).into(), + ])); } }; @@ -304,7 +304,7 @@ fn new_parsed_command( let prefix = if j == 0 { first_prefix } else { " " }; lines.push(Line::from(vec![ Span::styled(prefix, Style::default().add_modifier(Modifier::DIM)), - Span::styled(line_text.to_string(), Style::default().fg(LIGHT_BLUE)), + line_text.to_string().dim(), ])); } } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 7d605d68..2d9210ca 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -2,6 +2,7 @@ // The standalone `codex-tui` binary prints a short help message before the // alternate‑screen mode starts; that file opts‑out locally via `allow`. #![deny(clippy::print_stdout, clippy::print_stderr)] +#![deny(clippy::disallowed_methods)] use app::App; use codex_core::BUILT_IN_OSS_MODEL_PROVIDER_ID; use codex_core::config::Config; @@ -28,7 +29,6 @@ mod bottom_pane; mod chatwidget; mod citation_regex; mod cli; -mod colors; mod common; pub mod custom_terminal; mod diff_render; diff --git a/codex-rs/tui/src/onboarding/auth.rs b/codex-rs/tui/src/onboarding/auth.rs index de986786..0c223645 100644 --- a/codex-rs/tui/src/onboarding/auth.rs +++ b/codex-rs/tui/src/onboarding/auth.rs @@ -9,6 +9,7 @@ use ratatui::prelude::Widget; use ratatui::style::Color; use ratatui::style::Modifier; use ratatui::style::Style; +use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; use ratatui::widgets::Paragraph; @@ -19,8 +20,6 @@ use codex_login::AuthMode; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; -use crate::colors::LIGHT_BLUE; -use crate::colors::SUCCESS_GREEN; use crate::onboarding::onboarding_screen::KeyboardHandler; use crate::onboarding::onboarding_screen::StepStateProvider; use crate::shimmer::shimmer_spans; @@ -131,11 +130,8 @@ impl AuthModeWidget { let line1 = if is_selected { Line::from(vec![ - Span::styled( - format!("{} {}. ", caret, idx + 1), - Style::default().fg(LIGHT_BLUE).add_modifier(Modifier::DIM), - ), - Span::styled(text.to_owned(), Style::default().fg(LIGHT_BLUE)), + format!("{} {}. ", caret, idx + 1).blue().dim(), + text.to_string().blue(), ]) } else { Line::from(format!(" {}. {text}", idx + 1)) @@ -143,7 +139,8 @@ impl AuthModeWidget { let line2 = if is_selected { Line::from(format!(" {description}")) - .style(Style::default().fg(LIGHT_BLUE).add_modifier(Modifier::DIM)) + .fg(Color::Blue) + .add_modifier(Modifier::DIM) } else { Line::from(format!(" {description}")) .style(Style::default().add_modifier(Modifier::DIM)) @@ -197,12 +194,7 @@ impl AuthModeWidget { lines.push(Line::from(" If the link doesn't open automatically, open the following link to authenticate:")); lines.push(Line::from(vec![ Span::raw(" "), - Span::styled( - state.auth_url.as_str(), - Style::default() - .fg(LIGHT_BLUE) - .add_modifier(Modifier::UNDERLINED), - ), + state.auth_url.as_str().blue().underlined(), ])); lines.push(Line::from("")); } @@ -218,8 +210,7 @@ impl AuthModeWidget { fn render_chatgpt_success_message(&self, area: Rect, buf: &mut Buffer) { let lines = vec![ - Line::from("✓ Signed in with your ChatGPT account") - .style(Style::default().fg(SUCCESS_GREEN)), + Line::from("✓ Signed in with your ChatGPT account").fg(Color::Green), Line::from(""), Line::from("> Before you start:"), Line::from(""), @@ -233,8 +224,7 @@ impl AuthModeWidget { ]) .style(Style::default().add_modifier(Modifier::DIM)), Line::from(""), - Line::from(" Codex can make mistakes") - .style(Style::default().fg(Color::White)), + Line::from(" Codex can make mistakes"), Line::from(" Review the code it writes and commands it runs") .style(Style::default().add_modifier(Modifier::DIM)), Line::from(""), @@ -248,7 +238,7 @@ impl AuthModeWidget { ]) .style(Style::default().add_modifier(Modifier::DIM)), Line::from(""), - Line::from(" Press Enter to continue").style(Style::default().fg(LIGHT_BLUE)), + Line::from(" Press Enter to continue").fg(Color::Blue), ]; Paragraph::new(lines) @@ -257,10 +247,7 @@ impl AuthModeWidget { } fn render_chatgpt_success(&self, area: Rect, buf: &mut Buffer) { - let lines = vec![ - Line::from("✓ Signed in with your ChatGPT account") - .style(Style::default().fg(SUCCESS_GREEN)), - ]; + let lines = vec![Line::from("✓ Signed in with your ChatGPT account").fg(Color::Green)]; Paragraph::new(lines) .wrap(Wrap { trim: false }) @@ -268,8 +255,7 @@ impl AuthModeWidget { } fn render_env_var_found(&self, area: Rect, buf: &mut Buffer) { - let lines = - vec![Line::from("✓ Using OPENAI_API_KEY").style(Style::default().fg(SUCCESS_GREEN))]; + let lines = vec![Line::from("✓ Using OPENAI_API_KEY").fg(Color::Green)]; Paragraph::new(lines) .wrap(Wrap { trim: false }) diff --git a/codex-rs/tui/src/onboarding/trust_directory.rs b/codex-rs/tui/src/onboarding/trust_directory.rs index 3be9bac1..f5ad123b 100644 --- a/codex-rs/tui/src/onboarding/trust_directory.rs +++ b/codex-rs/tui/src/onboarding/trust_directory.rs @@ -18,8 +18,6 @@ use ratatui::widgets::Paragraph; use ratatui::widgets::WidgetRef; use ratatui::widgets::Wrap; -use crate::colors::LIGHT_BLUE; - use crate::onboarding::onboarding_screen::KeyboardHandler; use crate::onboarding::onboarding_screen::StepStateProvider; @@ -77,13 +75,7 @@ impl WidgetRef for &TrustDirectoryWidget { |idx: usize, option: TrustDirectorySelection, text: &str| -> Line<'static> { let is_selected = self.highlighted == option; if is_selected { - Line::from(vec![ - Span::styled( - format!("> {}. ", idx + 1), - Style::default().fg(LIGHT_BLUE).add_modifier(Modifier::DIM), - ), - Span::styled(text.to_owned(), Style::default().fg(LIGHT_BLUE)), - ]) + Line::from(format!("> {}. {text}", idx + 1)).blue() } else { Line::from(format!(" {}. {}", idx + 1, text)) } diff --git a/codex-rs/tui/src/shimmer.rs b/codex-rs/tui/src/shimmer.rs index 0d0e5d0b..2523f6ec 100644 --- a/codex-rs/tui/src/shimmer.rs +++ b/codex-rs/tui/src/shimmer.rs @@ -46,9 +46,14 @@ pub(crate) fn shimmer_spans(text: &str) -> Vec> { let brightness = 0.4 + 0.6 * t; let level = (brightness * 255.0).clamp(0.0, 255.0) as u8; let style = if has_true_color { - Style::default() - .fg(Color::Rgb(level, level, level)) - .add_modifier(Modifier::BOLD) + // Allow custom RGB colors, as the implementation is thoughtfully + // adjusting the level of the default foreground color. + #[allow(clippy::disallowed_methods)] + { + Style::default() + .fg(Color::Rgb(level, level, level)) + .add_modifier(Modifier::BOLD) + } } else { color_for_level(level) }; diff --git a/codex-rs/tui/styles.md b/codex-rs/tui/styles.md new file mode 100644 index 00000000..c0c68041 --- /dev/null +++ b/codex-rs/tui/styles.md @@ -0,0 +1,22 @@ +# Headers, primary, and secondary text + +- **Headers:** Use `bold`. For markdown with various header levels, leave in the `#` signs. +- **Primary text:** Default. +- **Secondary text:** Use `dim`. + +# Foreground colors + +- **Default:** Most of the time, just use the default foreground color. `reset` can help get it back. +- **Selection:** Use ANSI `blue`. (Ed & AE want to make this cyan too, but we'll do that in a followup since it's riskier in different themes.) +- **User input tips and status indicators:** Use ANSI `cyan`. +- **Success and additions:** Use ANSI `green`. +- **Errors, failures and deletions:** Use ANSI `red`. +- **Codex:** Use ANSI `magenta`. + +# Avoid + +- Avoid custom colors because there's no guarantee that they'll contrast well or look good in various terminal color themes. +- Avoid ANSI `black` & `white` as foreground colors because the default terminal theme color will do a better job. (Use `reset` if you need to in order to get those.) The exception is if you need contrast rendering over a manually colored background. +- Avoid ANSI `yellow` because for now the style guide doesn't use it. Prefer a foreground color mentioned above. + +(There are some rules to try to catch this in `clippy.toml`.)