Skip to content

fix(tui): guard degenerate scroll region when one row remains above viewport#519

Open
ritammehta wants to merge 2 commits into
mainfrom
fix-large-history-insertion
Open

fix(tui): guard degenerate scroll region when one row remains above viewport#519
ritammehta wants to merge 2 commits into
mainfrom
fix-large-history-insertion

Conversation

@ritammehta

Copy link
Copy Markdown

Summary

🤖 Generated with Nori

  • insert_history_lines emitted an invalid ESC[1;1r DECSTBM when exactly one row remained above the inline viewport (area.top() == 1). Terminals ignore invalid DECSTBM and fall back to a full-screen scroll region, so each inserted history line scrolled the live viewport itself — persistent display corruption, since ratatui's diff buffer believed the viewport was intact and never repainted it. Large completed-turn outputs make this state both more reachable (tall viewports) and more destructive (one full-screen scroll per line). This is the unguarded sibling of the top()==0 fix from fix(tui): Guard against degenerate scroll region when viewport fills screen #358.
  • The guard now covers area.top() <= 1: warn, stay cursor-position-neutral, persist any viewport move made by the preceding scroll-down branch, and return false so Tui::draw retains pending lines. The deferral is self-healing — the next insert scrolls the viewport down further and succeeds (covered by a retry-convergence test).
  • Four VT100-backed regression tests (the vt100 parser implements the same degenerate-DECSTBM full-screen fallback as real terminals, verified against its source) plus nori-rs/tui/docs.md updates.

Follow-ups found during investigation (not in this diff)

  • tui.rs scroll_region_up(0..area.top(), …) on viewport expansion emits the same invalid ESC[1;1r when area.top() == 1; masked by the subsequent terminal.clear(), but can push stale viewport rows into scrollback.
  • The MAX_PENDING_LINES (1000) trim in Tui::draw runs on every draw before insertion is attempted, so a single >1000-line batch loses its oldest lines even when insertion succeeds — contrary to its comment ("while blocked").
  • Architectural: VS Code terminal, Windows Terminal, tmux, and Zellij discard region-scrolled lines instead of moving them to scrollback (same open upstream family: openai/codex #27644, #24849, #15380); upstream mitigates via resize-triggered transcript reflow (#18575), which this fork predates.

Test Plan

  • TDD: three repro tests written first and confirmed RED against the unfixed code (viewport rows visibly overwritten by inserted lines), GREEN after the guard
  • cargo test -p nori-tui — 1349 passed, 0 failed; no pending insta snapshots
  • just fmt / just fix -p nori-tui clean
  • cargo build --bin nori && cargo test -p tui-pty-e2e — all runnable suites pass (requires mock_acp_agent binary built)
  • Closed the loop: drove ./target/debug/nori --agent elizacp in an isolated tmux session — full turn completes, reply inserted into history above the viewport, no corruption

Share Nori with your team: https://www.npmjs.com/package/nori-skillsets

ritammehta and others added 2 commits July 2, 2026 15:22
…iewport

insert_history_lines emitted ESC[1;1r when area.top() == 1 — an invalid
DECSTBM region (bottom margin must exceed top) that terminals ignore,
falling back to a full-screen scroll region. Every inserted history line
then scrolled the live viewport itself, and ratatui's diff buffer never
repaired the damage because it believed the viewport was intact. Large
completed-turn outputs made this state both more reachable (tall
viewports) and more destructive (one full-screen scroll per line).

Extend the existing top()==0 guard (#358) to top() <= 1: warn, keep the
call cursor-position-neutral, persist any viewport move made by the
scroll-down branch, and return false so the caller retains pending lines
for retry. The deferral is self-healing: the next insert scrolls the
viewport down further, opening a valid region.

Adds four VT100-backed regression tests: static top()==1 corruption and
return contract, the scroll-down-into-y==1 path, and retry convergence.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
- bump anyhow 1.0.100 -> 1.0.103 (RUSTSEC-2026-0190, unsound downcast_mut)
- ignore RUSTSEC-2026-0194/0195: quick-xml <0.41 is pinned transitively by
  plist (via os_info; latest plist still uses 0.39) and
  tauri-winrt-notification, and both parse locally-generated XML only
- ignore RUSTSEC-2026-0189: rmcp <1.4 DNS rebinding targets its HTTP server
  transport; our only rmcp HTTP server binds loopback and requires a
  per-session random bearer token on every request. rmcp 0.12 -> 1.4 is a
  major upgrade tracked separately
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant