From a97ae65362e8b03c196a24c961cf7bfaa129db56 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 19 Jul 2026 21:29:34 +0000 Subject: [PATCH] Remeasure dynamic cells in the transcript overlay (#34232) ## Why The transcript overlay cached every committed cell's height. Cells whose content changes after insertion, such as refreshed status output or newly available visualizations, could outgrow that cached height and be clipped. ## What changed - Let history cells declare whether their transcript height is stable, and remeasure dynamic cells on later overlay renders. - Preserve cached heights for stable cells when appending a committed cell instead of rebuilding all renderables. ## Testing - Cover status output growing after a rate-limit refresh. - Cover a visualization link replacing its unavailable placeholder. - Verify that appending a cell preserves cached measurements for stable cells. GitOrigin-RevId: 03e7166cb94d7353c9b89bff8abb3b7ccb84a6bb --- codex-rs/tui/src/history_cell/base.rs | 4 + codex-rs/tui/src/history_cell/messages.rs | 4 + codex-rs/tui/src/history_cell/mod.rs | 8 ++ .../tui/src/inline_visualization_tests.rs | 81 ++++++++++++ codex-rs/tui/src/pager_overlay.rs | 115 +++++++++++++----- ...erlay_visualization_becomes_available.snap | 30 +++++ ...ipt_overlay_status_rate_limit_refresh.snap | 66 ++++++++++ codex-rs/tui/src/status/tests.rs | 109 +++++++++++++++++ 8 files changed, 385 insertions(+), 32 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__inline_visualization__tests__transcript_overlay_visualization_becomes_available.snap create mode 100644 codex-rs/tui/src/status/snapshots/codex_tui__status__tests__transcript_overlay_status_rate_limit_refresh.snap diff --git a/codex-rs/tui/src/history_cell/base.rs b/codex-rs/tui/src/history_cell/base.rs index e9ade84945..a90a35c151 100644 --- a/codex-rs/tui/src/history_cell/base.rs +++ b/codex-rs/tui/src/history_cell/base.rs @@ -162,4 +162,8 @@ impl HistoryCell for CompositeHistoryCell { } out } + + fn has_stable_transcript_height(&self) -> bool { + false + } } diff --git a/codex-rs/tui/src/history_cell/messages.rs b/codex-rs/tui/src/history_cell/messages.rs index ce5082fef4..58892e4261 100644 --- a/codex-rs/tui/src/history_cell/messages.rs +++ b/codex-rs/tui/src/history_cell/messages.rs @@ -449,6 +449,10 @@ impl HistoryCell for AgentMarkdownCell { fn raw_lines(&self) -> Vec> { raw_lines_from_source(&self.markdown_source) } + + fn has_stable_transcript_height(&self) -> bool { + self.rendered_lines.is_some() + } } #[cfg(test)] diff --git a/codex-rs/tui/src/history_cell/mod.rs b/codex-rs/tui/src/history_cell/mod.rs index 0eb41da021..5708f27a21 100644 --- a/codex-rs/tui/src/history_cell/mod.rs +++ b/codex-rs/tui/src/history_cell/mod.rs @@ -279,6 +279,14 @@ pub(crate) trait HistoryCell: std::fmt::Debug + Send + Sync + Any { .unwrap_or(0) } + /// Whether the transcript height remains valid across later overlay renders. + /// + /// Cells backed by external state should return `false` so the pager remeasures them before + /// rendering instead of reusing a height that may now clip their content. + fn has_stable_transcript_height(&self) -> bool { + true + } + fn is_stream_continuation(&self) -> bool { false } diff --git a/codex-rs/tui/src/inline_visualization_tests.rs b/codex-rs/tui/src/inline_visualization_tests.rs index a883e58b67..4243d9517c 100644 --- a/codex-rs/tui/src/inline_visualization_tests.rs +++ b/codex-rs/tui/src/inline_visualization_tests.rs @@ -2,10 +2,15 @@ use super::*; use crate::history_cell::AgentMarkdownCell; use crate::history_cell::HistoryCell; use crate::history_cell::HistoryRenderMode; +use crate::keymap::RuntimeKeymap; +use crate::pager_overlay::TranscriptOverlay; use crate::streaming::controller::StreamController; use pretty_assertions::assert_eq; +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; use ratatui::style::Style; use ratatui::style::Stylize; +use std::sync::Arc; use tempfile::TempDir; fn context_with_fragment(fragment: &str) -> (TempDir, InlineVisualizationContext) { @@ -47,6 +52,28 @@ fn line_text(line: &ratatui::text::Line<'_>) -> String { .collect() } +fn buffer_to_text(buffer: &Buffer, width: u16) -> String { + buffer + .content + .chunks(usize::from(width)) + .map(|row| { + row.iter() + .map(|cell| { + let symbol = cell.symbol(); + symbol + .strip_prefix("\x1b]8;;") + .and_then(|symbol| symbol.split_once('\x07')) + .and_then(|(_, symbol)| symbol.strip_suffix("\x1b]8;;\x07")) + .unwrap_or(symbol) + }) + .collect::() + .trim_end() + .to_string() + }) + .collect::>() + .join("\n") +} + #[test] fn rewrites_complete_directive_to_trusted_static_file_placeholder() { let (_codex_home, context) = context_with_fragment("
chart
"); @@ -250,6 +277,60 @@ fn finalized_agent_cell_replays_visualization_link() { ); } +#[test] +fn transcript_overlay_remeasures_visualization_when_artifact_becomes_available() { + let codex_home = tempfile::tempdir().expect("temp codex home"); + let context = InlineVisualizationContext::new(codex_home.path(), ThreadId::new()) + .expect("UUIDv7 thread id should provide a timestamp"); + fs::create_dir_all(&context.thread_dir).expect("create visualization directory"); + + let cell = AgentMarkdownCell::new_with_inline_visualizations( + "::codex-inline-vis{file=\"chart.html\"}".to_string(), + Path::new("/workspace"), + Some(context.clone()), + ); + let mut overlay = TranscriptOverlay::new(vec![Arc::new(cell)], RuntimeKeymap::defaults().pager); + let area = Rect::new( + /*x*/ 0, /*y*/ 0, /*width*/ 240, /*height*/ 12, + ); + let mut buffer = Buffer::empty(area); + + overlay.render(area, &mut buffer); + let unavailable = buffer_to_text(&buffer, area.width); + assert!(unavailable.contains("Visualization unavailable on this device")); + + fs::write(context.thread_dir.join("chart.html"), "
chart
") + .expect("write visualization fragment"); + overlay.insert_cell(Arc::new(AgentMarkdownCell::new( + "next message".to_string(), + Path::new("/workspace"), + ))); + buffer = Buffer::empty(area); + overlay.render(area, &mut buffer); + + let available = buffer_to_text(&buffer, area.width); + assert!(available.contains("Open chart visualization in the browser")); + assert!( + available.contains("file://"), + "viewer URL was clipped: {available:?}" + ); + + let available = available + .lines() + .map(|line| { + line.find("file://").map_or_else( + || line.to_string(), + |start| format!("{}file://", &line[..start]), + ) + }) + .collect::>() + .join("\n"); + insta::assert_snapshot!( + "transcript_overlay_visualization_becomes_available", + format!("before:\n{unavailable}\n\nafter:\n{available}") + ); +} + #[test] fn agent_code_blocks_preserve_visualization_directive_literals() { let (_codex_home, context) = context_with_fragment("
chart
"); diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index cf263dd9a8..e3aee09268 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -393,14 +393,23 @@ impl Renderable for CachedRenderable { struct CellRenderable { cell: Arc, - style: Style, + highlighted: bool, } impl Renderable for CellRenderable { fn render(&self, area: Rect, buf: &mut Buffer) { let hyperlink_lines = self.cell.transcript_hyperlink_lines(area.width); + let style = if self.cell.as_any().is::() { + if self.highlighted { + user_message_style().reversed() + } else { + user_message_style() + } + } else { + Style::default() + }; let p = Paragraph::new(Text::from(visible_lines(hyperlink_lines.clone()))) - .style(self.style) + .style(style) .wrap(Wrap { trim: false }); p.render(area, buf); mark_buffer_hyperlinks(buf, area, &hyperlink_lines, /*scroll_rows*/ 0); @@ -488,40 +497,39 @@ impl TranscriptOverlay { cells .iter() .enumerate() - .flat_map(|(i, c)| { - let mut v: Vec> = Vec::new(); - let mut cell_renderable = if c.as_any().is::() { - Box::new(CachedRenderable::new(CellRenderable { - cell: c.clone(), - style: if highlight_cell == Some(i) { - user_message_style().reversed() - } else { - user_message_style() - }, - })) as Box - } else { - Box::new(CachedRenderable::new(CellRenderable { - cell: c.clone(), - style: Style::default(), - })) as Box - }; - if !c.is_stream_continuation() && i > 0 { - cell_renderable = Box::new(InsetRenderable::new( - cell_renderable, - Insets::tlbr( - /*top*/ 1, /*left*/ 0, /*bottom*/ 0, /*right*/ 0, - ), - )); - } - v.push(cell_renderable); - v - }) + .map(|(i, cell)| Self::render_cell(cell, i, highlight_cell)) .collect() } + /// Build the renderable for a committed cell, caching its height when the cell is stable. + fn render_cell( + cell: &Arc, + index: usize, + highlight_cell: Option, + ) -> Box { + let cell_renderable = CellRenderable { + cell: cell.clone(), + highlighted: highlight_cell == Some(index), + }; + let mut cell_renderable: Box = if cell.has_stable_transcript_height() { + Box::new(CachedRenderable::new(cell_renderable)) + } else { + Box::new(cell_renderable) + }; + if !cell.is_stream_continuation() && index > 0 { + cell_renderable = Box::new(InsetRenderable::new( + cell_renderable, + Insets::tlbr( + /*top*/ 1, /*left*/ 0, /*bottom*/ 0, /*right*/ 0, + ), + )); + } + cell_renderable + } + /// Insert a committed history cell while keeping any cached live tail. /// - /// The live tail is temporarily removed, the committed cells are rebuilt, + /// The live tail is temporarily removed, the new committed cell is appended, /// then the tail is reattached. If the tail previously had no leading /// spacing because it was the only renderable, we add the missing inset /// when the first committed cell arrives. @@ -533,8 +541,9 @@ impl TranscriptOverlay { let follow_bottom = self.view.is_scrolled_to_bottom(); let had_prior_cells = !self.cells.is_empty(); let tail_renderable = self.take_live_tail_renderable(); + let cell_renderable = Self::render_cell(&cell, self.cells.len(), self.highlight_cell); self.cells.push(cell); - self.view.renderables = Self::render_cells(&self.cells, self.highlight_cell); + self.view.renderables.push(cell_renderable); if let Some(tail) = tail_renderable { let tail = if !had_prior_cells && self @@ -940,6 +949,8 @@ mod tests { use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; use std::time::Duration; use crate::diff_model::FileChange; @@ -971,6 +982,26 @@ mod tests { } } + #[derive(Debug)] + struct HeightCountingCell { + height_calls: Arc, + } + + impl crate::history_cell::HistoryCell for HeightCountingCell { + fn display_lines(&self, _width: u16) -> Vec> { + vec![Line::from("counted")] + } + + fn raw_lines(&self) -> Vec> { + vec![Line::from("counted")] + } + + fn desired_transcript_height(&self, _width: u16) -> u16 { + self.height_calls.fetch_add(1, Ordering::Relaxed); + 1 + } + } + fn paragraph_block(label: &str, lines: usize) -> Box { let text = Text::from( (0..lines) @@ -1295,6 +1326,26 @@ mod tests { assert_eq!(overlay.view.scroll_offset, 0); } + #[test] + fn transcript_overlay_insert_preserves_cached_cell_heights() { + let height_calls = Arc::new(AtomicUsize::new(0)); + let mut overlay = transcript_overlay(vec![Arc::new(HeightCountingCell { + height_calls: height_calls.clone(), + })]); + let area = Rect::new(0, 0, 40, 12); + let mut buf = Buffer::empty(area); + + overlay.render(area, &mut buf); + assert_eq!(height_calls.load(Ordering::Relaxed), 1); + + overlay.insert_cell(Arc::new(TestCell { + lines: vec![Line::from("inserted")], + })); + overlay.render(area, &mut buf); + + assert_eq!(height_calls.load(Ordering::Relaxed), 1); + } + #[test] fn transcript_overlay_consolidation_remaps_highlight_inside_range() { let mut overlay = transcript_overlay( diff --git a/codex-rs/tui/src/snapshots/codex_tui__inline_visualization__tests__transcript_overlay_visualization_becomes_available.snap b/codex-rs/tui/src/snapshots/codex_tui__inline_visualization__tests__transcript_overlay_visualization_becomes_available.snap new file mode 100644 index 0000000000..fce2ba5a08 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__inline_visualization__tests__transcript_overlay_visualization_becomes_available.snap @@ -0,0 +1,30 @@ +--- +source: tui/src/inline_visualization_tests.rs +expression: "format!(\"before:\\n{unavailable}\\n\\nafter:\\n{available}\")" +--- +before: +/ T R A N S C R I P T / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / +• Visualization unavailable on this device. +~ +~ +~ +~ +~ +~ +───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 100% ─ + ↑/↓ to scroll pgup/pgdn to page home/end to jump + q to quit esc to edit prev + + +after: +/ T R A N S C R I P T / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / / +• Open chart visualization in the browser + file:// + +• next message +~ +~ +~ +───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 100% ─ + ↑/↓ to scroll pgup/pgdn to page home/end to jump + q to quit esc to edit prev diff --git a/codex-rs/tui/src/status/snapshots/codex_tui__status__tests__transcript_overlay_status_rate_limit_refresh.snap b/codex-rs/tui/src/status/snapshots/codex_tui__status__tests__transcript_overlay_status_rate_limit_refresh.snap new file mode 100644 index 0000000000..e2f12c8436 --- /dev/null +++ b/codex-rs/tui/src/status/snapshots/codex_tui__status__tests__transcript_overlay_status_rate_limit_refresh.snap @@ -0,0 +1,66 @@ +--- +source: tui/src/status/tests.rs +expression: "format!(\"before:\\n{before}\\n\\nafter:\\n{after}\")" +--- +before: +/ T R A N S C R I P T / / / / / / / / / / / / / / / / / / / / / / / / / / / / / +/status + +╭──────────────────────────────────────────────────────────────────────────╮ +│ >_ OpenAI Codex (v0.0.0) │ +│ │ +│ Visit https://chatgpt.com/codex/settings/usage for up-to-date │ +│ information on rate limits and credits │ +│ │ +│ Model: gpt-5.1-codex-max (reasoning none, summaries auto) │ +│ Directory: [[workspace]] │ +│ Permissions: Custom (workspace with network access, Ask for approval) │ +│ Agents.md: │ +│ │ +│ Token usage: 0 total (0 input + 0 output) │ +│ Limits: refresh requested; run /status again shortly. │ +╰──────────────────────────────────────────────────────────────────────────╯ +~ +~ +~ +~ +~ +~ +~ +~ +~ +───────────────────────────────────────────────────────────────────────── 100% ─ + ↑/↓ to scroll pgup/pgdn to page home/end to jump + q to quit esc to edit prev + + +after: +/ T R A N S C R I P T / / / / / / / / / / / / / / / / / / / / / / / / / / / / / +/status + +╭───────────────────────────────────────────────────────────────────────────╮ +│ >_ OpenAI Codex (v0.0.0) │ +│ │ +│ Visit https://chatgpt.com/codex/settings/usage for up-to-date │ +│ information on rate limits and credits │ +│ │ +│ Model: gpt-5.1-codex-max (reasoning none, summaries auto) │ +│ Directory: [[workspace]] │ +│ Permissions: Custom (workspace with network access, Ask for approval) │ +│ Agents.md: │ +│ │ +│ Token usage: 0 total (0 input + 0 output) │ +│ spark limit: │ +│ 5h limit: [███████████░░░░░░░░░] 55% left (resets soon) │ +│ Weekly limit: [██████████████░░░░░░] 70% left (resets later) │ +╰───────────────────────────────────────────────────────────────────────────╯ + +next message +~ +~ +~ +~ +~ +───────────────────────────────────────────────────────────────────────── 100% ─ + ↑/↓ to scroll pgup/pgdn to page home/end to jump + q to quit esc to edit prev diff --git a/codex-rs/tui/src/status/tests.rs b/codex-rs/tui/src/status/tests.rs index eedcfdfff2..6215899d2e 100644 --- a/codex-rs/tui/src/status/tests.rs +++ b/codex-rs/tui/src/status/tests.rs @@ -8,9 +8,12 @@ use super::rate_limits::SpendControlLimitSnapshotDisplay; use super::rate_limits::StatusRateLimitData; use super::rate_limits::compose_rate_limit_data_many; use crate::history_cell::HistoryCell; +use crate::history_cell::PlainHistoryCell; +use crate::keymap::RuntimeKeymap; use crate::legacy_core::config::Config; use crate::legacy_core::config::ConfigBuilder; use crate::legacy_core::config::PermissionProfileSnapshot; +use crate::pager_overlay::TranscriptOverlay; use crate::status::StatusAccountDisplay; use crate::status::remote_connection::RemoteConnectionStatus; use crate::test_support::PathBufExt; @@ -52,6 +55,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use insta::assert_snapshot; use pretty_assertions::assert_eq; use ratatui::prelude::*; +use std::sync::Arc; use tempfile::TempDir; use unicode_width::UnicodeWidthStr; @@ -206,6 +210,28 @@ fn sanitize_directory(lines: Vec) -> Vec { .collect() } +fn buffer_to_text(buffer: &Buffer, width: u16) -> String { + let lines = buffer + .content + .chunks(usize::from(width)) + .map(|row| { + row.iter() + .map(|cell| { + let symbol = cell.symbol(); + symbol + .strip_prefix("\x1b]8;;") + .and_then(|symbol| symbol.split_once('\x07')) + .and_then(|(_, symbol)| symbol.strip_suffix("\x1b]8;;\x07")) + .unwrap_or(symbol) + }) + .collect::() + .trim_end() + .to_string() + }) + .collect::>(); + sanitize_directory(lines).join("\n") +} + fn reset_at_from(captured_at: &chrono::DateTime, seconds: i64) -> i64 { (*captured_at + ChronoDuration::seconds(seconds)) .with_timezone(&Utc) @@ -1666,6 +1692,89 @@ async fn status_snapshot_shows_refreshing_limits_notice() { assert_snapshot!(sanitized); } +#[tokio::test] +async fn transcript_overlay_remeasures_status_after_rate_limit_refresh() { + let temp_home = TempDir::new().expect("temp home"); + let mut config = test_config(&temp_home).await; + config.model = Some("gpt-5.1-codex-max".to_string()); + set_workspace_cwd(&mut config, test_path_buf("/workspace/tests").abs()); + let usage = TokenUsage::default(); + let now = Local + .with_ymd_and_hms(2024, 6, 7, 8, 9, 10) + .single() + .expect("timestamp"); + let model_slug = get_model_offline_for_tests(config.model.as_deref()); + + let (status, handle) = new_status_output_with_rate_limits_handle( + &config, + /*runtime_model_provider_base_url*/ None, + /*remote_connection*/ None, + /*account_display*/ None, + /*token_info*/ None, + &usage, + &None, + /*thread_name*/ None, + /*forked_from*/ None, + /*rate_limits*/ &[], + None, + now, + &model_slug, + /*collaboration_mode*/ None, + /*reasoning_effort_override*/ None, + "".to_string(), + /*refreshing_rate_limits*/ true, + ); + let mut overlay = + TranscriptOverlay::new(vec![Arc::new(status)], RuntimeKeymap::defaults().pager); + let area = Rect::new( + /*x*/ 0, /*y*/ 0, /*width*/ 80, /*height*/ 30, + ); + let mut buffer = Buffer::empty(area); + overlay.render(area, &mut buffer); + let before = buffer_to_text(&buffer, area.width); + + handle.finish_rate_limit_refresh( + &[RateLimitSnapshotDisplay { + limit_name: "spark".to_string(), + captured_at: now, + primary: Some(RateLimitWindowDisplay { + used_percent: 45.0, + resets_at: Some("soon".to_string()), + window_minutes: Some(300), + }), + secondary: Some(RateLimitWindowDisplay { + used_percent: 30.0, + resets_at: Some("later".to_string()), + window_minutes: Some(10_080), + }), + credits: None, + individual_limit: None, + }], + now, + ); + overlay.insert_cell(Arc::new(PlainHistoryCell::new(vec!["next message".into()]))); + buffer = Buffer::empty(area); + overlay.render(area, &mut buffer); + let after = buffer_to_text(&buffer, area.width); + + assert!( + after.contains("spark limit"), + "status output was clipped: {after:?}" + ); + assert!( + after.contains("5h limit"), + "status output was clipped: {after:?}" + ); + assert!( + after.contains("Weekly limit"), + "status output was clipped: {after:?}" + ); + insta::assert_snapshot!( + "transcript_overlay_status_rate_limit_refresh", + format!("before:\n{before}\n\nafter:\n{after}") + ); +} + #[tokio::test] async fn status_snapshot_includes_credits_and_limits() { let temp_home = TempDir::new().expect("temp home");