fix(THU-791): stop button not working properly - #1230
Open
arienemaiara wants to merge 7 commits into
Open
Conversation
Semgrep Security ScanNo security issues found. |
arienemaiara
changed the title
|
Preview environment deployed 🚀
Stack: Auto-destroys on PR close/merge. Login via the bundled Keycloak realm — |
PR Metrics
Updated Fri, 04 Sep 2026 18:21:35 GMT · run #2925 |
There was a problem hiding this comment.
Pull request overview
Fixes THU-791 by unifying “turn activity” detection across the composer and message list so the Stop button and loading spinner stay in sync, and by improving abort/stop settling behavior (including empty-turn recovery and reasoning-only layout gaps).
Changes:
- Introduces a shared
getTurnActivityhelper used by bothChatPromptInput(Stop button / submit gating) andChatMessages(spinner / error gating). - Improves aborted-turn settling in
createChatInstance(drop empty assistant shells; finalize streaming reasoning parts; persist partial streamed answers on abort). - Updates reasoning display layout to avoid a persistent blank gap after stopping a reasoning-only turn, and adds targeted unit tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/components/chat/reasoning-display.tsx | Collapse reserved min-height once ephemeral reasoning fades to prevent post-stop blank space. |
| src/components/chat/reasoning-display.test.tsx | Tests for height reservation behavior while streaming vs settled. |
| src/components/chat/chat-prompt-input.tsx | Uses shared getTurnActivity to show Stop and gate submits whenever a turn is active. |
| src/components/chat/chat-prompt-input.test.tsx | Adds Stop-button visibility/behavior tests across submitted/streaming/recovery/backoff states. |
| src/components/chat/chat-messages.tsx | Replaces local activity derivation with getTurnActivity to keep spinner logic in sync with composer. |
| src/chats/turn-activity.ts | New pure helper to compute “turn activity” signals from useChat + session retry state. |
| src/chats/turn-activity.test.ts | Unit tests covering the activity matrix (submitted/streaming/empty-turn recovery/backoff/exhausted/error). |
| src/chats/chat-instance.ts | Enhances abort handling and overrides stop() to cancel retries and settle UI state. |
| src/chats/chat-instance.test.ts | Adds regression tests for aborted empty shells, reasoning finalization, and stop canceling retries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: 28f2bfea302f · mode: single · deferred 0 item(s) already reported by other bots (best-effort dedup)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/chats/chat-instance.ts:992
instance.stop()no longer returns/awaits the underlyingChat.stop()promise in the in-flight case (void originalStop()). This changes the method’s async contract: callers awaitingstop()may proceed before the abort is actually initiated/settled, and any rejection fromoriginalStop()will be swallowed. Preferreturn originalStop()(orawait originalStop(); return) to preserve behavior and error propagation.
instance.stop = async function () {
if (retryTimeout) {
clearTimeout(retryTimeout)
retryTimeout = null
}
if (instance.status === 'streaming' || instance.status === 'submitted') {
void originalStop()
return
}
src/chats/chat-instance.ts:752
- In the
hasStreamingReasoningabort branch you finalize and persistfinalized, but later the turn completion telemetry is emitted withmessage(the callback param), which may still containreasoning.state === 'streaming'or differ fromfinalized. This can lead to telemetry recording inconsistent message state vs what’s displayed/saved. Consider emitting completion with the finalized message for that branch (e.g., passfinalizedintoemitTurnCompleted) so telemetry reflects the settled message.
} else if (hasStreamingReasoning(lastMessage)) {
// Stopped mid-reasoning: the reasoning part is left `streaming`, so its
// spinner never stops. Finalize it in the live list and the saved copy.
const finalized = finalizeReasoning(lastMessage!)
const next = instance.messages.slice()
next[lastIndex] = finalized
instance.messages = next
finishedTurn.telemetry?.startPhase('final_save')
await saveMessages({ id, messages: [finalized] })
finishedTurn.telemetry?.endPhase('final_save')
} else if (message?.parts?.length) {
src/chats/turn-activity.ts:26
- The doc for
pendingEmptyTurnRecoverysays "statusis back toready", but the implementation doesn’t checkstatus === 'ready'(it keys off!isStreaming). Either update the doc to match the actual condition, or tighten the logic to requirestatus === 'ready'so callers can trust the meaning of this flag.
/** The model returned an empty turn and the app is (or should be) recovering
* it — the thread shows a spinner, `status` is back to `ready`. */
pendingEmptyTurnRecovery: boolean
src/components/chat/chat-messages.tsx:70
ChatMessagesnow deriveshasErrorfromgetTurnActivity, but the completion haptic still useschatErroronly. This can mis-signal success for error states that aren’t represented bychatError(e.g., an empty assistant turn withretriesExhaustedthat surfaces ashasError). Prefer using the derivedhasErrorhere so the notification matches the UI’s error state.
const wasStreaming = useRef(false)
useEffect(() => {
if (wasStreaming.current && !isStreaming) {
triggerNotification(chatError ? 'error' : 'success')
}
wasStreaming.current = isStreaming
}, [isStreaming, chatError, triggerNotification])
arienemaiara
marked this pull request as draft
August 20, 2026 17:06
raivieiraadriano92
marked this pull request as ready for review
September 4, 2026 16:52
- Stopping a turn now transitions the thread back to idle so the stop button reflects the actual streaming state. - Extract turn-activity tracking into its own module with tests.
- Reserve the 200px min-height only while the ephemeral reasoning is shown; a stopped reasoning-only turn never gets a text part to unmount the display, so an unconditional reserve left a permanent gap. - Add tests covering both the streaming (reserved) and settled (collapsed) states.
- Delegate stop to the SDK while streaming/submitted so onFinish({ isAbort })
settles on the correct currentTurn instead of it being swapped out early.
- Keep the retry-cancel, empty-shell drop, and idle settle only for the
backoff/recovery path, which fires no onFinish.
The AI SDK only aborts the request signal — nothing cancels a custom fetch's body — so the Pi harness kept calling the model and running tools after Stop. Thread init.signal into the harness stream.
sendAutomaticallyWhen is gated on isError, not isAbort, so a turn stopped before its assistant message existed left the user message trailing and the SDK re-sent it. Suppress it until the next explicit send, stop dropping the empty shell that created that trailing message, and cancel an open permission dialog.
Tearing a turn down is asynchronous — an ACP session/cancel round-trip, the harness draining its loop — so the button looked inert after the press. Track stopping on the session, derive it against the live request so a stale flag can't strand the composer, and swap in a spinner.
raivieiraadriano92
force-pushed
the
THU-791/stop-button-isnt-working
branch
from
September 4, 2026 17:18
1536789 to
c4a64a8
Compare
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: c4a64a824f07 · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
Stopping before the model's first turn began left a zero-part assistant message that renders an empty-turn recovery spinner nothing recovers. Safe to drop now that stopRequested gates the auto-send. Also skip partial saves of a zero-part turn, so the drop survives a reload.
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
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
The Stop button did nothing while the model was thinking or auto-retrying — it only worked once tokens were already streaming. Stop now ends the turn from any state and settles the thread back to idle.
Changes
submitted), streaming, or empty-turn/auto-retry recovery — via a sharedgetTurnActivitysignal used by both the composer and the message list, so the button and the loading spinner can't disagree.stopcancels any pending auto-retry and drops a trailing empty assistant turn, so the recovery spinner can't restart the turn.onFinishsettles the turn in one of three ways: drop an empty loader shell, finalize a mid-reasoning turn so its spinner stops, or keep a streamed partial answer.Evidences
Simulator.Screen.Recording.-.iPhone.17.-.2026-08-19.at.10.39.26.mov