Fix #1531: long conversations no longer truncate to oldest 200 + preserve SSE state on mid-session recovery#1538
Open
ztech-gthb wants to merge 3 commits intocoleam00:devfrom
Open
Conversation
added 2 commits
May 2, 2026 17:06
…#1531) ORDER BY ASC LIMIT N returns the OLDEST N — for any conversation past the cap (default 200) the most recent messages were silently invisible to the UI. Switch to ORDER BY DESC LIMIT N (newest N) and reverse to oldest-first for the chronological-display contract callers expect. Existing test fixture updated to match.
…lacing (coleam00#1531) When SSE timing causes a stuck thinking placeholder, the recovery path used to overwrite the entire React message state with the fetched DB snapshot. Anything in React state that wasn't in the server's window — client-only system messages, in-flight tool-call/streaming state, messages newer than the snapshot — was silently dropped. Merge by id instead so non-DB state survives. Combined with the messages.ts change, this also fixes the 'jump back to oldest 200' symptom mid-session triggered by sync-burst SSE events from coleam00#1516.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDB query for messages now selects the newest window (ORDER BY created_at DESC, id DESC) and reverses results to preserve oldest-first output; the web client’s stuck-placeholder recovery merges hydrated DB messages with existing client messages by ID/timestamp instead of replacing state. ChangesMessage Query and Recovery Merge
Sequence Diagram(s)sequenceDiagram
participant UI as Client(UI)
participant API as Server(API)
participant DB as Database
participant SSE as SSE Stream
UI->>SSE: receive events (streaming placeholders, live messages)
SSE-->>UI: streaming messages (isStreaming placeholders)
UI->>API: onLockChange(false) -> GET /messages (limit=N)
API->>DB: SELECT ... ORDER BY created_at DESC, id DESC LIMIT N
DB-->>API: rows (newest-first window)
API-->>UI: hydrated rows (oldest-first after reverse)
UI->>UI: compute hydratedIds, newestHydratedTs
UI->>UI: filter prev by ID & timestamp (preserve streaming, system, tool-calls, newer-than-hydrated)
UI->>UI: merge hydrated + preserved, sort by timestamp
UI-->>UI: render merged message list (no silent loss)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🧹 Nitpick comments (1)
packages/core/src/db/messages.test.ts (1)
106-116: ⚡ Quick winStrengthen this ordering test with distinct timestamps.
Right now both fixtures effectively share
created_at, so reversal is validated, but chronological intent is under-specified. Give rows different timestamps and assert final ID order explicitly.Suggested patch
const newestFirst: MessageRow[] = [ - { ...mockMessage, id: 'msg-124', role: 'assistant', content: 'Hi!' }, - mockMessage, + { + ...mockMessage, + id: 'msg-124', + role: 'assistant', + content: 'Hi!', + created_at: '2025-01-01T00:00:01.000Z', + }, + { ...mockMessage, created_at: '2025-01-01T00:00:00.000Z' }, ]; @@ - expect(result).toEqual([...newestFirst].reverse()); + expect(result.map(m => m.id)).toEqual(['msg-123', 'msg-124']);As per coding guidelines,
**/*.test.ts: “keep tests deterministic — no flaky timing or network dependence without guardrails.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/db/messages.test.ts` around lines 106 - 116, The test uses two MessageRow fixtures (newestFirst) that currently share the same created_at, so the reversal assertion doesn't prove chronological ordering; modify the fixtures used in newestFirst to include distinct created_at timestamps (e.g., older timestamp on the second element and newer on the first) and then assert the result by checking explicit ID order (e.g., expect(result.map(r => r.id)).toEqual([...])) after calling listMessages; keep mockQuery.mockResolvedValueOnce(createQueryResult(newestFirst)) and the call to listMessages unchanged, only change the created_at values and the final expectation to assert IDs in oldest-first order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/db/messages.ts`:
- Around line 68-69: The SQL ordering in the query string that currently uses
"ORDER BY created_at DESC LIMIT $2" is non-deterministic when multiple rows
share the same timestamp; update that SQL in messages.ts to add a stable
secondary key (e.g., the primary key column such as id or message_id) to the
ORDER BY clause (for example: ORDER BY created_at DESC, id DESC) so the LIMIT
window is deterministic; update any related query strings/constants in this file
(the one containing ORDER BY created_at DESC LIMIT $2) and run relevant tests.
In `@packages/web/src/components/chat/ChatInterface.tsx`:
- Around line 459-464: The merge keeps every prev message not in hydrated
(clientOnly) which preserves placeholder/optimistic duplicates (e.g., thinking-*
and msg-*) and can leave isStreaming stuck; update the setMessages merge so
clientOnly only retains items that truly have no canonical counterpart or are
actively streaming: filter prev to exclude IDs matching optimistic prefixes
(like /^thinking-/ or /^msg-/) unless the message.isStreaming is true, and also
drop optimistic entries when a hydrated row corresponds to the same
tempId/client-side id (compare a tempId or clientId field on prev items against
hydrated entries) so you don't keep duplicates; update the clientOnly
computation used in setMessages to implement these checks before merging and
sorting.
---
Nitpick comments:
In `@packages/core/src/db/messages.test.ts`:
- Around line 106-116: The test uses two MessageRow fixtures (newestFirst) that
currently share the same created_at, so the reversal assertion doesn't prove
chronological ordering; modify the fixtures used in newestFirst to include
distinct created_at timestamps (e.g., older timestamp on the second element and
newer on the first) and then assert the result by checking explicit ID order
(e.g., expect(result.map(r => r.id)).toEqual([...])) after calling listMessages;
keep mockQuery.mockResolvedValueOnce(createQueryResult(newestFirst)) and the
call to listMessages unchanged, only change the created_at values and the final
expectation to assert IDs in oldest-first order.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1822c668-c3ab-4a94-a6d6-5df456900241
📒 Files selected for processing (3)
packages/core/src/db/messages.test.tspackages/core/src/db/messages.tspackages/web/src/components/chat/ChatInterface.tsx
Collaborator
|
@ztech-gthb related to #1531 — conversation truncation fix. |
… merge (CodeRabbit review)
Contributor
Author
|
Both findings addressed in the latest commit: (1) ORDER BY now uses |
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
ORDER BY DESC LIMIT Nthen reversed to chronological —listMessagesnow returns the newest N in oldest-first order. Stuck-placeholder hydration inChatInterfacemerges by message id instead of replacing — anything in React state that the snapshot doesn't include (system banners, in-flight tool-calls, post-snapshot SSE messages) survives.ChatInterfacemount-time hydration path (already had a merge-by-id branch and isn't the source of mid-session loss),getRecentWorkflowResultMessages(different ordering contract — DESC + early termination, intentionally newest-first).UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
messages.ts:listMessagesmessages.ts:listMessagesChatInterface:stuck-placeholderChatInterface:mount-fetchgetRecentWorkflowResultMessagesLabel Snapshot
risk: low— DB query change is order/limit only (no schema, no semantics shift); React change is purely additive (anything in DB still gets through).size: Score, webcore:db.messages,web:chat.ChatInterfaceChange Metadata
bugmultiLinked Issue
Validation Evidence (required)
Per-commit: lint-staged (eslint --max-warnings 0 + prettier --write) clean on every staged file.
messages.test.tstest renamed and updated to assert DESC + reversal explicitly. No other fixtures needed update — the API and UI contracts (oldest-first as caller-visible order) are unchanged.Security Impact (required)
limitargument exactly as before.Compatibility / Migration
listMessagesunchanged: returns oldest-first up tolimitrows. The change is which up-to-limit rows are returned (newest N instead of oldest N).GET /api/conversations/:id/messages?limit=Nis unchanged in shape — only the row selection rule differs. Existing client code that doesn't passlimitcontinues to work; for conversations under 200 messages, behaviour is identical.Human Verification (required)
bun run type-checkclean.messages.test.tsupdated test passes (DESC ordering, reversed result).ChatInterface.tsxmerge logic —hydratedIdsset excludes DB-known ids,clientOnlyfilter retains the rest, sort by timestamp produces the natural chronological output.prev(mount-time):clientOnlyis empty, merged === hydrated.hydrated(very early in fresh conversation): early return preserved (if (rows.length === 0) return;).msg-{ts}IDs for SSE messages and DB UUIDs for persisted): hydrated wins (in the merged array first, but sort is stable and timestamps decide; the comment in ChatInterface explicitly relies on themsg-{ts}vs DB UUID disjunction, which this PR preserves).Side Effects / Blast Radius (required)
messages.ts:listMessagesis called only from one server route (api.ts:/messages) — verified via grep acrosspackages/. The change there is "what subset of rows you get" without changing types or shape. WebChatInterfacechange is contained to the stuck-placeholder branch.getRecentWorkflowResultMessages(in the same file) is not affected — different query, different ordering contract.Rollback Plan (required)
git revertthe two commits (a498e788+5260d6bd). One file each on the DB side, one on the UI side; no schema or persistent state to migrate.Risks and Mitigations
listMessagesto return oldest N rather than newest N (i.e. depends on the prior bug).api.ts:/messagesroute) consumes the function. The route serves the Web UI's chat-history fetch — newest N is what users would expect. No other consumer searched up via grep.?before=<id>.msg-{timestamp}ids that never collide with DB UUIDs. The existing comment at the mount-fetch path explicitly notes this; this PR maintains it.🤖 Investigation, write-up, and implementation produced with extensive back-and-forth using Claude — final code, naming choices, and architectural decisions reviewed and accepted by ztech-gthb.
Summary by CodeRabbit