fix: properly wrap lines in the Rust TUI (#937)

As discussed on
699ec5a#commitcomment-156776835,
to properly support scrolling long content in Ratatui for a sequence of
cells, we need to:

* take the `Vec<Line>` for each cell
* using the wrapping logic we want to use at render time, compute the
_effective line count_ using `Paragraph::line_count()` (see
`wrapped_line_count_for_cell()` in this PR)
* sum up the effective line count to compute the height of the area
being scrolled
* given a `scroll_position: usize`, index into the list of "effective
lines" and accumulate the appropriate `Vec<Line>` for the cells that
should be displayed
* take that `Vec<Line>` to create a `Paragraph` and use the same
line-wrapping policy that was used in `wrapped_line_count_for_cell()`
* display the resulting `Paragraph` and use the accounting to display a
scrollbar with the appropriate thumb size and offset without having to
render the `Vec<Line>` for the full history

With this change, lines wrap as I expect and everything appears to
redraw correctly as I resize my terminal!

Author: bolinfest

codex-rs/tui/src/conversation_history_widget.rs

@@ -11,11 +11,21 @@ use ratatui::style::Style;
 use ratatui::widgets::*;
 use serde_json::Value as JsonValue;
 use std::cell::Cell as StdCell;
+use std::cell::Cell;
 use std::collections::HashMap;
 use std::path::PathBuf;
 
+/// A single history entry plus its cached wrapped-line count.
+struct Entry {
+    cell: HistoryCell,
+    line_count: Cell<usize>,
+}
+
 pub struct ConversationHistoryWidget {
-    history: Vec<HistoryCell>,
+    entries: Vec<Entry>,
+    /// The width (in terminal cells/columns) that [`Entry::line_count`] was
+    /// computed for. When the available width changes we recompute counts.
+    cached_width: StdCell<u16>,
     scroll_position: usize,
     /// Number of lines the last time render_ref() was called
     num_rendered_lines: StdCell<usize>,
@@ -27,7 +37,8 @@ pub struct ConversationHistoryWidget {
 impl ConversationHistoryWidget {
     pub fn new() -> Self {
         Self {
-            history: Vec::new(),
+            entries: Vec::new(),
+            cached_width: StdCell::new(0),
             scroll_position: usize::MAX,
             num_rendered_lines: StdCell::new(0),
             last_viewport_height: StdCell::new(0),
@@ -73,7 +84,7 @@ impl ConversationHistoryWidget {
 
     fn scroll_up(&mut self, num_lines: u32) {
         // If a user is scrolling up from the "stick to bottom" mode, we need to
-        // map this to a specific scroll position so we can caluate the delta.
+        // map this to a specific scroll position so we can calculate the delta.
         // This requires us to care about how tall the screen is.
         if self.scroll_position == usize::MAX {
             self.scroll_position = self
@@ -97,9 +108,7 @@ impl ConversationHistoryWidget {
         // Compute the maximum explicit scroll offset that still shows a full
         // viewport. This mirrors the calculation in `scroll_page_down()` and
         // in the render path.
-        let max_scroll = num_rendered_lines
-            .saturating_sub(viewport_height)
-            .saturating_add(1);
+        let max_scroll = num_rendered_lines.saturating_sub(viewport_height);
 
         let new_pos = self.scroll_position.saturating_add(num_lines as usize);
 
@@ -144,7 +153,7 @@ impl ConversationHistoryWidget {
         // Calculate the maximum explicit scroll offset that is still within
         // range. This matches the logic in `scroll_down()` and the render
         // method.
-        let max_scroll = num_lines.saturating_sub(viewport_height).saturating_add(1);
+        let max_scroll = num_lines.saturating_sub(viewport_height);
 
         // Attempt to move down by a full page.
         let new_pos = self.scroll_position.saturating_add(viewport_height);
@@ -166,7 +175,7 @@ impl ConversationHistoryWidget {
     /// Note `model` could differ from `config.model` if the agent decided to
     /// use a different model than the one requested by the user.
     pub fn add_session_info(&mut self, config: &Config, event: SessionConfiguredEvent) {
-        let is_first_event = self.history.is_empty();
+        let is_first_event = self.entries.is_empty();
         self.add_to_history(HistoryCell::new_session_info(config, event, is_first_event));
     }
 
@@ -216,12 +225,22 @@ impl ConversationHistoryWidget {
     }
 
     fn add_to_history(&mut self, cell: HistoryCell) {
-        self.history.push(cell);
+        let width = self.cached_width.get();
+        let count = if width > 0 {
+            wrapped_line_count_for_cell(&cell, width)
+        } else {
+            0
+        };
+
+        self.entries.push(Entry {
+            cell,
+            line_count: Cell::new(count),
+        });
     }
 
     /// Remove all history entries and reset scrolling.
     pub fn clear(&mut self) {
-        self.history.clear();
+        self.entries.clear();
         self.scroll_position = usize::MAX;
     }
 
@@ -232,7 +251,9 @@ impl ConversationHistoryWidget {
         stderr: String,
         exit_code: i32,
     ) {
-        for cell in self.history.iter_mut() {
+        let width = self.cached_width.get();
+        for entry in self.entries.iter_mut() {
+            let cell = &mut entry.cell;
             if let HistoryCell::ActiveExecCommand {
                 call_id: history_id,
                 command,
@@ -250,6 +271,13 @@ impl ConversationHistoryWidget {
                             duration: start.elapsed(),
                         },
                     );
+
+                    // Update cached line count.
+                    if width > 0 {
+                        entry
+                            .line_count
+                            .set(wrapped_line_count_for_cell(cell, width));
+                    }
                     break;
                 }
             }
@@ -269,14 +297,15 @@ impl ConversationHistoryWidget {
                 .unwrap_or_else(|_| serde_json::Value::String("<serialization error>".into()))
         });
 
-        for cell in self.history.iter_mut() {
+        let width = self.cached_width.get();
+        for entry in self.entries.iter_mut() {
             if let HistoryCell::ActiveMcpToolCall {
                 call_id: history_id,
                 fq_tool_name,
                 invocation,
                 start,
                 ..
-            } = cell
+            } = &entry.cell
             {
                 if &call_id == history_id {
                     let completed = HistoryCell::new_completed_mcp_tool_call(
@@ -286,7 +315,14 @@ impl ConversationHistoryWidget {
                         success,
                         result_val,
                     );
-                    *cell = completed;
+                    entry.cell = completed;
+
+                    if width > 0 {
+                        entry
+                            .line_count
+                            .set(wrapped_line_count_for_cell(&entry.cell, width));
+                    }
+
                     break;
                 }
             }
@@ -311,105 +347,102 @@ impl WidgetRef for ConversationHistoryWidget {
             .border_type(BorderType::Rounded)
             .border_style(border_style);
 
-        // ------------------------------------------------------------------
-        // Build a *window* into the history instead of cloning the entire
-        // history into a brand‑new Vec every time we are asked to render.
-        //
-        // There can be an unbounded number of `Line` objects in the history,
-        // but the terminal will only ever display `height` of them at once.
-        // By materialising only the `height` lines that are scrolled into
-        // view we avoid the potentially expensive clone of the full
-        // conversation every frame.
-        // ------------------------------------------------------------------
-
         // Compute the inner area that will be available for the list after
         // the surrounding `Block` is drawn.
         let inner = block.inner(area);
         let viewport_height = inner.height as usize;
 
-        // Collect the lines that will actually be visible in the viewport
-        // while keeping track of the total number of lines so the scrollbar
-        // stays correct.
-        let num_lines: usize = self.history.iter().map(|c| c.lines().len()).sum();
+        // Cache (and if necessary recalculate) the wrapped line counts for
+        // every [`HistoryCell`] so that our scrolling math accounts for text
+        // wrapping.
+        let width = inner.width; // Width of the viewport in terminal cells.
+        if width == 0 {
+            return; // Nothing to draw – avoid division by zero.
+        }
+
+        // Recompute cache if the width changed.
+        let num_lines: usize = if self.cached_width.get() != width {
+            self.cached_width.set(width);
 
-        let max_scroll = num_lines.saturating_sub(viewport_height) + 1;
+            let mut num_lines: usize = 0;
+            for entry in &self.entries {
+                let count = wrapped_line_count_for_cell(&entry.cell, width);
+                num_lines += count;
+                entry.line_count.set(count);
+            }
+            num_lines
+        } else {
+            self.entries.iter().map(|e| e.line_count.get()).sum()
+        };
+
+        // Determine the scroll position. Note the existing value of
+        // `self.scroll_position` could exceed the maximum scroll offset if the
+        // user made the window wider since the last render.
+        let max_scroll = num_lines.saturating_sub(viewport_height);
         let scroll_pos = if self.scroll_position == usize::MAX {
             max_scroll
         } else {
             self.scroll_position.min(max_scroll)
         };
 
-        let mut visible_lines: Vec<Line<'static>> = Vec::with_capacity(viewport_height);
+        // ------------------------------------------------------------------
+        // Build a *window* into the history so we only clone the `Line`s that
+        // may actually be visible in this frame. We still hand the slice off
+        // to a `Paragraph` with an additional scroll offset to avoid slicing
+        // inside a wrapped line (we don’t have per-subline granularity).
+        // ------------------------------------------------------------------
 
-        if self.scroll_position == usize::MAX {
-            // Stick‑to‑bottom mode: walk the history backwards and keep the
-            // most recent `height` lines.  This touches at most `height`
-            // lines regardless of how large the conversation grows.
-            'outer_rev: for cell in self.history.iter().rev() {
-                for line in cell.lines().iter().rev() {
-                    visible_lines.push(line.clone());
-                    if visible_lines.len() == viewport_height {
-                        break 'outer_rev;
-                    }
-                }
+        // Find the first entry that intersects the current scroll position.
+        let mut cumulative = 0usize;
+        let mut first_idx = 0usize;
+        for (idx, entry) in self.entries.iter().enumerate() {
+            let next = cumulative + entry.line_count.get();
+            if next > scroll_pos {
+                first_idx = idx;
+                break;
             }
-            visible_lines.reverse();
-        } else {
-            // Arbitrary scroll position.  Skip lines until we reach the
-            // desired offset, then emit the next `height` lines.
-            let start_line = scroll_pos;
-            let mut current_index = 0usize;
-            'outer_fwd: for cell in &self.history {
-                for line in cell.lines() {
-                    if current_index >= start_line {
-                        visible_lines.push(line.clone());
-                        if visible_lines.len() == viewport_height {
-                            break 'outer_fwd;
-                        }
-                    }
-                    current_index += 1;
-                }
+            cumulative = next;
+        }
+
+        let offset_into_first = scroll_pos - cumulative;
+
+        // Collect enough raw lines from `first_idx` onward to cover the
+        // viewport. We may fetch *slightly* more than necessary (whole cells)
+        // but never the entire history.
+        let mut collected_wrapped = 0usize;
+        let mut visible_lines: Vec<Line<'static>> = Vec::new();
+
+        for entry in &self.entries[first_idx..] {
+            visible_lines.extend(entry.cell.lines().iter().cloned());
+            collected_wrapped += entry.line_count.get();
+            if collected_wrapped >= offset_into_first + viewport_height {
+                break;
             }
         }
 
-        // We track the number of lines in the struct so can let the user take over from
-        // something other than usize::MAX when they start scrolling up. This could be
-        // removed once we have the vec<Lines> in self.
-        self.num_rendered_lines.set(num_lines);
-        self.last_viewport_height.set(viewport_height);
+        // Build the Paragraph with wrapping enabled so long lines are not
+        // clipped. Apply vertical scroll so that `offset_into_first` wrapped
+        // lines are hidden at the top.
+        let paragraph = Paragraph::new(visible_lines)
+            .block(block)
+            .wrap(wrap_cfg())
+            .scroll((offset_into_first as u16, 0));
 
-        // The widget takes care of drawing the `block` and computing its own
-        // inner area, so we render it over the full `area`.
-        // We *manually* sliced the set of `visible_lines` to fit within the
-        // viewport above, so there is no need to ask the `Paragraph` widget
-        // to apply an additional scroll offset. Doing so would cause the
-        // content to be shifted *twice* – once by our own logic and then a
-        // second time by the widget – which manifested as the entire block
-        // drifting off‑screen when the user attempted to scroll.
-
-        // Currently, we do not use the `wrap` method on the `Paragraph` widget
-        // because it messes up our scrolling math above that assumes each Line
-        // contributes one line of height to the widget. Admittedly, this is
-        // bad because users cannot see content that is clipped without
-        // resizing the terminal.
-        let paragraph = Paragraph::new(visible_lines).block(block);
         paragraph.render(area, buf);
 
+        // Draw scrollbar if necessary.
         let needs_scrollbar = num_lines > viewport_height;
         if needs_scrollbar {
             let mut scroll_state = ScrollbarState::default()
-                // TODO(ragona):
-                // I don't totally understand this, but it appears to work exactly as expected
-                // if we set the content length as the lines minus the height. Maybe I was supposed
-                // to use viewport_content_length or something, but this works and I'm backing away.
+                // The Scrollbar widget expects the *content* height minus the
+                // viewport height, mirroring the calculation used previously.
                 .content_length(num_lines.saturating_sub(viewport_height))
                 .position(scroll_pos);
 
-            // Choose a thumb colour that stands out only when this pane has focus so that the
+            // Choose a thumb color that stands out only when this pane has focus so that the
             // user’s attention is naturally drawn to the active viewport. When unfocused we show
             // a low‑contrast thumb so the scrollbar fades into the background without becoming
             // invisible.
-
             let thumb_style = if self.has_input_focus {
                 Style::reset().fg(Color::LightYellow)
             } else {
@@ -418,25 +451,25 @@ impl WidgetRef for ConversationHistoryWidget {
 
             StatefulWidget::render(
                 // By default the Scrollbar widget inherits the style that was already present
-                // in the underlying buffer cells.  That means if a coloured line (for example a
+                // in the underlying buffer cells. That means if a colored line (for example a
                 // background task notification that we render in blue) happens to be underneath
-                // the scrollbar, the track and thumb adopt that colour and the scrollbar appears
-                // to “change colour”.  Explicitly setting the *track* and *thumb* styles ensures
+                // the scrollbar, the track and thumb adopt that color and the scrollbar appears
+                // to "change color." Explicitly setting the *track* and *thumb* styles ensures
                 // we always draw the scrollbar with the same palette regardless of what content
                 // is behind it.
                 //
-                // N.B.  Only the *foreground* colour matters here because the scrollbar symbols
+                // N.B. Only the *foreground* color matters here because the scrollbar symbols
                 // themselves are filled‐in block glyphs that completely overwrite the prior
-                // character cells.  We therefore leave the background at its default value so it
+                // character cells. We therefore leave the background at its default value so it
                 // blends nicely with the surrounding `Block`.
                 Scrollbar::new(ScrollbarOrientation::VerticalRight)
                     .begin_symbol(Some("↑"))
                     .end_symbol(Some("↓"))
                     .begin_style(Style::reset().fg(Color::DarkGray))
                     .end_style(Style::reset().fg(Color::DarkGray))
-                    // A solid thumb so that we can colour it distinctly from the track.
+                    // A solid thumb so that we can color it distinctly from the track.
                     .thumb_symbol("█")
-                    // Apply the dynamic thumb colour computed above. We still start from
+                    // Apply the dynamic thumb color computed above. We still start from
                     // Style::reset() to clear any inherited modifiers.
                     .thumb_style(thumb_style)
                     // Thin vertical line for the track.
@@ -447,5 +480,25 @@ impl WidgetRef for ConversationHistoryWidget {
                 &mut scroll_state,
             );
         }
+
+        // Update auxiliary stats that the scroll handlers rely on.
+        self.num_rendered_lines.set(num_lines);
+        self.last_viewport_height.set(viewport_height);
     }
 }
+
+/// Common [`Wrap`] configuration used for both measurement and rendering so
+/// they stay in sync.
+#[inline]
+const fn wrap_cfg() -> ratatui::widgets::Wrap {
+    ratatui::widgets::Wrap { trim: false }
+}
+
+/// Returns the wrapped line count for `cell` at the given `width` using the
+/// same wrapping rules that `ConversationHistoryWidget` uses during
+/// rendering.
+fn wrapped_line_count_for_cell(cell: &HistoryCell, width: u16) -> usize {
+    Paragraph::new(cell.lines().clone())
+        .wrap(wrap_cfg())
+        .line_count(width)
+}