feat(platform): add optimistic messages for existing chat threads#627
Merged
Conversation
…ove loading reliability Extend optimistic message support to existing threads using a lastMessageKey baseline captured at send time to detect when real messages arrive. Add terminalAssistantCount as a React 18 batching-immune backup for clearing isPending on slow networks. Save failed assistant messages on generation errors so the frontend loading state always resolves.
![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.
Contributor
📝 WalkthroughWalkthroughThe PR introduces a Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts`:
- Around line 559-583: Add a second assertion in the existing test that
simulates hydration jump from terminalAssistantCount 0 to a positive N while
isPending remains true and isGenerating is false: renderHook using
useChatLoadingState with initialProps terminalAssistantCount: 0 and isPending:
true, then rerender with terminalAssistantCount: (e.g. 1) and the same
isPending/isGenerating/pendingThreadId/threadId, and assert setIsPending was not
called with false; reference the existing renderHook/rerender calls, the
useChatLoadingState hook, and the setIsPending mock to implement this 0 -> N
regression case.
In `@services/platform/app/features/chat/hooks/use-chat-loading-state.ts`:
- Around line 29-32: Update the JSDoc in use-chat-loading-state.ts to reference
the correct React versions: replace the "React 18" mention with a
version-agnostic phrase like "React 18+" (or explicitly "React 18+ / React
19.2.4") so the comment about batching behavior that affects isGenerating and
the terminalAssistantCount fallback is accurate for the project's React 19.2.4;
ensure the sentence still explains that batching can coalesce the isGenerating
true→false toggle and that terminalAssistantCount is a monotonically-increasing
backup.
- Around line 75-81: The conditional in use-chat-loading-state.ts redundantly
null-coalesces pendingThreadId; update the if-statement so that after checking
pendingThreadId !== null you directly compare pendingThreadId to threadId (e.g.,
use pendingThreadId !== threadId) instead of (pendingThreadId ?? null) !==
(threadId ?? null), leaving the surrounding checks (isPending, pendingThreadId
!== null) and the call to setIsPending(false) intact to preserve behavior;
reference the variables isPending, pendingThreadId, threadId and the
setIsPending call when locating the change.
In `@services/platform/app/features/chat/hooks/use-pending-messages.ts`:
- Around line 37-41: The current isMatchingThread logic incorrectly treats
threadId === undefined as a wildcard; change the predicate in
use-pending-messages (the isMatchingThread check using pendingMessage.threadId
and threadId) to only match when pendingMessage.threadId === threadId or
pendingMessage.threadId === 'pending' (remove the threadId === undefined
condition) so new-chat route doesn't clear/surface unrelated threads; apply the
same fix to the second occurrence of the same check later in the file (the other
isMatchingThread instance around the 65-71 logic).
In `@services/platform/convex/lib/agent_chat/internal_actions.ts`:
- Around line 331-345: The current caller-side guard in the block that calls
ctx.runQuery(components.agent.streams.list, ...) then
ctx.runMutation(internal.streaming.internal_mutations.errorStream, ...) still
leaves a race where a stream can become aborted after the query; move the
protection into the mutation itself by implementing the check+write atomically
inside the errorStream mutation (read the stream status and only set error if
status !== 'aborted' in the same transaction) so callers (including this code
path using ctx.runMutation(internal.streaming.internal_mutations.errorStream, {
streamId })) no longer need the non-atomic pre-check; alternatively, if you
prefer the existing pattern, add a short comment referencing PR `#397` and
explicitly document that caller-side silent-early-return guards are the chosen
convention.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
services/platform/app/features/chat/components/chat-interface.tsxservices/platform/app/features/chat/context/chat-layout-context.tsxservices/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.tsservices/platform/app/features/chat/hooks/__tests__/use-pending-messages.test.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-pending-messages.tsservices/platform/app/features/chat/hooks/use-send-message.tsservices/platform/convex/lib/agent_chat/internal_actions.ts
Comment on lines
+559
to
+583
| it('handles terminalAssistantCount of 0 when messages not yet loaded', () => { | ||
| const { rerender } = renderHook((props) => useChatLoadingState(props), { | ||
| initialProps: { | ||
| isPending: true, | ||
| setIsPending, | ||
| isGenerating: false, | ||
| threadId: THREAD_A, | ||
| pendingThreadId: THREAD_A, | ||
| terminalAssistantCount: 0, | ||
| }, | ||
| }); | ||
|
|
||
| expect(setIsPending).not.toHaveBeenCalledWith(false); | ||
|
|
||
| rerender({ | ||
| isPending: true, | ||
| setIsPending, | ||
| isGenerating: false, | ||
| threadId: THREAD_A, | ||
| pendingThreadId: THREAD_A, | ||
| terminalAssistantCount: 0, | ||
| }); | ||
|
|
||
| expect(setIsPending).not.toHaveBeenCalledWith(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a hydration-jump regression case for terminal baseline logic.
This case only validates 0 -> 0. Please add 0 -> N (with isPending: true, isGenerating: false) to ensure historical message hydration doesn’t clear pending prematurely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts`
around lines 559 - 583, Add a second assertion in the existing test that
simulates hydration jump from terminalAssistantCount 0 to a positive N while
isPending remains true and isGenerating is false: renderHook using
useChatLoadingState with initialProps terminalAssistantCount: 0 and isPending:
true, then rerender with terminalAssistantCount: (e.g. 1) and the same
isPending/isGenerating/pendingThreadId/threadId, and assert setIsPending was not
called with false; reference the existing renderHook/rerender calls, the
useChatLoadingState hook, and the setIsPending mock to implement this 0 -> N
regression case.
Comment on lines
+29
to
+32
| * On slow networks (3G), React 18 may batch the isGenerating true→false toggle | ||
| * into a single render, so the handoff never fires. `terminalAssistantCount` | ||
| * acts as a backup: it's a monotonically-increasing integer that cannot be | ||
| * coalesced away by batching. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: JSDoc references React 18 but project uses React 19.
The batching behavior described applies to React 18+, including React 19.2.4 which this project uses. Consider updating the reference for accuracy.
📝 Suggested documentation update
- * On slow networks (3G), React 18 may batch the isGenerating true→false toggle
+ * On slow networks (3G), React may batch the isGenerating true→false toggle📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * On slow networks (3G), React 18 may batch the isGenerating true→false toggle | |
| * into a single render, so the handoff never fires. `terminalAssistantCount` | |
| * acts as a backup: it's a monotonically-increasing integer that cannot be | |
| * coalesced away by batching. | |
| * On slow networks (3G), React may batch the isGenerating true→false toggle | |
| * into a single render, so the handoff never fires. `terminalAssistantCount` | |
| * acts as a backup: it's a monotonically-increasing integer that cannot be | |
| * coalesced away by batching. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/hooks/use-chat-loading-state.ts` around
lines 29 - 32, Update the JSDoc in use-chat-loading-state.ts to reference the
correct React versions: replace the "React 18" mention with a version-agnostic
phrase like "React 18+" (or explicitly "React 18+ / React 19.2.4") so the
comment about batching behavior that affects isGenerating and the
terminalAssistantCount fallback is accurate for the project's React 19.2.4;
ensure the sentence still explains that batching can coalesce the isGenerating
true→false toggle and that terminalAssistantCount is a monotonically-increasing
backup.
Comment on lines
+75
to
+81
| if ( | ||
| isPending && | ||
| pendingThreadId !== null && | ||
| (pendingThreadId ?? null) !== (threadId ?? null) | ||
| ) { | ||
| setIsPending(false); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: Redundant nullish coalescing on pendingThreadId.
Since line 77 already checks pendingThreadId !== null, the (pendingThreadId ?? null) on line 78 always evaluates to pendingThreadId. This can be simplified.
♻️ Simplified condition
// Thread mismatch: navigated away from the pending thread
if (
isPending &&
pendingThreadId !== null &&
- (pendingThreadId ?? null) !== (threadId ?? null)
+ pendingThreadId !== (threadId ?? null)
) {
setIsPending(false);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/hooks/use-chat-loading-state.ts` around
lines 75 - 81, The conditional in use-chat-loading-state.ts redundantly
null-coalesces pendingThreadId; update the if-statement so that after checking
pendingThreadId !== null you directly compare pendingThreadId to threadId (e.g.,
use pendingThreadId !== threadId) instead of (pendingThreadId ?? null) !==
(threadId ?? null), leaving the surrounding checks (isPending, pendingThreadId
!== null) and the call to setIsPending(false) intact to preserve behavior;
reference the variables isPending, pendingThreadId, threadId and the
setIsPending call when locating the change.
Comment on lines
+37
to
+41
| const isMatchingThread = | ||
| pendingMessage.threadId === threadId || | ||
| threadId === undefined || | ||
| pendingMessage.threadId === 'pending'; | ||
| if (!isMatchingThread) return; |
Contributor
There was a problem hiding this comment.
Do not treat threadId === undefined as a wildcard thread match.
Current matching can surface or clear pending state from unrelated threads on the new-chat route.
💡 Proposed fix
- const isMatchingThread =
- pendingMessage.threadId === threadId ||
- threadId === undefined ||
- pendingMessage.threadId === 'pending';
+ const isMatchingThread =
+ pendingMessage.threadId === threadId ||
+ (threadId === undefined && pendingMessage.threadId === 'pending');
...
- const isMatchingThread =
- pendingMessage &&
- (pendingMessage.threadId === threadId ||
- threadId === undefined ||
- pendingMessage.threadId === 'pending');
+ const isMatchingThread =
+ pendingMessage &&
+ (pendingMessage.threadId === threadId ||
+ (threadId === undefined && pendingMessage.threadId === 'pending'));Also applies to: 65-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/hooks/use-pending-messages.ts` around
lines 37 - 41, The current isMatchingThread logic incorrectly treats threadId
=== undefined as a wildcard; change the predicate in use-pending-messages (the
isMatchingThread check using pendingMessage.threadId and threadId) to only match
when pendingMessage.threadId === threadId or pendingMessage.threadId ===
'pending' (remove the threadId === undefined condition) so new-chat route
doesn't clear/surface unrelated threads; apply the same fix to the second
occurrence of the same check later in the file (the other isMatchingThread
instance around the 65-71 logic).
Comment on lines
+331
to
+345
| try { | ||
| // Check for user-initiated abort to avoid overwriting cancellation status | ||
| const streams = await ctx.runQuery(components.agent.streams.list, { | ||
| threadId, | ||
| statuses: ['aborted'], | ||
| }); | ||
| const alreadyAborted = streams.some( | ||
| (s: { streamId: string }) => s.streamId === streamId, | ||
| ); | ||
| if (!alreadyAborted) { | ||
| await ctx.runMutation( | ||
| internal.streaming.internal_mutations.errorStream, | ||
| { streamId }, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find streaming internal mutation files
echo "=== Searching for streaming internal mutations ==="
fd "internal_mutations" services/platform/convex --type f
# Search for errorStream implementation
echo -e "\n=== Searching for errorStream definition ==="
rg -n "errorStream|export.*errorStream" services/platform/convex --type ts
# Look for stream status handling logic
echo -e "\n=== Searching for stream status transitions and aborted checks ==="
rg -n "status.*aborted|aborted.*status|streamStatus" services/platform/convex --type ts -A 3 -B 1
# Inspect the specific mutation function
echo -e "\n=== Looking for error/fail stream mutations ==="
rg -n "export.*const.*(error|fail)Stream|function.*Stream" services/platform/convex/lib --type ts -A 5Repository: tale-project/tale
Length of output: 14162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the errorStream mutation implementation
echo "=== errorStream mutation implementation ==="
sed -n '70,120p' services/platform/convex/streaming/internal_mutations.ts
# Also check stream schema to understand status field
echo -e "\n=== Stream status schema and mutations ==="
rg -n "streamStatus|status.*streaming|finished|aborted" services/platform/convex/streaming -A 2 -B 2 --type ts | head -80
# Look at full errorStream definition
echo -e "\n=== Full errorStream context ==="
rg -n "export const errorStream" services/platform/convex/streaming/internal_mutations.ts -A 20Repository: tale-project/tale
Length of output: 1212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for setStreamStatus implementation
echo "=== Searching for setStreamStatus implementation ==="
rg -n "setStreamStatus|export.*setStreamStatus" services/platform/convex --type ts -A 10 | head -60
# Check persistentTextStreaming lib
echo -e "\n=== Looking in persistentTextStreaming ==="
find services/platform/convex -path "*persistentTextStreaming*" -name "*.ts" -type f | head -20
# Check for any status validation before setting
echo -e "\n=== Checking for status transition logic ==="
rg -n "status.*error|error.*status|transition" services/platform/convex/streaming --type ts -B 2 -A 2Repository: tale-project/tale
Length of output: 5488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check test file that mentions skipping errorStream for aborted streams
echo "=== Test context for aborted stream handling ==="
sed -n '523,570p' services/platform/convex/lib/agent_response/__tests__/generate_response_error_handling.test.ts
# Check if there are comments or docs about race conditions
echo -e "\n=== Searching for race condition handling comments ==="
rg -n "race|race condition|abort.*before|concurrent" services/platform/convex/lib/agent_chat services/platform/convex/lib/agent_response --type ts -B 2 -A 2
# Look at the internal_actions.ts code more fully to see context
echo -e "\n=== Full context of the guard in internal_actions.ts ==="
sed -n '325,350p' services/platform/convex/lib/agent_chat/internal_actions.tsRepository: tale-project/tale
Length of output: 5104
The guard is already present, but the race window persists with current best-effort approach.
The code at line 340 already checks if (!alreadyAborted) before calling errorStream—this is intentional to avoid overwriting cancellation status (as stated in the comment). However, the race condition between the list query (lines 333–336) and the mutation invocation (line 342) still exists: the stream can transition to aborted after the check completes.
For truly atomic semantics, move the guard into the errorStream mutation itself (read the current status + conditionally write in a single transaction). This would close the race window entirely. Alternatively, document that the caller-side guard is the accepted pattern for this codebase (similar to approval mutations in PR #397, which use silent early returns for race protection).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/lib/agent_chat/internal_actions.ts` around lines 331
- 345, The current caller-side guard in the block that calls
ctx.runQuery(components.agent.streams.list, ...) then
ctx.runMutation(internal.streaming.internal_mutations.errorStream, ...) still
leaves a race where a stream can become aborted after the query; move the
protection into the mutation itself by implementing the check+write atomically
inside the errorStream mutation (read the stream status and only set error if
status !== 'aborted' in the same transaction) so callers (including this code
path using ctx.runMutation(internal.streaming.internal_mutations.errorStream, {
streamId })) no longer need the non-atomic pre-check; alternatively, if you
prefer the existing pattern, add a short comment referencing PR `#397` and
explicitly document that caller-side silent-early-return guards are the chosen
convention.
…cleanup Extract derived scalars (currentLastKey, hasMessages) outside the effect to avoid re-running on every streaming content update. Export the params type for proper test typing.
5 tasks
3 tasks
This was referenced Mar 23, 2026
This was referenced Apr 3, 2026
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
This was referenced Apr 9, 2026
This was referenced Apr 23, 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
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
lastMessageKeyfield on pending messages to anchor optimistic messages to the correct position and detect when the real message arrives.terminalAssistantCount— the hook now clearsisPendingwhen a new assistant response is committed to the thread, covering edge cases whereisGeneratingnever flips to true.internal_actions.tsto prevent silent failures during agent chat processing.Test plan
useChatLoadingStatecovering terminal assistant count transitions, baseline resets, and subsequent sendsusePendingMessagescovering existing thread append, real message replacement, pagination stability, cross-thread isolation, and no-duplicate guarantees🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes