feat(tui): unified spinner slot — Thinking / Working / Executing + regression fixtures#118
Conversation
…ding indicator
- TUI: keep the foreground spinner alive during reasoning streams and
swap its label to 'Thinking...' while reasoning parts arrive. Restore
the base label ('Working...') on reasoning-end. Reasoning parts no
longer count as the first visible part, so the spinner is only
cleared when real text/tool output arrives.
- TUI: BaseToolCallView pending indicator now uses the same braille
frames and cyan/dim styling as StatusSpinner; label normalized to
'Executing...'.
- CEA: internal pi-tui renderer pending spinner kept in sync with the
TUI style for consistency. Tests updated.
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request unifies the visual style of spinners across the TUI and CEA using braille frames and ANSI styling, and ensures the foreground spinner displays "Thinking..." during model reasoning. Feedback suggests adding a fallback for the onReasoningEnd callback to prevent the spinner from potentially getting stuck on "Thinking..." if the base loader message is undefined.
….. in fallback - New pending-spinner.ts exposes PENDING_SPINNER_FRAMES, PENDING_SPINNER_INTERVAL_MS, stylePendingIndicator, and createSpinnerTicker. StatusSpinner, FooterStatusBar, BaseToolCallView, and CEA's pi-tui-stream-renderer all route through this single primitive. - BaseToolCallView now renders the Executing... indicator automatically in the raw-fallback path (input streamed, no output/error/denied yet). Consumers with custom renderers are unaffected; minimal-agent gets the spinner with no code changes. - New tool-call-view.test.ts covers the auto fallback indicator lifecycle.
If a visible stream part (text-start, tool-result, etc.) arrives
before reasoning-start in the continuation stream,
clearStreamingLoader nulls the foreground spinner and the
subsequent setMessage('Thinking...') silently no-ops. Revive
the spinner via showLoader when reasoning-start fires on a null
foreground status, and tear down only the revived spinner on
reasoning-end so ordinary flows that kept 'Working...' alive
still restore that label cleanly.
Root cause: BaseToolCallView.content was a plain Markdown, which
retains the trailing '' line that pi-tui's renderToken() emits
from the terminal 'space' token in blocks.join('\n\n'). Container
concatenates children verbatim, so that blank line pushed the
inline pending indicator one row below the tool block.
Switch this.content to the TrimmedMarkdown already used for the
pretty-block header (which strips trailing blank lines). The
spinner now sits on the line immediately after the tool block
with no extra blank gap. Regression test added.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c62cd5a078
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ing... placement) Root cause: the inline 'Executing...' Text was a child of BaseToolCallView (chatContainer descendant), so during a tool call statusContainer concurrently mounted IdleStatusPlaceholder — which renders ['', ''] — between the inline indicator and the editor. Two blank lines separated 'Executing...' from the prompt, unlike Thinking...' which lives in the foreground StatusSpinner (statusContainer) and sits directly above the editor. Fix: route pending state to the same foreground slot as the other spinner labels: - Add onToolPendingStart/End callbacks to PiTuiStreamState. - Fire onToolPendingStart from handleToolCall. - Fire onToolPendingEnd from handleToolResult/Error/OutputDenied, unconditionally (even when showToolResults is false). - In renderAgentStream, wire the callbacks to showLoader/setMessage with a counter so parallel tool calls only restore the base 'Working...' label when all pending calls have resolved. - Remove the inline pending indicator and its helpers from BaseToolCallView; tests updated accordingly. Result: Executing... now sits on the same row as Thinking..., directly above the prompt, with no phantom gap.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fe9079c3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/src/agent-tui.ts">
<violation number="1" location="packages/tui/src/agent-tui.ts:1263">
P2: `onToolPendingEnd` unconditionally restores the base spinner message, which can clear/overwrite the active `Thinking...` state before `onReasoningEnd` fires. This causes incorrect spinner labeling during overlapping reasoning/tool lifecycles.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…blank above foreground spinner
Two separate issues that reinforced each other:
1. BaseToolCallView.setPrettyBlock({ isPending: true }) used to paint
a rotating 'Executing...' frame into readBody via startPendingSpinner.
That was painted inside the tool block (chatContainer), while the
new foreground spinner *also* shows 'Executing...' (statusContainer,
above the prompt). User saw two Executing... at the same time.
Pretty-block pending path now sets body to '' and all the
pretty-block spinner plumbing is deleted. Foreground spinner owns
the pending affordance, matching Thinking... behavior.
2. After removing the pretty-block spinner, 'Executing...' on the
foreground spinner had two blank lines above it instead of one.
Cause: ensurePrettyBlockComponents always added a standalone
Spacer(1) between header and body. When body rendered to [] (empty
text), that Spacer still emitted ['']. Combined with StatusSpinner's
own leading '', users saw two blanks above Executing...
Moved the separator into BackgroundBody.render(): it prepends its
own leading '' only when it has content to render. Non-pending
pretty blocks keep the header/body separator unchanged; pending
pretty blocks now render just the header with no trailing blank.
Regression tests added in tool-call-view.test.ts locking in the
pending (no trailing blank) and non-pending (1 blank between header
and body) shapes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5228e83f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ehind fixtures
Add fixture-style regression tests so each bug uncovered during this
PR is pinned in place and cannot silently regress:
pending-spinner.test.ts (new):
- PENDING_SPINNER_FRAMES exact order
- PENDING_SPINNER_INTERVAL_MS = 80
- stylePendingIndicator ANSI byte sequence (cyan frame + dim message)
- createSpinnerTicker: initial frame sync, 80ms cadence, wraparound,
stop() idempotency, emitInitialFrame:false, custom intervalMs
stream-handlers.test.ts (extended):
- isVisibleStreamPart: reasoning-start/delta/end ALL invisible under
any flag combination (regression: if reasoning becomes visible,
first-visible-part fires on reasoning-start and the Thinking... label
swap is lost)
- IGNORE_PART_TYPES no longer contains reasoning-start / reasoning-end
- STREAM_HANDLERS dispatch table coverage for every known part type
- handleReasoningStart / handleReasoningEnd fire their callbacks
- reasoning-start fires callback even when showReasoning flag is off
- Tool pending counter invariants for parallel tool calls
tool-call-view.test.ts (extended):
- Pretty-block pending inline snapshot: header-only, no trailing blank
- Pretty-block non-pending inline snapshot: header/blank/body shape
- Raw fallback NEVER emits 'Executing' or braille spinner glyphs
- Raw fallback NEVER emits a trailing blank line
spinner-layout.test.ts (new):
- End-to-end layout invariant: tool block + foreground spinner has
exactly 1 blank line between them, across raw fallback, pretty-
block pending, and pretty-block non-pending (regression: the
original user-reported '공백 2개' gap cannot return)
- BaseToolCallView.render() never emits trailing blanks across all
four rendering modes
Also soften setPrettyBlock({ isPending: true }) in both TUI and the
internal CEA renderer: the pending branch no longer clears the body,
it just writes the body text through. This preserves tool-renderer-
supplied content (e.g. edit_file's live diff preview) while the
foreground spinner owns the pending affordance. The now-dead
pending-spinner machinery inside CEA's internal renderer is also
removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cf85c47e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/thinking-spinner-label.md">
<violation number="1" location=".changeset/thinking-spinner-label.md:15">
P3: The changeset now contains contradictory behavior notes for `setPrettyBlock({ isPending: true })`, which makes the release notes inaccurate.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
minpeter
changed the title
- Apply ultracite formatter auto-fixes across 5 files (whitespace, multi-line array formatting, etc.) - Move braille spinner regex to module-level const (lint/performance/ useTopLevelRegex) in tool-call-view.test.ts - Replace index-based for loop with for...of (lint/style/useForOf) in pending-spinner.test.ts - Remove unused private 'requestRender' field from BaseToolCallView (tui) and internal ToolCallView (cea); constructor param preserved (prefixed with _ to mark intentional no-op) so external callers' signatures still compile unchanged - Drop dead TOOL_PENDING_MESSAGE / TOOL_PENDING_MARKER constants in CEA's pi-tui-stream-renderer.ts and simplify renderPendingOutput() to return an empty string (marker-replacement plumbing was removed with the pretty-block spinner)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13090b359b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address review feedback from cubic-dev-ai and chatgpt-codex-connector on PR #118: 1. End pending state when tool enters approval gate (chatgpt-codex-connector, 3x): handleToolApprovalRequest now fires state.onToolPendingEnd?.() before rendering the approval block, so approval-gated flows (tool-call → tool-approval-request) release the foreground spinner instead of leaving it stuck on 'Executing...' while execution is paused awaiting user/policy decision. 2. Don't overwrite active Thinking... state (cubic-dev-ai P2): add a reasoningActive flag to renderAgentStream. onToolPendingStart no longer changes the spinner label when reasoning is active, and onToolPendingEnd no longer calls restoreBaseSpinner() in that case either. This preserves the Thinking... label across overlapping tool lifecycles (e.g. a late tool-result arriving mid-reasoning on the next turn). 3. Rewrite the changeset (cubic-dev-ai P3): the previous version contained contradictory behavior notes for setPrettyBlock({ isPending: true }) (body set to '' vs body passed through unchanged). The changeset is rewritten to describe only the final end state: body is always written through unchanged regardless of isPending; the foreground spinner owns the pending affordance. Regression tests added: - handleToolApprovalRequest fires onToolPendingEnd - tool-call → tool-approval-request leaves the pending counter at 0
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc92e44442
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/src/agent-tui.ts">
<violation number="1" location="packages/tui/src/agent-tui.ts:1260">
P2: `onReasoningEnd` restores the base spinner even when tool calls are still pending, so active tool execution can be hidden behind `Working...`/cleared status.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ding
Address review feedback (cubic-dev-ai P2):
'onReasoningEnd restores the base spinner even when tool calls are
still pending, so active tool execution can be hidden behind
Working.../cleared status.'
Previously, when reasoning ended while tool calls were still in
flight, the spinner reverted to Working... (or cleared itself if
reasoning had revived the spinner) instead of reflecting the
still-active tool execution.
Extract the spinner state machine into a pure factory
(packages/tui/src/spinner-orchestrator.ts) so the invariants can
be unit-tested directly without spinning up a real TUI. The
orchestrator now:
- transitions to Executing... on onReasoningEnd when toolPendingCount > 0
- transfers spinner ownership (reasoningRevivedSpinner → toolRevivedSpinner)
so the spinner is cleared when the final tool ends, not when
reasoning ends
- still falls back to clearStatus / restoreBase when no tools are
pending (existing behavior preserved)
14 regression tests added in spinner-orchestrator.test.ts covering:
- Baseline reasoning / tool lifecycles (spinner present + cleared)
- Parallel tool counter semantics + non-negative floor
- Reasoning/tool overlap (the three regressions this PR addressed)
- Spinner ownership transfer mid-lifecycle
- Missing baseLoaderMessage (null / undefined)
- Adapter is not spuriously called during counter-only transitions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a993043f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/src/spinner-orchestrator.ts">
<violation number="1" location="packages/tui/src/spinner-orchestrator.ts:59">
P1: `onReasoningEnd` misses cleanup for `toolRevivedSpinner`, so a spinner revived by tool-pending can remain visible when tool completion happened during reasoning.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address review feedback (cubic-dev-ai P1):
'onReasoningEnd misses cleanup for toolRevivedSpinner, so a spinner
revived by tool-pending can remain visible when tool completion
happened during reasoning.'
Scenario:
1. No spinner. onToolPendingStart → showLoader + toolRevivedSpinner=true
2. onReasoningStart → setMessage(Thinking...), reasoningActive=true
3. onToolPendingEnd → count=0, but reasoningActive=true forced early
return; toolRevivedSpinner stayed true with no one watching it
4. onReasoningEnd → count=0 + reasoningRevivedSpinner=false → fell
through to restoreBase(), leaving spinner stuck on Working...
Fix: in onToolPendingEnd, when reasoningActive blocks the normal
cleanup path, transfer spinner ownership from tool → reasoning
(reasoningRevivedSpinner = true, toolRevivedSpinner = false). This
way onReasoningEnd's existing reasoningRevivedSpinner branch runs
clearStatus() naturally when reasoning ends.
Regression test added: 'tool-revived spinner becomes reasoning-owned
when tool ends mid-reasoning' locks in the full sequence and
verifies clearStatus fires on onReasoningEnd.
…rrupt counter
Address two review issues:
1. cubic-dev-ai P1 (onReasoningEnd missed cleanup for toolRevivedSpinner):
Fixed in spinner-orchestrator.ts by transferring spinner ownership
from tool → reasoning inside onToolPendingEnd when reasoningActive
blocks the normal cleanup path. The subsequent onReasoningEnd
correctly runs clearStatus via the reasoningRevivedSpinner branch.
2. chatgpt-codex-connector P2 (Guard pending-end for unmatched
approval requests):
Approval events can appear without a matching tool-call (headless
tests emit this pattern). The old unconditional onToolPendingEnd
call could drop the counter to zero and hide Executing... for an
unrelated parallel tool.
Fix: PiTuiStreamState gains a pendingToolCallIds Set. handleToolCall
adds the id (idempotent — duplicate dispatches don't double-count);
handleToolResult / handleToolError / handleToolOutputDenied /
handleToolApprovalRequest remove the id via firePendingEndIfTracked,
which only fires onToolPendingEnd when the id was actually tracked.
Unmatched terminals are silently ignored. Duplicate terminals for
the same id are also ignored.
Regression tests added in stream-handlers.test.ts:
- Approval with no matching tool-call does not fire onToolPendingEnd
- Unmatched approval does not affect another tool's pending counter
- Duplicate tool-call for same id fires onToolPendingStart once
- Duplicate terminal for same id fires onToolPendingEnd once
- Existing approval-gated flow test re-asserts after adding the
matching handleToolCall prefix
Regression test added in spinner-orchestrator.test.ts:
- tool-revived spinner becomes reasoning-owned when the tool ends
mid-reasoning, so onReasoningEnd clears the spinner instead of
leaving it stuck on Working...
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
@cubic review |
1 participant
Summary
Route all agent pending states (`Thinking...` / `Working...` / `Executing...`) through the same foreground spinner slot directly above the prompt editor, extract a shared spinner primitive to unify their visual language, and add comprehensive fixture/snapshot regression tests so every bug surfaced during this PR is pinned down.
Before: `Executing...` lived inside `BaseToolCallView` (a chatContainer descendant) with inconsistent styling and a 2-blank-line gap above the prompt. `Thinking...` was cleared the moment reasoning started, because reasoning parts were treated as "first visible".
After: All three spinner labels live in `statusContainer`'s `StatusSpinner`, share identical braille frames + cyan/dim styling, and sit on the same row immediately above the editor — no phantom gaps, no duplicate indicators, no label loss.
Feature changes
1. `Thinking...` during reasoning
2. `Executing...` during tool calls
3. Shared spinner primitive
New `packages/tui/src/pending-spinner.ts` exposes:
`StatusSpinner`, `FooterStatusBar`, `BaseToolCallView`'s pretty-block spinner, and CEA's internal `pi-tui-stream-renderer.ts` all route through this primitive.
4. Inline `Executing...` in tool block removed
`BaseToolCallView` used to paint an `Executing...` indicator inside the tool block itself (via `setPrettyBlock({ isPending: true })` spinner animation). This duplicated the new foreground spinner and produced the user-reported "2 Executing... at once" bug. The pretty-block pending spinner plumbing is deleted; `setPrettyBlock` now writes the body text through unchanged so `edit_file` live diff previews and similar `isPending: true` + non-empty body use cases still render correctly.
5. "공백 2개" gap fixed
`ensurePrettyBlockComponents` used to add a standalone `new Spacer(1)` between `readHeader` and `readBody`. Even when the body rendered to `[]` (empty text), that spacer still emitted `[""]`. Combined with `StatusSpinner.render()`'s own leading `""`, users saw 2 blank rows above `Executing...`.
Fix: the `Spacer(1)` is removed; `BackgroundBody.render()` now prepends its own leading blank only when it has content to render. `Executing...` sits with a single leading blank, identical to `Thinking...` / `Working...`.
Regression fixture suite
Every bug surfaced during this PR is locked down so it cannot silently return:
`pending-spinner.test.ts` (new, 10 tests) — locks spinner constants, exact ANSI byte sequence of `stylePendingIndicator`, full `createSpinnerTicker` lifecycle (initial sync emit, 80ms cadence, wraparound, `stop()` idempotency, `emitInitialFrame: false`, custom `intervalMs`).
`stream-handlers.test.ts` (extended, +17 tests) — parametric proof that all three reasoning parts are invisible under any flag combination (the invariant that keeps `Thinking...` alive), `IGNORE_PART_TYPES` does not contain `reasoning-start` / `reasoning-end`, `STREAM_HANDLERS` has a handler for every known part type, reasoning / tool lifecycle callbacks fire in the right places, parallel-tool-call counter semantics.
`tool-call-view.test.ts` (extended, +5 tests, 8 total) — inline `toMatchInlineSnapshot` fixtures for pretty-block pending (header-only, no trailing blank) and non-pending (header / blank / body) render shapes. Absolute assertions that `BaseToolCallView` never emits `Executing` or any braille spinner glyph. Raw fallback trailing-blank-free lock.
`spinner-layout.test.ts` (new, 7 tests) — end-to-end layout invariant: the combined `chatContainer + statusContainer` output always has exactly 1 blank line between the tool block and the foreground spinner, across raw fallback, pretty-block pending, and pretty-block non-pending modes. Parametric trailing-blank-free check across all four `BaseToolCallView` rendering modes.
Verification
Files
```
.changeset/thinking-spinner-label.md | 23 ++
packages/cea/src/interaction/pi-tui-stream-renderer.test.ts | 59 +---
packages/cea/src/interaction/pi-tui-stream-renderer.ts | 72 +----
packages/tui/src/agent-tui.ts | 106 ++++---
packages/tui/src/index.ts | 1 +
packages/tui/src/pending-spinner.test.ts | 127 ++++++++
packages/tui/src/pending-spinner.ts | 61 ++++
packages/tui/src/spinner-layout.test.ts | 196 +++++++++++++
packages/tui/src/stream-handlers.test.ts | 320 ++++++++++++++++++++-
packages/tui/src/stream-handlers.ts | 31 +-
packages/tui/src/tool-call-view.test.ts | 212 ++++++++++++++
packages/tui/src/tool-call-view.ts | 68 +----
12 files changed, 1050 insertions(+), 226 deletions(-)
```
Commits (chronological)