From 32c7a5be0b9ff8708c40ccbd508f4bbe5b4b9aeb Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 00:19:29 -0400 Subject: [PATCH 01/16] docs(review): ADR-037 progressive pipeline design --- docs/adr/037-review-progressive-pipeline.md | 145 ++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 docs/adr/037-review-progressive-pipeline.md diff --git a/docs/adr/037-review-progressive-pipeline.md b/docs/adr/037-review-progressive-pipeline.md new file mode 100644 index 00000000..3d3d950d --- /dev/null +++ b/docs/adr/037-review-progressive-pipeline.md @@ -0,0 +1,145 @@ +# 037 — Review TUI: Progressive Pipeline (Threads, Streaming Acquisition, Generation-Tagged Loads) + +Status: accepted (2026-07-10, progressive-pipeline design session) + +## Context + +The M7 performance pass (perf-gt-detect … perf-pty-responsiveness) removed the worst +launch and navigation stalls, but two synchronous gaps remain: the idle-deferred file load +runs on the event-loop thread (a huge file holds input hostage for its own load once the +80ms debounce fires), and startup diffs complete in full — behind the splash, but not +streamed — before the outline appears. Closing them means work moves off the event-loop +thread, which **supersedes M4's locked decision #4** ("a synchronous poll on the existing +`Tick`… No threads, no `mpsc`, no new deps" — recorded in `tui.rs`'s module doc, not an +ADR). This ADR retires the "no threads" letter of that decision while keeping its "no new +deps" spirit: everything below is `std::sync::mpsc` + `std::thread`. Zero new dependencies. + +Scope: streamed startup acquisition, off-thread file loads, a dedicated input thread, and +the refresh path riding the same pipeline (one acquisition path, not two). Explicitly out +of scope: overlapping the `theme=auto` terminal probe with acquisition (deprioritized; a +later changeset can reuse this seam). Nothing here reorders startup ahead of the theme +probe / `flush_pending_tty_input` sequence — that ordering is load-bearing (see the +pty_smoke silent-terminal canary). + +## Decision + +**Topology — three permanent threads plus transient wave workers.** The *main thread* +owns `App`, rendering, and every repo **write** (staging stays synchronous). The *input +thread* is the sole reader of terminal events: a blocking `crossterm::event::read()` loop +forwarding into the inbox. The *loader thread* owns its own long-lived `Repository` + +`TsHighlighter`; it serves file-load requests sequentially and, for a whole-stack diff +wave (startup, refresh), spawns a transient scoped worker pool — per-worker `Repository`, +exactly today's `diff_changesets` striping — but **streams each changeset's result as it +completes** instead of joining the batch. The parallel-diff win and streaming compose. + +**Protocol — one inbox, stateless loader.** A single `mpsc` inbox feeds the main loop; +`recv_timeout` replaces `event::poll`, and the timeout *is* the Tick beat (index-watcher +poll and the 80ms open-debounce survive unchanged as timeout arms). `AppEvent` grows +loader-result variants — `ChangesetReady { gen, idx, result }` and +`FileReady { gen, cs_idx, file_idx, views }` — and loses `derive(Copy)`. `drain_pending` +becomes a `try_recv` loop, so nav coalescing in `update_batch` carries over untouched. The +loader is stateless between jobs: each request carries what it needs (cloned `FileChange` + +span; content is read through the loader's own repo handle). `App` stays the single owner +of diff truth — no second copy of the stack to keep coherent across refreshes. + +**Generations — one global `u64`, mismatch is the only drop rule.** The invariant: +*generation bumps ⟺ the view caches were invalidated* (launch is gen 1; every refresh +bumps). Requests are stamped at send; results carry the stamp; the main loop discards +mismatches at one chokepoint. Within a generation every `FileReady` is cached **even if +the user navigated away** — the diff hasn't changed, so an early result is warmth, not +staleness (A→B→A bounces land on a warm A). The loader never decides staleness. +Per-changeset generations were rejected: refresh rebuilds view caches wholesale, so finer +tags would model granularity the app doesn't have. + +**Slots — `Pending | Ready | Failed` per changeset.** `App` is constructible from +resolved-but-undiffed changesets: all slots `Pending`, `current_cs` from lib-`current` +(metadata only), outline headers render immediately and file rows fill in per +`ChangesetReady`. Navigating onto a `Pending` changeset shows the existing placeholder +treatment. Waves diff the **current changeset first**, then input order — the changeset +the user lands on becomes interactive earliest, and the splash becomes redundant for +stacks (the first real frame is the live outline). + +**The lone-changeset launch stays synchronous.** `main.rs` forks on `changesets.len()`: +one changeset (non-Graphite default, ref/range, PR) keeps today's sync diff + empty-check ++ splash byte-identical. Streaming's grain is per-changeset, so a 1-changeset review gains +nothing from it — and the "nothing to review" exit-0 must stay tty-free (the +`clean_worktree_prints_nothing_to_review_and_exits_success` canary runs with no terminal; +an in-TUI empty-detection can never serve it). Consequently `App::from_changesets`'s +≥1 assert survives unchanged. + +**Force-completion — synchronous fallback on the main thread.** The `apply_action` +chokepoint keeps its meaning: an action that reads the view (`s`, cursor moves, selection) +finds the cache warm or loads *synchronously right there* — `App` keeps its own +`Repository` + `TsHighlighter` for exactly this and for staging. The in-flight loader +result later hits "already cached" and is discarded. The loader is thereby a **pure +cache-warmer: correctness never depends on it**, and the CS4 invariant (deferred-then- +completed open ≡ eager open, byte-identical) survives trivially. Accepted cost: `s` on a +just-reached huge file can still block for that file's load — the price of byte-identical +action semantics without action-replay machinery (queueing actions until `FileReady` was +rejected: replay ordering hazards for a rare case). Highlight determinism across the two +highlighter instances holds — highlighting is a pure function of content + grammar +(ADR-035's theme-free design). + +**Refresh — sync resolve, span-keyed reuse, uncommitted always sync.** Resolve stays on +the main thread (offline, cheap; PR sources remain refresh-no-ops). The rebuilt view list +carries over any `Ready` slot whose `(name, span)` is unchanged — a committed diff is a +pure function of its span — so an ordinary post-staging refresh re-diffs *nothing but the +uncommitted layer*, and a restack streams only what moved ("never blank" holds by +construction: stale-but-present content renders until replaced). The **uncommitted layer +always re-diffs synchronously** in every refresh: it is ms-scale, and this preserves +staging's guarantee that the next keystroke sees the post-op world — an async refresh +would let a second `s` compute its patch against a stale diff. One refresh shape; no +staging-vs-manual modes. Every refresh bumps the generation (reused-slot in-flight loads +die valid at the inbox; accepted waste for one global rule). + +**Failures — per-changeset degradation for stacks, fatal only where it's the whole +review.** `ChangesetReady { result: Err }` sets that slot `Failed`: the outline marks it, +navigating to it renders the error, the wave's first failure raises a footer notice, and +the review continues (34 reviewable changesets beat zero). The lone-changeset sync path +keeps today's pre-loop fatal miette exit. `r` is the retry — span reuse only carries +`Ready` slots. Contract change accepted: a stack review with one corrupt changeset now +exits 0 on quit where it previously died non-zero; the tty-less paths are unchanged. + +**Lifecycle — kill-on-exit, one justified `catch_unwind`.** No join on quit: the loader +never writes, so killing it mid-read corrupts nothing, and a join only adds quit lag. The +loader wraps each job in `catch_unwind`, converting a job panic into a `Failed` result — +the specific class this catches that nothing else does: a panicked job silently drops into +slots stranded `Pending` forever (the inbox stays connected via the input thread's +sender), an invisible hang instead of a visible error. Input-thread read errors are +forwarded into the inbox and exit the loop as `io::Error`, same observable behavior as +today. + +**Rejected: an async runtime (Tokio).** It relocates this complexity rather than removing +it: every job is blocking (libgit2 C calls, tree-sitter CPU, tty reads), so all work lands +in `spawn_blocking` — the same threads plus a runtime. `select!` buys nothing over the +single-inbox `recv_timeout`; future-cancellation cannot interrupt blocking work and +doesn't replace generation tags (the race is results-already-computed, not work-in- +flight); and the force-completion *synchronous* fallback — load-bearing for staging +correctness — is trivial in sync code and a genuine problem inside a task. The hard parts +of this design are state-model decisions that survive any executor. + +**Testing — real threads confined to one smoke layer.** (1) The loader job body is a pure +function `LoadRequest → AppEvent`, unit-tested synchronously (diff correctness, error +wrapping, panic-to-`Failed`). (2) Loop behavior is tested by feeding synthetic event +sequences through `update_batch` — slot transitions, gen drops, within-gen cache-after- +nav-away, span reuse, and the carried eager-equivalence invariant — no threads, no flake +surface. (3) One real-thread integration smoke plus a `pty_responsiveness` extension +asserting the first interactive frame lands before a full wave could have finished +(`#[ignore]`, run solo, per the existing wall-clock caveat). Existing eager-mode tests +stay untouched (defer off, slots constructed `Ready`). + +## Consequences + +- `tui.rs`'s module doc note pinning M4 locked decision #4 must be rewritten to point + here; the M4 index-watcher *semantics* (signature compare on the tick beat, echo + suppression) are unchanged — only the beat's mechanism moves from `event::poll` timeout + to `recv_timeout`. +- `AppEvent` stops being `Copy`; `drain_pending`/`next_event` reshape around the inbox; + the input thread becomes the only code that touches crossterm's event API. +- The splash survives only on the lone-changeset path; for stacks the first frame is the + live outline with `Pending` rows. +- Two visible behavior changes, both accepted: post-restack refreshes show placeholders + for moved changesets while unmoved ones stay readable (today the whole UI freezes), and + a corrupt changeset in a stack degrades to a `Failed` row instead of killing the launch. +- `App` and the loader each hold a `TsHighlighter`; grammar caches are duplicated + per-instance (modest, accepted for the sync-fallback guarantee). From f2a7a130d961963cd39f785bed3ddd5f1b0c170d Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 00:42:12 -0400 Subject: [PATCH 02/16] feat(review): per-changeset Pending/Ready/Failed slots --- git-workon-review/src/app.rs | 196 ++++++++++++++++++++++++++++++- git-workon-review/src/outline.rs | 71 +++++++++++ git-workon-review/src/render.rs | 28 +++++ 3 files changed, 293 insertions(+), 2 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index a4546454..057915ed 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -604,6 +604,27 @@ pub struct ChangesetView { views_combined: Vec>, views_unstaged: Vec>, views_staged: Vec>, + /// ADR-037's per-changeset acquisition state. `Ready` for every changeset this changeset + /// (this revision of the codebase) actually diffs through; `Pending`/`Failed` slots are + /// constructible today (state model + rendering) but nothing in the synchronous startup/ + /// refresh paths produces them yet — that lands with the streamed-acquisition changesets. + slot: ChangesetSlot, +} + +/// A [`ChangesetView`]'s acquisition state (ADR-037's "Slots" decision). `diff`/the `views_*` +/// caches stay meaningful only for `Ready` — a `Pending`/`Failed` view's [`DiffState`] is always +/// [`DiffState::empty`], so every existing `.diff.`-reading call site (file counts, outline +/// rows, nav guards) already treats it as "nothing to show" with no per-site branch needed; only +/// the render/outline paths that must actively DISTINGUISH the three states (vs. a genuinely +/// empty `Ready` changeset) read this directly. +#[derive(Debug, Clone, PartialEq, Eq)] +enum ChangesetSlot { + /// Acquisition hasn't run (or hasn't completed) for this changeset yet. + Pending, + /// The diff (and its view caches) are real. + Ready, + /// The acquisition attempt errored; the message is shown in place of a diff body. + Failed(String), } impl ChangesetView { @@ -615,6 +636,43 @@ impl ChangesetView { views_combined: (0..n).map(|_| None).collect(), views_unstaged: (0..n).map(|_| None).collect(), views_staged: (0..n).map(|_| None).collect(), + slot: ChangesetSlot::Ready, + } + } + + /// Construct a `Pending` slot for `cs` (ADR-037): no diff acquired yet. The outline shows + /// its header with a loading indication; navigating onto it renders a changeset-level + /// placeholder instead of "(no changes)"/per-file content. + pub fn pending(cs: Changeset) -> Self { + let mut view = Self::new(cs, DiffState::empty()); + view.slot = ChangesetSlot::Pending; + view + } + + /// Construct a `Failed` slot for `cs` carrying `message` (ADR-037): the acquisition attempt + /// for this changeset errored. The outline marks it; navigating onto it renders `message` + /// instead of a diff body. + pub fn failed(cs: Changeset, message: impl Into) -> Self { + let mut view = Self::new(cs, DiffState::empty()); + view.slot = ChangesetSlot::Failed(message.into()); + view + } + + /// Whether this changeset's diff hasn't been acquired yet (ADR-037). + pub fn is_pending(&self) -> bool { + matches!(self.slot, ChangesetSlot::Pending) + } + + /// Whether this changeset's acquisition attempt errored (ADR-037). + pub fn is_failed(&self) -> bool { + matches!(self.slot, ChangesetSlot::Failed(_)) + } + + /// This changeset's failure message, if [`Self::is_failed`] — `None` for `Pending`/`Ready`. + pub fn failure_message(&self) -> Option<&str> { + match &self.slot { + ChangesetSlot::Failed(msg) => Some(msg.as_str()), + ChangesetSlot::Pending | ChangesetSlot::Ready => None, } } @@ -991,6 +1049,19 @@ impl App { self.current_cs } + /// Whether the ACTIVE changeset's slot is `Pending` (ADR-037) — `render.rs`'s body path + /// shows a changeset-level loading placeholder instead of "(no changes)"/per-file content + /// while this holds. + pub fn is_current_pending(&self) -> bool { + self.cur().is_pending() + } + + /// The ACTIVE changeset's failure message, if its slot is `Failed` (ADR-037) — `render.rs`'s + /// body path shows this instead of a diff body. + pub fn current_failure(&self) -> Option<&str> { + self.cur().failure_message() + } + /// The active changeset's descriptor (name, source, restack status) — read by tests /// asserting which changeset [`Self::current_cs`] landed on. pub fn current_changeset(&self) -> &Changeset { @@ -1640,6 +1711,8 @@ impl App { label: v.cs.title.clone().unwrap_or_else(|| v.cs.name.clone()), current: v.cs.current, needs_restack: v.cs.needs_restack, + loading: v.is_pending(), + failed: v.is_failed(), files: v .files() .iter() @@ -2611,6 +2684,20 @@ impl From for DiffState { } impl DiffState { + /// An empty [`DiffState`] — every field zero-length. Used for `Pending`/`Failed` + /// [`ChangesetView`] slots (ADR-037), which carry no real diff; existing `.diff.` read sites + /// already treat an empty `files` list as "nothing to show," so this alone is enough to make + /// those slots render/navigate as inert with no per-site Pending/Failed branch. + fn empty() -> Self { + Self { + files: Vec::new(), + unstaged_model: DiffModel { files: Vec::new() }, + staged_model: DiffModel { files: Vec::new() }, + unstaged_idx: Vec::new(), + staged_idx: Vec::new(), + } + } + /// Build a [`DiffState`] for a COMMITTED changeset's [`DiffModel`] (`base..head`, already /// diffed by [`crate::acquire::diff_committed`]) — there is no staged/unstaged split for a /// committed range, so both sub-models are empty and every index map entry is `None`. This @@ -2797,8 +2884,8 @@ mod tests { use super::test_support::app_from_fixture; use super::{ - find_next_hunk_row, find_prev_hunk_row, App, ChangesetView, EffectiveZoom, Layout, Role, - Zoom, DEFAULT_OUTLINE_WIDTH, + find_next_hunk_row, find_prev_hunk_row, App, ChangesetView, DiffState, EffectiveZoom, + Layout, Role, Zoom, DEFAULT_OUTLINE_WIDTH, }; use crate::align::{AlignedRow, CellKind, DisplayRow, InlineRow, Row}; use crate::config::ReviewConfig; @@ -5538,6 +5625,8 @@ mod tests { label: "cs-a".to_string(), current: false, needs_restack: false, + loading: false, + failed: false, } ); let header_b = items @@ -5551,10 +5640,113 @@ mod tests { label: "cs-b".to_string(), current: true, needs_restack: true, + loading: false, + failed: false, } ); } + // ── ADR-037: per-changeset slots (Pending/Ready/Failed) ───────────────────── + + /// A minimal [`Changeset`] descriptor for the slot tests below — the slot model only cares + /// about the metadata `ChangesetView::pending`/`failed` carry alongside a diff-free + /// [`DiffState`], not any real git content. + fn bare_changeset(name: &str, current: bool) -> Changeset { + Changeset { + name: name.to_string(), + span: ChangesetSpan::Uncommitted, + title: None, + current, + needs_restack: false, + } + } + + #[test] + fn app_is_constructible_from_a_pending_changeset_alone() { + // ADR-037: `App::from_changesets`'s >=1 assert survives unchanged — an all-Pending stack + // (the streamed-launch shape, before any diff has landed) is a valid `App`. + let view = ChangesetView::pending(bare_changeset("cs-a", true)); + let fixture = FixtureBuilder::new().build().unwrap(); + let repo = Repository::open(fixture.repo().unwrap().workdir().unwrap()).unwrap(); + let app = App::from_changesets(repo, vec![view]); + + assert!(app.is_current_pending()); + assert_eq!(app.current_failure(), None); + assert!(app.files().is_empty()); + assert_eq!(app.changeset_count(), 1); + } + + #[test] + fn navigating_onto_a_pending_changeset_shows_no_files_and_stays_pending() { + let fixture = two_changes_one_hunk_fixture(); + let repo = fixture.repo().unwrap(); + let cs_ready = bare_changeset("cs-ready", true); + let diffs = crate::acquire::diff_uncommitted(repo).unwrap(); + let view_ready = ChangesetView::new(cs_ready, DiffState::from(diffs)); + let view_pending = ChangesetView::pending(bare_changeset("cs-pending", false)); + + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view_ready, view_pending]); + + assert!(!app.is_current_pending(), "opens on the Ready changeset"); + assert!(!app.files().is_empty()); + + app.next_changeset(); + + assert_eq!(app.current_cs(), 1); + assert!(app.is_current_pending()); + assert!( + app.files().is_empty(), + "a Pending changeset has no file rows to navigate onto" + ); + } + + #[test] + fn failed_slot_carries_its_error_message() { + let view = ChangesetView::failed(bare_changeset("cs-a", true), "diff acquisition failed"); + let fixture = FixtureBuilder::new().build().unwrap(); + let repo = Repository::open(fixture.repo().unwrap().workdir().unwrap()).unwrap(); + let app = App::from_changesets(repo, vec![view]); + + assert!(!app.is_current_pending()); + assert_eq!(app.current_failure(), Some("diff acquisition failed")); + assert!(app.files().is_empty()); + } + + #[test] + fn outline_marks_pending_and_failed_changeset_headers() { + let view_pending = ChangesetView::pending(bare_changeset("cs-pending", true)); + let view_failed = ChangesetView::failed(bare_changeset("cs-failed", false), "boom"); + let fixture = FixtureBuilder::new().build().unwrap(); + let repo = Repository::open(fixture.repo().unwrap().workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(repo, vec![view_pending, view_failed]); + app.outline.mode = OutlineMode::Stack; + + let items = app.outline_items(); + assert_eq!( + items, + vec![ + OutlineItem::Header { + cs_idx: 0, + label: "cs-pending".to_string(), + current: true, + needs_restack: false, + loading: true, + failed: false, + }, + OutlineItem::Header { + cs_idx: 1, + label: "cs-failed".to_string(), + current: false, + needs_restack: false, + loading: false, + failed: true, + }, + ], + "Pending/Failed changesets emit only their (marked) header, no file rows" + ); + } + #[test] fn staged_status_column_only_populated_for_the_uncommitted_changesets_files() { let mut app = committed_and_uncommitted_stack(); diff --git a/git-workon-review/src/outline.rs b/git-workon-review/src/outline.rs index 27dd12d3..f4328087 100644 --- a/git-workon-review/src/outline.rs +++ b/git-workon-review/src/outline.rs @@ -108,6 +108,12 @@ pub struct OutlineChangeset { pub current: bool, /// Mirrors `workon::Changeset::needs_restack` — drives the outline's amber warning glyph. pub needs_restack: bool, + /// ADR-037: the changeset's diff hasn't been acquired yet — the header shows a loading + /// indication in place of the (currently absent, since `files` is empty for a `Pending` + /// slot) file rows. + pub loading: bool, + /// ADR-037: the acquisition attempt for this changeset errored — the header marks it. + pub failed: bool, pub files: Vec, } @@ -132,6 +138,10 @@ pub enum OutlineItem { label: String, current: bool, needs_restack: bool, + /// ADR-037: this changeset hasn't been diffed yet — rendered as a loading indication. + loading: bool, + /// ADR-037: this changeset's acquisition attempt errored — rendered as a marker. + failed: bool, }, /// A directory row — only emitted in [`OutlineMode::Tree`]/[`OutlineMode::StackTree`]. Not a /// jump target: it carries no `cs_idx`/`file_idx`, so `App::outline_move_by` no-ops on it @@ -187,6 +197,8 @@ fn build_stack(changesets: &[OutlineChangeset]) -> Vec { label: cs.label.clone(), current: cs.current, needs_restack: cs.needs_restack, + loading: cs.loading, + failed: cs.failed, }); for (file_idx, file) in cs.files.iter().enumerate() { items.push(OutlineItem::File { @@ -353,6 +365,8 @@ fn build_stack_tree(changesets: &[OutlineChangeset]) -> Vec { label: cs.label.clone(), current: cs.current, needs_restack: cs.needs_restack, + loading: cs.loading, + failed: cs.failed, }); let mut root = TrieNode::default(); for (file_idx, file) in cs.files.iter().enumerate() { @@ -378,6 +392,8 @@ mod tests { label: label.to_string(), current, needs_restack, + loading: false, + failed: false, files: files .iter() .map(|(p, s)| OutlineFile { @@ -388,6 +404,19 @@ mod tests { } } + /// [`cs`] variant for ADR-037's slot tests — builds a `Pending`/`Failed` outline changeset + /// (no files, since a non-`Ready` [`crate::app::ChangesetView`] never has any). + fn cs_slot(label: &str, loading: bool, failed: bool) -> OutlineChangeset { + OutlineChangeset { + label: label.to_string(), + current: false, + needs_restack: false, + loading, + failed, + files: Vec::new(), + } + } + #[test] fn stack_mode_emits_a_header_before_each_changesets_files() { let changesets = vec![ @@ -403,6 +432,8 @@ mod tests { label: "cs-a".to_string(), current: false, needs_restack: false, + loading: false, + failed: false, }, OutlineItem::File { cs_idx: 0, @@ -416,6 +447,8 @@ mod tests { label: "cs-b".to_string(), current: true, needs_restack: true, + loading: false, + failed: false, }, OutlineItem::File { cs_idx: 1, @@ -428,6 +461,40 @@ mod tests { ); } + /// ADR-037: a `Pending`/`Failed` changeset (no files) still emits a Stack-mode header row, + /// carrying the loading/failed marker instead of any file rows. + #[test] + fn stack_mode_marks_pending_and_failed_headers_with_no_file_rows() { + let changesets = vec![ + cs_slot("cs-pending", true, false), + cs_slot("cs-failed", false, true), + ]; + let items = build_items(&changesets, OutlineMode::Stack); + assert_eq!( + items, + vec![ + OutlineItem::Header { + cs_idx: 0, + label: "cs-pending".to_string(), + current: false, + needs_restack: false, + loading: true, + failed: false, + }, + OutlineItem::Header { + cs_idx: 1, + label: "cs-failed".to_string(), + current: false, + needs_restack: false, + loading: false, + failed: true, + }, + ], + "a Pending/Failed changeset carries no file rows (its files list is empty), only its \ + own marked header" + ); + } + #[test] fn flat_mode_has_no_headers() { let changesets = vec![cs( @@ -625,6 +692,8 @@ mod tests { label: "cs-a".to_string(), current: false, needs_restack: false, + loading: false, + failed: false, }, OutlineItem::Dir { name: "x".to_string(), @@ -642,6 +711,8 @@ mod tests { label: "cs-b".to_string(), current: true, needs_restack: true, + loading: false, + failed: false, }, OutlineItem::File { cs_idx: 1, diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs index cfd053b4..a2cd0845 100644 --- a/git-workon-review/src/render.rs +++ b/git-workon-review/src/render.rs @@ -535,6 +535,8 @@ fn build_outline_line(item: &OutlineItem, theme: &Palette) -> Line<'static> { label, current, needs_restack, + loading, + failed, .. } => { let marker = if *current { "\u{25CF} " } else { " " }; @@ -551,6 +553,13 @@ fn build_outline_line(item: &OutlineItem, theme: &Palette) -> Line<'static> { if *needs_restack { spans.push(TSpan::styled(" \u{26A0}", Style::default().fg(FG_WARN))); } + // ADR-037: a Failed changeset's marker wins over Pending's (a slot is never both, + // but Failed is the more actionable state to surface if it somehow were). + if *failed { + spans.push(TSpan::styled(" \u{2717}", Style::default().fg(FG_ERROR))); + } else if *loading { + spans.push(TSpan::styled(" \u{2026}", Style::default().fg(theme.dim))); + } Line::from(spans) } OutlineItem::Dir { name, guides } => { @@ -766,6 +775,25 @@ fn render_loading_placeholder( } fn render_body(frame: &mut Frame, app: &mut App, area: Rect, theme: &Palette) { + // ADR-037: the active changeset's diff hasn't been acquired (or failed to acquire) yet — + // both cases have an empty `files()` list, so they must be checked BEFORE the "(no changes)" + // fallback below, which would otherwise misreport a Pending/Failed changeset as an + // intentionally empty one. + if let Some(message) = app.current_failure() { + let msg = format!("Failed to load this changeset: {message}"); + frame.render_widget( + Paragraph::new(msg).style(Style::default().fg(FG_ERROR)), + area, + ); + return; + } + if app.is_current_pending() { + frame.render_widget( + Paragraph::new("Loading\u{2026}").style(Style::default().fg(theme.dim)), + area, + ); + return; + } if app.files().is_empty() { frame.render_widget(Paragraph::new("(no changes)"), area); return; From 6c91066b3d124a4811999466c4fc5f6168fb4577 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 00:56:27 -0400 Subject: [PATCH 03/16] feat(review): route input through a dedicated thread and mpsc inbox --- git-workon-review/src/tui.rs | 281 +++++++++++++++++++++++++++++------ 1 file changed, 234 insertions(+), 47 deletions(-) diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index c10e9977..fd5b050d 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -1,17 +1,25 @@ //! Terminal lifecycle, event seam, and the main input loop for the review TUI. //! //! Ported loop shape from the `review-tui-spike` prototype's `main.rs` (`install_panic_hook`, -//! raw-mode + alternate-screen setup, `draw -> quit-check -> next_event -> update`), adapted to -//! read events through [`next_event`] rather than calling crossterm directly from the loop. +//! raw-mode + alternate-screen setup, `draw -> quit-check -> recv_event -> update`), adapted to +//! read events through the [`AppEvent`] inbox rather than calling crossterm directly from the +//! loop. //! -//! M4's index watcher (locked decision #4) does NOT swap `next_event`'s internals for a -//! channel-fed watcher thread, despite an earlier note here suggesting that direction — the -//! locked decision is a synchronous poll on the existing `Tick` (every `next_event` timeout), -//! comparing [`workon_review::refresh::IndexSignature`] and re-diffing in place via -//! [`App::on_tick`] when it changes. No threads, no `mpsc`, no new deps. +//! ADR-037 (progressive pipeline) supersedes M4's locked decision #4 — the "no threads, no +//! `mpsc`" letter of that note, recorded here in an earlier revision, no longer holds. A +//! dedicated *input thread* (spawned by [`Tui::run`]) is now the ONLY code that calls +//! crossterm's event API: it blocks on `event::read()` forever, maps each event exactly like +//! this module's old `next_event`/`drain_pending` read arms did, and forwards mapped events into +//! an `std::sync::mpsc` inbox that the main loop drains via [`recv_event`]/[`drain_pending`]. +//! `recv_timeout`'s timeout arm IS the `Tick` beat — unchanged from before, just relocated from +//! `event::poll`'s timeout to the channel's. The M4 index watcher's *semantics* are exactly +//! unchanged by this move: it still compares [`workon_review::refresh::IndexSignature`] and +//! re-diffs in place via [`App::on_tick`] on every `Tick`; only the beat's mechanism moved. use std::fs::File; use std::io::{self, Write}; +use std::sync::mpsc; +use std::thread; use std::time::Duration; use crossterm::event::{self, Event, KeyCode, KeyEvent, KeyEventKind}; @@ -28,51 +36,104 @@ use workon_review::keymap::{Command, Dispatch, KeyPress, Keymap}; use workon_review::render; use workon_review::theme::Palette; -/// One event the review loop reacts to. `Tick` is now also the index-watcher's poll beat (see the -/// module doc's note on locked decision #4) — `next_event`'s mapping and this enum otherwise stay -/// the shape M3 built. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// One event the review loop reacts to. `Tick` is synthesized by the main loop on an inbox +/// `recv_timeout` timeout — it is never sent through the channel itself (see [`recv_event`]). +/// `Key`/`Resize` are forwarded from the input thread via [`map_terminal_event`]. Not `Copy` +/// (ADR-037): the next slice's loader-result variants carry non-`Copy` payloads; dropping `Copy` +/// now is mechanical prep so this slice's diff doesn't collide with that one's. +#[derive(Debug, Clone, PartialEq, Eq)] pub enum AppEvent { Key(KeyEvent), Resize(u16, u16), Tick, } -/// Poll for the next terminal event, up to `timeout`. -/// -/// `Ok(Some(AppEvent::Tick))` on a plain timeout (the loop's regular redraw beat); `Ok(None)` for -/// a terminal event we don't map to an [`AppEvent`] (key release/repeat, mouse, paste, focus) — -/// the loop redraws and keeps going without calling `update`. -pub fn next_event(timeout: Duration) -> io::Result> { - if !event::poll(timeout)? { - return Ok(Some(AppEvent::Tick)); - } - Ok(match event::read()? { +/// The inbox message type: a mapped terminal event, or the input thread's terminal `event::read` +/// error forwarded verbatim (ADR-037: "the input thread never exits silently" — a read error is +/// still observable, just relayed rather than swallowed). `Tick` never appears here. +type InboxMessage = io::Result; + +/// Map one crossterm terminal [`Event`] to the [`AppEvent`] the loop reacts to — key-press and +/// resize map; key release/repeat, mouse, paste, and focus events are skipped (`None`), exactly +/// like this module's pre-ADR-037 `next_event`/`drain_pending` read arms did. Pure and +/// independent of any thread or channel, so it's unit-tested directly; the input thread's loop +/// body is a thin wrapper around it. +fn map_terminal_event(event: Event) -> Option { + match event { Event::Key(key) if key.kind == KeyEventKind::Press => Some(AppEvent::Key(key)), Event::Resize(w, h) => Some(AppEvent::Resize(w, h)), _ => None, - }) + } +} + +/// Spawn the dedicated input thread and return the receiving end of its inbox. Must be called +/// AFTER the terminal is acquired and any pre-takeover tty work (the theme probe, stray-input +/// flush) has finished — crossterm input must not be consumed before that ordering completes +/// (see `main.rs`'s block comment on the resolve/probe/acquire sequence). The thread loops +/// forever on a blocking `event::read()`, forwarding mapped events; on a read error it forwards +/// the error once and exits — the sole way this thread ever stops short of the process dying. +/// Never joined: [`Tui::run`] returns without waiting for it (ADR-037's kill-on-exit lifecycle — +/// the input thread, like the future loader thread, never writes, so an abandoned read can't +/// corrupt anything). +fn spawn_input_thread() -> mpsc::Receiver { + let (tx, rx) = mpsc::channel(); + thread::spawn(move || loop { + match event::read() { + Ok(event) => { + if let Some(mapped) = map_terminal_event(event) { + if tx.send(Ok(mapped)).is_err() { + return; // main loop is gone; nothing left to forward to + } + } + } + Err(err) => { + let _ = tx.send(Err(err)); + return; + } + } + }); + rx +} + +/// Receive the next event from `inbox`, waiting up to `timeout`. A timeout with nothing received +/// yields `Ok(AppEvent::Tick)` — the loop's regular redraw beat, and the mechanism the M4 index +/// watcher polls on (see the module doc). A disconnected inbox (the input thread panicked, or +/// exited after an error without this being observed yet) is surfaced as an `io::Error` rather +/// than spinning — the loop must exit, not busy-loop on an empty channel forever. +fn recv_event(inbox: &mpsc::Receiver, timeout: Duration) -> io::Result { + match inbox.recv_timeout(timeout) { + Ok(Ok(event)) => Ok(event), + Ok(Err(err)) => Err(err), + Err(mpsc::RecvTimeoutError::Timeout) => Ok(AppEvent::Tick), + Err(mpsc::RecvTimeoutError::Disconnected) => Err(io::Error::other( + "review TUI input thread disconnected without a final error", + )), + } } /// Cap on how many events [`drain_pending`] batches per iteration — leftover input past this -/// count is simply picked up by the next iteration's `next_event` call. +/// count is simply picked up by the next iteration's `recv_event` call. const MAX_DRAIN_BATCH: usize = 128; -/// Drain all immediately-available terminal events into `batch`, mapping them exactly like -/// [`next_event`]'s read arm (key-press and resize map; release/repeat/mouse/paste/focus are -/// skipped, not pushed). Unlike calling `next_event(Duration::ZERO)` in a loop, a not-ready poll -/// here simply stops draining — it must NOT fabricate a `Tick`, since `next_event`'s `!poll` arm -/// exists solely to give the loop its regular redraw beat on a real timeout, and reusing it here -/// would inject a spurious tick at the end of every drain. -fn drain_pending(batch: &mut Vec) -> io::Result<()> { +/// Drain all immediately-available events from `inbox` into `batch`. Unlike calling +/// `recv_event(inbox, Duration::ZERO)` in a loop, an empty inbox here simply stops draining — it +/// must NOT fabricate a `Tick`, since [`recv_event`]'s timeout arm exists solely to give the loop +/// its regular redraw beat on a real timeout, and reusing it here would inject a spurious tick at +/// the end of every drain. +fn drain_pending( + inbox: &mpsc::Receiver, + batch: &mut Vec, +) -> io::Result<()> { while batch.len() < MAX_DRAIN_BATCH { - if !event::poll(Duration::ZERO)? { - break; - } - match event::read()? { - Event::Key(key) if key.kind == KeyEventKind::Press => batch.push(AppEvent::Key(key)), - Event::Resize(w, h) => batch.push(AppEvent::Resize(w, h)), - _ => {} + match inbox.try_recv() { + Ok(Ok(event)) => batch.push(event), + Ok(Err(err)) => return Err(err), + Err(mpsc::TryRecvError::Empty) => break, + Err(mpsc::TryRecvError::Disconnected) => { + return Err(io::Error::other( + "review TUI input thread disconnected without a final error", + )) + } } } Ok(()) @@ -569,8 +630,16 @@ impl Tui { /// `main.rs`'s default) that call marks the open PENDING rather than loading eagerly, so the /// first frame shows CS4's placeholder for one `OPEN_DEBOUNCE` window instead of blocking on /// the initial file's load; a caller that never turned defer mode on gets eager behavior. + /// + /// Spawns the ADR-037 input thread here — after the terminal is fully acquired (`self` already + /// exists, so raw mode and the alternate screen are live) and after every earlier tty + /// consumer (`main.rs`'s theme probe and its stray-input flush) has already run, since those + /// must own the tty before crossterm's event stream has a reader racing them. The thread is + /// never joined: when `run` returns, `main` returns, and the process takes it down (ADR-037's + /// kill-on-exit lifecycle — the input thread never writes, so this can't corrupt anything). pub fn run(&mut self, app: &mut App, keymap: &Keymap, theme: &Palette) -> io::Result<()> { - let result = event_loop(&mut self.terminal, app, keymap, theme); + let inbox = spawn_input_thread(); + let result = event_loop(&mut self.terminal, app, keymap, theme, &inbox); let restored = self.restore(); result.and(restored) } @@ -618,6 +687,7 @@ fn event_loop( app: &mut App, keymap: &Keymap, theme: &Palette, + inbox: &mpsc::Receiver, ) -> io::Result<()> { let mut pending: Vec = Vec::new(); let mut quit = false; @@ -629,9 +699,9 @@ fn event_loop( return Ok(()); } - // While an open is pending, poll on the short debounce window instead of the regular + // While an open is pending, wait on the short debounce window instead of the regular // 200ms redraw beat, so the deferred load runs promptly once input goes quiet — a plain - // timeout (no new terminal event) is what "quiet" means here. This borrows the same + // timeout (no new inbox message) is what "quiet" means here. This borrows the same // `Tick` beat the M4 index watcher already polls on (see the module doc); the watcher // occasionally running ~120ms early during a debounce window is harmless (its own doc // comment already tolerates an "unseen" signature settling one tick late). @@ -641,14 +711,13 @@ fn event_loop( Duration::from_millis(200) }; - if let Some(event) = next_event(timeout)? { - if matches!(event, AppEvent::Tick) && app.open_pending() { - app.complete_pending_open(); - } - let mut batch = vec![event]; - drain_pending(&mut batch)?; - quit = update_batch(app, keymap, &mut pending, batch); + let event = recv_event(inbox, timeout)?; + if matches!(event, AppEvent::Tick) && app.open_pending() { + app.complete_pending_open(); } + let mut batch = vec![event]; + drain_pending(inbox, &mut batch)?; + quit = update_batch(app, keymap, &mut pending, batch); } } @@ -666,6 +735,124 @@ mod tests { KeyEvent::new(KeyCode::Char(c), KeyModifiers::CONTROL) } + // ── ADR-037: input thread's pure mapping + inbox draining ────────────────── + + #[test] + fn map_terminal_event_maps_key_press_and_resize() { + assert_eq!( + map_terminal_event(Event::Key(key(KeyCode::Char('q')))), + Some(AppEvent::Key(key(KeyCode::Char('q')))) + ); + assert_eq!( + map_terminal_event(Event::Resize(80, 24)), + Some(AppEvent::Resize(80, 24)) + ); + } + + #[test] + fn map_terminal_event_skips_release_repeat_mouse_paste_and_focus() { + use crossterm::event::{KeyEventState, MouseButton, MouseEvent, MouseEventKind}; + + let release = KeyEvent::new_with_kind( + KeyCode::Char('q'), + KeyModifiers::NONE, + KeyEventKind::Release, + ); + assert_eq!(map_terminal_event(Event::Key(release)), None); + + let repeat = KeyEvent::new_with_kind_and_state( + KeyCode::Char('q'), + KeyModifiers::NONE, + KeyEventKind::Repeat, + KeyEventState::NONE, + ); + assert_eq!(map_terminal_event(Event::Key(repeat)), None); + + assert_eq!( + map_terminal_event(Event::Mouse(MouseEvent { + kind: MouseEventKind::Moved, + column: 0, + row: 0, + modifiers: KeyModifiers::NONE, + })), + None + ); + assert_eq!(map_terminal_event(Event::Paste("pasted".to_string())), None); + assert_eq!(map_terminal_event(Event::FocusGained), None); + assert_eq!(map_terminal_event(Event::FocusLost), None); + let _ = MouseButton::Left; // silence an unused-import lint if MouseButton goes unused above + } + + #[test] + fn recv_event_yields_tick_on_a_plain_timeout() { + let (_tx, rx) = mpsc::channel::(); + let event = recv_event(&rx, Duration::from_millis(5)).expect("timeout is not an error"); + assert_eq!(event, AppEvent::Tick); + } + + #[test] + fn recv_event_forwards_a_sent_event_before_the_timeout() { + let (tx, rx) = mpsc::channel::(); + tx.send(Ok(AppEvent::Key(key(KeyCode::Char('q'))))).unwrap(); + let event = recv_event(&rx, Duration::from_secs(1)).unwrap(); + assert_eq!(event, AppEvent::Key(key(KeyCode::Char('q')))); + } + + #[test] + fn recv_event_propagates_a_forwarded_read_error() { + let (tx, rx) = mpsc::channel::(); + tx.send(Err(io::Error::other("read failed"))).unwrap(); + let err = recv_event(&rx, Duration::from_secs(1)).unwrap_err(); + assert_eq!(err.to_string(), "read failed"); + } + + #[test] + fn recv_event_errors_when_the_inbox_disconnects_instead_of_spinning() { + let (tx, rx) = mpsc::channel::(); + drop(tx); + let result = recv_event(&rx, Duration::from_millis(5)); + assert!( + result.is_err(), + "a disconnected inbox must surface as an error, not a Tick" + ); + } + + #[test] + fn drain_pending_collects_everything_immediately_available_without_a_tick() { + let (tx, rx) = mpsc::channel::(); + tx.send(Ok(AppEvent::Key(key(KeyCode::Char('a'))))).unwrap(); + tx.send(Ok(AppEvent::Key(key(KeyCode::Char('b'))))).unwrap(); + let mut batch = Vec::new(); + drain_pending(&rx, &mut batch).unwrap(); + assert_eq!( + batch, + vec![ + AppEvent::Key(key(KeyCode::Char('a'))), + AppEvent::Key(key(KeyCode::Char('b'))), + ] + ); + } + + #[test] + fn drain_pending_stops_on_an_empty_inbox_without_fabricating_a_tick() { + let (_tx, rx) = mpsc::channel::(); + let mut batch = Vec::new(); + drain_pending(&rx, &mut batch).unwrap(); + assert!( + batch.is_empty(), + "an empty inbox must not inject a spurious Tick" + ); + } + + #[test] + fn drain_pending_propagates_a_forwarded_read_error() { + let (tx, rx) = mpsc::channel::(); + tx.send(Err(io::Error::other("read failed"))).unwrap(); + let mut batch = Vec::new(); + let err = drain_pending(&rx, &mut batch).unwrap_err(); + assert_eq!(err.to_string(), "read failed"); + } + #[test] fn quit_keys_map_to_quit() { let km = Keymap::defaults(); From 2a15ad592ec52466d517666ace7f7ce58a5e5b5f Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:01:43 -0400 Subject: [PATCH 04/16] test(review): drop dead MouseButton import in event-mapping test --- git-workon-review/src/tui.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index fd5b050d..caa1d362 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -751,7 +751,7 @@ mod tests { #[test] fn map_terminal_event_skips_release_repeat_mouse_paste_and_focus() { - use crossterm::event::{KeyEventState, MouseButton, MouseEvent, MouseEventKind}; + use crossterm::event::{KeyEventState, MouseEvent, MouseEventKind}; let release = KeyEvent::new_with_kind( KeyCode::Char('q'), @@ -780,7 +780,6 @@ mod tests { assert_eq!(map_terminal_event(Event::Paste("pasted".to_string())), None); assert_eq!(map_terminal_event(Event::FocusGained), None); assert_eq!(map_terminal_event(Event::FocusLost), None); - let _ = MouseButton::Left; // silence an unused-import lint if MouseButton goes unused above } #[test] From 19e766b55b9eeb63d15b71374e4dc0352f9eaee4 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 01:30:57 -0400 Subject: [PATCH 05/16] feat(review): loader thread with generation-tagged file loads --- git-workon-review/src/app.rs | 575 +++++++++++++++++++++++++++++++--- git-workon-review/src/main.rs | 10 +- git-workon-review/src/tui.rs | 399 ++++++++++++++++++----- 3 files changed, 863 insertions(+), 121 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 057915ed..25b69a42 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -48,6 +48,7 @@ const SCROLLOFF: usize = 2; /// The new side reads from the **worktree file on disk**, not the index blob — unstaged /// content isn't in the object database; reading the staged (index) blob is an M4 concern (the /// staged/unstaged split zoom). +#[derive(Debug)] pub struct FileView { old_text: String, new_text: String, @@ -346,6 +347,59 @@ fn new_side_tree_for(repo: &Repository, span: ChangesetSpan) -> Option Option { + if file.is_binary { + return None; + } + // Re-peeled per call rather than cached: for the uncommitted layer `HEAD` can move between + // file loads, and the tree is cheap to re-peel either way (see `old_side_tree_for`'s doc + // comment). + let head_tree = old_side_tree_for(repo, span)?; + let new_tree = new_side_tree_for(repo, span); + Some(FileView::load( + repo, + &head_tree, + new_tree.as_ref(), + file, + Role::Combined, + ts, + )) +} + +/// Build a non-Combined ([`Role::Unstaged`]/[`Role::Staged`]) [`FileView`] against `repo`/`ts` for +/// sub-role file `file` — the mirror of [`build_combined_view`], shared the same way. Non-Combined +/// roles are uncommitted-only (a committed changeset's staged/unstaged sub-models are always +/// empty — see [`DiffState::from_committed`]), so the new side always stays worktree/index (`None` +/// to [`FileView::load`]) and the old side is always live `HEAD`, never a changeset's `base`. +/// `None` for a binary file or an unreadable `HEAD`; never panics. +fn build_sub_role_view( + repo: &Repository, + ts: &mut TsHighlighter, + role: Role, + file: &FileChange, +) -> Option { + debug_assert_ne!( + role, + Role::Combined, + "build_sub_role_view is non-Combined only" + ); + if file.is_binary { + return None; + } + let head_tree = repo.head().and_then(|h| h.peel_to_tree()).ok()?; + Some(FileView::load(repo, &head_tree, None, file, role, ts)) +} + fn read_head_blob(repo: &Repository, tree: &git2::Tree<'_>, path: &str) -> String { tree.get_path(Path::new(path)) .and_then(|entry| entry.to_object(repo)) @@ -825,6 +879,19 @@ pub struct App { /// while this is `true`, and the event loop calls [`Self::complete_pending_open`] once input /// has been quiet for `OPEN_DEBOUNCE`. Read via [`Self::open_pending`]. open_pending: bool, + /// Whether a [`crate::app::FileLoadSpec`] has already been dispatched to the ADR-037 loader + /// thread for the CURRENT pending open — set by [`Self::take_pending_load_spec`], cleared + /// whenever a fresh open is marked pending. Without this, every idle `Tick` while + /// `open_pending` stays true (the loader hasn't answered yet) would re-dispatch the same + /// request; this makes dispatch idempotent across the pending open's whole lifetime. + open_pending_dispatched: bool, + /// ADR-037's global generation counter. Invariant: bumps ⟺ every view cache was invalidated + /// — launch seeds it at `1` ([`Self::from_changesets`]); [`Self::refresh`] bumps it on every + /// successful rebuild (still synchronous in this slice). A loader result whose `gen` doesn't + /// match this is for a world that no longer exists and is dropped at the inbox chokepoint + /// ([`Self::apply_file_ready`]) — the ONLY drop rule; within a generation, results are cached + /// even if the user navigated away (warmth, not staleness — see the ADR's "Generations"). + generation: u64, } /// A destructive staging op deferred behind a [`Confirm`], identified by index into [`App::files`] @@ -962,6 +1029,8 @@ impl App { review_source: None, defer_loads: false, open_pending: false, + open_pending_dispatched: false, + generation: 1, }; // Position the outline cursor on the changeset/file the lib marked `current` (the same // row `sync_outline_to_current` would reposition to after any diff-initiated nav) rather @@ -1041,6 +1110,13 @@ impl App { &self.cur().diff.files } + /// ADR-037's global generation — see the field's doc comment for the invariant. The loader + /// thread stamps every [`FileLoadSpec`] request it's handed with this value at send time; + /// [`Self::apply_file_ready`] drops a result whose stamp no longer matches. + pub fn generation(&self) -> u64 { + self.generation + } + /// Index into the reviewed stack of the active changeset — read by tests asserting the /// [`Self::from_changesets`]/[`Self::refresh`] "honor lib `current`" rule (locked decision /// #6), and by changeset-nav's own tests ([`Self::next_changeset`]/[`Self::prev_changeset`]/ @@ -1185,6 +1261,11 @@ impl App { .unwrap_or_else(|| current_cs_index(&views)); self.base_label = base_label_for(&views[self.current_cs].cs); self.changesets = views; + // ADR-037: every refresh bumps the generation, right where the view caches it protects + // are actually replaced — an early `return` above (a failed resolve/diff) leaves the old + // world's caches intact, so it must NOT bump. Any loader result still in flight for the + // pre-refresh world now carries a stale `gen` and dies at `apply_file_ready`'s chokepoint. + self.generation += 1; let n = self.cur().diff.files.len(); self.current = current_path @@ -1362,62 +1443,26 @@ impl App { Role::Staged => self.cur().diff.staged_model.files[mi].clone(), Role::Combined => unreachable!(), }; - if file.is_binary { + // `file` is cloned out of `self.cur()` (rather than a borrow) because + // `build_sub_role_view` needs `&self.repo` and `&mut self.highlighter` at once, which + // a borrow still anchored in `self.cur()` would conflict with — same rationale as the + // combined path below. + let Some(view) = build_sub_role_view(&self.repo, &mut self.highlighter, role, &file) + else { return; - } - // Build the view in a block so `head_tree` (which borrows `self.repo`) drops before - // the `views_for_mut` reborrow — same reason the combined path below can assign a - // direct field while `head_tree` is live but this method-call path cannot. `file` is - // cloned out of `self.cur()` for the same reason: `FileView::load` needs `&self.repo` - // and `&mut self.highlighter` at once, which a borrow still anchored in `self.cur()` - // would conflict with. - let view = { - // Re-peeled per call, same rationale as the combined path below. - let Ok(head_tree) = self.repo.head().and_then(|h| h.peel_to_tree()) else { - return; - }; - // Non-Combined roles are uncommitted-only (committed changesets have empty - // staged/unstaged sub-models), so the new side always stays worktree/index — - // `None` here preserves that exactly. - FileView::load( - &self.repo, - &head_tree, - None, - &file, - role, - &mut self.highlighter, - ) }; self.views_for_mut(role)[idx] = Some(view); return; } - // Combined role. - // Re-peeled per call rather than cached on `App`: for the uncommitted layer `HEAD` can - // move between file loads, and the tree is cheap to re-peel either way. - // `self.cur().cs.span` is `Copy`, so reading it here borrows `self` only for this - // sub-expression — `head_tree` itself ends up borrowing `self.repo` alone (via the free - // `old_side_tree_for`), leaving `&mut self.highlighter` free below. A method tied to - // `&self` would instead have bound the tree's lifetime to all of `self`. - let Some(head_tree) = old_side_tree_for(&self.repo, self.cur().cs.span) else { + // Combined role. `self.cur().cs.span`/`self.cur().diff.files[idx].clone()` are read out + // (rather than borrowed) for the same reason as the sub-role branch above — + // `build_combined_view` needs `&self.repo` and `&mut self.highlighter` together. + let span = self.cur().cs.span; + let file = self.cur().diff.files[idx].clone(); + let Some(view) = build_combined_view(&self.repo, &mut self.highlighter, span, &file) else { return; }; - // New-side source mirrors the old side: `None` (worktree) for the uncommitted layer, - // the changeset's `head` tree for a committed changeset. Same free-fn borrow dance as - // `old_side_tree_for` — both trees borrow only `self.repo`, so `&mut self.highlighter` - // stays free for `FileView::load`. - let new_tree = new_side_tree_for(&self.repo, self.cur().cs.span); - let file = self.cur().diff.files[idx].clone(); - let view = FileView::load( - &self.repo, - &head_tree, - new_tree.as_ref(), - &file, - Role::Combined, - &mut self.highlighter, - ); - drop(head_tree); - drop(new_tree); self.cur_mut().views_combined[idx] = Some(view); } @@ -1510,6 +1555,9 @@ impl App { pub fn open_current(&mut self) { if self.defer_loads && !self.current_views_cached() { self.open_pending = true; + // A fresh pending open has nothing dispatched to the loader yet — see + // [`Self::take_pending_load_spec`]. + self.open_pending_dispatched = false; self.reset_panes(); return; } @@ -1550,6 +1598,104 @@ impl App { self.ensure_loaded(self.current); self.reset_panes(); self.open_pending = false; + self.open_pending_dispatched = false; + } + + // ── ADR-037: the loader thread's request/result seam ──────────────────────── + + /// Snapshot everything the ADR-037 loader needs to load the CURRENT file, mirroring exactly + /// what [`Self::ensure_loaded`] would read from live state — see [`FileLoadSpec`]'s doc + /// comment. Every field is owned (cloned out), so the spec outlives the borrow and can cross + /// to the loader thread. + fn current_load_spec(&self) -> FileLoadSpec { + let idx = self.current; + let zoom = self.effective_zoom_for(idx); + let diff = &self.cur().diff; + FileLoadSpec { + span: self.cur().cs.span, + combined_file: diff.files[idx].clone(), + zoom, + unstaged_file: diff + .unstaged_idx + .get(idx) + .copied() + .flatten() + .map(|mi| diff.unstaged_model.files[mi].clone()), + staged_file: diff + .staged_idx + .get(idx) + .copied() + .flatten() + .map(|mi| diff.staged_model.files[mi].clone()), + } + } + + /// Take the [`FileLoadSpec`] for the current pending open, tagged with the generation/ + /// changeset/file it was built against — but ONLY if a request hasn't already been dispatched + /// for this same pending open (see [`Self::open_pending_dispatched`]'s doc comment). The event + /// loop calls this on every idle `Tick` while an open is pending; without the dispatched guard + /// it would re-send the same request on every one of those ticks until the loader answers. + /// Returns `None` when nothing is pending, or a request already went out for it. + pub fn take_pending_load_spec(&mut self) -> Option<(u64, usize, usize, FileLoadSpec)> { + if !self.open_pending || self.open_pending_dispatched { + return None; + } + self.open_pending_dispatched = true; + Some(( + self.generation, + self.current_cs, + self.current, + self.current_load_spec(), + )) + } + + /// Apply one loader result (ADR-037's chokepoint, the `FileReady` inbox arm routes here): + /// dropped outright on a generation mismatch (`gen != self.generation` — the world it was + /// computed against no longer exists, see [`Self::generation`]'s doc comment). Otherwise: + /// + /// - `Ok(views)` caches every view the result carries, UNLESS that slot is already `Some` — a + /// result for an already-cached file is discarded (the loader is a pure cache-warmer, never + /// an overwriter; the synchronous force-completion fallback may have already filled it). + /// - `Err(message)` is a job that panicked or otherwise failed: surfaced as a visible footer + /// notice (never silently stranding the file — see this changeset's report for why a footer + /// notice, not a new per-file `Failed` state, is the shape chosen here). + /// + /// Either way, when the readied file IS the current pending open, it's seated exactly like + /// [`Self::complete_pending_open`]'s tail: `open_pending` clears regardless of `Ok`/`Err` — a + /// failed load must not leave the placeholder stuck forever. Correctness never depends on + /// this: the next nav/force-completion retries via [`Self::ensure_loaded`]'s ordinary + /// cache-miss path, which is where a load actually being CORRECT is guaranteed. + pub fn apply_file_ready( + &mut self, + gen: u64, + cs_idx: usize, + file_idx: usize, + result: Result, + ) { + if gen != self.generation { + return; + } + match result { + Ok(views) => { + if let Some(cs) = self.changesets.get_mut(cs_idx) { + match views { + LoadedViews::Single(role, view) => set_if_absent(cs, role, file_idx, view), + LoadedViews::Split { unstaged, staged } => { + set_if_absent(cs, Role::Unstaged, file_idx, unstaged); + set_if_absent(cs, Role::Staged, file_idx, staged); + } + } + } + } + Err(message) => { + self.notify(format!("failed to load file: {message}"), Severity::Error); + } + } + if cs_idx == self.current_cs && file_idx == self.current && self.open_pending { + self.reset_panes(); + self.open_pending = false; + self.open_pending_dispatched = false; + } } /// Cycle the requested zoom `Split → Combined → Unstaged → Staged → Split` (`z`). The new zoom @@ -2723,6 +2869,103 @@ fn current_cs_index(changesets: &[ChangesetView]) -> usize { changesets.iter().position(|v| v.cs.current).unwrap_or(0) } +// ── ADR-037: the loader thread's stateless request/job shape ──────────────────── + +/// Everything the ADR-037 loader job needs to reproduce one file's [`App::ensure_loaded`] work +/// against its OWN `Repository` + [`TsHighlighter`] — the loader is stateless between jobs (see +/// the ADR's "Protocol": "each request carries what it needs"). Built by +/// [`App::current_load_spec`] from live `App` state at request-send time; every field is owned +/// (cloned out of `App`), so the spec outlives the borrow and crosses to the loader thread. +#[derive(Debug, Clone)] +pub struct FileLoadSpec { + span: ChangesetSpan, + combined_file: FileChange, + /// The [`EffectiveZoom`] `App` had AT DISPATCH TIME — the views built are shaped by this, + /// not by whatever `App`'s zoom/current file happen to be when the result lands (which may + /// have changed by then; that's fine, see the ADR's "Generations": within a generation, a + /// result is warmth even after the user navigated away). + zoom: EffectiveZoom, + unstaged_file: Option, + staged_file: Option, +} + +/// The [`FileView`]s [`build_file_views`] built for one [`FileLoadSpec`], shaped exactly like the +/// [`EffectiveZoom`] it was built for — [`App::apply_file_ready`] reads this shape to know which +/// cache slot(s) to fill without re-deriving the zoom itself (which could disagree with the zoom +/// the views were actually built against — see [`FileLoadSpec::zoom`]'s doc comment). +/// `FileView` fields (`Box`ed here, see below) — a `FileReady` `AppEvent` carrying this unboxed +/// would otherwise make the WHOLE `AppEvent` enum balloon to `FileView`'s size on every variant +/// (clippy's `large_enum_variant`), even the plain `Key`/`Tick` ones sent on every keystroke. +#[derive(Debug)] +pub enum LoadedViews { + Single(Role, Option>), + Split { + unstaged: Option>, + staged: Option>, + }, +} + +/// Build every [`FileView`] a [`FileLoadSpec`] needs, against `repo`/`ts` — the ADR-037 loader +/// thread's pure job body: unit-testable directly against a fixture repo, no threads or channels +/// involved. Routes through the SAME [`build_combined_view`]/[`build_sub_role_view`] free +/// functions [`App::ensure_role_loaded`] calls, so a deferred-then-loader-completed open is +/// byte-identical to an eager [`App::open_current`] — the invariant ADR-037 carries over from +/// CS4's `complete_pending_open`. +pub fn build_file_views( + repo: &Repository, + ts: &mut TsHighlighter, + spec: &FileLoadSpec, +) -> LoadedViews { + match spec.zoom { + EffectiveZoom::Single(role) => { + let view = match role { + Role::Combined => build_combined_view(repo, ts, spec.span, &spec.combined_file), + Role::Unstaged => spec + .unstaged_file + .as_ref() + .and_then(|f| build_sub_role_view(repo, ts, Role::Unstaged, f)), + Role::Staged => spec + .staged_file + .as_ref() + .and_then(|f| build_sub_role_view(repo, ts, Role::Staged, f)), + }; + LoadedViews::Single(role, view.map(Box::new)) + } + EffectiveZoom::Split => LoadedViews::Split { + unstaged: spec + .unstaged_file + .as_ref() + .and_then(|f| build_sub_role_view(repo, ts, Role::Unstaged, f)) + .map(Box::new), + staged: spec + .staged_file + .as_ref() + .and_then(|f| build_sub_role_view(repo, ts, Role::Staged, f)) + .map(Box::new), + }, + } +} + +/// Cache `view` into changeset `cs`'s `role` view slot for file `idx`, UNLESS that slot is +/// already `Some` — [`App::apply_file_ready`]'s "a result for an already-cached file is +/// discarded" rule (the loader is a pure cache-warmer, never an overwriter). A no-op if `idx` is +/// out of range (the changeset shrank across a refresh — should already be unreachable, since a +/// refresh bumps the generation and `apply_file_ready` drops stale-generation results before +/// this ever runs, but `get_mut` stays defensive rather than indexing). +fn set_if_absent(cs: &mut ChangesetView, role: Role, idx: usize, view: Option>) { + let view = view.map(|boxed| *boxed); + let slots = match role { + Role::Combined => &mut cs.views_combined, + Role::Unstaged => &mut cs.views_unstaged, + Role::Staged => &mut cs.views_staged, + }; + if let Some(slot) = slots.get_mut(idx) { + if slot.is_none() { + *slot = view; + } + } +} + /// [`App::base_label`] for the changeset that would become active — a committed changeset's /// base rev (7-char short-sha), or `"HEAD"` for the uncommitted layer (worktree ↔ `HEAD`, /// unchanged from M2–M4). @@ -2884,8 +3127,8 @@ mod tests { use super::test_support::app_from_fixture; use super::{ - find_next_hunk_row, find_prev_hunk_row, App, ChangesetView, DiffState, EffectiveZoom, - Layout, Role, Zoom, DEFAULT_OUTLINE_WIDTH, + build_file_views, find_next_hunk_row, find_prev_hunk_row, App, ChangesetView, DiffState, + EffectiveZoom, Layout, LoadedViews, Role, Severity, Zoom, DEFAULT_OUTLINE_WIDTH, }; use crate::align::{AlignedRow, CellKind, DisplayRow, InlineRow, Row}; use crate::config::ReviewConfig; @@ -3103,6 +3346,238 @@ mod tests { assert_eq!(app.scroll, scroll_before); } + // ── ADR-037: the loader's request/result seam ──────────────────────────────── + + #[test] + fn build_file_views_matches_ensure_loaded_for_the_combined_role() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("tracked.txt", "line1\nline2\n", "line1\nCHANGED\n") + .build() + .unwrap(); + + let mut eager = app_from_fixture(&fixture); + // The file only has an unstaged change, so the default `Split` zoom would collapse to + // `Role::Unstaged` — force `Combined` explicitly so this test exercises the role its + // name promises (a separate test would be needed for the Split/sub-role shape). + eager.set_zoom(Zoom::Combined); + eager.ensure_loaded(0); + let eager_view = eager.current_view_ref().expect("eager view loaded"); + + // A SEPARATE `App` gives us `current_load_spec()` for the same file, and a SEPARATE + // `Repository` handle + fresh `TsHighlighter` stands in for the loader thread's own — + // exactly the two-handle shape `Tui::run`/`spawn_loader_thread` build for real. + let mut spec_app = app_from_fixture(&fixture); + spec_app.set_zoom(Zoom::Combined); + let spec = spec_app.current_load_spec(); + let repo = fixture.repo().unwrap(); + let loader_repo = + Repository::open(repo.workdir().unwrap()).expect("loader's own repo handle"); + let mut loader_ts = crate::highlight::TsHighlighter::new(); + let views = build_file_views(&loader_repo, &mut loader_ts, &spec); + + let LoadedViews::Single(role, Some(loader_view)) = views else { + panic!("expected a loaded Combined-role view"); + }; + assert_eq!(role, Role::Combined); + assert_eq!(loader_view.old_text(), eager_view.old_text()); + assert_eq!(loader_view.new_text(), eager_view.new_text()); + assert_eq!(loader_view.display.len(), eager_view.display.len()); + } + + #[test] + fn apply_file_ready_completes_a_pending_open_byte_identical_to_eager_open() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nold\nl10\nl11\nl12\n", + "l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nnew\nl10\nl11\nl12\n", + ) + .build() + .unwrap(); + + let (mut deferred, eager) = defer_and_eager_twins(&fixture); + assert!(deferred.open_pending()); + + // The ASYNC path: take the pending spec (as `tui.rs`'s event loop would on the debounce + // `Tick`), build its views through a SEPARATE repo/highlighter (standing in for the + // loader thread's own), then apply the result — never `complete_pending_open`. + let (gen, cs_idx, file_idx, spec) = deferred + .take_pending_load_spec() + .expect("a fresh pending open has an undispatched spec"); + let repo = fixture.repo().unwrap(); + let loader_repo = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut loader_ts = crate::highlight::TsHighlighter::new(); + let views = build_file_views(&loader_repo, &mut loader_ts, &spec); + + deferred.apply_file_ready(gen, cs_idx, file_idx, Ok(views)); + + assert!( + !deferred.open_pending(), + "apply_file_ready must clear the pending flag for the file it just seated" + ); + assert_eq!( + deferred.cursor, eager.cursor, + "cursor must land on the same (first-hunk) row an eager open would have" + ); + assert_eq!(deferred.scroll, eager.scroll); + let deferred_view = deferred.current_view_ref().expect("view now loaded"); + let eager_view = eager.current_view_ref().expect("eager view loaded"); + assert_eq!(deferred_view.old_text(), eager_view.old_text()); + assert_eq!(deferred_view.new_text(), eager_view.new_text()); + assert_eq!(deferred_view.display.len(), eager_view.display.len()); + } + + #[test] + fn take_pending_load_spec_dispatches_at_most_once_per_pending_open() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); + assert!( + app.take_pending_load_spec().is_some(), + "first take dispatches" + ); + assert!( + app.take_pending_load_spec().is_none(), + "a second take before the result lands must not re-dispatch" + ); + } + + #[test] + fn apply_file_ready_drops_a_stale_generation_result() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); + let (gen, cs_idx, file_idx, spec) = app.take_pending_load_spec().unwrap(); + + let repo = fixture.repo().unwrap(); + let loader_repo = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut loader_ts = crate::highlight::TsHighlighter::new(); + let views = build_file_views(&loader_repo, &mut loader_ts, &spec); + + // A refresh between dispatch and result bumps the generation — the result now belongs + // to a world that no longer exists and must be dropped outright, leaving `open_pending` + // untouched (a FRESH open may since be pending for a different generation). + app.generation += 1; + app.apply_file_ready(gen, cs_idx, file_idx, Ok(views)); + + assert!( + app.open_pending(), + "a stale-generation result must not clear a (possibly fresh) pending open" + ); + assert!( + app.current_view_ref().is_none(), + "a stale-generation result must not populate the view cache" + ); + } + + #[test] + fn apply_file_ready_caches_a_result_even_after_navigating_away() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .unstaged_file("b.txt", "two\n", "two\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_zoom(Zoom::Combined); + app.set_defer_loads(true); + app.open_current(); // a.txt: uncached — defers + let (gen, cs_idx, file_idx, spec) = app.take_pending_load_spec().unwrap(); + assert_eq!(file_idx, 0); + + // Navigate away from a.txt BEFORE the (simulated) loader result lands. + app.current = 1; + app.open_current(); + + let repo = fixture.repo().unwrap(); + let loader_repo = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut loader_ts = crate::highlight::TsHighlighter::new(); + let views = build_file_views(&loader_repo, &mut loader_ts, &spec); + app.apply_file_ready(gen, cs_idx, file_idx, Ok(views)); + + // Still within the same generation — the result is warmth, not staleness: a.txt's cache + // is populated even though the user is no longer looking at it. + assert!( + app.role_view_ref(0, Role::Combined).is_some(), + "a within-generation result must cache even after the user navigated away" + ); + } + + #[test] + fn apply_file_ready_discards_a_result_for_an_already_cached_file() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_zoom(Zoom::Combined); + app.ensure_loaded(0); // eagerly cached already + assert!(app.role_view_ref(0, Role::Combined).is_some()); + let old_text_before = app + .role_view_ref(0, Role::Combined) + .unwrap() + .old_text() + .to_string(); + + // A result claiming NOTHING loaded for this role (e.g. a stale/racing answer) must not + // clobber the already-cached view — the loader is a pure cache-warmer, never an + // overwriter. + app.apply_file_ready( + app.generation(), + app.current_cs(), + 0, + Ok(LoadedViews::Single(Role::Combined, None)), + ); + + let view = app.role_view_ref(0, Role::Combined).expect("still cached"); + assert_eq!(view.old_text(), old_text_before); + } + + #[test] + fn apply_file_ready_err_surfaces_a_footer_notice_and_clears_pending() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); + let (gen, cs_idx, file_idx, _spec) = app.take_pending_load_spec().unwrap(); + assert!(app.notice.is_none()); + + app.apply_file_ready(gen, cs_idx, file_idx, Err("boom".to_string())); + + assert!( + !app.open_pending(), + "a failed load must not strand the placeholder pending forever" + ); + let notice = app + .notice + .as_ref() + .expect("a failed load surfaces a notice"); + assert_eq!(notice.severity, Severity::Error); + assert!(notice.text.contains("boom")); + } + // Hunk-nav helpers below operate purely over `DisplayRow` vectors — no fixture repo needed. fn ctx_row(n: usize) -> DisplayRow { diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index 6425cec1..c6f3f264 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -164,6 +164,14 @@ fn main() -> Result<()> { return Ok(()); } + // Captured BEFORE `repo` moves into `App` below — the ADR-037 loader thread needs its own + // `Repository` handle onto the same on-disk repo (`git2::Repository` is `Send` but not + // `Sync`, so it can't cross threads directly), opened the same way + // `crate::acquire::diff_changesets`'s worker threads already do: at the workdir so the + // uncommitted layer's index/worktree diffs resolve correctly, falling back to the gitdir for + // a bare repo (where only committed spans can occur). + let repo_path = repo.workdir().unwrap_or_else(|| repo.path()).to_path_buf(); + // `App` owns its own `Repository` handle (see `app.rs`'s doc comment) — moved in here after // acquisition is done borrowing it. `App::from_changesets` opens on whichever changeset the // lib marked `current` (locked decision #6). @@ -195,7 +203,7 @@ fn main() -> Result<()> { // A carried acquire failure surfaces HERE — the same logical point (running the TUI) it // surfaced at before CS5 moved the terminal takeover ahead of the diff phase. tui.into_diagnostic()? - .run(&mut app, &keymap, &theme) + .run(&mut app, &keymap, &theme, repo_path) .into_diagnostic()?; Ok(()) diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index caa1d362..ba66c205 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -18,6 +18,7 @@ use std::fs::File; use std::io::{self, Write}; +use std::path::PathBuf; use std::sync::mpsc; use std::thread; use std::time::Duration; @@ -27,25 +28,56 @@ use crossterm::execute; use crossterm::terminal::{ disable_raw_mode, enable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen, }; +use git2::Repository; use ratatui::backend::CrosstermBackend; use ratatui::style::{Modifier, Style}; use ratatui::widgets::Paragraph; use ratatui::{Frame, Terminal}; -use workon_review::app::App; +use workon_review::app::{self, App, FileLoadSpec, LoadedViews}; +use workon_review::highlight::TsHighlighter; use workon_review::keymap::{Command, Dispatch, KeyPress, Keymap}; use workon_review::render; use workon_review::theme::Palette; /// One event the review loop reacts to. `Tick` is synthesized by the main loop on an inbox /// `recv_timeout` timeout — it is never sent through the channel itself (see [`recv_event`]). -/// `Key`/`Resize` are forwarded from the input thread via [`map_terminal_event`]. Not `Copy` -/// (ADR-037): the next slice's loader-result variants carry non-`Copy` payloads; dropping `Copy` -/// now is mechanical prep so this slice's diff doesn't collide with that one's. -#[derive(Debug, Clone, PartialEq, Eq)] +/// `Key`/`Resize` are forwarded from the input thread via [`map_terminal_event`]; `FileReady` is +/// forwarded from the loader thread via [`run_load_job`]. Not `Copy`/`Clone`/`PartialEq`/`Eq` +/// (ADR-037): `FileReady`'s payload carries [`LoadedViews`], which wraps +/// [`workon_review::app::FileView`] — a type with none of those (its highlight/word-diff caches +/// don't implement them, and rebuilding one is cheap enough that nothing has ever needed to). +#[derive(Debug)] pub enum AppEvent { Key(KeyEvent), Resize(u16, u16), Tick, + /// One [`LoadRequest`]'s result — ADR-037's loader-result variant. `gen`/`cs_idx`/`file_idx` + /// echo the request's stamp; `result` is `Err` for a job that panicked or otherwise failed + /// (see [`run_load_job`]'s doc comment for why a footer notice, not a new AppEvent shape, is + /// how that surfaces). Applied at ONE chokepoint: [`App::apply_file_ready`]. + FileReady { + gen: u64, + cs_idx: usize, + file_idx: usize, + result: Result, + }, +} + +impl PartialEq for AppEvent { + /// Manual, deliberately PARTIAL equality (can't derive — `FileReady`'s `LoadedViews` payload + /// isn't `PartialEq`, see the enum's doc comment): `Key`/`Resize`/`Tick` compare structurally, + /// exactly like the pre-ADR-037 derive did, for the input-thread tests that still assert + /// mapped-event shape via `assert_eq!`. Two `FileReady` events are never considered equal — + /// there's no sound definition of "the same loader result" once `FileView` can't be compared, + /// and nothing needs one; tests that care about a `FileReady`'s fields match on them directly. + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (AppEvent::Key(a), AppEvent::Key(b)) => a == b, + (AppEvent::Resize(w1, h1), AppEvent::Resize(w2, h2)) => w1 == w2 && h1 == h2, + (AppEvent::Tick, AppEvent::Tick) => true, + _ => false, + } + } } /// The inbox message type: a mapped terminal event, or the input thread's terminal `event::read` @@ -66,17 +98,19 @@ fn map_terminal_event(event: Event) -> Option { } } -/// Spawn the dedicated input thread and return the receiving end of its inbox. Must be called -/// AFTER the terminal is acquired and any pre-takeover tty work (the theme probe, stray-input -/// flush) has finished — crossterm input must not be consumed before that ordering completes -/// (see `main.rs`'s block comment on the resolve/probe/acquire sequence). The thread loops -/// forever on a blocking `event::read()`, forwarding mapped events; on a read error it forwards -/// the error once and exits — the sole way this thread ever stops short of the process dying. -/// Never joined: [`Tui::run`] returns without waiting for it (ADR-037's kill-on-exit lifecycle — -/// the input thread, like the future loader thread, never writes, so an abandoned read can't -/// corrupt anything). -fn spawn_input_thread() -> mpsc::Receiver { - let (tx, rx) = mpsc::channel(); +/// Spawn the dedicated input thread against an already-built inbox sender. Must be called AFTER +/// the terminal is acquired and any pre-takeover tty work (the theme probe, stray-input flush) +/// has finished — crossterm input must not be consumed before that ordering completes (see +/// `main.rs`'s block comment on the resolve/probe/acquire sequence). The thread loops forever on +/// a blocking `event::read()`, forwarding mapped events; on a read error it forwards the error +/// once and exits — the sole way this thread ever stops short of the process dying. Never joined: +/// [`Tui::run`] returns without waiting for it (ADR-037's kill-on-exit lifecycle — the input +/// thread, like the loader thread, never writes, so an abandoned read can't corrupt anything). +/// +/// `tx` is a clone of the SAME inbox sender the loader thread also holds (ADR-037's "one inbox" — +/// [`Tui::run`] builds the channel once and hands a clone to each producer thread), so both +/// threads' events interleave into a single `recv_event`/`drain_pending` stream. +fn spawn_input_thread(tx: mpsc::Sender) { thread::spawn(move || loop { match event::read() { Ok(event) => { @@ -92,7 +126,94 @@ fn spawn_input_thread() -> mpsc::Receiver { } } }); - rx +} + +/// One file-load request handed to the loader thread (ADR-037's "Protocol": the loader is +/// stateless between jobs — everything a job needs rides along on the request). `gen`/`cs_idx`/ +/// `file_idx` are stamped at send time from [`App::take_pending_load_spec`]'s return and echoed +/// back verbatim on the [`AppEvent::FileReady`] result, so [`App::apply_file_ready`] can apply +/// (or drop) it without the loader ever touching `App`. +struct LoadRequest { + gen: u64, + cs_idx: usize, + file_idx: usize, + spec: FileLoadSpec, +} + +/// Extract a human-readable message from a `catch_unwind` panic payload — the common `&str`/ +/// `String` panic-message shapes get their text; anything else (a panic with a non-string +/// payload) falls back to a generic message rather than failing to report at all. +fn panic_message(payload: Box) -> String { + if let Some(s) = payload.downcast_ref::<&str>() { + s.to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "loader job panicked".to_string() + } +} + +/// The ADR-037 loader job's pure body: `LoadRequest -> AppEvent`, unit-tested directly (no +/// threads) against a fixture repo + highlighter. Wrapped in `catch_unwind` per the ADR's +/// "Lifecycle" decision — the specific failure mode this catches that nothing else does: a +/// panicked job would otherwise silently drop into a slot stranded `Pending` forever (the file +/// never re-requested, since [`App::open_pending_dispatched`]'s guard already marked it sent), an +/// invisible hang instead of a visible error. +/// +/// A panic's message surfaces through [`AppEvent::FileReady`]'s `Err` arm, which +/// [`App::apply_file_ready`] turns into a footer notice — a footer notice, not a new per-file +/// `Failed` slot, is the shape chosen here (see this changeset's report): it's visible, it +/// doesn't strand `open_pending`, and correctness never depended on the loader succeeding in the +/// first place (the force-completion sync fallback is where correctness actually lives). +fn run_load_job(repo: &Repository, ts: &mut TsHighlighter, req: LoadRequest) -> AppEvent { + let LoadRequest { + gen, + cs_idx, + file_idx, + spec, + } = req; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + app::build_file_views(repo, ts, &spec) + })) + .map_err(panic_message); + AppEvent::FileReady { + gen, + cs_idx, + file_idx, + result, + } +} + +/// Spawn the ADR-037 loader thread: it owns its own long-lived `Repository` + `TsHighlighter` +/// (never `App`'s — the loader is a separate thread and can't touch `App`'s handles) and serves +/// [`LoadRequest`]s sequentially off `req_rx`, forwarding each job's [`AppEvent::FileReady`] into +/// the shared inbox via `tx` (a clone of the same sender the input thread holds). Returns the +/// `Sender` half the main loop dispatches requests through. +/// +/// If `repo_path` can't be opened here (should be unreachable — `main.rs` already opened it once +/// to build `App`), the thread exits immediately without serving anything: every subsequent +/// dispatch attempt just accumulates in `req_rx`'s buffer until the main loop's `send` starts +/// erroring, which is harmless (the force-completion sync fallback is what correctness actually +/// depends on — see [`run_load_job`]'s doc comment). Never joined — same kill-on-exit lifecycle as +/// the input thread. +fn spawn_loader_thread( + repo_path: PathBuf, + tx: mpsc::Sender, +) -> mpsc::Sender { + let (req_tx, req_rx) = mpsc::channel::(); + thread::spawn(move || { + let Ok(repo) = Repository::open(&repo_path) else { + return; + }; + let mut ts = TsHighlighter::new(); + for req in req_rx { + let event = run_load_job(&repo, &mut ts, req); + if tx.send(Ok(event)).is_err() { + return; // main loop is gone; nothing left to forward to + } + } + }); + req_tx } /// Receive the next event from `inbox`, waiting up to `timeout`. A timeout with nothing received @@ -417,6 +538,15 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap false } AppEvent::Resize(_, _) => false, + AppEvent::FileReady { + gen, + cs_idx, + file_idx, + result, + } => { + app.apply_file_ready(gen, cs_idx, file_idx, result); + false + } } } @@ -628,18 +758,33 @@ impl Tui { /// Run the main loop against `app`, then restore the terminal. Callers must have already /// called `app.open_current()` — under CS4's deferred-load mode (`app.set_defer_loads(true)`, /// `main.rs`'s default) that call marks the open PENDING rather than loading eagerly, so the - /// first frame shows CS4's placeholder for one `OPEN_DEBOUNCE` window instead of blocking on - /// the initial file's load; a caller that never turned defer mode on gets eager behavior. + /// first frame shows CS4's placeholder until the ADR-037 loader thread answers (or a + /// force-completion chokepoint loads it synchronously first); a caller that never turned defer + /// mode on gets eager behavior. + /// + /// `repo_path` opens the loader thread's OWN `Repository` handle — a second handle onto the + /// same on-disk repo `app` already holds one of, exactly like `crate::acquire::diff_changesets`'s + /// worker threads (`app` can't hand its handle across threads: `git2::Repository` is `Send` + /// but not `Sync`). /// - /// Spawns the ADR-037 input thread here — after the terminal is fully acquired (`self` already - /// exists, so raw mode and the alternate screen are live) and after every earlier tty - /// consumer (`main.rs`'s theme probe and its stray-input flush) has already run, since those - /// must own the tty before crossterm's event stream has a reader racing them. The thread is - /// never joined: when `run` returns, `main` returns, and the process takes it down (ADR-037's - /// kill-on-exit lifecycle — the input thread never writes, so this can't corrupt anything). - pub fn run(&mut self, app: &mut App, keymap: &Keymap, theme: &Palette) -> io::Result<()> { - let inbox = spawn_input_thread(); - let result = event_loop(&mut self.terminal, app, keymap, theme, &inbox); + /// Builds the single ADR-037 inbox HERE and spawns both the input thread and the loader thread + /// against clones of its sender — after the terminal is fully acquired (`self` already exists, + /// so raw mode and the alternate screen are live) and after every earlier tty consumer + /// (`main.rs`'s theme probe and its stray-input flush) has already run, since those must own + /// the tty before crossterm's event stream has a reader racing them. Neither thread is joined: + /// when `run` returns, `main` returns, and the process takes both down (ADR-037's kill-on-exit + /// lifecycle — neither thread ever writes, so an abandoned one can't corrupt anything). + pub fn run( + &mut self, + app: &mut App, + keymap: &Keymap, + theme: &Palette, + repo_path: PathBuf, + ) -> io::Result<()> { + let (tx, rx) = mpsc::channel::(); + spawn_input_thread(tx.clone()); + let load_tx = spawn_loader_thread(repo_path, tx); + let result = event_loop(&mut self.terminal, app, keymap, theme, &rx, &load_tx); let restored = self.restore(); result.and(restored) } @@ -688,6 +833,7 @@ fn event_loop( keymap: &Keymap, theme: &Palette, inbox: &mpsc::Receiver, + load_tx: &mpsc::Sender, ) -> io::Result<()> { let mut pending: Vec = Vec::new(); let mut quit = false; @@ -700,11 +846,11 @@ fn event_loop( } // While an open is pending, wait on the short debounce window instead of the regular - // 200ms redraw beat, so the deferred load runs promptly once input goes quiet — a plain - // timeout (no new inbox message) is what "quiet" means here. This borrows the same - // `Tick` beat the M4 index watcher already polls on (see the module doc); the watcher - // occasionally running ~120ms early during a debounce window is harmless (its own doc - // comment already tolerates an "unseen" signature settling one tick late). + // 200ms redraw beat, so the deferred load's request goes out promptly once input goes + // quiet — a plain timeout (no new inbox message) is what "quiet" means here. This borrows + // the same `Tick` beat the M4 index watcher already polls on (see the module doc); the + // watcher occasionally running ~120ms early during a debounce window is harmless (its own + // doc comment already tolerates an "unseen" signature settling one tick late). let timeout = if app.open_pending() { OPEN_DEBOUNCE } else { @@ -712,8 +858,21 @@ fn event_loop( }; let event = recv_event(inbox, timeout)?; + // ADR-037: the debounce-fired deferred open is now an ASYNC `LoadFile` request rather + // than a synchronous `complete_pending_open` — the placeholder keeps rendering until the + // loader's `FileReady` result lands (or a force-completion chokepoint loads it + // synchronously first, e.g. the user presses `j` before the loader answers). + // `take_pending_load_spec` is idempotent across repeated debounce-window Ticks: it + // returns `None` once a request has already gone out for the current pending open. if matches!(event, AppEvent::Tick) && app.open_pending() { - app.complete_pending_open(); + if let Some((gen, cs_idx, file_idx, spec)) = app.take_pending_load_spec() { + let _ = load_tx.send(LoadRequest { + gen, + cs_idx, + file_idx, + spec, + }); + } } let mut batch = vec![event]; drain_pending(inbox, &mut batch)?; @@ -782,11 +941,20 @@ mod tests { assert_eq!(map_terminal_event(Event::FocusLost), None); } + /// `AppEvent` dropped `PartialEq`/`Eq` in ADR-037 (`FileReady`'s `LoadedViews` payload wraps + /// `FileView`, which has neither) — this test-only helper is the `matches!`-based replacement + /// for the `assert_eq!(event, AppEvent::Key(key(...)))` shape used throughout this module's + /// tests. Only compares `code`/`modifiers`/`kind` (what `key(...)`/`ctrl_key(...)` set), same + /// fields a `PartialEq` derive on `KeyEvent` itself would have compared. + fn is_key_event(event: &AppEvent, expected: KeyEvent) -> bool { + matches!(event, AppEvent::Key(k) if *k == expected) + } + #[test] fn recv_event_yields_tick_on_a_plain_timeout() { let (_tx, rx) = mpsc::channel::(); let event = recv_event(&rx, Duration::from_millis(5)).expect("timeout is not an error"); - assert_eq!(event, AppEvent::Tick); + assert!(matches!(event, AppEvent::Tick)); } #[test] @@ -794,7 +962,7 @@ mod tests { let (tx, rx) = mpsc::channel::(); tx.send(Ok(AppEvent::Key(key(KeyCode::Char('q'))))).unwrap(); let event = recv_event(&rx, Duration::from_secs(1)).unwrap(); - assert_eq!(event, AppEvent::Key(key(KeyCode::Char('q')))); + assert!(is_key_event(&event, key(KeyCode::Char('q')))); } #[test] @@ -823,13 +991,9 @@ mod tests { tx.send(Ok(AppEvent::Key(key(KeyCode::Char('b'))))).unwrap(); let mut batch = Vec::new(); drain_pending(&rx, &mut batch).unwrap(); - assert_eq!( - batch, - vec![ - AppEvent::Key(key(KeyCode::Char('a'))), - AppEvent::Key(key(KeyCode::Char('b'))), - ] - ); + assert_eq!(batch.len(), 2); + assert!(is_key_event(&batch[0], key(KeyCode::Char('a')))); + assert!(is_key_event(&batch[1], key(KeyCode::Char('b')))); } #[test] @@ -1043,6 +1207,88 @@ mod tests { App::new(owned, diffs) } + // ── ADR-037: the loader job's pure body ────────────────────────────────────── + + #[test] + fn panic_message_reads_a_str_payload() { + let payload: Box = Box::new("boom"); + assert_eq!(panic_message(payload), "boom"); + } + + #[test] + fn panic_message_reads_a_string_payload() { + let payload: Box = Box::new("boom".to_string()); + assert_eq!(panic_message(payload), "boom"); + } + + #[test] + fn panic_message_falls_back_for_a_non_string_payload() { + let payload: Box = Box::new(42_i32); + assert_eq!(panic_message(payload), "loader job panicked"); + } + + #[test] + fn run_load_job_result_matches_a_synchronous_ensure_loaded() { + use git_workon_fixture::prelude::*; + use workon_review::app::Role; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut eager = app_from_fixture(&fixture); + eager.ensure_loaded(0); + let eager_view = eager.current_view_ref().expect("eager view loaded"); + let eager_old_text = eager_view.old_text().to_string(); + let eager_new_text = eager_view.new_text().to_string(); + + // Same two-handle shape the real loader thread uses: `app`'s own repo builds the spec, + // a SEPARATE repo + highlighter (standing in for `spawn_loader_thread`'s own) runs the + // job. + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); + let (gen, cs_idx, file_idx, spec) = app + .take_pending_load_spec() + .expect("a fresh pending open has an undispatched spec"); + + let repo = fixture.repo().unwrap(); + let loader_repo = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut loader_ts = TsHighlighter::new(); + let event = run_load_job( + &loader_repo, + &mut loader_ts, + LoadRequest { + gen, + cs_idx, + file_idx, + spec, + }, + ); + + let AppEvent::FileReady { + gen: got_gen, + cs_idx: got_cs_idx, + file_idx: got_file_idx, + result, + } = event + else { + panic!("run_load_job must return a FileReady event"); + }; + assert_eq!(got_gen, gen); + assert_eq!(got_cs_idx, cs_idx); + assert_eq!(got_file_idx, file_idx); + + let LoadedViews::Single(role, Some(view)) = result.expect("job must not fail") else { + panic!("expected a loaded single-role view"); + }; + assert_eq!(role, Role::Unstaged, "a.txt has only an unstaged change"); + assert_eq!(view.old_text(), eager_old_text); + assert_eq!(view.new_text(), eager_new_text); + } + #[test] fn key_event_through_update_clears_a_previously_set_notice() { use git_workon_fixture::prelude::*; @@ -1754,14 +2000,19 @@ mod tests { let km = Keymap::defaults(); let mut pending_batch: Vec = Vec::new(); let mut pending_seq: Vec = Vec::new(); - let events = vec![ - AppEvent::Key(key(KeyCode::Char('j'))), - AppEvent::Key(key(KeyCode::Char('j'))), - AppEvent::Key(key(KeyCode::Char('j'))), - ]; + // `AppEvent` isn't `Clone` (ADR-037: `FileReady`'s payload wraps a non-`Clone` + // `FileView`), so the batch/sequential runs each build their own copy of the same + // three-key press sequence rather than sharing one `Vec` via `.clone()`. + let build_events = || { + vec![ + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + ] + }; - update_batch(&mut app_batch, &km, &mut pending_batch, events.clone()); - for event in events { + update_batch(&mut app_batch, &km, &mut pending_batch, build_events()); + for event in build_events() { update(&mut app_seq, &km, &mut pending_seq, event); } @@ -1790,15 +2041,19 @@ mod tests { let km = Keymap::defaults(); let mut pending_batch: Vec = Vec::new(); let mut pending_seq: Vec = Vec::new(); - let events = vec![ - AppEvent::Key(key(KeyCode::Char('j'))), - AppEvent::Key(key(KeyCode::Char('j'))), - AppEvent::Key(key(KeyCode::Char('j'))), - AppEvent::Key(key(KeyCode::Char('k'))), - ]; + // See the sibling test above for why this builds two independent copies rather than + // cloning one `Vec`. + let build_events = || { + vec![ + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('k'))), + ] + }; - update_batch(&mut app_batch, &km, &mut pending_batch, events.clone()); - for event in events { + update_batch(&mut app_batch, &km, &mut pending_batch, build_events()); + for event in build_events() { update(&mut app_seq, &km, &mut pending_seq, event); } assert_eq!(app_batch.cursor, app_seq.cursor); @@ -1817,18 +2072,20 @@ mod tests { let mut app_seq2 = many_files_app(&fixture_seq2, 1); let mut pending_batch2: Vec = Vec::new(); let mut pending_seq2: Vec = Vec::new(); - let boundary_events = vec![ - AppEvent::Key(key(KeyCode::Char('k'))), - AppEvent::Key(key(KeyCode::Char('j'))), - ]; + let build_boundary_events = || { + vec![ + AppEvent::Key(key(KeyCode::Char('k'))), + AppEvent::Key(key(KeyCode::Char('j'))), + ] + }; update_batch( &mut app_batch2, &km, &mut pending_batch2, - boundary_events.clone(), + build_boundary_events(), ); - for event in boundary_events { + for event in build_boundary_events() { update(&mut app_seq2, &km, &mut pending_seq2, event); } assert_eq!(app_batch2.cursor, app_seq2.cursor); @@ -1902,13 +2159,15 @@ mod tests { let mut pending_batch: Vec = Vec::new(); let mut pending_seq: Vec = Vec::new(); let cursor_before = app_batch.cursor; - let events = vec![ - AppEvent::Key(key(KeyCode::Char('j'))), // swallowed by the confirm modal - AppEvent::Key(key(KeyCode::Char('n'))), // cancels the confirm - ]; + let build_events = || { + vec![ + AppEvent::Key(key(KeyCode::Char('j'))), // swallowed by the confirm modal + AppEvent::Key(key(KeyCode::Char('n'))), // cancels the confirm + ] + }; - update_batch(&mut app_batch, &km, &mut pending_batch, events.clone()); - for event in events { + update_batch(&mut app_batch, &km, &mut pending_batch, build_events()); + for event in build_events() { update(&mut app_seq, &km, &mut pending_seq, event); } From 559a2a7c15cd5bb6179fe7f40bb2cd749c8c889b Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:04:21 -0400 Subject: [PATCH 06/16] fix(review): make load-spec building total for fileless changesets --- git-workon-review/src/app.rs | 54 ++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 25b69a42..54a1b275 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -1607,13 +1607,19 @@ impl App { /// what [`Self::ensure_loaded`] would read from live state — see [`FileLoadSpec`]'s doc /// comment. Every field is owned (cloned out), so the spec outlives the borrow and can cross /// to the loader thread. - fn current_load_spec(&self) -> FileLoadSpec { + /// + /// `None` when the current changeset's file list doesn't have an entry at `self.current` — + /// a clean uncommitted layer (zero files) is the common case, and a Pending/Failed slot + /// (also zero files) will join this once ADR-037's later changesets land. Total by + /// construction rather than relying on callers to guard first. + fn current_load_spec(&self) -> Option { let idx = self.current; let zoom = self.effective_zoom_for(idx); let diff = &self.cur().diff; - FileLoadSpec { + let combined_file = diff.files.get(idx)?.clone(); + Some(FileLoadSpec { span: self.cur().cs.span, - combined_file: diff.files[idx].clone(), + combined_file, zoom, unstaged_file: diff .unstaged_idx @@ -1627,7 +1633,7 @@ impl App { .copied() .flatten() .map(|mi| diff.staged_model.files[mi].clone()), - } + }) } /// Take the [`FileLoadSpec`] for the current pending open, tagged with the generation/ @@ -1635,18 +1641,18 @@ impl App { /// for this same pending open (see [`Self::open_pending_dispatched`]'s doc comment). The event /// loop calls this on every idle `Tick` while an open is pending; without the dispatched guard /// it would re-send the same request on every one of those ticks until the loader answers. - /// Returns `None` when nothing is pending, or a request already went out for it. + /// Returns `None` when nothing is pending, a request already went out for it, or the + /// current file has no spec to build (see [`Self::current_load_spec`]) — the pending flags + /// are left alone in that last case, matching upstack's eventual empty-file guard in + /// [`Self::open_current`]: this is a total fallback for a defer that outraced it, not a + /// second copy of that guard. pub fn take_pending_load_spec(&mut self) -> Option<(u64, usize, usize, FileLoadSpec)> { if !self.open_pending || self.open_pending_dispatched { return None; } + let spec = self.current_load_spec()?; self.open_pending_dispatched = true; - Some(( - self.generation, - self.current_cs, - self.current, - self.current_load_spec(), - )) + Some((self.generation, self.current_cs, self.current, spec)) } /// Apply one loader result (ADR-037's chokepoint, the `FileReady` inbox arm routes here): @@ -3369,7 +3375,9 @@ mod tests { // exactly the two-handle shape `Tui::run`/`spawn_loader_thread` build for real. let mut spec_app = app_from_fixture(&fixture); spec_app.set_zoom(Zoom::Combined); - let spec = spec_app.current_load_spec(); + let spec = spec_app + .current_load_spec() + .expect("fixture has a file at index 0"); let repo = fixture.repo().unwrap(); let loader_repo = Repository::open(repo.workdir().unwrap()).expect("loader's own repo handle"); @@ -3450,6 +3458,28 @@ mod tests { ); } + #[test] + fn take_pending_load_spec_is_none_for_a_fileless_changeset_without_panicking() { + // A clean uncommitted layer diffs to zero files. A pending open onto it (e.g. one that + // outraces a refresh, or the Pending/Failed slots ADR-037's later changesets introduce) + // must not panic `current_load_spec`'s file-list indexing — F7's regression. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + assert!(app.files().is_empty(), "fixture must have no diffed files"); + app.set_defer_loads(true); + app.open_current(); + assert!(app.open_pending(), "empty-file open still marks pending"); + + assert!( + app.take_pending_load_spec().is_none(), + "no spec can be built for a file that doesn't exist" + ); + } + #[test] fn apply_file_ready_drops_a_stale_generation_result() { let fixture = FixtureBuilder::new() From d731ba7212fff7f93f4372d4b026192c2dc3d5c7 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:07:18 -0400 Subject: [PATCH 07/16] fix(review): re-dispatch a deferred open when zoom outran its load --- git-workon-review/src/app.rs | 102 +++++++++++++++++++++++++++++++---- 1 file changed, 92 insertions(+), 10 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 54a1b275..76419b8d 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -1666,11 +1666,18 @@ impl App { /// notice (never silently stranding the file — see this changeset's report for why a footer /// notice, not a new per-file `Failed` state, is the shape chosen here). /// - /// Either way, when the readied file IS the current pending open, it's seated exactly like - /// [`Self::complete_pending_open`]'s tail: `open_pending` clears regardless of `Ok`/`Err` — a - /// failed load must not leave the placeholder stuck forever. Correctness never depends on - /// this: the next nav/force-completion retries via [`Self::ensure_loaded`]'s ordinary - /// cache-miss path, which is where a load actually being CORRECT is guaranteed. + /// Either way, when the readied file IS the current pending open, it's seated like + /// [`Self::complete_pending_open`]'s tail — with one refinement over a plain "always clear" + /// rule: an `Ok` result only clears the pending open when its SHAPE satisfies the current + /// effective zoom (see [`loaded_views_satisfy`]). Without this, a zoom cycled mid-load + /// (`z` is exempt from force-completion — [`Self::open_current`] re-defers with + /// `open_pending_dispatched = false`) lets the stale-shaped in-flight result seat only the + /// old view, clear the pending flags, and strand the new zoom's view forever un-dispatched. + /// When unsatisfied, `open_pending` stays set and `open_pending_dispatched` resets to + /// `false` so the next idle Tick re-dispatches against the NOW-current zoom — mirroring a + /// fresh [`Self::open_current`] defer. An `Err` result keeps clearing unconditionally: a + /// failed load must not leave the placeholder stuck forever, and correctness never depends + /// on this path — the sync fallback owns correctness (see this method's summary above). pub fn apply_file_ready( &mut self, gen: u64, @@ -1681,8 +1688,12 @@ impl App { if gen != self.generation { return; } + let is_current_pending = + cs_idx == self.current_cs && file_idx == self.current && self.open_pending; match result { Ok(views) => { + let satisfies_current_zoom = is_current_pending + && loaded_views_satisfy(&views, self.effective_zoom_for(file_idx)); if let Some(cs) = self.changesets.get_mut(cs_idx) { match views { LoadedViews::Single(role, view) => set_if_absent(cs, role, file_idx, view), @@ -1692,16 +1703,25 @@ impl App { } } } + if is_current_pending { + if satisfies_current_zoom { + self.reset_panes(); + self.open_pending = false; + self.open_pending_dispatched = false; + } else { + self.open_pending_dispatched = false; + } + } } Err(message) => { self.notify(format!("failed to load file: {message}"), Severity::Error); + if is_current_pending { + self.reset_panes(); + self.open_pending = false; + self.open_pending_dispatched = false; + } } } - if cs_idx == self.current_cs && file_idx == self.current && self.open_pending { - self.reset_panes(); - self.open_pending = false; - self.open_pending_dispatched = false; - } } /// Cycle the requested zoom `Split → Combined → Unstaged → Staged → Split` (`z`). The new zoom @@ -2911,6 +2931,20 @@ pub enum LoadedViews { }, } +/// Whether a loaded result's SHAPE — what zoom it was built against, per [`FileLoadSpec::zoom`] +/// — still matches `current_zoom`, the current file's effective zoom at result-apply time. Used +/// by [`App::apply_file_ready`] to tell a still-useful deferred-open result apart from one a +/// mid-load `z` cycle outran: `Single` satisfies only the SAME role's `Single`, `Split` +/// satisfies only `Split` (never the reverse — a `Split` result doesn't seat a `Single` open, +/// and vice versa, even though `set_if_absent` already caches whichever roles it carries). +fn loaded_views_satisfy(views: &LoadedViews, current_zoom: EffectiveZoom) -> bool { + match (views, current_zoom) { + (LoadedViews::Single(role, _), EffectiveZoom::Single(want)) => *role == want, + (LoadedViews::Split { .. }, EffectiveZoom::Split) => true, + _ => false, + } +} + /// Build every [`FileView`] a [`FileLoadSpec`] needs, against `repo`/`ts` — the ADR-037 loader /// thread's pure job body: unit-testable directly against a fixture repo, no threads or channels /// involved. Routes through the SAME [`build_combined_view`]/[`build_sub_role_view`] free @@ -3437,6 +3471,54 @@ mod tests { assert_eq!(deferred_view.display.len(), eager_view.display.len()); } + #[test] + fn apply_file_ready_redispatches_when_zoom_outran_the_in_flight_load() { + // F2 regression: a zoom cycled mid-load must not let the stale-shaped in-flight result + // seat and clear the pending open — the new zoom's view would then never be dispatched. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .partially_staged_file("f.txt", "committed\n", "staged\n", "workdir\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + // Default zoom is `Split`; this file has both staged and unstaged sub-diffs, so the + // effective zoom stays `Split` too. + app.open_current(); + assert!(app.open_pending(), "deferred open must be pending"); + + let (gen, cs_idx, file_idx, spec) = app + .take_pending_load_spec() + .expect("first take dispatches against the Split zoom"); + assert_eq!(spec.zoom, EffectiveZoom::Split); + + // Mid-load `z`: CycleZoom is exempt from force-completion, so this re-defers the open + // against the NEW zoom instead of blocking for it. + app.cycle_zoom(); + assert!( + app.open_pending(), + "cycling zoom while a load is pending must still be pending" + ); + + // The loader answers the now-STALE (Split) request. + let repo = fixture.repo().unwrap(); + let loader_repo = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut loader_ts = crate::highlight::TsHighlighter::new(); + let views = build_file_views(&loader_repo, &mut loader_ts, &spec); + + app.apply_file_ready(gen, cs_idx, file_idx, Ok(views)); + + assert!( + app.open_pending(), + "a stale-shaped result must not clear the pending open" + ); + assert!( + app.take_pending_load_spec().is_some(), + "the next Tick must re-dispatch against the current (Combined) zoom" + ); + } + #[test] fn take_pending_load_spec_dispatches_at_most_once_per_pending_open() { let fixture = FixtureBuilder::new() From 15b57cb822907d2f14dd8d30923e7e0960fad688 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 01:50:16 -0400 Subject: [PATCH 08/16] feat(review): stream startup changeset diffs behind a live outline --- git-workon-review/src/app.rs | 175 +++++++++++++++++++++++++++++++++ git-workon-review/src/main.rs | 180 ++++++++++++++++++++-------------- git-workon-review/src/tui.rs | 129 ++++++++++++++++++++++++ 3 files changed, 410 insertions(+), 74 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 76419b8d..6cb46f57 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -892,6 +892,12 @@ pub struct App { /// ([`Self::apply_file_ready`]) — the ONLY drop rule; within a generation, results are cached /// even if the user navigated away (warmth, not staleness — see the ADR's "Generations"). generation: u64, + /// Whether the startup wave (ADR-037's streamed launch) has already raised its one footer + /// notice for a `ChangesetReady { result: Err }`. Set by [`Self::apply_changeset_ready`], + /// never cleared in this slice (only ONE wave — startup — exists yet; the refresh path stays + /// synchronous, so nothing re-arms it). "the wave's first failure raises a footer notice" — + /// this is what makes it FIRST, not every one of a bad stack's failures. + wave_failure_notified: bool, } /// A destructive staging op deferred behind a [`Confirm`], identified by index into [`App::files`] @@ -1031,6 +1037,7 @@ impl App { open_pending: false, open_pending_dispatched: false, generation: 1, + wave_failure_notified: false, }; // Position the outline cursor on the changeset/file the lib marked `current` (the same // row `sync_outline_to_current` would reposition to after any diff-initiated nav) rather @@ -1724,6 +1731,60 @@ impl App { } } + /// Apply one streamed-diff wave result (ADR-037's `ChangesetReady` chokepoint, the streamed- + /// launch counterpart to [`Self::apply_file_ready`]): dropped outright on a generation + /// mismatch, same rule and same reason (the world the wave was diffing no longer exists — + /// e.g. a refresh ran mid-wave). Otherwise replaces changeset `idx`'s slot in place: + /// + /// - `Ok(diff)` builds its `Ready` [`ChangesetView`] via [`ChangesetView::from_changeset_diff`] + /// — the SAME router [`main.rs`'s lone-changeset sync path uses, so a streamed changeset's + /// `DiffState`/view caches are byte-identical to what a synchronous diff would have built. + /// - `Err(message)` builds a `Failed` slot carrying it ([`ChangesetView::failed`]); the wave's + /// FIRST failure (across the whole launch, not per-changeset) raises a footer notice — see + /// [`Self::wave_failure_notified`]'s doc comment — and the review continues (a stack with one + /// corrupt changeset still shows the other N-1). + /// + /// When `idx` IS the active changeset (the outline cursor already sits there — either it was + /// the lib-marked `current` changeset at launch, or the user navigated onto its still-`Pending` + /// placeholder), it's seated exactly as a fresh open would be: `current` resets to its first + /// file and [`Self::open_current`] runs (deferred-open semantics — CS4's placeholder shows + /// until the file itself loads), then the outline cursor resyncs. Nothing here requires the + /// user to navigate away and back for a just-readied active changeset to become interactive. + pub fn apply_changeset_ready( + &mut self, + gen: u64, + idx: usize, + result: Result, + ) { + if gen != self.generation { + return; + } + let Some(existing) = self.changesets.get(idx) else { + return; + }; + let cs = existing.cs.clone(); + match result { + Ok(diff) => { + self.changesets[idx] = ChangesetView::from_changeset_diff(cs, diff); + } + Err(message) => { + if !self.wave_failure_notified { + self.notify( + format!("failed to diff a changeset: {message}"), + Severity::Error, + ); + self.wave_failure_notified = true; + } + self.changesets[idx] = ChangesetView::failed(cs, message); + } + } + if idx == self.current_cs { + self.current = 0; + self.open_current(); + self.sync_outline_to_current(); + } + } + /// Cycle the requested zoom `Split → Combined → Unstaged → Staged → Split` (`z`). The new zoom /// persists across file navigation; both panes reset to their first hunks so `cursor`/`scroll` /// are always valid for the now-active view(s). @@ -6334,6 +6395,120 @@ mod tests { ); } + // ── ADR-037: the streamed-launch wave's chokepoint ─────────────────────────── + + #[test] + fn apply_changeset_ready_seats_the_active_changeset_when_its_diff_lands() { + let fixture = two_changes_one_hunk_fixture(); + let repo = fixture.repo().unwrap(); + let view_a = ChangesetView::pending(bare_changeset("cs-a", true)); + let view_b = ChangesetView::pending(bare_changeset("cs-b", false)); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view_a, view_b]); + assert!(app.is_current_pending()); + + let diffs = crate::acquire::diff_uncommitted(repo).unwrap(); + app.apply_changeset_ready( + app.generation(), + 0, + Ok(crate::acquire::ChangesetDiff::Uncommitted(diffs)), + ); + + assert!( + !app.is_current_pending(), + "the readied ACTIVE changeset must be seated, not left Pending" + ); + assert!(!app.files().is_empty()); + assert!( + app.current_view_ref().is_some(), + "seating an active changeset opens its first file exactly like a fresh open would" + ); + } + + #[test] + fn apply_changeset_ready_marks_a_non_active_changeset_ready_without_disturbing_current() { + let fixture = two_changes_one_hunk_fixture(); + let repo = fixture.repo().unwrap(); + let view_a = ChangesetView::pending(bare_changeset("cs-a", true)); + let view_b = ChangesetView::pending(bare_changeset("cs-b", false)); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view_a, view_b]); + + let diffs = crate::acquire::diff_uncommitted(repo).unwrap(); + app.apply_changeset_ready( + app.generation(), + 1, + Ok(crate::acquire::ChangesetDiff::Uncommitted(diffs)), + ); + + assert_eq!(app.current_cs(), 0, "the active changeset must not move"); + assert!( + app.is_current_pending(), + "cs-a is still Pending — only cs-b's slot changed" + ); + app.next_changeset(); + assert!( + !app.is_current_pending(), + "cs-b's slot is now Ready after navigating onto it" + ); + } + + #[test] + fn apply_changeset_ready_drops_a_stale_generation_result() { + let fixture = two_changes_one_hunk_fixture(); + let repo = fixture.repo().unwrap(); + let view = ChangesetView::pending(bare_changeset("cs-a", true)); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + let stale_gen = app.generation(); + app.generation += 1; // simulate a refresh landing between dispatch and result + + let diffs = crate::acquire::diff_uncommitted(repo).unwrap(); + app.apply_changeset_ready( + stale_gen, + 0, + Ok(crate::acquire::ChangesetDiff::Uncommitted(diffs)), + ); + + assert!( + app.is_current_pending(), + "a stale-generation result must not seat a changeset from a world that no longer exists" + ); + } + + #[test] + fn apply_changeset_ready_err_marks_failed_and_notifies_only_on_the_first_failure() { + let fixture = two_changes_one_hunk_fixture(); + let repo = fixture.repo().unwrap(); + let view_a = ChangesetView::pending(bare_changeset("cs-a", true)); + let view_b = ChangesetView::pending(bare_changeset("cs-b", false)); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view_a, view_b]); + assert!(app.notice.is_none()); + + app.apply_changeset_ready(app.generation(), 0, Err("first failure".to_string())); + assert!( + app.current_failure().is_some(), + "the active changeset's Failed slot carries the message" + ); + let notice = app + .notice + .as_ref() + .expect("the wave's first failure raises a footer notice"); + assert_eq!(notice.severity, Severity::Error); + assert!(notice.text.contains("first failure")); + + // A SECOND failure in the same wave must not raise a second notice — only the wave's + // FIRST failure does (see `App::wave_failure_notified`'s doc comment). The review + // continues: cs-b's slot still becomes Failed even though no new notice fires. + app.apply_changeset_ready(app.generation(), 1, Err("second failure".to_string())); + let notice_after = app.notice.as_ref().unwrap(); + assert!( + notice_after.text.contains("first failure"), + "a second failure in the same wave must not overwrite the first's notice" + ); + } + #[test] fn staged_status_column_only_populated_for_the_uncommitted_changesets_files() { let mut app = committed_and_uncommitted_stack(); diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index c6f3f264..1fec2c48 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -119,92 +119,124 @@ fn main() -> Result<()> { terminal_query::flush_pending_tty_input(); } - // CS5: take the terminal and show launch activity while the diffs build — on a deep stack - // this is the bulk of the launch, and until CS5 it left the terminal dead the whole time. - // Everything that could print, prompt, or flush is done (see the block comment above the - // resolve), so from here the terminal belongs to the TUI. `Tui`'s Drop restores it, so the - // `?`s below put the shell back before miette prints their error. + // CS5: take the terminal while the diffs build — on a deep stack this used to be the bulk of + // the launch with the terminal dead the whole time. Everything that could print, prompt, or + // flush is done (see the block comment above the resolve), so from here the terminal belongs + // to the TUI. `Tui`'s Drop restores it, so the `?`s below put the shell back before miette + // prints their error. // // An acquire FAILURE (no controlling tty — CI, a test harness, a bare pipe) is carried, not // propagated here: a clean worktree's "nothing to review" is only detectable AFTER the diff // below (resolve always yields at least the uncommitted changeset), and that exit must stay // tty-free, exactly as it was when the terminal was only taken inside the run call. The - // error surfaces at the run call — the same logical point it always did. Splash failures on - // an acquired terminal are cosmetic (the run call will surface anything real) and ignored. + // error surfaces at the run call — the same logical point it always did. let mut tui = tui::Tui::acquire(); - if let Ok(tui) = tui.as_mut() { - let noun = if changesets.len() == 1 { - "changeset" - } else { - "changesets" - }; - let _ = tui.splash(&format!("diffing {} {noun}…", changesets.len())); - } - let diffs = diff_changesets(&repo, &changesets).into_diagnostic()?; - let views: Vec = changesets - .into_iter() - .zip(diffs) - .map(|(cs, diff)| ChangesetView::from_changeset_diff(cs, diff)) - .collect(); - - // The single-uncommitted-changeset case with nothing in it only shows up in the built - // views' file counts — the mirror of the resolve-level empty check above, and the same - // "nothing to review" + exit 0 (ADR-036), never a `views` list handed to - // `App::from_changesets`, which panics on empty input. Restore the terminal BEFORE - // printing: the message must land on the normal screen, not vanish with the alternate one. - // A tty-less launch has no terminal to restore — the message prints exactly as before CS5. - if views.is_empty() || (views.len() == 1 && views[0].file_count() == 0) { + + // ADR-037: `main.rs` forks on `changesets.len()` — streaming's grain is per-changeset, so a + // lone changeset (the non-Graphite default, a ref/range, a PR) gains nothing from it and + // keeps today's synchronous path byte-identical (down to the `clean_worktree_prints_ + // nothing_to_review_and_exits_success` canary, which must stay tty-free). A real stack + // streams instead: the outline appears immediately with every row `Pending`, diffs land + // as they complete, and the splash — redundant once the first frame IS the live outline — + // is skipped entirely. + let repo_path = repo.workdir().unwrap_or_else(|| repo.path()).to_path_buf(); + + if changesets.len() == 1 { if let Ok(tui) = tui.as_mut() { - tui.restore().into_diagnostic()?; + let _ = tui.splash("diffing 1 changeset…"); } - match cli.source.as_deref() { - Some(text) => eprintln!("nothing to review in {text}"), - None => eprintln!("nothing to review"), + let diffs = diff_changesets(&repo, &changesets).into_diagnostic()?; + let views: Vec = changesets + .into_iter() + .zip(diffs) + .map(|(cs, diff)| ChangesetView::from_changeset_diff(cs, diff)) + .collect(); + + // The single-uncommitted-changeset case with nothing in it only shows up in the built + // views' file counts — the mirror of the resolve-level empty check above, and the same + // "nothing to review" + exit 0 (ADR-036), never a `views` list handed to + // `App::from_changesets`, which panics on empty input. Restore the terminal BEFORE + // printing: the message must land on the normal screen, not vanish with the alternate + // one. A tty-less launch has no terminal to restore — the message prints exactly as + // before CS5. + if views.is_empty() || (views.len() == 1 && views[0].file_count() == 0) { + if let Ok(tui) = tui.as_mut() { + tui.restore().into_diagnostic()?; + } + match cli.source.as_deref() { + Some(text) => eprintln!("nothing to review in {text}"), + None => eprintln!("nothing to review"), + } + return Ok(()); } - return Ok(()); - } - // Captured BEFORE `repo` moves into `App` below — the ADR-037 loader thread needs its own - // `Repository` handle onto the same on-disk repo (`git2::Repository` is `Send` but not - // `Sync`, so it can't cross threads directly), opened the same way - // `crate::acquire::diff_changesets`'s worker threads already do: at the workdir so the - // uncommitted layer's index/worktree diffs resolve correctly, falling back to the gitdir for - // a bare repo (where only committed spans can occur). - let repo_path = repo.workdir().unwrap_or_else(|| repo.path()).to_path_buf(); + // `App` owns its own `Repository` handle (see `app.rs`'s doc comment) — moved in here + // after acquisition is done borrowing it. `App::from_changesets` opens on whichever + // changeset the lib marked `current` (locked decision #6). + let mut app = App::from_changesets(repo, views); + if let Some(source) = source { + app.set_review_source(source); + } + // CS4: defer file loads to the event loop's input-idle window rather than blocking here + // (or on any later selection change) — `app.open_current()` below marks the initial open + // pending instead of loading eagerly; see `tui::run`'s doc comment for the resulting + // startup contract. + app.set_defer_loads(true); + + // Apply CS7's view-config settings BEFORE `open_current`: `App::apply_view_config`'s + // setters only set the raw layout/zoom/mode/width fields, and `open_current` is what + // derives `cursor`/`scroll` fresh from whichever settings just landed (see each setter's + // doc comment). + let view_config_warnings = app.apply_view_config(&view_config); + app.open_current(); + + // A misconfigured keybinding or view-config setting is non-fatal: show the collected + // warnings as a startup notice (cleared on the first keypress, like any notice) and run + // with the defaults for those keys/settings. + let mut warnings = keymap.warnings().to_vec(); + warnings.extend(view_config_warnings); + if !warnings.is_empty() { + app.notify(warnings.join("; "), Severity::Error); + } - // `App` owns its own `Repository` handle (see `app.rs`'s doc comment) — moved in here after - // acquisition is done borrowing it. `App::from_changesets` opens on whichever changeset the - // lib marked `current` (locked decision #6). - let mut app = App::from_changesets(repo, views); - if let Some(source) = source { - app.set_review_source(source); - } - // CS4: defer file loads to the event loop's input-idle window rather than blocking here (or - // on any later selection change) — `app.open_current()` below marks the initial open pending - // instead of loading eagerly; see `tui::run`'s doc comment for the resulting startup contract. - app.set_defer_loads(true); - - // Apply CS7's view-config settings BEFORE `open_current`: `App::apply_view_config`'s setters - // only set the raw layout/zoom/mode/width fields, and `open_current` is what derives - // `cursor`/`scroll` fresh from whichever settings just landed (see each setter's doc - // comment). - let view_config_warnings = app.apply_view_config(&view_config); - app.open_current(); - - // A misconfigured keybinding or view-config setting is non-fatal: show the collected - // warnings as a startup notice (cleared on the first keypress, like any notice) and run with - // the defaults for those keys/settings. - let mut warnings = keymap.warnings().to_vec(); - warnings.extend(view_config_warnings); - if !warnings.is_empty() { - app.notify(warnings.join("; "), Severity::Error); - } + // A carried acquire failure surfaces HERE — the same logical point (running the TUI) it + // surfaced at before CS5 moved the terminal takeover ahead of the diff phase. + tui.into_diagnostic()? + .run(&mut app, &keymap, &theme, repo_path) + .into_diagnostic()?; + } else { + // Every changeset starts `Pending` (ADR-037's "Slots") — `App` is constructible from + // resolved-but-undiffed changesets, so the outline's headers render on the FIRST frame, + // before a single byte has been diffed. No splash: the live outline IS the launch + // feedback. + let views: Vec = changesets + .iter() + .cloned() + .map(ChangesetView::pending) + .collect(); + + let mut app = App::from_changesets(repo, views); + if let Some(source) = source { + app.set_review_source(source); + } + app.set_defer_loads(true); + let view_config_warnings = app.apply_view_config(&view_config); + // The active changeset is `Pending` (no files yet) — `open_current` is still the right + // call: it's a no-op on an empty file list, and re-running it the moment the active + // changeset's diff lands (`Tui::run_streamed`'s `ChangesetReady` handling) is what + // actually seats the first real file. + app.open_current(); + + let mut warnings = keymap.warnings().to_vec(); + warnings.extend(view_config_warnings); + if !warnings.is_empty() { + app.notify(warnings.join("; "), Severity::Error); + } - // A carried acquire failure surfaces HERE — the same logical point (running the TUI) it - // surfaced at before CS5 moved the terminal takeover ahead of the diff phase. - tui.into_diagnostic()? - .run(&mut app, &keymap, &theme, repo_path) - .into_diagnostic()?; + tui.into_diagnostic()? + .run_streamed(&mut app, &keymap, &theme, repo_path, changesets) + .into_diagnostic()?; + } Ok(()) } diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index ba66c205..51761df2 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -33,6 +33,8 @@ use ratatui::backend::CrosstermBackend; use ratatui::style::{Modifier, Style}; use ratatui::widgets::Paragraph; use ratatui::{Frame, Terminal}; +use workon::Changeset; +use workon_review::acquire::{diff_changeset, ChangesetDiff}; use workon_review::app::{self, App, FileLoadSpec, LoadedViews}; use workon_review::highlight::TsHighlighter; use workon_review::keymap::{Command, Dispatch, KeyPress, Keymap}; @@ -61,6 +63,17 @@ pub enum AppEvent { file_idx: usize, result: Result, }, + /// One changeset's streamed-diff result — ADR-037's streamed-launch counterpart to + /// `FileReady`, forwarded from the wave thread [`spawn_wave_thread`] spawns. `gen`/`idx` echo + /// the wave's stamp/the changeset's position in `App`'s stack; `result` is `Err` for a + /// changeset whose diff itself failed (a bad/garbage `Oid`, not a job panic — see + /// [`spawn_wave_thread`]'s doc comment). Applied at ONE chokepoint: + /// [`App::apply_changeset_ready`]. + ChangesetReady { + gen: u64, + idx: usize, + result: Result, + }, } impl PartialEq for AppEvent { @@ -216,6 +229,91 @@ fn spawn_loader_thread( req_tx } +/// Spawn the ADR-037 startup wave: stripe `changesets` (lib-`current` first, then input order) +/// across `available_parallelism`-many transient WORKER threads — same fan-out shape as +/// `crate::acquire::diff_changesets` (each worker opens its own `Repository`, since +/// `git2::Repository` is `Send` but not `Sync`) — but STREAM each result the instant it completes +/// via `tx` rather than joining the batch. Never joined itself either — a wave straggler left +/// running past quit is harmless (it only ever sends into an inbox nothing is listening to +/// anymore; `tx.send` failing is the signal each worker already checks). +/// +/// A DELIBERATELY separate set of threads from the loader thread (ADR-037 leaves this shape +/// open — "yours to shape"): the wave never touches the loader's request queue, so an in-flight +/// wave can never starve a `LoadFile` request behind it — they run on entirely disjoint threads +/// with entirely disjoint work queues. The cost is a second family of `Repository` handles +/// (`workers + 1`, alongside the loader's one) alive for the wave's brief lifetime; accepted for +/// the starvation-freedom it buys for free. +/// +/// A changeset whose own diff fails (a bad/garbage `Oid` — see [`diff_changeset`]'s doc comment) +/// sends `Err` for THAT changeset only; a worker whose own `Repository::open` fails sends `Err` +/// for every changeset in its chunk (mirroring `diff_changesets`' per-chunk failure shape) rather +/// than silently dropping them — every index must get exactly one result, or its slot stays +/// `Pending` forever with nothing left to complete it. +fn spawn_wave_thread( + repo_path: PathBuf, + tx: mpsc::Sender, + changesets: Vec, + gen: u64, +) { + thread::spawn(move || { + let n = changesets.len(); + if n == 0 { + return; + } + // Current changeset first, then input order for the rest — the changeset the user lands + // on becomes interactive earliest (ADR-037's "Slots"). + let current_idx = changesets.iter().position(|cs| cs.current); + let mut order: Vec = Vec::with_capacity(n); + order.extend(current_idx); + order.extend((0..n).filter(|&i| Some(i) != current_idx)); + + let workers = thread::available_parallelism() + .map(std::num::NonZeroUsize::get) + .unwrap_or(1) + .min(n); + let chunk = n.div_ceil(workers.max(1)); + + thread::scope(|scope| { + for idx_chunk in order.chunks(chunk) { + let tx = tx.clone(); + let changesets = &changesets; + let repo_path = &repo_path; + scope.spawn(move || { + let repo = match Repository::open(repo_path) { + Ok(repo) => repo, + Err(err) => { + let message = err.to_string(); + for &idx in idx_chunk { + if tx + .send(Ok(AppEvent::ChangesetReady { + gen, + idx, + result: Err(message.clone()), + })) + .is_err() + { + return; // main loop is gone + } + } + return; + } + }; + for &idx in idx_chunk { + let result = + diff_changeset(&repo, &changesets[idx]).map_err(|e| e.to_string()); + if tx + .send(Ok(AppEvent::ChangesetReady { gen, idx, result })) + .is_err() + { + return; // main loop is gone; nothing left to forward to + } + } + }); + } + }); + }); +} + /// Receive the next event from `inbox`, waiting up to `timeout`. A timeout with nothing received /// yields `Ok(AppEvent::Tick)` — the loop's regular redraw beat, and the mechanism the M4 index /// watcher polls on (see the module doc). A disconnected inbox (the input thread panicked, or @@ -547,6 +645,10 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap app.apply_file_ready(gen, cs_idx, file_idx, result); false } + AppEvent::ChangesetReady { gen, idx, result } => { + app.apply_changeset_ready(gen, idx, result); + false + } } } @@ -789,6 +891,33 @@ impl Tui { result.and(restored) } + /// ADR-037's streamed-launch counterpart to [`Self::run`]: for a stack of MORE than one + /// changeset, `main.rs` calls this instead — `app` is already constructible from + /// resolved-but-undiffed changesets (every slot `Pending`), and this is what starts the + /// diffing itself, alongside the input/loader threads `run` always spawns. No splash: the + /// first frame `event_loop` draws IS the live outline with `Pending` rows (see + /// `main.rs`'s block comment on the `changesets.len()` fork). + /// + /// `changesets` is the SAME resolved list `app`'s `Pending` slots were built from — handed + /// here (rather than re-read off `app`) since `App` only keeps [`workon_review::app:: + /// ChangesetView`]s, not the bare [`Changeset`]s the wave diffs against. + pub fn run_streamed( + &mut self, + app: &mut App, + keymap: &Keymap, + theme: &Palette, + repo_path: PathBuf, + changesets: Vec, + ) -> io::Result<()> { + let (tx, rx) = mpsc::channel::(); + spawn_input_thread(tx.clone()); + let load_tx = spawn_loader_thread(repo_path.clone(), tx.clone()); + spawn_wave_thread(repo_path, tx, changesets, app.generation()); + let result = event_loop(&mut self.terminal, app, keymap, theme, &rx, &load_tx); + let restored = self.restore(); + result.and(restored) + } + /// Put the terminal back (raw mode off, leave the alternate screen, cursor shown). Idempotent /// — a second call (including the one [`Drop`] always makes) is a no-op, so explicit callers /// (the "nothing to review" exit, which must restore BEFORE its `eprintln`) and the drop From c24640c680a94cd20b4b0f301e6ce151e559816b Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:10:21 -0400 Subject: [PATCH 09/16] fix(review): keep outline cursor anchored as streamed diffs land --- git-workon-review/src/app.rs | 93 ++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 6cb46f57..6cfb5071 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -1763,6 +1763,18 @@ impl App { return; }; let cs = existing.cs.clone(); + // F3: a landed NON-active changeset inserts file rows into the outline's row list, + // silently shifting a plain row-index cursor. Capture the identity of the row under the + // cursor now (before the slot swap rebuilds `outline_items()`) so it can be re-found + // afterward — the active-changeset case doesn't need this, since `sync_outline_to_current` + // below already repositions by diff identity, not row index. + let cursor_identity = if idx != self.current_cs { + self.outline_items() + .get(self.outline.cursor) + .and_then(outline_row_identity) + } else { + None + }; match result { Ok(diff) => { self.changesets[idx] = ChangesetView::from_changeset_diff(cs, diff); @@ -1782,6 +1794,18 @@ impl App { self.current = 0; self.open_current(); self.sync_outline_to_current(); + } else if let Some(identity) = cursor_identity { + let items = self.outline_items(); + if let Some(new_idx) = items + .iter() + .position(|it| outline_row_identity(it) == Some(identity)) + { + self.outline.cursor = new_idx; + } else { + // The identified row is gone (e.g. Flat mode deduped it out) — fall back to the + // same clamp `sync_outline_to_current` uses. + self.outline.cursor = self.outline.cursor.min(items.len().saturating_sub(1)); + } } } @@ -2956,6 +2980,21 @@ fn current_cs_index(changesets: &[ChangesetView]) -> usize { changesets.iter().position(|v| v.cs.current).unwrap_or(0) } +/// The `(cs_idx, file_idx)` identity an [`OutlineItem::Header`]/[`OutlineItem::File`] row +/// carries — `file_idx` is `None` for a header row. `OutlineItem::Dir` carries no `cs_idx` at +/// all (see its doc comment) and has no identity to preserve. Used by +/// [`App::apply_changeset_ready`] (F3) to re-find the row the outline cursor was on after a +/// streamed diff landing inserts/removes rows ahead of it in the row-index space. +fn outline_row_identity(item: &OutlineItem) -> Option<(usize, Option)> { + match item { + OutlineItem::Header { cs_idx, .. } => Some((*cs_idx, None)), + OutlineItem::File { + cs_idx, file_idx, .. + } => Some((*cs_idx, Some(*file_idx))), + OutlineItem::Dir { .. } => None, + } +} + // ── ADR-037: the loader thread's stateless request/job shape ──────────────────── /// Everything the ADR-037 loader job needs to reproduce one file's [`App::ensure_loaded`] work @@ -6509,6 +6548,60 @@ mod tests { ); } + #[test] + fn apply_changeset_ready_keeps_outline_cursor_anchored_when_an_earlier_non_active_changeset_lands( + ) { + // F3 regression: cs-a sits BEFORE the active cs-b in the outline row list. Landing cs-a's + // diff inserts its file rows ahead of cs-b's header, shifting every row-index cursor at + // or after cs-a's header — a plain row-index cursor would silently drift onto one of + // cs-a's new file rows instead of staying on cs-b's header. + let fixture = two_changes_one_hunk_fixture(); + let repo = fixture.repo().unwrap(); + let view_a = ChangesetView::pending(bare_changeset("cs-a", false)); + let view_b = ChangesetView::pending(bare_changeset("cs-b", true)); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view_a, view_b]); + app.outline.mode = OutlineMode::Stack; + assert_eq!( + app.current_cs(), + 1, + "cs-b is the lib-marked current changeset" + ); + + let items_before = app.outline_items(); + let cursor_before = items_before + .iter() + .position(|it| matches!(it, OutlineItem::Header { cs_idx: 1, .. })) + .expect("cs-b's header row exists before cs-a lands"); + app.outline.cursor = cursor_before; + + let diffs = crate::acquire::diff_uncommitted(repo).unwrap(); + app.apply_changeset_ready( + app.generation(), + 0, + Ok(crate::acquire::ChangesetDiff::Uncommitted(diffs)), + ); + + let items_after = app.outline_items(); + assert!( + items_after.len() > items_before.len(), + "cs-a's file rows must have been inserted ahead of cs-b's header" + ); + assert_eq!( + items_after[app.outline_cursor()], + OutlineItem::Header { + cs_idx: 1, + label: "cs-b".to_string(), + current: true, + needs_restack: false, + loading: true, + failed: false, + }, + "the outline cursor must still identify cs-b's header row, not whatever row now \ + sits at its old index" + ); + } + #[test] fn staged_status_column_only_populated_for_the_uncommitted_changesets_files() { let mut app = committed_and_uncommitted_stack(); From f7ae06041f72f593f3a63a30bb8227ffefa5fbc2 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:11:51 -0400 Subject: [PATCH 10/16] refactor(review): hoist the shared app-seating tail out of the launch fork --- git-workon-review/src/main.rs | 86 ++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 42 deletions(-) diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index 1fec2c48..61ed4cb0 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -173,31 +173,7 @@ fn main() -> Result<()> { // `App` owns its own `Repository` handle (see `app.rs`'s doc comment) — moved in here // after acquisition is done borrowing it. `App::from_changesets` opens on whichever // changeset the lib marked `current` (locked decision #6). - let mut app = App::from_changesets(repo, views); - if let Some(source) = source { - app.set_review_source(source); - } - // CS4: defer file loads to the event loop's input-idle window rather than blocking here - // (or on any later selection change) — `app.open_current()` below marks the initial open - // pending instead of loading eagerly; see `tui::run`'s doc comment for the resulting - // startup contract. - app.set_defer_loads(true); - - // Apply CS7's view-config settings BEFORE `open_current`: `App::apply_view_config`'s - // setters only set the raw layout/zoom/mode/width fields, and `open_current` is what - // derives `cursor`/`scroll` fresh from whichever settings just landed (see each setter's - // doc comment). - let view_config_warnings = app.apply_view_config(&view_config); - app.open_current(); - - // A misconfigured keybinding or view-config setting is non-fatal: show the collected - // warnings as a startup notice (cleared on the first keypress, like any notice) and run - // with the defaults for those keys/settings. - let mut warnings = keymap.warnings().to_vec(); - warnings.extend(view_config_warnings); - if !warnings.is_empty() { - app.notify(warnings.join("; "), Severity::Error); - } + let mut app = seat_app(repo, views, source, &view_config, &keymap); // A carried acquire failure surfaces HERE — the same logical point (running the TUI) it // surfaced at before CS5 moved the terminal takeover ahead of the diff phase. @@ -215,23 +191,7 @@ fn main() -> Result<()> { .map(ChangesetView::pending) .collect(); - let mut app = App::from_changesets(repo, views); - if let Some(source) = source { - app.set_review_source(source); - } - app.set_defer_loads(true); - let view_config_warnings = app.apply_view_config(&view_config); - // The active changeset is `Pending` (no files yet) — `open_current` is still the right - // call: it's a no-op on an empty file list, and re-running it the moment the active - // changeset's diff lands (`Tui::run_streamed`'s `ChangesetReady` handling) is what - // actually seats the first real file. - app.open_current(); - - let mut warnings = keymap.warnings().to_vec(); - warnings.extend(view_config_warnings); - if !warnings.is_empty() { - app.notify(warnings.join("; "), Severity::Error); - } + let mut app = seat_app(repo, views, source, &view_config, &keymap); tui.into_diagnostic()? .run_streamed(&mut app, &keymap, &theme, repo_path, changesets) @@ -240,3 +200,45 @@ fn main() -> Result<()> { Ok(()) } + +/// The app-seating tail both `changesets.len()` arms of `main` share byte-identically (F5): +/// build `App` from `views`, wire the review source, defer file loads (CS4), apply CS7's +/// view-config settings, open the current file, and surface any keymap/view-config warnings as +/// a startup notice. `open_current` is a no-op on an empty file list — safe for the streamed +/// arm's `Pending` slots (no files yet), which `Tui::run_streamed`'s `ChangesetReady` handling +/// re-runs it for once the active changeset's diff actually lands. +fn seat_app( + repo: Repository, + views: Vec, + source: Option, + view_config: &config::RawViewConfig, + keymap: &Keymap, +) -> App { + let mut app = App::from_changesets(repo, views); + if let Some(source) = source { + app.set_review_source(source); + } + // CS4: defer file loads to the event loop's input-idle window rather than blocking here (or + // on any later selection change) — `app.open_current()` below marks the initial open pending + // instead of loading eagerly; see `tui::run`'s doc comment for the resulting startup + // contract. + app.set_defer_loads(true); + + // Apply CS7's view-config settings BEFORE `open_current`: `App::apply_view_config`'s setters + // only set the raw layout/zoom/mode/width fields, and `open_current` is what derives + // `cursor`/`scroll` fresh from whichever settings just landed (see each setter's doc + // comment). + let view_config_warnings = app.apply_view_config(view_config); + app.open_current(); + + // A misconfigured keybinding or view-config setting is non-fatal: show the collected + // warnings as a startup notice (cleared on the first keypress, like any notice) and run with + // the defaults for those keys/settings. + let mut warnings = keymap.warnings().to_vec(); + warnings.extend(view_config_warnings); + if !warnings.is_empty() { + app.notify(warnings.join("; "), Severity::Error); + } + + app +} From 1322103f9fa7e616dcd063187863381852bb398c Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 02:26:35 -0400 Subject: [PATCH 11/16] feat(review): stream refresh diffs with span-keyed slot reuse --- git-workon-review/src/app.rs | 561 +++++++++++++++++++++++++++++++---- git-workon-review/src/tui.rs | 126 ++++++-- 2 files changed, 607 insertions(+), 80 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 6cfb5071..1f3d2c33 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -730,6 +730,15 @@ impl ChangesetView { } } + /// Whether this changeset's diff is real and ready (ADR-037's third slot state, named from + /// the other side) — [`App::refresh`]'s span-keyed reuse reads this to decide which existing + /// slots may be carried over wholesale. Deliberately excludes `Failed` (reuse only carries + /// `Ready` slots — `r` naturally retries a failed one instead, see the ADR's "Failures") and + /// `Pending` (nothing yet to reuse). + fn is_ready(&self) -> bool { + matches!(self.slot, ChangesetSlot::Ready) + } + /// Build the [`ChangesetView`] for `cs` from its acquired [`ChangesetDiff`] (see /// [`crate::acquire::diff_changeset`]) — the router from "how was this changeset diffed" to /// the uniform [`DiffState`] shape every [`ChangesetView`] carries. @@ -892,12 +901,22 @@ pub struct App { /// ([`Self::apply_file_ready`]) — the ONLY drop rule; within a generation, results are cached /// even if the user navigated away (warmth, not staleness — see the ADR's "Generations"). generation: u64, - /// Whether the startup wave (ADR-037's streamed launch) has already raised its one footer - /// notice for a `ChangesetReady { result: Err }`. Set by [`Self::apply_changeset_ready`], - /// never cleared in this slice (only ONE wave — startup — exists yet; the refresh path stays - /// synchronous, so nothing re-arms it). "the wave's first failure raises a footer notice" — - /// this is what makes it FIRST, not every one of a bad stack's failures. + /// Whether the CURRENT wave (startup's, or the most recent refresh's) has already raised its + /// one footer notice for a `ChangesetReady { result: Err }`. Set by + /// [`Self::apply_changeset_ready`]; reset to `false` by every [`Self::refresh`] right + /// alongside the generation bump, since a refresh dispatching a NEW wave (ADR-037's "Refresh" + /// changeset) starts that wave's own "first failure" count over — otherwise a stack whose + /// first-ever wave had one bad changeset would never notify again for a LATER, unrelated + /// failure. "the wave's first failure raises a footer notice" — this is what makes it FIRST + /// per wave, not every one of a bad stack's failures within it. wave_failure_notified: bool, + /// The ADR-037 refresh wave [`Self::refresh`] most recently queued (span-keyed reuse's + /// changed/new committed spans, stamped with the generation they belong to), if any — taken + /// (and cleared) by [`Self::take_pending_wave`]. Mirrors [`Self::open_pending`]/ + /// [`Self::take_pending_load_spec`]'s shape: `App` computes WHAT needs diffing but never + /// touches a thread or a `Repository`-carrying `Sender` itself, so it stays constructible (and + /// `refresh` stays synchronously testable) with nothing wired up to actually dispatch this. + pending_wave: Option<(u64, Vec<(usize, Changeset)>)>, } /// A destructive staging op deferred behind a [`Confirm`], identified by index into [`App::files`] @@ -1038,6 +1057,7 @@ impl App { open_pending_dispatched: false, generation: 1, wave_failure_notified: false, + pending_wave: None, }; // Position the outline cursor on the changeset/file the lib marked `current` (the same // row `sync_outline_to_current` would reposition to after any diff-initiated nav) rather @@ -1170,31 +1190,60 @@ impl App { } /// Re-run [`crate::acquire::resolve_changesets`] against the CURRENT `HEAD` branch and - /// rebuild every [`ChangesetView`] from scratch — the operation both a manual refresh (`r`) - /// and (later) a post-staging-op/external-write refresh need. Re-assembling (not just - /// re-diffing the active changeset) matters because a restack can change the stack's - /// topology, not just its diffs. + /// rebuild [`Self::changesets`] — the operation both a manual refresh (`r`) and the + /// post-staging-op/external-write refresh need. Re-assembling (not just re-diffing the + /// active changeset) matters because a restack can change the stack's topology, not just its + /// diffs. + /// + /// ADR-037 "Refresh" — span-keyed reuse, uncommitted always sync: + /// + /// - Resolve (this method's first half) stays fully synchronous on the main thread — it's + /// offline and cheap, and re-running it on every refresh is what keeps [`Self::review_source`] + /// honored (see below). + /// - The rebuilt view list carries over any existing `Ready` slot whose `(name, span)` is + /// unchanged — a committed diff is a pure function of its span + /// ([`ChangesetSpan::Committed`] compares `base`/`head`; [`ChangesetSpan::CommittedRoot`] + /// compares `head`) — so an ordinary post-staging refresh re-diffs *nothing but the + /// uncommitted layer*. A carried slot keeps its `DiffState` AND warm view caches verbatim: + /// never blanked, never re-diffed. Reuse only ever carries a `Ready` slot — a `Failed` one + /// goes back through the `Pending`+wave path below, which is how `r` naturally retries it + /// with no separate retry machinery. + /// - The [`ChangesetSpan::Uncommitted`] layer is never "unchanged": it re-diffs + /// SYNCHRONOUSLY, right here, on every refresh — ms-scale, and this is what preserves + /// staging's guarantee that the next keystroke sees the post-op world (an async refresh + /// would let a second `s` compute its patch against a stale diff). A failed sync re-diff + /// becomes a `Failed` slot plus a footer notice (an explicit error beats stale wrong + /// content) rather than aborting the whole refresh. + /// - Every other changed-or-new committed span becomes a `Pending` slot; the caller (the + /// event loop, via [`Self::take_pending_wave`]) dispatches those as an async wave, current- + /// first if the active changeset is among them, same as the streamed-launch wave. Their + /// results land through [`Self::apply_changeset_ready`] tagged with the NEW generation. + /// - Every refresh bumps the generation exactly once, right where the view caches it protects + /// are actually replaced — reused (carried) slots' in-flight loader results now carry a + /// stale `gen` and die at [`Self::apply_file_ready`]'s chokepoint; accepted waste for one + /// global rule (see the ADR's "Generations"). [`Self::wave_failure_notified`] resets + /// alongside it, so the freshly-dispatched wave gets its own first-failure notice. /// - /// - Rebuilds [`Self::changesets`] and [`Self::base_label`] in place. Does NOT touch `repo` - /// (same handle), `highlighter` (its per-instance grammar cache would have to re-parse - /// every language from scratch if rebuilt), `layout`, or `zoom` (the user's current view - /// mode shouldn't reset just because they pressed `r`, or because a background refresh - /// fired). + /// Position rules, adapted to the streamed world: /// - Preserves the active changeset by NAME: if a changeset with that name still exists in /// the rebuilt stack, `current_cs` follows it; otherwise it falls back to whichever /// changeset the lib now reports as `current`, or index `0`. - /// - Preserves file position by PATH within the (possibly different) active changeset, same - /// rule M4 used: `current` follows the path if it still exists, else clamps into the new - /// list (or `0` if empty). - /// - Re-seats the (possibly changed) current file at its first hunk via [`Self::open_current`] - /// — the same path a file switch already uses. This does NOT try to preserve the exact - /// cursor row: the rows under an old cursor position may no longer correspond to the same - /// content once the diff is rebuilt, so jumping to the first hunk (like opening a file fresh) - /// is the only always-valid choice, consistent with how zoom/layout switches already treat - /// cursor position as non-transferable across a reshape. + /// - A carried (still-`Ready`) active changeset — or the always-sync uncommitted layer — + /// preserves file position by PATH exactly like before streaming: `current` follows the + /// path if it still exists, else clamps into the new list (or `0` if empty), then + /// [`Self::open_current`] re-seats it at the first hunk (this does NOT try to preserve the + /// exact cursor row — jumping to the first hunk is the only always-valid choice once the + /// diff is rebuilt, consistent with zoom/layout switches). An active changeset that went + /// `Pending` instead has no diff yet to preserve a path INTO — `current` resets to `0` and + /// [`Self::apply_changeset_ready`] re-seats it (to its first file) exactly as it already + /// does for a freshly-`Pending` changeset, once that `ChangesetReady` lands. + /// - The rebuilt changeset list can resize/reorder the outline's row list out from under its + /// cursor — reposition it, same as every other diff-initiated nav (does NOT touch + /// `outline.open`/`focused`/`mode`, which persist across a refresh like `layout`/`zoom`). /// - /// On any assembly/diff error, leaves all existing state untouched and sets an error - /// [`Notice`] instead (via [`Self::notify`]) — a failed refresh must never blank the review. + /// On a resolve/assembly error (or the uncommitted layer's own sync re-diff failing — see + /// above), leaves the rest of `Self::changesets` untouched and sets an error [`Notice`] + /// instead (via [`Self::notify`]) — a failed refresh must never blank the review. /// /// Dispatches on [`Self::review_source`] (M7 CS2 fix): a no-argument launch (`None`) re-runs /// today's auto-detect ([`crate::acquire::resolve_changesets`]); an explicit-source launch @@ -1234,22 +1283,9 @@ impl App { return; } }; - - let diffs = match crate::acquire::diff_changesets(&self.repo, &changesets) { - Ok(diffs) => diffs, - Err(err) => { - self.notify(format!("refresh failed: {err}"), Severity::Error); - return; - } - }; - let views: Vec = changesets - .into_iter() - .zip(diffs) - .map(|(cs, diff)| ChangesetView::from_changeset_diff(cs, diff)) - .collect(); // `resolve_changesets` always returns at least one changeset (a lone Uncommitted entry // when no stack is active), but stay defensive rather than index an empty `Vec` below. - if views.is_empty() { + if changesets.is_empty() { self.notify("refresh failed: no changesets to review", Severity::Error); return; } @@ -1262,28 +1298,80 @@ impl App { .get(self.current) .map(|f| f.path.clone()); - self.current_cs = views + // Span-keyed reuse: pull the OLD view list out so a `Ready` slot whose `(name, span)` + // survives can be moved (not cloned) into the rebuilt list, keeping its warm view caches. + // `Vec::remove`'s O(n) shift is immaterial at stack sizes (a handful of changesets). + let mut old_views = std::mem::take(&mut self.changesets); + + let mut new_views: Vec = Vec::with_capacity(changesets.len()); + let mut to_diff: Vec<(usize, Changeset)> = Vec::new(); + let mut uncommitted_diff_failed: Option = None; + + for cs in changesets { + if cs.span == ChangesetSpan::Uncommitted { + match crate::acquire::diff_changeset(&self.repo, &cs) { + Ok(diff) => new_views.push(ChangesetView::from_changeset_diff(cs, diff)), + Err(err) => { + let message = err.to_string(); + uncommitted_diff_failed = Some(message.clone()); + new_views.push(ChangesetView::failed(cs, message)); + } + } + continue; + } + if let Some(pos) = old_views + .iter() + .position(|v| v.is_ready() && v.cs.name == cs.name && v.cs.span == cs.span) + { + // Carry the slot's diff/view caches verbatim, but adopt the FRESH descriptor — + // metadata like `needs_restack` can change even when the span itself didn't. + let mut reused = old_views.remove(pos); + reused.cs = cs; + new_views.push(reused); + } else { + let idx = new_views.len(); + to_diff.push((idx, cs.clone())); + new_views.push(ChangesetView::pending(cs)); + } + } + + self.current_cs = new_views .iter() .position(|v| v.cs.name == prev_cs_name) - .unwrap_or_else(|| current_cs_index(&views)); - self.base_label = base_label_for(&views[self.current_cs].cs); - self.changesets = views; + .unwrap_or_else(|| current_cs_index(&new_views)); + self.base_label = base_label_for(&new_views[self.current_cs].cs); + self.changesets = new_views; // ADR-037: every refresh bumps the generation, right where the view caches it protects - // are actually replaced — an early `return` above (a failed resolve/diff) leaves the old + // are actually replaced — an early `return` above (a failed resolve) leaves the old // world's caches intact, so it must NOT bump. Any loader result still in flight for the - // pre-refresh world now carries a stale `gen` and dies at `apply_file_ready`'s chokepoint. + // pre-refresh world now carries a stale `gen` and dies at `apply_file_ready`'s chokepoint; + // same for a wave result still in flight for a superseded generation. self.generation += 1; + self.wave_failure_notified = false; - let n = self.cur().diff.files.len(); - self.current = current_path - .and_then(|path| self.cur().diff.files.iter().position(|f| f.path == path)) - .unwrap_or(if n == 0 { 0 } else { self.current.min(n - 1) }); - + if self.cur().is_pending() { + self.current = 0; + } else { + let n = self.cur().diff.files.len(); + self.current = current_path + .and_then(|path| self.cur().diff.files.iter().position(|f| f.path == path)) + .unwrap_or(if n == 0 { 0 } else { self.current.min(n - 1) }); + } self.open_current(); - // The rebuilt changeset list can resize/reorder the outline's row list out from under - // its cursor — reposition it, same as every other diff-initiated nav (does NOT touch - // `outline.open`/`focused`/`mode`, which persist across a refresh like `layout`/`zoom`). self.sync_outline_to_current(); + + if let Some(err) = uncommitted_diff_failed { + self.notify( + format!("refresh failed: uncommitted diff failed: {err}"), + Severity::Error, + ); + } + + self.pending_wave = if to_diff.is_empty() { + None + } else { + Some((self.generation, to_diff)) + }; } /// Resolve the [`EffectiveZoom`] for file `idx` this frame: the requested [`Self::zoom`] gated @@ -1560,7 +1648,13 @@ impl App { /// navigation of all, and it must render immediately. Outside defer mode this is exactly /// the pre-defer eager behavior. pub fn open_current(&mut self) { - if self.defer_loads && !self.current_views_cached() { + // An empty file list (a `Pending`/`Failed` slot, ADR-037, or a genuinely empty committed + // changeset) has nothing to defer: `current_load_spec` indexes `diff.files[self.current]` + // unconditionally, which would panic on the loader-dispatch path if `open_pending` were + // set here for a file that doesn't exist. There's nothing to load either way — this is + // the same "no-op on an empty file list" contract `main.rs`'s streamed-launch comment + // documents for a fresh `Pending` changeset, made actually true rather than incidental. + if self.defer_loads && !self.files().is_empty() && !self.current_views_cached() { self.open_pending = true; // A fresh pending open has nothing dispatched to the loader yet — see // [`Self::take_pending_load_spec`]. @@ -1662,6 +1756,19 @@ impl App { Some((self.generation, self.current_cs, self.current, spec)) } + /// Take the ADR-037 refresh wave [`Self::refresh`] most recently queued (span-keyed reuse's + /// changed/new committed spans), if any — `None` when the last refresh had nothing left to + /// diff asynchronously (every span was reused or is the always-sync uncommitted layer; this + /// is what keeps a single-uncommitted-changeset session's refresh effectively synchronous, + /// see the ADR's "Refresh"). Mirrors [`Self::take_pending_load_spec`]'s take-once shape: the + /// caller (the event loop — the only place with thread-spawning ability) is responsible for + /// actually dispatching it; `App` never touches a `Sender`/`Repository`-carrying handle + /// itself, so it stays constructible — and `refresh` stays synchronously testable — with + /// nothing wired up to consume this at all. + pub fn take_pending_wave(&mut self) -> Option<(u64, Vec<(usize, Changeset)>)> { + self.pending_wave.take() + } + /// Apply one loader result (ADR-037's chokepoint, the `FileReady` inbox arm routes here): /// dropped outright on a generation mismatch (`gen != self.generation` — the world it was /// computed against no longer exists, see [`Self::generation`]'s doc comment). Otherwise: @@ -4752,6 +4859,352 @@ mod tests { ); } + // ---- ADR-037 refresh: span-keyed reuse, uncommitted always sync, async waves ---------- + + /// Build a two-commit chain (`root` then `head`) on the fixture's default branch and return + /// both `Oid`s — the shared setup every span-keyed-reuse test below diffs a + /// [`ChangesetSpan::Committed`] across. + fn root_and_head_commits(fixture: &Fixture) -> (git2::Oid, git2::Oid) { + let root = fixture + .commit("main") + .file("r.txt", "r\n") + .create("root") + .unwrap(); + let head = fixture + .commit("main") + .file("a.txt", "a\n") + .file("b.txt", "b\n") + .create("head") + .unwrap(); + (root, head) + } + + #[test] + fn refresh_reuses_a_ready_committed_slot_with_unchanged_span_keeping_warm_caches() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let (root, head) = root_and_head_commits(&fixture); + let repo = fixture.repo().unwrap(); + + // `head_text: "main"` re-resolves through the branch ref on every refresh — the span + // stays `Committed { base: root, head }` as long as `main` doesn't move, exercising the + // REAL re-resolve path (not a hand-frozen span) for the "unchanged" case. + let cs = Changeset { + name: format!("{root}..main"), + span: ChangesetSpan::Committed { base: root, head }, + title: None, + current: true, + needs_restack: false, + }; + let view = ChangesetView::from_changeset_diff( + cs.clone(), + crate::acquire::diff_changeset(repo, &cs).unwrap(), + ); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.set_review_source(crate::source::Source::Range { + base_text: root.to_string(), + head_text: "main".to_string(), + dots: crate::source::RangeDots::Two, + }); + + app.open_current(); // caches file 0 ("a.txt") + assert_eq!(app.files().len(), 2, "root..head touches a.txt and b.txt"); + app.next_file(); // caches file 1 ("b.txt") too + app.prev_file(); // back to file 0 — the file `refresh`'s tail will re-seat + assert!(app.role_view_ref(1, Role::Combined).is_some()); + + let gen_before = app.generation(); + app.refresh(); + + assert_eq!( + app.generation(), + gen_before + 1, + "every refresh bumps the generation, reused slot or not" + ); + assert!( + !app.is_current_pending(), + "an unchanged span must be carried over Ready, never go through Pending" + ); + assert_eq!(app.files().len(), 2); + assert_eq!(app.current, 0, "file position by path is preserved"); + assert!( + app.role_view_ref(1, Role::Combined).is_some(), + "file 1's view cache must survive untouched — refresh's tail only (re)opens the \ + CURRENT file (0), so a still-populated cache at 1 proves the whole ChangesetView \ + (not just its diff) was carried over rather than rebuilt fresh" + ); + assert!( + app.take_pending_wave().is_none(), + "a fully-reused refresh has nothing left to diff asynchronously" + ); + } + + #[test] + fn refresh_sends_a_changed_committed_span_through_a_wave_instead_of_reusing_it() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let (root, old_head) = root_and_head_commits(&fixture); + let repo = fixture.repo().unwrap(); + + let cs = Changeset { + name: format!("{root}..main"), + span: ChangesetSpan::Committed { + base: root, + head: old_head, + }, + title: None, + current: true, + needs_restack: false, + }; + let view = ChangesetView::from_changeset_diff( + cs.clone(), + crate::acquire::diff_changeset(repo, &cs).unwrap(), + ); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.set_review_source(crate::source::Source::Range { + base_text: root.to_string(), + head_text: "main".to_string(), + dots: crate::source::RangeDots::Two, + }); + app.open_current(); + + // Advance `main` past `old_head` — the same shape as a real amend/restack: the name + // ("{root}..main") stays identical, but the span's `head` moves. + let new_head = fixture + .commit("main") + .file("c.txt", "c\n") + .create("new head") + .unwrap(); + + let gen_before = app.generation(); + app.refresh(); + let gen_after = app.generation(); + assert_eq!(gen_after, gen_before + 1); + + assert!( + app.is_current_pending(), + "a changed span must NOT be reused — it goes Pending for the wave to diff" + ); + assert!(app.files().is_empty()); + + let (wave_gen, to_diff) = app + .take_pending_wave() + .expect("a changed committed span must queue a wave request"); + assert_eq!(wave_gen, gen_after); + assert_eq!(to_diff.len(), 1); + assert_eq!( + to_diff[0].0, 0, + "the stack index the result must be seated at" + ); + assert_eq!( + to_diff[0].1.span, + ChangesetSpan::Committed { + base: root, + head: new_head, + }, + "the wave must diff the NEW span, not the stale one" + ); + assert!( + app.take_pending_wave().is_none(), + "take_pending_wave is a take-once — a second call must find nothing left" + ); + + // A stale-generation result (as if it were still in flight for the pre-refresh world) + // must be dropped outright. + app.apply_changeset_ready( + gen_before, + 0, + Ok(crate::acquire::ChangesetDiff::Committed( + crate::acquire::diff_committed(repo, root, new_head).unwrap(), + )), + ); + assert!( + app.is_current_pending(), + "a stale-generation ChangesetReady must be dropped, not seat the changeset" + ); + + // The NEW generation's result lands and seats the (still active) changeset. + app.apply_changeset_ready( + gen_after, + 0, + Ok(crate::acquire::ChangesetDiff::Committed( + crate::acquire::diff_committed(repo, root, new_head).unwrap(), + )), + ); + assert!(!app.is_current_pending()); + assert_eq!( + app.files().len(), + 3, + "root..new_head accumulates a.txt/b.txt (from `head`) and c.txt (from `new_head`)" + ); + assert_eq!( + app.cur().cs.span, + ChangesetSpan::Committed { + base: root, + head: new_head, + } + ); + } + + #[test] + fn refresh_retries_a_failed_committed_slot_via_a_wave_rather_than_reusing_it() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let (root, head) = root_and_head_commits(&fixture); + let repo = fixture.repo().unwrap(); + + let cs = Changeset { + name: format!("{root}..main"), + span: ChangesetSpan::Committed { base: root, head }, + title: None, + current: true, + needs_restack: false, + }; + // Seed the slot as `Failed` for this exact (name, span) — as if a previous wave's diff + // for it had errored. + let view = ChangesetView::failed(cs, "a previous diff attempt failed"); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.set_review_source(crate::source::Source::Range { + base_text: root.to_string(), + head_text: "main".to_string(), + dots: crate::source::RangeDots::Two, + }); + assert!(app.current_failure().is_some()); + + app.refresh(); // span is UNCHANGED from the Failed slot's — reuse must still skip it + + assert!( + app.is_current_pending(), + "reuse only carries `Ready` slots — a `Failed` one goes back through Pending+wave, \ + which is what makes `r` a retry with no separate retry machinery" + ); + let (_, to_diff) = app + .take_pending_wave() + .expect("the retried span must be queued for the wave"); + assert_eq!(to_diff.len(), 1); + assert_eq!( + to_diff[0].1.span, + ChangesetSpan::Committed { base: root, head } + ); + } + + #[test] + fn coordinated_refresh_after_staging_rebuilds_the_uncommitted_layer_synchronously_while_reusing_an_unchanged_committed_span( + ) { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .unstaged_file("dirty.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + repo.set_head("refs/heads/a").unwrap(); + repo.checkout_head(None).unwrap(); + + let changesets = crate::acquire::resolve_changesets(repo, "a").unwrap(); + assert_eq!( + changesets.len(), + 2, + "expected the 'a' Graphite node plus the dirty tree's uncommitted layer" + ); + let diffs = crate::acquire::diff_changesets(repo, &changesets).unwrap(); + let views: Vec = changesets + .into_iter() + .zip(diffs) + .map(|(cs, diff)| ChangesetView::from_changeset_diff(cs, diff)) + .collect(); + + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, views); + assert_eq!( + app.cur().cs.span, + ChangesetSpan::Uncommitted, + "opens on the uncommitted layer (lib-`current`)" + ); + + app.open_current(); // cursor lands on dirty.txt's one hunk + app.stage_hunk(); // run_op -> coordinated_refresh -> refresh, synchronously + + // The post-op world is visible SYNCHRONOUSLY, before the next event loop iteration. + repo.assert(predicate::repo::has_staged_file("dirty.txt")); + assert!( + !app.is_current_pending(), + "the uncommitted layer always re-diffs sync — it must never go through Pending" + ); + assert!( + app.take_pending_wave().is_none(), + "the 'a' node's committed span didn't change — staging must not have dispatched a \ + wave for it" + ); + } + + #[test] + fn refresh_marks_the_uncommitted_layer_failed_when_its_sync_diff_errors() { + use super::Severity; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_review_source(crate::source::Source::Uncommitted); + // Deliberately do NOT call `app.open_current()` here: reading a file's content already + // walks `HEAD`'s commit/tree chain through `app`'s OWN `Repository` handle, which would + // warm libgit2's per-handle object cache for the exact commit this test corrupts below — + // a cache hit would silently mask the corruption instead of exercising the failure path. + + // Corrupt the loose object `HEAD` points to (not the `HEAD` ref itself): `repo.head()`'s + // SHORTHAND still resolves fine (refresh's own branch-name read, and + // `Source::Uncommitted`'s resolution, which is a pure string wrap needing no repo access + // at all), but `diff_uncommitted`'s `repo.head()?.peel_to_tree()` — which walks all the + // way to the commit object, through `app`'s never-yet-used-for-this-object handle — fails. + let repo = fixture.repo().unwrap(); + let head_oid = repo.head().unwrap().target().unwrap(); + let hex = head_oid.to_string(); + let object_path = repo.path().join("objects").join(&hex[0..2]).join(&hex[2..]); + // Loose objects are written read-only by git — reclaim write permission before + // clobbering the bytes, or the write itself fails with EACCES. + let mut perms = std::fs::metadata(&object_path).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(false); + std::fs::set_permissions(&object_path, perms).unwrap(); + std::fs::write(&object_path, b"garbage-not-a-git-object\n").unwrap(); + + app.refresh(); + + assert!( + !app.is_current_pending(), + "a sync diff failure sets Failed, not Pending — nothing async is retrying this" + ); + assert!( + app.current_failure().is_some(), + "the uncommitted layer's failed sync re-diff must become a Failed slot" + ); + let notice = app + .notice + .as_ref() + .expect("a failed uncommitted sync re-diff must set a footer notice"); + assert_eq!(notice.severity, Severity::Error); + assert!( + notice.text.contains("uncommitted diff failed"), + "got notice text: {:?}", + notice.text + ); + assert!(app.take_pending_wave().is_none()); + } + // ---- M4 index watcher (`on_tick`) ------------------------------------------------------- /// Stage `path` in the fixture's index, exactly as an external `git add` would — the write diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index 51761df2..dcb9f229 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -18,7 +18,7 @@ use std::fs::File; use std::io::{self, Write}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::mpsc; use std::thread; use std::time::Duration; @@ -229,13 +229,23 @@ fn spawn_loader_thread( req_tx } -/// Spawn the ADR-037 startup wave: stripe `changesets` (lib-`current` first, then input order) -/// across `available_parallelism`-many transient WORKER threads — same fan-out shape as +/// Spawn a ADR-037 diff wave — the startup wave over the whole resolved stack, or (ADR-037 +/// "Refresh") a refresh's span-keyed reuse leftovers, the changed/new committed spans +/// [`workon_review::app::App::take_pending_wave`] queued. Stripes `to_diff` (`current_idx`-first +/// if the active changeset is among the pairs being diffed, then input order) across +/// `available_parallelism`-many transient WORKER threads — same fan-out shape as /// `crate::acquire::diff_changesets` (each worker opens its own `Repository`, since /// `git2::Repository` is `Send` but not `Sync`) — but STREAM each result the instant it completes /// via `tx` rather than joining the batch. Never joined itself either — a wave straggler left -/// running past quit is harmless (it only ever sends into an inbox nothing is listening to -/// anymore; `tx.send` failing is the signal each worker already checks). +/// running past quit (or superseded by a later refresh's generation) is harmless: it only ever +/// sends into an inbox nothing is listening to anymore, or a result [`App::apply_changeset_ready`] +/// drops outright on a generation mismatch; `tx.send` failing is the signal each worker already +/// checks for the former. +/// +/// `to_diff`'s `usize` is the pair's index into `App`'s FULL changeset stack (not a position +/// within `to_diff` itself) — carried straight through to each `ChangesetReady { idx, .. }` so +/// [`App::apply_changeset_ready`] can seat the result without `App` and this wave ever agreeing +/// on a separate numbering. /// /// A DELIBERATELY separate set of threads from the loader thread (ADR-037 leaves this shape /// open — "yours to shape"): the wave never touches the loader's request queue, so an in-flight @@ -252,20 +262,22 @@ fn spawn_loader_thread( fn spawn_wave_thread( repo_path: PathBuf, tx: mpsc::Sender, - changesets: Vec, + to_diff: Vec<(usize, Changeset)>, gen: u64, + current_idx: Option, ) { thread::spawn(move || { - let n = changesets.len(); + let n = to_diff.len(); if n == 0 { return; } - // Current changeset first, then input order for the rest — the changeset the user lands - // on becomes interactive earliest (ADR-037's "Slots"). - let current_idx = changesets.iter().position(|cs| cs.current); + // The active changeset first (if it's among these pairs at all), then input order for + // the rest — the changeset the user lands on becomes interactive earliest (ADR-037's + // "Slots"). `current_pos` is a position WITHIN `to_diff`, not the stack index itself. + let current_pos = current_idx.and_then(|ci| to_diff.iter().position(|(idx, _)| *idx == ci)); let mut order: Vec = Vec::with_capacity(n); - order.extend(current_idx); - order.extend((0..n).filter(|&i| Some(i) != current_idx)); + order.extend(current_pos); + order.extend((0..n).filter(|&i| Some(i) != current_pos)); let workers = thread::available_parallelism() .map(std::num::NonZeroUsize::get) @@ -274,20 +286,21 @@ fn spawn_wave_thread( let chunk = n.div_ceil(workers.max(1)); thread::scope(|scope| { - for idx_chunk in order.chunks(chunk) { + for pos_chunk in order.chunks(chunk) { let tx = tx.clone(); - let changesets = &changesets; + let to_diff = &to_diff; let repo_path = &repo_path; scope.spawn(move || { let repo = match Repository::open(repo_path) { Ok(repo) => repo, Err(err) => { let message = err.to_string(); - for &idx in idx_chunk { + for &pos in pos_chunk { + let (idx, _) = &to_diff[pos]; if tx .send(Ok(AppEvent::ChangesetReady { gen, - idx, + idx: *idx, result: Err(message.clone()), })) .is_err() @@ -298,11 +311,15 @@ fn spawn_wave_thread( return; } }; - for &idx in idx_chunk { - let result = - diff_changeset(&repo, &changesets[idx]).map_err(|e| e.to_string()); + for &pos in pos_chunk { + let (idx, cs) = &to_diff[pos]; + let result = diff_changeset(&repo, cs).map_err(|e| e.to_string()); if tx - .send(Ok(AppEvent::ChangesetReady { gen, idx, result })) + .send(Ok(AppEvent::ChangesetReady { + gen, + idx: *idx, + result, + })) .is_err() { return; // main loop is gone; nothing left to forward to @@ -314,6 +331,20 @@ fn spawn_wave_thread( }); } +/// The ADR-037 pipeline handles [`event_loop`] needs to dispatch off-thread work — bundled into +/// one struct (rather than four separate parameters) so `event_loop` stays under clippy's +/// `too_many_arguments`. `inbox` is the single shared receiver; `load_tx`/`wave_tx` dispatch to +/// the loader thread and a fresh diff-wave thread respectively; `repo_path` is what any +/// newly-spawned wave thread opens its own `Repository` handle against (a refresh can queue a +/// wave well after startup, so this is kept around for the whole loop, not just its setup). +#[derive(Clone, Copy)] +struct Pipeline<'a> { + inbox: &'a mpsc::Receiver, + load_tx: &'a mpsc::Sender, + wave_tx: &'a mpsc::Sender, + repo_path: &'a Path, +} + /// Receive the next event from `inbox`, waiting up to `timeout`. A timeout with nothing received /// yields `Ok(AppEvent::Tick)` — the loop's regular redraw beat, and the mechanism the M4 index /// watcher polls on (see the module doc). A disconnected inbox (the input thread panicked, or @@ -885,8 +916,14 @@ impl Tui { ) -> io::Result<()> { let (tx, rx) = mpsc::channel::(); spawn_input_thread(tx.clone()); - let load_tx = spawn_loader_thread(repo_path, tx); - let result = event_loop(&mut self.terminal, app, keymap, theme, &rx, &load_tx); + let load_tx = spawn_loader_thread(repo_path.clone(), tx.clone()); + let pipeline = Pipeline { + inbox: &rx, + load_tx: &load_tx, + wave_tx: &tx, + repo_path: &repo_path, + }; + let result = event_loop(&mut self.terminal, app, keymap, theme, &pipeline); let restored = self.restore(); result.and(restored) } @@ -912,8 +949,24 @@ impl Tui { let (tx, rx) = mpsc::channel::(); spawn_input_thread(tx.clone()); let load_tx = spawn_loader_thread(repo_path.clone(), tx.clone()); - spawn_wave_thread(repo_path, tx, changesets, app.generation()); - let result = event_loop(&mut self.terminal, app, keymap, theme, &rx, &load_tx); + // `App::from_changesets` (which built `app`'s all-`Pending` slots) picked `current_cs` + // via the same lib-`current` lookup this enumeration mirrors, so `app.current_cs()` IS + // that changeset's index into `to_diff` here — no separate lookup needed. + let to_diff: Vec<(usize, Changeset)> = changesets.into_iter().enumerate().collect(); + spawn_wave_thread( + repo_path.clone(), + tx.clone(), + to_diff, + app.generation(), + Some(app.current_cs()), + ); + let pipeline = Pipeline { + inbox: &rx, + load_tx: &load_tx, + wave_tx: &tx, + repo_path: &repo_path, + }; + let result = event_loop(&mut self.terminal, app, keymap, theme, &pipeline); let restored = self.restore(); result.and(restored) } @@ -961,9 +1014,14 @@ fn event_loop( app: &mut App, keymap: &Keymap, theme: &Palette, - inbox: &mpsc::Receiver, - load_tx: &mpsc::Sender, + pipeline: &Pipeline<'_>, ) -> io::Result<()> { + let Pipeline { + inbox, + load_tx, + wave_tx, + repo_path, + } = *pipeline; let mut pending: Vec = Vec::new(); let mut quit = false; @@ -1006,6 +1064,22 @@ fn event_loop( let mut batch = vec![event]; drain_pending(inbox, &mut batch)?; quit = update_batch(app, keymap, &mut pending, batch); + + // ADR-037 "Refresh": every refresh trigger (`r`, the on-tick index watcher, a + // post-staging drain) runs through `App::refresh` somewhere inside the `update_batch` + // call above, however deeply nested — `App` itself never touches a thread, so it just + // queues the span-keyed-reuse leftovers on `Self::pending_wave` for whoever's holding the + // thread-spawning ability to pick up. This ONE checkpoint, run after every batch, is that + // pickup: it covers every refresh trigger uniformly with no per-trigger wiring. + if let Some((gen, to_diff)) = app.take_pending_wave() { + spawn_wave_thread( + repo_path.to_path_buf(), + wave_tx.clone(), + to_diff, + gen, + Some(app.current_cs()), + ); + } } } From 133563fc8897db29efd6e5df76c764a906c34bd6 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:13:56 -0400 Subject: [PATCH 12/16] fix(review): clear stale deferred-open flags on non-deferred opens --- git-workon-review/src/app.rs | 77 ++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 1f3d2c33..502675e2 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -1664,6 +1664,15 @@ impl App { } self.ensure_loaded(self.current); self.reset_panes(); + // F1: an eager load or the empty-file no-op above supersedes any STALE deferred open — + // e.g. a pending open set before `r`, followed by a refresh that turns the active + // changeset `Pending` (empty files, skipping the defer branch above since there's + // nothing to load). Without this, the stale flags survive the refresh and wedge the + // idle-Tick fast-poll loop (`take_pending_load_spec` keeps returning `None` for a file + // that no longer exists) while the placeholder stays stuck. Mirrors + // [`Self::complete_pending_open`]'s tail. + self.open_pending = false; + self.open_pending_dispatched = false; } /// Whether the view(s) the current file's effective zoom needs are already cached, making a @@ -5052,6 +5061,74 @@ mod tests { ); } + #[test] + fn refresh_that_makes_the_active_changeset_pending_clears_stale_deferred_open_flags() { + // F1 regression: a pending open still in flight (dispatched, awaiting a loader result) + // right before `r`, followed by a refresh that turns the active changeset Pending (a + // changed span goes through the wave, landing empty files) must not leave + // `open_pending`/`open_pending_dispatched` wedged. `open_current`'s empty-file guard + // skips the defer branch (nothing to load) after the refresh, so nothing else would + // clear stale flags without this fix. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let (root, old_head) = root_and_head_commits(&fixture); + let repo = fixture.repo().unwrap(); + + let cs = Changeset { + name: format!("{root}..main"), + span: ChangesetSpan::Committed { + base: root, + head: old_head, + }, + title: None, + current: true, + needs_restack: false, + }; + let view = ChangesetView::from_changeset_diff( + cs.clone(), + crate::acquire::diff_changeset(repo, &cs).unwrap(), + ); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.set_review_source(crate::source::Source::Range { + base_text: root.to_string(), + head_text: "main".to_string(), + dots: crate::source::RangeDots::Two, + }); + app.set_defer_loads(true); + app.open_current(); + let _ = app + .take_pending_load_spec() + .expect("a fresh pending open dispatches"); + assert!(app.open_pending(), "the open is pending, awaiting a result"); + + // Advance `main`, changing the span — refresh must send this through the wave, landing + // the active changeset Pending (empty files) rather than reusing the stale slot. + fixture + .commit("main") + .file("c.txt", "c\n") + .create("new head") + .unwrap(); + + app.refresh(); + + assert!( + app.is_current_pending(), + "a changed span must go Pending for the wave" + ); + assert!(app.files().is_empty()); + assert!( + !app.open_pending(), + "a stale deferred open must not survive a refresh that empties the active changeset" + ); + assert!( + app.take_pending_load_spec().is_none(), + "no load spec can be produced for a Pending slot with no files" + ); + } + #[test] fn refresh_retries_a_failed_committed_slot_via_a_wave_rather_than_reusing_it() { let fixture = FixtureBuilder::new() From a62937ba3d6e41ddfde1607eac20a07253f67653 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:44:14 -0400 Subject: [PATCH 13/16] test(review): fileless open pins cleared flags after flag hygiene --- git-workon-review/src/app.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 502675e2..1bbe99d8 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -3758,9 +3758,12 @@ mod tests { #[test] fn take_pending_load_spec_is_none_for_a_fileless_changeset_without_panicking() { - // A clean uncommitted layer diffs to zero files. A pending open onto it (e.g. one that - // outraces a refresh, or the Pending/Failed slots ADR-037's later changesets introduce) - // must not panic `current_load_spec`'s file-list indexing — F7's regression. + // A clean uncommitted layer diffs to zero files. A pending open onto it must not panic + // `current_load_spec`'s file-list indexing — F7's regression. Where this test was born + // (the loader changeset), a fileless `open_current` still MARKED the open pending and + // only the spec-building was total; this changeset's flag hygiene supersedes that — + // a fileless open now never marks (and actively clears) `open_pending`, so both the + // flag and the spec must come back empty. let fixture = FixtureBuilder::new() .config("core.autocrlf", "false") .build() @@ -3770,7 +3773,10 @@ mod tests { assert!(app.files().is_empty(), "fixture must have no diffed files"); app.set_defer_loads(true); app.open_current(); - assert!(app.open_pending(), "empty-file open still marks pending"); + assert!( + !app.open_pending(), + "a fileless open never marks pending (the non-deferred path clears the flags)" + ); assert!( app.take_pending_load_spec().is_none(), From df06bc28bea1b722521a91fba24e9f4877c5e046 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 03:31:45 -0400 Subject: [PATCH 14/16] test(review): real-thread pipeline smoke and streamed-launch oracle --- git-workon-review/src/tui.rs | 194 ++++++++++++++++++ git-workon-review/tests/pty_responsiveness.rs | 177 +++++++++++++++- 2 files changed, 369 insertions(+), 2 deletions(-) diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index dcb9f229..c893b286 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -2599,4 +2599,198 @@ mod tests { "splash frame must show the launch-activity message, got: {top_row:?}" ); } + + // ── ADR-037: real-thread integration smoke ───────────────────────────────── + + /// ADR-037's Testing layer 3, part one: the ONE test in this module that spawns the REAL + /// [`spawn_loader_thread`]/[`spawn_wave_thread`] against real `mpsc` channels — everything + /// else in this file drives `update`/`update_batch` with synthetic events specifically to + /// avoid real threads (see the ADR's "Testing" decision: layers 1-2 are thread-free by + /// design). This is the one exception, confined here. + /// + /// Mirrors `main.rs`'s streamed-launch shape exactly: `App::from_changesets` over + /// all-`Pending` slots, `set_defer_loads(true)`, `open_current()`, then the same + /// `spawn_wave_thread` call `Tui::run_streamed` makes. From there this test plays the event + /// loop's OWN role by hand — draining the shared inbox and routing `ChangesetReady`/ + /// `FileReady` through the exact chokepoints `update`'s match arms call + /// (`App::apply_changeset_ready`/`App::apply_file_ready`), plus the same post-batch + /// `take_pending_load_spec` dispatch `event_loop` runs on every idle tick while an open is + /// pending — except here it's driven the instant the active changeset seats, not gated behind + /// a real debounce `Tick`, since nothing here is racing real terminal input. + /// + /// Bounded by `recv_timeout` per receive (not a wall-clock test deadline): a wedged thread + /// times out and fails loudly rather than hanging the suite, but a healthy run's actual + /// duration is however long the real diff/load work takes — no sleeping, no fixed budget, so + /// this stays load-tolerant enough to run unconditionally (unlike `pty_responsiveness.rs`'s + /// `#[ignore]` siblings, which assert actual elapsed wall-clock time). + #[test] + fn real_threads_stream_a_wave_and_complete_a_deferred_file_open() { + use git_workon_fixture::prelude::*; + use workon::{Changeset, ChangesetSpan}; + use workon_review::app::ChangesetView; + + // A 4-changeset committed stack, each adding one file — the streamed-launch "multi- + // changeset fixture stack" the plan calls for, deep enough that the wave has real + // fan-out work (`spawn_wave_thread` stripes across `available_parallelism` workers) and + // that the ACTIVE (last, `current: true`) changeset has a real, uncached file for the + // deferred-open assertion. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let root = fixture + .commit("main") + .file("root.txt", "r\n") + .create("root") + .unwrap(); + let c1 = fixture + .commit("main") + .file("a.txt", "a\n") + .create("c1") + .unwrap(); + let c2 = fixture + .commit("main") + .file("b.txt", "b\n") + .create("c2") + .unwrap(); + let c3 = fixture + .commit("main") + .file("c.txt", "c\n") + .create("c3") + .unwrap(); + let c4 = fixture + .commit("main") + .file("d.txt", "d\n") + .create("c4") + .unwrap(); + + let bare = |name: &str, base, head, current| Changeset { + name: name.to_string(), + span: ChangesetSpan::Committed { base, head }, + title: None, + current, + needs_restack: false, + }; + let changesets = vec![ + bare("cs-1", root, c1, false), + bare("cs-2", c1, c2, false), + bare("cs-3", c2, c3, false), + bare("cs-4", c3, c4, true), + ]; + + let repo = fixture.repo().unwrap(); + let repo_path = repo.workdir().unwrap().to_path_buf(); + let owned = Repository::open(&repo_path).unwrap(); + + let pending_views: Vec = changesets + .iter() + .cloned() + .map(ChangesetView::pending) + .collect(); + let mut app = App::from_changesets(owned, pending_views); + app.set_defer_loads(true); + app.open_current(); + + let active_idx = app.current_cs(); + assert_eq!( + active_idx, 3, + "the lib-current changeset (cs-4) opens active" + ); + assert!(app.is_current_pending(), "every slot starts Pending"); + + let (tx, rx) = mpsc::channel::(); + let load_tx = spawn_loader_thread(repo_path.clone(), tx.clone()); + let to_diff: Vec<(usize, Changeset)> = changesets.into_iter().enumerate().collect(); + spawn_wave_thread( + repo_path.clone(), + tx.clone(), + to_diff, + app.generation(), + Some(active_idx), + ); + drop(tx); // this test's only senders now are the two spawned threads + + let deadline_per_recv = Duration::from_secs(15); + let mut changesets_ready = vec![false; app.changeset_count()]; + let mut active_file_loaded = false; + let mut dispatched_active_load = false; + + loop { + if changesets_ready.iter().all(|&r| r) && active_file_loaded { + break; + } + let event = rx + .recv_timeout(deadline_per_recv) + .expect("loader/wave thread must answer within the deadline") + .expect("neither real thread should forward a read error in this test"); + + match event { + AppEvent::ChangesetReady { gen, idx, result } => { + assert!( + result.is_ok(), + "a committed changeset diff must not fail here" + ); + app.apply_changeset_ready(gen, idx, result); + changesets_ready[idx] = true; + } + AppEvent::FileReady { + gen, + cs_idx, + file_idx, + result, + } => { + assert!(result.is_ok(), "a real file load must not fail here"); + app.apply_file_ready(gen, cs_idx, file_idx, result); + if cs_idx == active_idx && file_idx == 0 { + active_file_loaded = true; + } + } + other => panic!("unexpected event in the real-thread smoke: {other:?}"), + } + + // The same post-batch checkpoint `event_loop` runs on every idle `Tick` while an + // open is pending — dispatched here the instant it's possible (right after the + // active changeset seats) rather than gated behind a real debounce, since nothing in + // this test races real terminal input. + if !dispatched_active_load && app.open_pending() { + if let Some((gen, cs_idx, file_idx, spec)) = app.take_pending_load_spec() { + load_tx + .send(LoadRequest { + gen, + cs_idx, + file_idx, + spec, + }) + .expect("loader thread must still be alive to receive the dispatch"); + dispatched_active_load = true; + } + } + } + + assert!( + changesets_ready.iter().all(|&r| r), + "every slot must land Ready: {changesets_ready:?}" + ); + assert!( + !app.is_current_pending(), + "the active changeset must be seated once its wave result lands" + ); + assert_eq!( + app.current_cs(), + active_idx, + "seating must not move which changeset is active" + ); + assert!( + active_file_loaded, + "the active changeset's deferred file open must complete via a real FileReady" + ); + assert!( + !app.open_pending(), + "a completed FileReady must clear the pending-open flag" + ); + assert!( + app.current_view_ref().is_some(), + "the active file's view must be cached after its FileReady lands" + ); + } } diff --git a/git-workon-review/tests/pty_responsiveness.rs b/git-workon-review/tests/pty_responsiveness.rs index a1b0fcdf..134d13b1 100644 --- a/git-workon-review/tests/pty_responsiveness.rs +++ b/git-workon-review/tests/pty_responsiveness.rs @@ -1,5 +1,6 @@ -//! PTY responsiveness smoke tests for the 2026-07 performance pass — the launch path and the -//! rapid-outline-nav path, driven against the real binary in a pseudo-terminal. +//! PTY responsiveness smoke tests for the 2026-07 performance pass — the launch path, the +//! rapid-outline-nav path, and (ADR-037) the streamed-startup path, driven against the real +//! binary in a pseudo-terminal. //! //! These guard the *regression classes* that pass fixed, not the milliseconds it measured: //! @@ -13,6 +14,11 @@ //! burst→quit; if input coalescing (`update_batch`) or idle-deferred loads //! (`open_pending`/`OPEN_DEBOUNCE`) regress, the quit waits behind the sum of every //! intermediate file's load and blows the bound. +//! - **Streamed startup (ADR-037):** before the progressive pipeline, a multi-changeset launch +//! diffed the WHOLE stack sequentially-then-in-parallel before the first frame ever drew — +//! the wait scaled with stack depth. The streamed-startup test bounds spawn→quit on a deep +//! stack so a reintroduced "wait for the full wave" launch fails loudly; see that test's doc +//! comment for the measured before/after numbers that sized its bound. //! //! The bounds are deliberately blunt (seconds, not milliseconds): absolute wall-clock //! assertions flake under parallel CPU load, exactly like git-workon's @@ -63,6 +69,37 @@ const BURST_FILES: usize = 36; /// Lines per generated fixture file — see `BURST_FILES`. const BURST_FILE_LINES: usize = 2_000; +/// Upper bound on spawn→quit for `streamed_startup_lands_before_a_full_wave_could_have_finished` +/// (ADR-037). Sized from real measurements on this fixture (debug build, `q` sent right after +/// alternate-screen entry — same protocol as `LAUNCH_RESPONSIVE`, so `q` buffers in the PTY +/// until the event loop actually starts polling input): +/// +/// - The CURRENT (streamed) binary: ~1.1-1.4s — dominated by fixed per-launch costs +/// (`gt --version`'s ~350ms subprocess spawn, checking out `STREAMED_STACK_SIZE` branches' +/// worth of files) plus ONE changeset's diff (the active one, streamed first). +/// - The PRE-ADR-037 binary (commit `2214558`, built and run against the identical fixture): +/// ~6.4s — it blocks on the full stack's diff wave before the event loop ever starts, so `q` +/// sits in the PTY the whole time. +/// +/// This bound sits roughly 2× above the healthy number and comfortably (~2×) below the +/// regressed one — the same blunt, load-tolerant margin philosophy as `LAUNCH_RESPONSIVE`/ +/// `BURST_RESPONSIVE`, just re-measured for this fixture rather than reused, since the streamed +/// path's fixed costs (checkout of a much deeper stack) differ from the single-changeset launch +/// test's. +const STREAMED_STARTUP_RESPONSIVE: Duration = Duration::from_secs(3); + +/// Depth of the Graphite stack `streamed_startup_lands_before_a_full_wave_could_have_finished` +/// builds — deep and wide enough (with `STREAMED_STARTUP_FILE_LINES`) that the full wave's +/// total diff cost is many seconds, well clear of `STREAMED_STARTUP_RESPONSIVE`, while a single +/// changeset's diff (what the streamed path actually waits on before its first frame) stays +/// well under it. See `STREAMED_STARTUP_RESPONSIVE`'s doc comment for the measurements that +/// picked this size. +const STREAMED_STARTUP_STACK_SIZE: usize = 150; + +/// Lines per generated fixture file in the streamed-startup stack — see +/// `STREAMED_STARTUP_STACK_SIZE`. +const STREAMED_STARTUP_FILE_LINES: usize = 3_000; + /// A plausible-enough Rust source of ~`lines` lines, distinct per `seed`, so the tree-sitter /// highlighter has real parsing work per file (the regression cost being guarded). fn rust_source(seed: usize, lines: usize) -> String { @@ -153,3 +190,139 @@ fn rapid_outline_nav_burst_stays_responsive() { (input coalescing or idle-deferred loads regressed)" ); } + +/// Commit `path`/`content` as a child of `parent`, without moving any branch ref — mirrors +/// `diff_model.rs`'s `commit_onto`, duplicated here rather than shared: this file needs it only +/// to build one deep real Graphite chain, not worth a cross-test-file dependency for. +fn commit_onto( + repo: &git2::Repository, + parent: &git2::Commit, + path: &str, + content: &str, +) -> git2::Oid { + let mut treebuilder = repo.treebuilder(Some(&parent.tree().unwrap())).unwrap(); + let blob_oid = repo.blob(content.as_bytes()).unwrap(); + treebuilder + .insert(path, blob_oid, git2::FileMode::Blob.into()) + .unwrap(); + let tree_oid = treebuilder.write().unwrap(); + let tree = repo.find_tree(tree_oid).unwrap(); + let sig = repo.signature().unwrap(); + repo.commit(None, &sig, &sig, "test commit", &tree, &[parent]) + .unwrap() +} + +/// ADR-037's Testing layer 3, part two: the `pty_responsiveness` extension the ADR calls for — +/// "asserting the first interactive frame lands before a full wave could have finished". A real +/// `STREAMED_STARTUP_STACK_SIZE`-deep Graphite stack, each node adding one +/// `STREAMED_STARTUP_FILE_LINES`-line file (real content, so each changeset's diff is real, +/// non-trivial work — the cost being guarded), checked out on the TIP branch so the real +/// binary's no-argument auto-detect (`StackModel::detect` + `assemble_changesets`) sees the +/// whole stack, exactly like a real multi-changeset Graphite review. +/// +/// The oracle is the SAME shape as `launch_reaches_the_tui_and_quits_promptly`'s: `q` sent the +/// instant the alternate screen appears, bounding spawn→quit — `q` buffers in the PTY until the +/// event loop starts polling input, so this elapsed time IS "time until the event loop is +/// actually running and responsive", not just "time to first draw". That is precisely what a +/// de-streaming regression breaks: reverting `Tui::run_streamed` to block on the full diff wave +/// (e.g. joining `spawn_wave_thread`, or calling `diff_changesets` synchronously like the +/// pre-ADR-037 multi-changeset path did) delays the event loop's first `recv_event` by the +/// WHOLE wave's cost, not just the active changeset's — so `q` sits unanswered in the PTY for +/// the full wave's duration. See `STREAMED_STARTUP_RESPONSIVE`'s doc comment for the actual +/// measured numbers (streamed ~1.1-1.4s, reverted-to-pre-ADR-037 ~6.4s on this exact fixture) +/// that sized the bound and confirm this assertion fails on the regressed shape, the same +/// validation discipline `BURST_RESPONSIVE`'s doc comment describes. +#[test] +#[ignore = "PTY smoke — run explicitly: cargo test -p git-workon-review --test pty_responsiveness -- --ignored"] +fn streamed_startup_lands_before_a_full_wave_could_have_finished() { + use workon::{assemble_changesets, StackModel, UncommittedLayer}; + + let n = STREAMED_STARTUP_STACK_SIZE; + let mut builder = FixtureBuilder::new() + .config("core.autocrlf", "false") + .config("workon.review.theme", "dark") + .graphite_config(&["main"]); + for i in 0..n { + let parent = if i == 0 { + "main".to_string() + } else { + format!("cs{}", i - 1) + }; + builder = builder.branch_metadata(&format!("cs{i}"), &parent); + } + let fixture = builder.build().expect("fixture"); + let repo = fixture.repo().expect("fixture repo"); + + // `branch_metadata` creates each branch ref at build() time (co-located with `main`'s tip); + // advance each one to a REAL, distinct commit forming an actual linear chain — same pattern + // `diff_model.rs`'s Graphite tests use. + let main_tip = repo + .find_branch("main", git2::BranchType::Local) + .expect("main branch") + .get() + .target() + .expect("main tip"); + let mut parent_oid = main_tip; + for i in 0..n { + let parent_commit = repo.find_commit(parent_oid).expect("parent commit"); + let head = commit_onto( + repo, + &parent_commit, + &format!("f{i}.rs"), + &rust_source(i, STREAMED_STARTUP_FILE_LINES), + ); + fixture + .update_branch(&format!("cs{i}"), head) + .expect("advance branch"); + parent_oid = head; + } + + // Sanity: the stack really does resolve to `n` real changesets, each with a real diff to + // do — a fixture bug here (e.g. all branches landing on the same commit) would make the + // bound pass for the wrong reason. + let changesets = assemble_changesets( + repo, + &format!("cs{}", n - 1), + StackModel::Graphite, + UncommittedLayer::Include, + ) + .expect("assemble the real Graphite stack"); + assert_eq!( + changesets + .iter() + .filter(|c| c.name.starts_with("cs")) + .count(), + n, + "fixture setup must produce every tracked changeset" + ); + + // Check out the tip branch as HEAD — the real binary's no-argument auto-detect reads + // `repo.head()`'s shorthand, not an explicit `[SOURCE]` argument. + repo.set_head(&format!("refs/heads/cs{}", n - 1)) + .expect("set HEAD to the tip branch"); + let mut checkout = git2::build::CheckoutBuilder::new(); + checkout.force(); + repo.checkout_head(Some(&mut checkout)) + .expect("checkout the tip branch"); + + let launched = Instant::now(); + let mut session = spawn_review(&fixture); + session + .expect("\x1b[?1049h") + .expect("TUI entered the alternate screen"); + + // Same protocol as the launch test: `q` buffers in the PTY until the event loop polls + // input, so spawn→quit IS time-to-interactive plus one quit — the full wave's cost, if the + // event loop were blocked behind it, lands entirely inside this window. + session.send("q").expect("send q"); + session.expect(expectrl::Eof).expect("app exited on q"); + + let elapsed = launched.elapsed(); + eprintln!("streamed startup ({n} changesets)→quit: {elapsed:?}"); + assert!( + elapsed < STREAMED_STARTUP_RESPONSIVE, + "streamed-startup→quit took {elapsed:?} on a {n}-changeset stack — the event loop \ + looks blocked behind the full diff wave again (streamed launch regressed to a \ + synchronous wait)" + ); +} From a8d6d760b2688b9e0048a4827df649f0489fb2bb Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 10:28:20 -0400 Subject: [PATCH 15/16] test(review): pin per-changeset spans in the streamed-startup fixture --- git-workon-review/tests/pty_responsiveness.rs | 65 +++++++++++++++---- 1 file changed, 52 insertions(+), 13 deletions(-) diff --git a/git-workon-review/tests/pty_responsiveness.rs b/git-workon-review/tests/pty_responsiveness.rs index 134d13b1..7b59c1be 100644 --- a/git-workon-review/tests/pty_responsiveness.rs +++ b/git-workon-review/tests/pty_responsiveness.rs @@ -70,19 +70,27 @@ const BURST_FILES: usize = 36; const BURST_FILE_LINES: usize = 2_000; /// Upper bound on spawn→quit for `streamed_startup_lands_before_a_full_wave_could_have_finished` -/// (ADR-037). Sized from real measurements on this fixture (debug build, `q` sent right after +/// (ADR-037). Sized from real measurements on this fixture (`q` sent right after /// alternate-screen entry — same protocol as `LAUNCH_RESPONSIVE`, so `q` buffers in the PTY /// until the event loop actually starts polling input): /// -/// - The CURRENT (streamed) binary: ~1.1-1.4s — dominated by fixed per-launch costs -/// (`gt --version`'s ~350ms subprocess spawn, checking out `STREAMED_STACK_SIZE` branches' -/// worth of files) plus ONE changeset's diff (the active one, streamed first). -/// - The PRE-ADR-037 binary (commit `2214558`, built and run against the identical fixture): -/// ~6.4s — it blocks on the full stack's diff wave before the event loop ever starts, so `q` -/// sits in the PTY the whole time. +/// - The CURRENT (streamed) binary: ~1.0-1.3s (debug build; ~1.0s release) — dominated by fixed +/// per-launch costs (`gt --version`'s ~350ms subprocess spawn, checking out +/// `STREAMED_STARTUP_STACK_SIZE` branches' worth of files) plus entering the alternate screen +/// and starting the event loop, which — this is the whole point of ADR-037 — happens BEFORE +/// any changeset is diffed: the wave runs on a background thread, off the spawn→interactive +/// critical path entirely. Re-measured after fixing the fixture setup (F4) that was building +/// every `cs{i}`'s base against main's tip instead of its own predecessor's head; since none +/// of a changeset's diff cost is on this critical path either way, the number barely moved — +/// confirming spawn→quit here really is a fixed-cost bound, not a diff-size one. +/// - The PRE-ADR-037 binary (commit `2214558`, built and run against an equivalent fixture +/// pre-F4): ~6.4s — it blocks on the full stack's diff wave before the event loop ever starts, +/// so `q` sits in the PTY the whole time. Not re-measured against the F4-corrected fixture +/// (would need rebuilding that historical commit); a synchronous full-wave wait over 150 +/// real per-changeset diffs is unambiguously far past this bound either way. /// -/// This bound sits roughly 2× above the healthy number and comfortably (~2×) below the -/// regressed one — the same blunt, load-tolerant margin philosophy as `LAUNCH_RESPONSIVE`/ +/// This bound sits comfortably (~2-3×) above the healthy number and well below the regressed +/// one — the same blunt, load-tolerant margin philosophy as `LAUNCH_RESPONSIVE`/ /// `BURST_RESPONSIVE`, just re-measured for this fixture rather than reused, since the streamed /// path's fixed costs (checkout of a much deeper stack) differ from the single-changeset launch /// test's. @@ -287,14 +295,45 @@ fn streamed_startup_lands_before_a_full_wave_could_have_finished() { UncommittedLayer::Include, ) .expect("assemble the real Graphite stack"); + let cs_changesets: Vec<&workon::Changeset> = changesets + .iter() + .filter(|c| c.name.starts_with("cs")) + .collect(); assert_eq!( - changesets - .iter() - .filter(|c| c.name.starts_with("cs")) - .count(), + cs_changesets.len(), n, "fixture setup must produce every tracked changeset" ); + // F4: pin the intended shape — each `cs{i}`'s span is base→head against its OWN immediate + // predecessor (cs0's base is main's tip), not every changeset cumulatively based on main. + // `branch_metadata`'s revisions resolve once at `build()` time (before the `update_branch` + // loop above moves any ref), so a fixture that doesn't keep those recorded revisions in + // sync makes `resolve_graphite_base` fall back to main's tip for every one of them — + // cumulative 150-file spans and `needs_restack = true` everywhere, contradicting both this + // shape and the "one changeset's diff" cost this test's bound is sized against. + let mut expected_base = main_tip; + for (i, cs) in cs_changesets.iter().enumerate() { + assert_eq!( + cs.name, + format!("cs{i}"), + "changesets must come back in base→head order" + ); + match cs.span { + workon::ChangesetSpan::Committed { base, head } => { + assert_eq!( + base, expected_base, + "cs{i}'s base must be its own predecessor's head, not main's tip" + ); + expected_base = head; + } + other => panic!("cs{i} must be a Committed span, got {other:?}"), + } + assert!( + !cs.needs_restack, + "cs{i} must not need a restack — its recorded parent revision must track its \ + parent's live tip" + ); + } // Check out the tip branch as HEAD — the real binary's no-argument auto-detect reads // `repo.head()`'s shorthand, not an explicit `[SOURCE]` argument. From e58fa6f7964855af3629d131fec9a97f6c6b20ee Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 10 Jul 2026 12:54:22 -0400 Subject: [PATCH 16/16] perf(review): cache silent-terminal probe verdicts for theme auto --- Cargo.lock | 2 + Cargo.toml | 1 + git-workon-review/Cargo.toml | 2 + git-workon-review/src/lib.rs | 1 + git-workon-review/src/main.rs | 12 +- git-workon-review/src/probe_cache.rs | 332 +++++++++++++++++++++ git-workon-review/src/terminal_query.rs | 89 +++++- git-workon-review/tests/pty_smoke.rs | 32 +- git-workon-review/tests/pty_support/mod.rs | 15 +- 9 files changed, 462 insertions(+), 24 deletions(-) create mode 100644 git-workon-review/src/probe_cache.rs diff --git a/Cargo.lock b/Cargo.lock index e78c6c1a..7cb53d85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -965,6 +965,7 @@ dependencies = [ "clap", "clap_complete", "crossterm", + "dirs", "expectrl", "git-workon-fixture", "git-workon-lib", @@ -973,6 +974,7 @@ dependencies = [ "miette", "predicates", "ratatui", + "serde_json", "similar", "thiserror 2.0.19", "tree-sitter", diff --git a/Cargo.toml b/Cargo.toml index 8f2147b3..f68fe8c4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,7 @@ clap_complete = { version = "4.6.5", features = ["unstable-dynamic"] } clap_mangen = "0.3.0" crossterm = "0.29.0" dialoguer = { version = "0.12.0", features = ["fuzzy-select"] } +dirs = "6.0" env_logger = "0.11.10" git-workon-lib = { version = "0.13.2", path = "./git-workon-lib" } git-workon-fixture = { path = "./git-workon-fixture" } diff --git a/git-workon-review/Cargo.toml b/git-workon-review/Cargo.toml index 6c4e3533..32bc7e9c 100644 --- a/git-workon-review/Cargo.toml +++ b/git-workon-review/Cargo.toml @@ -35,11 +35,13 @@ vendored = ["git-workon-lib/vendored", "git2/vendored-libgit2", "git2/vendored-o clap.workspace = true clap_complete.workspace = true crossterm.workspace = true +dirs.workspace = true git-workon-lib.workspace = true git2.workspace = true libc.workspace = true miette.workspace = true ratatui.workspace = true +serde_json.workspace = true similar.workspace = true thiserror.workspace = true tree-sitter.workspace = true diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index b45d452d..cff5f735 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -25,6 +25,7 @@ pub mod keymap; pub mod model; pub mod ops; pub mod outline; +pub mod probe_cache; pub mod queue; pub mod refresh; pub mod render; diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index 61ed4cb0..40ca46f5 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -94,12 +94,16 @@ fn main() -> Result<()> { // probe (CS6), which needs the controlling tty and so lives outside the pure `theme.rs`; it is // bounded by a hard timeout and always yields a curated fallback on a silent/hostile terminal, // never a hang. `Dark`/`Light` stay CS5's I/O-free `for_theme` path. + // `probed` is whether a real probe conversation happened on the tty this launch — NOT just + // "theme was auto". `detect_auto_palette` reports `false` on a cached "silent terminal" + // verdict (see `probe_cache`), since a cache hit writes nothing to the tty and so owes no + // flush; every other path (an answered probe, a timed-out-uncached probe, a non-auto theme) + // is `false`/`true` exactly as before. let selection = ReviewConfig::new(&repo).theme(); - let probed = matches!(selection, Ok(config::Theme::Auto)); - let theme = match selection { + let (theme, probed) = match selection { Ok(config::Theme::Auto) => terminal_query::detect_auto_palette(), - Ok(selection) => Palette::for_theme(selection), - Err(_) => Palette::dark(), + Ok(selection) => (Palette::for_theme(selection), false), + Err(_) => (Palette::dark(), false), }; // Resolve the view-config settings (outline width/mode, diff layout/zoom) the same way, diff --git a/git-workon-review/src/probe_cache.rs b/git-workon-review/src/probe_cache.rs new file mode 100644 index 00000000..46b8d4b8 --- /dev/null +++ b/git-workon-review/src/probe_cache.rs @@ -0,0 +1,332 @@ +//! Cache for the `theme = auto` probe's "this terminal never answers" verdict (a follow-up to +//! ADR-037, whose scope section deferred it). +//! +//! [`crate::terminal_query::detect_auto_palette`] pays an 800ms timeout ONLY when the terminal +//! answers nothing at all — every real interactive terminal answers within a few ms, so a full +//! timeout means the controlling terminal (tmux without passthrough, plain ssh, CI, a dumb +//! terminal) structurally cannot answer and never will, this launch or the next. This module +//! remembers that one verdict so later launches from the same terminal skip the probe and go +//! straight to the curated fallback ([`crate::theme::Palette::dark`] — exactly what an empty +//! probe result already produces, so a cache hit changes timing, never the resulting palette). +//! +//! **Only silence is cached.** A terminal that answers ANYTHING — even just the DA1 sentinel, +//! even a partial/malformed color — returns in milliseconds and is never recorded here, so live +//! theme detection (e.g. macOS Terminal flipping between its light and dark profiles) keeps +//! working every launch. +//! +//! ## Key +//! The controlling tty's device path (`/dev/ttysNNN` via `ttyname_r`) plus `$TERM` and +//! `$TERM_PROGRAM`. The tty path scopes a verdict to one terminal window; TERM/TERM_PROGRAM guard +//! against a later, DIFFERENT emulator reusing a recycled tty number and inheriting a stale +//! "silent" verdict it never earned. +//! +//! ## Store +//! A small human-readable JSON array of `{tty, term, term_program, timestamp}` objects under +//! [`dirs::cache_dir`]`/git-workon-review/silent-terminals.json` (overridable via the +//! `WORKON_REVIEW_PROBE_CACHE` env var — used by the PTY test suites to keep them off the real +//! user cache; see `tests/pty_support/mod.rs`). Entries expire after [`TTL_SECS`] (30 days) and +//! are pruned opportunistically the next time an entry is written, so the file never grows +//! unboundedly across many terminals. +//! +//! ## Escaping a wrong verdict +//! A "silent" verdict recorded against a terminal that later becomes capable of answering (rare — +//! e.g. reconfiguring tmux passthrough) self-heals after [`TTL_SECS`]. To force it sooner: delete +//! the cache file, or pin `workon.review.theme` to `dark`/`light` instead of `auto`. +//! +//! ## Failure posture +//! Every step here — resolving the cache dir, reading, parsing, writing — degrades silently to +//! "probe again": a missing/corrupt/unwritable cache file, no resolvable cache dir, or no +//! controlling tty (so no key) all fall through to [`crate::terminal_query::detect_auto_palette`] +//! running its probe exactly as it would with no cache at all. No new error type exists for this +//! module on purpose — there is nothing for a caller to react to. + +use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use serde_json::{json, Value}; + +/// How long a "silent terminal" verdict stays trusted before the probe runs again. +const TTL_SECS: u64 = 30 * 24 * 60 * 60; + +/// Identifies one controlling terminal window for cache lookups — see the module doc's "Key" +/// section for the rationale behind each field. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct TerminalKey { + tty: String, + term: String, + term_program: String, +} + +/// Build this launch's [`TerminalKey`] from the controlling tty and environment. `None` when +/// there's no controlling tty to key against (no `/dev/tty`, not unix) — callers treat that the +/// same as a cache miss. +#[cfg(unix)] +pub(crate) fn terminal_key() -> Option { + use std::ffi::CStr; + use std::os::unix::io::AsRawFd; + + let tty = std::fs::File::options().read(true).open("/dev/tty").ok()?; + let fd = tty.as_raw_fd(); + let mut buf = [0 as std::os::raw::c_char; 256]; + if unsafe { libc::ttyname_r(fd, buf.as_mut_ptr(), buf.len()) } != 0 { + return None; + } + // Safety: `ttyname_r` returning 0 guarantees `buf` holds a NUL-terminated string. + let tty_path = unsafe { CStr::from_ptr(buf.as_ptr()) } + .to_str() + .ok()? + .to_string(); + + Some(TerminalKey { + tty: tty_path, + term: std::env::var("TERM").unwrap_or_default(), + term_program: std::env::var("TERM_PROGRAM").unwrap_or_default(), + }) +} + +#[cfg(not(unix))] +pub(crate) fn terminal_key() -> Option { + None +} + +/// Whether `key` has a live (unexpired) "silent" verdict cached. Any failure to resolve or read +/// the cache — no cache dir, missing/corrupt file — reports `false`, so the caller probes. +pub(crate) fn is_cached_silent(key: &TerminalKey) -> bool { + match cache_path() { + Some(path) => is_silent_at(&path, key, now_unix()), + None => false, + } +} + +/// Record that `key`'s terminal timed out silent on this launch (which already paid the full +/// probe deadline). Best-effort: any failure to resolve the cache dir or write the file is +/// swallowed — a launch that can't cache its verdict just probes again next time, same as today. +pub(crate) fn record_silent(key: &TerminalKey) { + if let Some(path) = cache_path() { + record_silent_at(&path, key, now_unix()); + } +} + +/// The cache file's location: `WORKON_REVIEW_PROBE_CACHE` when set (the PTY test suites' escape +/// hatch — see the module doc), else `dirs::cache_dir()/git-workon-review/silent-terminals.json`. +/// `None` when neither resolves (no `HOME`/`XDG_CACHE_HOME` equivalent for `dirs` to find). +fn cache_path() -> Option { + if let Ok(overridden) = std::env::var("WORKON_REVIEW_PROBE_CACHE") { + return Some(PathBuf::from(overridden)); + } + Some( + dirs::cache_dir()? + .join("git-workon-review") + .join("silent-terminals.json"), + ) +} + +fn now_unix() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0) +} + +// ── The pure, path-injected core (unit-tested against a temp dir, never the real cache) ──────── + +/// `true` when `path`'s cache holds a `key` entry whose `timestamp` is within [`TTL_SECS`] of +/// `now`. A missing or corrupt file, or an all-expired/no-match set of entries, is `false`. +fn is_silent_at(path: &Path, key: &TerminalKey, now: u64) -> bool { + read_entries(path) + .iter() + .any(|entry| entry_matches(entry, key) && entry_is_live(entry, now)) +} + +/// Write a fresh `key` verdict timestamped `now`, first dropping any expired entry (per +/// [`TTL_SECS`]) and any existing entry for `key` (a re-verdict replaces, not duplicates). Silent +/// on any I/O failure — see the module doc's "Failure posture". +fn record_silent_at(path: &Path, key: &TerminalKey, now: u64) { + let mut entries = read_entries(path); + entries.retain(|entry| entry_is_live(entry, now) && !entry_matches(entry, key)); + entries.push(json!({ + "tty": key.tty, + "term": key.term, + "term_program": key.term_program, + "timestamp": now, + })); + write_entries(path, &entries); +} + +fn entry_matches(entry: &Value, key: &TerminalKey) -> bool { + entry.get("tty").and_then(Value::as_str) == Some(key.tty.as_str()) + && entry.get("term").and_then(Value::as_str) == Some(key.term.as_str()) + && entry.get("term_program").and_then(Value::as_str) == Some(key.term_program.as_str()) +} + +fn entry_is_live(entry: &Value, now: u64) -> bool { + matches!( + entry.get("timestamp").and_then(Value::as_u64), + Some(ts) if now.saturating_sub(ts) < TTL_SECS + ) +} + +/// Read the cache file into a list of raw JSON entries. Anything short of "a valid JSON array" — +/// a missing file, unreadable file, malformed JSON, or JSON that isn't an array — is treated as +/// an empty cache, never an error. +fn read_entries(path: &Path) -> Vec { + let Ok(contents) = std::fs::read_to_string(path) else { + return Vec::new(); + }; + match serde_json::from_str::(&contents) { + Ok(Value::Array(entries)) => entries, + _ => Vec::new(), + } +} + +/// Best-effort pretty-printed write of `entries`, creating the parent directory if needed. Any +/// failure (read-only filesystem, missing permissions, ...) is swallowed. +fn write_entries(path: &Path, entries: &[Value]) { + if let Some(parent) = path.parent() { + if std::fs::create_dir_all(parent).is_err() { + return; + } + } + if let Ok(text) = serde_json::to_string_pretty(&Value::Array(entries.to_vec())) { + let _ = std::fs::write(path, text); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn key(tty: &str) -> TerminalKey { + TerminalKey { + tty: tty.to_string(), + term: "xterm-256color".to_string(), + term_program: "iTerm.app".to_string(), + } + } + + fn cache_file() -> (assert_fs::TempDir, PathBuf) { + let dir = assert_fs::TempDir::new().expect("temp dir"); + let path = dir.path().join("silent-terminals.json"); + (dir, path) + } + + #[test] + fn missing_file_is_not_silent() { + let (_dir, path) = cache_file(); + assert!(!is_silent_at(&path, &key("/dev/ttys000"), 1_000)); + } + + #[test] + fn record_then_lookup_round_trips() { + let (_dir, path) = cache_file(); + let k = key("/dev/ttys000"); + record_silent_at(&path, &k, 1_000); + assert!( + is_silent_at(&path, &k, 1_500), + "just-recorded verdict must be live" + ); + } + + #[test] + fn a_different_tty_term_or_term_program_is_a_miss() { + let (_dir, path) = cache_file(); + record_silent_at(&path, &key("/dev/ttys000"), 1_000); + + assert!( + !is_silent_at(&path, &key("/dev/ttys001"), 1_000), + "different tty" + ); + + let mut different_term = key("/dev/ttys000"); + different_term.term = "screen".to_string(); + assert!( + !is_silent_at(&path, &different_term, 1_000), + "different $TERM" + ); + + let mut different_program = key("/dev/ttys000"); + different_program.term_program = "Apple_Terminal".to_string(); + assert!( + !is_silent_at(&path, &different_program, 1_000), + "different $TERM_PROGRAM — guards against tty-number recycling" + ); + } + + #[test] + fn an_expired_entry_is_ignored() { + let (_dir, path) = cache_file(); + let k = key("/dev/ttys000"); + record_silent_at(&path, &k, 1_000); + + assert!( + is_silent_at(&path, &k, 1_000 + TTL_SECS - 1), + "just inside the TTL" + ); + assert!( + !is_silent_at(&path, &k, 1_000 + TTL_SECS), + "exactly at the TTL boundary" + ); + assert!( + !is_silent_at(&path, &k, 1_000 + TTL_SECS + 1_000), + "well past the TTL" + ); + } + + #[test] + fn writing_prunes_expired_entries() { + let (_dir, path) = cache_file(); + let stale = key("/dev/ttys000"); + let fresh = key("/dev/ttys001"); + record_silent_at(&path, &stale, 1_000); + // Advance well past the stale entry's TTL, then record a second (different) verdict — + // the write must drop the stale entry rather than accumulate it forever. + record_silent_at(&path, &fresh, 1_000 + TTL_SECS + 1); + + let entries = read_entries(&path); + assert_eq!( + entries.len(), + 1, + "the expired entry must be pruned on write" + ); + assert!(entry_matches(&entries[0], &fresh)); + } + + #[test] + fn re_recording_the_same_key_replaces_rather_than_duplicates() { + let (_dir, path) = cache_file(); + let k = key("/dev/ttys000"); + record_silent_at(&path, &k, 1_000); + record_silent_at(&path, &k, 2_000); + + let entries = read_entries(&path); + assert_eq!(entries.len(), 1, "a re-verdict must replace, not duplicate"); + assert_eq!( + entries[0].get("timestamp").and_then(Value::as_u64), + Some(2_000) + ); + } + + #[test] + fn a_corrupt_file_is_tolerated_and_overwritten() { + let (_dir, path) = cache_file(); + std::fs::write(&path, b"not json at all { [").expect("write garbage"); + + assert!( + !is_silent_at(&path, &key("/dev/ttys000"), 1_000), + "corrupt file must read back as no cache, not a crash" + ); + + // Recovery: a subsequent write must succeed and be readable, proving the corrupt file + // doesn't wedge the cache permanently. + record_silent_at(&path, &key("/dev/ttys000"), 1_000); + assert!(is_silent_at(&path, &key("/dev/ttys000"), 1_000)); + } + + #[test] + fn a_missing_cache_directory_is_created_on_write() { + let dir = assert_fs::TempDir::new().expect("temp dir"); + let path = dir.path().join("nested").join("silent-terminals.json"); + record_silent_at(&path, &key("/dev/ttys000"), 1_000); + assert!(path.exists(), "write must create missing parent dirs"); + } +} diff --git a/git-workon-review/src/terminal_query.rs b/git-workon-review/src/terminal_query.rs index 0310adaf..b4f9cc4e 100644 --- a/git-workon-review/src/terminal_query.rs +++ b/git-workon-review/src/terminal_query.rs @@ -41,6 +41,7 @@ use std::time::Duration; use ratatui::style::Color; +use crate::probe_cache; use crate::theme::{self, tint_toward, Base16, Palette}; /// The colors read back from a terminal OSC probe. `ansi16` is `Some` only if **all 16** ANSI @@ -65,8 +66,31 @@ pub struct ProbeResult { /// full deadline — and giving up early on a merely-slow terminal is worse than the wait, because /// replies that arrive after the probe stopped listening leak into crossterm as phantom /// keystrokes (`r` → refresh storms, `d` → a discard confirm that captures the keyboard). -pub fn detect_auto_palette() -> Palette { - palette_for_auto(&probe_terminal(Duration::from_millis(800))) +/// +/// [`crate::probe_cache`] remembers a terminal that has already paid this deadline and gotten +/// nothing back: when this launch's controlling terminal has a live "silent" verdict cached, the +/// probe is skipped entirely and the curated fallback returns immediately (identical to what an +/// empty probe result would have produced). A terminal that answers ANYTHING is never cached, so +/// live detection keeps working there every launch. +/// +/// The second element of the returned tuple is whether a real probe conversation happened on the +/// controlling tty this call — `false` only on a cache hit. `main.rs` uses it (instead of just +/// "theme was auto") to decide whether [`flush_pending_tty_input`] is needed: a cache hit writes +/// nothing to the tty, so no replies are ever owed and flushing would only risk eating legitimate +/// type-ahead (see that function's doc comment). +pub fn detect_auto_palette() -> (Palette, bool) { + let key = probe_cache::terminal_key(); + if key.as_ref().is_some_and(probe_cache::is_cached_silent) { + return (Palette::dark(), false); + } + + let (probe, timed_out_silent) = probe_terminal(Duration::from_millis(800)); + if timed_out_silent { + if let Some(key) = &key { + probe_cache::record_silent(key); + } + } + (palette_for_auto(&probe), true) } /// Discard any bytes pending on the controlling tty's input queue. `main.rs` calls this after the @@ -148,21 +172,43 @@ pub fn build_base16(ansi: &[Color; 16], background: Color, foreground: Option ProbeResult { +/// +/// The second element is `true` only when [`query_terminal_raw`] paid the FULL `timeout` and +/// still got zero reply bytes — [`probe_cache`]'s one cacheable case. A terminal that answered +/// (even partially) or a probe that couldn't even start (no `/dev/tty`, not a tty, a failed +/// write) are both `false`: the former has nothing to cache, the latter never waited long enough +/// for caching to save anything. +fn probe_terminal(timeout: Duration) -> (ProbeResult, bool) { #[cfg(unix)] { match query_terminal_raw(&build_query(), timeout) { - Some(bytes) => parse_osc_replies(&bytes), - None => ProbeResult::default(), + ProbeOutcome::Replied(bytes) => (parse_osc_replies(&bytes), false), + ProbeOutcome::TimedOutSilent => (ProbeResult::default(), true), + ProbeOutcome::Unavailable => (ProbeResult::default(), false), } } #[cfg(not(unix))] { let _ = timeout; - ProbeResult::default() + (ProbeResult::default(), false) } } +/// The outcome of one attempt at [`query_terminal_raw`] — distinguishes "the terminal answered" +/// from the two different ways it can answer nothing, only one of which is worth caching (see +/// [`probe_terminal`]'s doc comment). +#[cfg(unix)] +enum ProbeOutcome { + /// At least one reply byte arrived. + Replied(Vec), + /// The probe wrote its query, waited the full `timeout`, and got nothing back — the terminal + /// this launch already paid the deadline for. + TimedOutSilent, + /// Probing wasn't possible this launch at all (no `/dev/tty`, not a tty, the query write + /// failed) — always fast, never worth remembering. + Unavailable, +} + /// The bytes we write to the terminal: `OSC 4;n;?` for each of the 16 ANSI colors, then /// `OSC 11;?` (background) and `OSC 10;?` (foreground), then a primary Device Attributes query /// (`ESC [ c`). Terminals answer in order, so the DA1 reply is a sentinel: once we see it, every @@ -312,24 +358,31 @@ fn has_da1_terminator(bytes: &[u8]) -> bool { /// is the one function the unit tests do NOT call (it needs a real tty); everything it feeds /// ([`parse_osc_replies`], [`build_base16`], [`palette_for_auto`]) is pure and tested directly. /// -/// Returns the raw reply bytes, or `None` if `/dev/tty` can't be opened, isn't a tty, or the read -/// yields nothing before the timeout. `None` and an empty read both degrade to the curated -/// fallback upstream. +/// Returns a [`ProbeOutcome`]: `Replied` bytes, `TimedOutSilent` when the full `timeout` elapsed +/// with nothing back, or `Unavailable` when `/dev/tty` can't be opened, isn't a tty, or the query +/// write itself fails (all of which return fast, well under `timeout`). The +/// `elapsed >= timeout` check distinguishing the latter two is deliberately a wall-clock +/// comparison rather than a distinct signal threaded up from [`read_replies`] — it needs no +/// change to that function or its already-covered unit tests, and the two cases are only ever +/// milliseconds vs. the full deadline apart. #[cfg(unix)] -fn query_terminal_raw(query: &[u8], timeout: Duration) -> Option> { +fn query_terminal_raw(query: &[u8], timeout: Duration) -> ProbeOutcome { use std::os::unix::io::AsRawFd; + use std::time::Instant; - let mut tty = std::fs::File::options() + let Ok(mut tty) = std::fs::File::options() .read(true) .write(true) .open("/dev/tty") - .ok()?; + else { + return ProbeOutcome::Unavailable; + }; let fd = tty.as_raw_fd(); // Save the current termios; bail (leaving the tty untouched) if this isn't a tty. let mut saved: libc::termios = unsafe { std::mem::zeroed() }; if unsafe { libc::tcgetattr(fd, &mut saved) } != 0 { - return None; + return ProbeOutcome::Unavailable; } // Switch to raw so the OSC replies (terminated by ST/BEL, not newline) arrive uncooked and @@ -341,9 +394,10 @@ fn query_terminal_raw(query: &[u8], timeout: Duration) -> Option> { raw.c_cc[libc::VMIN] = 0; raw.c_cc[libc::VTIME] = 1; if unsafe { libc::tcsetattr(fd, libc::TCSANOW, &raw) } != 0 { - return None; // termios unchanged — nothing to restore + return ProbeOutcome::Unavailable; // termios unchanged — nothing to restore } + let started = Instant::now(); let outcome = read_replies(&mut tty, fd, query, timeout); // Discard anything still in the terminal's input queue before handing the tty back — a @@ -356,7 +410,12 @@ fn query_terminal_raw(query: &[u8], timeout: Duration) -> Option> { // ALWAYS restore, on success or failure. unsafe { libc::tcsetattr(fd, libc::TCSANOW, &saved) }; - outcome + + match outcome { + Some(bytes) => ProbeOutcome::Replied(bytes), + None if started.elapsed() >= timeout => ProbeOutcome::TimedOutSilent, + None => ProbeOutcome::Unavailable, // the query write failed — an early bail, not a wait + } } /// The read half of [`query_terminal_raw`], factored out so `termios` restoration wraps it on diff --git a/git-workon-review/tests/pty_smoke.rs b/git-workon-review/tests/pty_smoke.rs index 5b72c5f0..0d9902fa 100644 --- a/git-workon-review/tests/pty_smoke.rs +++ b/git-workon-review/tests/pty_smoke.rs @@ -68,12 +68,19 @@ fn answer_probe(session: &mut Session) { session.flush().expect("flush probe replies"); } -/// Wait for the TUI to be up (alternate screen entered), let any straggler reply bytes land, -/// then press `q` and require a prompt exit. -fn assert_q_quits_promptly(mut session: Session) { +/// Wait for the TUI to be up (alternate screen entered). Split out from +/// [`assert_q_quits_promptly`] so a caller that needs to time spawn→alternate-screen itself can +/// do so without that function re-`expect`-ing a step already consumed. +fn wait_for_alt_screen(session: &mut Session) { session .expect("\x1b[?1049h") // EnterAlternateScreen — tui::run has the terminal .expect("TUI entered the alternate screen"); +} + +/// Wait for the TUI to be up, let any straggler reply bytes land, then press `q` and require a +/// prompt exit. +fn assert_q_quits_promptly(mut session: Session) { + wait_for_alt_screen(&mut session); // Give leaked bytes (the regression case) time to reach crossterm before q, so a regressed // binary deterministically has its discard-confirm modal up — and swallows the q. @@ -109,9 +116,26 @@ fn theme_auto_stays_responsive_when_the_terminal_answers() { #[ignore = "PTY smoke — run explicitly: cargo test -p git-workon-review --test pty_smoke -- --ignored"] fn theme_auto_stays_responsive_when_the_terminal_is_silent() { // The no-hang guarantee: a terminal that never answers (tmux without passthrough, CI) must - // cost at most the probe deadline, then fall back to a curated theme and run normally. + // cost at most the probe deadline, then fall back to a curated theme and run normally. This + // launch also exercises the `probe_cache` write path (a timed-out-silent probe records a + // verdict) — see `spawn_review`'s doc comment for how the cache file is kept off the real + // user cache during this run. let fixture = auto_theme_fixture(); let session = spawn_review(&fixture); assert_q_quits_promptly(session); + + // NOT asserted here: that a SECOND launch on this same (now cache-hit) terminal is fast. + // That behavior is real (manually verified end-to-end with the actual binary under `expect` + // — a first silent launch pays the ~800ms deadline and records a verdict; a second launch on + // the same controlling tty skips the probe and reaches the alternate screen in well under a + // millisecond) and is unit-tested at the cache layer in `probe_cache.rs`. It does NOT fit + // cleanly as a second `spawn_review` in THIS test, though: back-to-back `expectrl` sessions + // in one test process reproducibly hit `ExpectTimeout` waiting for the alternate-screen + // sequence on the second (cache-hit-fast) launch specifically, even though `Session::check` + // proves the bytes are actually present in the stream at that point — an `expectrl`/PTY + // interaction this suite's existing patterns (a single `spawn_review` per test) don't hit. + // Chasing that harness quirk was out of scope here; two-process verification stays a manual + // workflow for this one behavior, same posture pty_responsiveness.rs takes for precise + // per-phase timings. } diff --git a/git-workon-review/tests/pty_support/mod.rs b/git-workon-review/tests/pty_support/mod.rs index 1253c867..1887a730 100644 --- a/git-workon-review/tests/pty_support/mod.rs +++ b/git-workon-review/tests/pty_support/mod.rs @@ -15,12 +15,25 @@ use git_workon_fixture::prelude::*; /// Spawn the review binary in a PTY sized like a real terminal (an unsized PTY is 0×0 and /// ratatui draws nothing), cwd'd into the fixture's worktree. +/// +/// `WORKON_REVIEW_PROBE_CACHE` is pinned to a file inside the fixture's own tempdir (never the +/// real user cache dir): without this, the `theme = auto` silent-terminal cache (added alongside +/// this comment) would read and write the developer's/CI runner's actual +/// `dirs::cache_dir()/git-workon-review/silent-terminals.json` — a second run of the silent-PTY +/// test on the same real terminal would then hit a live cache entry from a PRIOR run and skip +/// the probe, breaking `theme_auto_stays_responsive_when_the_terminal_is_silent`'s timing +/// assumptions (and leaking test state into the real cache to boot). Deriving the path from the +/// fixture's workdir means repeat `spawn_review` calls against the SAME fixture share one cache +/// file, while different fixtures — different tempdirs — never collide. pub fn spawn_review(fixture: &Fixture) -> Session { let repo = fixture.repo().expect("fixture repo"); let workdir = repo.workdir().expect("fixture workdir").to_path_buf(); + let probe_cache = workdir.join(".git-workon-review-probe-cache.json"); let mut cmd = std::process::Command::new(env!("CARGO_BIN_EXE_git-workon-review")); - cmd.current_dir(workdir).env("TERM", "xterm-256color"); + cmd.current_dir(&workdir) + .env("TERM", "xterm-256color") + .env("WORKON_REVIEW_PROBE_CACHE", &probe_cache); let mut session = expectrl::Session::spawn(cmd).expect("spawn in PTY"); session