Fix segmented chat response reconciliation#1469
Merged
senamakel merged 3 commits intoMay 11, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughChatRuntimeProvider adds a bounded, TTL-based per-turn segment-delivery cache and helpers (prune, get/create, take, delete). onSegment buffers segments, onDone takes and compares reconstructed concatenation exactly to event.full_response, and onError deletes delivery state. Tests expand reconciliation, ordering, dedupe, and TTL expiry cases. ChangesSegment Delivery Reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/providers/ChatRuntimeProvider.tsx`:
- Around line 532-538: The segmentDeliveriesRef Map can grow unbounded if
terminal events (chat_done/chat_error) are missed; change creation/use of
entries (where deliveryKey is computed and segmentDeliveriesRef.current.get/set
is used) to a bounded/expiring cache: when creating a new delivery entry put an
object { segments: Map<number,string>, createdAt: number } and on every set/get
prune or replace entries older than a TTL (e.g. 5 minutes) and enforce a
maxEntries limit (LRU or simple oldest-first eviction) so stale turns are
removed automatically; apply the same bounded/expiring logic to all places that
call segmentDeliveriesRef.current.get/set (the blocks using segmentText,
segmentDeliveryKey, delivery.segments.set shown here and the other occurrences
referenced).
- Around line 64-77: The current hasCompleteSegmentDelivery incorrectly uses
event.full_response.includes(segment) which ignores order; instead, in
hasCompleteSegmentDelivery build a concatenated string by iterating indices
0..expected-1, retrieving delivery.segments.get(i) (from the
SegmentDelivery.segments Map), ensure none are undefined, append them in index
order, and finally compare the resulting concatenated string for exact equality
with event.full_response (and keep the existing checks for expected > 0 and
delivery.segments.size >= expected); update the function
hasCompleteSegmentDelivery to use this ordered concatenation equality check to
determine completeness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f6c3eb-c3df-43c6-9524-384092703ea3
📒 Files selected for processing (2)
app/src/providers/ChatRuntimeProvider.tsxapp/src/providers/__tests__/ChatRuntimeProvider.test.tsx
Comment on lines
+64
to
+77
| function hasCompleteSegmentDelivery( | ||
| event: ChatDoneEvent, | ||
| delivery: SegmentDelivery | undefined | ||
| ): boolean { | ||
| const expected = event.segment_total ?? 0; | ||
| if (expected <= 0 || !delivery) return false; | ||
| if (delivery.segments.size < expected) return false; | ||
|
|
||
| for (let i = 0; i < expected; i += 1) { | ||
| const segment = delivery.segments.get(i); | ||
| if (segment === undefined || !event.full_response.includes(segment)) return false; | ||
| } | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Strengthen segment completeness check to preserve ordering correctness.
Current includes checks can mark delivery as “complete” even when segment order/content mapping is wrong, which can incorrectly skip reconciliation.
Proposed fix
function hasCompleteSegmentDelivery(
event: ChatDoneEvent,
delivery: SegmentDelivery | undefined
): boolean {
const expected = event.segment_total ?? 0;
if (expected <= 0 || !delivery) return false;
if (delivery.segments.size < expected) return false;
+ let cursor = 0;
for (let i = 0; i < expected; i += 1) {
const segment = delivery.segments.get(i);
- if (segment === undefined || !event.full_response.includes(segment)) return false;
+ if (segment === undefined) return false;
+ const matchIndex = event.full_response.indexOf(segment, cursor);
+ if (matchIndex < 0) return false;
+ cursor = matchIndex + segment.length;
}
return true;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/providers/ChatRuntimeProvider.tsx` around lines 64 - 77, The current
hasCompleteSegmentDelivery incorrectly uses
event.full_response.includes(segment) which ignores order; instead, in
hasCompleteSegmentDelivery build a concatenated string by iterating indices
0..expected-1, retrieving delivery.segments.get(i) (from the
SegmentDelivery.segments Map), ensure none are undefined, append them in index
order, and finally compare the resulting concatenated string for exact equality
with event.full_response (and keep the existing checks for expected > 0 and
delivery.segments.size >= expected); update the function
hasCompleteSegmentDelivery to use this ordered concatenation equality check to
determine completeness.
Comment on lines
+532
to
+538
| const content = segmentText(event); | ||
| const deliveryKey = segmentDeliveryKey(event.thread_id, event.request_id); | ||
| const delivery = segmentDeliveriesRef.current.get(deliveryKey) ?? { | ||
| segments: new Map<number, string>(), | ||
| }; | ||
| delivery.segments.set(event.segment_index, content); | ||
| segmentDeliveriesRef.current.set(deliveryKey, delivery); |
Contributor
There was a problem hiding this comment.
Bound segmentDeliveriesRef to avoid stale-turn memory growth.
Entries are only removed on chat_done / chat_error. If terminal events are missed, this cache can accumulate for the lifetime of the provider.
Proposed fix
-type SegmentDelivery = { segments: Map<number, string> };
+type SegmentDelivery = { segments: Map<number, string>; updatedAt: number };
// in onSegment
+const now = Date.now();
const delivery = segmentDeliveriesRef.current.get(deliveryKey) ?? {
segments: new Map<number, string>(),
+ updatedAt: now,
};
delivery.segments.set(event.segment_index, content);
+delivery.updatedAt = now;
segmentDeliveriesRef.current.set(deliveryKey, delivery);
+// prune stale deliveries
+const ttlMs = 10 * 60_000;
+for (const [key, value] of segmentDeliveriesRef.current) {
+ if (now - value.updatedAt > ttlMs) {
+ segmentDeliveriesRef.current.delete(key);
+ }
+}Also applies to: 662-666, 767-767
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/providers/ChatRuntimeProvider.tsx` around lines 532 - 538, The
segmentDeliveriesRef Map can grow unbounded if terminal events
(chat_done/chat_error) are missed; change creation/use of entries (where
deliveryKey is computed and segmentDeliveriesRef.current.get/set is used) to a
bounded/expiring cache: when creating a new delivery entry put an object {
segments: Map<number,string>, createdAt: number } and on every set/get prune or
replace entries older than a TTL (e.g. 5 minutes) and enforce a maxEntries limit
(LRU or simple oldest-first eviction) so stale turns are removed automatically;
apply the same bounded/expiring logic to all places that call
segmentDeliveriesRef.current.get/set (the blocks using segmentText,
segmentDeliveryKey, delivery.segments.set shown here and the other occurrences
referenced).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/providers/__tests__/ChatRuntimeProvider.test.tsx (2)
327-368: 💤 Low valueGood coverage for the strict-equality reconciliation branch.
This test exercises exactly the case where
includes-style completeness checks used to mis-classify the turn as complete: the buffered concatenation ('Alpha' + 'Beta') doesn't equalevent.full_response('BetaAlpha'), so reconciliation correctly appends the canonical response and we end at 3 total appends.A tiny note on the test name: "not in full_response order" reads as if the segments themselves arrived out of order, but the scenario is really that the server's
full_responseorders content differently thansegment_index. Renaming to something like "reconciles when buffered segments don't reconstruct full_response" would describe the assertion more precisely. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/providers/__tests__/ChatRuntimeProvider.test.tsx` around lines 327 - 368, The test name is slightly misleading; rename the test description string in the test case for clarity—change the it(...) label currently "reconciles when segments are present but not in full_response order" to a clearer description such as "reconciles when buffered segments don't reconstruct full_response" so it accurately reflects that the server full_response ordering differs from segment_index rather than out-of-order arrivals (update the test case identification string inside the it(...) call in ChatRuntimeProvider.test.tsx).
370-404: 💤 Low valueTTL expiry test is solid; consider also asserting the appended content.
The boundary offset (
SEGMENT_DELIVERY_TTL_MS + 1) andtry/finallyaroundnowSpy.mockRestore()correctly drive the stale-eviction path throughtakeSegmentDelivery→ prune → undefined → reconciliation, and you verify the second append fires after expiry.Optional: in addition to
toHaveBeenCalledTimes(2), asserting the second call's content (e.g.expect.objectContaining({ content: 'Stale segment.', sender: 'agent' })) would lock in that the reconciliation append carries thefull_responsepayload rather than e.g. an empty/error string, in line with the behavior-over-implementation guideline.Proposed addition
await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledTimes(2)); + expect(threadApi.appendMessage).toHaveBeenLastCalledWith( + 't-stale', + expect.objectContaining({ content: 'Stale segment.', sender: 'agent' }) + );As per coding guidelines: "Prefer behavior over implementation in tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/providers/__tests__/ChatRuntimeProvider.test.tsx` around lines 370 - 404, Add an assertion that the reconciliation append actually carries the stale payload: after the second waitFor that checks threadApi.appendMessage call count, assert the second call (threadApi.appendMessage call `#2` / index 1) was invoked with an argument matching the expected message shape (object containing content: 'Stale segment.' and sender: 'agent' or whatever sender your code uses). Locate this in the test around listeners.onSegment / listeners.onDone and use the existing threadApi.appendMessage mock to assert the second call's payload (e.g., via expect.objectContaining or checking mock.calls[1][0]) so the test verifies behavior not just call count; references: listeners.onSegment, listeners.onDone, threadApi.appendMessage, SEGMENT_DELIVERY_TTL_MS / takeSegmentDelivery.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/providers/__tests__/ChatRuntimeProvider.test.tsx`:
- Around line 327-368: The test name is slightly misleading; rename the test
description string in the test case for clarity—change the it(...) label
currently "reconciles when segments are present but not in full_response order"
to a clearer description such as "reconciles when buffered segments don't
reconstruct full_response" so it accurately reflects that the server
full_response ordering differs from segment_index rather than out-of-order
arrivals (update the test case identification string inside the it(...) call in
ChatRuntimeProvider.test.tsx).
- Around line 370-404: Add an assertion that the reconciliation append actually
carries the stale payload: after the second waitFor that checks
threadApi.appendMessage call count, assert the second call
(threadApi.appendMessage call `#2` / index 1) was invoked with an argument
matching the expected message shape (object containing content: 'Stale segment.'
and sender: 'agent' or whatever sender your code uses). Locate this in the test
around listeners.onSegment / listeners.onDone and use the existing
threadApi.appendMessage mock to assert the second call's payload (e.g., via
expect.objectContaining or checking mock.calls[1][0]) so the test verifies
behavior not just call count; references: listeners.onSegment, listeners.onDone,
threadApi.appendMessage, SEGMENT_DELIVERY_TTL_MS / takeSegmentDelivery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88ca465b-3815-455b-9560-fdae2c082269
📒 Files selected for processing (2)
app/src/providers/ChatRuntimeProvider.tsxapp/src/providers/__tests__/ChatRuntimeProvider.test.tsx
senamakel
approved these changes
May 11, 2026
This was referenced May 12, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
Co-authored-by: Jwalin Shah <jshah1331@gmail.com>
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
3 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
Validation
Notes
Summary by CodeRabbit
Bug Fixes
Tests