Skip to content

Commit 17d0954

Browse files
hotlongclaude
andauthored
fix(services): listInbox 的 unreadCount 数总未读,不再只数取回窗口 (#6363) (#6439)
* fix(services): listInbox counts TOTAL unread, not the fetched window (#6363) `ListNotificationsResponseSchema.unreadCount` is published into the API reference as "Total number of unread notifications", but the count happened inside `rows.map(...)` — over rows `limit` had already truncated — so the badge saturated at the window size forever. Measured on a real stack with 60 unread: no `limit` answered 50, `?limit=10` answered 10. Maintainer ruling (2026-08-07, Option A): make the declaration true. Read-state lives on `sys_notification_receipt` (ADR-0030), so the total is a reverse join; it runs only when the window came back saturated (a short window already IS the whole matching set), and then reads one projected column under the same `where` with no `orderBy` and no `limit` — the same order as the receipt scan `listInbox` already performs unconditionally. `notifications[]` keeps its window unchanged (default 50, cap 200, newest first), and both bounds are now pinned separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD * test(runtime): state the #6363 wire pin as a property, not only an equality `unreadCount === all.unreadCount` alone would go vacuous if a future fixture change flattened both sides. The property the route now guarantees is that the badge can exceed the window it arrived in, so assert that directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 35353bd commit 17d0954

4 files changed

Lines changed: 345 additions & 19 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@objectstack/service-messaging": patch
3+
---
4+
5+
fix(services): `unreadCount` counts the TOTAL unread, not the returned window (#6363)
6+
7+
`ListNotificationsResponseSchema.unreadCount` is published into the API
8+
reference as **"Total number of unread notifications"** — a `.describe()`, so it
9+
is the documentation shipped to every consumer of
10+
`GET /api/v1/notifications`. It was counted inside `rows.map(...)` in
11+
`MessagingService.listInbox`, i.e. over the rows that `limit` had already
12+
truncated, so the badge saturated at the window size forever.
13+
14+
Measured on a real stack (sqlite-wasm + ObjectQL + service-messaging + hono +
15+
dispatcher) with 60 unread messages:
16+
17+
| request | `notifications[]` | `unreadCount` (before) | `unreadCount` (after) |
18+
|:---|---:|---:|---:|
19+
| no `limit` | 50 | **50** | **60** |
20+
| `?limit=10` | 10 | **10** | **60** |
21+
22+
The declaration was right and the implementation was wrong, so the
23+
implementation moved (maintainer ruling, 2026-08-07). Every consumer that
24+
renders `unreadCount` as a bell badge now gets the number it asked for; nothing
25+
had to learn an implementation detail to read the field correctly.
26+
27+
**The list itself is unchanged.** `notifications[]` is still the window —
28+
`limit` rows, default 50, hard cap 200, newest first. The two bounds were
29+
conflated, not shared.
30+
31+
Read-state lives on `sys_notification_receipt`, not on the inbox row
32+
(ADR-0030), so the total is a reverse join rather than a `count()`. It is
33+
computed only when the window came back **saturated** (`rows.length === limit`)
34+
— a short window is already the whole matching set, so the common inbox costs
35+
exactly what it cost before. When the window does saturate, the extra work is
36+
one projection read of a single column (`notification_id`) under the same
37+
`where`, no `orderBy` and no `limit`: the same order as the receipt scan
38+
`listInbox` already performs unconditionally, and exact under a `type` filter
39+
and for rows carrying no `notification_id`.
40+
41+
Two related behaviours are unchanged and now pinned: a `read` filter narrows
42+
the list and never the badge (asking for the read half does not mean zero
43+
unread), and a `type` filter narrows both (the count answers the query that was
44+
asked).

‎packages/runtime/src/notification-schema-conformance.integration.test.ts‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -216,36 +216,48 @@ describe('[#5792] the notification wire bodies conform to the schemas the catalo
216216
});
217217

218218
// ═══════════════════════════════════════════════════════════════════════════
219-
// Declared, not delivered — recorded, NOT endorsed
219+
// The gaps the double assertion cannot see — one now closed, one still open
220220
// ═══════════════════════════════════════════════════════════════════════════
221221
//
222-
// The two assertions above are 3/3 green for this family. These two facts are
223-
// real inconsistencies that BOTH assertions are structurally blind to, and
224-
// that is the point worth writing down for #3877's Stage D ratchet:
222+
// The two assertions above are 3/3 green for this family. These two facts
223+
// were real inconsistencies that BOTH assertions are structurally blind to,
224+
// and that is the point worth writing down for #3877's Stage D ratchet:
225225
//
226226
// * `unreadCount` is a `number` whether it counts the total or the window,
227227
// so a VALUE assertion cannot see a wrong semantic;
228228
// * `cursor` is `optional`, so "no producer ever emits it" is a legal
229229
// parse and the KEY assertion (⊆, not =) cannot see it either.
230230
//
231-
// Pinned as the measured behaviour of `origin/main`, with the issues that own
232-
// the judgement call. Whichever way #6361 / #6363 are ruled, these two
233-
// assertions are the ones that must flip — which is why they are here rather
234-
// than left for the next reader to rediscover.
231+
// Both were pinned here as the measured behaviour of `origin/main`, on the
232+
// note that whichever way #6361 / #6363 were ruled, these assertions are the
233+
// ones that must flip. #6363 has been ruled (2026-08-07, Option A: make the
234+
// declaration true) and its assertion has flipped — it now pins the fix, over
235+
// the wire, which is the only place the whole stack is in play. The `cursor`
236+
// half is unchanged: it is one capability's two halves and is being retired
237+
// with #6361, so it stays pinned as measured until that lands.
238+
//
239+
// The Stage D input stands either way, and is if anything sharper now: the
240+
// ratchet still cannot see EITHER fact. Both had to be written by hand, and
241+
// the fix below would have been just as invisible to it as the defect was.
235242
describe('[#6361 / #6363] the gaps the double assertion cannot see', () => {
236-
it('[#6363] `unreadCount` counts the RETURNED WINDOW, not the total the schema describes', async () => {
243+
it('[#6363] `unreadCount` is the TOTAL the schema describes, and survives a smaller window', async () => {
237244
const all = await getJson(GAP_USER, '/api/v1/notifications');
238245
expect(all.unreadCount, 'fixture must leave more than one unread for this to mean anything')
239246
.toBeGreaterThan(1);
240247

241248
const windowed = await getJson(GAP_USER, '/api/v1/notifications?limit=1');
242249

250+
// The LIST is still windowed — that half never changed.
243251
expect(windowed.notifications).toHaveLength(1);
244-
// Declared: 'Total number of unread notifications'. Delivered: the unread
245-
// count within the fetched window. See #6363.
246-
expect(windowed.unreadCount).toBe(1);
247-
expect(windowed.unreadCount).not.toBe(all.unreadCount);
248-
// …and it still parses, which is exactly the blind spot.
252+
// The BADGE is not: declared 'Total number of unread notifications', and
253+
// now delivered as one. Before #6363 this read `1` — the window's size,
254+
// which is what a user with more unread than the page size was told
255+
// forever. The parse was green either way; only this assertion can tell.
256+
expect(windowed.unreadCount).toBe(all.unreadCount);
257+
// Stated as the property rather than only as an equality, so the pin
258+
// cannot go quietly vacuous if a future fixture change flattens both
259+
// sides: the count MUST be able to exceed the window it came back in.
260+
expect(windowed.unreadCount).toBeGreaterThan(windowed.notifications.length);
249261
expect(ListNotificationsResponseSchema.safeParse(windowed).success).toBe(true);
250262
});
251263

‎packages/services/service-messaging/src/messaging-service.test.ts‎

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -592,3 +592,204 @@ describe('MessagingService — inbox read API (ADR-0030)', () => {
592592
expect(await svc.listInbox('')).toEqual({ notifications: [], unreadCount: 0 });
593593
});
594594
});
595+
596+
/**
597+
* `n` inbox rows for one user, oldest first. `created_at` carries a padded
598+
* millisecond index so the fake engine's lexicographic `desc` sort is the real
599+
* newest-first order for any `n` — the window tests below all depend on
600+
* knowing exactly WHICH rows a truncated window holds.
601+
*/
602+
function seedInbox(
603+
userId: string,
604+
n: number,
605+
topicAt: (i: number) => string = () => 'task.assigned',
606+
): Array<Record<string, unknown>> {
607+
return Array.from({ length: n }, (_, i) => ({
608+
id: `m${i + 1}`,
609+
user_id: userId,
610+
notification_id: `n${i + 1}`,
611+
topic: topicAt(i),
612+
title: `Notification ${i + 1}`,
613+
body_md: 'body',
614+
created_at: `2026-01-01T00:00:00.${String(i).padStart(3, '0')}Z`,
615+
}));
616+
}
617+
618+
/** An inbox receipt in a read state, for the message `seedInbox` numbered `i`. */
619+
function readReceipt(userId: string, i: number): Record<string, unknown> {
620+
return { id: `r${i}`, notification_id: `n${i}`, user_id: userId, channel: 'inbox', state: 'read' };
621+
}
622+
623+
/**
624+
* Record every `find` an existing engine is asked to run, in order. Wraps the
625+
* double rather than declaring another one: the cost claims below are about how
626+
* many reads `listInbox` issues, which is only observable at the call site.
627+
*/
628+
function recordFinds(engine: any): Array<{ object: string; query: any }> {
629+
const calls: Array<{ object: string; query: any }> = [];
630+
const real = engine.find.bind(engine);
631+
engine.find = async (object: string, query: any = {}) => {
632+
calls.push({ object, query });
633+
return real(object, query);
634+
};
635+
return calls;
636+
}
637+
638+
/**
639+
* [#6363] `ListNotificationsResponseSchema.unreadCount` is published into the
640+
* API reference as "Total number of unread notifications". It was counted
641+
* inside `rows.map(...)`, i.e. over the `limit`-truncated window, so the badge
642+
* saturated at the window size forever: measured on a real stack with 60
643+
* unread, the route answered `unreadCount: 50` unfiltered and `10` at
644+
* `?limit=10`. Maintainer ruling (2026-08-07, Option A): make the declaration
645+
* true — count the total — and leave the list itself windowed.
646+
*
647+
* The fixtures below are the issue's measured shape (60 unread, `limit=10`).
648+
*/
649+
describe('[#6363] listInbox — unreadCount is the TOTAL unread, not the fetched window', () => {
650+
const logger = silentLogger();
651+
652+
it('counts every unread message when the window truncates the inbox', async () => {
653+
const engine = inboxEngine({ inbox: seedInbox('u1', 60) });
654+
const svc = new MessagingService({ logger, getData: () => engine });
655+
656+
// No `limit`: the default clamp windows the LIST at 50 — unchanged.
657+
const unfiltered = await svc.listInbox('u1');
658+
expect(unfiltered.notifications).toHaveLength(50);
659+
expect(unfiltered.unreadCount).toBe(60); // was 50 — the window's size
660+
661+
// `?limit=10`: the list shrinks with the window, the badge does not.
662+
const windowed = await svc.listInbox('u1', { limit: 10 });
663+
expect(windowed.notifications).toHaveLength(10);
664+
expect(windowed.unreadCount).toBe(60); // was 10 — the window's size
665+
});
666+
667+
it('subtracts read-state across the whole inbox, not just inside the window', async () => {
668+
// The 20 read messages are the OLDEST, so none of them is inside a
669+
// newest-first `limit=10` window: a window-scoped count cannot see
670+
// them, and a total that ignored receipts would answer 60.
671+
const engine = inboxEngine({
672+
inbox: seedInbox('u1', 60),
673+
receipts: Array.from({ length: 20 }, (_, i) => readReceipt('u1', i + 1)),
674+
});
675+
const svc = new MessagingService({ logger, getData: () => engine });
676+
677+
const res = await svc.listInbox('u1', { limit: 10 });
678+
expect(res.notifications).toHaveLength(10);
679+
expect(res.notifications.every((n) => n.read === false)).toBe(true); // window is all-unread
680+
expect(res.unreadCount).toBe(40);
681+
});
682+
683+
it('counts rows carrying no notification_id — never receipted, so never read', async () => {
684+
const inbox = seedInbox('u1', 60);
685+
// A synthetic/legacy row with no event id keys no receipt at all.
686+
for (const row of inbox.slice(0, 5)) row.notification_id = null;
687+
const engine = inboxEngine({ inbox });
688+
const svc = new MessagingService({ logger, getData: () => engine });
689+
690+
expect((await svc.listInbox('u1', { limit: 10 })).unreadCount).toBe(60);
691+
});
692+
693+
it('answers the `type` filter it was asked, over the whole inbox', async () => {
694+
// 60 messages alternating between two topics ⇒ 30 of each.
695+
const engine = inboxEngine({
696+
inbox: seedInbox('u1', 60, (i) => (i % 2 === 0 ? 'deal.won' : 'task.assigned')),
697+
});
698+
const svc = new MessagingService({ logger, getData: () => engine });
699+
700+
const res = await svc.listInbox('u1', { type: 'deal.won', limit: 10 });
701+
expect(res.notifications).toHaveLength(10);
702+
expect(res.notifications.every((n) => n.type === 'deal.won')).toBe(true);
703+
expect(res.unreadCount).toBe(30);
704+
});
705+
706+
it('counts only the addressed user, at any window size', async () => {
707+
const engine = inboxEngine({ inbox: [...seedInbox('u1', 60), ...seedInbox('u2', 7)] });
708+
const svc = new MessagingService({ logger, getData: () => engine });
709+
710+
expect((await svc.listInbox('u1', { limit: 10 })).unreadCount).toBe(60);
711+
expect((await svc.listInbox('u2', { limit: 10 })).unreadCount).toBe(7);
712+
});
713+
714+
it('the `read` filter narrows the list and never the badge', async () => {
715+
const engine = inboxEngine({
716+
inbox: seedInbox('u1', 60),
717+
receipts: Array.from({ length: 20 }, (_, i) => readReceipt('u1', i + 1)),
718+
});
719+
const svc = new MessagingService({ logger, getData: () => engine });
720+
721+
// Asking for the READ half does not mean the unread badge is zero.
722+
const readOnly = await svc.listInbox('u1', { read: true, limit: 10 });
723+
expect(readOnly.notifications).toEqual([]); // the newest 10 are all unread
724+
expect(readOnly.unreadCount).toBe(40);
725+
726+
const unreadOnly = await svc.listInbox('u1', { read: false, limit: 10 });
727+
expect(unreadOnly.notifications).toHaveLength(10);
728+
expect(unreadOnly.unreadCount).toBe(40);
729+
});
730+
731+
it('a window that came back SHORT costs no second read', async () => {
732+
// Nothing was truncated, so the window count already IS the total and
733+
// the reverse join would re-read what the first `find` returned.
734+
const engine = inboxEngine({ inbox: seedInbox('u1', 3) });
735+
const calls = recordFinds(engine);
736+
const svc = new MessagingService({ logger, getData: () => engine });
737+
738+
const res = await svc.listInbox('u1');
739+
expect(res.unreadCount).toBe(3);
740+
expect(calls.filter((c) => c.object === 'sys_inbox_message')).toHaveLength(1);
741+
});
742+
743+
it('the total read is one narrow projection, unwindowed, over the same predicate', async () => {
744+
const engine = inboxEngine({ inbox: seedInbox('u1', 60) });
745+
const calls = recordFinds(engine);
746+
const svc = new MessagingService({ logger, getData: () => engine });
747+
748+
await svc.listInbox('u1', { type: 'task.assigned', limit: 10 });
749+
750+
const inboxReads = calls.filter((c) => c.object === 'sys_inbox_message');
751+
expect(inboxReads).toHaveLength(2);
752+
const [windowRead, totalRead] = inboxReads;
753+
expect(windowRead.query.limit).toBe(10);
754+
// Same predicate as the windowed read — the count answers the same
755+
// question, minus the window — and one column, so the extra work stays
756+
// the same order as the receipt scan `listInbox` already performs.
757+
expect(totalRead.query.where).toEqual(windowRead.query.where);
758+
expect(totalRead.query.limit).toBeUndefined();
759+
expect(totalRead.query.fields).toEqual(['notification_id']);
760+
});
761+
762+
it('a failing total read is NOT swallowed into a window-sized answer', async () => {
763+
// The receipt read degrades (different object, may be absent from a
764+
// minimal stack); this one re-reads the object whose `find` just
765+
// succeeded, so a failure is a data-layer outage — not a licence to
766+
// quietly re-tell the window-sized lie.
767+
const engine = inboxEngine({ inbox: seedInbox('u1', 60) });
768+
const real = engine.find.bind(engine);
769+
engine.find = async (object: string, query: any = {}) => {
770+
if (object === 'sys_inbox_message' && query.fields) throw new Error('connection lost');
771+
return real(object, query);
772+
};
773+
const svc = new MessagingService({ logger, getData: () => engine });
774+
775+
await expect(svc.listInbox('u1', { limit: 10 })).rejects.toThrow('connection lost');
776+
});
777+
778+
/* ------------------------------------------------------------------ */
779+
/* The other half of the ruling: the LIST window is unchanged. */
780+
/* ------------------------------------------------------------------ */
781+
782+
it('the list keeps its window: default 50, hard cap 200, floor 1, newest first', async () => {
783+
const engine = inboxEngine({ inbox: seedInbox('u1', 250) });
784+
const svc = new MessagingService({ logger, getData: () => engine });
785+
786+
const dflt = await svc.listInbox('u1');
787+
expect(dflt.notifications).toHaveLength(50);
788+
expect(dflt.notifications[0].id).toBe('n250'); // newest first, still
789+
expect(dflt.unreadCount).toBe(250);
790+
791+
expect((await svc.listInbox('u1', { limit: 500 })).notifications).toHaveLength(200);
792+
expect((await svc.listInbox('u1', { limit: 0 })).notifications).toHaveLength(1);
793+
expect((await svc.listInbox('u1', { limit: 120 })).notifications).toHaveLength(120);
794+
});
795+
});

0 commit comments

Comments
 (0)