From e219fab0643e219c76e7af027210e1826e5e61d8 Mon Sep 17 00:00:00 2001 From: nyxb Date: Tue, 28 Apr 2026 18:56:35 +0200 Subject: [PATCH 1/8] fix(blocks): generation counter + envelope-owned padding + idempotent pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #46, #47, #48. The three regressions surfaced as separate bug reports but share one architectural root: state ownership at the wrong layer. Each fix moves the invariant to the producer that actually owns it. #46 — Snapshot cache invalidation --------------------------------- `render_view.rs` keyed the active-block snapshot cache on `snapshot.total_rows() as u64`, so any in-place cell write that left the row count alone — progress bars, `\r` overwrites, partial-line emission — never advanced the cache key. The render-side memoize at `block_list.rs::memoize_active_snapshot` then replayed the stale snapshot, hiding output for seconds before a row commit finally flipped the key and dumped a flood. `ActiveBlock` now carries a Lamport-style `Generation: AtomicU64` that bumps on every observable mutation: grid writes, atlas interns, hyperlink/grapheme/image inserts, metadata changes, replay-buffer appends, kind promotion. Mirrors `text::Buffer::version` from Zed's clock crate. The producer holds the single source of truth; the render layer reads it via `block.generation()` into the existing `(id, frame_id)` cache key. `Relaxed` ordering — the surrounding `Mutex` already provides synchronization at every mutation site. Tests in `carrot_term::block::active::generation_tests` cover the full envelope: starts above zero, advances on every mutation method, holds on no-op (`record_bytes` with empty slice, repeated `promote_kind_to_tui` after the kind is already Tui), and the specifically-regressing case where many `grid_mut()` calls happen without `total_rows()` changing. #47 — Block-content left-edge alignment --------------------------------------- The block header chips lived inside a `div` with `.px(BLOCK_HEADER_PAD_X)`; the cell-grid `BlockElement` was a sibling of that div with no matching inset. Header sat at x=16, grid at x=0, hit-test compensated by subtracting the constant against the wrong coordinate, and `terminal_pane.rs` reserved `BLOCK_HEADER_PAD_X * 2` from the PTY column budget — three independent ideas of where the content edge lived. The block envelope now owns the inset: a single `.px(BLOCK_HEADER_PAD_X)` on the parent `div` in `render_block_entry`. Both children (header, grid) render flush against the inset. Same consolidation in the fold area — `fold_area` parent gets the inset, each fold-line / fold-counter renders flush. `GridOriginStore` was widened from `Option` (just y) to `Option>` (full origin). `BlockElement::prepaint` writes `bounds.origin` straight in. `hit_test` reads x and y from the store and stops re-applying `BLOCK_HEADER_PAD_X` itself — the constant has exactly two consumers now (the layout call in `block_list.rs` and the PTY column budget in `terminal_pane.rs`), one rule, no drift. The `block_list/layout.rs` duplicate type alias is gone — re-exports the canonical one from `carrot_block_render`. #48 — Scroll viewport oscillation --------------------------------- The visible bounce was a §A symptom: with the wrong cache key the active block's measured height drifted between frames, and `logical_scroll_top` recomputed against shifting heights produced an unstable scroll position. Fixing #46 stabilizes the heights, the scroll stops oscillating. Layered on top: `BlockListView` now caches `last_pinned: Option` and only invokes `BlockState::pin` / `unpin` when the desired pin actually differs. Pin and unpin are pseudo-idempotent on the state side (no-op when the value matches), but they still take the inner `RefCell::borrow_mut` — wasteful at one render per PTY tick. The cache moves the no-op all the way out. No change to `BlockState`'s scroll model itself: the existing `follow_tail` semantics, `logical_scroll_top` resolution, and `scroll_to_end` idempotence are correct (Zed's `ListState` shape, upstream-validated). --- .../carrot-block-render/src/block_element.rs | 24 ++-- crates/carrot-term/src/block/active.rs | 127 +++++++++++++++++- crates/carrot-term/src/block/render_view.rs | 15 ++- crates/carrot-terminal-view/src/block_list.rs | 30 ++++- .../src/block_list/fold.rs | 2 - .../src/block_list/hit_test.rs | 28 ++-- .../src/block_list/layout.rs | 15 +-- 7 files changed, 190 insertions(+), 51 deletions(-) diff --git a/crates/carrot-block-render/src/block_element.rs b/crates/carrot-block-render/src/block_element.rs index ff2e0c58..74b2f2d0 100644 --- a/crates/carrot-block-render/src/block_element.rs +++ b/crates/carrot-block-render/src/block_element.rs @@ -144,17 +144,19 @@ pub struct BlockElement { search_match_color: Oklch, search_active_color: Oklch, /// Optional slot into which the element's actual paint-time - /// origin Y is written during prepaint. Consumers install one per - /// block so hit-testing can map mouse-y → data-row with zero - /// sub-pixel drift (no re-derivation from layout). + /// origin (both x and y) is written during prepaint. Consumers + /// install one per block so hit-testing can map mouse coordinates + /// → data row + column with zero sub-pixel drift and without + /// re-deriving positions from the layout tree. origin_store: Option, } /// Shared cell into which [`BlockElement`] writes its actual paint- -/// time origin Y coordinate. Used by the owning list view for -/// pixel-accurate mouse-to-row hit testing — avoids recomputing -/// positions from the layout tree after the frame paints. -pub type GridOriginStore = std::rc::Rc>>; +/// time origin. Used by the owning list view for pixel-accurate +/// mouse-to-row/column hit testing — the stored point reflects the +/// real painted origin after every layout pass, so hit-test never +/// re-applies layout-side padding tokens itself. +pub type GridOriginStore = std::rc::Rc>>>; impl BlockElement { pub fn new( @@ -275,11 +277,13 @@ impl Element for BlockElement { let font_id = window.text_system().resolve_font(&self.font); let effective_cols = self.viewport_cols.max(1); - // Stash the actual paint origin Y for the owning view's hit + // Stash the actual paint origin for the owning view's hit // tester. Writing it here (in prepaint) captures the - // post-layout coordinate — no sub-pixel drift later. + // post-layout coordinate, including any inset applied by the + // parent envelope — no sub-pixel drift later, no constants to + // re-apply downstream. if let Some(ref store) = self.origin_store { - store.set(Some(bounds.origin.y)); + store.set(Some(bounds.origin)); } let mut glyphs = Vec::new(); diff --git a/crates/carrot-term/src/block/active.rs b/crates/carrot-term/src/block/active.rs index bf8203e7..57965dce 100644 --- a/crates/carrot-term/src/block/active.rs +++ b/crates/carrot-term/src/block/active.rs @@ -10,6 +10,7 @@ //! Arc-wrappable, cheap to share across the render thread without locks. use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; use std::time::Instant; use carrot_grid::{ @@ -62,6 +63,18 @@ pub struct ActiveBlock { /// **sticky**, never reverts. Renderers route on this to pick /// inline (Shell) vs PinnedFooter (Tui). kind: super::kind::BlockKind, + /// Lamport-style monotonic counter. Incremented on every observable + /// mutation (grid write, atlas intern, hyperlink/grapheme/image + /// insert, metadata change, kind promotion, …). Render-side caches + /// trust this single field as the dirty signal — they re-extract a + /// snapshot iff the counter advanced since their last frame. + /// + /// Mirrors `text::Buffer::version` in Zed: one source of truth at + /// the producer, atomic to keep the VT thread lock-free against the + /// render thread, `Relaxed` ordering because the surrounding + /// `Mutex` already provides synchronization for every actual + /// mutation site. + generation: AtomicU64, } impl ActiveBlock { @@ -86,9 +99,22 @@ impl ActiveBlock { selection: None, live_frame: None, kind: super::kind::BlockKind::default(), + generation: AtomicU64::new(1), } } + /// Current generation. Render-side caches compare against their + /// last-seen value to decide whether to re-extract a snapshot. + #[inline] + pub fn generation(&self) -> u64 { + self.generation.load(Ordering::Relaxed) + } + + #[inline] + fn bump_generation(&self) { + self.generation.fetch_add(1, Ordering::Relaxed); + } + /// Lifecycle marker — `Shell` until the TUI detector promotes it. #[inline] pub fn kind(&self) -> super::kind::BlockKind { @@ -98,7 +124,11 @@ impl ActiveBlock { /// Internal write access for the TUI detector path. Promotes the /// kind in place — sticky, idempotent. pub(super) fn promote_kind_to_tui(&mut self) { + let was_tui = self.kind.is_tui(); self.kind.promote_to_tui(); + if !was_tui { + self.bump_generation(); + } } /// Access the selection slot (internal — the public surface lives @@ -110,6 +140,7 @@ impl ActiveBlock { /// Mutable access to the selection slot. pub(super) fn selection_slot_mut(&mut self) -> &mut Option { + self.bump_generation(); &mut self.selection } @@ -123,6 +154,7 @@ impl ActiveBlock { pub(super) fn live_frame_slot_mut( &mut self, ) -> &mut Option { + self.bump_generation(); &mut self.live_frame } @@ -136,7 +168,10 @@ impl ActiveBlock { /// before dispatching to the actual state machine. Truncation is /// silent; check `replay().is_truncated()` to detect it. pub fn record_bytes(&mut self, bytes: &[u8]) { - self.replay.extend(bytes); + if !bytes.is_empty() { + self.replay.extend(bytes); + self.bump_generation(); + } } /// Access the underlying page list (read-only). @@ -147,6 +182,7 @@ impl ActiveBlock { /// Mutable access to the underlying page list. The VT writer uses /// this for row-level edits (insert/delete lines, scroll region). pub fn grid_mut(&mut self) -> &mut PageList { + self.bump_generation(); &mut self.grid } @@ -157,6 +193,7 @@ impl ActiveBlock { /// Mutable style atlas for interning styles as SGR state changes. pub fn atlas_mut(&mut self) -> &mut CellStyleAtlas { + self.bump_generation(); &mut self.atlas } @@ -173,6 +210,7 @@ impl ActiveBlock { /// Mutable hyperlink store — the VT writer interns here when the /// remote emits `ESC ] 8 ; ... ; uri ST`. pub fn hyperlinks_mut(&mut self) -> &mut HyperlinkStore { + self.bump_generation(); &mut self.hyperlinks } @@ -186,6 +224,7 @@ impl ActiveBlock { /// Mutable grapheme store — VtWriter interns here when a /// zero-width combiner attaches to the previous cell. pub fn graphemes_mut(&mut self) -> &mut GraphemeStore { + self.bump_generation(); &mut self.graphemes } @@ -196,27 +235,32 @@ impl ActiveBlock { /// Mutably access metadata to set command/cwd/etc. at command-start. pub fn metadata_mut(&mut self) -> &mut ActiveMetadata { + self.bump_generation(); &mut self.metadata } /// Intern a style and return its id. See [`CellStyleAtlas::intern`]. pub fn intern_style(&mut self, style: CellStyle) -> CellStyleId { + self.bump_generation(); self.atlas.intern(style) } /// Append a row of cells to the grid. O(1) amortized. pub fn append_row(&mut self, row: &[Cell]) { self.grid.append_row(row); + self.bump_generation(); } /// Mark a specific row dirty for the next render pass. pub fn mark_dirty(&mut self, row: usize) { self.grid.mark_dirty(row); + self.bump_generation(); } /// Access the image store mutably — the VT state machine appends /// entries here when it sees image protocol sequences. pub fn images_mut(&mut self) -> &mut ImageStore { + self.bump_generation(); &mut self.images } @@ -248,3 +292,84 @@ impl ActiveBlock { )) } } + +#[cfg(test)] +mod generation_tests { + use super::*; + use carrot_grid::{Cell, CellStyle}; + + #[test] + fn generation_starts_above_zero() { + let block = ActiveBlock::new(80); + // Zero is the never-observed sentinel for cache consumers; the + // initial state is already a real value. + assert!(block.generation() > 0); + } + + #[test] + fn generation_advances_on_append_row() { + let mut block = ActiveBlock::new(4); + let g0 = block.generation(); + block.append_row(&[Cell::ascii(b'x', CellStyleId(0)); 4]); + assert!(block.generation() > g0); + } + + #[test] + fn generation_advances_on_grid_mut() { + let mut block = ActiveBlock::new(4); + let g0 = block.generation(); + let _ = block.grid_mut(); + assert!(block.generation() > g0); + } + + #[test] + fn generation_advances_on_intern_style() { + let mut block = ActiveBlock::new(4); + let g0 = block.generation(); + let _ = block.intern_style(CellStyle::DEFAULT); + assert!(block.generation() > g0); + } + + #[test] + fn generation_advances_on_record_bytes() { + let mut block = ActiveBlock::new(4); + let g0 = block.generation(); + block.record_bytes(b"hello"); + assert!(block.generation() > g0); + } + + #[test] + fn generation_holds_on_empty_record() { + let mut block = ActiveBlock::new(4); + let g0 = block.generation(); + block.record_bytes(&[]); + // Empty input is not a mutation — the cache must not invalidate. + assert_eq!(block.generation(), g0); + } + + #[test] + fn generation_advances_on_in_place_grid_writes() { + // The bug this whole counter exists to fix: write to a single + // row repeatedly, total_rows stays flat, generation must still + // advance so the render-side cache invalidates. + let mut block = ActiveBlock::new(4); + let pre_rows = block.total_rows(); + let g0 = block.generation(); + let _ = block.grid_mut(); + let _ = block.grid_mut(); + let _ = block.grid_mut(); + assert_eq!(block.total_rows(), pre_rows); + assert!(block.generation() >= g0 + 3); + } + + #[test] + fn generation_holds_on_idempotent_tui_promotion() { + let mut block = ActiveBlock::new(4); + block.promote_kind_to_tui(); + let g_after_first = block.generation(); + block.promote_kind_to_tui(); + // Already-TUI second call must not bump — the kind is sticky and + // re-promotion is a true no-op, not a state change. + assert_eq!(block.generation(), g_after_first); + } +} diff --git a/crates/carrot-term/src/block/render_view.rs b/crates/carrot-term/src/block/render_view.rs index 746558c9..fd3ce96e 100644 --- a/crates/carrot-term/src/block/render_view.rs +++ b/crates/carrot-term/src/block/render_view.rs @@ -81,13 +81,14 @@ pub struct ActiveBlockView { /// Lifecycle marker — `Shell` until promoted to `Tui` by the /// detector. Sticky for the block's lifetime. pub kind: BlockKind, - /// Monotonic frame id. Bumped every time the active block's - /// `sync_update_frame_id` advances — Layer 5 memoizes its - /// rendered snapshot keyed on `(block_id, frame_id)`. + /// Monotonic frame id sourced from `ActiveBlock::generation` — the + /// single Lamport counter the producer ticks on every observable + /// mutation. Layer 5 memoizes its rendered snapshot keyed on + /// `(block_id, frame_id)`. /// - /// Implemented as a simple content-hash-adjacent counter in - /// later commits; today we use the row count as a proxy (monotonic - /// with appends, ~identity for a given block state). + /// Was previously a row-count proxy; that mis-keyed in-place + /// updates (progress bars, `\r` overwrites, partial-line emission) + /// because the row count stayed flat while cell content advanced. pub sync_update_frame_id: u64, } @@ -151,7 +152,7 @@ fn active_view( let graphemes = Arc::new(block.graphemes().clone()); ActiveBlockView { id: entry.id, - sync_update_frame_id: snapshot.total_rows() as u64, + sync_update_frame_id: block.generation(), snapshot, hyperlinks, graphemes, diff --git a/crates/carrot-terminal-view/src/block_list.rs b/crates/carrot-terminal-view/src/block_list.rs index f967264b..f0973f2b 100644 --- a/crates/carrot-terminal-view/src/block_list.rs +++ b/crates/carrot-terminal-view/src/block_list.rs @@ -71,6 +71,13 @@ pub struct BlockListView { /// (DEC 2026 sync update or between PTY chunks). Cleared when /// either key component changes. pub(crate) last_active_render: Option<(RouterBlockId, u64, BlockSnapshot)>, + /// Last pinned-footer id we asked `BlockState` to hold, or `None` + /// when nothing was pinned. Lets `Render` skip the + /// `BlockState::pin / unpin` borrow_mut entirely when the desired + /// pin matches the live pin — pin/unpin are pseudo-idempotent on + /// the state side but still take the inner `RefCell`, which is + /// wasteful at 60–120 fps with one render per PTY tick. + pub(crate) last_pinned: Option, } impl BlockListView { @@ -94,6 +101,7 @@ impl BlockListView { search_highlights: Vec::new(), active_highlight_index: None, last_active_render: None, + last_pinned: None, } } @@ -340,16 +348,21 @@ impl Render for BlockListView { // TUI block is present we explicitly unpin — a previously // pinned TUI block that's now Shell (impossible by sticky // promotion, but defensive) or pruned would otherwise leave a - // stale pin. + // stale pin. Skip the call entirely when the desired pin + // matches the live pin — avoids a per-frame `borrow_mut` on + // `BlockState`'s inner `RefCell`. let pinned_tui = entries.iter().enumerate().rev().find_map(|(i, e)| { e.kind .is_tui() .then_some(()) .and_then(|_| self.list_ids.get(i).copied()) }); - match pinned_tui { - Some(id) => self.list_state.pin(id), - None => self.list_state.unpin(), + if pinned_tui != self.last_pinned { + match pinned_tui { + Some(id) => self.list_state.pin(id), + None => self.list_state.unpin(), + } + self.last_pinned = pinned_tui; } // Cache the v2 id mapping + layout per entry. @@ -450,6 +463,7 @@ impl Render for BlockListView { .w_full() .flex_shrink_0() .flex_col() + .px(px(BLOCK_HEADER_PAD_X)) .border_b_1() .border_color(Oklch::white().opacity(0.12)); @@ -581,8 +595,15 @@ fn render_block_entry( let error_bg = inazuma::oklcha(0.25, 0.08, 25.0, 0.15); + // Block envelope owns the horizontal inset. Both children — header + // chips and grid — render flush against the inset edge, so the + // content edge is the same x-coordinate everywhere a renderer or + // hit-tester might look at it. PTY column-budget at + // `terminal_pane.rs` and the prepainted `GridOriginStore` are then + // the only consumers of `BLOCK_HEADER_PAD_X`. div() .w_full() + .px(px(BLOCK_HEADER_PAD_X)) .when(is_selected, |d| d.bg(block_selected_bg(theme))) .when(is_error && !is_selected, |d| d.bg(error_bg)) .border_t_1() @@ -594,7 +615,6 @@ fn render_block_entry( }) .child( div() - .px(px(BLOCK_HEADER_PAD_X)) .pt(header_top_padding()) .pb(header_top_padding()) .flex() diff --git a/crates/carrot-terminal-view/src/block_list/fold.rs b/crates/carrot-terminal-view/src/block_list/fold.rs index c95f18da..74266983 100644 --- a/crates/carrot-terminal-view/src/block_list/fold.rs +++ b/crates/carrot-terminal-view/src/block_list/fold.rs @@ -46,7 +46,6 @@ pub fn render_fold_line( .flex() .flex_row() .items_center() - .px(px(BLOCK_HEADER_PAD_X)) .gap(px(8.0)) .when(is_error, |d| d.bg(error_bg)) .cursor_pointer() @@ -99,7 +98,6 @@ pub fn render_fold_counter( .w_full() .flex() .items_center() - .px(px(BLOCK_HEADER_PAD_X)) .text_size(px(11.0)) .text_color(header_metadata_fg(theme)) .cursor_pointer() diff --git a/crates/carrot-terminal-view/src/block_list/hit_test.rs b/crates/carrot-terminal-view/src/block_list/hit_test.rs index 0931dd58..8294b63d 100644 --- a/crates/carrot-terminal-view/src/block_list/hit_test.rs +++ b/crates/carrot-terminal-view/src/block_list/hit_test.rs @@ -9,7 +9,7 @@ use carrot_term::index::{Column, Line, Point, Side}; use inazuma::{Pixels, Point as GpuiPoint, px}; use crate::block_list::BlockListView; -use crate::constants::*; +use crate::constants::BLOCK_BODY_PAD_BOTTOM; impl BlockListView { /// Convert a pixel position (relative to this view) to a @@ -27,21 +27,17 @@ impl BlockListView { if let Some(bounds) = self.list_state.bounds_for_block(inazuma_id) && bounds.contains(&pos) { + // Grid origin: prefer the value the grid element wrote + // during prepaint (post-layout, includes any envelope + // padding). Fall back to a geometric estimate only when + // the element hasn't laid out yet — first-frame race. let grid_height = cell_height * entry.content_rows as f32; - let grid_start_y = entry.grid_origin_store.get().unwrap_or_else(|| { - bounds.origin.y + bounds.size.height - px(BLOCK_BODY_PAD_BOTTOM) - grid_height - }); - let y_in_grid = pos.y - grid_start_y; - - log::debug!( - "hit_test: pos.y={:.1} grid_origin={:.1} y_in_grid={:.1} \ - cell_h={:.1} visual_row={}", - f32::from(pos.y), - f32::from(grid_start_y), - f32::from(y_in_grid), - f32::from(cell_height), - (f32::from(y_in_grid) / f32::from(cell_height)) as i32, - ); + let grid_origin = entry.grid_origin_store.get().unwrap_or(GpuiPoint::new( + bounds.origin.x, + bounds.origin.y + bounds.size.height - px(BLOCK_BODY_PAD_BOTTOM) - grid_height, + )); + let y_in_grid = pos.y - grid_origin.y; + let x_in_grid = pos.x - grid_origin.x; // Click above grid = header area → block selection. if y_in_grid < px(0.0) { @@ -59,7 +55,7 @@ impl BlockListView { let row = visual_row - entry.command_row_count as i32 - entry.grid_history_size as i32; - let col_f = (f32::from(pos.x) - BLOCK_HEADER_PAD_X) / f32::from(cell_width); + let col_f = f32::from(x_in_grid) / f32::from(cell_width); let col = col_f.max(0.0) as usize; let side = if col_f.fract() < 0.5 { Side::Left diff --git a/crates/carrot-terminal-view/src/block_list/layout.rs b/crates/carrot-terminal-view/src/block_list/layout.rs index b19ba13a..3dae8b2a 100644 --- a/crates/carrot-terminal-view/src/block_list/layout.rs +++ b/crates/carrot-terminal-view/src/block_list/layout.rs @@ -8,14 +8,8 @@ use std::cell::Cell; use std::rc::Rc; +pub(crate) use carrot_block_render::GridOriginStore; use carrot_term::BlockId; -use inazuma::Pixels; - -/// Shared slot that `TerminalGridElement` writes the grid's Y origin -/// into during prepaint, and `hit_test` reads during input handling. -/// `None` = the element hasn't laid out yet this frame, so the caller -/// falls back to a geometric estimate from the block's bounds. -pub(crate) type GridOriginStore = Rc>>; /// Cached layout info for one block — used for pixel→grid hit testing. #[derive(Clone)] @@ -25,9 +19,10 @@ pub(crate) struct BlockLayoutEntry { pub(crate) content_rows: usize, pub(crate) command_row_count: usize, pub(crate) grid_history_size: usize, - /// Shared store: the grid element writes actual grid origin Y - /// during prepaint, hit_test reads it for exact pixel→row mapping - /// without sub-pixel drift. + /// Shared slot the grid element writes its actual paint-time origin + /// into during prepaint. Hit-test reads both x and y from this slot, + /// so layout-side padding tokens never have to be re-applied + /// downstream. pub(crate) grid_origin_store: GridOriginStore, } From 268cec4a1ebf5ccee8bdd00d8eef8d0055d8bf61 Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 10:50:40 +0200 Subject: [PATCH 2/8] fix(blocks): clip topmost entry when content overflows under FollowTail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores the overshoot offset that the Inazuma `BlockState` port lost relative to its Zed `gpui::list` ancestor. When the active block streams enough output to exceed the viewport, the layout pass now sets `scroll_top.offset_in_entry = rendered_height - main_available_height` on the topmost visible entry, so the BOTTOM of that entry coincides with the bottom edge of the viewport — instead of the top of the entry pinning to the top edge and new rows scrolling off the bottom. This is the regression behind the user-visible "running command's output slides down out of view, scrollbar drifts up" symptom on multi-second streaming output (e.g. the for-pkg-in-… loop that emits ~50 lines of `==> Fetching … / Verifying … / Compiling …` over a minute). Mirrors `gpui::list::layout` at `.reference/zed/crates/gpui/src/elements/list.rs:821-838`. The port was line-for-line accurate elsewhere but dropped exactly this assignment, defaulting `offset_in_entry` to `px(0.0)` regardless of whether content fit or overflowed. Regression test --------------- `elements::block::tests::follow_tail_clips_topmost_entry_when_content_overflows`: single 1000-px entry, viewport 400 px. After the layout pass, `bounds_for_block(id)` reports `top = -600 px` and `bottom = 400 px` — the entry's bottom edge lines up exactly with the viewport bottom, overshoot of 600 px clipped from above. --- crates/inazuma/src/elements/block/layout.rs | 16 +++++++++++- crates/inazuma/src/elements/block/tests.rs | 28 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/crates/inazuma/src/elements/block/layout.rs b/crates/inazuma/src/elements/block/layout.rs index 4a97a02b..fd5b5b75 100644 --- a/crates/inazuma/src/elements/block/layout.rs +++ b/crates/inazuma/src/elements/block/layout.rs @@ -145,9 +145,23 @@ pub(super) fn layout_entries( }); } + // When the rendered content overflows the viewport, the topmost + // entry has to be clipped from above by exactly the overshoot — + // otherwise the LAST visible rows of the LAST entry slide off + // the bottom edge as new content streams in. The clip lives on + // the first entry's `offset_in_entry`; the per-entry paint loop + // in `element.rs` consumes it via `start_y - offset_in_entry`. + // + // Mirrors Zed's `gpui::list::layout` + // (`.reference/zed/crates/gpui/src/elements/list.rs:821-838`): + // ports are line-for-line, but the overshoot assignment was + // dropped on the way in. Recovering it fixes the + // "active block taller than viewport ⇒ content scrolls out the + // bottom while the user watches" regression. + let overshoot = (rendered_height - main_available_height).max(px(0.0)); scroll_top = BlockOffset { entry_ix: cursor.start().0, - offset_in_entry: px(0.0), + offset_in_entry: overshoot, }; if rendered_height < main_available_height { diff --git a/crates/inazuma/src/elements/block/tests.rs b/crates/inazuma/src/elements/block/tests.rs index 7281778a..5244d88a 100644 --- a/crates/inazuma/src/elements/block/tests.rs +++ b/crates/inazuma/src/elements/block/tests.rs @@ -133,6 +133,34 @@ fn follow_tail_sticks_to_tail_on_new_entries(cx: &mut TestAppContext) { assert_eq!(scroll.entry_ix, 6); } +#[inazuma::test] +fn follow_tail_clips_topmost_entry_when_content_overflows(cx: &mut TestAppContext) { + // Regression: with `VisualAnchor::Bottom` + `FollowTail`, when the + // last entry alone is taller than the viewport, the layout pass must + // clip the topmost visible entry from above by the overshoot — not + // pin it to the top edge. Otherwise newly-streaming content inside a + // tall active block scrolls off the bottom of the viewport while the + // user watches. Verified through `bounds_for_block`: the entry's + // bottom edge must coincide with the viewport's bottom edge, and + // its top edge must sit `overshoot` pixels above the viewport top. + let mut cx = cx.add_empty_window(); + let state = BlockState::new( + BlockConfig::default() + .visual_anchor(VisualAnchor::Bottom) + .scroll_behavior(ScrollBehavior::FollowTail) + .measuring_behavior(BlockMeasuringBehavior::All), + ); + let id = state.push(BlockMetadata::default(), None); + let height = Rc::new(Cell::new(1000.0)); + draw_view(&mut cx, &state, &height, size(px(100.), px(400.))); + + let bounds = state.bounds_for_block(id).expect("entry is in viewport"); + // Bottom of the only entry sits exactly on the bottom edge. + assert_eq!(bounds.bottom(), px(400.)); + // Top of the only entry is 600 px above the viewport top. + assert_eq!(bounds.top(), px(-600.)); +} + #[inazuma::test] fn manual_scroll_breaks_follow_tail(cx: &mut TestAppContext) { let mut cx = cx.add_empty_window(); From 1994d1b104c972d955b0acfdd2b8eca6ced2ab4d Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 18:43:08 +0200 Subject: [PATCH 3/8] fix(blocks): refresh window after scroll mutation so trackpad runs smooth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BlockState::scroll` updated `logical_scroll_top` on every wheel event but never told the window to repaint. The new scroll position only reached the GPU on the next unrelated frame trigger — a PTY tick, a hover event, a focus change. Trackpad motion looked chunky because every other event landed before the next render did, and the viewport snapped forward in coarse steps. Inazuma's `BlockState` is a near-clone of Zed's `gpui::list::ListState` but the port lost the `cx.notify(current_view)` call at the end of `scroll` (`.reference/zed/crates/gpui/src/elements/list.rs:634`). Without an Entity to notify, the equivalent here is calling `window.refresh()` from the wheel-event closure right after the state mutation — `Window::refresh` flips the dirty bit on the current window so the next frame paints from the new scroll position, no EntityId plumbing needed. --- crates/inazuma/src/elements/block/element.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/inazuma/src/elements/block/element.rs b/crates/inazuma/src/elements/block/element.rs index 776b1fe3..871487d9 100644 --- a/crates/inazuma/src/elements/block/element.rs +++ b/crates/inazuma/src/elements/block/element.rs @@ -175,7 +175,14 @@ impl Element for Block { block_state .0 .borrow_mut() - .scroll(&scroll_top, height, pixel_delta) + .scroll(&scroll_top, height, pixel_delta); + // Mutating the inner state alone does not invalidate the + // window — without this `refresh()` the new scroll + // position only reaches the GPU on the next unrelated + // frame trigger (PTY tick, window event, …), so trackpad + // motion looks chunky. Mirrors Zed's `cx.notify(view)` + // at the end of `gpui::list::scroll`. + window.refresh(); } }); } From 8ecdf41ae8635c48471e0ef07c7d6f7fc339f98b Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 19:34:54 +0200 Subject: [PATCH 4/8] fix(blocks): wire terminal::Clear so Cmd+K actually empties the list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent gaps held the keymap shortcut hostage. All keymap files (`assets/keymaps/default-{macos,linux,windows}.toml`) bound `cmd-k` / `ctrl-shift-k` / `ctrl-l` to `terminal::Clear`, but the action was never declared, never handled, and the closest existing `BlockListView::clear()` resolved to a no-op against the visible blocks. Pressing Cmd+K silently did nothing. Architectural fix in three layers, each owning one invariant. Layer 2 — producer (`carrot_term::block::BlockRouter`) ------------------------------------------------------ Add `BlockRouter::clear()`. Drains the `blocks` Vec, clears `active_id` + `pending_command`, recreates a fresh prompt buffer, zeroes the display scroll state. `next_id` stays monotonic so any external observer holding an old id can detect the rotation cleanly. Layer 3 — view-state primitive (`inazuma::BlockState`) ------------------------------------------------------ Add `BlockState::clear()`. Drops the sumtree entries, the id-to-index map, the pinned-footer slot, scroll cache, and resets `follow_tail` to its config-derived initial value. Without this the old ids would survive in the sumtree even after the router's underlying entries are gone, and the next render's `bounds_for_block` would resolve to garbage. Layer 5 — feature wiring (`carrot-terminal-view`) ------------------------------------------------- - `terminal_panel.rs`: declare the missing `Clear` action in the `terminal` namespace (alongside `NewTerminal`, `OpenTerminal`, `ToggleFocus`, `Toggle`, `SendInterrupt`). - `terminal_pane/lifecycle.rs`: `on_clear` handler that calls `block_list.clear()` and notifies. - `terminal_pane.rs`: register the action via `.on_action(cx.listener(Self::on_clear))` next to the existing `on_send_interrupt` registration. - `block_list.rs::BlockListView::clear`: drop every consumer-side cache that holds block ids — `list_state.clear()`, the local `list_ids` / `router_ids` / `block_layout`, plus `last_active_render`, `last_pinned`, search and selection state. Tests cover the three new entry points: producer drops every block, state primitive resets every cache, and id monotonicity survives clear so a fresh push after clear is index 0 with a strictly greater id. --- crates/carrot-term/src/block/router.rs | 63 +++++++++++++++++++ crates/carrot-terminal-view/src/block_list.rs | 31 ++++++--- .../carrot-terminal-view/src/terminal_pane.rs | 1 + .../src/terminal_pane/lifecycle.rs | 11 ++++ .../src/terminal_panel.rs | 7 ++- crates/inazuma/src/elements/block/state.rs | 21 +++++++ crates/inazuma/src/elements/block/tests.rs | 33 ++++++++++ 7 files changed, 158 insertions(+), 9 deletions(-) diff --git a/crates/carrot-term/src/block/router.rs b/crates/carrot-term/src/block/router.rs index 3e6dcc1f..02bed0d5 100644 --- a/crates/carrot-term/src/block/router.rs +++ b/crates/carrot-term/src/block/router.rs @@ -276,6 +276,22 @@ impl BlockRouter { self.prompt = ActiveBlock::new(self.cols); } + /// User-initiated full reset (Cmd+K / Ctrl+L). Drops every frozen + /// block, ends any in-flight active block, resets the prompt + /// buffer, and zeroes the display scroll state. `next_id` stays + /// monotonic so any external observers that cached an old id + /// continue to detect the rotation cleanly. + /// + /// The router is left in the same shape as right after `new()` + /// except for the preserved `next_id` and `cols`. + pub fn clear(&mut self) { + self.blocks.clear(); + self.active_id = None; + self.pending_command = None; + self.prompt = ActiveBlock::with_page_bytes(self.cols, self.page_bytes); + self.display = super::display::DisplayState::new(); + } + /// `OSC 133 ; C` — command execution started. Allocates a new /// active block, wires it as the routing target, returns its /// freshly-minted ID. @@ -642,4 +658,51 @@ mod tests { m.exit_code = Some(-1); assert!(m.is_error(), "any nonzero code is error"); } + + #[test] + fn clear_drops_all_blocks_and_resets_active() { + let mut r = BlockRouter::new(80); + r.on_command_start(); + r.on_command_end(0); + r.on_command_start(); + r.on_command_end(0); + r.on_command_start(); + assert!(r.has_active_block()); + assert_eq!(r.len(), 3); + + r.clear(); + assert_eq!(r.len(), 0); + assert!(!r.has_active_block()); + assert!(r.entries().is_empty()); + } + + #[test] + fn clear_keeps_id_counter_monotonic() { + let mut r = BlockRouter::new(80); + let id_a = r.on_command_start(); + r.on_command_end(0); + r.clear(); + let id_b = r.on_command_start(); + assert!( + id_b.0 > id_a.0, + "next id after clear must not collide with pre-clear ids \ + (a={}, b={})", + id_a.0, + id_b.0, + ); + } + + #[test] + fn clear_resets_pending_command_too() { + let mut r = BlockRouter::new(80); + r.set_pending_command("cargo build"); + r.clear(); + let id = r.on_command_start(); + let entry = r.entry(id).expect("active block"); + assert!( + entry.metadata.command.is_none(), + "stale pending command leaked across clear: {:?}", + entry.metadata.command, + ); + } } diff --git a/crates/carrot-terminal-view/src/block_list.rs b/crates/carrot-terminal-view/src/block_list.rs index f0973f2b..4ad04bca 100644 --- a/crates/carrot-terminal-view/src/block_list.rs +++ b/crates/carrot-terminal-view/src/block_list.rs @@ -116,20 +116,35 @@ impl BlockListView { (font, font_size, line_height, symbol_maps) } - /// Clear all blocks. The v2 router doesn't expose a bulk-clear - /// API yet, so we drive a fresh prompt cycle (which empties the - /// router on the next on_prompt_start) and drop our local state. + /// Clear all blocks. Drops every frozen entry plus any in-flight + /// active block, resets the prompt buffer, zeroes scroll state, + /// and clears local view-side caches (selection, search + /// highlights, snapshot memoization, pin tracking, layout cache). + /// + /// Triggered by the user-facing `terminal::Clear` action + /// (Cmd+K / Ctrl+L) — the room visibly empties out and the next + /// shell prompt lands on a blank scrollback. pub fn clear(&mut self) { let handle = self.terminal.clone(); let mut term = handle.lock(); - // Switching to prompt resets the active id without evicting - // frozen entries. The renderer re-reads entries every frame - // from the router, so dropping our cache is enough to stop - // showing stale rows. - term.block_router_mut().on_prompt_start(); + term.block_router_mut().clear(); drop(term); + + // Producer-side cleared. Mirror the wipe in every consumer + // that holds block ids: the inazuma `BlockState` (sumtree + + // id-to-index map), the local id mappings, layout cache, + // selection / search / pin / snapshot caches. + self.list_state.clear(); + self.list_ids.clear(); + self.router_ids.clear(); + self.block_layout.clear(); self.selected_block = None; self.selecting_block = None; + self.pending_block_toggle = None; + self.search_highlights.clear(); + self.active_highlight_index = None; + self.last_active_render = None; + self.last_pinned = None; } /// Get the selected block index. diff --git a/crates/carrot-terminal-view/src/terminal_pane.rs b/crates/carrot-terminal-view/src/terminal_pane.rs index dac98515..95dfda5f 100644 --- a/crates/carrot-terminal-view/src/terminal_pane.rs +++ b/crates/carrot-terminal-view/src/terminal_pane.rs @@ -479,6 +479,7 @@ impl Render for TerminalPane { .bg(bg_color) .track_focus(&self.focus_handle) .on_action(cx.listener(Self::on_send_interrupt)) + .on_action(cx.listener(Self::on_clear)) .on_action(cx.listener(Self::on_insert_into_input)) .on_key_down(cx.listener(Self::on_key_down_interactive)); diff --git a/crates/carrot-terminal-view/src/terminal_pane/lifecycle.rs b/crates/carrot-terminal-view/src/terminal_pane/lifecycle.rs index 3e3156ad..9ef2c1e9 100644 --- a/crates/carrot-terminal-view/src/terminal_pane/lifecycle.rs +++ b/crates/carrot-terminal-view/src/terminal_pane/lifecycle.rs @@ -38,6 +38,17 @@ impl TerminalPane { cx.notify(); } + /// Wipe the visible block list (Cmd+K / Ctrl+L). + pub(crate) fn on_clear( + &mut self, + _: &crate::terminal_panel::Clear, + _window: &mut Window, + cx: &mut Context, + ) { + self.block_list.update(cx, |list, _cx| list.clear()); + cx.notify(); + } + /// Paste a command line into the input editor without executing. Used /// by the command palette's History source (`Cmd+R`) so recalling a /// past command gives the user a chance to edit before hitting enter. diff --git a/crates/carrot-terminal-view/src/terminal_panel.rs b/crates/carrot-terminal-view/src/terminal_panel.rs index fd4660ed..4e2e2adb 100644 --- a/crates/carrot-terminal-view/src/terminal_panel.rs +++ b/crates/carrot-terminal-view/src/terminal_panel.rs @@ -164,6 +164,11 @@ actions!( OpenTerminal, ToggleFocus, Toggle, - SendInterrupt + SendInterrupt, + /// User-initiated "clear all blocks" — bound to Cmd+K and Ctrl+L + /// in the default keymaps. Empties the block router, resets + /// the prompt buffer, drops every consumer-side cache, leaves + /// the next shell prompt on a blank scrollback. + Clear ] ); diff --git a/crates/inazuma/src/elements/block/state.rs b/crates/inazuma/src/elements/block/state.rs index 6d4f2135..e053ee30 100644 --- a/crates/inazuma/src/elements/block/state.rs +++ b/crates/inazuma/src/elements/block/state.rs @@ -243,6 +243,27 @@ impl BlockState { id } + /// Drop every block. Resets entries, id-to-index map, pinned + /// footer slot, scroll position, and layout caches. `next_id` + /// stays monotonic so any external observer that cached an old + /// id detects the rotation cleanly. `follow_tail` is restored to + /// its initial value derived from `scroll_behavior`. + /// + /// Used by view-level "clear all" actions (e.g. terminal Cmd+K) + /// when the producer has already dropped its underlying entries + /// and the list state has to follow. + pub fn clear(&self) { + let mut state = self.0.borrow_mut(); + state.entries = SumTree::default(); + state.id_to_ix.clear(); + state.pinned_footer = None; + state.logical_scroll_top = None; + state.last_resolved_scroll_top = None; + state.last_leading_space = px(0.0); + state.measured_all = false; + state.follow_tail = matches!(state.config.scroll_behavior, ScrollBehavior::FollowTail); + } + /// Remove the block with the given id. No-op if the id is not known. /// If the removed block was the pinned footer, the pin is cleared /// — leaving a stale `pinned_footer` id pointing at a removed entry diff --git a/crates/inazuma/src/elements/block/tests.rs b/crates/inazuma/src/elements/block/tests.rs index 5244d88a..714c6aff 100644 --- a/crates/inazuma/src/elements/block/tests.rs +++ b/crates/inazuma/src/elements/block/tests.rs @@ -161,6 +161,39 @@ fn follow_tail_clips_topmost_entry_when_content_overflows(cx: &mut TestAppContex assert_eq!(bounds.top(), px(-600.)); } +#[inazuma::test] +fn clear_drops_every_block_and_resets_caches(cx: &mut TestAppContext) { + // `BlockState::clear` is the consumer-side mirror of + // `BlockRouter::clear`. Both have to drop every entry, otherwise + // the next render reads stale ids out of the sumtree and hits + // the unwrap-or-default branch in layout. + let mut cx = cx.add_empty_window(); + let state = BlockState::new( + BlockConfig::default() + .visual_anchor(VisualAnchor::Bottom) + .scroll_behavior(ScrollBehavior::FollowTail), + ); + for _ in 0..5 { + state.push(BlockMetadata::default(), None); + } + let height = Rc::new(Cell::new(80.0)); + draw_view(&mut cx, &state, &height, size(px(100.), px(400.))); + assert_eq!(state.entry_count(), 5); + + state.clear(); + assert_eq!(state.entry_count(), 0); + + // After clear, push a fresh entry and confirm it lands at index 0 + // — id-to-index map was reset. + let id = state.push(BlockMetadata::default(), None); + draw_view(&mut cx, &state, &height, size(px(100.), px(400.))); + assert_eq!(state.entry_count(), 1); + let bounds = state + .bounds_for_block(id) + .expect("freshly-pushed entry is visible after clear"); + assert!(bounds.bottom() <= px(400.)); +} + #[inazuma::test] fn manual_scroll_breaks_follow_tail(cx: &mut TestAppContext) { let mut cx = cx.add_empty_window(); From 13442f9c8e3afdf070566ab2e2b14729a5c49ba1 Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 20:02:56 +0200 Subject: [PATCH 5/8] fix(blocks): de-duplicate terminal::Clear by moving terminal actions home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous commit hit `Action with name 'terminal::Clear' already registered` at startup because `crates/carrot-app/src/main.rs` already declared `terminal::Clear` (alongside Copy / Paste / NewTab / ScrollPageUp / Find / etc.) and my addition in `carrot-terminal-view/src/terminal_panel.rs` was a second declaration of the same name. Two architectural rules are at stake: - CLAUDE.md "no code in carrot-app outside main.rs bootstrap and Global registration" — action declarations are feature types, not bootstrap. - CLAUDE.md "Feature Crates never depend on `carrot-app`" — meaning the terminal-view crate cannot import the type from main.rs even if we wanted to share. The action has to live where its handler lives. Moving the entire `actions!(terminal, [...])` block from `main.rs` into `terminal_panel.rs` resolves both: every `terminal::*` action is now declared exactly once, in the crate that owns the surface, and the handler I added in the previous commit keeps working unchanged (`crate::terminal_panel::Clear` resolves to the same struct, no duplicate registration). The eleven actions moved (`Clear`, `Copy`, `Paste`, `NewTab`, `NextTab`, `PreviousTab`, `ScrollPageUp`, `ScrollPageDown`, `ScrollToTop`, `ScrollToBottom`, `Find`) all flow through the keymap by string name — none are imported as a Rust type from any other crate, so the move is internally invisible. --- crates/carrot-app/src/main.rs | 20 +++------------- .../src/terminal_panel.rs | 23 ++++++++++++++++++- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/crates/carrot-app/src/main.rs b/crates/carrot-app/src/main.rs index bcc3e314..b2de06e6 100644 --- a/crates/carrot-app/src/main.rs +++ b/crates/carrot-app/src/main.rs @@ -30,23 +30,9 @@ actions!( ] ); -// Terminal actions -actions!( - terminal, - [ - Copy, - Paste, - Clear, - NewTab, - NextTab, - PreviousTab, - ScrollPageUp, - ScrollPageDown, - ScrollToTop, - ScrollToBottom, - Find, - ] -); +// `terminal::*` actions live in `carrot_terminal_view::terminal_panel` +// — they own the surface those keystrokes act on, and Layer-4 +// `carrot-app` is bootstrap-only per the architecture rules. // Input actions (Carrot Mode) actions!( diff --git a/crates/carrot-terminal-view/src/terminal_panel.rs b/crates/carrot-terminal-view/src/terminal_panel.rs index 4e2e2adb..e79c309d 100644 --- a/crates/carrot-terminal-view/src/terminal_panel.rs +++ b/crates/carrot-terminal-view/src/terminal_panel.rs @@ -169,6 +169,27 @@ actions!( /// in the default keymaps. Empties the block router, resets /// the prompt buffer, drops every consumer-side cache, leaves /// the next shell prompt on a blank scrollback. - Clear + Clear, + /// Copy the current selection to the clipboard. + Copy, + /// Paste the clipboard contents into the focused terminal. + Paste, + /// Open a new terminal tab in the focused workspace. + NewTab, + /// Move focus to the next terminal tab. + NextTab, + /// Move focus to the previous terminal tab. + PreviousTab, + /// Scroll the block list one page up. + ScrollPageUp, + /// Scroll the block list one page down. + ScrollPageDown, + /// Scroll to the top of the block list. + ScrollToTop, + /// Scroll to the bottom of the block list, re-engaging + /// follow-tail. + ScrollToBottom, + /// Open the in-pane find bar. + Find, ] ); From 1ece757fd8613a5f4119044a3f55f9e9cd6ae54b Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 21:06:20 +0200 Subject: [PATCH 6/8] =?UTF-8?q?fix(blocks):=20drop=20silent-success=20comm?= =?UTF-8?q?ands=20in=20the=20router=20(cd,=20touch,=20=E2=80=A6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cd Projects` followed by `cd carrot` left two empty-header blocks in the scrollback because every command — even those that produce no output and exit cleanly — was frozen as a permanent entry. The user sees a wall of placeholder headers for commands that are by their nature invisible. Architectural choice: handle this at the producer (`carrot_term::block::BlockRouter`) rather than as a render-side filter. The producer owns the lifecycle; filtering at the consumer would force render, search, history, and replay to each maintain their own copy of the same predicate, with drift potential whenever the rule changes. The rule lives in `BlockRouter::on_command_end`. Before the freeze swap, the active entry is checked against three conjuncts: zero output rows, exit code 0, kind not promoted to Tui. If all hold, the entry is removed from `self.blocks` and `on_command_end` returns `None`. Failures (any non-zero exit) keep the entry no matter how empty — the user still wants to see the command failed. TUI sessions keep the entry no matter how empty — the alt-screen buffer is separate from the persistent grid, but the session itself was meaningful. `next_id` stays monotonic across the drop: any external observer that cached the dropped id detects the rotation cleanly and never sees the id reused. Tests ----- Four new regression tests in `block::router::tests`: - `silent_success_command_drops_entry`: cd-style command leaves no trace. - `silent_failure_keeps_entry`: empty stderr but non-zero exit stays. - `produced_output_keeps_entry_even_on_zero_exit`: one row of stdout is enough to keep an entry. - `tui_session_keeps_entry_even_when_grid_is_empty`: alt-screen promotion sticks across the freeze decision. - `silent_success_keeps_id_counter_monotonic`: dropped ids do not get reused. Existing tests that drove `on_command_end(0)` with no output now either push a row first (when the test is about freeze / eviction / metadata behavior) or use `on_command_end(1)` (when it is specifically about the no-output case but still needs a frozen entry for the assertion). Helper `produce_row(&mut router)` factored out for the common "this isn't a silent-success scenario" setup. --- crates/carrot-term/src/block/render_view.rs | 7 ++ crates/carrot-term/src/block/replay.rs | 5 +- crates/carrot-term/src/block/router.rs | 130 +++++++++++++++++++- 3 files changed, 138 insertions(+), 4 deletions(-) diff --git a/crates/carrot-term/src/block/render_view.rs b/crates/carrot-term/src/block/render_view.rs index fd3ce96e..e7088a72 100644 --- a/crates/carrot-term/src/block/render_view.rs +++ b/crates/carrot-term/src/block/render_view.rs @@ -199,10 +199,17 @@ mod tests { #[test] fn frozen_blocks_survive_snapshot() { + use carrot_grid::Cell; let mut r = BlockRouter::new(40); r.on_command_start(); + // One row of output — without it the silent-success rule in + // `on_command_end` would drop the entry. + if let crate::block::ActiveTarget::Block { block, .. } = r.active() { + block.append_row(&[Cell::default()]); + } r.on_command_end(0); r.on_command_start(); + // Exit 1 — failures stay regardless of empty output. r.on_command_end(1); let state = VtWriterState::new(40, 24); let display = DisplayState::new(); diff --git a/crates/carrot-term/src/block/replay.rs b/crates/carrot-term/src/block/replay.rs index 00dd353d..d556c830 100644 --- a/crates/carrot-term/src/block/replay.rs +++ b/crates/carrot-term/src/block/replay.rs @@ -301,7 +301,10 @@ mod tests { processor.advance(&mut writer, b""); writer.finalize(); } - let frozen = router.on_command_end(0).expect("frozen block"); + // Non-zero exit so the silent-success rule in + // `on_command_end` doesn't drop the entry — this test wants a + // frozen handle with an empty replay buffer, not a dropped one. + let frozen = router.on_command_end(1).expect("frozen block"); assert!(replay_frozen_block(&frozen, 20).is_none()); } } diff --git a/crates/carrot-term/src/block/router.rs b/crates/carrot-term/src/block/router.rs index 02bed0d5..a25fb98d 100644 --- a/crates/carrot-term/src/block/router.rs +++ b/crates/carrot-term/src/block/router.rs @@ -314,11 +314,35 @@ impl BlockRouter { /// Freezes the active block and returns its `Arc` /// for consumers (render thread, replay, scrollback search). /// + /// **Silent-success drop**: if the command produced zero output + /// rows AND exited with code `0` AND never promoted to a TUI + /// session, the entry is removed from the router instead of + /// frozen — `cd`, `touch`, `export FOO=bar`, ASSIGN-only commands, + /// etc. leave no visual record. Failures, TUI sessions, and any + /// command that wrote even a single row keep their entry. Single + /// invariant lives in the producer; consumers (render, search, + /// history, replay) never see the suppressed entries and never + /// have to filter independently. + /// /// Returns `None` if there was no active block (e.g. a spurious - /// `;D` without a preceding `;C`). + /// `;D` without a preceding `;C`) or if the silent-success rule + /// dropped this one. pub fn on_command_end(&mut self, exit_code: i32) -> Option> { let id = self.active_id.take()?; - let entry = self.blocks.iter_mut().find(|e| e.id == id)?; + let pos = self.blocks.iter().position(|e| e.id == id)?; + + // Silent-success path — remove the entry outright before + // committing exit metadata or building a Frozen wrapper. The + // active grid is dropped on the way out; nothing references + // its replay buffer either. + let drop_silently = matches!(&self.blocks[pos].variant, BlockVariant::Active(active) + if exit_code == 0 && active.total_rows() == 0 && !active.kind().is_tui()); + if drop_silently { + self.blocks.remove(pos); + return None; + } + + let entry = &mut self.blocks[pos]; entry.metadata.exit_code = Some(exit_code); entry.metadata.finished_at = Some(Instant::now()); // Swap the variant out, call `finish`, store the Frozen back. @@ -489,6 +513,16 @@ impl<'a> ActiveTarget<'a> { mod tests { use super::*; + /// Push one row of output into the active block before ending it, + /// so `on_command_end` doesn't apply the silent-success drop. Most + /// router tests want to exercise the freeze/eviction/metadata path, + /// not the silent-success one. + fn produce_row(r: &mut BlockRouter) { + if let ActiveTarget::Block { block, .. } = r.active() { + block.append_row(&[Cell::default()]); + } + } + #[test] fn new_router_has_no_active_block() { let r = BlockRouter::new(80); @@ -523,6 +557,7 @@ mod tests { fn command_end_freezes_block_and_clears_active() { let mut r = BlockRouter::new(80); let id = r.on_command_start(); + produce_row(&mut r); let frozen = r.on_command_end(0).expect("frozen block on end"); assert_eq!(Arc::strong_count(&frozen), 2); // router + caller assert!(!r.has_active_block()); @@ -551,6 +586,7 @@ mod tests { let mut r = BlockRouter::with_limits(80, RouterLimits { max_blocks: 3 }); for _ in 0..5 { r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); } assert_eq!(r.len(), 3); @@ -563,8 +599,10 @@ mod tests { fn eviction_never_drops_active_block() { let mut r = BlockRouter::with_limits(80, RouterLimits { max_blocks: 2 }); r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); // Now start a new one without ending it, then push another // end to overflow. @@ -593,6 +631,7 @@ mod tests { fn metadata_attaches_to_last() { let mut r = BlockRouter::new(80); r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); r.set_last_metadata(RouterBlockMetadata { cwd: Some("/tmp".into()), @@ -635,13 +674,15 @@ mod tests { let mut r = BlockRouter::new(10); r.on_prompt_start(); r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); r.on_prompt_start(); r.on_command_start(); // One frozen, one active. assert_eq!(r.frozen_entries().count(), 1); assert_eq!(r.active_entries().count(), 1); - // Finish the second — now two frozen, zero active. + // Finish the second — now two frozen, zero active. Exit 1 + // keeps the entry visible regardless of empty output. r.on_command_end(1); assert_eq!(r.frozen_entries().count(), 2); assert_eq!(r.active_entries().count(), 0); @@ -663,8 +704,10 @@ mod tests { fn clear_drops_all_blocks_and_resets_active() { let mut r = BlockRouter::new(80); r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); r.on_command_start(); assert!(r.has_active_block()); @@ -680,6 +723,7 @@ mod tests { fn clear_keeps_id_counter_monotonic() { let mut r = BlockRouter::new(80); let id_a = r.on_command_start(); + produce_row(&mut r); r.on_command_end(0); r.clear(); let id_b = r.on_command_start(); @@ -705,4 +749,84 @@ mod tests { entry.metadata.command, ); } + + #[test] + fn silent_success_command_drops_entry() { + // `cd Projects` / `touch x` / `export FOO=bar` — no stdout, + // exit 0. Router should drop the entry outright. + let mut r = BlockRouter::new(80); + r.on_command_start(); + assert_eq!(r.len(), 1, "active block exists pre-end"); + let frozen = r.on_command_end(0); + assert!( + frozen.is_none(), + "silent success must not produce a Frozen handle", + ); + assert_eq!( + r.len(), + 0, + "silent-success entry must be removed from the router", + ); + assert!(!r.has_active_block()); + } + + #[test] + fn silent_failure_keeps_entry() { + // Rare but real: a command exits non-zero with no stderr. + // The user still wants to see it failed. + let mut r = BlockRouter::new(80); + r.on_command_start(); + let frozen = r.on_command_end(1); + assert!( + frozen.is_some(), + "non-zero exit with no output must still freeze", + ); + assert_eq!(r.len(), 1); + } + + #[test] + fn produced_output_keeps_entry_even_on_zero_exit() { + let mut r = BlockRouter::new(80); + r.on_command_start(); + produce_row(&mut r); + let frozen = r.on_command_end(0); + assert!(frozen.is_some()); + assert_eq!(r.len(), 1); + } + + #[test] + fn tui_session_keeps_entry_even_when_grid_is_empty() { + // Alt-screen TUI sessions promote `kind` to `Tui`; their + // persistent grid is often empty when they exit, but the + // session itself was meaningful — keep it. + use crate::block::LiveFrameSource; + let mut r = BlockRouter::new(80); + r.on_command_start(); + if let ActiveTarget::Block { block, .. } = r.active() { + block.activate_live_frame(0, 1, LiveFrameSource::Heuristic); + } + let frozen = r.on_command_end(0); + assert!( + frozen.is_some(), + "TUI session must not be silently dropped on exit 0", + ); + assert_eq!(r.len(), 1); + } + + #[test] + fn silent_success_keeps_id_counter_monotonic() { + // Suppressing the entry should NOT roll back `next_id`. + // Otherwise an external observer that cached the dropped id + // would see it reused on the next command. + let mut r = BlockRouter::new(80); + let dropped = r.on_command_start(); + r.on_command_end(0); + let next = r.on_command_start(); + assert!( + next.0 > dropped.0, + "next id ({}) must exceed the dropped id ({})", + next.0, + dropped.0, + ); + } } From 271e2c970e458268d0d48df26e1f9fc954087b76 Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 21:23:07 +0200 Subject: [PATCH 7/8] =?UTF-8?q?Revert=20"fix(blocks):=20drop=20silent-succ?= =?UTF-8?q?ess=20commands=20in=20the=20router=20(cd,=20touch,=20=E2=80=A6)?= =?UTF-8?q?"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 1ece757. Wrong call. Looking at Warp side-by-side, silent-success commands DO keep their block — what makes them feel "non-empty" is that Warp renders the **command text itself** inside the block envelope, right between the chip header and the output grid. Carrot was rendering neither side of the empty: no command echo, no output, just the chips. Dropping the entry fixed the symptom but lost the user's record of what they ran. Wrong layer for the fix. The real fix lands in the next commit (render the command text). --- crates/carrot-term/src/block/render_view.rs | 7 - crates/carrot-term/src/block/replay.rs | 5 +- crates/carrot-term/src/block/router.rs | 130 +----------------- crates/carrot-terminal-view/src/block_list.rs | 18 +++ 4 files changed, 22 insertions(+), 138 deletions(-) diff --git a/crates/carrot-term/src/block/render_view.rs b/crates/carrot-term/src/block/render_view.rs index e7088a72..fd3ce96e 100644 --- a/crates/carrot-term/src/block/render_view.rs +++ b/crates/carrot-term/src/block/render_view.rs @@ -199,17 +199,10 @@ mod tests { #[test] fn frozen_blocks_survive_snapshot() { - use carrot_grid::Cell; let mut r = BlockRouter::new(40); r.on_command_start(); - // One row of output — without it the silent-success rule in - // `on_command_end` would drop the entry. - if let crate::block::ActiveTarget::Block { block, .. } = r.active() { - block.append_row(&[Cell::default()]); - } r.on_command_end(0); r.on_command_start(); - // Exit 1 — failures stay regardless of empty output. r.on_command_end(1); let state = VtWriterState::new(40, 24); let display = DisplayState::new(); diff --git a/crates/carrot-term/src/block/replay.rs b/crates/carrot-term/src/block/replay.rs index d556c830..00dd353d 100644 --- a/crates/carrot-term/src/block/replay.rs +++ b/crates/carrot-term/src/block/replay.rs @@ -301,10 +301,7 @@ mod tests { processor.advance(&mut writer, b""); writer.finalize(); } - // Non-zero exit so the silent-success rule in - // `on_command_end` doesn't drop the entry — this test wants a - // frozen handle with an empty replay buffer, not a dropped one. - let frozen = router.on_command_end(1).expect("frozen block"); + let frozen = router.on_command_end(0).expect("frozen block"); assert!(replay_frozen_block(&frozen, 20).is_none()); } } diff --git a/crates/carrot-term/src/block/router.rs b/crates/carrot-term/src/block/router.rs index a25fb98d..02bed0d5 100644 --- a/crates/carrot-term/src/block/router.rs +++ b/crates/carrot-term/src/block/router.rs @@ -314,35 +314,11 @@ impl BlockRouter { /// Freezes the active block and returns its `Arc` /// for consumers (render thread, replay, scrollback search). /// - /// **Silent-success drop**: if the command produced zero output - /// rows AND exited with code `0` AND never promoted to a TUI - /// session, the entry is removed from the router instead of - /// frozen — `cd`, `touch`, `export FOO=bar`, ASSIGN-only commands, - /// etc. leave no visual record. Failures, TUI sessions, and any - /// command that wrote even a single row keep their entry. Single - /// invariant lives in the producer; consumers (render, search, - /// history, replay) never see the suppressed entries and never - /// have to filter independently. - /// /// Returns `None` if there was no active block (e.g. a spurious - /// `;D` without a preceding `;C`) or if the silent-success rule - /// dropped this one. + /// `;D` without a preceding `;C`). pub fn on_command_end(&mut self, exit_code: i32) -> Option> { let id = self.active_id.take()?; - let pos = self.blocks.iter().position(|e| e.id == id)?; - - // Silent-success path — remove the entry outright before - // committing exit metadata or building a Frozen wrapper. The - // active grid is dropped on the way out; nothing references - // its replay buffer either. - let drop_silently = matches!(&self.blocks[pos].variant, BlockVariant::Active(active) - if exit_code == 0 && active.total_rows() == 0 && !active.kind().is_tui()); - if drop_silently { - self.blocks.remove(pos); - return None; - } - - let entry = &mut self.blocks[pos]; + let entry = self.blocks.iter_mut().find(|e| e.id == id)?; entry.metadata.exit_code = Some(exit_code); entry.metadata.finished_at = Some(Instant::now()); // Swap the variant out, call `finish`, store the Frozen back. @@ -513,16 +489,6 @@ impl<'a> ActiveTarget<'a> { mod tests { use super::*; - /// Push one row of output into the active block before ending it, - /// so `on_command_end` doesn't apply the silent-success drop. Most - /// router tests want to exercise the freeze/eviction/metadata path, - /// not the silent-success one. - fn produce_row(r: &mut BlockRouter) { - if let ActiveTarget::Block { block, .. } = r.active() { - block.append_row(&[Cell::default()]); - } - } - #[test] fn new_router_has_no_active_block() { let r = BlockRouter::new(80); @@ -557,7 +523,6 @@ mod tests { fn command_end_freezes_block_and_clears_active() { let mut r = BlockRouter::new(80); let id = r.on_command_start(); - produce_row(&mut r); let frozen = r.on_command_end(0).expect("frozen block on end"); assert_eq!(Arc::strong_count(&frozen), 2); // router + caller assert!(!r.has_active_block()); @@ -586,7 +551,6 @@ mod tests { let mut r = BlockRouter::with_limits(80, RouterLimits { max_blocks: 3 }); for _ in 0..5 { r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); } assert_eq!(r.len(), 3); @@ -599,10 +563,8 @@ mod tests { fn eviction_never_drops_active_block() { let mut r = BlockRouter::with_limits(80, RouterLimits { max_blocks: 2 }); r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); // Now start a new one without ending it, then push another // end to overflow. @@ -631,7 +593,6 @@ mod tests { fn metadata_attaches_to_last() { let mut r = BlockRouter::new(80); r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); r.set_last_metadata(RouterBlockMetadata { cwd: Some("/tmp".into()), @@ -674,15 +635,13 @@ mod tests { let mut r = BlockRouter::new(10); r.on_prompt_start(); r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); r.on_prompt_start(); r.on_command_start(); // One frozen, one active. assert_eq!(r.frozen_entries().count(), 1); assert_eq!(r.active_entries().count(), 1); - // Finish the second — now two frozen, zero active. Exit 1 - // keeps the entry visible regardless of empty output. + // Finish the second — now two frozen, zero active. r.on_command_end(1); assert_eq!(r.frozen_entries().count(), 2); assert_eq!(r.active_entries().count(), 0); @@ -704,10 +663,8 @@ mod tests { fn clear_drops_all_blocks_and_resets_active() { let mut r = BlockRouter::new(80); r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); r.on_command_start(); assert!(r.has_active_block()); @@ -723,7 +680,6 @@ mod tests { fn clear_keeps_id_counter_monotonic() { let mut r = BlockRouter::new(80); let id_a = r.on_command_start(); - produce_row(&mut r); r.on_command_end(0); r.clear(); let id_b = r.on_command_start(); @@ -749,84 +705,4 @@ mod tests { entry.metadata.command, ); } - - #[test] - fn silent_success_command_drops_entry() { - // `cd Projects` / `touch x` / `export FOO=bar` — no stdout, - // exit 0. Router should drop the entry outright. - let mut r = BlockRouter::new(80); - r.on_command_start(); - assert_eq!(r.len(), 1, "active block exists pre-end"); - let frozen = r.on_command_end(0); - assert!( - frozen.is_none(), - "silent success must not produce a Frozen handle", - ); - assert_eq!( - r.len(), - 0, - "silent-success entry must be removed from the router", - ); - assert!(!r.has_active_block()); - } - - #[test] - fn silent_failure_keeps_entry() { - // Rare but real: a command exits non-zero with no stderr. - // The user still wants to see it failed. - let mut r = BlockRouter::new(80); - r.on_command_start(); - let frozen = r.on_command_end(1); - assert!( - frozen.is_some(), - "non-zero exit with no output must still freeze", - ); - assert_eq!(r.len(), 1); - } - - #[test] - fn produced_output_keeps_entry_even_on_zero_exit() { - let mut r = BlockRouter::new(80); - r.on_command_start(); - produce_row(&mut r); - let frozen = r.on_command_end(0); - assert!(frozen.is_some()); - assert_eq!(r.len(), 1); - } - - #[test] - fn tui_session_keeps_entry_even_when_grid_is_empty() { - // Alt-screen TUI sessions promote `kind` to `Tui`; their - // persistent grid is often empty when they exit, but the - // session itself was meaningful — keep it. - use crate::block::LiveFrameSource; - let mut r = BlockRouter::new(80); - r.on_command_start(); - if let ActiveTarget::Block { block, .. } = r.active() { - block.activate_live_frame(0, 1, LiveFrameSource::Heuristic); - } - let frozen = r.on_command_end(0); - assert!( - frozen.is_some(), - "TUI session must not be silently dropped on exit 0", - ); - assert_eq!(r.len(), 1); - } - - #[test] - fn silent_success_keeps_id_counter_monotonic() { - // Suppressing the entry should NOT roll back `next_id`. - // Otherwise an external observer that cached the dropped id - // would see it reused on the next command. - let mut r = BlockRouter::new(80); - let dropped = r.on_command_start(); - r.on_command_end(0); - let next = r.on_command_start(); - assert!( - next.0 > dropped.0, - "next id ({}) must exceed the dropped id ({})", - next.0, - dropped.0, - ); - } } diff --git a/crates/carrot-terminal-view/src/block_list.rs b/crates/carrot-terminal-view/src/block_list.rs index 4ad04bca..c89d7a44 100644 --- a/crates/carrot-terminal-view/src/block_list.rs +++ b/crates/carrot-terminal-view/src/block_list.rs @@ -679,6 +679,24 @@ fn render_block_entry( ) }), ) + // Command echo — the line the user typed. Rendered in the + // terminal font + size so it aligns with the cells of the + // output grid below; without it, silent-success commands + // (`cd`, `touch`, `export …`) would render as empty headers + // with no signal of what was actually run. Skipped when the + // shell hooks didn't emit a command (rare — only for the + // very first prompt before the integration loads). + .when(!entry.command.trim().is_empty(), |d| { + d.child( + div() + .w_full() + .pb(px(2.0)) + .font(font.clone()) + .text_size(px(font_size)) + .text_color(theme.colors().foreground) + .child(inazuma::SharedString::from(entry.command.clone())), + ) + }) .child(element) } From 6db03df526674693196a875f91e70fc2fc8d1bd0 Mon Sep 17 00:00:00 2001 From: nyxb Date: Wed, 29 Apr 2026 22:18:07 +0200 Subject: [PATCH 8/8] fix(blocks): wire command-text end to end via OSC 7777 + cmdline pending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original symptom — `cd Projects` rendering as an empty block — came from three layered gaps. None of the previous attempts fixed the actual data flow; they patched downstream of the loss point. The data flow now: - The user types a command in our cmdline. `set_pending_block_command` stashes the text on the router's pending slot. - The shell's preexec hook (zsh / bash / fish / nu) emits a second `OSC 7777;carrot-precmd;` carrying `{"command": ""}`. Reuses the existing carrot-precmd channel — same parser path, same hex-JSON pipeline, escape-handling for free. - The shell emits `OSC 133;C`. Our OSC dispatch in `terminal.rs` now calls `BlockRouter::on_command_start` directly. The deleted wrapper `Term::route_to_new_block` used to clobber the pending slot with `String::new()`; that's the bug that hid the typed command. - `on_command_start` consumes the pending slot into the new block's `RouterBlockMetadata.command`. - `handle_shell_marker(CommandStart)` then calls `set_last_metadata`, preferring `shell_context.command` (preexec emit) over the existing pending-derived value, and falling back to `None` only when neither source is set. - `render_block_entry` paints the command text in the terminal font, full width, between the chip header and the output grid — the same layout Warp uses. Coverage: - Cmdline-typed: pending slot fills first, preexec emit confirms with the same text. Both paths produce the same value, idempotent. - Paste-and-execute / agent-PTY-write / shell-internal scripts: no cmdline involvement. Pending stays None; preexec emit is the only source. Now visible in the block. - Shells without OSC 7777 integration: cmdline-pending is the authoritative source; preexec emit is missing. Block still has the command via the cmdline path. Schema changes (forward-compat preserved by `#[serde(default)]`): - `ShellMetadataPayload.cwd: String` → `Option` so the preexec emit (which carries only `command`) does not clobber the cwd captured at precmd. - New `ShellMetadataPayload.command: Option`. - New `ShellContext.command: Option` storing the latest preexec emit until `CommandStart` consumes it via `take()`. - `ShellContext::update_from_metadata` now does field-by-field optional merge instead of unconditional overwrite — needed so precmd-emit and preexec-emit layer correctly. Shell hooks (4 files) updated to emit `OSC 7777;carrot-precmd` with `{"command": ""}` from preexec-equivalent hooks: zsh `preexec`, bash `DEBUG`-trap (deduped against PROMPT_COMMAND), fish `fish_preexec`, nushell `pre_execution`. JSON escape handles backslashes, quotes, and newlines for multiline commands. Threat model: a shell hook executing in the user's session can spoof any command-text it wants, but it can also just run a different command — the deception is upstream of any nonce we could add. No authentication layer warranted. Tests: - 4 in `metadata_payload_tests` covering the new partial-payload parse path, escape roundtrip, and forward-compat for unknown keys. - 3 in `merge_tests` for the field-by-field optional merge in `ShellContext::update_from_metadata` — partial command payload preserves cwd/git, two sequential emits layer correctly, and a cwd-only payload does not clear a queued command. `Term::route_to_new_block` deleted. --- .../carrot-shell-integration/src/context.rs | 124 ++++++++++++++++-- .../carrot-shell-integration/src/metadata.rs | 71 +++++++++- crates/carrot-term/src/term.rs | 5 - .../src/terminal_pane/shell.rs | 54 ++++++-- crates/carrot-terminal/src/terminal.rs | 20 ++- shell/carrot.bash | 15 +++ shell/carrot.fish | 15 +++ shell/carrot.zsh | 18 +++ shell/nushell/vendor/autoload/carrot.nu | 17 ++- 9 files changed, 301 insertions(+), 38 deletions(-) diff --git a/crates/carrot-shell-integration/src/context.rs b/crates/carrot-shell-integration/src/context.rs index cd4c5342..209ce33d 100644 --- a/crates/carrot-shell-integration/src/context.rs +++ b/crates/carrot-shell-integration/src/context.rs @@ -27,6 +27,11 @@ pub struct ShellContext { /// Populated opportunistically — `None` when outside a git repo /// or before the first prompt fires with metadata. pub git_root: Option, + /// Latest command line surfaced via `ShellMetadataPayload.command` + /// at preexec time. The OSC 133;C dispatch consumes this with + /// `take()` into `RouterBlockMetadata.command`, then resets to + /// `None` so the next prompt cycle starts clean. + pub command: Option, } impl ShellContext { @@ -55,30 +60,55 @@ impl ShellContext { git_branch, git_stats, git_root, + command: None, } } /// Update context dynamically from shell metadata (OSC 7777). + /// + /// Field-by-field optional merge: every payload field that is + /// `Some` overwrites the corresponding context field; `None` + /// fields are left untouched. This lets two emits per command + /// cycle layer correctly — precmd fills cwd/git/user/host, + /// preexec fills command, neither clobbers the other. + /// + /// Boolean / structured fields without an Option (`git_stats`) + /// follow the same rule by treating "unspecified" as preserve. pub fn update_from_metadata(&mut self, payload: &ShellMetadataPayload) { - self.cwd = payload.cwd.clone(); - self.cwd_short = shorten_path(&payload.cwd); + if let Some(ref cwd) = payload.cwd { + self.cwd = cwd.clone(); + self.cwd_short = shorten_path(cwd); + } if let Some(ref u) = payload.username { self.username = u.clone(); } if let Some(ref h) = payload.hostname { self.hostname = h.clone(); } - self.git_branch = payload.git_branch.clone(); - self.git_stats = if payload.git_dirty == Some(true) { - Some(GitStats { - files_changed: 1, - insertions: 0, - deletions: 0, - }) - } else { - None - }; - self.git_root = payload.git_root.clone(); + if payload.git_branch.is_some() { + self.git_branch = payload.git_branch.clone(); + } + // `git_dirty` is the only signal the shell sends about repo + // state; absence means "no opinion this emit", so leave the + // existing `git_stats` untouched. Presence with `false` means + // "explicitly clean", which clears any prior dirty flag. + if let Some(dirty) = payload.git_dirty { + self.git_stats = if dirty { + Some(GitStats { + files_changed: 1, + insertions: 0, + deletions: 0, + }) + } else { + None + }; + } + if payload.git_root.is_some() { + self.git_root = payload.git_root.clone(); + } + if payload.command.is_some() { + self.command = payload.command.clone(); + } } } @@ -92,6 +122,7 @@ impl Default for ShellContext { git_branch: None, git_stats: None, git_root: None, + command: None, } } } @@ -148,6 +179,73 @@ fn detect_git_root(cwd: &std::path::Path) -> Option { if root.is_empty() { None } else { Some(root) } } +#[cfg(test)] +mod merge_tests { + use super::*; + + fn ctx_with(cwd: &str, branch: Option<&str>) -> ShellContext { + ShellContext { + cwd: cwd.into(), + cwd_short: cwd.into(), + hostname: "host".into(), + username: "user".into(), + git_branch: branch.map(str::to_string), + git_stats: None, + git_root: None, + command: None, + } + } + + #[test] + fn partial_command_payload_preserves_cwd_and_git() { + // The preexec emit carries only `command`. Ensure cwd / git + // / user fields captured at precmd survive intact. + let mut ctx = ctx_with("/proj", Some("main")); + let payload = ShellMetadataPayload { + command: Some("ls -la".into()), + ..Default::default() + }; + ctx.update_from_metadata(&payload); + assert_eq!(ctx.cwd, "/proj"); + assert_eq!(ctx.git_branch.as_deref(), Some("main")); + assert_eq!(ctx.command.as_deref(), Some("ls -la")); + } + + #[test] + fn precmd_then_preexec_layers_correctly() { + // Real flow: precmd emit fills cwd/git, preexec emit fills + // command. Both survive, neither clobbers the other. + let mut ctx = ctx_with("/old", None); + ctx.update_from_metadata(&ShellMetadataPayload { + cwd: Some("/new".into()), + git_branch: Some("main".into()), + ..Default::default() + }); + ctx.update_from_metadata(&ShellMetadataPayload { + command: Some("git status".into()), + ..Default::default() + }); + assert_eq!(ctx.cwd, "/new"); + assert_eq!(ctx.git_branch.as_deref(), Some("main")); + assert_eq!(ctx.command.as_deref(), Some("git status")); + } + + #[test] + fn cwd_only_payload_does_not_clear_command() { + // Edge case: a metadata emit between two CommandStart cycles + // (e.g. shell internal that re-fires precmd) must not erase + // a command we already have queued. + let mut ctx = ctx_with("/x", None); + ctx.command = Some("echo first".into()); + ctx.update_from_metadata(&ShellMetadataPayload { + cwd: Some("/y".into()), + ..Default::default() + }); + assert_eq!(ctx.cwd, "/y"); + assert_eq!(ctx.command.as_deref(), Some("echo first")); + } +} + fn detect_git_stats(cwd: &std::path::Path) -> Option { let output = Command::new("git") .args(["diff", "--stat", "--shortstat", "HEAD"]) diff --git a/crates/carrot-shell-integration/src/metadata.rs b/crates/carrot-shell-integration/src/metadata.rs index d25d1f27..5a02ea7c 100644 --- a/crates/carrot-shell-integration/src/metadata.rs +++ b/crates/carrot-shell-integration/src/metadata.rs @@ -1,14 +1,20 @@ /// Shell metadata received via OSC 7777;carrot-precmd. /// -/// Deserialized from hex-encoded JSON sent by the shell precmd hook. -/// Contains environment context for updating UI chips and block headers. +/// Deserialized from hex-encoded JSON sent by the shell precmd hook AND +/// the preexec hook. Two emits per command-cycle: the precmd-time emit +/// carries cwd / git / user / host / exit / duration; the preexec-time +/// emit carries the about-to-execute `command` line. +/// +/// Every field is `Option`-typed so a partial emit (e.g. preexec with +/// only `command`) does not clobber the values captured at precmd. +/// `ShellContext::update_from_metadata` performs field-by-field merge. /// /// Extensible with `#[serde(default)]` — new fields can be added without /// breaking shells that don't send them yet. #[derive(Debug, Clone, Default, serde::Deserialize)] #[serde(default)] pub struct ShellMetadataPayload { - pub cwd: String, + pub cwd: Option, pub username: Option, pub hostname: Option, pub git_branch: Option, @@ -17,6 +23,65 @@ pub struct ShellMetadataPayload { pub last_exit_code: Option, pub last_duration_ms: Option, pub shell: Option, + /// Command line about to be executed. Set by the preexec-time + /// emit; consumed by the OSC 133;C dispatch into the new block's + /// `RouterBlockMetadata.command`. Authoritative source — wins + /// over the cmdline-side `pending_command` slot, since the shell + /// sees what actually runs (handles paste-and-execute, scripts, + /// agent-PTY-writes that bypass the cmdline). + pub command: Option, +} + +#[cfg(test)] +mod metadata_payload_tests { + use super::*; + + #[test] + fn payload_with_only_command_parses() { + // Preexec-time emit carries only the command field. cwd / + // git / user fields stay None — the precmd-time emit owns + // those, and field-by-field merge in update_from_metadata + // ensures they don't get clobbered. + let json = r#"{"command":"cd Projects"}"#; + let payload: ShellMetadataPayload = + serde_json::from_str(json).expect("partial payload should parse"); + assert_eq!(payload.command.as_deref(), Some("cd Projects")); + assert!(payload.cwd.is_none()); + assert!(payload.git_branch.is_none()); + } + + #[test] + fn payload_with_only_cwd_parses_without_command() { + // Precmd-time emit (existing path) — no command field, must + // still parse. + let json = r#"{"cwd":"/home/x","username":"x","hostname":"y","shell":"zsh"}"#; + let payload: ShellMetadataPayload = + serde_json::from_str(json).expect("legacy payload should parse"); + assert_eq!(payload.cwd.as_deref(), Some("/home/x")); + assert!(payload.command.is_none()); + } + + #[test] + fn payload_with_escaped_command_roundtrips() { + // JSON escapes for the kinds of characters real shells emit + // — quotes, backslashes, newlines for multiline commands. + let json = r#"{"command":"echo \"hi\\nthere\""}"#; + let payload: ShellMetadataPayload = + serde_json::from_str(json).expect("escaped payload should parse"); + assert_eq!(payload.command.as_deref(), Some("echo \"hi\\nthere\"")); + } + + #[test] + fn unknown_fields_are_skipped() { + // Forward-compat: future shells may add fields the current + // build doesn't know yet. `#[serde(default)]` on the struct + // skips them without erroring. + let json = r#"{"command":"ls","future_field":42,"cwd":"/x"}"#; + let payload: ShellMetadataPayload = + serde_json::from_str(json).expect("unknown fields should be skipped"); + assert_eq!(payload.command.as_deref(), Some("ls")); + assert_eq!(payload.cwd.as_deref(), Some("/x")); + } } /// TUI-mode hint received via OSC 7777;carrot-tui-hint. diff --git a/crates/carrot-term/src/term.rs b/crates/carrot-term/src/term.rs index 6833afbd..affb95ba 100644 --- a/crates/carrot-term/src/term.rs +++ b/crates/carrot-term/src/term.rs @@ -265,11 +265,6 @@ impl Term { self.block_router.on_prompt_start(); } - pub fn route_to_new_block(&mut self, command: String) { - self.block_router.set_pending_command(command); - let _ = self.block_router.on_command_start(); - } - pub fn route_finalize_block(&mut self, exit_code: i32) { if self.mode.contains(TermMode::SYNC_UPDATE) { self.pending_finalize = Some(exit_code); diff --git a/crates/carrot-terminal-view/src/terminal_pane/shell.rs b/crates/carrot-terminal-view/src/terminal_pane/shell.rs index b899f4bc..2068fabf 100644 --- a/crates/carrot-terminal-view/src/terminal_pane/shell.rs +++ b/crates/carrot-terminal-view/src/terminal_pane/shell.rs @@ -66,20 +66,37 @@ impl TerminalPane { if let carrot_terminal::ShellMarker::Metadata(ref json) = marker { match serde_json::from_str::(json) { Ok(payload) => { - let cwd_changed = self.shell_context.cwd != payload.cwd; - let new_cwd = std::path::PathBuf::from(&payload.cwd); + // The preexec emit carries only `command`; the + // precmd emit carries cwd / git / etc. Each branch + // below short-circuits when the relevant field is + // absent so a partial emit doesn't trigger + // worktree-scope reclassification or cache + // invalidation that the user didn't ask for. + let cwd_changed = payload + .cwd + .as_ref() + .is_some_and(|c| self.shell_context.cwd != *c); + let new_cwd = payload.cwd.as_deref().map(std::path::PathBuf::from); self.shell_context.update_from_metadata(&payload); - self.shell_completion.update_cwd(new_cwd.clone()); - self.detection_cache.invalidate(); - self.last_exit_code = payload.last_exit_code; - self.last_duration_ms = payload.last_duration_ms; - - let new_git_root = payload.git_root.as_ref().map(std::path::PathBuf::from); - if new_git_root != self.current_git_root { - self.current_git_root = new_git_root; + if let Some(cwd) = new_cwd.as_ref() { + self.shell_completion.update_cwd(cwd.clone()); + self.detection_cache.invalidate(); + } + if let Some(exit) = payload.last_exit_code { + self.last_exit_code = Some(exit); + } + if let Some(dur) = payload.last_duration_ms { + self.last_duration_ms = Some(dur); + } + + if let Some(git_root) = payload.git_root.as_ref() { + let new_git_root = Some(std::path::PathBuf::from(git_root)); + if new_git_root != self.current_git_root { + self.current_git_root = new_git_root; + } } - if cwd_changed { + if cwd_changed && let Some(new_cwd) = new_cwd { use carrot_shell::scope_policy::{ProjectKind, WorktreeRoot, classify}; use inazuma_settings_framework::Settings as _; @@ -158,9 +175,22 @@ impl TerminalPane { { let handle = self.terminal.handle(); let mut term = handle.lock(); + // Priority for the command field: shell-metadata + // (preexec emit) > cmdline pending slot > None. + // The pending slot was already consumed into + // `entries().last().metadata.command` by + // `on_command_start`; preserve that value when our + // shell_context has nothing to add. `take()` + // because the next command cycle starts clean. + let existing_command = term + .block_router() + .entries() + .last() + .and_then(|e| e.metadata.command.clone()); + let command = self.shell_context.command.take().or(existing_command); term.block_router_mut().set_last_metadata( carrot_term::block::RouterBlockMetadata { - command: None, + command, cwd: Some(self.shell_context.cwd.clone()), username: Some(self.shell_context.username.clone()), hostname: Some(self.shell_context.hostname.clone()), diff --git a/crates/carrot-terminal/src/terminal.rs b/crates/carrot-terminal/src/terminal.rs index ff718907..23b65688 100644 --- a/crates/carrot-terminal/src/terminal.rs +++ b/crates/carrot-terminal/src/terminal.rs @@ -255,7 +255,25 @@ impl Terminal { term.route_to_prompt(); } crate::osc_parser::ShellMarker::CommandStart => { - term.route_to_new_block(String::new()); + // Direct call into the + // primitive — the wrapper + // `route_to_new_block` was + // deleted because it + // combined "set pending + // command + start", which + // makes sense only for the + // cmdline-submit path that + // owns the command text. + // Here at OSC 133;C we are + // the start signal, not the + // command source — pending + // (if any) was set earlier + // by the cmdline; the new + // block consumes whichever + // wins via the priority + // ladder in + // `handle_shell_marker`. + let _ = term.block_router_mut().on_command_start(); } crate::osc_parser::ShellMarker::CommandEnd { exit_code, diff --git a/shell/carrot.bash b/shell/carrot.bash index c9347aa7..e1f1cd13 100644 --- a/shell/carrot.bash +++ b/shell/carrot.bash @@ -81,6 +81,21 @@ _carrot_preexec() { _carrot_cmd_start=$(date +%s%3N 2>/dev/null || echo 0) + # Emit the about-to-run command line via the OSC 7777 channel that + # the precmd-time emit uses. ShellContext merges the two emits + # field-by-field. Sent BEFORE OSC 133;C so it's buffered when + # CommandStart fires. + local _cmd="$BASH_COMMAND" + local _esc="${_cmd//\\/\\\\}" + _esc="${_esc//\"/\\\"}" + _esc="${_esc//$'\n'/\\n}" + _esc="${_esc//$'\r'/\\r}" + _esc="${_esc//$'\t'/\\t}" + local _cmd_json='{"command":"'"$_esc"'"}' + local _cmd_hex + _cmd_hex=$(builtin printf '%s' "$_cmd_json" | xxd -p | tr -d '\n') + builtin printf '\e]7777;carrot-precmd;%s\a' "$_cmd_hex" >&$_carrot_fd + builtin printf '\e]133;B\a' >&$_carrot_fd builtin printf '\e]133;C\a' >&$_carrot_fd diff --git a/shell/carrot.fish b/shell/carrot.fish index 469a8702..56c45464 100644 --- a/shell/carrot.fish +++ b/shell/carrot.fish @@ -69,6 +69,21 @@ end function _carrot_preexec --on-event fish_preexec set -g _carrot_cmd_start (date +%s%3N 2>/dev/null; or echo 0) + + # Emit the about-to-run command line via the OSC 7777 channel that + # the precmd-time emit uses. ShellContext merges the two emits + # field-by-field. Sent BEFORE OSC 133;C so it's buffered when + # CommandStart fires. + set -l _cmd "$argv[1]" + set -l _esc (string replace -a '\\' '\\\\' -- $_cmd) + set _esc (string replace -a '"' '\\"' -- $_esc) + set _esc (string replace -a \n '\\n' -- $_esc) + set _esc (string replace -a \r '\\r' -- $_esc) + set _esc (string replace -a \t '\\t' -- $_esc) + set -l _cmd_json '{"command":"'$_esc'"}' + set -l _cmd_hex (printf '%s' "$_cmd_json" | xxd -p | string join '') + printf '\e]7777;carrot-precmd;%s\a' "$_cmd_hex" + printf '\e]133;B\a' printf '\e]133;C\a' diff --git a/shell/carrot.zsh b/shell/carrot.zsh index 72802e9e..0dfea062 100644 --- a/shell/carrot.zsh +++ b/shell/carrot.zsh @@ -88,6 +88,24 @@ _carrot_preexec() { zmodload -F zsh/datetime p:EPOCHREALTIME 2>/dev/null _carrot_cmd_start=$EPOCHREALTIME + # Emit the about-to-run command line via the same OSC 7777 channel + # the precmd-time emit uses. Reusing `carrot-precmd` keeps the + # parser path single — a new emit just merges into ShellContext + # field-by-field. JSON escapes backslashes / quotes / newlines so + # multiline commands and embedded quotes survive intact. Sent + # BEFORE OSC 133;C so the metadata is buffered when CommandStart + # fires. + local _cmd="$1" + local _esc="${_cmd//\\/\\\\}" + _esc="${_esc//\"/\\\"}" + _esc="${_esc//$'\n'/\\n}" + _esc="${_esc//$'\r'/\\r}" + _esc="${_esc//$'\t'/\\t}" + local _cmd_json='{"command":"'$_esc'"}' + local _cmd_hex + _cmd_hex=$(builtin printf '%s' "$_cmd_json" | xxd -p | tr -d '\n') + builtin printf '\e]7777;carrot-precmd;%s\a' "$_cmd_hex" >&$_carrot_fd + # Mark prompt end / input region start builtin printf '\e]133;B\a' >&$_carrot_fd diff --git a/shell/nushell/vendor/autoload/carrot.nu b/shell/nushell/vendor/autoload/carrot.nu index 23aa4342..8b4a2b22 100644 --- a/shell/nushell/vendor/autoload/carrot.nu +++ b/shell/nushell/vendor/autoload/carrot.nu @@ -53,19 +53,28 @@ if "sudo" in $features { } } -# TUI hint: emit OSC 7777 carrot-tui-hint for known TUI commands via the -# pre_execution hook, before the command writes its first byte. Matches -# the colon-separated $env.CARROT_KNOWN_TUIS list set by pty.rs. +# Pre-execution emit: carries the about-to-run command line via the +# carrot-precmd channel, plus the TUI hint when the command matches +# $env.CARROT_KNOWN_TUIS. Both pieces of information land while the +# command is queued but before its first byte arrives, so the +# CommandStart marker (emitted by reedline immediately after) sees +# the metadata already buffered. $env.config = ($env.config | upsert hooks {|config| let existing = ($config.hooks? | default {}) let existing_pre_exec = ($existing.pre_execution? | default []) $existing | upsert pre_execution ($existing_pre_exec | append {|cmd| + # Always emit the command line itself — `to json -r` handles + # quoting / escaping for any input. ShellContext merges this + # with the precmd-time emit (cwd / git / etc.) field-by-field. + let cmd_hex = ({command: $cmd} | to json -r | encode hex) + print -n $"\e]7777;carrot-precmd;($cmd_hex)\u{07}" + let tuis = ($env.CARROT_KNOWN_TUIS? | default "" | split row ":") if ($tuis | is-empty) { return } - # Skip when the command carries a non-interactive flag. + # Skip the TUI hint when the command carries a non-interactive flag. if ($cmd =~ ' (--version|--help|-V|-h|-\?)( |$)') { return }