fix: terminal restore on panic, per-session render caches, incremental animation frames, sanitizer gaps#12
Merged
Merged
Conversation
A panic anywhere in the TUI previously left the terminal stranded in raw mode + alternate screen + mouse capture (restore only ran on the Result path), making the shell unusable and hiding the panic message. Install a std::panic hook chained onto the default hook that restores the terminal (disable_raw_mode, DisableMouseCapture, DisableBracketedPaste, LeaveAlternateScreen, cursor Show) before the default hook prints, plus a TerminalGuard RAII drop for early-return paths. Restore is idempotent via an AtomicBool so hook, guard, and the normal exit path can all call it safely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
apply_delta / upsert / session status-change did front-to-back linear scans, but streaming events target the NEWEST message — so every streamed chunk paid a full-transcript scan on long sessions (visible as prompt lag after many turns). Search from the back instead: the common case becomes O(1) and duplicate-id edge cases resolve to the newest occurrence in both paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…reachable scroll_offset was u16, silently capping the transcript scroll range at 65 535 wrapped rows — the top of big sessions was unreachable via scroll_to_top or PgUp. Widen to usize end-to-end (scroll_up/down, page_step, anchored-scroll maintenance in note_total_rendered, the worker-row indicator). The final Paragraph::scroll stays u16-safe because only the intra-viewport remainder from visible_window reaches ratatui, and it is clamped explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…NSI parser Two transcript-corruption bugs in the shared text-scrubbing layer, fixed in BOTH render/sanitize.rs and render/ansi.rs: - ESC P / ESC X / ESC ^ / ESC _ (DCS/SOS/PM/APC) were treated as two-byte escapes, so a sixel image or tmux-passthrough payload flooded the transcript as text. They now consume their payload up to ST (ESC backslash) or BEL, mirroring the OSC arm. The 8-bit C1 CSI introducer (U+009B) is also parsed (previously its parameter bytes leaked as text; in ansi.rs it now applies SGR like ESC[). - Tabs were kept verbatim, but ratatui 0.29 silently drops a lone tab as a zero-width grapheme, destroying the alignment of git status / Makefile output. Tabs now expand to spaces up to the next 8-column stop, column-aware (unicode-width, CJK-correct) within the current line segment, including across styled spans and streamed feeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Switching sessions used to evict every OTHER session's cached message renders: the renderer called retain_only(live_ids) with only the FOCUSED session's ids, and ScrollbackBuild held a single-session build slot — so Tab A->B->A was a full O(N) re-parse in both directions, forever. - RenderCache now groups entries per session; retention (retain_session) prunes dead message ids within ONE session only, so focus switches never touch other sessions' entries. - ScrollbackBuild becomes ScrollbackBuildCache: an LRU (4 sessions, most-recently-focused first) of per-session assembled builds, so flipping back to a recent session is a pure cache hit. - Memory stays bounded: when a session falls off the build LRU, its per-message render-cache entries are evicted with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
While any tool was running, every 100 ms tick forced a full scrollback rebuild: every cached message's Vec<Line> was re-cloned AND every line re-measured with Paragraph::line_count — O(total lines) per frame, felt as CPU burn and sluggishness on long transcripts. Two-part fix: - Wrapped-row counts are now cached alongside each message's rendered lines in RenderCache, keyed by (version, width): a content delta re-measures only that message, a resize invalidates everything. Full rebuilds assemble the prefix sum from cached integers instead of re-wrapping the transcript. - Animation-only frames (same session epoch, same width) no longer rebuild at all: the build tracks per-message line segments, and ScrollbackBuild::splice_message re-renders JUST the animating messages and splices them into the cached buffer — O(animating lines) per frame. The prefix sum is recomputed (integer adds only) only when the spliced message's wrapped shape actually changed; a spinner glyph swap doesn't. Correctness preserved on content deltas, resize, message insertion, and tool-state transitions — all of those bump epoch/width and take the full-rebuild path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR updates terminal restoration, duplicate-id resolution, scroll arithmetic, session-scoped render/scrollback caching, scrollback rendering, and ANSI/sanitizer escape and tab handling across codeoid-tui. ChangesTUI reliability and rendering fixes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
==========================================
+ Coverage 51.18% 58.22% +7.04%
==========================================
Files 31 31
Lines 5523 6344 +821
==========================================
+ Hits 2827 3694 +867
+ Misses 2696 2650 -46
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codeoid-tui/src/app.rs (1)
463-480: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReverse-scan needs a duplicate-id guard or test
SessionListis just aVec,replace()can preserve duplicate ids, andupsert()updates the first match. If duplicate session ids can occur, this status change can land on the wrong entry; add a regression test or enforce uniqueness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codeoid-tui/src/app.rs` around lines 463 - 480, The reverse search in DaemonMessage::SessionStatusChange can update the wrong session when SessionList contains duplicate ids, since sessions.items().iter().rev().find(...) assumes uniqueness while replace() and upsert() may preserve duplicates. Fix this by either enforcing unique session ids in SessionList/merge_session/upsert, or by adding a regression test that covers duplicate ids and verifies the intended entry is updated. Keep the change localized around the SessionStatusChange handler and the SessionList update path.
🧹 Nitpick comments (1)
crates/codeoid-tui/src/ui/scrollback.rs (1)
72-76: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid the full message scan on cached idle frames.
any_animatingwalks the whole focused transcript before the cache-hit decision, so prompt keystrokes/idle frames are still O(message count). Consider storing animating message keys in the build during rebuild and checking that small set on hits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codeoid-tui/src/ui/scrollback.rs` around lines 72 - 76, The scrollback render path is still scanning the full transcript via any_animating before the cache-hit check, which makes idle frames O(message count). Update the Scrollback build/cache flow in scrollback.rs so animating message keys are tracked during the rebuild path and reused on cache hits, and then have the cache-hit logic consult that small set instead of calling messages(&session_id).iter().any(is_animating).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/codeoid-tui/src/state/mod.rs`:
- Around line 221-228: The tool-selection update logic in the focused-session
path is clearing the entire scrollback build cache instead of evicting only the
current session’s entry, which breaks the LRU/memory-bound behavior and removes
unrelated session hits. Update the code around the focused-session invalidation
block in the state module so that `scrollback_build` is evicted or invalidated
only for `self.sessions.focused_id()` (the same session used for
`render_cache.invalidate`) rather than calling `clear()`, and apply the same
targeted eviction in the corresponding other tool-selection change path.
In `@crates/codeoid-tui/src/ui/scrollback.rs`:
- Around line 101-119: The scrollback render path in
render_message_with_animation and the BuildState splice flow are keying only on
m.message_id, which breaks when a session contains duplicate ids. Update the
RenderCache lookup and BuildSegment identification to include a
per-occurrence/render key (or another disambiguator) so each duplicate message
gets its own cached render and splice target, or disable cache/splice reuse when
duplicates are detected.
---
Outside diff comments:
In `@crates/codeoid-tui/src/app.rs`:
- Around line 463-480: The reverse search in DaemonMessage::SessionStatusChange
can update the wrong session when SessionList contains duplicate ids, since
sessions.items().iter().rev().find(...) assumes uniqueness while replace() and
upsert() may preserve duplicates. Fix this by either enforcing unique session
ids in SessionList/merge_session/upsert, or by adding a regression test that
covers duplicate ids and verifies the intended entry is updated. Keep the change
localized around the SessionStatusChange handler and the SessionList update
path.
---
Nitpick comments:
In `@crates/codeoid-tui/src/ui/scrollback.rs`:
- Around line 72-76: The scrollback render path is still scanning the full
transcript via any_animating before the cache-hit check, which makes idle frames
O(message count). Update the Scrollback build/cache flow in scrollback.rs so
animating message keys are tracked during the rebuild path and reused on cache
hits, and then have the cache-hit logic consult that small set instead of
calling messages(&session_id).iter().any(is_animating).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f88264-020d-43f2-8d89-e970dba88b9a
📒 Files selected for processing (9)
crates/codeoid-tui/src/app.rscrates/codeoid-tui/src/main.rscrates/codeoid-tui/src/render/ansi.rscrates/codeoid-tui/src/render/sanitize.rscrates/codeoid-tui/src/state/messages.rscrates/codeoid-tui/src/state/mod.rscrates/codeoid-tui/src/state/render_cache.rscrates/codeoid-tui/src/state/scrollback_build.rscrates/codeoid-tui/src/ui/scrollback.rs
Comment on lines
+101
to
+119
| for m in messages | ||
| .messages(&session_id) | ||
| .iter() | ||
| .filter(|m| is_animating(m)) | ||
| { | ||
| let per_block_expanded = expanded_ids.contains(&m.message_id); | ||
| let is_selected = selected_id.as_deref() == Some(m.message_id.as_str()); | ||
| let mut new_lines = render_message( | ||
| m, | ||
| anim_tick, | ||
| verbose_tools || per_block_expanded, | ||
| is_selected, | ||
| ); | ||
| if !new_lines.is_empty() { | ||
| // Trailing separator, exactly as the full rebuild | ||
| // appends per rendered message. | ||
| new_lines.push(Line::raw("")); | ||
| } | ||
| build.splice_message(&m.message_id, new_lines); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Disambiguate duplicate message ids in cache keys and build segments.
Replay can leave duplicate message_ids in one session, but this path keys RenderCache and BuildSegment only by m.message_id. A rebuild can reuse the first duplicate’s rendered lines for later duplicates, and animation splice updates the first matching segment rather than the newest/actual occurrence. Include an occurrence/render key in both the cache lookup and segment mapping, or bypass cache/splice when duplicates are present.
Also applies to: 141-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codeoid-tui/src/ui/scrollback.rs` around lines 101 - 119, The
scrollback render path in render_message_with_animation and the BuildState
splice flow are keying only on m.message_id, which breaks when a session
contains duplicate ids. Update the RenderCache lookup and BuildSegment
identification to include a per-occurrence/render key (or another disambiguator)
so each duplicate message gets its own cached render and splice target, or
disable cache/splice reuse when duplicates are detected.
Tool-block selection and per-block expand are focused-session-local, but both paths called scrollback_build.clear(), dropping every session's assembled build from the LRU (losing A->B->A tab-flip hits) while leaving their render-cache entries orphaned outside the build-LRU memory bound. Add ScrollbackBuildCache::evict_session and evict just the focused session alongside the existing per-message invalidations. Also harden SessionList::replace against duplicate session ids from a buggy replay (dedupe, last occurrence wins, stable tab order) so every by-id lookup - focus, status updates, upsert, the reverse status-change scan - can rely on uniqueness. Addresses CodeRabbit review on PR #12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Addressed the review in 8afce81:
234 tests pass, clippy warning set identical to baseline, fmt clean. |
Owner
Author
|
Review addressed through 8afce81:
218 tests (+4), clippy = baseline exactly, fmt clean. |
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
1 participant
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.
Fixes (from the session/rendering audit)
Panic leaves the terminal unusable (P1). No panic hook meant any panic stranded the terminal in raw mode + alternate screen + mouse capture (
resetrequired). Now: a chained panic hook restores the terminal before the default hook prints (message + backtrace land on a usable screen), plus aTerminalGuardRAII drop for early-return paths. Restore is idempotent via anAtomicBoolarmed before the first mode change, so a partialsetup_terminalfailure also unwinds.Session switch was a full O(N) re-parse, both directions, forever (P1).
retain_onlyevicted every non-focused session's render cache on each rebuild, and the build slot held one session. Now:RenderCacheis grouped per session (retain_session/evict_session), and assembled builds live in a 4-session MRU LRU (ScrollbackBuildCache) — Tab A→B→A is a pure cache hit both ways. Memory stays bounded: sessions falling off the LRU take their render-cache entries with them.Whole-transcript rebuild every animation frame (P1). While any tool ran, every 100ms tick cloned all cached lines and re-ran
Paragraph::line_countover the entire transcript — O(total lines) at 10Hz, degrading input latency exactly while output streams. Now: per-line wrapped-row counts are cached per message keyed by (version, width) (measurement identity with the renderer preserved — sameParagraphwrap config), the build records per-message segment ranges, and animation framessplice_messageonly the animating messages in place — O(animating lines)/frame. The integer-only prefix-sum rebuild runs only when a splice changes the message's wrapped shape (a spinner glyph swap doesn't). Content deltas / resize / insertion still take the full-rebuild path; a sentinel-based test proves splice-on-tick and rebuild-on-completion.Scrollback beyond 65,535 wrapped rows was unreachable (P2).
scroll_offset: u16saturated, sog/Home on a large session landed mid-transcript with no way higher. Widened tousizeend-to-end; only the intra-viewport remainder (explicitly clamped) reaches ratatui's u16Paragraph::scroll.DCS/SOS/PM/APC payloads flooded the transcript (P2). Both the sanitizer and the ANSI parser treated
ESC P/X/^/_as two-byte escapes, so sixel or tmux-passthrough payloads rendered as kilobytes of junk text. Both now consume to ST/BEL (mirroring the OSC arm); 8-bit C1 CSI (U+009B) parses as CSI.Tabs silently vanished (P2). The sanitizer preserved
\ton the premise that ratatui renders tab stops — false: ratatui 0.29 skips zero-width graphemes, sogit status/Makefile output lost all indentation. Tabs now expand to 8-column stops at parse time, column-aware via unicode-width (CJK-correct, resets on\n/\r, persists across streamed feeds and styled spans).Streaming message lookup was a worst-case forward scan (P2).
apply_delta/upsert/status-change scanned front-to-back while streaming targets the newest message; now.rev().Tests
45 net new (214 total, up from 169): sanitizer DCS/SOS/PM/APC/C1-CSI consumption in both parsers incl. unterminated-at-EOF; tab expansion (stops, CJK width, newline/CR reset, styled-span and streamed-feed continuity); per-session cache retention + LRU eviction/touch semantics; scroll offset past the former u16 ceiling; back-to-front lookup with duplicate ids; row-count cache invalidation (content delta = that message only, width change = all); splice correctness (shape-preserving, growing, shrinking, offset rebuild, sentinel splice-not-rebuild on animation tick).
cargo test --workspacegreen,cargo clippy --workspace --all-targetshas zero new warnings (one fewer than main — the usize change removed a cast warning),cargo fmtclean.Out of scope (follow-up PR material): session tab-bar scrolling past the clip width, attach/detach LRU of session subscriptions, reconnect replay storm.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores