fixing bugs

codex-rs/tui/src/conversation_history_widget.rs

@@ -12,6 +12,7 @@ use ratatui::style::Style;
 use ratatui::text::Span;
 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;
@@ -26,24 +27,31 @@ pub struct ConversationHistoryWidget {
     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: Cell<u16>,
+    cached_width: StdCell<u16>,
     scroll_position: usize,
     /// Number of lines the last time render_ref() was called
-    num_rendered_lines: Cell<usize>,
+    num_rendered_lines: StdCell<usize>,
     /// The height of the viewport last time render_ref() was called
-    last_viewport_height: Cell<usize>,
+    last_viewport_height: StdCell<usize>,
     has_input_focus: bool,
+    /// Scratch buffer used while incrementally streaming an agent message.
+    /// We accumulate the full text so we can re-render markdown cleanly when the turn finishes.
+    streaming_agent_message_buf: String,
+    /// Scratch buffer used while incrementally streaming agent reasoning.
+    streaming_agent_reasoning_buf: String,
 }
 
 impl ConversationHistoryWidget {
     pub fn new() -> Self {
         Self {
             entries: Vec::new(),
-            cached_width: Cell::new(0),
+            cached_width: StdCell::new(0),
             scroll_position: usize::MAX,
-            num_rendered_lines: Cell::new(0),
-            last_viewport_height: Cell::new(0),
+            num_rendered_lines: StdCell::new(0),
+            last_viewport_height: StdCell::new(0),
             has_input_focus: false,
+            streaming_agent_message_buf: String::new(),
+            streaming_agent_reasoning_buf: String::new(),
         }
     }
 
@@ -84,26 +92,38 @@ impl ConversationHistoryWidget {
     }
 
     fn scroll_up(&mut self, num_lines: u32) {
-        // Convert sticky-to-bottom sentinel into a concrete offset anchored at the bottom.
+        // 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 calculate the delta.
+        // This requires us to care about how tall the screen is.
         if self.scroll_position == usize::MAX {
-            self.scroll_position = sticky_offset(
-                self.num_rendered_lines.get(),
-                self.last_viewport_height.get(),
-            );
+            self.scroll_position = self
+                .num_rendered_lines
+                .get()
+                .saturating_sub(self.last_viewport_height.get());
         }
+
         self.scroll_position = self.scroll_position.saturating_sub(num_lines as usize);
     }
 
     fn scroll_down(&mut self, num_lines: u32) {
-        // Nothing to do if we're already pinned to the bottom.
+        // If we're already pinned to the bottom there's nothing to do.
         if self.scroll_position == usize::MAX {
             return;
         }
+
         let viewport_height = self.last_viewport_height.get().max(1);
-        let max_scroll = sticky_offset(self.num_rendered_lines.get(), viewport_height);
+        let num_rendered_lines = self.num_rendered_lines.get();
+
+        // 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);
+
         let new_pos = self.scroll_position.saturating_add(num_lines as usize);
+
         if new_pos >= max_scroll {
-            // Switch to sticky-bottom mode so subsequent output pins view.
+            // Reached (or passed) the bottom – switch to stick‑to‑bottom mode
+            // so that additional output keeps the view pinned automatically.
             self.scroll_position = usize::MAX;
         } else {
             self.scroll_position = new_pos;
@@ -113,21 +133,44 @@ impl ConversationHistoryWidget {
     /// Scroll up by one full viewport height (Page Up).
     fn scroll_page_up(&mut self) {
         let viewport_height = self.last_viewport_height.get().max(1);
+
+        // If we are currently in the "stick to bottom" mode, first convert the
+        // implicit scroll position (`usize::MAX`) into an explicit offset that
+        // represents the very bottom of the scroll region.  This mirrors the
+        // logic from `scroll_up()`.
         if self.scroll_position == usize::MAX {
-            self.scroll_position = sticky_offset(self.num_rendered_lines.get(), viewport_height);
+            self.scroll_position = self
+                .num_rendered_lines
+                .get()
+                .saturating_sub(viewport_height);
         }
+
+        // Move up by a full page.
         self.scroll_position = self.scroll_position.saturating_sub(viewport_height);
     }
 
     /// Scroll down by one full viewport height (Page Down).
     fn scroll_page_down(&mut self) {
+        // Nothing to do if we're already stuck to the bottom.
         if self.scroll_position == usize::MAX {
             return;
         }
+
         let viewport_height = self.last_viewport_height.get().max(1);
-        let max_scroll = sticky_offset(self.num_rendered_lines.get(), viewport_height);
+        let num_lines = self.num_rendered_lines.get();
+
+        // 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);
+
+        // Attempt to move down by a full page.
         let new_pos = self.scroll_position.saturating_add(viewport_height);
+
         if new_pos >= max_scroll {
+            // We have reached (or passed) the bottom – switch back to
+            // automatic stick‑to‑bottom mode so that subsequent output keeps
+            // the viewport pinned.
             self.scroll_position = usize::MAX;
         } else {
             self.scroll_position = new_pos;
@@ -160,35 +203,133 @@ impl ConversationHistoryWidget {
     }
 
     pub fn add_agent_message(&mut self, config: &Config, message: String) {
+        // Reset streaming buffer – we are starting a new message.
+        self.streaming_agent_message_buf.clear();
+        self.streaming_agent_message_buf.push_str(&message);
         self.add_to_history(HistoryCell::new_agent_message(config, message));
     }
 
     pub fn add_agent_reasoning(&mut self, config: &Config, text: String) {
+        self.streaming_agent_reasoning_buf.clear();
+        self.streaming_agent_reasoning_buf.push_str(&text);
         self.add_to_history(HistoryCell::new_agent_reasoning(config, text));
     }
 
-    /// Append incremental assistant text.  Streaming mode deliberately avoids expensive
-    /// markdown re-rendering on every token; we just extend the final (currently-visible)
-    /// body line(s) and honour explicit newlines from the delta chunk.  When the turn
-    /// completes the caller should invoke `replace_last_agent_message()` with the *full*
-    /// accumulated markdown so that styling (code fences, emphasis, links, etc.) is correct.
+    /// Append incremental assistant text without *forcing* a newline between chunks.
+    ///
+    /// The earlier implementation re-ran the markdown renderer on each delta.
+    /// Because `tui_markdown` always produces whole `Line` values, every chunk
+    /// showed up on its own row ("stair‑stepping" output).  Instead we take a
+    /// lightweight approach during streaming: extend the last visible line with
+    /// plain text and only honour explicit `\n` boundaries from the model.  When
+    /// the turn completes the caller should invoke `replace_last_agent_message()`
+    /// to re-render the full accumulated markdown so styling (code blocks, bold,
+    /// links, etc.) is correct.
     pub fn append_agent_message_delta(&mut self, _config: &Config, text: String) {
         if text.is_empty() {
             return;
         }
-        // Fast path: mutate the most recent AgentMessage cell if present; else create one.
-        if let Some(idx) = last_agent_message_idx(&self.entries) {
-            let width = self.cached_width.get();
-            let entry = &mut self.entries[idx];
-            if let HistoryCell::AgentMessage { view } = &mut entry.cell {
-                append_streaming_text_chunks(&mut view.lines, &text);
-                if width > 0 {
-                    update_entry_height(entry, width);
+
+        // Accumulate in scratch buffer so we can re-render later.
+        self.streaming_agent_message_buf.push_str(&text);
+
+        // If the newly received chunk contains a newline we re-render the entire
+        // accumulated buffer using the markdown renderer so formatting (bold,
+        // code blocks, links) becomes visible incrementally.  This is cheaper and
+        // less visually noisy than re-rendering on every token while still
+        // giving the user feedback at natural boundaries.
+        if text.contains('\n') {
+            // Rebuild the most recent AgentMessage entry from scratch.
+            let mut found_idx: Option<usize> = None;
+            for i in (0..self.entries.len()).rev() {
+                if matches!(self.entries[i].cell, HistoryCell::AgentMessage { .. }) {
+                    found_idx = Some(i);
+                    break;
+                }
+            }
+            if let Some(idx) = found_idx {
+                let width = self.cached_width.get();
+                // Normalize single newlines so streaming layout matches final markdown.
+                let collapsed =
+                    collapse_single_newlines_for_streaming(&self.streaming_agent_message_buf);
+                // Rebuild cell then borrow entry once.
+                let rebuilt = HistoryCell::new_agent_message(_config, collapsed);
+                let entry = &mut self.entries[idx];
+                entry.cell = rebuilt;
+                // Drop the trailing blank added by new_agent_message so we can continue streaming.
+                if let HistoryCell::AgentMessage { view } = &mut entry.cell {
+                    if let Some(last) = view.lines.last() {
+                        if last.spans.len() == 1 && last.spans[0].content.is_empty() {
+                            view.lines.pop();
+                        }
+                    }
+                    if width > 0 {
+                        entry.line_count.set(view.height(width));
+                    }
                 }
                 return;
             }
         }
-        // No existing cell – begin a new (header‑only) streaming cell and retry.
+
+        if let Some(entry) = self.entries.last_mut() {
+            if let HistoryCell::AgentMessage { view } = &mut entry.cell {
+                // Ensure there is *at least* one line available for content after the header.
+                // new_agent_message() with an empty string produces a header line and one blank line.
+                // We keep that blank as our first append target.
+                if view.lines.len() < 2 {
+                    view.lines.push(Line::from(""));
+                }
+
+                // Trim *at most one* trailing separator line that we added for spacing.
+                // Preserve user-intended blank lines (paragraph breaks) so we do not collapse newlines.
+                if view.lines.len() > 1 {
+                    if let Some(last) = view.lines.last() {
+                        if last.spans.len() == 1 && last.spans[0].content.is_empty() {
+                            view.lines.pop();
+                        }
+                    }
+                }
+
+                // Append respecting embedded newlines from the chunk.
+                let mut first_part = true;
+                for part in text.split_inclusive('\n') {
+                    let has_newline = part.ends_with('\n');
+                    let content = part.trim_end_matches('\n');
+                    if first_part {
+                        if let Some(last_line) = view.lines.last_mut() {
+                            last_line.spans.push(Span::raw(content.to_string()));
+                        } else {
+                            view.lines.push(Line::from(content.to_string()));
+                        }
+                        first_part = false;
+                    } else {
+                        // Option 1: If this is a new line, and content starts with a space, trim it.
+                        let trimmed_content = if content.starts_with(' ')
+                            && matches!(view.lines.last(), Some(l) if l.spans.is_empty())
+                        {
+                            content.trim_start()
+                        } else {
+                            content
+                        };
+                        view.lines.push(Line::from(trimmed_content.to_string()));
+                    }
+                    if has_newline {
+                        // honour explicit newline: start a fresh empty line (content target)
+                        view.lines.push(Line::from(""));
+                    }
+                }
+
+                // DO NOT push the cell separator yet; we'll add it on finalisation.
+
+                let width = self.cached_width.get();
+                if width > 0 {
+                    entry.line_count.set(view.height(width));
+                }
+                return;
+            }
+        }
+        // Fallback: no existing AgentMessage – start a new one.
+        // Start a streaming cell with an *empty* body so we don't parse partial markdown.
         self.add_agent_message(_config, String::new());
         self.append_agent_message_delta(_config, text);
     }
@@ -198,13 +339,80 @@ impl ConversationHistoryWidget {
         if text.is_empty() {
             return;
         }
-        if let Some(idx) = last_agent_reasoning_idx(&self.entries) {
-            let width = self.cached_width.get();
-            let entry = &mut self.entries[idx];
+        self.streaming_agent_reasoning_buf.push_str(&text);
+
+        // Re-render incrementally at newline boundaries.
+        if text.contains('\n') {
+            let mut found_idx: Option<usize> = None;
+            for i in (0..self.entries.len()).rev() {
+                if matches!(self.entries[i].cell, HistoryCell::AgentReasoning { .. }) {
+                    found_idx = Some(i);
+                    break;
+                }
+            }
+            if let Some(idx) = found_idx {
+                let width = self.cached_width.get();
+                let collapsed =
+                    collapse_single_newlines_for_streaming(&self.streaming_agent_reasoning_buf);
+                let rebuilt = HistoryCell::new_agent_reasoning(_config, collapsed);
+                let entry = &mut self.entries[idx];
+                entry.cell = rebuilt;
+                if let HistoryCell::AgentReasoning { view } = &mut entry.cell {
+                    if let Some(last) = view.lines.last() {
+                        if last.spans.len() == 1 && last.spans[0].content.is_empty() {
+                            view.lines.pop();
+                        }
+                    }
+                    if width > 0 {
+                        entry.line_count.set(view.height(width));
+                    }
+                }
+                return;
+            }
+        }
+
+        if let Some(entry) = self.entries.last_mut() {
             if let HistoryCell::AgentReasoning { view } = &mut entry.cell {
-                append_streaming_text_chunks(&mut view.lines, &text);
+                if view.lines.len() < 2 {
+                    view.lines.push(Line::from(""));
+                }
+                if view.lines.len() > 1 {
+                    if let Some(last) = view.lines.last() {
+                        if last.spans.len() == 1 && last.spans[0].content.is_empty() {
+                            view.lines.pop();
+                        }
+                    }
+                }
+                let mut first_part = true;
+                for part in text.split_inclusive('\n') {
+                    let has_newline = part.ends_with('\n');
+                    let content = part.trim_end_matches('\n');
+                    if first_part {
+                        if let Some(last_line) = view.lines.last_mut() {
+                            last_line.spans.push(Span::raw(content.to_string()));
+                        } else {
+                            view.lines.push(Line::from(content.to_string()));
+                        }
+                        first_part = false;
+                    } else {
+                        let trimmed_content = if content.starts_with(' ')
+                            && matches!(view.lines.last(), Some(l) if l.spans.is_empty())
+                        {
+                            content.trim_start()
+                        } else {
+                            content
+                        };
+                        view.lines.push(Line::from(trimmed_content.to_string()));
+                    }
+                    if has_newline {
+                        view.lines.push(Line::from(""));
+                    }
+                }
+                // no separator until finalisation
+
+                let width = self.cached_width.get();
                 if width > 0 {
-                    update_entry_height(entry, width);
+                    entry.line_count.set(view.height(width));
                 }
                 return;
             }
@@ -216,12 +424,18 @@ impl ConversationHistoryWidget {
     /// Replace the most recent AgentMessage cell with the fully accumulated `text`.
     /// This should be called once the turn is complete so we can render proper markdown.
     pub fn replace_last_agent_message(&mut self, config: &Config, text: String) {
-        if let Some(idx) = last_agent_message_idx(&self.entries) {
+        self.streaming_agent_message_buf.clear();
+        // Find the most recent AgentMessage entry (search from end).
+        if let Some(idx) = self
+            .entries
+            .iter()
+            .rposition(|e| matches!(e.cell, HistoryCell::AgentMessage { .. }))
+        {
             let width = self.cached_width.get();
             let entry = &mut self.entries[idx];
             entry.cell = HistoryCell::new_agent_message(config, text);
             if width > 0 {
-                update_entry_height(entry, width);
+                entry.line_count.set(entry.cell.height(width));
             }
         } else {
             // No existing AgentMessage (shouldn't happen) – append new.
@@ -231,12 +445,17 @@ impl ConversationHistoryWidget {
 
     /// Replace the most recent AgentReasoning cell with the fully accumulated `text`.
     pub fn replace_last_agent_reasoning(&mut self, config: &Config, text: String) {
-        if let Some(idx) = last_agent_reasoning_idx(&self.entries) {
+        self.streaming_agent_reasoning_buf.clear();
+        if let Some(idx) = self
+            .entries
+            .iter()
+            .rposition(|e| matches!(e.cell, HistoryCell::AgentReasoning { .. }))
+        {
             let width = self.cached_width.get();
             let entry = &mut self.entries[idx];
             entry.cell = HistoryCell::new_agent_reasoning(config, text);
             if width > 0 {
-                update_entry_height(entry, width);
+                entry.line_count.set(entry.cell.height(width));
             }
         } else {
             self.add_agent_reasoning(config, text);
@@ -320,7 +539,7 @@ impl ConversationHistoryWidget {
 
                     // Update cached line count.
                     if width > 0 {
-                        update_entry_height(entry, width);
+                        entry.line_count.set(cell.height(width));
                     }
                     break;
                 }
@@ -354,7 +573,7 @@ impl ConversationHistoryWidget {
                     entry.cell = completed;
 
                     if width > 0 {
-                        update_entry_height(entry, width);
+                        entry.line_count.set(entry.cell.height(width));
                     }
 
                     break;
@@ -411,12 +630,14 @@ impl WidgetRef for ConversationHistoryWidget {
             self.entries.iter().map(|e| e.line_count.get()).sum()
         };
 
-        // Determine the scroll position (respect sticky-to-bottom sentinel and clamp).
-        let max_scroll = sticky_offset(num_lines, viewport_height);
+        // 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 {
-            clamp_scroll_pos(self.scroll_position, max_scroll)
+            self.scroll_position.min(max_scroll)
         };
 
         // ------------------------------------------------------------------
@@ -537,88 +758,46 @@ pub(crate) const fn wrap_cfg() -> ratatui::widgets::Wrap {
     ratatui::widgets::Wrap { trim: false }
 }
 
-// ---------------------------------------------------------------------------
-// Scrolling helpers (private)
-// ---------------------------------------------------------------------------
-#[inline]
-fn sticky_offset(num_lines: usize, viewport_height: usize) -> usize {
-    num_lines.saturating_sub(viewport_height.max(1))
-}
-
-#[inline]
-fn clamp_scroll_pos(pos: usize, max_scroll: usize) -> usize {
-    pos.min(max_scroll)
-}
-
-// ---------------------------------------------------------------------------
-// Streaming helpers (private)
-// ---------------------------------------------------------------------------
-
-/// Locate the most recent `HistoryCell::AgentMessage` entry.
-fn last_agent_message_idx(entries: &[Entry]) -> Option<usize> {
-    entries
-        .iter()
-        .rposition(|e| matches!(e.cell, HistoryCell::AgentMessage { .. }))
-}
-
-/// Locate the most recent `HistoryCell::AgentReasoning` entry.
-fn last_agent_reasoning_idx(entries: &[Entry]) -> Option<usize> {
-    entries
-        .iter()
-        .rposition(|e| matches!(e.cell, HistoryCell::AgentReasoning { .. }))
-}
-
-/// True if the line is an empty spacer (single empty span).
-fn is_blank_line(line: &Line<'_>) -> bool {
-    line.spans.len() == 1 && line.spans[0].content.is_empty()
-}
-
-/// Ensure that the vector has *at least* one body line after the header.
-/// A freshly-created AgentMessage/Reasoning cell always has a header + blank line,
-/// but streaming cells may be created empty; this makes sure we have a target line.
-fn ensure_body_line(lines: &mut Vec<Line<'static>>) {
-    if lines.len() < 2 {
-        lines.push(Line::from(""));
-    }
-}
-
-/// Trim a single trailing blank spacer line (but preserve intentional paragraph breaks).
-fn drop_trailing_blank_line(lines: &mut Vec<Line<'static>>) {
-    if let Some(last) = lines.last() {
-        if is_blank_line(last) {
-            lines.pop();
+/// Collapse *single* newlines in a streaming buffer into spaces so that
+/// interim streaming renders more closely match final Markdown layout.
+///
+/// Markdown typically treats a single bare newline inside a paragraph as a
+/// soft break (i.e., a space). Users were seeing temporary "jumps" where a
+/// line streamed as:
+///     The\n user might be asking …
+/// would later collapse to "The user…" after the full Markdown re-render.
+/// By collapsing single newlines up front we show the same layout during
+/// streaming that the final render will produce.
+///
+/// Runs in O(n) over the input string; allocates a new String only when a
+/// collapse is actually needed.
+fn collapse_single_newlines_for_streaming(src: &str) -> String {
+    let mut out = String::with_capacity(src.len());
+    let mut pending_newlines = 0usize;
+    for ch in src.chars() {
+        if ch == '\n' {
+            pending_newlines += 1;
+            continue;
         }
-    }
-}
-
-/// Append streaming text, honouring embedded newlines.
-fn append_streaming_text_chunks(lines: &mut Vec<Line<'static>>, text: &str) {
-    ensure_body_line(lines);
-    drop_trailing_blank_line(lines);
-
-    // We use `split_inclusive` so we can detect (and preserve) explicit newlines.
-    let mut first = true;
-    for chunk in text.split_inclusive('\n') {
-        let had_nl = chunk.ends_with('\n');
-        let content = chunk.trim_end_matches('\n');
-        if first {
-            if let Some(last_line) = lines.last_mut() {
-                last_line.spans.push(Span::raw(content.to_string()));
-            } else {
-                lines.push(Line::from(content.to_string()));
+        if pending_newlines == 1 {
+            // soft break -> space
+            out.push(' ');
+        } else if pending_newlines > 1 {
+            // preserve paragraph breaks exactly
+            for _ in 0..pending_newlines {
+                out.push('\n');
             }
-            first = false;
-        } else {
-            lines.push(Line::from(content.to_string()));
         }
-        if had_nl {
-            // Start a fresh (potentially target) line for subsequent appends.
-            lines.push(Line::from(""));
+        pending_newlines = 0;
+        out.push(ch);
+    }
+    // flush tail
+    if pending_newlines == 1 {
+        out.push(' ');
+    } else if pending_newlines > 1 {
+        for _ in 0..pending_newlines {
+            out.push('\n');
         }
     }
-}
-
-/// Re-measure a mutated entry at `width` columns and update its cached height.
-fn update_entry_height(entry: &Entry, width: u16) {
-    entry.line_count.set(entry.cell.height(width));
+    out
 }