fix: wrap lines in the TUI

codex-rs/tui/src/conversation_history_widget.rs

@@ -8,6 +8,7 @@ use crossterm::event::KeyCode;
 use crossterm::event::KeyEvent;
 use ratatui::prelude::*;
 use ratatui::style::Style;
+use ratatui::widgets::Wrap;
 use ratatui::widgets::*;
 use serde_json::Value as JsonValue;
 use std::cell::Cell as StdCell;
@@ -16,6 +17,14 @@ use std::path::PathBuf;
 
 pub struct ConversationHistoryWidget {
     history: Vec<HistoryCell>,
+    /// Cached number of *wrapped* lines for every [`HistoryCell`] in
+    /// `history`.  The length is always kept in sync with `history` so that
+    /// `line_counts[i]` corresponds to `history[i]`.
+    line_counts: std::cell::RefCell<Vec<usize>>,
+    /// The width (in terminal cells/columns) that `line_counts` was computed
+    /// for. When the available width changes we have to recompute the cached
+    /// values.
+    cached_width: StdCell<u16>,
     scroll_position: usize,
     /// Number of lines the last time render_ref() was called
     num_rendered_lines: StdCell<usize>,
@@ -28,6 +37,8 @@ impl ConversationHistoryWidget {
     pub fn new() -> Self {
         Self {
             history: Vec::new(),
+            line_counts: std::cell::RefCell::new(Vec::new()),
+            cached_width: StdCell::new(0),
             scroll_position: usize::MAX,
             num_rendered_lines: StdCell::new(0),
             last_viewport_height: StdCell::new(0),
@@ -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);
@@ -216,15 +225,43 @@ impl ConversationHistoryWidget {
     }
 
     fn add_to_history(&mut self, cell: HistoryCell) {
+        // Keep the cached line count vector in sync with `history`.  If we
+        // already know the current width of the viewport, eagerly compute the
+        // wrapped line count for the newly appended cell. Otherwise we push a
+        // placeholder that will be filled in the next render cycle.
+        let width = self.cached_width.get();
+        {
+            let mut counts = self.line_counts.borrow_mut();
+            if width > 0 {
+                let count = Self::wrapped_line_count_for_cell(&cell, width);
+                counts.push(count);
+            } else {
+                counts.push(0);
+            }
+        }
+
         self.history.push(cell);
     }
 
     /// Remove all history entries and reset scrolling.
     pub fn clear(&mut self) {
         self.history.clear();
+        self.line_counts.borrow_mut().clear();
         self.scroll_position = usize::MAX;
     }
 
+    /// Helper that returns the *wrapped* line count for the given
+    /// `HistoryCell` when rendered in a [`Paragraph`] with the provided
+    /// `width`.
+    fn wrapped_line_count_for_cell(cell: &HistoryCell, width: u16) -> usize {
+        // We do *not* enable trimming here because we want to exactly mirror
+        // how the lines will be rendered in `render_ref`, which uses
+        // `wrap(trim: false)` so that long words are not elided.
+        Paragraph::new(cell.lines().clone())
+            .wrap(ratatui::widgets::Wrap { trim: false })
+            .line_count(width)
+    }
+
     pub fn record_completed_exec_command(
         &mut self,
         call_id: String,
@@ -232,7 +269,8 @@ impl ConversationHistoryWidget {
         stderr: String,
         exit_code: i32,
     ) {
-        for cell in self.history.iter_mut() {
+        let width = self.cached_width.get();
+        for (idx, cell) in self.history.iter_mut().enumerate() {
             if let HistoryCell::ActiveExecCommand {
                 call_id: history_id,
                 command,
@@ -250,6 +288,13 @@ impl ConversationHistoryWidget {
                             duration: start.elapsed(),
                         },
                     );
+
+                    // Update cached line count.
+                    if width > 0 {
+                        if let Some(slot) = self.line_counts.borrow_mut().get_mut(idx) {
+                            *slot = Self::wrapped_line_count_for_cell(&self.history[idx], width);
+                        }
+                    }
                     break;
                 }
             }
@@ -269,7 +314,8 @@ 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 (idx, cell) in self.history.iter_mut().enumerate() {
             if let HistoryCell::ActiveMcpToolCall {
                 call_id: history_id,
                 fq_tool_name,
@@ -287,6 +333,13 @@ impl ConversationHistoryWidget {
                         result_val,
                     );
                     *cell = completed;
+
+                    if width > 0 {
+                        if let Some(slot) = self.line_counts.borrow_mut().get_mut(idx) {
+                            *slot = Self::wrapped_line_count_for_cell(&self.history[idx], width);
+                        }
+                    }
+
                     break;
                 }
             }
@@ -311,97 +364,83 @@ 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 max_scroll = num_lines.saturating_sub(viewport_height) + 1;
+        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 or if we do not have counts
+        // for all entries yet.
+        if self.cached_width.get() != width || self.line_counts.borrow().len() != self.history.len()
+        {
+            self.cached_width.set(width);
+
+            let mut counts = self.line_counts.borrow_mut();
+            counts.clear();
+            counts.reserve(self.history.len());
+            for cell in &self.history {
+                let cnt = Self::wrapped_line_count_for_cell(cell, width);
+                counts.push(cnt);
+            }
+        }
+
+        let num_lines: usize = self.line_counts.borrow().iter().sum();
+
+        // ------------------------------------------------------------------
+        // Scroll position logic (largely unchanged but now using wrapped line
+        // counts instead of naïve line lengths).
+        // ------------------------------------------------------------------
+
+        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);
+        // Materialise *all* lines in a single Vec so we can hand it off to a
+        // Paragraph that takes care of wrapping and scrolling. Although this
+        // means cloning the full conversation buffer every frame, in
+        // practice the performance is perfectly adequate for typical
+        // workloads and keeps the rendering code straightforward.
 
-        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;
-                    }
-                }
-            }
-            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;
-                }
-            }
+        let mut all_lines: Vec<Line<'static>> = Vec::new();
+        for cell in &self.history {
+            all_lines.extend(cell.lines().iter().cloned());
         }
 
-        // 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.  Horizontal trimming is disabled – we want long words to
+        // overflow onto subsequent lines instead of being elided.
+        let paragraph = Paragraph::new(all_lines)
+            .block(block)
+            .wrap(Wrap { trim: false })
+            // Apply the vertical scroll so the correct portion of the text
+            // is visible.
+            .scroll((scroll_pos 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 (unchanged except for using wrapped line counts).
+        // ------------------------------------------------------------------
+
         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);
 
@@ -447,5 +486,9 @@ 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);
     }
 }