fix(repl): blank prompt prefix during suppressed-spinner dispatch (#2…#2551
fix(repl): blank prompt prefix during suppressed-spinner dispatch (#2…#2551Davidson3556 wants to merge 2 commits into
Conversation
…acer-Cloud#2116) While a nested live renderer (Rich Live investigation footer or the append-only progress display) owns the bottom of the terminal during a dispatch, the REPL prompt was still falling back to ``idle_hint_ansi()`` on every refresh tick. The shortcut text fought the investigation footer for the same row and produced a visible flash. Extract the prompt-message composition into a module-level ``_build_prompt_message`` helper and keep the prefix slot blank when a dispatch is running with the spinner suppressed. Confirmation banners and the streaming spinner still take priority; idle behavior is unchanged. Slot height stays one row in every state so prompt-toolkit's cursor management remains accurate.
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
Greptile SummaryThis PR fixes a terminal rendering race (#2116) where
Confidence Score: 5/5Safe to merge — the change is a targeted, additive fix to a single pure function with no state side effects, and all four prefix states are unit-tested. The refactor is minimal: one new module-level pure function replaces an equivalent inline closure, and the only behavioral change is a new blank-prefix branch for the dispatch running + spinner suppressed state. Both the original priority order and the one-row height invariant are preserved and verified by the new tests. No public API changes, no shared-state mutations, and all is untouched. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_build_prompt_message called\n(every 0.25s refresh tick)"] --> B{is_awaiting_confirmation?}
B -- Yes --> C["Return: confirm_prompt_text + base\n(y/N banner)"]
B -- No --> D{spinner.streaming?}
D -- Yes --> E["Return: inline_spinner_ansi + base\n(pondering… animation)"]
D -- No --> F{is_dispatch_running?}
F -- "Yes (spinner suppressed\nby nested live renderer)" --> G["Return: '' + base\n(blank prefix — #2116 fix)"]
F -- No --> H["Return: idle_hint_ansi + base\n('/ for commands · ↑↓ history')"]
style G fill:#d4f1c0,stroke:#4caf50
style C fill:#fff3cd,stroke:#f0ad4e
style E fill:#cce5ff,stroke:#004085
style H fill:#f8d7da,stroke:#721c24
Reviews (2): Last reviewed commit: "fix(repl): address greptile review on pr..." | Re-trigger Greptile |
- Promote the blank-prefix branch to a peer of the other three priority-list items in the ``_build_prompt_message`` docstring so the four-state ordering reads at a glance instead of relying on a trailing prose paragraph. - Drop ``_build_prompt_message`` from ``__all__``. The leading underscore already marks it module-private; the tests import it via attribute lookup on ``loop_module``, which works regardless of the export list, so the entry only created a contradiction between the name and the public surface.
|
@greptile review |
|
@muddlebee please review |
|
Looks good 👍 |
Fixes #2116
Describe the changes you have made in this PR -
The REPL prompt's "reserved first row" was falling back to
idle_hint_ansi()(/ for commands · ↑↓ history) on everyprompt_asyncrefresh tick (every 0.25s) — including while a dispatch was running with the streaming spinner suppressed. The spinner is suppressed whenever a nested live renderer (the Rich Live investigation footer or the append-only progress display) is about to own the bottom of the terminal. With the spinner off, the prompt kept redrawing shortcut text on the same rows the investigation footer was animating on, which produced the visible flash reported in #2116.Changes:
app/cli/interactive_shell/runtime/loop.py_message_with_spinnerclosure into a module-level helper_build_prompt_message(session, state, spinner). The closure now just delegates, so theprompt_async(message=…)wiring is unchanged.tests/cli/interactive_shell/test_terminal_runtime.pyTestBuildPromptMessageclass with 6 cases covering: idle, streaming, dispatch with active spinner, dispatch with suppressed spinner (the [BUG] REPL spinner flashes and overlaps investigation footer during live RCA #2116 regression), confirmation banner priority over the blank prefix, and constant 1-row prefix height across all states.Demo/Screenshot for feature changes and bug fixes -
before

after

before

after

Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The bug is a race between two independent renderers writing to the same terminal rows:
prompt_asyncredraws its "message" everyPROMPT_REFRESH_INTERVAL_S(0.25s) so the spinner can animate. The message is built by_message_with_spinner, which historically returnedinline_spinner_ansi() or idle_hint_ansi()as the prefix above the prompt rule.console.suppress_prompt_spinner()is called. That setsspinner.streaming = False, so on the next prompt refresh the prefix silently fell back to the idle hint — and that hint is what the user saw flashing against the live footer.Alternatives I considered:
prompt_asyncentirely during dispatches. Rejected — confirmation prompts, follow-up typing, andesc to cancelall rely on the prompt staying live. PR fix(repl): improve /investigate picker UX and restore live stream #2470 already split exclusive vs non-exclusive stdin paths for the same reason.refresh_intervalduring dispatch. Rejected —refresh_intervalis set once atprompt_asynccall time and isn't safe to mutate dynamically; and we still need 0.25s ticks for the streaming spinner animation in pure chat turns.I chose the minimum-surface fix: keep the existing idle hint, but treat "dispatch running with spinner suppressed" as a fourth explicit state where the prefix is blank. This is symmetric with how
is_awaiting_confirmation()already takes over the same slot. The helper was lifted out of the closure so the four-state logic is unit-testable directly, not just observable via integration tests ofrun_interactive.Key components I added:
_build_prompt_message(session, state, spinner) -> ANSI— pure function over the runtime state. Returns the confirmation banner, the streaming spinner, an empty prefix, or the idle hint, in that priority order. Always 1 row tall.TestBuildPromptMessage— exercises each branch. The regression test for [BUG] REPL spinner flashes and overlaps investigation footer during live RCA #2116 istest_dispatch_running_with_suppressed_spinner_uses_blank_prefix, which asserts that neither/ for commandsnor the streaming verbs leak into the prefix when a dispatch is in flight without an active spinner.Edge cases checked:
test_confirmation_takes_priority_over_dispatch_blank).test_dispatch_running_with_spinner_keeps_spinner).test_prefix_row_height_is_constant) so prompt-toolkit's renderer doesn't misplace the cursor between transitions./investigate— already routed viadispatch_needs_exclusive_stdin, soprompt_asyncis parked atawait state.queue.join()during the picker + investigation. This path is unaffected and continues to use the full Rich Live region.Gate results on this branch:
make lint— passedmake format-check— passedmake typecheck— passed (687 source files)uv run pytest tests/cli/non-live — 1584 passed, 27 deselected (the deselected cases aretest_live_llm_planner_matches_prompt_contract, which requireOPENAI_API_KEYand fail identically onmain)Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.