valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "refactor transcript view to handle HistoryCells" (#3614)

Browse Source 
Reverts openai/codex#3538
It panics on forking first message. It also calculates the index in a
wrong way.
This commit is contained in:
Ahmed Ibrahim
2025-09-14 23:39:36 -04:00
committed by GitHub
parent 6581da9b57
commit 26f1246a89
10 changed files with 391 additions and 311 deletions
Download Patch File Download Diff File Expand all files Collapse all files

302
codex-rs/tui/src/pager_overlay.rs
View File

@@ -1,8 +1,6 @@
use std::io::Result;
use std::sync::Arc;
use std::time::Duration;
use crate::history_cell::HistoryCell;
use crate::render::line_utils::push_owned_lines;
use crate::tui;
use crate::tui::TuiEvent;
@@ -17,7 +15,6 @@ use ratatui::style::Styled;
use ratatui::style::Stylize;
use ratatui::text::Line;
use ratatui::text::Span;
use ratatui::text::Text;
use ratatui::widgets::Paragraph;
use ratatui::widgets::WidgetRef;
@@ -27,8 +24,8 @@ pub(crate) enum Overlay {
}
impl Overlay {
pub(crate) fn new_transcript(cells: Vec<Arc<dyn HistoryCell>>) -> Self {
Self::Transcript(TranscriptOverlay::new(cells))
pub(crate) fn new_transcript(lines: Vec<Line<'static>>) -> Self {
Self::Transcript(TranscriptOverlay::new(lines))
}
pub(crate) fn new_static_with_title(lines: Vec<Line<'static>>, title: String) -> Self {
@@ -76,24 +73,21 @@ fn render_key_hints(area: Rect, buf: &mut Buffer, pairs: &[(&str, &str)]) {
/// Generic widget for rendering a pager view.
struct PagerView {
texts: Vec<Text<'static>>,
lines: Vec<Line<'static>>,
scroll_offset: usize,
title: String,
wrap_cache: Option<WrapCache>,
last_content_height: Option<usize>,
/// If set, on next render ensure this chunk is visible.
pending_scroll_chunk: Option<usize>,
}
impl PagerView {
fn new(texts: Vec<Text<'static>>, title: String, scroll_offset: usize) -> Self {
fn new(lines: Vec<Line<'static>>, title: String, scroll_offset: usize) -> Self {
Self {
texts,
lines,
scroll_offset,
title,
wrap_cache: None,
last_content_height: None,
pending_scroll_chunk: None,
}
}
@@ -102,14 +96,6 @@ impl PagerView {
let content_area = self.scroll_area(area);
self.update_last_content_height(content_area.height);
self.ensure_wrapped(content_area.width);
// If there is a pending request to scroll a specific chunk into view,
// satisfy it now that wrapping is up to date for this width.
if let (Some(idx), Some(cache)) =
(self.pending_scroll_chunk.take(), self.wrap_cache.as_ref())
&& let Some(range) = cache.chunk_ranges.get(idx).cloned()
{
self.ensure_range_visible(range, content_area.height as usize, cache.wrapped.len());
}
// Compute page bounds without holding an immutable borrow on cache while mutating self
let wrapped_len = self
.wrap_cache
@@ -122,12 +108,40 @@ impl PagerView {
let start = self.scroll_offset;
let end = (start + content_area.height as usize).min(wrapped_len);
let wrapped = self.cached();
let (wrapped, _src_idx) = self.cached();
let page = &wrapped[start..end];
self.render_content_page_prepared(content_area, buf, page);
self.render_bottom_bar(area, content_area, buf, wrapped);
}
fn render_with_highlight(
&mut self,
area: Rect,
buf: &mut Buffer,
highlight: Option<(usize, usize)>,
) {
self.render_header(area, buf);
let content_area = self.scroll_area(area);
self.update_last_content_height(content_area.height);
self.ensure_wrapped(content_area.width);
// Compute page bounds first to avoid borrow conflicts
let wrapped_len = self
.wrap_cache
.as_ref()
.map(|c| c.wrapped.len())
.unwrap_or(0);
self.scroll_offset = self
.scroll_offset
.min(wrapped_len.saturating_sub(content_area.height as usize));
let start = self.scroll_offset;
let end = (start + content_area.height as usize).min(wrapped_len);
let (wrapped, src_idx) = self.cached();
let page = self.page_with_optional_highlight(wrapped, src_idx, start, end, highlight);
self.render_content_page_prepared(content_area, buf, &page);
self.render_bottom_bar(area, content_area, buf, wrapped);
}
fn render_header(&self, area: Rect, buf: &mut Buffer) {
Span::from("/ ".repeat(area.width as usize / 2))
.dim()
@@ -256,8 +270,7 @@ impl PagerView {
struct WrapCache {
width: u16,
wrapped: Vec<Line<'static>>,
/// For each input Text chunk, the inclusive-excluded range of wrapped lines produced.
chunk_ranges: Vec<std::ops::Range<usize>>,
src_idx: Vec<usize>,
base_len: usize,
}
@@ -265,39 +278,74 @@ impl PagerView {
fn ensure_wrapped(&mut self, width: u16) {
let width = width.max(1);
let needs = match self.wrap_cache {
Some(ref c) => c.width != width || c.base_len != self.texts.len(),
Some(ref c) => c.width != width || c.base_len != self.lines.len(),
None => true,
};
if !needs {
return;
}
let mut wrapped: Vec<Line<'static>> = Vec::new();
let mut chunk_ranges: Vec<std::ops::Range<usize>> = Vec::with_capacity(self.texts.len());
for text in &self.texts {
let start = wrapped.len();
for line in &text.lines {
let ws = crate::wrapping::word_wrap_line(line, width as usize);
push_owned_lines(&ws, &mut wrapped);
}
let end = wrapped.len();
chunk_ranges.push(start..end);
let mut src_idx: Vec<usize> = Vec::new();
for (i, line) in self.lines.iter().enumerate() {
let ws = crate::wrapping::word_wrap_line(line, width as usize);
src_idx.extend(std::iter::repeat_n(i, ws.len()));
push_owned_lines(&ws, &mut wrapped);
}
self.wrap_cache = Some(WrapCache {
width,
wrapped,
chunk_ranges,
base_len: self.texts.len(),
src_idx,
base_len: self.lines.len(),
});
}
fn cached(&self) -> &[Line<'static>] {
fn cached(&self) -> (&[Line<'static>], &[usize]) {
if let Some(cache) = self.wrap_cache.as_ref() {
&cache.wrapped
(&cache.wrapped, &cache.src_idx)
} else {
&[]
(&[], &[])
}
}
fn page_with_optional_highlight<'a>(
&self,
wrapped: &'a [Line<'static>],
src_idx: &[usize],
start: usize,
end: usize,
highlight: Option<(usize, usize)>,
) -> std::borrow::Cow<'a, [Line<'static>]> {
use ratatui::style::Modifier;
let (hi_start, hi_end) = match highlight {
Some(r) => r,
None => return std::borrow::Cow::Borrowed(&wrapped[start..end]),
};
let mut out: Vec<Line<'static>> = Vec::with_capacity(end - start);
let mut bold_done = false;
for (row, src_line) in wrapped
.iter()
.enumerate()
.skip(start)
.take(end.saturating_sub(start))
{
let mut line = src_line.clone();
if let Some(src) = src_idx.get(row).copied()
&& src >= hi_start
&& src < hi_end
{
for (i, s) in line.spans.iter_mut().enumerate() {
s.style.add_modifier |= Modifier::REVERSED;
if !bold_done && i == 0 {
s.style.add_modifier |= Modifier::BOLD;
bold_done = true;
}
}
}
out.push(line);
}
std::borrow::Cow::Owned(out)
}
fn is_scrolled_to_bottom(&self) -> bool {
if self.scroll_offset == usize::MAX {
return true;
@@ -315,108 +363,38 @@ impl PagerView {
let max_scroll = cache.wrapped.len().saturating_sub(visible);
self.scroll_offset >= max_scroll
}
/// Request that the given text chunk index be scrolled into view on next render.
fn scroll_chunk_into_view(&mut self, chunk_index: usize) {
self.pending_scroll_chunk = Some(chunk_index);
}
fn ensure_range_visible(
&mut self,
range: std::ops::Range<usize>,
viewport_height: usize,
total_wrapped: usize,
) {
if viewport_height == 0 || total_wrapped == 0 {
return;
}
let first = range.start.min(total_wrapped.saturating_sub(1));
let last = range
.end
.saturating_sub(1)
.min(total_wrapped.saturating_sub(1));
let current_top = self.scroll_offset.min(total_wrapped.saturating_sub(1));
let current_bottom = current_top.saturating_add(viewport_height.saturating_sub(1));
if first < current_top {
self.scroll_offset = first;
} else if last > current_bottom {
// Scroll just enough so that 'last' is visible at the bottom
self.scroll_offset = last.saturating_sub(viewport_height.saturating_sub(1));
}
}
}
pub(crate) struct TranscriptOverlay {
view: PagerView,
cells: Vec<Arc<dyn HistoryCell>>,
highlight_cell: Option<usize>,
highlight_range: Option<(usize, usize)>,
is_done: bool,
}
impl TranscriptOverlay {
pub(crate) fn new(transcript_cells: Vec<Arc<dyn HistoryCell>>) -> Self {
pub(crate) fn new(transcript_lines: Vec<Line<'static>>) -> Self {
Self {
view: PagerView::new(
Self::render_cells_to_texts(&transcript_cells, None),
transcript_lines,
"T R A N S C R I P T".to_string(),
usize::MAX,
),
cells: transcript_cells,
highlight_cell: None,
highlight_range: None,
is_done: false,
}
}
fn render_cells_to_texts(
cells: &[Arc<dyn HistoryCell>],
highlight_cell: Option<usize>,
) -> Vec<Text<'static>> {
let mut texts: Vec<Text<'static>> = Vec::new();
let mut first = true;
for (idx, cell) in cells.iter().enumerate() {
let mut lines: Vec<Line<'static>> = Vec::new();
if !cell.is_stream_continuation() && !first {
lines.push(Line::from(""));
}
let cell_lines = if Some(idx) == highlight_cell {
cell.transcript_lines()
.into_iter()
.map(|l| l.reversed())
.collect()
} else {
cell.transcript_lines()
};
lines.extend(cell_lines);
texts.push(Text::from(lines));
first = false;
}
texts
}
pub(crate) fn insert_cell(&mut self, cell: Arc<dyn HistoryCell>) {
pub(crate) fn insert_lines(&mut self, lines: Vec<Line<'static>>) {
let follow_bottom = self.view.is_scrolled_to_bottom();
// Append as a new Text chunk (with a separating blank if needed)
let mut lines: Vec<Line<'static>> = Vec::new();
if !cell.is_stream_continuation() && !self.cells.is_empty() {
lines.push(Line::from(""));
}
lines.extend(cell.transcript_lines());
self.view.texts.push(Text::from(lines));
self.cells.push(cell);
self.view.lines.extend(lines);
self.view.wrap_cache = None;
if follow_bottom {
self.view.scroll_offset = usize::MAX;
}
}
pub(crate) fn set_highlight_cell(&mut self, cell: Option<usize>) {
self.highlight_cell = cell;
self.view.wrap_cache = None;
self.view.texts = Self::render_cells_to_texts(&self.cells, self.highlight_cell);
if let Some(idx) = self.highlight_cell {
self.view.scroll_chunk_into_view(idx);
}
pub(crate) fn set_highlight_range(&mut self, range: Option<(usize, usize)>) {
self.highlight_range = range;
}
fn render_hints(&self, area: Rect, buf: &mut Buffer) {
@@ -424,7 +402,9 @@ impl TranscriptOverlay {
let line2 = Rect::new(area.x, area.y.saturating_add(1), area.width, 1);
render_key_hints(line1, buf, PAGER_KEY_HINTS);
let mut pairs: Vec<(&str, &str)> = vec![("q", "quit"), ("Esc", "edit prev")];
if self.highlight_cell.is_some() {
if let Some((start, end)) = self.highlight_range
&& end > start
{
pairs.push(("⏎", "edit message"));
}
render_key_hints(line2, buf, &pairs);
@@ -434,7 +414,8 @@ impl TranscriptOverlay {
let top_h = area.height.saturating_sub(3);
let top = Rect::new(area.x, area.y, area.width, top_h);
let bottom = Rect::new(area.x, area.y + top_h, area.width, 3);
self.view.render(top, buf);
self.view
.render_with_highlight(top, buf, self.highlight_range);
self.render_hints(bottom, buf);
}
}
@@ -477,6 +458,9 @@ impl TranscriptOverlay {
pub(crate) fn is_done(&self) -> bool {
self.is_done
}
pub(crate) fn set_scroll_offset(&mut self, offset: usize) {
self.view.scroll_offset = offset;
}
}
pub(crate) struct StaticOverlay {
@@ -487,7 +471,7 @@ pub(crate) struct StaticOverlay {
impl StaticOverlay {
pub(crate) fn with_title(lines: Vec<Line<'static>>, title: String) -> Self {
Self {
view: PagerView::new(vec![Text::from(lines)], title, 0),
view: PagerView::new(lines, title, 0),
is_done: false,
}
}
@@ -550,26 +534,9 @@ mod tests {
use ratatui::Terminal;
use ratatui::backend::TestBackend;
#[derive(Debug)]
struct TestCell {
lines: Vec<Line<'static>>,
}
impl crate::history_cell::HistoryCell for TestCell {
fn display_lines(&self, _width: u16) -> Vec<Line<'static>> {
self.lines.clone()
}
fn transcript_lines(&self) -> Vec<Line<'static>> {
self.lines.clone()
}
}
#[test]
fn edit_prev_hint_is_visible() {
let mut overlay = TranscriptOverlay::new(vec![Arc::new(TestCell {
lines: vec![Line::from("hello")],
})]);
let mut overlay = TranscriptOverlay::new(vec![Line::from("hello")]);
// Render into a small buffer and assert the backtrack hint is present
let area = Rect::new(0, 0, 40, 10);
@@ -594,15 +561,9 @@ mod tests {
fn transcript_overlay_snapshot_basic() {
// Prepare a transcript overlay with a few lines
let mut overlay = TranscriptOverlay::new(vec![
Arc::new(TestCell {
lines: vec![Line::from("alpha")],
}),
Arc::new(TestCell {
lines: vec![Line::from("beta")],
}),
Arc::new(TestCell {
lines: vec![Line::from("gamma")],
}),
Line::from("alpha"),
Line::from("beta"),
Line::from("gamma"),
]);
let mut term = Terminal::new(TestBackend::new(40, 10)).expect("term");
term.draw(|f| overlay.render(f.area(), f.buffer_mut()))
@@ -612,15 +573,8 @@ mod tests {
#[test]
fn transcript_overlay_keeps_scroll_pinned_at_bottom() {
let mut overlay = TranscriptOverlay::new(
(0..20)
.map(|i| {
Arc::new(TestCell {
lines: vec![Line::from(format!("line{i}"))],
}) as Arc<dyn HistoryCell>
})
.collect(),
);
let mut overlay =
TranscriptOverlay::new((0..20).map(|i| Line::from(format!("line{i}"))).collect());
let mut term = Terminal::new(TestBackend::new(40, 12)).expect("term");
term.draw(|f| overlay.render(f.area(), f.buffer_mut()))
.expect("draw");
@@ -630,33 +584,22 @@ mod tests {
"expected initial render to leave view at bottom"
);
overlay.insert_cell(Arc::new(TestCell {
lines: vec!["tail".into()],
}));
overlay.insert_lines(vec!["tail".into()]);
assert_eq!(overlay.view.scroll_offset, usize::MAX);
}
#[test]
fn transcript_overlay_preserves_manual_scroll_position() {
let mut overlay = TranscriptOverlay::new(
(0..20)
.map(|i| {
Arc::new(TestCell {
lines: vec![Line::from(format!("line{i}"))],
}) as Arc<dyn HistoryCell>
})
.collect(),
);
let mut overlay =
TranscriptOverlay::new((0..20).map(|i| Line::from(format!("line{i}"))).collect());
let mut term = Terminal::new(TestBackend::new(40, 12)).expect("term");
term.draw(|f| overlay.render(f.area(), f.buffer_mut()))
.expect("draw");
overlay.view.scroll_offset = 0;
overlay.insert_cell(Arc::new(TestCell {
lines: vec!["tail".into()],
}));
overlay.insert_lines(vec!["tail".into()]);
assert_eq!(overlay.view.scroll_offset, 0);
}
@@ -677,21 +620,17 @@ mod tests {
#[test]
fn pager_wrap_cache_reuses_for_same_width_and_rebuilds_on_change() {
let long = "This is a long line that should wrap multiple times to ensure non-empty wrapped output.";
let mut pv = PagerView::new(
vec![Text::from(vec![long.into()]), Text::from(vec![long.into()])],
"T".to_string(),
0,
);
let mut pv = PagerView::new(vec![long.into(), long.into()], "T".to_string(), 0);
// Build cache at width 24
pv.ensure_wrapped(24);
let w1 = pv.cached();
let (w1, _) = pv.cached();
assert!(!w1.is_empty(), "expected wrapped output to be non-empty");
let ptr1 = w1.as_ptr();
// Re-run with same width: cache should be reused (pointer stability heuristic)
pv.ensure_wrapped(24);
let w2 = pv.cached();
let (w2, _) = pv.cached();
let ptr2 = w2.as_ptr();
assert_eq!(ptr1, ptr2, "cache should not rebuild for unchanged width");
@@ -699,7 +638,7 @@ mod tests {
// Drop immutable borrow before mutating
let prev_len = w2.len();
pv.ensure_wrapped(36);
let w3 = pv.cached();
let (w3, _) = pv.cached();
assert_ne!(
prev_len,
w3.len(),
@@ -710,16 +649,15 @@ mod tests {
#[test]
fn pager_wrap_cache_invalidates_on_append() {
let long = "Another long line for wrapping behavior verification.";
let mut pv = PagerView::new(vec![Text::from(vec![long.into()])], "T".to_string(), 0);
let mut pv = PagerView::new(vec![long.into()], "T".to_string(), 0);
pv.ensure_wrapped(28);
let w1 = pv.cached();
let (w1, _) = pv.cached();
let len1 = w1.len();
// Append new lines should cause ensure_wrapped to rebuild due to len change
pv.texts.push(Text::from(vec![long.into()]));
pv.texts.push(Text::from(vec![long.into()]));
pv.lines.extend([long.into(), long.into()]);
pv.ensure_wrapped(28);
let w2 = pv.cached();
let (w2, _) = pv.cached();
assert!(
w2.len() >= len1,
"wrapped length should grow or stay same after append"