fix: wrap lines in the TUI

codex-rs/tui/src/conversation_history_widget.rs

@@ -11,11 +11,41 @@ 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;
 
+// ────────────────────────────────────────────────────────────────────────────
+// Helper functions
+// ────────────────────────────────────────────────────────────────────────────
+
+/// 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)
+}
+
+/// 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 +57,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),
@@ -97,9 +128,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 +173,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 +195,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,15 +245,27 @@ 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;
     }
 
+
+
     pub fn record_completed_exec_command(
         &mut self,
         call_id: String,
@@ -232,7 +273,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 +293,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 +319,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 +337,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,97 +369,78 @@ 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.
+        if self.cached_width.get() != width {
+            self.cached_width.set(width);
+
+            for entry in &self.entries {
+                let cnt = wrapped_line_count_for_cell(&entry.cell, width);
+                entry.line_count.set(cnt);
+            }
+        }
+
+        let num_lines: usize = self.entries.iter().map(|e| e.line_count.get()).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 entry in &self.entries {
+            all_lines.extend(entry.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_cfg())
+            // 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);
     }
 }