fix(frontend): interleave thinking messages correctly in snapshots#891
Merged
Gkrumbach07 merged 2 commits intomainfrom Mar 13, 2026
Merged
fix(frontend): interleave thinking messages correctly in snapshots#891Gkrumbach07 merged 2 commits intomainfrom
Gkrumbach07 merged 2 commits intomainfrom
Conversation
WalkthroughThe message-sorting logic in handleMessagesSnapshot was changed to record original indices and use them as a tiebreaker when timestamps are equal or missing, preserving original interleaving while still ordering by timestamp when available. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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)
📝 Coding Plan
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 |
MESSAGES_SNAPSHOT sort was unstable for messages with identical timestamps, causing thinking blocks to group together instead of being interleaved with agent messages. Now preserves original snapshot order as tiebreaker. Fixes: RHOAIENG-52655 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ambient-code
bot
force-pushed
the
fix/thinking-messages-stacking-52655
branch
from
db5f7b7 to
4a5e0b2
Compare
Contributor
🤖 PR Fix ReportSummaryPR #891 has been successfully rebased and validated. All checks passing. Actions Taken✅ Rebased onto latest main
✅ Reviewed feedback
✅ Code quality review
✅ Validation
CI Status
Commits Pushed🤖 Automated fix by PR Fixer Bot |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/hooks/agui/event-handlers.ts`:
- Around line 853-864: The tie-breaker currently builds originalOrder from
filtered (variable filtered) so equal-timestamp messages keep the pre-reconnect
local ordering; change the comparator to use the snapshot/normalized order
instead: derive originalOrder (or a separate snapshotOrder map) from
normalizedMessages and use that map in the sort comparator (inside
filtered.sort) as the fallback when timestamps are equal or missing so messages
are reordered to match the snapshot's normalizedMessages order rather than the
existing filtered order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf4ae894-ece2-4e0c-9564-181ef328ddf0
📒 Files selected for processing (1)
components/frontend/src/hooks/agui/event-handlers.ts
Comment on lines
+853
to
+864
| const originalOrder = new Map(filtered.map((msg, idx) => [msg.id, idx])) | ||
| filtered.sort((a, b) => { | ||
| const ta = a.timestamp ? new Date(a.timestamp).getTime() : null | ||
| const tb = b.timestamp ? new Date(b.timestamp).getTime() : null | ||
| if (ta == null && tb == null) return 0 | ||
| if (ta == null) return 0 // keep relative position | ||
| if (tb == null) return 0 | ||
| return ta - tb | ||
|
|
||
| if (ta != null && tb != null) { | ||
| const diff = ta - tb | ||
| if (diff !== 0) return diff | ||
| } | ||
|
|
||
| // Equal or missing timestamps: preserve original snapshot order | ||
| return (originalOrder.get(a.id) ?? 0) - (originalOrder.get(b.id) ?? 0) |
There was a problem hiding this comment.
Use snapshot order, not merged state order, as the tie-breaker.
originalOrder is derived from filtered, which already inherits state.messages ordering for existing IDs. If the local order is wrong before reconnect, equal-timestamp messages stay wrong after the snapshot too, because this sort never reorders them to match normalizedMessages. That means the reconnect path can still leave thinking blocks stacked instead of interleaved.
Proposed fix
- const originalOrder = new Map(filtered.map((msg, idx) => [msg.id, idx]))
+ const snapshotOrder = new Map(normalizedMessages.map((msg, idx) => [msg.id, idx]))
+ const mergedOrder = new Map(filtered.map((msg, idx) => [msg.id, idx]))
filtered.sort((a, b) => {
const ta = a.timestamp ? new Date(a.timestamp).getTime() : null
const tb = b.timestamp ? new Date(b.timestamp).getTime() : null
if (ta != null && tb != null) {
const diff = ta - tb
if (diff !== 0) return diff
}
- // Equal or missing timestamps: preserve original snapshot order
- return (originalOrder.get(a.id) ?? 0) - (originalOrder.get(b.id) ?? 0)
+ // Equal or missing timestamps: prefer authoritative snapshot order.
+ // Fall back to current merged order for messages not present in the snapshot.
+ return (snapshotOrder.get(a.id) ?? mergedOrder.get(a.id) ?? 0)
+ - (snapshotOrder.get(b.id) ?? mergedOrder.get(b.id) ?? 0)
})📝 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
| const originalOrder = new Map(filtered.map((msg, idx) => [msg.id, idx])) | |
| filtered.sort((a, b) => { | |
| const ta = a.timestamp ? new Date(a.timestamp).getTime() : null | |
| const tb = b.timestamp ? new Date(b.timestamp).getTime() : null | |
| if (ta == null && tb == null) return 0 | |
| if (ta == null) return 0 // keep relative position | |
| if (tb == null) return 0 | |
| return ta - tb | |
| if (ta != null && tb != null) { | |
| const diff = ta - tb | |
| if (diff !== 0) return diff | |
| } | |
| // Equal or missing timestamps: preserve original snapshot order | |
| return (originalOrder.get(a.id) ?? 0) - (originalOrder.get(b.id) ?? 0) | |
| const snapshotOrder = new Map(normalizedMessages.map((msg, idx) => [msg.id, idx])) | |
| const mergedOrder = new Map(filtered.map((msg, idx) => [msg.id, idx])) | |
| filtered.sort((a, b) => { | |
| const ta = a.timestamp ? new Date(a.timestamp).getTime() : null | |
| const tb = b.timestamp ? new Date(b.timestamp).getTime() : null | |
| if (ta != null && tb != null) { | |
| const diff = ta - tb | |
| if (diff !== 0) return diff | |
| } | |
| // Equal or missing timestamps: prefer authoritative snapshot order. | |
| // Fall back to current merged order for messages not present in the snapshot. | |
| return (snapshotOrder.get(a.id) ?? mergedOrder.get(a.id) ?? 0) | |
| - (snapshotOrder.get(b.id) ?? mergedOrder.get(b.id) ?? 0) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/hooks/agui/event-handlers.ts` around lines 853 - 864,
The tie-breaker currently builds originalOrder from filtered (variable filtered)
so equal-timestamp messages keep the pre-reconnect local ordering; change the
comparator to use the snapshot/normalized order instead: derive originalOrder
(or a separate snapshotOrder map) from normalizedMessages and use that map in
the sort comparator (inside filtered.sort) as the fallback when timestamps are
equal or missing so messages are reordered to match the snapshot's
normalizedMessages order rather than the existing filtered order.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/frontend/src/hooks/agui/event-handlers.ts (1)
853-864:⚠️ Potential issue | 🟠 MajorUse snapshot order as the tie-breaker, not merged
filteredorder.At Line 853,
originalOrderis derived fromfiltered, so equal/missing-timestamp items keep pre-reconnect local ordering. If that ordering is already wrong, reconnect replay still won’t re-interleave thinking blocks correctly.Proposed fix
- const originalOrder = new Map(filtered.map((msg, idx) => [msg.id, idx])) + const snapshotOrder = new Map(normalizedMessages.map((msg, idx) => [msg.id, idx])) + const mergedOrder = new Map(filtered.map((msg, idx) => [msg.id, idx])) filtered.sort((a, b) => { const ta = a.timestamp ? new Date(a.timestamp).getTime() : null const tb = b.timestamp ? new Date(b.timestamp).getTime() : null if (ta != null && tb != null) { const diff = ta - tb if (diff !== 0) return diff } - // Equal or missing timestamps: preserve original snapshot order - return (originalOrder.get(a.id) ?? 0) - (originalOrder.get(b.id) ?? 0) + // Equal or missing timestamps: prefer authoritative snapshot order, + // then fall back to merged order for non-snapshot entries. + return (snapshotOrder.get(a.id) ?? mergedOrder.get(a.id) ?? 0) + - (snapshotOrder.get(b.id) ?? mergedOrder.get(b.id) ?? 0) })As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/hooks/agui/event-handlers.ts` around lines 853 - 864, The tie-breaker uses originalOrder built from filtered, which preserves the current (possibly incorrect) merged order; change originalOrder to map IDs from the pre-reconnect snapshot array (the saved snapshot used to rebuild messages) so the sort in filtered.sort(...) uses snapshot order as the stable tie-breaker; update the creation of originalOrder (and any variable name if needed) to reference the snapshot message IDs instead of filtered to ensure reconnect replay re-interleaves blocks correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/frontend/src/hooks/agui/event-handlers.ts`:
- Around line 853-864: The tie-breaker uses originalOrder built from filtered,
which preserves the current (possibly incorrect) merged order; change
originalOrder to map IDs from the pre-reconnect snapshot array (the saved
snapshot used to rebuild messages) so the sort in filtered.sort(...) uses
snapshot order as the stable tie-breaker; update the creation of originalOrder
(and any variable name if needed) to reference the snapshot message IDs instead
of filtered to ensure reconnect replay re-interleaves blocks correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 914456c1-1579-4ced-82a1-b061acaa5ef8
📒 Files selected for processing (1)
components/frontend/src/hooks/agui/event-handlers.ts
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
Test plan
Fixes: RHOAIENG-52655
🤖 Generated with Claude Code
Jira: RHOAIENG-52655