fix(tui): commit queued-drain echo through the scrollback handoff - #227
Merged
Conversation
Echoing a drained queued command with a raw console.print left the input card in the frame prompt_toolkit's run_in_terminal teardown erases, so under heavy CPU load the card's top border fossilized into scrollback just above the echoed command (the rare "queued follow-up ghost"). Route the echo through the view's scrollback handoff via a new _PromptLiveView.commit_scrollback_echo, which raises the handoff depth first so running_prompt_hide_input_card is True at the instant the echo is written — keeping the border out of the torn-down frame, exactly as streamed turn content is committed. Adds a deterministic regression test asserting the card is hidden when the echo commits (verified to fail on the raw-print path). The steer echo already commits via pending-scrollback/handoff; the initial-input echo runs at stable idle geometry with no view and was verified fossil-free under heavy load.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughQueued follow-up command echoes now use the prompt view’s scrollback handoff instead of direct console printing. Tests verify input-card hiding, state restoration, and best-effort handling when handoff teardown fails. ChangesQueued echo handoff
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Shell
participant PromptLiveView
participant Console
Shell->>PromptLiveView: Commit queued command echo
PromptLiveView->>Console: Print inside scrollback handoff
Console-->>PromptLiveView: Emit echo and restore input-card state
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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/__init__.py`:
- Around line 1500-1507: Update the queue handling around commit_scrollback_echo
so the queued command remains available until the handoff succeeds. Avoid
removing it with pending.pop(0) before the await; remove or acknowledge the item
only after a successful commit, while preserving explicit failure cleanup so the
command is retried or reported as dropped.
In `@tests/ui_and_conv/test_visualize_running_prompt.py`:
- Around line 737-778: Add a caller-level async regression test for
commit_scrollback_echo that forces its scrollback handoff/emission to fail, then
assert the queued command remains available for retry or is explicitly reported
as dropped according to the existing contract. Keep the successful
hidden-input-card assertions unchanged and cover the failure or partial-success
outcome without introducing unrelated behavior.
- Around line 767-772: Update the test around
view.running_prompt_hide_input_card() to stop patching
shared_console._force_terminal, which is Rich’s private field. Create a public
Console(force_terminal=False) instance and patch the module’s console reference
with it, while preserving the existing print interception and assertion
behavior.
🪄 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
Run ID: 3f2ab6d5-38a2-4961-bcd0-7aa3c9ee7b98
📒 Files selected for processing (4)
CHANGELOG.mdsrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/visualize/_interactive.pytests/ui_and_conv/test_visualize_running_prompt.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Address review on the queued-follow-up ghost fix: - commit_scrollback_echo now logs a failed scrollback handoff instead of raising it. The echo is cosmetic and the command still runs via run_soul, so a handoff error must not drop the queued command the shell already popped. Cancellation still propagates (CancelledError is not an Exception). - Add a failure-path regression asserting the best-effort behavior. - Stop patching Rich's private _force_terminal in the hide-card test; use a public Console(force_terminal=False) swapped into the module reference.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/ui_and_conv/test_visualize_running_prompt.py`:
- Around line 784-813: Update
test_commit_scrollback_echo_is_best_effort_when_handoff_fails to exercise the
real console handoff instead of monkeypatching the private
_run_scrollback_handoff method. Arrange for a non-terminal Console.print
operation to raise, invoke commit_scrollback_echo, and assert that the print
seam was reached while the exception remains swallowed.
🪄 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
Run ID: e3362368-e787-479b-a7e6-d52eceda4fb7
📒 Files selected for processing (2)
src/pythinker_code/ui/shell/visualize/_interactive.pytests/ui_and_conv/test_visualize_running_prompt.py
Instead of monkeypatching the private _run_scrollback_handoff, make a non-terminal Console.print raise so the handoff runs its real emit and commit_scrollback_echo swallows the failure from the actual seam. Asserts the print was reached and no exception propagated.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
Fixes a rare "queued follow-up ghost": when a follow-up is queued mid-turn and later drained, the input card's top border (
──── ● off) occasionally fossilizes into scrollback just above the executed command — visible only under heavy CPU load.Root cause
The shell echoes each drained queued command with a raw
console.print(render_user_echo_text(...))(ui/shell/__init__.py). Once the first turn has ended,running_prompt_hide_input_card()returnsFalse, so the input card is painted. The raw print triggers prompt_toolkit'srun_in_terminalerase/redraw with that border still in the pre-handoff frame; under load the teardown's erase-height drifts and strands the border in scrollback.This is the asymmetry: streamed turn content commits through
_run_scrollback_handoff(which raises the handoff depth → hide-gateTrue→ invalidate — before emitting), but the queued-drain echo did neither.Fix
Add
_PromptLiveView.commit_scrollback_echo(renderable)— a thin wrapper that routes the echo through_run_scrollback_handoff, so the input card is hidden at the instant the echo is written, keeping the border out of the frame the terminal teardown erases. The shell drain loop nowawaits it instead of the raw print. This makes the queued-drain echo consistent with how streamed content is committed.Verification
test_commit_scrollback_echo_hides_input_card_during_emit): drives the real method and asserts the hide-gate isTrueat the exact moment the echo commits. Confirmed non-circular — it fails ([False] == [True]) when the method is reverted to a raw print, and passes with the fix.test_mid_turn_queued_input_renders_once_and_executes_once, CI-skipped) passed 25/25 under an 8×yesCPU load with the fix, where the old code fossilized under the same load.make check-pythinker-codeclean;make test-pythinker-codegreen (8289 passed, 7 skipped, 1 xfailed; e2e 65 passed).Sibling echo sites (audited, no-defer)
_emit_steer_echo(_interactive.py) — safe by design: commits via_append_pending_scrollback→_flush_pending_scrollback→ handoff, not a raw print._echo_agent_input(initial-input echo,__init__.py) — verified safe: fires at stable idle geometry with no view/delegate attached (where the erase-height doesn't drift); the two existing initial-echo fossil guards passed 12/12 under the same 8×yesload.Summary by CodeRabbit
Bug Fixes
Tests