fix(blocks): generation counter + envelope-owned padding + idempotent pin#49
Open
nyxb wants to merge 8 commits into
Open
fix(blocks): generation counter + envelope-owned padding + idempotent pin#49nyxb wants to merge 8 commits into
nyxb wants to merge 8 commits into
Conversation
… pin 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<Term>` 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<Pixels>` (just y) to `Option<Point<Pixels>>` (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<BlockId>` 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).
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.
…ooth `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.
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.
…home 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.
`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.
…touch, …)" 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).
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;<hex-json>` carrying `{"command": "<text>"}`.
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<String>` so the
preexec emit (which carries only `command`) does not clobber the
cwd captured at precmd.
- New `ShellMetadataPayload.command: Option<String>`.
- New `ShellContext.command: Option<String>` 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": "<line>"}` 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #46
Closes #47
Closes #48
Stacked on PR #40. The buggy code paths live on that branch — branching from
mainwould lose the necessary context.What & why
Three regressions reported as separate bugs share one architectural root: block-system state lived at the wrong layer. Each fix moves the invariant to the producer that owns it. No symptom-level patches.
Architectural plan synthesized from a deep read of
.reference/zed/(clock, text, terminal, gpui list state) plus published 2026 Rust streaming-render patterns (arc-swap, Lamport version vectors, sticky-bottom invariant). Brief lives in the PR description; principles distilled into a hard-rules update is a follow-up.How
#46 — Snapshot cache invalidation
crates/carrot-term/src/block/render_view.rskeyed the active-block snapshot cache onsnapshot.total_rows() as u64. In-place cell writes (progress bars,\roverwrites, partial-line emission) leave the row count alone, so the cache key never advanced and the render-side memoize atblock_list.rs::memoize_active_snapshotreturned the previous frame's stale snapshot. Output looked frozen for seconds, then dumped in one flood when a newline finally flipped the key.ActiveBlocknow carries a Lamport-stylegeneration: AtomicU64that bumps on every observable mutation — grid writes, atlas interns, hyperlink / grapheme / image inserts, metadata changes, replay-buffer appends, kind promotion. Mirrorstext::Buffer::versionfrom Zed's clock crate. The producer holds the single source of truth; the render layer readsblock.generation()directly into the existing(id, frame_id)cache key.Relaxedordering — the surroundingMutex<Term>already provides synchronization at every mutation site.Tests in
carrot_term::block::active::generation_testscover the full envelope: starts above zero, advances on every mutation method, holds on no-op (record_byteswith empty slice, repeatedpromote_kind_to_tui), and the specifically-regressing case where manygrid_mut()calls happen withouttotal_rows()changing.#47 — Block-content left-edge alignment
The block header chips lived inside a
divwith.px(BLOCK_HEADER_PAD_X); the cell-gridBlockElementwas a sibling with no matching inset. Header sat at x=16, grid at x=0, hit-test compensated by subtracting the constant from the wrong coordinate, andterminal_pane.rsreservedBLOCK_HEADER_PAD_X * 2from the PTY column budget — three independent ideas of the content edge.The block envelope now owns the inset: a single
.px(BLOCK_HEADER_PAD_X)on the parentdivinrender_block_entry. Both children (header, grid) render flush against the inset. Same consolidation in the fold area —fold_areaparent gets the inset, fold-line / fold-counter render flush.GridOriginStorewidened fromOption<Pixels>(just y) toOption<Point<Pixels>>(full origin).BlockElement::prepaintwritesbounds.originstraight in.hit_testreads x and y from the store and stops re-applyingBLOCK_HEADER_PAD_Xitself — the constant has exactly two consumers now (the layout call inblock_list.rsand the PTY column budget interminal_pane.rs), one rule, no drift. Theblock_list/layout.rsduplicate type alias is gone — re-exports the canonical one fromcarrot-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_toprecomputed against shifting heights produced an unstable scroll position. Fixing #46 stabilises the heights, the scroll stops oscillating.Layered on top:
BlockListViewnow cacheslast_pinned: Option<BlockId>and only invokesBlockState::pin/unpinwhen 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 innerRefCell::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 existingfollow_tailsemantics,logical_scroll_topresolution, andscroll_to_endidempotence are correct (Zed'sListStateshape, upstream-validated).Test plan
cargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warningspassescargo test -pfor every touched crate (carrot-term,carrot-block-render,carrot-terminal-view)generation_tests(8 tests) verify producer-side counter behaviour, including the in-place-write case that motivated the fixyes | head -n 1000,seq 1 200 | while read i; do echo step $i; sleep 0.05; done, progress-bar emitters — output must stream linearly, not in floodsPerformance impact
fetch_add(1, Relaxed)on every mutation site — atomic add, single instruction on x86_64 / aarch64. Lost in the noise vs. the VT writer's actual work.BlockSnapshot::from_pagesrow clone) when the active block didn't change. Net win at idle / partial-update workloads.RefCell::borrow_mutper render frame in the common case (no Tui block, or stable Tui pin).Notes for reviewers
The architectural reasoning lives in the commit body. The hard-rules update for "Block envelope owns horizontal inset" and "Producer-side generation counter is the single dirty signal" is a follow-up doc PR — does not need to land with this fix.
SharedTerminal/ActiveBlockSnapshot/FrozenBlockHandleincarrot-block-render/src/snapshot.rsare unused dead code (set up but never wired through). The deadgeneration: u64field onActiveBlockSnapshotis intentionally untouched in this PR — cleaning up the orphaned types is a separate refactor pass, not in scope here.