fix(tui): smoother streaming, no preamble ghosting, clearer ReadFile cards, ANSI-safe diffs - #162
Merged
Conversation
DiffLive grew the live frame with a bare "\n" on every appended row, which scrolls the terminal viewport up even when the new row still fits the visible region — the "text jumps up" symptom during non-interactive streaming. Use cursor-down when the next row provably fits (frame origin + target row vs height) and fall back to newline scroll only on genuine overflow. Adds PYTHINKER_DIFF_LIVE_LOG tracing and scroll-geometry tests.
…doff Stacked "Vibing… (Ns)" rows and duplicate tips fossilized into permanent scrollback during streaming. Root cause: scrollback handoffs tear down and redraw the prompt app around run_in_terminal on every tool transition / turn end, and the height-varying transient preamble (agent stream body + elapsed verb spinner + tips) re-rendered in that window is left behind when the teardown erase miscounts. Suppress all transient preamble (body, spinner, tips) for the duration of a scrollback handoff via _suppress_transient_preamble, and treat a terminal resize as a hard invalidation boundary that briefly hides multi-row tips while prompt_toolkit settles at the new geometry. Committed prose is emitted by the handoff itself; the live tail returns once the handoff completes. Tests sample the preamble renderers from inside the real handoff emit window to prove suppression is live, not just a getter contract.
Collapsed reads said "Read 1 file (ctrl+o to expand)", hiding the line count
and content so a successful read looked like it returned nothing. Render a
line-count-aware summary with the file basename (e.g. "Read 140 lines from
console.py") plus a short, cell-width-capped preview of the leading lines.
Line count prefers the tool message, falls back to counting the body, and
stays truthful when unknowable ("Read file content") or empty ("Read 0
lines"). Preview is ANSI-sanitized, capped to a few visual lines, width-capped
per line, and skipped on narrow terminals. Expanded mode and the LLM-facing
tool result are unchanged.
render_diff rendered untrusted file content and model-supplied edit text verbatim, so a crafted edit could smuggle ANSI escapes (cursor movement, color) into the terminal through Update/Write diff cards and the approval/ pager diffs that share the renderer. Sanitize diff_text once at the top of render_diff; sanitize_ansi keeps newlines/tabs so +/- prefix and line-number parsing are unaffected, and visible text is preserved.
Contributor
|
Warning Review limit reached
More reviews will be available in 13 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughFour TUI rendering bug fixes: ChangesTUI Rendering Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 208-214: The _safe_prompt_invalidate method is catching exceptions
and only logging them via _handoff_trace, which is environment-gated and may not
log in default runs, causing failures to be silently swallowed. Add proper
logging (using a standard logger) before the _handoff_trace call to ensure
failures are always logged to an actionable diagnostic, while maintaining the
existing recovery behavior that prevents exceptions from aborting handoff
cleanup. This addresses both the missing diagnostic logging in default runs and
the coding guideline violation for bare exception catches without proper
logging.
- Around line 160-163: The `_flush_pending_scrollback()` method is called
concurrently from multiple locations, causing batch duplication and loss of
queued entries because the copy and delete operations are not atomic. Add a lock
to serialize access to `_pending_scrollback` and make the batch copy and clear
operation atomic within the critical section. Additionally, modify
`_flush_pending_scrollback()` to accept a parameter indicating shutdown or
interrupt flushes, and when this flag is set, bypass the resize deferral logic
to ensure final scrollback is flushed immediately without deferred retry. Update
all callers at lines 347, 517, 521, 528, 545, and 557 to pass the appropriate
shutdown flag when calling `_flush_pending_scrollback()`.
In `@tests/ui_and_conv/test_diff_live_scroll.py`:
- Around line 63-94: The test function test_append_row_transition_geometry has
an unused parameter base_row that is declared in both the parametrize decorator
and the function signature, but is never referenced in the test body. Remove
base_row from the parametrize decorator's parameter tuple (after target_row),
remove the base_row values from each parametrize test case tuple, and remove the
base_row: int parameter from the function signature. This will eliminate the
dead code flagged by Ruff ARG001.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 20d2bbcf-f385-4314-b0d1-44c0422ce329
📒 Files selected for processing (10)
CHANGELOG.mdsrc/pythinker_code/ui/shell/components/diff.pysrc/pythinker_code/ui/shell/tool_renderers/read.pysrc/pythinker_code/ui/shell/visualize/_diff_live.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pytests/ui_and_conv/test_diff_live_scroll.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_tui_transcript_enhancements.pytests/ui_and_conv/test_visualize_running_prompt.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Resize recovery could defer pending scrollback indefinitely when prompt geometry was unavailable, leaving PTY sessions stuck on Finalizing without emitting completed response text. Force flush on outermost turn end and always tick recovery down each status refresh frame.
Serialize pending scrollback flush with a lock, force flush on shutdown, and log handoff failures. Flatten tool results for non-native Anthropic/OpenAI hosts so compatibility proxies do not drop multi-part payloads.
This was referenced Jul 2, 2026
This was referenced Jul 20, 2026
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.
Summary
A set of surgical TUI rendering fixes. All changes are display-only — no tool execution, wire events, agent specs, or LLM-facing payloads change.
Fallback live renderer no longer scrolls the viewport on every wrapped row.
DiffLivegrew the live frame with a bare"\n"per appended row, scrolling the terminal even when the new row still fit the visible region (the "text jumps up" symptom). Now uses cursor-down when the next row provably fits (frame origin + target row vs height) and falls back to newline scroll only on genuine overflow. AddsPYTHINKER_DIFF_LIVE_LOGtracing.No more stacked
Vibing… (Ns)ghost rows during streaming. Interactive scrollback handoffs (every tool transition / turn end) tear down and redraw the prompt app aroundrun_in_terminal; the height-varying transient preamble (agent stream body + elapsed verb spinner + tips) re-rendered in that window was left behind when the teardown erase miscounted. Handoffs now suppress all transient preamble via_suppress_transient_preamble, and a terminal resize is treated as a hard invalidation boundary that briefly hides multi-row tips while prompt_toolkit settles.Collapsed
ReadFilecards show what happened. Replaced the genericRead 1 filewith a line-count-aware summary + file basename (e.g.Read 140 lines from console.py) plus a short, cell-width-capped, ANSI-sanitized preview of the leading lines. Stays truthful when the count is unknowable (Read file content) or empty (Read 0 lines); expanded mode and the LLM-facing result are unchanged.Diff cards strip terminal control sequences.
render_diffrendered untrusted file content and model-supplied edit text verbatim, so a crafted edit could smuggle ANSI/cursor escapes into the terminal through Update/Write cards and the approval/pager diffs that share the renderer. Sanitized at the single render boundary; visible text preserved.Testing
make check-pythinker-code— green (ruff + format + pyright + ty)tests/ui_and_conv/+ diff suites — 2250 passed_run_scrollback_handoffemit window; collapsed ReadFile count/preview/empty/narrow-width/ANSI cases;render_diffANSI strippingNotes
CHANGELOG.mdupdated under## Unreleasedfor all four user-facing changes.Summary by CodeRabbit
Bug Fixes
New Features
Tests