From 101bf9b3c25b111f65e3ce01bc3371371c783951 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 14:17:07 +0000 Subject: [PATCH 1/7] perf(tui): compute Tasks sidebar row sets once per frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit task_panel_rows() and task_panel_hover_texts() each independently called active_tool_rows(), reasoning_task_rows(), background_task_rows() (a clone+sort of app.task_panel), and recent_tool_rows() on every frame the Tasks panel was visible — doubling the row-building work per frame. Introduce TaskPanelRowSets, computed once in render_sidebar_tasks and passed by reference to both consumers. The focus-conditional live-tool dedup argument to background_task_rows (Tasks focus dedups, Auto mode does not) is preserved and now evaluated in one place. As a side benefit, both consumers now see one snapshot, so hover rows can no longer disagree with rendered rows across an Instant::elapsed TTL boundary within a single frame. Also stop double-cloning every SidebarAgentRow in sort_sidebar_agent_rows_as_tree: emit an index order first, then materialize by move (the `seen` set guarantees each index is emitted exactly once), noted as a related win in the issue. New tests pin the focus-conditional background dedup, rows/hover snapshot alignment, and exactly-once emission of agent rows (including the rootless parent-cycle orphan sweep). Fixes #3898 --- crates/tui/src/tui/sidebar.rs | 261 ++++++++++++++++++++++++++++------ 1 file changed, 215 insertions(+), 46 deletions(-) diff --git a/crates/tui/src/tui/sidebar.rs b/crates/tui/src/tui/sidebar.rs index 72bd8b2877..58fe5170a3 100644 --- a/crates/tui/src/tui/sidebar.rs +++ b/crates/tui/src/tui/sidebar.rs @@ -1221,9 +1221,11 @@ fn render_sidebar_tasks(f: &mut Frame, area: Rect, app: &mut App) { let content_width = area.width.saturating_sub(4) as usize; let usable_rows = area.height.saturating_sub(3) as usize; - let (lines, row_actions) = task_panel_rows(app, content_width.max(1), usable_rows.max(1)); + let row_sets = task_panel_row_sets(app); + let (lines, row_actions) = + task_panel_rows(app, &row_sets, content_width.max(1), usable_rows.max(1)); - let full_texts = task_panel_hover_texts(app, usable_rows.max(1)); + let full_texts = task_panel_hover_texts(app, &row_sets, usable_rows.max(1)); render_sidebar_section(f, area, "Tasks", lines, full_texts, row_actions, app); } @@ -1235,9 +1237,43 @@ struct SidebarToolRow { duration_ms: Option, } +/// Row sets shared by the Tasks panel line renderer and hover-text builder. +/// +/// Computed once per frame in `render_sidebar_tasks` (#3898): both consumers +/// previously recomputed `active_tool_rows` / `reasoning_task_rows` / +/// `background_task_rows` (a clone+sort of `app.task_panel`) independently, +/// doubling the work and risking the two passes disagreeing across an +/// `Instant::elapsed` TTL boundary within the same frame. +struct TaskPanelRowSets { + active: Vec, + reasoning: Vec, + background: Vec, + recent: Vec, +} + +fn task_panel_row_sets(app: &App) -> TaskPanelRowSets { + let explicit_tasks_focus = app.sidebar_focus == SidebarFocus::Tasks; + let active = active_tool_rows(app); + let reasoning = reasoning_task_rows(app); + // Auto/Pinned mode deliberately skips live-tool dedup (passes an empty + // slice), matching the previous per-consumer call sites. + let background = background_task_rows(app, if explicit_tasks_focus { &active } else { &[] }); + let recent = if explicit_tasks_focus { + recent_tool_rows(app, 4) + } else { + Vec::new() + }; + TaskPanelRowSets { + active, + reasoning, + background, + recent, + } +} + #[cfg(test)] fn task_panel_lines(app: &App, content_width: usize, max_rows: usize) -> Vec> { - task_panel_rows(app, content_width, max_rows).0 + task_panel_rows(app, &task_panel_row_sets(app), content_width, max_rows).0 } /// Build the Tasks panel lines together with a parallel per-line click-action @@ -1245,6 +1281,7 @@ fn task_panel_lines(app: &App, content_width: usize, max_rows: usize) -> Vec (Vec>, Vec>) { @@ -1273,26 +1310,19 @@ fn task_panel_rows( ))); } - let active_rows = active_tool_rows(app); + let active_rows = &row_sets.active; if explicit_tasks_focus && !active_rows.is_empty() && lines.len() < max_rows { push_sidebar_label_theme(&mut lines, "Live tools", theme); - push_tool_rows(&mut lines, &active_rows, content_width, max_rows, theme); + push_tool_rows(&mut lines, active_rows, content_width, max_rows, theme); } - let reasoning_rows = reasoning_task_rows(app); + let reasoning_rows = &row_sets.reasoning; if explicit_tasks_focus && !reasoning_rows.is_empty() && lines.len() < max_rows { push_sidebar_label_theme(&mut lines, "Model reasoning", theme); - push_reasoning_rows(&mut lines, &reasoning_rows, content_width, max_rows, theme); + push_reasoning_rows(&mut lines, reasoning_rows, content_width, max_rows, theme); } - let background_rows = background_task_rows( - app, - if explicit_tasks_focus { - &active_rows - } else { - &[] - }, - ); + let background_rows = &row_sets.background; // Lines pushed so far (turn label, Live tools header, live tool rows) // are not clickable — backfill their action slots. actions.resize(lines.len(), None); @@ -1391,10 +1421,10 @@ fn task_panel_rows( } if explicit_tasks_focus && lines.len() < max_rows { - let recent_rows = recent_tool_rows(app, 4); + let recent_rows = &row_sets.recent; if !recent_rows.is_empty() { push_sidebar_label_theme(&mut lines, "Recent tools", theme); - push_tool_rows(&mut lines, &recent_rows, content_width, max_rows, theme); + push_tool_rows(&mut lines, recent_rows, content_width, max_rows, theme); } } @@ -1430,7 +1460,7 @@ fn task_panel_rows( (lines, actions) } -fn task_panel_hover_texts(app: &App, max_rows: usize) -> Vec { +fn task_panel_hover_texts(app: &App, row_sets: &TaskPanelRowSets, max_rows: usize) -> Vec { let mut texts = Vec::with_capacity(max_rows.max(4)); let explicit_tasks_focus = app.sidebar_focus == SidebarFocus::Tasks; @@ -1439,26 +1469,19 @@ fn task_panel_hover_texts(app: &App, max_rows: usize) -> Vec { texts.push(format!("turn {turn_id} ({status})")); } - let active_rows = active_tool_rows(app); + let active_rows = &row_sets.active; if explicit_tasks_focus && !active_rows.is_empty() && texts.len() < max_rows { texts.push("Live tools".to_string()); - push_tool_row_hover_texts(&mut texts, &active_rows, max_rows); + push_tool_row_hover_texts(&mut texts, active_rows, max_rows); } - let reasoning_rows = reasoning_task_rows(app); + let reasoning_rows = &row_sets.reasoning; if explicit_tasks_focus && !reasoning_rows.is_empty() && texts.len() < max_rows { texts.push("Model reasoning".to_string()); - push_reasoning_row_hover_texts(&mut texts, &reasoning_rows, max_rows); + push_reasoning_row_hover_texts(&mut texts, reasoning_rows, max_rows); } - let background_rows = background_task_rows( - app, - if explicit_tasks_focus { - &active_rows - } else { - &[] - }, - ); + let background_rows = &row_sets.background; if !background_rows.is_empty() && texts.len() < max_rows { let running = background_rows .iter() @@ -1510,10 +1533,10 @@ fn task_panel_hover_texts(app: &App, max_rows: usize) -> Vec { } if explicit_tasks_focus && texts.len() < max_rows { - let recent_rows = recent_tool_rows(app, 4); + let recent_rows = &row_sets.recent; if !recent_rows.is_empty() { texts.push("Recent tools".to_string()); - push_tool_row_hover_texts(&mut texts, &recent_rows, max_rows); + push_tool_row_hover_texts(&mut texts, recent_rows, max_rows); } } @@ -2616,28 +2639,36 @@ fn sort_sidebar_agent_rows_as_tree(rows: Vec) -> Vec>, seen: &mut std::collections::HashSet, - out: &mut Vec, + order: &mut Vec, ) { if !seen.insert(idx) { return; } - out.push(rows[idx].clone()); + order.push(idx); if let Some(child_indices) = children.get(&rows[idx].id) { for child_idx in child_indices { - push_tree(*child_idx, rows, children, seen, out); + push_tree(*child_idx, rows, children, seen, order); } } } - let mut out = Vec::with_capacity(rows.len()); + let mut order = Vec::with_capacity(rows.len()); let mut seen = std::collections::HashSet::new(); for idx in roots { - push_tree(idx, &rows, &children, &mut seen, &mut out); + push_tree(idx, &rows, &children, &mut seen, &mut order); } for idx in 0..rows.len() { - push_tree(idx, &rows, &children, &mut seen, &mut out); + push_tree(idx, &rows, &children, &mut seen, &mut order); } - out + + // Materialize by move instead of cloning each row a second time (#3898): + // `seen` guarantees every index lands in `order` exactly once, so each + // slot is taken exactly once and no row is dropped. + let mut slots: Vec> = rows.into_iter().map(Some).collect(); + order + .into_iter() + .map(|idx| slots[idx].take().expect("each row emitted exactly once")) + .collect() } fn subagent_status_text(status: &SubAgentStatus) -> &'static str { @@ -3277,7 +3308,8 @@ mod tests { normalize_activity_text, render_sidebar, sidebar_agent_rows, sidebar_hover_rows, sidebar_work_summary, sort_sidebar_agent_rows_as_tree, subagent_panel_hover_texts, subagent_panel_lines, subagent_panel_rows, task_panel_hover_texts, task_panel_lines, - task_panel_rows, work_panel_empty_hint, work_panel_hover_texts, work_panel_lines, + task_panel_row_sets, task_panel_rows, work_panel_empty_hint, work_panel_hover_texts, + work_panel_lines, }; use crate::config::Config; use crate::palette; @@ -4342,6 +4374,98 @@ mod tests { ); } + #[test] + fn task_panel_row_sets_dedup_background_only_when_tasks_focused() { + let mut app = create_test_app(); + let mut active = ActiveCell::new(); + active.push_tool( + "tool-1", + HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: "rlm_search".to_string(), + status: ToolStatus::Running, + input_summary: Some("scanning workspace".to_string()), + output: None, + prompts: None, + spillover_path: None, + output_summary: None, + is_diff: false, + })), + ); + app.active_cell = Some(active); + app.task_panel.push(TaskPanelEntry { + id: "rlm-123".to_string(), + status: "running".to_string(), + prompt_summary: "RLM: scanning workspace".to_string(), + duration_ms: Some(1_000), + kind: TaskPanelEntryKind::Background, + stale: false, + elapsed_since_output_ms: None, + owner_agent_id: None, + owner_agent_name: None, + }); + + app.sidebar_focus = SidebarFocus::Tasks; + let focused = task_panel_row_sets(&app); + assert!( + focused.background.is_empty(), + "Tasks focus dedups background jobs against live tools: {:?}", + focused.background + ); + + app.sidebar_focus = SidebarFocus::Auto; + let auto = task_panel_row_sets(&app); + assert_eq!( + auto.background.len(), + 1, + "Auto mode keeps background jobs even when a live tool matches" + ); + } + + #[test] + fn task_panel_rows_and_hover_share_one_snapshot() { + let mut app = create_test_app(); + app.sidebar_focus = SidebarFocus::Tasks; + app.runtime_turn_id = Some("turn_abcdef123456".to_string()); + app.runtime_turn_status = Some("in_progress".to_string()); + app.turn_counter = 3; + app.task_panel.push(TaskPanelEntry { + id: "job_123".to_string(), + status: "running".to_string(), + prompt_summary: "shell: cargo test --workspace".to_string(), + duration_ms: Some(12_000), + kind: TaskPanelEntryKind::Background, + stale: false, + elapsed_since_output_ms: None, + owner_agent_id: None, + owner_agent_name: None, + }); + + let row_sets = task_panel_row_sets(&app); + let (lines, actions) = task_panel_rows(&app, &row_sets, 80, 12); + let hover = task_panel_hover_texts(&app, &row_sets, 12); + let text = lines_to_text(&lines); + + assert_eq!(lines.len(), actions.len(), "actions align with lines"); + let job_idx = text + .iter() + .position(|line| line.contains("Bash running")) + .unwrap_or_else(|| panic!("bash job row missing: {text:?}")); + assert!( + hover[job_idx].contains("cargo test --workspace"), + "hover text at the job row index carries the full command: {hover:?}" + ); + assert!( + text[0].starts_with("Turn 3"), + "line shows stable turn label: {:?}", + text[0] + ); + assert!( + hover[0].contains("turn_abcdef123456"), + "hover carries the full turn id: {:?}", + hover[0] + ); + } + #[test] fn tasks_panel_collapses_stale_running_tool_rows() { let mut app = create_test_app(); @@ -4648,7 +4772,7 @@ mod tests { owner_agent_name: None, }); - let (lines, actions) = task_panel_rows(&app, 80, 12); + let (lines, actions) = task_panel_rows(&app, &task_panel_row_sets(&app), 80, 12); let text = lines_to_text(&lines); assert_eq!(lines.len(), actions.len()); @@ -4688,7 +4812,7 @@ mod tests { owner_agent_name: None, }); - let (lines, actions) = task_panel_rows(&app, 80, 12); + let (lines, actions) = task_panel_rows(&app, &task_panel_row_sets(&app), 80, 12); let text = lines_to_text(&lines); assert!( @@ -4742,7 +4866,7 @@ mod tests { owner_agent_name: None, }); - let (lines, actions) = task_panel_rows(&app, 96, 16); + let (lines, actions) = task_panel_rows(&app, &task_panel_row_sets(&app), 96, 16); let text = lines_to_text(&lines); assert_eq!(lines.len(), actions.len()); @@ -4809,7 +4933,7 @@ mod tests { owner_agent_name: None, }); - let (lines, actions) = task_panel_rows(&app, 80, 12); + let (lines, actions) = task_panel_rows(&app, &task_panel_row_sets(&app), 80, 12); let text = lines_to_text(&lines); let label_idx = text @@ -4860,7 +4984,7 @@ mod tests { owner_agent_name: None, }); - let (lines, actions) = task_panel_rows(&app, 96, 16); + let (lines, actions) = task_panel_rows(&app, &task_panel_row_sets(&app), 96, 16); let text = lines_to_text(&lines); assert_eq!( lines.len(), @@ -4963,6 +5087,50 @@ mod tests { assert_eq!(actions[agent_idx].as_deref(), Some("/fleet status")); } + #[test] + fn sort_sidebar_agent_rows_as_tree_emits_each_row_exactly_once() { + let agent_row = |id: &str, parent: Option<&str>| SidebarAgentRow { + id: id.to_string(), + parent_run_id: parent.map(str::to_string), + spawn_depth: 1, + name: id.to_string(), + role: "explore".to_string(), + status: "running".to_string(), + objective: None, + git_branch: None, + progress: None, + steps_taken: 1, + duration_ms: None, + }; + // Parent + child + a two-node parent cycle: the cycle has no root, so + // only the orphan sweep reaches it. Every row must still come out + // exactly once (the move-based materialization panics on a double + // take and this pins the drop case too). + let rows = vec![ + agent_row("agent_parent", None), + agent_row("agent_child", Some("agent_parent")), + agent_row("agent_cycle_a", Some("agent_cycle_b")), + agent_row("agent_cycle_b", Some("agent_cycle_a")), + ]; + + let sorted = sort_sidebar_agent_rows_as_tree(rows); + + assert_eq!(sorted.len(), 4, "no rows dropped or duplicated"); + let mut ids: Vec<&str> = sorted.iter().map(|row| row.id.as_str()).collect(); + ids.sort_unstable(); + assert_eq!( + ids, + vec![ + "agent_child", + "agent_cycle_a", + "agent_cycle_b", + "agent_parent" + ] + ); + assert_eq!(sorted[0].id, "agent_parent"); + assert_eq!(sorted[1].id, "agent_child"); + } + #[test] fn subagent_sidebar_orders_and_indents_nested_children() { let rows = vec![ @@ -4996,6 +5164,7 @@ mod tests { let sorted = sort_sidebar_agent_rows_as_tree(rows); assert_eq!(sorted[0].id, "agent_parent"); assert_eq!(sorted[1].id, "agent_grandchild"); + assert_eq!(sorted.len(), 2, "tree sort must not drop or duplicate rows"); let summary = SidebarSubagentSummary { cached_total: 2, @@ -5791,7 +5960,7 @@ mod tests { "raw turn UUID must stay out of the compact row: {text:?}" ); - let hover = task_panel_hover_texts(&app, 8); + let hover = task_panel_hover_texts(&app, &task_panel_row_sets(&app), 8); assert!( hover[0].contains("0196f0a3-1111-2222-3333-444455556666"), "full turn UUID must remain available in hover text: {hover:?}" From 18284b28b7f89a58851979e5f518eebfc09f1835 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 14:23:49 +0000 Subject: [PATCH 2/7] perf(tui): take file-tree expand/collapse off the UI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Toggling a directory in the file-tree pane called rebuild(), which re-walked the workspace root plus every expanded subtree with synchronous read_dir on the UI thread — freezing the UI for the duration of the walk on large directories. Only the initial build was offloaded (#399 S3). Collapse now needs no I/O at all: the directory's visible descendants are spliced out of the flat entry list in memory. Descendant expansion state stays in expanded_dirs, so re-expanding the parent restores it, exactly as the full rebuild did. Expand marks the entry expanded immediately (the ▼ acknowledges the keypress), then walks only the newly expanded subtree on a background thread via the same spawn_blocking_supervised + poll-cell pattern as the initial build, splicing the children in when the walk lands. Per-directory sequence numbers plus a collapse-clears-pending rule guarantee a stale walk can never splice into a re-toggled or collapsed node. Without a tokio runtime (plain unit tests) both the initial build and expand walk run synchronously, preserving test semantics. One behavioral delta, deliberate: the old full rebuild re-read every expanded directory on each toggle, so unrelated filesystem changes were incidentally picked up; the splice path only reads the newly expanded subtree. Tests cover splice-vs-full-rebuild parity across expansion orders, I/O-free collapse, descendant-state restoration, stale-result discarding, cursor stability across splices, and the async end-to-end path. Fixes #3900 --- crates/tui/src/tui/file_tree.rs | 398 +++++++++++++++++++++++++++++++- 1 file changed, 386 insertions(+), 12 deletions(-) diff --git a/crates/tui/src/tui/file_tree.rs b/crates/tui/src/tui/file_tree.rs index 34cd1486a8..f2c99a9d41 100644 --- a/crates/tui/src/tui/file_tree.rs +++ b/crates/tui/src/tui/file_tree.rs @@ -4,7 +4,7 @@ //! navigate, Enter expands/collapses directories or inserts `@path` for files, //! Esc closes the pane. -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; @@ -34,6 +34,15 @@ pub struct FileTreeEntry { pub expanded: bool, } +/// An in-flight background expand walk (#3900). The sequence number +/// distinguishes the latest walk for a directory from superseded ones so a +/// stale result can never be spliced in after a re-toggle. +#[derive(Debug, Clone)] +struct PendingExpand { + seq: u64, + cell: Arc>>>, +} + /// Mutable state for the file-tree pane. #[derive(Debug, Clone)] pub struct FileTreeState { @@ -51,13 +60,32 @@ pub struct FileTreeState { pub is_loading: bool, /// Shared cell for async tree-building results (#399 S3). loading_cell: Option>>>>, + /// In-flight expand walks keyed by normalised directory path (#3900). + pending_expands: HashMap, + /// Monotonic counter identifying the latest expand walk per directory. + expand_seq: u64, } impl FileTreeState { /// Build a fresh tree state by walking `workspace`. - /// Spawns the initial walk on a background thread (#399 S3). + /// Spawns the initial walk on a background thread (#399 S3); without a + /// tokio runtime (plain unit tests) the walk runs synchronously. pub fn new(workspace: &Path) -> Self { let expanded_dirs = HashSet::new(); + if tokio::runtime::Handle::try_current().is_err() { + let entries = build_file_tree_inner(workspace, &expanded_dirs, None); + return Self { + entries, + cursor: 0, + scroll_offset: 0, + expanded_dirs, + workspace: workspace.to_path_buf(), + is_loading: false, + loading_cell: None, + pending_expands: HashMap::new(), + expand_seq: 0, + }; + } let loading_cell = Arc::new(Mutex::new(None)); let cell = loading_cell.clone(); let ws = workspace.to_path_buf(); @@ -75,6 +103,8 @@ impl FileTreeState { workspace: workspace.to_path_buf(), is_loading: true, loading_cell: Some(loading_cell), + pending_expands: HashMap::new(), + expand_seq: 0, } } @@ -103,15 +133,23 @@ impl FileTreeState { } } - /// Rebuild the flat entry list from the current `expanded_dirs` set. - /// When loading is in progress, the rebuild is deferred. - pub fn rebuild(&mut self) { - if self.is_loading { - // Defer rebuild until async load completes + /// Poll for background expand-walk results and splice them in. + /// Call from the render loop, after [`Self::poll_loading`] (#3900). + pub fn poll_pending_expands(&mut self) { + if self.pending_expands.is_empty() { return; } - self.entries = build_file_tree_inner(&self.workspace, &self.expanded_dirs, None); - self.clamp_cursor(); + let mut ready: Vec<(PathBuf, u64, Vec)> = Vec::new(); + for (dir, pending) in &self.pending_expands { + if let Ok(mut guard) = pending.cell.lock() + && let Some(children) = guard.take() + { + ready.push((dir.clone(), pending.seq, children)); + } + } + for (dir, seq, children) in ready { + self.apply_expand_result(&dir, seq, children); + } } /// Move the cursor up by one. @@ -140,11 +178,10 @@ impl FileTreeState { if entry.is_dir { let norm = normalize_path(&entry.path); if self.expanded_dirs.contains(&norm) { - self.expanded_dirs.remove(&norm); + self.collapse_dir_at(self.cursor); } else { - self.expanded_dirs.insert(norm); + self.expand_dir_at(self.cursor); } - self.rebuild(); None } else { // Return the path relative to workspace. @@ -158,6 +195,125 @@ impl FileTreeState { } } + /// Collapse the directory at `idx` by splicing its visible descendants + /// out of the flat entry list — no filesystem I/O at all (#3900). + /// + /// Descendant directories stay in `expanded_dirs` so re-expanding the + /// parent restores their expanded state, matching the previous + /// full-rebuild behavior. + fn collapse_dir_at(&mut self, idx: usize) { + let Some(entry) = self.entries.get_mut(idx) else { + return; + }; + if !entry.is_dir { + return; + } + let depth = entry.depth; + entry.expanded = false; + let norm = normalize_path(&entry.path); + self.expanded_dirs.remove(&norm); + // Drop any in-flight expand walk for this directory; its result must + // not splice into a collapsed node. + self.pending_expands.remove(&norm); + + let end = self.entries[idx + 1..] + .iter() + .position(|e| e.depth <= depth) + .map_or(self.entries.len(), |offset| idx + 1 + offset); + let removed = end - (idx + 1); + self.entries.drain(idx + 1..end); + if self.cursor > idx { + self.cursor = if self.cursor < end { + idx + } else { + self.cursor - removed + }; + } + self.clamp_cursor(); + self.clamp_scroll(); + } + + /// Expand the directory at `idx`. The subtree walk runs on a background + /// thread and is spliced in by [`Self::poll_pending_expands`] (#3900); + /// without a tokio runtime (plain unit tests) it runs synchronously. + /// + /// The entry is marked expanded immediately (▼) so the keypress is + /// acknowledged; children appear when the walk completes. + fn expand_dir_at(&mut self, idx: usize) { + let Some(entry) = self.entries.get_mut(idx) else { + return; + }; + if !entry.is_dir { + return; + } + entry.expanded = true; + let dir = entry.path.clone(); + let norm = normalize_path(&dir); + self.expanded_dirs.insert(norm.clone()); + self.expand_seq = self.expand_seq.wrapping_add(1); + let seq = self.expand_seq; + let ws = self.workspace.clone(); + let expanded_snapshot = self.expanded_dirs.clone(); + + let cell = Arc::new(Mutex::new(None)); + self.pending_expands.insert( + norm.clone(), + PendingExpand { + seq, + cell: cell.clone(), + }, + ); + if tokio::runtime::Handle::try_current().is_ok() { + crate::utils::spawn_blocking_supervised("file-tree-expand", move || { + let children = build_file_tree_inner(&ws, &expanded_snapshot, Some(&dir)); + if let Ok(mut guard) = cell.lock() { + *guard = Some(children); + } + }); + } else { + let children = build_file_tree_inner(&ws, &expanded_snapshot, Some(&dir)); + self.apply_expand_result(&norm, seq, children); + } + } + + /// Splice a completed expand walk into the entry list, unless it has + /// been superseded (newer walk for the same directory), the directory + /// was collapsed while the walk was in flight, or the directory is no + /// longer visible (an ancestor collapsed). + fn apply_expand_result(&mut self, dir: &Path, seq: u64, children: Vec) { + let is_current = self + .pending_expands + .get(dir) + .is_some_and(|pending| pending.seq == seq); + if !is_current { + return; + } + self.pending_expands.remove(dir); + if !self.expanded_dirs.contains(dir) { + return; + } + let Some(idx) = self + .entries + .iter() + .position(|e| e.is_dir && normalize_path(&e.path) == *dir) + else { + return; + }; + let depth = self.entries[idx].depth; + // Defensive: never splice a subtree in twice. + if self.entries.get(idx + 1).is_some_and(|e| e.depth > depth) { + return; + } + self.entries[idx].expanded = true; + let inserted = children.len(); + self.entries.splice(idx + 1..idx + 1, children); + if self.cursor > idx { + self.cursor += inserted; + } + self.clamp_cursor(); + self.clamp_scroll(); + } + /// Ensure the cursor is within bounds. fn clamp_cursor(&mut self) { if !self.entries.is_empty() && self.cursor >= self.entries.len() { @@ -294,6 +450,7 @@ pub fn render_file_tree( mode: palette::PaletteMode, ) { state.poll_loading(); + state.poll_pending_expands(); if area.width < FILE_TREE_MIN_WIDTH || area.height < 3 { return; } @@ -370,3 +527,220 @@ pub fn render_file_tree( f.render_widget(section, area); } + +#[cfg(test)] +mod tests { + use super::*; + use std::time::Duration; + + fn fixture_workspace() -> tempfile::TempDir { + let dir = tempfile::TempDir::new().expect("temp dir"); + let root = dir.path(); + std::fs::create_dir_all(root.join("src/nested")).expect("mkdir src/nested"); + std::fs::create_dir_all(root.join("docs")).expect("mkdir docs"); + std::fs::create_dir_all(root.join("node_modules/pkg")).expect("mkdir node_modules"); + std::fs::write(root.join("README.md"), "readme").expect("write README.md"); + std::fs::write(root.join("src/main.rs"), "fn main() {}").expect("write main.rs"); + std::fs::write(root.join("src/Lib.rs"), "").expect("write Lib.rs"); + std::fs::write(root.join("src/nested/mod.rs"), "").expect("write mod.rs"); + std::fs::write(root.join("docs/guide.md"), "").expect("write guide.md"); + dir + } + + fn index_of(state: &FileTreeState, name: &str) -> usize { + state + .entries + .iter() + .position(|e| e.name == name) + .unwrap_or_else(|| panic!("entry {name} missing: {:?}", entry_names(state))) + } + + fn entry_names(state: &FileTreeState) -> Vec { + state.entries.iter().map(|e| e.name.clone()).collect() + } + + fn expand_by_name(state: &mut FileTreeState, name: &str) { + state.cursor = index_of(state, name); + assert!(state.activate().is_none(), "expanding {name} returns None"); + } + + /// The incremental expand path (splice) must produce exactly the flat + /// list the old full rebuild produced, for several expansion orders. + #[test] + fn incremental_expand_matches_full_rebuild() { + let ws = fixture_workspace(); + for order in [["src", "nested", "docs"], ["docs", "src", "nested"]] { + // Plain test: no tokio runtime, so expand walks run synchronously. + let mut state = FileTreeState::new(ws.path()); + assert!(!state.is_loading, "sync fallback builds immediately"); + for name in order { + expand_by_name(&mut state, name); + } + + let oracle = build_file_tree_inner(ws.path(), &state.expanded_dirs, None); + assert_eq!( + state.entries.len(), + oracle.len(), + "entry count parity for order {order:?}: {:?}", + entry_names(&state) + ); + for (spliced, rebuilt) in state.entries.iter().zip(oracle.iter()) { + assert_eq!(spliced.name, rebuilt.name); + assert_eq!(spliced.path, rebuilt.path); + assert_eq!(spliced.is_dir, rebuilt.is_dir); + assert_eq!(spliced.depth, rebuilt.depth); + assert_eq!(spliced.expanded, rebuilt.expanded); + } + } + } + + #[test] + fn collapse_splices_out_subtree_without_io() { + let ws = fixture_workspace(); + let mut state = FileTreeState::new(ws.path()); + expand_by_name(&mut state, "src"); + expand_by_name(&mut state, "nested"); + assert!(state.entries.iter().any(|e| e.name == "mod.rs")); + + // Collapse src: descendants leave the list, nested stays remembered. + let src_idx = index_of(&state, "src"); + state.cursor = src_idx; + assert!(state.activate().is_none()); + + assert!(!state.entries[src_idx].expanded); + assert!(!state.entries.iter().any(|e| e.name == "main.rs")); + assert!(!state.entries.iter().any(|e| e.name == "mod.rs")); + assert_eq!(state.cursor, src_idx, "cursor stays on the collapsed dir"); + let nested_norm = normalize_path(&ws.path().join("src/nested")); + assert!( + state.expanded_dirs.contains(&nested_norm), + "collapsing a parent keeps descendant expansion state" + ); + } + + #[test] + fn re_expand_restores_descendant_expansion() { + let ws = fixture_workspace(); + let mut state = FileTreeState::new(ws.path()); + expand_by_name(&mut state, "src"); + expand_by_name(&mut state, "nested"); + state.cursor = index_of(&state, "src"); + assert!(state.activate().is_none()); // collapse + assert!(state.activate().is_none()); // re-expand + + let nested_idx = index_of(&state, "nested"); + assert!(state.entries[nested_idx].expanded); + assert!( + state.entries.iter().any(|e| e.name == "mod.rs"), + "re-expanding the parent restores the expanded child subtree: {:?}", + entry_names(&state) + ); + } + + #[test] + fn stale_expand_results_are_discarded() { + let ws = fixture_workspace(); + let mut state = FileTreeState::new(ws.path()); + expand_by_name(&mut state, "src"); + let src_norm = normalize_path(&ws.path().join("src")); + + // Collapse removes the pending walk; a result landing afterwards is + // dropped instead of splicing into the collapsed node. + state.cursor = index_of(&state, "src"); + assert!(state.activate().is_none()); + let ghost = vec![FileTreeEntry { + name: "ghost.rs".to_string(), + path: ws.path().join("src/ghost.rs"), + is_dir: false, + depth: 1, + expanded: false, + }]; + state.apply_expand_result(&src_norm, state.expand_seq, ghost.clone()); + assert!(!state.entries.iter().any(|e| e.name == "ghost.rs")); + + // A superseded sequence number is also dropped, and the newer + // pending walk stays registered. + state.pending_expands.insert( + src_norm.clone(), + PendingExpand { + seq: 7, + cell: Arc::new(Mutex::new(None)), + }, + ); + state.expanded_dirs.insert(src_norm.clone()); + state.apply_expand_result(&src_norm, 6, ghost); + assert!(!state.entries.iter().any(|e| e.name == "ghost.rs")); + assert!( + state.pending_expands.contains_key(&src_norm), + "a stale result must not clear the newer pending walk" + ); + } + + #[test] + fn splice_shifts_cursor_positioned_after_the_expanded_dir() { + let ws = fixture_workspace(); + let mut state = FileTreeState::new(ws.path()); + let src_norm = normalize_path(&ws.path().join("src")); + let src_idx = index_of(&state, "src"); + let readme_idx = index_of(&state, "README.md"); + assert!(readme_idx > src_idx); + + state.expanded_dirs.insert(src_norm.clone()); + state.entries[src_idx].expanded = true; + state.pending_expands.insert( + src_norm.clone(), + PendingExpand { + seq: 1, + cell: Arc::new(Mutex::new(None)), + }, + ); + state.cursor = readme_idx; + let children = build_file_tree_inner( + ws.path(), + &state.expanded_dirs, + Some(&ws.path().join("src")), + ); + let inserted = children.len(); + assert!(inserted > 0); + + state.apply_expand_result(&src_norm, 1, children); + + assert_eq!(state.cursor, readme_idx + inserted); + assert_eq!(state.entries[state.cursor].name, "README.md"); + } + + #[tokio::test] + async fn async_expand_splices_children_after_poll() { + let ws = fixture_workspace(); + let mut state = FileTreeState::new(ws.path()); + for _ in 0..500 { + state.poll_loading(); + if !state.is_loading { + break; + } + tokio::time::sleep(Duration::from_millis(5)).await; + } + assert!(!state.is_loading, "initial async build completes"); + + state.cursor = index_of(&state, "src"); + assert!(state.activate().is_none()); + assert!( + state.entries[state.cursor].expanded, + "expand acknowledges the keypress immediately" + ); + + for _ in 0..500 { + state.poll_pending_expands(); + if state.entries.iter().any(|e| e.name == "main.rs") { + break; + } + tokio::time::sleep(Duration::from_millis(5)).await; + } + assert!( + state.entries.iter().any(|e| e.name == "main.rs"), + "background walk results are spliced in on poll: {:?}", + entry_names(&state) + ); + assert!(state.pending_expands.is_empty()); + } +} From a2c86f95be8f8d0c32674da940c931a3bd5264b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 14:30:52 +0000 Subject: [PATCH 3/7] perf(tui): stop deep-cloning visible transcript cells in the collapse path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With tool-run auto-collapse active (the default once any run of >=3 successful low-risk tools exists), ChatWidget::new rebuilt a Vec of every non-collapsed cell on every frame — deep-cloning the entire visible transcript (assistant/user bodies, tool outputs, sub-agent cards) at frame rate even when nothing had changed. The per-cell revision cache underneath never touches an unchanged cell, so the clones were pure waste. The filtered path now borrows cells instead of cloning them: the few synthetic tool-run summary cells are materialized into a stable local Vec first, then the filter builds a Vec<&HistoryCell> over history, active entries, and those summaries. TranscriptViewCache gains an ensure_filtered(&[&HistoryCell], ...) entry point; ensure_split and the new variant both delegate to one private iterator-driven helper, so the render/reuse/invalidation logic stays a single code path and the fast path plus all existing callers are untouched. Filtered sequence, revision values, collapsed_cell_map, and folded-thinking resolution are unchanged, so output is identical — now with zero cell clones on every frame, changed or not. New tests: ensure_filtered/ensure_split output parity, per-cell reuse through ensure_filtered, frame-stability of the collapse path at the ChatWidget level, and a collapsed run spanning the history/active boundary. Fixes #3896 --- crates/tui/src/tui/transcript.rs | 264 +++++++++++++++++++++++------- crates/tui/src/tui/widgets/mod.rs | 112 +++++++++++-- 2 files changed, 302 insertions(+), 74 deletions(-) diff --git a/crates/tui/src/tui/transcript.rs b/crates/tui/src/tui/transcript.rs index 68fa259ec2..2c21eb75e2 100644 --- a/crates/tui/src/tui/transcript.rs +++ b/crates/tui/src/tui/transcript.rs @@ -159,7 +159,56 @@ impl TranscriptViewCache { original_index_map: Option<&[usize]>, ) { let total_cells: usize = cell_shards.iter().map(|s| s.len()).sum(); + self.ensure_iter( + total_cells, + cell_shards.iter().flat_map(|shard| shard.iter()), + cell_revisions, + width, + options, + folded_cells, + original_index_map, + ); + } + + /// `ensure_split` over an already-filtered list of borrowed cells. + /// + /// The collapse path substitutes synthetic tool-run summary cells and + /// skips collapsed cells, so it cannot hand over contiguous shard + /// slices. Accepting `&[&HistoryCell]` lets it pass borrows instead of + /// deep-cloning every visible cell into a fresh `Vec` each + /// frame (#3896). + #[allow(clippy::too_many_arguments)] + pub fn ensure_filtered( + &mut self, + cells: &[&HistoryCell], + cell_revisions: &[u64], + width: u16, + options: TranscriptRenderOptions, + folded_cells: &HashSet, + original_index_map: Option<&[usize]>, + ) { + self.ensure_iter( + cells.len(), + cells.iter().copied(), + cell_revisions, + width, + options, + folded_cells, + original_index_map, + ); + } + #[allow(clippy::too_many_arguments)] + fn ensure_iter<'a>( + &mut self, + total_cells: usize, + cells: impl Iterator, + cell_revisions: &[u64], + width: u16, + options: TranscriptRenderOptions, + folded_cells: &HashSet, + original_index_map: Option<&[usize]>, + ) { let layout_changed = self.width != width || self.options != options; let folded_changed = self.folded_cells != *folded_cells; if layout_changed || folded_changed { @@ -183,71 +232,69 @@ impl TranscriptViewCache { let revisions_match = cell_revisions.len() == total_cells; let mut idx: usize = 0; - for shard in cell_shards { - for cell in *shard { - let current_rev = if revisions_match { - cell_revisions[idx] - } else { - // No matching revisions — force a re-render this cycle. - u64::MAX - }; - - // Reuse cached entry if the revision matches AND it's at the - // same index (cells can shift on insert/remove, so we only - // reuse when the index is identical — a stricter invariant - // codex also uses for its active-cell tail). - if let Some(prev) = self.per_cell.get(idx) - && !layout_changed - && prev.revision == current_rev - && revisions_match - { - new_per_cell.push(prev.clone()); - idx += 1; - continue; - } - - any_dirty = true; - first_dirty = Some(first_dirty.map_or(idx, |current| current.min(idx))); - let is_tool_groupable = matches!(cell, HistoryCell::Tool(_)); - let render_width = if is_tool_groupable { - width.saturating_sub(2).max(1) - } else { - width - }; - let original_idx = original_index_map - .map(|m| *m.get(idx).unwrap_or(&idx)) - .unwrap_or(idx); - let folded = folded_cells.contains(&original_idx); - let rendered = cell.lines_with_copy_metadata_folded(render_width, options, folded); - let mut lines = Vec::with_capacity(rendered.len()); - let mut copy_separators = Vec::with_capacity(rendered.len()); - let mut copy_prefix_widths = Vec::with_capacity(rendered.len()); - for rendered_line in rendered { - lines.push(rendered_line.line); - copy_prefix_widths.push(rendered_line.copy_prefix_width); - copy_separators.push(rendered_line.copy_separator_after); - } - let is_empty = lines.is_empty(); - new_per_cell.push(CachedCell { - revision: current_rev, - lines: Arc::new(lines), - copy_separators: Arc::new(copy_separators), - copy_prefix_widths: Arc::new(copy_prefix_widths), - is_empty, - is_stream_continuation: cell.is_stream_continuation(), - is_conversational: cell.is_conversational(), - is_system_or_tool: matches!( - cell, - HistoryCell::System { .. } - | HistoryCell::Error { .. } - | HistoryCell::Tool(_) - | HistoryCell::SubAgent(_) - | HistoryCell::ArchivedContext { .. } - ), - is_tool_groupable, - }); + for cell in cells { + let current_rev = if revisions_match { + cell_revisions[idx] + } else { + // No matching revisions — force a re-render this cycle. + u64::MAX + }; + + // Reuse cached entry if the revision matches AND it's at the + // same index (cells can shift on insert/remove, so we only + // reuse when the index is identical — a stricter invariant + // codex also uses for its active-cell tail). + if let Some(prev) = self.per_cell.get(idx) + && !layout_changed + && prev.revision == current_rev + && revisions_match + { + new_per_cell.push(prev.clone()); idx += 1; + continue; + } + + any_dirty = true; + first_dirty = Some(first_dirty.map_or(idx, |current| current.min(idx))); + let is_tool_groupable = matches!(cell, HistoryCell::Tool(_)); + let render_width = if is_tool_groupable { + width.saturating_sub(2).max(1) + } else { + width + }; + let original_idx = original_index_map + .map(|m| *m.get(idx).unwrap_or(&idx)) + .unwrap_or(idx); + let folded = folded_cells.contains(&original_idx); + let rendered = cell.lines_with_copy_metadata_folded(render_width, options, folded); + let mut lines = Vec::with_capacity(rendered.len()); + let mut copy_separators = Vec::with_capacity(rendered.len()); + let mut copy_prefix_widths = Vec::with_capacity(rendered.len()); + for rendered_line in rendered { + lines.push(rendered_line.line); + copy_prefix_widths.push(rendered_line.copy_prefix_width); + copy_separators.push(rendered_line.copy_separator_after); } + let is_empty = lines.is_empty(); + new_per_cell.push(CachedCell { + revision: current_rev, + lines: Arc::new(lines), + copy_separators: Arc::new(copy_separators), + copy_prefix_widths: Arc::new(copy_prefix_widths), + is_empty, + is_stream_continuation: cell.is_stream_continuation(), + is_conversational: cell.is_conversational(), + is_system_or_tool: matches!( + cell, + HistoryCell::System { .. } + | HistoryCell::Error { .. } + | HistoryCell::Tool(_) + | HistoryCell::SubAgent(_) + | HistoryCell::ArchivedContext { .. } + ), + is_tool_groupable, + }); + idx += 1; } self.per_cell = new_per_cell; @@ -1054,6 +1101,97 @@ mod tests { eprintln!(" ✓ well under 1 MB even for very long sessions"); } + #[test] + fn ensure_filtered_matches_ensure_split_output() { + let cells = vec![ + user_cell("hello"), + assistant_cell("some **markdown** body", false), + exec_tool_cell("cargo test"), + user_cell("again"), + ]; + let revisions = vec![1u64, 2, 3, 4]; + let index_map: Vec = vec![0, 1, 2, 3]; + + let mut split_cache = TranscriptViewCache::new(); + split_cache.ensure_split( + &[&cells], + &revisions, + 40, + TranscriptRenderOptions::default(), + &HashSet::new(), + Some(&index_map), + ); + + let refs: Vec<&HistoryCell> = cells.iter().collect(); + let mut filtered_cache = TranscriptViewCache::new(); + filtered_cache.ensure_filtered( + &refs, + &revisions, + 40, + TranscriptRenderOptions::default(), + &HashSet::new(), + Some(&index_map), + ); + + assert_eq!(plain_lines(&split_cache), plain_lines(&filtered_cache)); + assert_eq!( + split_cache.line_meta().len(), + filtered_cache.line_meta().len() + ); + } + + #[test] + fn ensure_filtered_reuses_unchanged_cells() { + let cells = vec![ + user_cell("hello"), + assistant_cell("streaming", true), + user_cell("again"), + ]; + let mut revisions = vec![1u64, 1, 1]; + let refs: Vec<&HistoryCell> = cells.iter().collect(); + + let mut cache = TranscriptViewCache::new(); + cache.ensure_filtered( + &refs, + &revisions, + 80, + TranscriptRenderOptions::default(), + &HashSet::new(), + None, + ); + let first = plain_lines(&cache); + + cache.ensure_filtered( + &refs, + &revisions, + 80, + TranscriptRenderOptions::default(), + &HashSet::new(), + None, + ); + assert_eq!(first, plain_lines(&cache)); + for (idx, cached) in cache.per_cell.iter().enumerate() { + assert_eq!( + cached.revision, 1, + "cell {idx} must be reused, not re-rendered" + ); + } + + // Bump one revision: only that entry re-renders. + revisions[1] = 2; + cache.ensure_filtered( + &refs, + &revisions, + 80, + TranscriptRenderOptions::default(), + &HashSet::new(), + None, + ); + assert_eq!(cache.per_cell[0].revision, 1); + assert_eq!(cache.per_cell[1].revision, 2); + assert_eq!(cache.per_cell[2].revision, 1); + } + #[test] fn folded_thinking_cache_invalidation() { let long_content = "reasoning line\n".repeat(50); diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index e44b6608ab..8134eac8c9 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -185,10 +185,26 @@ impl ChatWidget { None, ); } else { - // Slow path: clone non-collapsed cells into filtered vecs so - // collapsed cells are excluded from rendering. Build the - // filtered→original index mapping. - let mut filtered_cells: Vec = + // Slow path: borrow non-collapsed cells into a filtered ref list + // so collapsed cells are excluded from rendering, and build the + // filtered→original index mapping. Collapsed run starts render a + // synthetic summary cell; those few summaries are materialized + // up front so the ref list can borrow from a stable Vec — + // avoiding the per-frame deep clone of every visible cell that + // this path used to pay (#3896). + let summary_cells: Vec<(usize, HistoryCell)> = tool_runs + .iter() + .filter(|run| collapsed_run_starts.contains(&run.start)) + .map(|run| (run.start, tool_run_summary_cell(run))) + .collect(); + let summary_cell_for = |idx: usize| -> Option<&HistoryCell> { + summary_cells + .iter() + .find(|(start, _)| *start == idx) + .map(|(_, cell)| cell) + }; + + let mut filtered_cells: Vec<&HistoryCell> = Vec::with_capacity(history_len + active_entries.len()); let mut filtered_revs: Vec = Vec::with_capacity(history_len + active_entries.len()); @@ -206,7 +222,7 @@ impl ChatWidget { .iter() .find(|run| run.start == idx && collapsed_run_starts.contains(&idx)) { - filtered_cells.push(tool_run_summary_cell(run)); + filtered_cells.push(summary_cell_for(idx).expect("summary cell materialized")); filtered_revs.push(tool_run_summary_revision( run, &app.history_revisions, @@ -216,7 +232,7 @@ impl ChatWidget { filtered_to_original.push(idx); continue; } - filtered_cells.push(cell.clone()); + filtered_cells.push(cell); filtered_revs.push(app.history_revisions[idx]); filtered_to_original.push(idx); } @@ -234,7 +250,8 @@ impl ChatWidget { if let Some(run) = tool_runs.iter().find(|run| { run.start == original_idx && collapsed_run_starts.contains(&original_idx) }) { - filtered_cells.push(tool_run_summary_cell(run)); + filtered_cells + .push(summary_cell_for(original_idx).expect("summary materialized")); filtered_revs.push(tool_run_summary_revision( run, &app.history_revisions, @@ -244,7 +261,7 @@ impl ChatWidget { filtered_to_original.push(original_idx); continue; } - filtered_cells.push(cell.clone()); + filtered_cells.push(cell); let salt = (i as u64).wrapping_add(1); filtered_revs.push(active_entry_revision(active_rev, salt)); filtered_to_original.push(original_idx); @@ -253,9 +270,8 @@ impl ChatWidget { app.collapsed_cell_map = filtered_to_original; - let shards: [&[HistoryCell]; 1] = [&filtered_cells]; - app.viewport.transcript_cache.ensure_split( - &shards, + app.viewport.transcript_cache.ensure_filtered( + &filtered_cells, &filtered_revs, content_area.width.max(1), render_options, @@ -3541,6 +3557,80 @@ mod tests { assert_eq!(app.collapsed_cell_map, vec![0, 1, 2]); } + #[test] + fn chat_widget_collapse_path_stable_across_frames() { + let mut app = create_test_app(); + app.tool_collapse_mode = ToolCollapseMode::Compact; + app.tool_collapse_threshold = 3; + add_dense_tool_run(&mut app); + app.add_message(HistoryCell::User { + content: "trailing prompt".to_string(), + }); + + let area = Rect { + x: 0, + y: 0, + width: 80, + height: 10, + }; + + let mut first_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut first_buf); + let first = buffer_text(&first_buf, area); + let first_map = app.collapsed_cell_map.clone(); + let first_total = app.viewport.last_transcript_total; + + // Second frame without any app mutation: the borrowed filtered path + // must reproduce the identical output and index map. + let mut second_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut second_buf); + let second = buffer_text(&second_buf, area); + + assert_eq!(first, second, "collapse path is frame-stable"); + assert_eq!(first_map, app.collapsed_cell_map); + assert_eq!(first_total, app.viewport.last_transcript_total); + assert!(first.contains("Explored 2 files, 1 search"), "{first}"); + assert!(first.contains("trailing prompt"), "{first}"); + } + + #[test] + fn chat_widget_collapses_run_spanning_history_and_active_entries() { + let mut app = create_test_app(); + app.tool_collapse_mode = ToolCollapseMode::Compact; + app.tool_collapse_threshold = 3; + app.add_message(success_tool_cell("read_file")); + app.add_message(success_tool_cell("list_dir")); + let active = app.active_cell.get_or_insert_with(ActiveCell::new); + active.push_untracked(success_tool_cell("web_search")); + app.bump_active_cell_revision(); + + let area = Rect { + x: 0, + y: 0, + width: 80, + height: 8, + }; + let mut buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut buf); + let rendered = buffer_text(&buf, area); + + assert_eq!(app.collapsed_cell_map, vec![0]); + assert!( + rendered.contains("Explored 2 files, 1 search"), + "run spanning the history/active boundary renders one summary: {rendered}" + ); + + // Mutating the active tail must re-render the summary (its revision + // folds in the covered active entries). + let rev_before = app.active_cell_revision; + app.bump_active_cell_revision(); + assert_ne!(rev_before, app.active_cell_revision); + let mut second_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut second_buf); + let second = buffer_text(&second_buf, area); + assert!(second.contains("Explored 2 files, 1 search"), "{second}"); + } + #[test] fn pad_lines_to_bottom_noop_when_already_filled() { let mut lines = vec![Line::from("one"), Line::from("two")]; From 0fc32aa5413f6f2b6c2aaaefa7f5ab75b26c3830 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 14:44:06 +0000 Subject: [PATCH 4/7] perf(tui): move @mention completion walks off the UI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The completion cache keys on the exact partial, so every keystroke that changed the needle missed and ran a fresh synchronous workspace walk on the UI thread — a gitignore-aware walk plus, for path-like needles ('.', '/', '\\'), the gitignore-disabled local-reference walk capped at 4096 paths (the same walk behind the #1921 WSL2 freezes). Typing a path-like @mention in a large repo stalled the input loop on every keystroke. The walk now runs on a background thread (spawn_blocking_supervised + shared-cell poll, same shape as the file-tree build and workspace-context refresh): the cache key is factored into MentionCompletionKey, at most one walk is in flight, and the latest keystroke wins — a completed walk for an older needle is kept only as the transient popup fallback under its true key, never returned as the live needle's result. While a walk is in flight the popup keeps showing the previous keystroke's entries (same session context only) so it never closes mid-word and Enter keeps routing to the popup instead of submitting the message. poll_mention_completion drains finished walks once per event-loop iteration, sets needs_redraw only when a result actually landed, and drops orphaned walks when the cursor leaves the mention token. Without a tokio runtime the walk runs synchronously, so completion results for a given input are byte-identical to before and the existing plain #[test] suite (including the cursor-move cache-reuse and freshness pins) passes unchanged. Tab completion (try_autocomplete_file_mention) stays synchronous: a deliberate one-shot action, unlike per-keystroke popup refreshes. Fixes #3899 --- crates/tui/src/tui/app.rs | 46 ++++++- crates/tui/src/tui/file_mention.rs | 158 +++++++++++++++++++----- crates/tui/src/tui/ui.rs | 3 + crates/tui/src/tui/ui/tests.rs | 185 ++++++++++++++++++++++++++++- 4 files changed, 354 insertions(+), 38 deletions(-) diff --git a/crates/tui/src/tui/app.rs b/crates/tui/src/tui/app.rs index 2f414e272f..286aa4553b 100644 --- a/crates/tui/src/tui/app.rs +++ b/crates/tui/src/tui/app.rs @@ -1201,10 +1201,11 @@ impl VimMode { } } -/// Cached @-mention completion results to avoid re-walking the filesystem when -/// the cursor moves inside the same mention token. -#[derive(Debug, Clone)] -pub struct MentionCompletionCache { +/// Everything an @-mention completion walk depends on. Two walks with equal +/// keys produce identical results, so the key doubles as the cache/invalidation +/// identity for both the completed cache and the in-flight background walk. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct MentionCompletionKey { /// Workspace root used for this completion walk. pub workspace: PathBuf, /// Process cwd captured for cwd-relative completion entries. @@ -1222,10 +1223,43 @@ pub struct MentionCompletionCache { /// Whether symlink following was enabled for this completion walk. /// Included so live config changes invalidate cached popup results. pub follow_links: bool, +} + +impl MentionCompletionKey { + /// Whether `other` differs from `self` only in the typed partial — + /// i.e. the same session/config context at a different keystroke. + #[must_use] + pub fn same_context(&self, other: &Self) -> bool { + self.workspace == other.workspace + && self.cwd == other.cwd + && self.limit == other.limit + && self.walk_depth == other.walk_depth + && self.behavior == other.behavior + && self.follow_links == other.follow_links + } +} + +/// Cached @-mention completion results to avoid re-walking the filesystem when +/// the cursor moves inside the same mention token. +#[derive(Debug, Clone)] +pub struct MentionCompletionCache { + /// The walk inputs that produced `entries`. + pub key: MentionCompletionKey, /// Cached completion entries. pub entries: Vec, } +/// An @-mention completion walk running on a background thread (#3899). +/// The result lands in `cell` and is promoted (or discarded, if the needle +/// moved on) by `visible_mention_menu_entries` / `poll_mention_completion`. +#[derive(Debug, Clone)] +pub struct MentionCompletionPending { + /// The walk inputs the background thread was started with. + pub key: MentionCompletionKey, + /// Shared cell the background walk writes its entries into. + pub cell: std::sync::Arc>>>, +} + /// Composer input state — grouped fields for the text input area. pub struct ComposerState { /// Current composer text content. @@ -1257,6 +1291,8 @@ pub struct ComposerState { /// Cached @-mention completions to avoid re-walking the filesystem when /// the cursor moves inside the same mention token. pub mention_completion_cache: Option, + /// In-flight background @-mention completion walk, if any (#3899). + pub mention_completion_pending: Option, /// Whether vim modal editing is enabled for this composer. /// Sourced from `Settings::composer_vim_mode` at startup. pub vim_enabled: bool, @@ -1293,6 +1329,7 @@ impl Default for ComposerState { mention_menu_selected: 0, mention_menu_hidden: false, mention_completion_cache: None, + mention_completion_pending: None, vim_enabled: false, vim_mode: VimMode::Normal, vim_pending_d: false, @@ -2573,6 +2610,7 @@ impl App { mention_menu_selected: 0, mention_menu_hidden: false, mention_completion_cache: None, + mention_completion_pending: None, vim_enabled: composer_vim_enabled, vim_mode: VimMode::Normal, vim_pending_d: false, diff --git a/crates/tui/src/tui/file_mention.rs b/crates/tui/src/tui/file_mention.rs index 9c9128fb8b..5890db10e4 100644 --- a/crates/tui/src/tui/file_mention.rs +++ b/crates/tui/src/tui/file_mention.rs @@ -27,7 +27,9 @@ use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; -use crate::tui::app::{App, MentionCompletionCache}; +use crate::tui::app::{ + App, MentionCompletionCache, MentionCompletionKey, MentionCompletionPending, +}; use crate::working_set::Workspace; /// Maximum number of `@`-mentions whose contents are inlined into one user @@ -219,47 +221,139 @@ pub fn visible_mention_menu_entries(app: &mut App, limit: usize) -> Vec return Vec::new(); } - let workspace = app.workspace.clone(); - let cwd = std::env::current_dir().ok(); - let walk_depth = app.mention_walk_depth; - let behavior = app.mention_menu_behavior.clone(); - let follow_links = app.workspace_follow_symlinks; + let key = MentionCompletionKey { + workspace: app.workspace.clone(), + cwd: std::env::current_dir().ok(), + partial, + limit, + walk_depth: app.mention_walk_depth, + behavior: app.mention_menu_behavior.clone(), + follow_links: app.workspace_follow_symlinks, + }; + if let Some(ref cache) = app.composer.mention_completion_cache - && cache.workspace == workspace - && cache.cwd == cwd - && cache.partial == partial - && cache.limit == limit - && cache.walk_depth == walk_depth - && cache.behavior == behavior - && cache.follow_links == follow_links + && cache.key == key { return cache.entries.clone(); } + // Drain a completed background walk: promote it when it still matches + // the live key, discard it when the needle has moved on (#3899). + if let Some(pending) = app.composer.mention_completion_pending.take() { + let ready = pending.cell.lock().ok().and_then(|mut cell| cell.take()); + match ready { + Some(entries) => { + let promoted = pending.key == key; + app.composer.mention_completion_cache = Some(MentionCompletionCache { + key: pending.key, + entries: entries.clone(), + }); + if promoted { + return entries; + } + } + None => { + // Still walking. Keep it only if it is for the current key; + // a stale-needle walk is dropped so the respawn below starts + // the walk for the fresh needle (latest keystroke wins, at + // most one walk in flight). + if pending.key == key { + app.composer.mention_completion_pending = Some(pending); + return stale_mention_fallback_entries(app, &key); + } + } + } + } + + // Cache miss with no matching walk in flight: start one. Without a tokio + // runtime (plain unit tests) fall back to the synchronous walk so + // results and test semantics are identical to the pre-async behavior. + if tokio::runtime::Handle::try_current().is_ok() { + let cell = std::sync::Arc::new(std::sync::Mutex::new(None)); + let walker_cell = cell.clone(); + let walker_key = key.clone(); + crate::utils::spawn_blocking_supervised("mention-completion", move || { + let entries = run_mention_completion_walk(&walker_key); + if let Ok(mut guard) = walker_cell.lock() { + *guard = Some(entries); + } + }); + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key: key.clone(), + cell, + }); + stale_mention_fallback_entries(app, &key) + } else { + let entries = run_mention_completion_walk(&key); + app.composer.mention_completion_cache = Some(MentionCompletionCache { + key, + entries: entries.clone(), + }); + entries + } +} + +/// Run the completion walk described by `key`. Called on a background thread +/// for live keystrokes and synchronously when no runtime is available. +fn run_mention_completion_walk(key: &MentionCompletionKey) -> Vec { let ws = Workspace::with_cwd_depth_and_follow_links( - workspace.clone(), - cwd.clone(), - walk_depth, - app.workspace_follow_symlinks, + key.workspace.clone(), + key.cwd.clone(), + key.walk_depth, + key.follow_links, ); - let entries = if behavior == "browser" { - find_file_mention_browser_completions(&ws, &partial, limit) + if key.behavior == "browser" { + find_file_mention_browser_completions(&ws, &key.partial, key.limit) } else { - find_file_mention_completions(&ws, &partial, limit) - }; + find_file_mention_completions(&ws, &key.partial, key.limit) + } +} - app.composer.mention_completion_cache = Some(MentionCompletionCache { - workspace, - cwd, - partial, - limit, - walk_depth, - behavior, - follow_links, - entries: entries.clone(), - }); +/// While a walk is in flight, keep showing the previous keystroke's entries +/// so the popup stays open and Enter/Up/Down keep routing to it — but only +/// when the cached results come from the same session context (an older +/// partial is fine; a different workspace/behavior/depth is not). +fn stale_mention_fallback_entries(app: &App, key: &MentionCompletionKey) -> Vec { + app.composer + .mention_completion_cache + .as_ref() + .filter(|cache| cache.key.same_context(key)) + .map(|cache| cache.entries.clone()) + .unwrap_or_default() +} - entries +/// Drain a completed background completion walk outside the render path. +/// Called once per event-loop iteration (next to the workspace-context +/// refresh) so a finished walk repaints the popup without waiting for an +/// input event. Sets `needs_redraw` only when the drained result changes +/// what the popup would show — never while merely waiting (#3899). +pub fn poll_mention_completion(app: &mut App) { + if app.composer.mention_completion_pending.is_none() { + return; + } + // Cursor left the mention token: the walk result can never be shown. + if app.mention_menu_hidden + || partial_file_mention_at_cursor(&app.input, app.cursor_position).is_none() + { + app.composer.mention_completion_pending = None; + return; + } + let Some(pending) = app.composer.mention_completion_pending.take() else { + return; + }; + let ready = pending.cell.lock().ok().and_then(|mut cell| cell.take()); + match ready { + Some(entries) => { + app.composer.mention_completion_cache = Some(MentionCompletionCache { + key: pending.key, + entries, + }); + app.needs_redraw = true; + } + None => { + app.composer.mention_completion_pending = Some(pending); + } + } } /// Apply the currently selected `@`-mention popup entry to the composer diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index c014b82cdd..1037c51d78 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -3288,6 +3288,9 @@ async fn run_event_loop( let allow_workspace_context_refresh = !app.is_loading && !has_running_agents && !app.is_compacting && !app.is_purging; workspace_context::refresh_if_needed(app, now, allow_workspace_context_refresh); + // Drain any completed background @mention completion walk so the + // popup repaints as soon as results are ready (#3899). + crate::tui::file_mention::poll_mention_completion(app); // Draw is gated by the frame-rate limiter (120 FPS cap). When a // redraw is needed but the limiter says we're inside the cooldown diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index a32ff6890f..5c55d312aa 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -6,10 +6,14 @@ use crate::config::{ use crate::config_ui::{self, WebConfigSession, WebConfigSessionEvent}; use crate::core::engine::mock_engine_handle; use crate::tui::active_cell::ActiveCell; -use crate::tui::app::{ReasoningEffort, SidebarHoverRow, SidebarHoverSection, ToolDetailRecord}; +use crate::tui::app::{ + MentionCompletionKey, MentionCompletionPending, ReasoningEffort, SidebarHoverRow, + SidebarHoverSection, ToolDetailRecord, +}; use crate::tui::file_mention::{ apply_mention_menu_selection, find_file_mention_completions, partial_file_mention_at_cursor, - try_autocomplete_file_mention, user_request_with_file_mentions, visible_mention_menu_entries, + poll_mention_completion, try_autocomplete_file_mention, user_request_with_file_mentions, + visible_mention_menu_entries, }; use crate::tui::footer_ui::{ active_tool_status_label, footer_auxiliary_spans, footer_balance_spans, footer_cache_spans, @@ -7910,6 +7914,183 @@ fn apply_mention_menu_selection_is_noop_outside_a_mention() { assert_eq!(app.input, "no @ here"); } +// ---- #3899: background @-mention completion walk ---- + +fn mention_key_for( + app: &crate::tui::app::App, + partial: &str, + limit: usize, +) -> MentionCompletionKey { + MentionCompletionKey { + workspace: app.workspace.clone(), + cwd: std::env::current_dir().ok(), + partial: partial.to_string(), + limit, + walk_depth: app.mention_walk_depth, + behavior: app.mention_menu_behavior.clone(), + follow_links: app.workspace_follow_symlinks, + } +} + +#[test] +fn mention_completion_sync_fallback_fills_cache_without_runtime() { + let tmpdir = TempDir::new().expect("tempdir"); + std::fs::create_dir_all(tmpdir.path().join("docs")).unwrap(); + std::fs::write(tmpdir.path().join("docs/alpha.md"), "x").unwrap(); + + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + + // Plain test — no tokio runtime, so the walk runs synchronously and the + // result is cached immediately with nothing left in flight. + let entries = visible_mention_menu_entries(&mut app, 6); + assert!(entries.iter().any(|e| e == "docs/alpha.md"), "{entries:?}"); + assert!(app.composer.mention_completion_pending.is_none()); + assert!(app.composer.mention_completion_cache.is_some()); +} + +#[tokio::test] +async fn mention_completion_walk_runs_off_thread_and_promotes() { + let tmpdir = TempDir::new().expect("tempdir"); + std::fs::create_dir_all(tmpdir.path().join("docs")).unwrap(); + std::fs::write(tmpdir.path().join("docs/alpha.md"), "x").unwrap(); + + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + + // First call starts a background walk; with a cold cache the stale + // fallback is empty, but the pending walk must be registered. + let first = visible_mention_menu_entries(&mut app, 6); + assert!(first.is_empty(), "cold cache returns the empty fallback"); + assert!(app.composer.mention_completion_pending.is_some()); + + let mut entries = Vec::new(); + for _ in 0..400 { + entries = visible_mention_menu_entries(&mut app, 6); + if !entries.is_empty() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + assert!( + entries.iter().any(|e| e == "docs/alpha.md"), + "background walk result is promoted for the live key: {entries:?}" + ); + assert!(app.composer.mention_completion_pending.is_none()); +} + +#[tokio::test] +async fn mention_completion_stale_needle_walk_is_replaced() { + let tmpdir = TempDir::new().expect("tempdir"); + std::fs::create_dir_all(tmpdir.path().join("docs")).unwrap(); + std::fs::write(tmpdir.path().join("docs/alpha.md"), "x").unwrap(); + + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + + // Seed a completed walk for an older needle. + let stale_key = mention_key_for(&app, "old", 6); + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key: stale_key, + cell: std::sync::Arc::new(std::sync::Mutex::new(Some(vec![ + "stale-entry.md".to_string(), + ]))), + }); + + let entries = visible_mention_menu_entries(&mut app, 6); + // The completed old-needle walk serves as the transient anti-flicker + // fallback (the popup must not close mid-word), but it is stored under + // its true key — never promoted as the live needle's result — and a + // fresh walk is spawned for the live key. + assert_eq!(entries, vec!["stale-entry.md".to_string()]); + let cached = app + .composer + .mention_completion_cache + .as_ref() + .expect("stale result cached under its own key"); + assert_eq!(cached.key.partial, "old"); + let pending = app + .composer + .mention_completion_pending + .as_ref() + .expect("a fresh walk is spawned for the live key"); + assert_eq!(pending.key.partial, "docs/"); + + // Once the fresh walk lands, the live needle's results replace the + // fallback. + let mut entries = Vec::new(); + for _ in 0..400 { + entries = visible_mention_menu_entries(&mut app, 6); + if entries.iter().any(|e| e == "docs/alpha.md") { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + assert!( + entries.iter().any(|e| e == "docs/alpha.md"), + "fresh walk replaces the stale fallback: {entries:?}" + ); +} + +#[test] +fn poll_mention_completion_promotes_once_and_sets_needs_redraw() { + let tmpdir = TempDir::new().expect("tempdir"); + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + + let key = mention_key_for(&app, "docs/", 6); + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key, + cell: std::sync::Arc::new(std::sync::Mutex::new(Some(vec![ + "docs/alpha.md".to_string(), + ]))), + }); + + app.needs_redraw = false; + poll_mention_completion(&mut app); + assert!(app.needs_redraw, "a drained result schedules a repaint"); + assert!(app.composer.mention_completion_pending.is_none()); + assert!(app.composer.mention_completion_cache.is_some()); + + // Nothing left to drain: the poller must not busy-repaint. + app.needs_redraw = false; + poll_mention_completion(&mut app); + assert!(!app.needs_redraw); +} + +#[test] +fn poll_mention_completion_clears_pending_when_cursor_leaves_mention() { + let tmpdir = TempDir::new().expect("tempdir"); + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + let key = mention_key_for(&app, "docs/", 6); + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key, + cell: std::sync::Arc::new(std::sync::Mutex::new(None)), + }); + + app.input = "no mention anymore".to_string(); + app.cursor_position = app.input.chars().count(); + + app.needs_redraw = false; + poll_mention_completion(&mut app); + assert!( + app.composer.mention_completion_pending.is_none(), + "an orphaned walk must not keep the loop polling forever" + ); + assert!(!app.needs_redraw); +} + #[test] fn apply_mention_menu_selection_with_no_entries_is_noop() { let mut app = create_test_app(); From 5ee6893b97e1f5b2044637790e5a1b207190ddf9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 14:57:56 +0000 Subject: [PATCH 5/7] perf(tui): render streaming markdown incrementally instead of re-parsing the whole message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While a message streams, every committed chunk bumped the cell's revision and re-rendered the entire growing content: a full markdown parse plus a full word-wrap/span build, O(N) per chunk and O(N²/chunk) over the life of the stream. During an active stream chunks commit on nearly every 24ms loop tick, so long answers got visibly slower to render as they grew. The Ctrl+T overlay doubled the cost at a second width. The parser makes prefix stability provable: classification is per line with a single carried bool (the code-fence state), so appended bytes can never re-classify an earlier complete line — only the partial trailing line is volatile. Rendering is per block except consecutive table rows, whose column widths derive from the whole group, so a trailing table run stays volatile until a non-table block closes it. render_markdown_tagged_streaming keeps a small thread-local LRU of StreamRenderState slots (main transcript + overlay widths + a thinking body can interleave): per call it verifies the committed prefix with one memcmp, parses only newly completed lines, freezes their rendered lines, and re-renders just the volatile tail. Parse and render go through the same helpers as the full path (parse_line_into, render_blocks_tagged), and a debug_assert differential guard compares against the full re-parse on every call in debug builds, so the whole existing suite doubles as a byte-identity test. Streaming assistant cells route through the new path via render_assistant_message*; the final render after streaming ends takes the plain full-parse path, so finished messages are a full re-parse by construction. New tests: per-chunk byte-identity across chunk sizes 1/3/7 and widths 24/40/80 over headings/lists/fences/tables/links/CJK, an O(delta) parsed-line-count guard, non-extension fallback, two-width interleaving, trailing-table volatility, and char-by-char unclosed fences. Fixes #3897 --- crates/tui/src/tui/history.rs | 38 +- crates/tui/src/tui/history/message.rs | 52 ++- crates/tui/src/tui/markdown_render.rs | 627 ++++++++++++++++++++------ 3 files changed, 557 insertions(+), 160 deletions(-) diff --git a/crates/tui/src/tui/history.rs b/crates/tui/src/tui/history.rs index dfbcb67b2e..2e0da86212 100644 --- a/crates/tui/src/tui/history.rs +++ b/crates/tui/src/tui/history.rs @@ -42,7 +42,8 @@ use constants::{ use constants::{TOOL_RUNNING_SYMBOLS, TOOL_STATUS_SYMBOL_MS}; use message::{ RenderedTranscriptLine, assistant_label_style_for, hard_break_copy_lines, message_body_style, - render_message, render_message_with_copy_metadata, render_plain_message, render_user_message, + render_assistant_message, render_assistant_message_with_copy_metadata, render_message, + render_message_with_copy_metadata, render_plain_message, render_user_message, system_body_style, system_label_style, user_body_style, user_label_style, }; use thinking::{render_hidden_thinking_activity, render_thinking}; @@ -184,13 +185,9 @@ impl HistoryCell { pub fn lines(&self, width: u16) -> Vec> { match self { HistoryCell::User { content } => render_user_message(content, width), - HistoryCell::Assistant { content, streaming } => render_message( - ASSISTANT_GLYPH, - assistant_label_style_for(*streaming, /*low_motion*/ false), - message_body_style(), - content, - width, - ), + HistoryCell::Assistant { content, streaming } => { + render_assistant_message(content, width, *streaming, /*low_motion*/ false) + } HistoryCell::System { content } => { if is_cycle_boundary(content) { render_cycle_boundary(content, width) @@ -307,13 +304,9 @@ impl HistoryCell { } HistoryCell::Tool(cell) => cell.lines_with_motion(width, options.low_motion), HistoryCell::User { content } => render_user_message(content, width), - HistoryCell::Assistant { content, streaming } => render_message( - ASSISTANT_GLYPH, - assistant_label_style_for(*streaming, options.low_motion), - message_body_style(), - content, - width, - ), + HistoryCell::Assistant { content, streaming } => { + render_assistant_message(content, width, *streaming, options.low_motion) + } HistoryCell::System { .. } | HistoryCell::Error { .. } => self.lines(width), HistoryCell::SubAgent(cell) => cell.lines(width), HistoryCell::ArchivedContext { .. } => { @@ -341,13 +334,14 @@ impl HistoryCell { HistoryCell::User { content } => { hard_break_copy_lines(render_user_message(content, width)) } - HistoryCell::Assistant { content, streaming } => render_message_with_copy_metadata( - ASSISTANT_GLYPH, - assistant_label_style_for(*streaming, options.low_motion), - message_body_style(), - content, - width, - ), + HistoryCell::Assistant { content, streaming } => { + render_assistant_message_with_copy_metadata( + content, + width, + *streaming, + options.low_motion, + ) + } HistoryCell::System { content } if !is_cycle_boundary(content) => { render_message_with_copy_metadata( "Note", diff --git a/crates/tui/src/tui/history/message.rs b/crates/tui/src/tui/history/message.rs index 5089a33ede..002c29bfff 100644 --- a/crates/tui/src/tui/history/message.rs +++ b/crates/tui/src/tui/history/message.rs @@ -29,12 +29,57 @@ pub(super) fn render_message( .collect() } +/// Render an assistant cell for the live transcript. While the cell is +/// streaming, the markdown parse/wrap goes through the incremental streaming +/// cache so each committed chunk costs O(appended source), not O(whole +/// message) (#3897); the final render after streaming ends takes the plain +/// full-parse path, so a finished message is by construction byte-identical +/// to a full re-parse. +pub(super) fn render_assistant_message( + content: &str, + width: u16, + streaming: bool, + low_motion: bool, +) -> Vec> { + render_assistant_message_with_copy_metadata(content, width, streaming, low_motion) + .into_iter() + .map(|rendered| rendered.line) + .collect() +} + +pub(super) fn render_assistant_message_with_copy_metadata( + content: &str, + width: u16, + streaming: bool, + low_motion: bool, +) -> Vec { + render_message_with_copy_metadata_impl( + ASSISTANT_GLYPH, + assistant_label_style_for(streaming, low_motion), + message_body_style(), + content, + width, + streaming, + ) +} + pub(super) fn render_message_with_copy_metadata( prefix: &str, label_style: Style, body_style: Style, content: &str, width: u16, +) -> Vec { + render_message_with_copy_metadata_impl(prefix, label_style, body_style, content, width, false) +} + +fn render_message_with_copy_metadata_impl( + prefix: &str, + label_style: Style, + body_style: Style, + content: &str, + width: u16, + streaming: bool, ) -> Vec { // An assistant cell whose content is entirely whitespace (e.g. a stray // newline streamed between reasoning and a tool call) would otherwise @@ -49,8 +94,11 @@ pub(super) fn render_message_with_copy_metadata( let prefix_width_u16 = u16::try_from(prefix_width.saturating_add(2)).unwrap_or(u16::MAX); let content_width = usize::from(width.saturating_sub(prefix_width_u16).max(1)); let mut lines = Vec::new(); - let rendered = - markdown_render::render_markdown_tagged(content, content_width as u16, body_style); + let rendered = if streaming { + markdown_render::render_markdown_tagged_streaming(content, content_width as u16, body_style) + } else { + markdown_render::render_markdown_tagged(content, content_width as u16, body_style) + }; for (idx, rendered_line) in rendered.into_iter().enumerate() { let line = if idx == 0 { let mut spans = Vec::new(); diff --git a/crates/tui/src/tui/markdown_render.rs b/crates/tui/src/tui/markdown_render.rs index 916d3d8c7f..4d5f532d47 100644 --- a/crates/tui/src/tui/markdown_render.rs +++ b/crates/tui/src/tui/markdown_render.rs @@ -26,6 +26,7 @@ #[cfg(test)] use std::cell::Cell; +use std::cell::RefCell; use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; @@ -55,6 +56,25 @@ pub fn reset_parse_invocation_count() { PARSE_INVOCATIONS.with(|c| c.set(0)); } +// Thread-local counter of source lines classified by `parse_line_into`. Used +// by tests to prove the streaming path parses O(delta) lines per chunk, not +// the whole growing message (#3897). +#[cfg(test)] +thread_local! { + static PARSED_LINES: Cell = const { Cell::new(0) }; +} + +#[cfg(test)] +#[must_use] +pub fn parsed_line_count() -> u64 { + PARSED_LINES.with(|c| c.get()) +} + +#[cfg(test)] +pub fn reset_parsed_line_count() { + PARSED_LINES.with(|c| c.set(0)); +} + /// One classified line of markdown source, width-independent. /// /// All decisions that depend only on the source text (heading level, bullet @@ -122,67 +142,78 @@ pub fn parse(content: &str) -> ParsedMarkdown { let mut in_code_block = false; for raw_line in content.lines() { - let trimmed = raw_line.trim_start(); - if trimmed.starts_with("```") { - in_code_block = !in_code_block; - continue; - } + parse_line_into(raw_line, &mut in_code_block, &mut blocks); + } - if in_code_block { - blocks.push(Block::Code { - line: raw_line.to_string(), - }); - continue; - } + ParsedMarkdown { blocks } +} - if let Some((level, text)) = parse_heading(trimmed) { - blocks.push(Block::Heading { - level, - text: text.to_string(), - }); - if level == 1 { - blocks.push(Block::HeadingRule); - } - continue; - } +/// Classify one source line into blocks. `in_code_block` is the only +/// cross-line parser state; it is advanced in place. Shared by [`parse`] and +/// the incremental streaming path (#3897), so both classify identically by +/// construction. +fn parse_line_into(raw_line: &str, in_code_block: &mut bool, blocks: &mut Vec) { + #[cfg(test)] + PARSED_LINES.with(|c| c.set(c.get() + 1)); - if let Some((bullet, text)) = parse_list_item(trimmed) { - blocks.push(Block::ListItem { - bullet, - text: text.to_string(), - }); - continue; - } + let trimmed = raw_line.trim_start(); + if trimmed.starts_with("```") { + *in_code_block = !*in_code_block; + return; + } - if is_horizontal_rule(trimmed) { - blocks.push(Block::HorizontalRule); - continue; - } + if *in_code_block { + blocks.push(Block::Code { + line: raw_line.to_string(), + }); + return; + } - match parse_table_row(trimmed) { - Some(cells) => { - blocks.push(Block::TableRow(cells)); - continue; - } - None if trimmed.starts_with('|') => { - blocks.push(Block::TableSeparator); - continue; - } - None => {} + if let Some((level, text)) = parse_heading(trimmed) { + blocks.push(Block::Heading { + level, + text: text.to_string(), + }); + if level == 1 { + blocks.push(Block::HeadingRule); } + return; + } - if trimmed.is_empty() { - // Whitespace-only lines are blank paragraphs. - blocks.push(Block::Blank); - continue; + if let Some((bullet, text)) = parse_list_item(trimmed) { + blocks.push(Block::ListItem { + bullet, + text: text.to_string(), + }); + return; + } + + if is_horizontal_rule(trimmed) { + blocks.push(Block::HorizontalRule); + return; + } + + match parse_table_row(trimmed) { + Some(cells) => { + blocks.push(Block::TableRow(cells)); + return; + } + None if trimmed.starts_with('|') => { + blocks.push(Block::TableSeparator); + return; } + None => {} + } - blocks.push(Block::Paragraph { - text: raw_line.to_string(), - }); + if trimmed.is_empty() { + // Whitespace-only lines are blank paragraphs. + blocks.push(Block::Blank); + return; } - ParsedMarkdown { blocks } + blocks.push(Block::Paragraph { + text: raw_line.to_string(), + }); } /// Render a parsed-markdown AST at the given terminal width. @@ -208,24 +239,48 @@ pub fn render_parsed_tagged( ) -> Vec { let width = width.max(1) as usize; let mut out: Vec = Vec::with_capacity(parsed.blocks.len()); + render_blocks_tagged(&parsed.blocks, width, base_style, &mut out); + + if out.is_empty() { + out.push(empty_rendered_line()); + } + + out +} + +fn empty_rendered_line() -> RenderedMarkdownLine { + RenderedMarkdownLine { + line: Line::from(""), + is_code: false, + copy_prefix_width: 0, + copy_separator_after: CopyLineSeparator::Newline, + } +} +fn is_table_block(block: &Block) -> bool { + matches!(block, Block::TableRow(_) | Block::TableSeparator) +} + +/// Render a run of blocks, grouping consecutive table rows/separators so +/// column widths derive from the whole group. Shared by the full render and +/// the incremental streaming path (#3897), so both produce identical lines +/// by construction. No empty-output fallback here — callers add it over the +/// COMBINED output. +fn render_blocks_tagged( + blocks: &[Block], + width: usize, + base_style: Style, + out: &mut Vec, +) { let mut i = 0; - while i < parsed.blocks.len() { - if matches!( - &parsed.blocks[i], - Block::TableRow(_) | Block::TableSeparator - ) { + while i < blocks.len() { + if is_table_block(&blocks[i]) { let start = i; - while i < parsed.blocks.len() - && matches!( - &parsed.blocks[i], - Block::TableRow(_) | Block::TableSeparator - ) - { + while i < blocks.len() && is_table_block(&blocks[i]) { i += 1; } out.extend( - render_table_group(&parsed.blocks[start..i], width, base_style) + render_table_group(&blocks[start..i], width, base_style) .into_iter() .map(|line| RenderedMarkdownLine { line, @@ -237,84 +292,80 @@ pub fn render_parsed_tagged( continue; } - match &parsed.blocks[i] { - Block::Heading { text, .. } => { - let style = Style::default() - .fg(palette::DEEPSEEK_SKY) - .add_modifier(Modifier::BOLD); - out.extend(render_wrapped_line_tagged(text, width, style, false, false)); - } - Block::HeadingRule => { - out.push(RenderedMarkdownLine { - line: Line::from(Span::styled( - "─".repeat(width.min(40)), - Style::default().fg(palette::TEXT_DIM), - )), - is_code: false, - copy_prefix_width: 0, - copy_separator_after: CopyLineSeparator::Newline, - }); - } - Block::HorizontalRule => { - out.push(RenderedMarkdownLine { - line: Line::from(Span::styled( - "─".repeat(width.min(60)), - Style::default().fg(palette::TEXT_DIM), - )), - is_code: false, - copy_prefix_width: 0, - copy_separator_after: CopyLineSeparator::Newline, - }); - } - Block::ListItem { bullet, text } => { - let bullet_style = Style::default().fg(palette::DEEPSEEK_SKY); - out.extend(render_list_line_tagged( - bullet, - text, - width, - bullet_style, - base_style, - )); - } - Block::Code { line } => { - let code_style = Style::default() - .fg(palette::DEEPSEEK_SKY) - .add_modifier(Modifier::ITALIC); - out.extend(render_wrapped_line_tagged( - line, width, code_style, true, true, - )); - } - Block::Paragraph { text } => { - let link_style = Style::default() - .fg(palette::WHALE_ACCENT_PRIMARY) - .add_modifier(Modifier::UNDERLINED); - out.extend(render_line_with_links_tagged( - text, width, base_style, link_style, - )); - } - Block::Blank => { - out.push(RenderedMarkdownLine { - line: Line::from(""), - is_code: false, - copy_prefix_width: 0, - copy_separator_after: CopyLineSeparator::Newline, - }); - } - Block::TableRow(_) | Block::TableSeparator => unreachable!(), - } + render_block_tagged(&blocks[i], width, base_style, out); i += 1; } +} - if out.is_empty() { - out.push(RenderedMarkdownLine { - line: Line::from(""), - is_code: false, - copy_prefix_width: 0, - copy_separator_after: CopyLineSeparator::Newline, - }); +/// Render one non-table block. +fn render_block_tagged( + block: &Block, + width: usize, + base_style: Style, + out: &mut Vec, +) { + match block { + Block::Heading { text, .. } => { + let style = Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD); + out.extend(render_wrapped_line_tagged(text, width, style, false, false)); + } + Block::HeadingRule => { + out.push(RenderedMarkdownLine { + line: Line::from(Span::styled( + "─".repeat(width.min(40)), + Style::default().fg(palette::TEXT_DIM), + )), + is_code: false, + copy_prefix_width: 0, + copy_separator_after: CopyLineSeparator::Newline, + }); + } + Block::HorizontalRule => { + out.push(RenderedMarkdownLine { + line: Line::from(Span::styled( + "─".repeat(width.min(60)), + Style::default().fg(palette::TEXT_DIM), + )), + is_code: false, + copy_prefix_width: 0, + copy_separator_after: CopyLineSeparator::Newline, + }); + } + Block::ListItem { bullet, text } => { + let bullet_style = Style::default().fg(palette::DEEPSEEK_SKY); + out.extend(render_list_line_tagged( + bullet, + text, + width, + bullet_style, + base_style, + )); + } + Block::Code { line } => { + let code_style = Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::ITALIC); + out.extend(render_wrapped_line_tagged( + line, width, code_style, true, true, + )); + } + Block::Paragraph { text } => { + let link_style = Style::default() + .fg(palette::WHALE_ACCENT_PRIMARY) + .add_modifier(Modifier::UNDERLINED); + out.extend(render_line_with_links_tagged( + text, width, base_style, link_style, + )); + } + Block::Blank => { + out.push(empty_rendered_line()); + } + Block::TableRow(_) | Block::TableSeparator => { + unreachable!("table blocks are grouped by render_blocks_tagged") + } } - - out } /// Convenience wrapper: parse + render in one call. @@ -339,6 +390,197 @@ pub fn render_markdown_tagged( render_parsed_tagged(&parsed, width, base_style) } +// --------------------------------------------------------------------------- +// Incremental rendering for streaming cells (#3897) +// --------------------------------------------------------------------------- + +/// How many concurrent streaming render states to keep. The main transcript +/// and the Ctrl+T overlay render the same growing message at different +/// widths within one frame, and a streaming thinking body can interleave — +/// a single slot would thrash between them. +const STREAM_RENDER_SLOTS: usize = 4; + +thread_local! { + // Thread-local, like PARSE_INVOCATIONS: the TUI renders on one thread, + // and per-thread state keeps parallel tests isolated. + static STREAM_RENDER_CACHE: RefCell> = const { RefCell::new(Vec::new()) }; +} + +/// Incremental parse+render state for one growing markdown source. +/// +/// Prefix stability is provable for this parser: classification is per line +/// with a single carried bool (`in_code_block`), so appending bytes can never +/// re-classify an earlier COMPLETE line — only the trailing partial line +/// (after the last `\n`) is volatile. Rendering is per block except +/// consecutive table rows, whose column widths derive from the whole group; +/// a trailing table run therefore stays volatile until a non-table block +/// lands after it. +struct StreamRenderState { + /// The committed source prefix (everything up to `cutoff`). Stored so a + /// candidate content can be verified as an extension via one memcmp — + /// orders of magnitude cheaper than the parse+wrap it replaces. + committed_source: String, + /// Rendered-cell width the frozen lines were produced at. + width: usize, + base_style: Style, + /// OSC8 hyperlink support is a process-global flag that changes rendered + /// spans; captured so a flip invalidates the slot. + osc8: bool, + /// Parser fence state after the committed prefix. + in_code_block: bool, + /// Blocks parsed from the committed prefix. + stable_blocks: Vec, + /// Rendered output of `stable_blocks[..frozen_block_count]` — every + /// stable block except a trailing table-group run. + frozen_lines: Vec, + frozen_block_count: usize, +} + +impl StreamRenderState { + fn new(width: usize, base_style: Style, osc8: bool) -> Self { + Self { + committed_source: String::new(), + width, + base_style, + osc8, + in_code_block: false, + stable_blocks: Vec::new(), + frozen_lines: Vec::new(), + frozen_block_count: 0, + } + } + + fn matches(&self, content: &str, width: usize, base_style: Style, osc8: bool) -> bool { + self.width == width + && self.base_style == base_style + && self.osc8 == osc8 + && content.len() >= self.committed_source.len() + && content.as_bytes()[..self.committed_source.len()] + == *self.committed_source.as_bytes() + } + + fn render(&mut self, content: &str) -> Vec { + // Absorb newly completed lines (everything up to the last '\n'). + // `lines()` strips a trailing '\r' itself, so a lone '\r' at the + // very end belongs to the volatile tail along with the partial line. + let cutoff = self.committed_source.len(); + let new_cutoff = content.rfind('\n').map_or(0, |pos| pos + 1); + if new_cutoff > cutoff { + for raw_line in content[cutoff..new_cutoff].lines() { + parse_line_into(raw_line, &mut self.in_code_block, &mut self.stable_blocks); + } + self.committed_source.push_str(&content[cutoff..new_cutoff]); + } + + // Freeze forward: render every stable block except a trailing + // table-group run (its column layout can still change when the tail + // appends more rows). + let mut stable_end = self.stable_blocks.len(); + while stable_end > self.frozen_block_count + && is_table_block(&self.stable_blocks[stable_end - 1]) + { + stable_end -= 1; + } + if stable_end > self.frozen_block_count { + render_blocks_tagged( + &self.stable_blocks[self.frozen_block_count..stable_end], + self.width, + self.base_style, + &mut self.frozen_lines, + ); + self.frozen_block_count = stable_end; + } + + // Volatile region: the stable trailing table run (if any) plus the + // partial last line, re-parsed with a scratch copy of the fence + // state so a "```" sitting in the partial line can't poison the + // committed parser state. + let mut volatile_blocks: Vec = + self.stable_blocks[self.frozen_block_count..].to_vec(); + let tail = &content[self.committed_source.len()..]; + if !tail.is_empty() { + let mut tail_fence_state = self.in_code_block; + for raw_line in tail.lines() { + parse_line_into(raw_line, &mut tail_fence_state, &mut volatile_blocks); + } + } + + let mut out = self.frozen_lines.clone(); + render_blocks_tagged(&volatile_blocks, self.width, self.base_style, &mut out); + if out.is_empty() { + // Match render_parsed_tagged's fallback over the COMBINED output. + out.push(empty_rendered_line()); + } + out + } +} + +/// Incremental variant of [`render_markdown_tagged`] for append-only +/// streaming sources. Per call it parses only the source appended since the +/// last call (plus the volatile trailing partial line / open table group) +/// and re-renders only unfrozen blocks, instead of re-parsing and +/// re-wrapping the whole growing message on every committed chunk (#3897). +/// +/// Output is byte-identical to [`render_markdown_tagged`] on the same +/// input; a non-extension of the cached source (backtrack, different cell) +/// simply misses the cache and rebuilds from scratch. Callers should route +/// the FINAL render of a finished message through [`render_markdown_tagged`]. +#[must_use] +pub fn render_markdown_tagged_streaming( + content: &str, + width: u16, + base_style: Style, +) -> Vec { + let width_cells = width.max(1) as usize; + let osc8_enabled = osc8::enabled(); + let out = STREAM_RENDER_CACHE.with(|cache| { + let mut slots = cache.borrow_mut(); + let mut state = match slots + .iter() + .position(|slot| slot.matches(content, width_cells, base_style, osc8_enabled)) + { + Some(idx) => slots.remove(idx), + None => StreamRenderState::new(width_cells, base_style, osc8_enabled), + }; + let rendered = state.render(content); + // Move-to-front LRU keeps the active stream(s) resident. + slots.insert(0, state); + slots.truncate(STREAM_RENDER_SLOTS); + rendered + }); + + // Differential guard: any future parser/renderer change that breaks + // prefix stability fails loudly in debug builds and under `cargo test`. + #[cfg(debug_assertions)] + { + // Keep the test-only parsed-line counter measuring the streaming + // path alone — the guard's full re-parse is not part of the cost + // under test. + #[cfg(test)] + let parsed_lines_before = parsed_line_count(); + let full = render_markdown_tagged(content, width, base_style); + #[cfg(test)] + PARSED_LINES.with(|c| c.set(parsed_lines_before)); + debug_assert!( + rendered_lines_eq(&out, &full), + "streaming markdown render diverged from the full re-parse", + ); + } + + out +} + +#[cfg(debug_assertions)] +fn rendered_lines_eq(a: &[RenderedMarkdownLine], b: &[RenderedMarkdownLine]) -> bool { + a.len() == b.len() + && a.iter().zip(b.iter()).all(|(x, y)| { + x.line == y.line + && x.is_code == y.is_code + && x.copy_prefix_width == y.copy_prefix_width + && x.copy_separator_after == y.copy_separator_after + }) +} + /// Render plain text: split on newlines, word-wrap each line independently, /// preserve leading whitespace and blank lines. No markdown interpretation. #[must_use] @@ -1925,4 +2167,117 @@ mod tests { // Any output is acceptable (the path is degenerate); assert no panic. let _ = rendered; } + + // ---- #3897: incremental rendering for streaming cells ---- + + fn assert_streaming_matches_full(content: &str, width: u16) { + // The streaming entry point carries its own debug_assert differential + // guard, but assert explicitly here too so a failure names the input. + let streaming = render_markdown_tagged_streaming(content, width, Style::default()); + let full = render_markdown_tagged(content, width, Style::default()); + assert_eq!( + streaming.len(), + full.len(), + "line count diverged at width {width} for {content:?}" + ); + for (idx, (s, f)) in streaming.iter().zip(full.iter()).enumerate() { + assert_eq!(s.line, f.line, "line {idx} diverged for {content:?}"); + assert_eq!(s.is_code, f.is_code, "is_code {idx} diverged"); + assert_eq!(s.copy_prefix_width, f.copy_prefix_width); + assert_eq!(s.copy_separator_after, f.copy_separator_after); + } + } + + const STREAMING_FIXTURE: &str = "# Title\n\nIntro paragraph with a [link](https://example.com) inside.\n\n## Sub-heading\n\n- first bullet\n- second bullet with 中文宽字符 text\n\n```rust\nfn main() {\n println!(\"hi\");\n}\n```\n\n| col a | col b |\n|-------|-------|\n| 1 | 2 |\n| 3 | 4 | 5 |\n\n---\n\nClosing paragraph after the table.\n"; + + #[test] + fn streaming_render_matches_full_render_at_every_chunk() { + for width in [24u16, 40, 80] { + for chunk_bytes in [1usize, 3, 7] { + let mut fed = String::new(); + let mut remaining = STREAMING_FIXTURE; + while !remaining.is_empty() { + let mut take = chunk_bytes.min(remaining.len()); + while !remaining.is_char_boundary(take) { + take += 1; + } + fed.push_str(&remaining[..take]); + remaining = &remaining[take..]; + assert_streaming_matches_full(&fed, width); + } + } + } + } + + #[test] + fn streaming_render_parses_only_the_tail() { + let lines: Vec = (0..120).map(|i| format!("paragraph line {i}\n")).collect(); + let mut fed = String::new(); + + reset_parsed_line_count(); + for line in &lines { + fed.push_str(line); + let _ = render_markdown_tagged_streaming(&fed, 80, Style::default()); + } + let parsed = parsed_line_count(); + let n = lines.len() as u64; + // Each committed line is parsed once by the incremental path (plus + // the volatile tail, which is empty here because every chunk ends in + // '\n'). A full re-parse per chunk would cost N*(N+1)/2 ≈ 7260 lines. + assert!( + parsed <= 2 * n + 4, + "streaming path parsed {parsed} lines for {n} single-line chunks — not O(delta)" + ); + } + + #[test] + fn streaming_slot_falls_back_on_non_extension() { + let first = "# Message A\n\nbody of a\n"; + let _ = render_markdown_tagged_streaming(first, 60, Style::default()); + // Shorter, unrelated content: not an extension of the cached prefix. + assert_streaming_matches_full("totally different", 60); + // Backtrack of the original content (shrunk mid-stream). + assert_streaming_matches_full("# Message A\n", 60); + } + + #[test] + fn streaming_two_widths_interleaved_do_not_thrash() { + let mut fed = String::new(); + for i in 0..20 { + fed.push_str(&format!("interleaved line {i} with some words\n")); + // Main transcript and overlay render the same stream at two + // widths in the same frame; both must stay byte-identical. + assert_streaming_matches_full(&fed, 60); + assert_streaming_matches_full(&fed, 80); + } + } + + #[test] + fn trailing_table_group_stays_volatile() { + let mut fed = String::from("intro\n\n"); + let rows = [ + "| a | b |\n", + "|---|---|\n", + "| 1 | 2 |\n", + "| wide-cell-that-changes-layout | 4 | extra |\n", + ]; + for row in rows { + fed.push_str(row); + // A later row can widen the whole group's column layout; earlier + // rows must re-render exactly as the full parse does. + assert_streaming_matches_full(&fed, 60); + } + fed.push_str("\nafter the table\n"); + assert_streaming_matches_full(&fed, 60); + } + + #[test] + fn unclosed_code_fence_streams_byte_identical() { + let source = "before\n```rust\nfn x() {\n```\nafter\n"; + let mut fed = String::new(); + for ch in source.chars() { + fed.push(ch); + assert_streaming_matches_full(&fed, 40); + } + } } From 80e058a75fc84fc963f4eaee53842ccc4bc61806 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 15:26:16 +0000 Subject: [PATCH 6/7] fix(tui): close the async-window regressions found by adversarial review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review of the five perf commits confirmed four real regressions in the two fixes that moved work off the UI thread; this closes all of them. File tree (#3900): a finished background expand walk never triggered a redraw — with the loop idle, an expanded directory looked empty until the next unrelated input event. The event loop now drains file-tree walks (initial build and expands) each iteration via poll_background() and sets needs_redraw when results were applied, matching the @mention poll wiring. Mention completion (#3899), three fixes: - Enter could submit a half-typed @mention during the first walk's in-flight window (popup routing keyed only on non-empty entries; the pre-async walk finished inside the key event, making that impossible). Enter is now held with a "Scanning files for @…" status while a walk for the live token is pending and the popup has nothing to show; Esc hides the pending popup; forced Ctrl+Enter submit keeps priority. - The stale popup fallback was unbounded: cached results for an UNRELATED earlier mention token could be shown and Enter/Tab would splice the wrong file over the live token. The fallback now requires same token lineage (live partial extends the cached one or vice versa) in addition to same context. - Superseded in-flight walks were dropped and respawned per keystroke; since a blocking walk cannot be cancelled, fast typing stacked concurrent full-workspace walks. The in-flight walk is now retained until it lands (its stale result is discarded), so real walks are serialized — at most one running — and the fresh needle's walk starts on the next call after landing. Tests: pending-expand drain requests exactly one repaint; fallback lineage positive/negative; in-flight walk retained across a needle change; mention_walk_pending true only for the live, un-hidden token. Follow-up to #3899/#3900 (PR #3902). --- crates/tui/src/tui/file_mention.rs | 41 ++++++++--- crates/tui/src/tui/file_tree.rs | 47 +++++++++++++ crates/tui/src/tui/ui.rs | 31 ++++++++- crates/tui/src/tui/ui/tests.rs | 107 +++++++++++++++++++++++++++-- 4 files changed, 210 insertions(+), 16 deletions(-) diff --git a/crates/tui/src/tui/file_mention.rs b/crates/tui/src/tui/file_mention.rs index 5890db10e4..94ee992aa3 100644 --- a/crates/tui/src/tui/file_mention.rs +++ b/crates/tui/src/tui/file_mention.rs @@ -253,14 +253,15 @@ pub fn visible_mention_menu_entries(app: &mut App, limit: usize) -> Vec } } None => { - // Still walking. Keep it only if it is for the current key; - // a stale-needle walk is dropped so the respawn below starts - // the walk for the fresh needle (latest keystroke wins, at - // most one walk in flight). - if pending.key == key { - app.composer.mention_completion_pending = Some(pending); - return stale_mention_fallback_entries(app, &key); - } + // Still walking. Keep it even when the needle has moved on: + // dropping the handle cannot stop the blocking walk, so + // respawning per keystroke would stack concurrent + // full-workspace walks on the blocking pool. Keeping the + // old walk serializes them — its result is discarded on + // landing (key mismatch above) and the very next call + // spawns the walk for the live needle. + app.composer.mention_completion_pending = Some(pending); + return stale_mention_fallback_entries(app, &key); } } } @@ -311,17 +312,37 @@ fn run_mention_completion_walk(key: &MentionCompletionKey) -> Vec { /// While a walk is in flight, keep showing the previous keystroke's entries /// so the popup stays open and Enter/Up/Down keep routing to it — but only -/// when the cached results come from the same session context (an older -/// partial is fine; a different workspace/behavior/depth is not). +/// when the cached results come from the same session context AND the same +/// token lineage (the live partial extends the cached one, or vice versa +/// for backspacing). Results for an unrelated earlier mention token must +/// never surface here: Enter/Tab apply the shown list against the live +/// token, so an unbounded fallback could splice the wrong file. fn stale_mention_fallback_entries(app: &App, key: &MentionCompletionKey) -> Vec { app.composer .mention_completion_cache .as_ref() .filter(|cache| cache.key.same_context(key)) + .filter(|cache| { + key.partial.starts_with(&cache.key.partial) + || cache.key.partial.starts_with(&key.partial) + }) .map(|cache| cache.entries.clone()) .unwrap_or_default() } +/// Whether a completion walk is in flight for the mention token under the +/// cursor while the popup has nothing to show yet. The key router treats +/// this as popup-open for Enter/Esc: before #3899 the walk ran +/// synchronously inside the key event, so Enter could never submit a +/// half-typed mention — swallowing Enter for the walk's duration preserves +/// that guarantee without blocking the UI thread. +#[must_use] +pub fn mention_walk_pending(app: &App) -> bool { + !app.mention_menu_hidden + && app.composer.mention_completion_pending.is_some() + && partial_file_mention_at_cursor(&app.input, app.cursor_position).is_some() +} + /// Drain a completed background completion walk outside the render path. /// Called once per event-loop iteration (next to the workspace-context /// refresh) so a finished walk repaints the popup without waiting for an diff --git a/crates/tui/src/tui/file_tree.rs b/crates/tui/src/tui/file_tree.rs index f2c99a9d41..ed431d1c49 100644 --- a/crates/tui/src/tui/file_tree.rs +++ b/crates/tui/src/tui/file_tree.rs @@ -133,6 +133,20 @@ impl FileTreeState { } } + /// Drain any completed background walks (initial build or expands). + /// Returns `true` when new results were applied so the event loop can + /// schedule a repaint — without this, a walk finishing while the loop + /// is idle would leave the expanded directory looking empty until the + /// next unrelated input event (#3900). + pub fn poll_background(&mut self) -> bool { + let was_loading = self.is_loading; + self.poll_loading(); + let finished_loading = was_loading && !self.is_loading; + let pending_before = self.pending_expands.len(); + self.poll_pending_expands(); + finished_loading || self.pending_expands.len() != pending_before + } + /// Poll for background expand-walk results and splice them in. /// Call from the render loop, after [`Self::poll_loading`] (#3900). pub fn poll_pending_expands(&mut self) { @@ -709,6 +723,39 @@ mod tests { assert_eq!(state.entries[state.cursor].name, "README.md"); } + #[test] + fn poll_background_reports_applied_expand_results() { + let ws = fixture_workspace(); + let mut state = FileTreeState::new(ws.path()); + let src_norm = normalize_path(&ws.path().join("src")); + let src_idx = index_of(&state, "src"); + + state.expanded_dirs.insert(src_norm.clone()); + state.entries[src_idx].expanded = true; + let children = build_file_tree_inner( + ws.path(), + &state.expanded_dirs, + Some(&ws.path().join("src")), + ); + state.pending_expands.insert( + src_norm.clone(), + PendingExpand { + seq: 1, + cell: Arc::new(Mutex::new(Some(children))), + }, + ); + + assert!( + state.poll_background(), + "a drained expand result must request a repaint" + ); + assert!(state.entries.iter().any(|e| e.name == "main.rs")); + assert!( + !state.poll_background(), + "nothing pending: no repaint requested" + ); + } + #[tokio::test] async fn async_expand_splices_children_after_poll() { let ws = fixture_workspace(); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 1037c51d78..0e27cefa71 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -3291,6 +3291,13 @@ async fn run_event_loop( // Drain any completed background @mention completion walk so the // popup repaints as soon as results are ready (#3899). crate::tui::file_mention::poll_mention_completion(app); + // Drain completed file-tree walks (initial build / expands) so the + // spliced children repaint without waiting for an input event (#3900). + if let Some(tree) = app.file_tree.as_mut() + && tree.poll_background() + { + app.needs_redraw = true; + } // Draw is gated by the frame-rate limiter (120 FPS cap). When a // redraw is needed but the limiter says we're inside the cooldown @@ -4088,6 +4095,12 @@ async fn run_event_loop( if mention_menu_open && app.mention_menu_selected >= mention_menu_entries.len() { app.mention_menu_selected = mention_menu_entries.len().saturating_sub(1); } + // A background completion walk for the token under the cursor + // with nothing to show yet (#3899): Enter must wait rather than + // submit a half-typed @mention, matching the pre-async behavior + // where the walk finished inside the key event. + let mention_walk_pending = + !mention_menu_open && crate::tui::file_mention::mention_walk_pending(app); // Cancel a pending Esc-Esc prime as soon as any non-Esc key // arrives. Without this the prime would hang around for the @@ -4347,7 +4360,7 @@ async fn run_event_loop( KeyCode::Esc if app.clear_composer_attachment_selection() => { continue; } - KeyCode::Esc if mention_menu_open => { + KeyCode::Esc if mention_menu_open || mention_walk_pending => { app.mention_menu_hidden = true; app.mention_menu_selected = 0; } @@ -4744,6 +4757,22 @@ async fn run_event_loop( } } } + KeyCode::Enter if mention_walk_pending => { + // Completion results for the mention under the cursor + // haven't landed yet — hold the submit so a half-typed + // @mention isn't sent (#3899). The walk finishing sets + // needs_redraw, so the popup opens (or, with no + // matches, a following Enter submits) moments later. + if let Some((_, partial)) = + crate::tui::file_mention::partial_file_mention_at_cursor( + &app.input, + app.cursor_position, + ) + { + app.status_message = Some(format!("Scanning files for @{partial}…")); + } + continue; + } KeyCode::Enter => { // #573: when the user typed a slash-command prefix that // the popup is matching (e.g. `/mo` → `/model`), Enter diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 5c55d312aa..0469ef7bbe 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -8004,11 +8004,14 @@ async fn mention_completion_stale_needle_walk_is_replaced() { }); let entries = visible_mention_menu_entries(&mut app, 6); - // The completed old-needle walk serves as the transient anti-flicker - // fallback (the popup must not close mid-word), but it is stored under - // its true key — never promoted as the live needle's result — and a - // fresh walk is spawned for the live key. - assert_eq!(entries, vec!["stale-entry.md".to_string()]); + // The completed old-needle walk is stored under its true key — never + // promoted as the live needle's result — and because "old" shares no + // token lineage with "docs/", it must not surface as the fallback + // either (Enter/Tab apply the shown list against the live token). + assert!( + entries.is_empty(), + "unrelated-token results must not surface: {entries:?}" + ); let cached = app .composer .mention_completion_cache @@ -8038,6 +8041,100 @@ async fn mention_completion_stale_needle_walk_is_replaced() { ); } +#[tokio::test] +async fn mention_fallback_limited_to_same_token_lineage() { + let tmpdir = TempDir::new().expect("tempdir"); + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + + // Cached results from an earlier keystroke of the SAME token ("doc" is + // a prefix of "docs/") serve as the anti-flicker fallback while the + // fresh walk runs. + app.composer.mention_completion_cache = Some(crate::tui::app::MentionCompletionCache { + key: mention_key_for(&app, "doc", 6), + entries: vec!["docs/alpha.md".to_string()], + }); + let entries = visible_mention_menu_entries(&mut app, 6); + assert_eq!(entries, vec!["docs/alpha.md".to_string()]); + + // Cached results from an unrelated token never surface as fallback. + let mut other = create_test_app(); + other.workspace = tmpdir.path().to_path_buf(); + other.input = "look at @docs/".to_string(); + other.cursor_position = other.input.chars().count(); + other.composer.mention_completion_cache = Some(crate::tui::app::MentionCompletionCache { + key: mention_key_for(&other, "src/mai", 6), + entries: vec!["src/main.rs".to_string()], + }); + let entries = visible_mention_menu_entries(&mut other, 6); + assert!( + entries.is_empty(), + "results for another token must not be applied against this one: {entries:?}" + ); +} + +#[test] +fn mention_inflight_walk_is_not_respawned_per_keystroke() { + let tmpdir = TempDir::new().expect("tempdir"); + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + + // A still-running walk for an older needle: dropping it cannot stop + // the blocking work, so it must be kept (serialized) rather than + // replaced with a concurrent walk for the new needle. + let old_key = mention_key_for(&app, "old", 6); + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key: old_key, + cell: std::sync::Arc::new(std::sync::Mutex::new(None)), + }); + + let entries = visible_mention_menu_entries(&mut app, 6); + assert!(entries.is_empty()); + let pending = app + .composer + .mention_completion_pending + .as_ref() + .expect("in-flight walk is retained"); + assert_eq!( + pending.key.partial, "old", + "no concurrent walk is spawned while one is in flight" + ); +} + +#[test] +fn mention_walk_pending_only_for_live_token() { + let tmpdir = TempDir::new().expect("tempdir"); + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/".to_string(); + app.cursor_position = app.input.chars().count(); + let key = mention_key_for(&app, "docs/", 6); + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key, + cell: std::sync::Arc::new(std::sync::Mutex::new(None)), + }); + + assert!(crate::tui::file_mention::mention_walk_pending(&app)); + + app.mention_menu_hidden = true; + assert!( + !crate::tui::file_mention::mention_walk_pending(&app), + "Esc-hidden popup must not swallow Enter" + ); + app.mention_menu_hidden = false; + + app.input = "no mention".to_string(); + app.cursor_position = app.input.chars().count(); + assert!( + !crate::tui::file_mention::mention_walk_pending(&app), + "cursor outside a mention must not swallow Enter" + ); +} + #[test] fn poll_mention_completion_promotes_once_and_sets_needs_redraw() { let tmpdir = TempDir::new().expect("tempdir"); From ebf2313db098d9541f9059bbe2002469f213a4a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 16:24:06 +0000 Subject: [PATCH 7/7] fix(tui): close both @mention follow-ups from the PR #3902 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Strict equivalence for the stale popup fallback: entries are now filtered to those still matching the LIVE needle (case-insensitive substring, mirroring the walk's own match rule), so a fast Enter after a keystroke can only apply an entry the fresh walk would also have returned — the reviewer's `docs/apple.md` vs `@docs/ab` repro is pinned by a test. When the filter empties the list the popup reads as pending and the existing Enter-hold takes over. 2. A panicking walk can no longer soft-lock Enter for its token: the walker closure now routes its result through a completion guard whose Drop writes an empty result on unwind, so mention_completion_pending always drains. Requested in review on PR #3902. --- crates/tui/src/tui/file_mention.rs | 78 ++++++++++++++++++++++++++---- crates/tui/src/tui/ui/tests.rs | 29 +++++++++++ 2 files changed, 98 insertions(+), 9 deletions(-) diff --git a/crates/tui/src/tui/file_mention.rs b/crates/tui/src/tui/file_mention.rs index 7f61d4138b..09db6d3c6e 100644 --- a/crates/tui/src/tui/file_mention.rs +++ b/crates/tui/src/tui/file_mention.rs @@ -324,13 +324,10 @@ pub fn visible_mention_menu_entries(app: &mut App, limit: usize) -> Vec // results and test semantics are identical to the pre-async behavior. if tokio::runtime::Handle::try_current().is_ok() { let cell = std::sync::Arc::new(std::sync::Mutex::new(None)); - let walker_cell = cell.clone(); + let completion = MentionWalkCompletion { cell: cell.clone() }; let walker_key = key.clone(); crate::utils::spawn_blocking_supervised("mention-completion", move || { - let entries = run_mention_completion_walk(&walker_key); - if let Ok(mut guard) = walker_cell.lock() { - *guard = Some(entries); - } + completion.fill(run_mention_completion_walk(&walker_key)); }); app.composer.mention_completion_pending = Some(MentionCompletionPending { key: key.clone(), @@ -347,6 +344,35 @@ pub fn visible_mention_menu_entries(app: &mut App, limit: usize) -> Vec } } +/// Completion guarantee for a background mention walk: the shared cell is +/// ALWAYS filled, even if the walk panics. Without this, a panicking walk +/// would leave `mention_completion_pending` set forever and the Enter-hold +/// for the pending token would never release (until Esc). On unwind the +/// `Drop` impl writes an empty result, which drains normally. +struct MentionWalkCompletion { + cell: std::sync::Arc>>>, +} + +impl MentionWalkCompletion { + fn fill(self, entries: Vec) { + if let Ok(mut guard) = self.cell.lock() { + *guard = Some(entries); + } + // Normal path: `self` drops here with the cell already filled, so + // the Drop impl is a no-op. + } +} + +impl Drop for MentionWalkCompletion { + fn drop(&mut self) { + if let Ok(mut guard) = self.cell.lock() + && guard.is_none() + { + *guard = Some(Vec::new()); + } + } +} + /// Run the completion walk described by `key`. Called on a background thread /// for live keystrokes and synchronously when no runtime is available. fn run_mention_completion_walk(key: &MentionCompletionKey) -> Vec { @@ -367,10 +393,15 @@ fn run_mention_completion_walk(key: &MentionCompletionKey) -> Vec { /// so the popup stays open and Enter/Up/Down keep routing to it — but only /// when the cached results come from the same session context AND the same /// token lineage (the live partial extends the cached one, or vice versa -/// for backspacing). Results for an unrelated earlier mention token must -/// never surface here: Enter/Tab apply the shown list against the live -/// token, so an unbounded fallback could splice the wrong file. +/// for backspacing), and only the entries that still match the LIVE +/// needle. Enter/Tab apply the shown list against the live token, so a +/// fast Enter after a keystroke must never splice an entry the fresh walk +/// would not have returned (e.g. `docs/apple.md` against `@docs/ab`). +/// The filter mirrors the walk's case-insensitive substring match, so the +/// surviving entries are a subset of the fresh walk's results; when it +/// empties the list, the popup reads as pending and Enter is held instead. fn stale_mention_fallback_entries(app: &App, key: &MentionCompletionKey) -> Vec { + let needle = key.partial.to_lowercase(); app.composer .mention_completion_cache .as_ref() @@ -379,7 +410,14 @@ fn stale_mention_fallback_entries(app: &App, key: &MentionCompletionKey) -> Vec< key.partial.starts_with(&cache.key.partial) || cache.key.partial.starts_with(&key.partial) }) - .map(|cache| cache.entries.clone()) + .map(|cache| { + cache + .entries + .iter() + .filter(|entry| entry.to_lowercase().contains(&needle)) + .cloned() + .collect() + }) .unwrap_or_default() } @@ -1060,6 +1098,28 @@ mod tests { use super::*; use tempfile::TempDir; + /// A background walk must ALWAYS complete its cell — a panicking walk + /// otherwise leaves `mention_completion_pending` set forever and the + /// Enter-hold for that token never releases. + #[test] + fn mention_walk_completion_fills_cell_even_on_unwind() { + // Normal path: the filled result is preserved. + let cell = std::sync::Arc::new(std::sync::Mutex::new(None)); + let completion = MentionWalkCompletion { cell: cell.clone() }; + completion.fill(vec!["docs/a.md".to_string()]); + assert_eq!( + cell.lock().unwrap().as_deref(), + Some(&["docs/a.md".to_string()][..]) + ); + + // Unwind path: dropping without filling writes an empty result so + // the pending walk drains instead of soft-locking. + let cell = std::sync::Arc::new(std::sync::Mutex::new(None)); + let completion = MentionWalkCompletion { cell: cell.clone() }; + drop(completion); + assert_eq!(cell.lock().unwrap().as_deref(), Some(&[][..])); + } + /// #101 regression — workspace-vs-cwd divergence: `@bar.txt` typed from /// the cwd `/sub` MUST resolve to `/sub/bar.txt`, never to /// `/bar.txt` (which doesn't exist). diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index f93114ccba..8ee3b160f2 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -8308,6 +8308,35 @@ async fn mention_fallback_limited_to_same_token_lineage() { ); } +#[test] +fn mention_fallback_drops_entries_that_no_longer_match_the_live_needle() { + // Reviewer repro on PR #3902: `@docs/a` shows docs/apple.md, the user + // types `b`, and a fast Enter before the fresh walk lands must NOT be + // able to apply docs/apple.md against `@docs/ab`. + let tmpdir = TempDir::new().expect("tempdir"); + let mut app = create_test_app(); + app.workspace = tmpdir.path().to_path_buf(); + app.input = "look at @docs/ab".to_string(); + app.cursor_position = app.input.chars().count(); + + app.composer.mention_completion_cache = Some(crate::tui::app::MentionCompletionCache { + key: mention_key_for(&app, "docs/a", 6), + entries: vec!["docs/apple.md".to_string(), "docs/abc.md".to_string()], + }); + // An in-flight walk keeps the call on the fallback path in plain tests. + app.composer.mention_completion_pending = Some(MentionCompletionPending { + key: mention_key_for(&app, "docs/ab", 6), + cell: std::sync::Arc::new(std::sync::Mutex::new(None)), + }); + + let entries = visible_mention_menu_entries(&mut app, 6); + assert_eq!( + entries, + vec!["docs/abc.md".to_string()], + "only entries matching the live needle may be shown or applied" + ); +} + #[test] fn mention_inflight_walk_is_not_respawned_per_keystroke() { let tmpdir = TempDir::new().expect("tempdir");