fix(platform): fix stop generation streaming and follow-up reliability#608
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR implements a stop-generation feature for the chat interface. Changes include: new UI controls to halt message generation with a stop button in ChatInput that replaces the send button when loading; backend mutation cancelGeneration to abort active streams and update assistant messages with termination state; a new useStopGenerating hook that coordinates freezing the display stream, capturing content, and calling the backend mutation; modifications to message processing to track aborted states and expose activeMessage; enhancements to useStreamBuffer for global freeze/thaw mechanics; and updates to error handling in generateAgentResponse to detect and respect cancellation signals. Supporting changes include test coverage for new hooks and mutations, translation strings for UI labels, and .gitignore updates. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 64-65: The .gitignore currently ignores .agent/* which hides the
rules/ directory so the negation !.agent/rules/README.md cannot re-include the
nested file; update the ignore rules to explicitly unignore the intermediate
directory by adding an unignore pattern for .agent/rules/ (e.g., add
!.agent/rules/ before the README negation) so that !.agent/rules/README.md can
take effect.
In `@services/platform/app/features/chat/components/chat-input.tsx`:
- Around line 300-307: The stop button is clickable even when onStopGenerating
is undefined; update the conditional Button (rendered when isLoading is true) to
pass a disabled prop when onStopGenerating is falsy so it becomes
non-interactive and screen-reader friendly; target the Button in chat-input.tsx
that uses onStopGenerating and CircleStop and set disabled={!onStopGenerating}
(and optionally add aria-disabled="true" when disabled) so the UI reflects the
unavailable callback.
In `@services/platform/app/features/chat/components/chat-messages.tsx`:
- Around line 44-45: Remove the unused prop streamingMessage: delete
streamingMessage from the props interface and from the destructured props in the
ChatMessages component (where activeMessage and streamingMessage are listed),
and update any callers (e.g., ChatInterface or other components passing
streamingMessage) to stop supplying it; if you must keep the prop for API
compatibility, rename it to _streamingMessage (prefix underscore) so ESLint
ignores it; ensure ThinkingAnimation continues to use activeMessage as intended.
In
`@services/platform/app/features/chat/hooks/__tests__/use-message-processing.test.ts`:
- Around line 199-234: The test title "prefers streaming over pending"
contradicts its assertions: useMessageProcessing is expected to set
activeMessage to the pending message and streamingMessage to undefined; update
the test to reflect actual behavior by either renaming the test to something
like "prefers pending over streaming" or change the assertions to expect
streaming to be preferred (i.e., activeMessage === streamingMsg,
streamingMessage === streamingMsg, pendingToolResponse === pendingMsg); locate
this test around the useMessageProcessing call and the assertions for
activeMessage, streamingMessage, and pendingToolResponse to make the change.
In
`@services/platform/app/features/chat/hooks/__tests__/use-stream-buffer.test.ts`:
- Around line 969-974: The frozen snapshot mismatch occurs because
consumeFrozenDisplayText() can read more characters than the rendered
displayLength at the moment freezeActiveStream() is called; fix by capturing and
storing the current display length inside freezeActiveStream() (e.g., set
frozenLength = displayLength or a dedicated frozenDisplayLength property) and
update consumeFrozenDisplayText() to use that frozen length when slicing or
returning text (use frozenDisplayLength instead of recomputing/displayLength);
update any related helpers (e.g., where displayLength is derived) so tests
referencing frozenLength/consumeFrozenDisplayText() reliably reflect the exact
rendered portion frozen at freeze time.
- Around line 1284-1303: The hook useStreamBuffer is not setting isDraining to
true when isStreaming transitions to false while displayLength (or buffer) is
still less than text.length; update the effect or state transition inside
useStreamBuffer that responds to changes in isStreaming/text (the same logic
that advances displayLength using initialBufferChars and advanceFrames) to: when
isStreaming becomes false AND displayLength < text.length, set isDraining = true
and keep the draining loop running until displayLength === text.length, then
clear isDraining; ensure the state reset/cleanup still occurs once fully drained
so subsequent renders behave correctly.
- Around line 1096-1147: The resetGlobalFreeze path currently clears the global
flag but does not clear the active instance-level freeze, so resetGlobalFreeze()
must also clear the per-instance frozen state and notify subscribers so
useStreamBuffer instances update immediately; update resetGlobalFreeze to reset
both the globalFrozen flag and the activeFreeze/activeInstance state used by
freezeActiveStream/isStreamFrozen (or call an existing thaw function), and
ensure useStreamBuffer subscribes to that change (or checks the active instance
state) so result.current.displayLength is updated when resetGlobalFreeze() is
called.
In
`@services/platform/convex/lib/agent_response/__tests__/generate_response_error_handling.test.ts`:
- Around line 614-630: The test currently assigns capturedSignalAborted in the
mock but never reads it; update the mockBuildStructuredContext to set
capturedSignalAborted based on the passed-in AbortSignal (e.g.,
capturedSignalAborted = signal?.aborted) during the simulated long-running
operation, then add an assertion in the test (e.g.,
expect(capturedSignalAborted).toBe(true)) after the awaited call so the variable
is used and the test validates the intended cancellation behavior; alternatively
remove capturedSignalAborted entirely if you prefer not to assert signal state.
In `@services/platform/convex/lib/agent_response/generate_response.ts`:
- Around line 611-645: The retry stream uses a new AbortController
(retryAbortController) so it isn't cancelled when the watcher aborts
(abortController); change this so the retry is cancelled by the watcher: either
reuse the existing abortController.signal when calling retryAgent.streamText
(replace retryAbortController with abortController) or add a listener from
abortController.signal to call retryAbortController.abort() before starting the
retry stream; update the call sites around retryAgent.streamText and the await
withTimeout (which currently uses retryAbortController) to use the watcher
abortController or the wired retryAbortController so the watcher stop will
promptly cancel the in-flight retry.
- Around line 134-136: The early-cancel polling loop using streams and
baselineAbortedIds never reliably exits because streams is filtered for
statuses:['aborted'] so streams.length > baselineAbortedIds.size only flips
after a real cancellation; add a bounded guard to end the “early message” phase
regardless: in the generate_response flow introduce a maxEarlyChecks or an
earlyCheckDeadline (use existing generationWindowMs or a new earlyCheckWindowMs)
and, inside the loop that currently sets earlyCheckDone (referencing streams,
baselineAbortedIds, earlyCheckDone), also set earlyCheckDone = true when the
attempt counter exceeds maxEarlyChecks or when Date.now() > earlyCheckDeadline
so early polling is bounded.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (21)
.gitignoreservices/platform/app/features/chat/components/chat-input.tsxservices/platform/app/features/chat/components/chat-interface.tsxservices/platform/app/features/chat/components/chat-messages.tsxservices/platform/app/features/chat/components/message-bubble.tsxservices/platform/app/features/chat/components/message-bubble/types.tsservices/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.tsservices/platform/app/features/chat/hooks/__tests__/use-message-processing.test.tsservices/platform/app/features/chat/hooks/__tests__/use-stop-generating.test.tsservices/platform/app/features/chat/hooks/__tests__/use-stream-buffer.test.tsservices/platform/app/features/chat/hooks/mutations.tsservices/platform/app/features/chat/hooks/use-chat-loading-state.tsservices/platform/app/features/chat/hooks/use-message-processing.tsservices/platform/app/features/chat/hooks/use-stop-generating.tsservices/platform/app/features/chat/hooks/use-stream-buffer.tsservices/platform/convex/lib/agent_response/__tests__/generate_response_error_handling.test.tsservices/platform/convex/lib/agent_response/generate_response.tsservices/platform/convex/threads/cancel_generation.test.tsservices/platform/convex/threads/cancel_generation.tsservices/platform/convex/threads/mutations.tsservices/platform/messages/en.json
larryro
force-pushed
the
fix/chat-stop-generation
branch
2 times, most recently
from
ce17527 to
0360456
Compare
Israeltheminer
force-pushed
the
fix/chat-stop-generation
branch
from
0360456 to
e473689
Compare
larryro
reviewed
Mar 1, 2026
Collaborator
larryro
left a comment
larryro
left a comment
There was a problem hiding this comment.
Overall review: 9 issues found across the PR (4 critical, 3 high, 1 medium, 1 low). The core streaming and cancellation features are well-structured, but several edge cases need attention — particularly around error recovery, race conditions, and the heuristic-based cancellation detection in the catch block.
Israeltheminer
added a commit
that referenced
this pull request
Mar 1, 2026
The chat loading state (isLoading) now uses a combined debounce + creation-time baseline strategy to prevent the stop button from disappearing mid-generation due to Convex subscription oscillation. Previously isPending was cleared the instant a terminal assistant appeared in the subscription data. During active streaming the paginated subscription can briefly flash terminal statuses or hide the streaming message entirely, causing isLoading to go false and the stop button to vanish. Now isPending is only cleared when ALL three conditions hold simultaneously for a 2-second debounce period: 1. The subscription acknowledged the generation (hasSeenActive) 2. No active streaming messages exist (!isMessageActive) 3. A genuinely new terminal assistant appeared (_creationTime after the baseline snapshot taken at send time) Also addresses all PR #608 review comments: stream buffer reset logic, linter fixes, comment cleanup, and cancel-generation test coverage.
… follow-up reliability Switch backend from generateText to streamText with saveStreamDeltas for real-time streaming. Add abort watcher that polls for cancelled streams using baseline snapshots to prevent stale data from killing new generations. Filter early cancellation checks by status==='failed' to distinguish SDK-created messages from user-cancelled ones. On the frontend, add freeze-then-cancel flow (use-stop-generating, use-stream-buffer) so the UI instantly freezes on stop click, and resetGlobalFreeze before each new message to clear stale frozen state. Mark aborted messages (failed + no text) with "Generation stopped" UI.
- Use streamId instead of _id for stream objects (streams.list API doesn't return _id) - Prefix unused variables with _ (capturedSignalAborted, streamingMessage) - Fix filter callback typing in mock assertion - Change null to undefined for UIMessage text field - Fix resetGlobalFreeze to also clear instance-level frozenRef so non-streaming text renders after a freeze+reset cycle - Fix isDraining to use isStreaming prop instead of stale ref value - Add module-level state cleanup to all test afterEach blocks - Use tolerance-based assertions for timing-dependent freeze tests - Use longer test text to prevent adaptive CPS from completing early
After a freeze+reset cycle, wasStreamingRef stayed true from the previous streaming session, causing new non-streaming text to enter the drain branch instead of showing immediately. Register activeWasStreamingRef at module level and clear it in resetGlobalFreeze alongside frozenRef.
The blinking cursor persisted after stopping because freezeActiveStream() could not access the React state setter for isTyping, and isDraining stayed true since wasStreamingRef was never cleared when frozen. Use isStreamFrozen() in TypewriterText's showCursor computation to hide the cursor immediately on freeze. Also guard RAF re-registration in the streaming useEffect to prevent wasteful animation loops post-freeze.
Textarea, paste, and attach button were accessible while a response was being generated. Derive inputDisabled from disabled || isLoading so the entire input area is locked during both streaming and non-streaming generation.
When a user stopped mid-stream, a race condition could cause the fallback message to be concatenated onto already-streamed content via appendToStream. Guard the append+complete block with an abort check. Also adds: post-generation abort check for non-streaming path, bounded early-cancel polling, retry abort controller wiring, contradictory test name fix, and .gitignore unignore pattern fix.
The chat loading state (isLoading) now uses a combined debounce + creation-time baseline strategy to prevent the stop button from disappearing mid-generation due to Convex subscription oscillation. Previously isPending was cleared the instant a terminal assistant appeared in the subscription data. During active streaming the paginated subscription can briefly flash terminal statuses or hide the streaming message entirely, causing isLoading to go false and the stop button to vanish. Now isPending is only cleared when ALL three conditions hold simultaneously for a 2-second debounce period: 1. The subscription acknowledged the generation (hasSeenActive) 2. No active streaming messages exist (!isMessageActive) 3. A genuinely new terminal assistant appeared (_creationTime after the baseline snapshot taken at send time) Also addresses all PR #608 review comments: stream buffer reset logic, linter fixes, comment cleanup, and cancel-generation test coverage.
…ng query Replace the triple-condition debounced clearing strategy with a reactive Convex subscription that checks stream status directly via listStreams. Stream status transitions are atomic and don't oscillate, unlike paginated message subscriptions, making this a reliable source of truth. isLoading = isPending || isGenerating - Add isThreadGenerating query to convex/threads/queries.ts - Simplify useChatLoadingState from ~150 lines to ~75 lines - Remove all client-side heuristics (debounce, creation-time baseline, hasSeenActive, isMessageActive) - Wire up useConvexQuery subscription in chat-interface.tsx - Unify send/stop into single button to prevent DOM remount flicker
Israeltheminer
force-pushed
the
fix/chat-stop-generation
branch
from
7364545 to
c2581d2
Compare
This was referenced Mar 2, 2026
4 tasks
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 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
2 participants
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
generateTexttostreamTextwithsaveStreamDeltasfor real-time streaming, fixing stop button flickering and slow TTFT during tool-call-heavy prompts.baselineAbortedIds,baselineNewestAssistantId) to detect cancellations without false positives from stale data. Filter early checks bystatus === 'failed'to distinguish SDK messages from user-cancelled ones.use-stop-generating+use-stream-buffer) so the UI instantly freezes on stop click.resetGlobalFreezebefore each new message to clear stale frozen state. Aborted messages (failed + no text) show "Generation stopped".cancel_generation.tsmutation that aborts active streams or creates a failed assistant message for early stops.Test plan
Summary by CodeRabbit
Release Notes
New Features
Tests