feat: add after_id pagination for chat messages#24531
Conversation
Adds an optional after_id query parameter to
GET /api/experimental/chats/{chat}/messages alongside the existing
before_id, so callers can poll for messages newer than a known ID
without opening a websocket. Queued messages are now only returned
when neither cursor is set.
SQL picks up the new bound via an additive CASE clause. The SDK
ChatMessagesPaginationOptions gains an AfterID field. The docs
section is updated with a query-parameter table.
Refs: CODAGT-206
Co-authored-by: Coder Agents <noreply@coder.com>
david-fraley
changed the title
Keeps the TypeScript client in sync with the SDK change to ChatMessagesPaginationOptions added in the parent commit. Existing callers (infinite-scroll) continue to work unchanged.
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped addition. The SQL is symmetric with the existing before_id clause, the SDK field is additive, tests cover the six semantic cells plus validation. CI passes. The queued-messages guard tightening is well-motivated and the comment rewrite earns its place.
Severity count: 1 P2, 2 P3, 7 Nit.
The P2 drew convergence from 10 of 17 reviewers (severities ranging P1 to P3). The core concern: after_id on a DESC-ordered, limit-capped query is the wrong shape for the polling use case the PR advertises. A burst larger than limit silently drops the oldest new messages, and the recovery protocol (dual-cursor walk) is neither documented nor tested. GetChatMessagesByChatIDAscPaginated already exists in chats.sql:243 and would eliminate the hazard for the polling case.
Panelist quote worth preserving, from Hisoka: "Shall I show you the race? The naive poller's next tick sets after_id=N+60 (the newest they just saw). [N+1..N+10] are lost forever. No error, no warning, the queue simply skipped ten messages."
Two pre-existing notes surfaced but not posted as findings: (1) the stream handler at exp_chats.go:2568 uses raw strconv.ParseInt for the same after_id parameter name while this handler uses PositiveInt64, creating two validation regimes on adjacent routes (Knov, Zoro); (2) PostgreSQL sequence allocation outside transactions means cursor-based polling can skip messages that commit out of order, currently mitigated by the single-writer-per-chat invariant (Knuckle). Neither is introduced by this PR.
Commit scope coderd/exp_chats is not a real filesystem path per AGENTS.md and the change spans 8 files across multiple directories. Cross-cutting changes should omit the scope or use a broader path (Leorio).
coderd/x/chatd/chatd_internal_test.go:386
Nit [DEREM-2] The PR adds explicit AfterID: 0 in chatd.go:2255 "for readability" next to BeforeID: 0. The expectation struct here (and at line 549, and dbauthz_test.go:765) still sets only BeforeID: 0 without the matching AfterID: 0. Either apply the readability rationale consistently or skip it everywhere. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
…nation-for-messages
…date bounds Polling callers that set only after_id now receive messages in ASCENDING id order by dispatching to GetChatMessagesByChatIDAscPaginated. The previous DESC behavior silently dropped the oldest rows between polls when a burst exceeded `limit`: a caller that advanced after_id = max(returned) would permanently miss messages in the gap. The DESC path still serves the initial load (no cursor), before_id history walks, and the two-cursor open range. Also rejects the transposed-cursor case (after_id >= before_id) with a 400 rather than returning an empty page indistinguishable from "no messages in this range." Other review follow-ups: - AfterID godoc explains the ASC-on-polling behavior. - Docs note the order difference and the transposed-cursor error. - New subtests cover polling burst walks, steady-state empty pages, and the 400 guard. - Replace manual range-and-break with slices.ContainsFunc; use for i := range count; Message assertion symmetry across 400 tests. - Drop the stray AfterID: 0 from chatd.go regenerate call; go idiom omits zero-value fields.
|
Thanks for the review. Pushed
Not actioned: the two pre-existing notes about the stream handler's raw This response was prepared by Coder Agents on behalf of the PR author. |
There was a problem hiding this comment.
All 10 R1 findings addressed in f634fb1. Netero verified every fix mechanically. The panel (Mafuuu, Meruem, Bisky, Knov) found no new issues and confirmed the fixes address root causes, not symptoms.
The ASC dispatch for the polling path (DEREM-3) is particularly well-executed: the handler switch partitions cursor states cleanly, the ASC params struct lacks BeforeID entirely (making misuse impossible), and the burst-walk test proves gap-free coverage over 60 rows in pages of 25.
Severity count: 0 new findings. CI green (25 passed, 6 skipped).
Bisky's summary: "Ten subtests, all genuine. Every assertion proves behavior. None are costume jewelry."
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Nothing blocking, but one observation for follow-up.
| AND CASE | ||
| WHEN @after_id::bigint > 0 THEN id > @after_id::bigint | ||
| ELSE true | ||
| END |
There was a problem hiding this comment.
We now have a divergence between the GetChatMessagesByChatIDDescPaginated and GetChatMessagesByChatIDAscPaginated queries where one accepts both before_id and after_id and the other only accepts after_id.
Potential follow-up: could these two queries be combined into one e.g.
ORDER BY CASE WHEN @asc THEN id ASC
ELSE id DESC ENDAddress review feedback to make the cursor-validation and dispatch comments less verbose while keeping the core rationale.
david-fraley
deleted the
davidfraley/codagt-206-support-after_id-pagination-for-messages
branch
2 participants
Adds an optional
after_idquery parameter toGET /api/experimental/chats/{chat}/messagesalongside the existingbefore_id, so callers can poll for messages newer than a known IDwithout opening a websocket.
queued_messagesis now only returnedwhen neither cursor is set so polling callers do not receive the
snapshot on every request.
The SQL query gains an additive
CASE WHEN @after_id > 0 THEN id > @after_idbound, preserving current behavior at the sentinel
0. The SDKChatMessagesPaginationOptionsexposes the new field.Refs: CODAGT-206
Implementation plan
Problem
Messages endpoint supported only
before_idfor backward infinite-scroll.The stream endpoint already accepts
after_idfor reconnect skipping.API users polling for new messages had no equivalent for the messages
endpoint.
Requirements
after_idint64 query parameter.before_idsemantics whenafter_idis absent.after_idis set: returnid > after_id, DESC-ordered.after_id < id < before_id.queued_messageswhenever any cursor is set.after_idwith the same path asbefore_id(400 on negative/non-numeric).Implementation
coderd/database/queries/chats.sqlGetChatMessagesByChatIDDescPaginatedwith@after_id::bigintbound.coderd/exp_chats.goafter_id, forward to query, gate queued messages on both cursors being unset.codersdk/chats.goAfterID int64toChatMessagesPaginationOptions, encode when> 0.coderd/x/chatd/chatd.goAfterID: 0next to existingBeforeID: 0for readability.docs/ai-coder/agents/chats-api.mdGenerated via
make gen:queries.sql.go,typesGenerated.ts.Risk
Low. SQL change is additive (
CASE ... WHEN @after_id > 0), sentinel0preserves current behavior for existing callers. SDK field additionis backward-compatible (zero value = no filter). The queued-messages
guard becomes tighter (
beforeID == 0 && afterID == 0instead ofbeforeID == 0), but that is indistinguishable for any client thatdoes not send
after_id.Tests
TestGetChatMessages_Paginationcovers:before_idreturns older messages, suppresses queued.after_idreturns newer messages DESC, suppresses queued.after_id+before_idreturns the open range, suppresses queued.limit+after_idsetshas_more=truewhen more rows exist.after_idreturn 400.This PR was prepared by Coder Agents on behalf of the issue owner. Please review before merging.