Add since_id filter to message.list (A1 — WS refactor follow-up) #15

New issue

Closed

opened 2026-04-23 19:40:54 +00:00 by sameh-farouk · 1 comment

sameh-farouk commented

2026-04-23 19:40:54 +00:00

Member

Follow-up to #13. Closes gap A1 from the post-refactor architectural assessment.

Problem: the refactor added a reconnect catch-up path that calls rpc('message.list', { channel_id, since_id, limit: 100 }). But the server's message.list doesn't honor since_id — it just returns the latest 100 messages every reconnect. The client dedups by id so correctness holds, but on a quiet channel where 2 messages arrived during a 5s disconnect, the server still sends all 100 most recent. ~50× overfetch per reconnect.

Solution: add optional since_id: Option<u64> to MessageList input; conditionally add AND id > ? to the query. Client already sends the field (the refactor left a TODO comment referencing this follow-up).

Docs:

Spec: plan/feature-message-since-id.md
Impl plan: plan/impl-message-since-id.md

Size: ~70 lines total (25 production + 50 tests + JSON schema + SDK regen). 2 tasks.

Branch: lands on feat/ws-refactor — removes a visible TODO we just added and pairs naturally with the other follow-ups.

Follow-up to #13. Closes gap A1 from the post-refactor architectural assessment. **Problem:** the refactor added a reconnect catch-up path that calls `rpc('message.list', { channel_id, since_id, limit: 100 })`. But the server's `message.list` doesn't honor `since_id` — it just returns the latest 100 messages every reconnect. The client dedups by id so correctness holds, but on a quiet channel where 2 messages arrived during a 5s disconnect, the server still sends all 100 most recent. ~50× overfetch per reconnect. **Solution:** add optional `since_id: Option<u64>` to `MessageList` input; conditionally add `AND id > ?` to the query. Client already sends the field (the refactor left a TODO comment referencing this follow-up). **Docs:** - Spec: [`plan/feature-message-since-id.md`](../src/branch/feat/ws-refactor/plan/feature-message-since-id.md) - Impl plan: [`plan/impl-message-since-id.md`](../src/branch/feat/ws-refactor/plan/impl-message-since-id.md) **Size:** ~70 lines total (25 production + 50 tests + JSON schema + SDK regen). 2 tasks. **Branch:** lands on `feat/ws-refactor` — removes a visible TODO we just added and pairs naturally with the other follow-ups.

sameh-farouk added the

type_story

label

2026-04-23 19:40:54 +00:00

sameh-farouk referenced this issue

2026-04-23 19:42:05 +00:00

Refactor realtime transport: one WS per user + server-side fanout (Slack/Mattermost model) #13

sameh-farouk referenced this issue from a commit

2026-04-23 20:51:31 +00:00

feat(server): message.list since_id filter for reconnect delta fetch

sameh-farouk referenced this issue

2026-04-23 20:52:27 +00:00

Refactor realtime transport: one WS per user + server-side fanout (Slack/Mattermost model) #13

sameh-farouk commented

2026-04-23 20:58:46 +00:00

Author

Member

Implementation landed

Three commits on feat/ws-refactor:

bd19bf7 feat(server): message.list since_id filter for reconnect delta fetch
Adds since_id: Option<u64> to the message.list RPC params. Refactors the handler's SQL from a 2-branch with_before/without_before pattern into a dynamic builder that composes channel_id / thread_id / before / since_id / limit conjunctively. Registers the param in openrpc.json; proc-macro auto-regens MessageListInput.since_id: Option<i64> in the SDK.
4187060 refactor(ui): drop since_id TODO — server honours it now
Removes the stale 5-line TODO comment referencing the pending server-side support.
32f299c fix(ui): wire since_id into reconnect catch-up message.list call
Fix commit — the original plan assumed P8.3 had wired since_id into the client's RPC call. P8.3's executor had actually chosen the Option-B fallback ("refetch 100 + dedup") and never wired it. A1.2 as briefed would have left A1 functionally inert on the client side. This commit adds since_id: state.messages[state.messages.length - 1].id to the rpc('message.list', ...) call inside onWsReconnected.

Architectural notes

Net improvement beyond the feature: the dynamic SQL builder replaces a 2-branch control flow that would have gotten uglier as filter count grew. Now adding future filters (e.g. from_user_id) is a 4-line conditional append instead of an N×M branch explosion.
Dedup-by-id retained client-side. Even with since_id > 0, the client still dedups received messages by id. Defence in depth: protects against any theoretical server-side bug where overlapping messages slip through.
Exclusive cursor semantics: id > since_id, not >=. The client's lastId is already in its view, so the inclusive form would re-send.

Retrospective

Three plan errors were caught by implementers reading source instead of trusting the plan (-32029 vs -32005, raw_rpc return type, and A1's client-wiring assumption). This was a direct consequence of me re-using my memory of the P8.3 commit's intent instead of its content. Process note in #13's comment — going forward, first step of any plan task that assumes pre-existing behavior should be a grep or git show against the landed commit.

Tests impact

cargo test -p hero_collab_server: integration 64 (+1 new message_list_filters_by_since_id test covering no-filter / since_id=msg3 / since_id past end).

Known follow-up debt

None for A1 itself. A future ergonomics pass could add a typed MessageList struct in handlers/inputs.rs (currently the handler uses raw Value extraction). Non-blocking.

## Implementation landed Three commits on `feat/ws-refactor`: - [`bd19bf7`](../commit/bd19bf7) `feat(server): message.list since_id filter for reconnect delta fetch` Adds `since_id: Option<u64>` to the `message.list` RPC params. Refactors the handler's SQL from a 2-branch `with_before`/`without_before` pattern into a dynamic builder that composes `channel_id` / `thread_id` / `before` / `since_id` / `limit` conjunctively. Registers the param in `openrpc.json`; proc-macro auto-regens `MessageListInput.since_id: Option<i64>` in the SDK. - [`4187060`](../commit/4187060) `refactor(ui): drop since_id TODO — server honours it now` Removes the stale 5-line TODO comment referencing the pending server-side support. - [`32f299c`](../commit/32f299c) `fix(ui): wire since_id into reconnect catch-up message.list call` **Fix commit** — the original plan assumed P8.3 had wired `since_id` into the client's RPC call. P8.3's executor had actually chosen the Option-B fallback ("refetch 100 + dedup") and never wired it. A1.2 as briefed would have left A1 functionally inert on the client side. This commit adds `since_id: state.messages[state.messages.length - 1].id` to the `rpc('message.list', ...)` call inside `onWsReconnected`. ## Architectural notes - **Net improvement beyond the feature:** the dynamic SQL builder replaces a 2-branch control flow that would have gotten uglier as filter count grew. Now adding future filters (e.g. `from_user_id`) is a 4-line conditional append instead of an N×M branch explosion. - **Dedup-by-id retained client-side.** Even with `since_id > 0`, the client still dedups received messages by id. Defence in depth: protects against any theoretical server-side bug where overlapping messages slip through. - **Exclusive cursor semantics:** `id > since_id`, not `>=`. The client's `lastId` is already in its view, so the inclusive form would re-send. ## Retrospective Three plan errors were caught by implementers reading source instead of trusting the plan (`-32029` vs `-32005`, `raw_rpc` return type, and A1's client-wiring assumption). This was a direct consequence of me re-using my memory of the P8.3 commit's *intent* instead of its *content*. Process note in [#13's comment](../issues/13#issuecomment-23258) — going forward, first step of any plan task that assumes pre-existing behavior should be a `grep` or `git show` against the landed commit. ## Tests impact `cargo test -p hero_collab_server`: integration 64 (+1 new `message_list_filters_by_since_id` test covering no-filter / since_id=msg3 / since_id past end). ## Known follow-up debt None for A1 itself. A future ergonomics pass could add a typed `MessageList` struct in `handlers/inputs.rs` (currently the handler uses raw `Value` extraction). Non-blocking.

sameh-farouk referenced this issue

2026-04-23 21:01:10 +00:00

Refactor realtime transport: single WS per user + server-authoritative fanout + reliability polish #19

sameh-farouk closed this issue

2026-04-24 00:55:01 +00:00