From 20427325f978a3d6cbfc94f1e62a53f0e7f45ff3 Mon Sep 17 00:00:00 2001 From: Jo <10510431+j178@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:36:21 +0800 Subject: [PATCH] Fix progress insertion after collapsed rows --- crates/prek/src/cli/run/reporter.rs | 416 +++++++++++++++++++--------- 1 file changed, 292 insertions(+), 124 deletions(-) diff --git a/crates/prek/src/cli/run/reporter.rs b/crates/prek/src/cli/run/reporter.rs index dd28f61d7..a5062ad19 100644 --- a/crates/prek/src/cli/run/reporter.rs +++ b/crates/prek/src/cli/run/reporter.rs @@ -1,3 +1,57 @@ +//! Progress UI for concurrent hook execution. +//! +//! This module renders one root progress line, optional project header lines, +//! hook progress rows, short live output previews, and collapsed summaries for +//! completed hooks that no longer fit in the terminal. +//! +//! # Row model +//! +//! A hook run is represented by a `HookBar`. Its main progress row is always +//! present while the hook is active or waiting for its final result. If the hook +//! streams output for long enough, up to `HOOK_OUTPUT_PREVIEW_LINES` preview +//! rows are inserted directly below the main row. +//! +//! `HookRunReporter::running` owns active `HookBar`s. `HookGroup` owns the +//! per-project rows that can outlive an active hook: the optional project +//! header, the collapsed hidden-summary row, and completed hook rows. A +//! completed hook moves from `running` into its project's `CompletedBars` before +//! the running lock is released, so other layout operations never observe the +//! hook as missing from both states. +//! +//! # Visual order and anchors +//! +//! Each hook main row receives a monotonic `line_order` when it starts. Hooks +//! may complete in a different order, so completed rows are stored by +//! `line_order`, not by completion time or hook index. +//! +//! A newly started hook is inserted after the visually latest row in its +//! project. The candidates are the collapsed summary row, the latest running +//! hook's visual tail, and the latest visible completed hook row. If the project +//! has none of those rows, the project header is used; otherwise the hook is +//! inserted before the root progress line. Preview rows do not receive their own +//! `line_order`; they are addressed through the owning hook's `visual_tail`. +//! +//! # Collapse invariants +//! +//! Terminal-height pressure is handled by collapsing old completed hook rows +//! into one summary line. Only completed rows with a known pass/fail result can +//! be collapsed, because the summary has to preserve the result counts. The +//! first collapse hides two completed rows to make room for the new summary; a +//! later collapse hides one more row and reuses the existing summary. +//! +//! The summary row's visual order is fixed at the first hidden completed row. +//! It intentionally does not advance as later completed rows are hidden: +//! running rows can still be visually interleaved with completed rows from the +//! same project, and moving the summary order forward would make future hooks +//! insert above those still-running rows. +//! +//! # Synchronization +//! +//! `running` is the single source of truth for active hook rows and their +//! preview tails. `HookGroup` does not cache running row positions. Code paths +//! that need both maps lock `running` before `groups`, matching the state +//! transition from running to completed and avoiding stale insertion anchors. + use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::hash_map::Entry; @@ -61,22 +115,15 @@ impl HookBar { /// Streams one output chunk into the preview rows. /// - /// Returns the new visual tail when this chunk caused new preview rows to - /// be inserted. The owning `HookGroup` uses that tail as the insertion - /// anchor for subsequent hooks in the same project. - fn push_output( - &mut self, - reporter: &ProgressReporter, - width: usize, - chunk: &[u8], - ) -> Option { + /// Returns whether this chunk inserted any new preview rows. + fn push_output(&mut self, reporter: &ProgressReporter, width: usize, chunk: &[u8]) -> bool { self.output_preview.push_chunk(chunk); if self.output_bars.is_empty() && self.started_at.elapsed() < HOOK_OUTPUT_PREVIEW_DELAY { - return None; + return false; } let lines = self.output_preview.visible_lines(); - let mut inserted_tail = None; + let mut inserted = false; for (idx, line) in lines.iter().enumerate() { if idx == self.output_bars.len() { @@ -90,7 +137,7 @@ impl HookBar { ); output.set_prefix(HOOK_OUTPUT_PREVIEW_PREFIX); self.output_bars.push(output); - inserted_tail = self.output_bars.last().cloned(); + inserted = true; } let line = line.trim_end(); @@ -102,7 +149,11 @@ impl HookBar { self.output_bars[idx].set_message(message); } - inserted_tail + inserted + } + + fn visual_tail(&self) -> &ProgressBar { + self.output_bars.last().unwrap_or(&self.progress) } } @@ -124,20 +175,27 @@ impl HookKey { #[derive(Debug, Default)] struct CompletedBars { visible: BTreeMap, + /// Visual order of the collapsed summary line, fixed at the first hidden row. + hidden_summary_order: Option, hidden_passed: usize, hidden_failed: usize, } #[derive(Debug)] struct CollapsedCompletedBars { - anchor: ProgressBar, removed: Vec, } +impl CollapsedCompletedBars { + fn anchor(&self) -> &ProgressBar { + &self.removed[0].progress + } +} + impl CompletedBars { fn push(&mut self, completed: HookBar) { - // Hooks can finish in a different order than their progress rows were inserted. - // Collapsing must still remove a visual prefix. + // Hooks can finish in a different order than their progress rows were inserted; + // collapse completed rows by their original visual order. let replaced = self.visible.insert(completed.line_order, completed); debug_assert!(replaced.is_none()); } @@ -148,10 +206,10 @@ impl CompletedBars { } let count = self.collapse_count(); - let anchor = self.visible.first_key_value()?.1.progress.clone(); let mut removed = Vec::with_capacity(count); for _ in 0..count { - let (_, completed) = self.visible.pop_first()?; + let (line_order, completed) = self.visible.pop_first()?; + self.hidden_summary_order.get_or_insert(line_order); match completed.passed { Some(true) => self.hidden_passed += 1, Some(false) => self.hidden_failed += 1, @@ -160,7 +218,7 @@ impl CompletedBars { removed.push(completed); } - Some(CollapsedCompletedBars { anchor, removed }) + Some(CollapsedCompletedBars { removed }) } fn record_result(&mut self, hook_key: HookKey, passed: bool) -> Option { @@ -214,11 +272,14 @@ impl CompletedBars { Some(format!("⋮ {hidden} hooks hidden: {status}")) } - fn has_visible(&self) -> bool { - !self.visible.is_empty() + fn last_visible(&self) -> Option<(usize, &ProgressBar)> { + self.visible + .last_key_value() + .map(|(line_order, completed)| (*line_order, &completed.progress)) } fn drain_visible(&mut self) -> impl Iterator { + self.hidden_summary_order = None; self.hidden_passed = 0; self.hidden_failed = 0; std::mem::take(&mut self.visible).into_values() @@ -236,10 +297,6 @@ struct HookGroup { order: usize, /// Optional project header line shown above hooks when project headers are enabled. header: Option, - /// Current insertion anchor for the next line in this project. - last_line: Option, - /// Running hook whose preview rows currently extend `last_line`. - active_tail: Option, /// Summary line for completed hooks hidden to fit the terminal height. hidden_summary: Option, /// Completed hook rows owned by this project. @@ -248,12 +305,9 @@ struct HookGroup { impl HookGroup { fn new(order: usize, header: Option) -> Self { - let last_line = header.clone(); Self { order, header, - last_line, - active_tail: None, hidden_summary: None, completed: CompletedBars::default(), } @@ -262,6 +316,31 @@ impl HookGroup { fn line_count(&self) -> usize { usize::from(self.header.is_some()) + self.completed.line_count() } + + fn insertion_anchor<'a>( + &'a self, + project_idx: usize, + running: &'a FxHashMap, + ) -> Option<&'a ProgressBar> { + let hidden_summary = self + .completed + .hidden_summary_order + .zip(self.hidden_summary.as_ref()); + let latest_running = running + .values() + .filter(|bar| bar.hook_key.project_idx == project_idx) + .max_by_key(|bar| bar.line_order) + .map(|bar| (bar.line_order, bar.visual_tail())); + let latest_completed = self.completed.last_visible(); + + hidden_summary + .into_iter() + .chain(latest_running) + .chain(latest_completed) + .max_by_key(|(line_order, _)| *line_order) + .map(|(_, progress)| progress) + .or(self.header.as_ref()) + } } /// Project groups keyed by `workspace::Project::idx()`. @@ -379,9 +458,9 @@ fn truncate_to_width(input: &str, width: usize) -> Cow<'_, str> { /// Coordinates the hook-run progress UI. /// /// `running` owns active hook bars by progress id. `groups` owns per-project -/// layout state and completed hook rows. Keeping those maps separate lets output -/// updates touch one running hook first and update the project insertion anchor -/// only when the hook grows new preview rows. +/// layout state and completed hook rows. The insertion anchor for a new hook is +/// derived from both maps while `running` is locked, so there is no separate +/// per-project cache of running row positions to keep in sync. pub(crate) struct HookRunReporter { reporter: Arc, dots: usize, @@ -389,6 +468,10 @@ pub(crate) struct HookRunReporter { /// Active hooks keyed by the id returned from `on_run_start`. running: Mutex>, /// Per-project layout and completed-hook state. + /// + /// Code paths that move rows between running and completed state lock + /// `running` before `groups`, so `on_run_start` cannot observe a hook as + /// neither running nor completed. groups: Mutex, } @@ -425,9 +508,11 @@ impl HookRunReporter { self.remove_hook_bar(completed); } let group = groups.get_mut(&project_idx).unwrap(); - let progress = self.hook_progress_bar(group.last_line.as_ref(), hook, progress_len); - group.last_line = Some(progress.clone()); - group.active_tail = Some(id); + let progress = self.hook_progress_bar( + group.insertion_anchor(project_idx, &running), + hook, + progress_len, + ); running.insert(id, HookBar::new(hook, id, progress)); id @@ -499,57 +584,61 @@ impl HookRunReporter { fn on_run_output(&self, id: usize, chunk: &[u8]) { let width = self.dots.saturating_sub(HOOK_OUTPUT_PREVIEW_PREFIX.width()); - let update = { + let removed = { let mut running = self.running.lock().unwrap(); - let update = { - let Some(run_bar) = running.get_mut(&id) else { - return; - }; - - run_bar - .push_output(&self.reporter, width, chunk) - .map(|tail| (run_bar.hook_key.project_idx, tail)) + let Some(run_bar) = running.get_mut(&id) else { + return; }; + if !run_bar.push_output(&self.reporter, width, chunk) { + return; + } - update.map(|(project_idx, tail)| (project_idx, tail, Self::running_lines(&running))) - }; - let Some((project_idx, tail, running_lines)) = update else { - return; + let running_lines = Self::running_lines(&running); + let mut groups = self.groups.lock().unwrap(); + // Growing preview rows may exceed the terminal height; hide old completed rows. + self.collapse_to_fit_new_progress(&mut groups, running_lines, 0) }; - - let mut groups = self.groups.lock().unwrap(); - if let Some(group) = groups.get_mut(&project_idx) - && group.active_tail == Some(id) - { - // New hooks in this project should be inserted after the active hook's preview. - group.last_line = Some(tail); - } - - // Growing preview rows may exceed the terminal height; hide old completed rows. - let removed = self.collapse_to_fit_new_progress(&mut groups, running_lines, 0); - drop(groups); for completed in removed { self.remove_hook_bar(completed); } } pub fn on_run_complete(&self, id: usize) { - let mut completed = { + enum CompletedPlacement { + Stored(Vec), + Orphan(HookBar), + } + + let placement = { let mut running = self.running.lock().unwrap(); - running.remove(&id).unwrap() + let mut completed = running.remove(&id).unwrap(); + self.reporter.root.inc(1); + + // Keep the completed line visible until the group result is rendered. + let progress = &completed.progress; + progress.set_position(progress.length().unwrap_or(1)); + progress.finish(); + + // Move the hook into its group before releasing `running`, so layout + // accounting never observes the main row as neither running nor completed. + let mut groups = self.groups.lock().unwrap(); + if let Some(group) = groups.get_mut(&completed.hook_key.project_idx) { + let output_bars = std::mem::take(&mut completed.output_bars); + group.completed.push(completed); + CompletedPlacement::Stored(output_bars) + } else { + CompletedPlacement::Orphan(completed) + } }; - self.reporter.root.inc(1); - for output_bar in &completed.output_bars { - self.reporter.children.remove(output_bar); + match placement { + CompletedPlacement::Stored(output_bars) => { + for output_bar in output_bars { + self.reporter.children.remove(&output_bar); + } + } + CompletedPlacement::Orphan(completed) => self.remove_hook_bar(completed), } - completed.output_bars.clear(); - - // Keep the completed line visible until the group result is rendered. - let progress = &completed.progress; - progress.set_position(progress.length().unwrap_or(1)); - progress.finish(); - self.remember_completed(id, completed); } pub fn on_run_result(&self, hook: &Hook, passed: bool) { @@ -623,20 +712,6 @@ impl HookRunReporter { self.reporter.on_complete(); } - fn remember_completed(&self, id: usize, completed: HookBar) { - let mut groups = self.groups.lock().unwrap(); - let Some(group) = groups.get_mut(&completed.hook_key.project_idx) else { - drop(groups); - self.remove_hook_bar(completed); - return; - }; - if group.active_tail == Some(id) { - group.last_line = Some(completed.progress.clone()); - group.active_tail = None; - } - group.completed.push(completed); - } - fn clear_group(&self, mut group: HookGroup) { if let Some(header) = group.header { self.reporter.children.remove(&header); @@ -668,24 +743,14 @@ impl HookRunReporter { let summary = if let Some(summary) = &group.hidden_summary { summary.clone() } else { - let summary = if let Some(header) = &group.header { - self.reporter.children.insert_after( - header, - ProgressBar::with_draw_target(None, self.reporter.printer.target()), - ) - } else { - self.reporter.children.insert_before( - anchor, - ProgressBar::with_draw_target(None, self.reporter.printer.target()), - ) - }; + let summary = self.reporter.children.insert_before( + anchor, + ProgressBar::with_draw_target(None, self.reporter.printer.target()), + ); summary.set_style(ProgressStyle::with_template("{wide_msg}").unwrap()); group.hidden_summary = Some(summary.clone()); summary }; - if !group.completed.has_visible() { - group.last_line = Some(summary.clone()); - } if group.header.is_some() { summary.set_message(format!(" {}", message.dimmed())); } else { @@ -737,7 +802,7 @@ impl HookRunReporter { let Some(collapsed) = group.completed.collapse_one_line() else { break; }; - self.update_group_summary(group, &collapsed.anchor); + self.update_group_summary(group, collapsed.anchor()); removed.extend(collapsed.removed); } @@ -765,21 +830,12 @@ mod tests { } fn project_completed_bar(project_idx: usize, hook_idx: usize, passed: Option) -> HookBar { - project_completed_bar_with_order(project_idx, hook_idx, hook_idx, passed) - } - - fn project_completed_bar_with_order( - project_idx: usize, - hook_idx: usize, - line_order: usize, - passed: Option, - ) -> HookBar { HookBar { hook_key: HookKey { project_idx, hook_idx, }, - line_order, + line_order: hook_idx, progress: ProgressBar::hidden(), output_bars: Vec::new(), output_preview: OutputPreview::default(), @@ -825,6 +881,18 @@ mod tests { ) } + fn hidden_bar(message: &'static str) -> ProgressBar { + let progress = ProgressBar::hidden(); + progress.set_message(message); + progress + } + + fn project_running_bar(project_idx: usize, hook_idx: usize, message: &'static str) -> HookBar { + let mut bar = project_completed_bar(project_idx, hook_idx, None); + bar.progress = hidden_bar(message); + bar + } + fn visible_hook_indices(completed: &CompletedBars) -> Vec { completed .visible @@ -915,9 +983,9 @@ mod tests { let reporter = HookRunReporter::new(Printer::Silent, 80, false); let mut hook_bar = running_hook_bar(&reporter, Instant::now()); - let tail = hook_bar.push_output(&reporter.reporter, 80, b"first\n"); + let inserted = hook_bar.push_output(&reporter.reporter, 80, b"first\n"); - assert!(tail.is_none()); + assert!(!inserted); assert!(hook_bar.output_bars.is_empty()); assert_eq!(hook_bar.output_preview.visible_lines(), ["first"]); } @@ -929,9 +997,9 @@ mod tests { hook_bar.push_output(&reporter.reporter, 80, b"first\n"); hook_bar.started_at = elapsed_start(); - let tail = hook_bar.push_output(&reporter.reporter, 80, b"second\n"); + let inserted = hook_bar.push_output(&reporter.reporter, 80, b"second\n"); - assert!(tail.is_some()); + assert!(inserted); let messages = hook_bar .output_bars .iter() @@ -959,7 +1027,7 @@ mod tests { let collapsed = completed.collapse_one_line().unwrap(); assert_eq!(collapsed.removed.len(), 1); - assert!(!completed.has_visible()); + assert!(visible_hook_indices(&completed).is_empty()); assert_eq!( completed.hidden_summary().as_deref(), Some("⋮ 3 hooks hidden: 2 passed, 1 failed") @@ -970,11 +1038,11 @@ mod tests { fn collapsing_completed_bars_uses_visual_order_after_out_of_order_completion() { let mut completed = CompletedBars::default(); - completed.push(project_completed_bar_with_order(0, 4, 4, Some(true))); - completed.push(project_completed_bar_with_order(0, 0, 0, Some(true))); - completed.push(project_completed_bar_with_order(0, 1, 1, Some(true))); - completed.push(project_completed_bar_with_order(0, 2, 2, Some(true))); - completed.push(project_completed_bar_with_order(0, 3, 3, Some(true))); + completed.push(project_completed_bar(0, 4, Some(true))); + completed.push(project_completed_bar(0, 0, Some(true))); + completed.push(project_completed_bar(0, 1, Some(true))); + completed.push(project_completed_bar(0, 2, Some(true))); + completed.push(project_completed_bar(0, 3, Some(true))); let collapsed = completed.collapse_one_line().unwrap(); let removed_hooks = collapsed @@ -986,6 +1054,34 @@ mod tests { assert_eq!(visible_hook_indices(&completed), [2, 3, 4]); } + #[test] + fn collapsing_completed_bars_keeps_summary_at_first_hidden_row() { + let mut group = hook_group(0, false); + + group + .completed + .push(project_completed_bar(0, 0, Some(true))); + group + .completed + .push(project_completed_bar(0, 2, Some(true))); + let collapsed = group.completed.collapse_one_line().unwrap(); + let removed_hooks = collapsed + .removed + .iter() + .map(|bar| bar.hook_key.hook_idx) + .collect::>(); + group.hidden_summary = Some(hidden_bar("summary")); + let mut running = FxHashMap::default(); + running.insert(1, project_running_bar(0, 1, "running-1")); + + assert_eq!(removed_hooks, [0, 2]); + assert_eq!(group.completed.hidden_summary_order, Some(0)); + assert_eq!( + group.insertion_anchor(0, &running).unwrap().message(), + "running-1" + ); + } + #[test] fn collapsing_requires_a_known_result_prefix() { let mut completed = CompletedBars::default(); @@ -1068,15 +1164,19 @@ mod tests { let mut group = HookGroup::new(0, Some(progress_bar(&reporter))); group.completed.hidden_passed = 2; group.completed.hidden_failed = 1; + let anchor = progress_bar(&reporter); + + group.completed.hidden_summary_order = Some(2); - reporter.update_group_summary(&mut group, &ProgressBar::hidden()); + reporter.update_group_summary(&mut group, &anchor); let summary = group.hidden_summary.as_ref().unwrap(); let message = summary.message().clone(); assert!(message.starts_with(" ")); assert!(message.contains("⋮ 3 hooks hidden: 2 passed, 1 failed")); + let running = FxHashMap::default(); assert_eq!( - group.last_line.as_ref().unwrap().message(), + group.insertion_anchor(0, &running).unwrap().message(), summary.message() ); } @@ -1088,18 +1188,85 @@ mod tests { let mut group = hook_group(0, false); group.completed.hidden_failed = 1; + group.completed.hidden_summary_order = Some(0); + reporter.update_group_summary(&mut group, &anchor); let summary = group.hidden_summary.as_ref().unwrap(); let message = summary.message().clone(); assert!(!message.starts_with(" ")); assert!(message.contains("⋮ 1 hooks hidden: 1 failed")); + let running = FxHashMap::default(); assert_eq!( - group.last_line.as_ref().unwrap().message(), + group.insertion_anchor(0, &running).unwrap().message(), summary.message() ); } + #[test] + fn insertion_anchor_prefers_running_tail_over_hidden_summary() { + let mut group = hook_group(0, false); + group.completed.hidden_summary_order = Some(0); + group.hidden_summary = Some(hidden_bar("summary")); + let mut running = FxHashMap::default(); + running.insert(1, project_running_bar(0, 1, "running")); + + assert_eq!( + group.insertion_anchor(0, &running).unwrap().message(), + "running" + ); + } + + #[test] + fn insertion_anchor_uses_hidden_summary_when_it_is_visual_tail() { + let mut group = hook_group(0, false); + group.completed.hidden_summary_order = Some(2); + group.hidden_summary = Some(hidden_bar("summary")); + let mut running = FxHashMap::default(); + running.insert(0, project_running_bar(0, 0, "running-0")); + + assert_eq!( + group.insertion_anchor(0, &running).unwrap().message(), + "summary" + ); + } + + #[test] + fn insertion_anchor_uses_latest_visible_line_order() { + let mut group = hook_group(0, false); + let mut running = FxHashMap::default(); + running.insert(1, project_running_bar(0, 1, "running-1")); + let completed = project_completed_bar(0, 2, Some(true)); + completed.progress.set_message("completed-2"); + group.completed.push(completed); + + assert_eq!( + group.insertion_anchor(0, &running).unwrap().message(), + "completed-2" + ); + + running.insert(3, project_running_bar(0, 3, "running-3")); + + assert_eq!( + group.insertion_anchor(0, &running).unwrap().message(), + "running-3" + ); + } + + #[test] + fn insertion_anchor_uses_running_output_tail() { + let group = hook_group(0, false); + let mut running_bar = project_running_bar(0, 1, "running-main"); + running_bar.output_bars.push(hidden_bar("running-tail")); + let mut running = FxHashMap::default(); + running.insert(1, running_bar); + + assert_eq!( + group.insertion_anchor(0, &running).unwrap().message(), + "running-tail" + ); + } + #[test] fn update_group_summary_is_noop_without_hidden_completed() { let reporter = HookRunReporter::new(Printer::Silent, 80, false); @@ -1109,6 +1276,7 @@ mod tests { reporter.update_group_summary(&mut group, &anchor); assert!(group.hidden_summary.is_none()); - assert!(group.last_line.is_none()); + let running = FxHashMap::default(); + assert!(group.insertion_anchor(0, &running).is_none()); } }