From 315adb62e2c28030b4989b0bc72762a54dc344dc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 18:34:39 +0000 Subject: [PATCH 1/2] fix(services): listInbox counts TOTAL unread, not the fetched window (#6363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD --- .../notification-unread-count-true-total.md | 44 ++++ ...ion-schema-conformance.integration.test.ts | 36 ++-- .../src/messaging-service.test.ts | 201 ++++++++++++++++++ .../src/messaging-service.ts | 79 ++++++- 4 files changed, 341 insertions(+), 19 deletions(-) create mode 100644 .changeset/notification-unread-count-true-total.md diff --git a/.changeset/notification-unread-count-true-total.md b/.changeset/notification-unread-count-true-total.md new file mode 100644 index 0000000000..868a44f2bc --- /dev/null +++ b/.changeset/notification-unread-count-true-total.md @@ -0,0 +1,44 @@ +--- +"@objectstack/service-messaging": patch +--- + +fix(services): `unreadCount` counts the TOTAL unread, not the returned window (#6363) + +`ListNotificationsResponseSchema.unreadCount` is published into the API +reference as **"Total number of unread notifications"** — a `.describe()`, so it +is the documentation shipped to every consumer of +`GET /api/v1/notifications`. It was counted inside `rows.map(...)` in +`MessagingService.listInbox`, i.e. over the rows that `limit` had already +truncated, so the badge saturated at the window size forever. + +Measured on a real stack (sqlite-wasm + ObjectQL + service-messaging + hono + +dispatcher) with 60 unread messages: + +| request | `notifications[]` | `unreadCount` (before) | `unreadCount` (after) | +|:---|---:|---:|---:| +| no `limit` | 50 | **50** | **60** | +| `?limit=10` | 10 | **10** | **60** | + +The declaration was right and the implementation was wrong, so the +implementation moved (maintainer ruling, 2026-08-07). Every consumer that +renders `unreadCount` as a bell badge now gets the number it asked for; nothing +had to learn an implementation detail to read the field correctly. + +**The list itself is unchanged.** `notifications[]` is still the window — +`limit` rows, default 50, hard cap 200, newest first. The two bounds were +conflated, not shared. + +Read-state lives on `sys_notification_receipt`, not on the inbox row +(ADR-0030), so the total is a reverse join rather than a `count()`. It is +computed only when the window came back **saturated** (`rows.length === limit`) +— a short window is already the whole matching set, so the common inbox costs +exactly what it cost before. When the window does saturate, the extra work is +one projection read of a single column (`notification_id`) under the same +`where`, no `orderBy` and no `limit`: the same order as the receipt scan +`listInbox` already performs unconditionally, and exact under a `type` filter +and for rows carrying no `notification_id`. + +Two related behaviours are unchanged and now pinned: a `read` filter narrows +the list and never the badge (asking for the read half does not mean zero +unread), and a `type` filter narrows both (the count answers the query that was +asked). diff --git a/packages/runtime/src/notification-schema-conformance.integration.test.ts b/packages/runtime/src/notification-schema-conformance.integration.test.ts index e830f491cc..bcee4381b6 100644 --- a/packages/runtime/src/notification-schema-conformance.integration.test.ts +++ b/packages/runtime/src/notification-schema-conformance.integration.test.ts @@ -216,36 +216,44 @@ describe('[#5792] the notification wire bodies conform to the schemas the catalo }); // ═══════════════════════════════════════════════════════════════════════════ - // Declared, not delivered — recorded, NOT endorsed + // The gaps the double assertion cannot see — one now closed, one still open // ═══════════════════════════════════════════════════════════════════════════ // - // The two assertions above are 3/3 green for this family. These two facts are - // real inconsistencies that BOTH assertions are structurally blind to, and - // that is the point worth writing down for #3877's Stage D ratchet: + // The two assertions above are 3/3 green for this family. These two facts + // were real inconsistencies that BOTH assertions are structurally blind to, + // and that is the point worth writing down for #3877's Stage D ratchet: // // * `unreadCount` is a `number` whether it counts the total or the window, // so a VALUE assertion cannot see a wrong semantic; // * `cursor` is `optional`, so "no producer ever emits it" is a legal // parse and the KEY assertion (⊆, not =) cannot see it either. // - // Pinned as the measured behaviour of `origin/main`, with the issues that own - // the judgement call. Whichever way #6361 / #6363 are ruled, these two - // assertions are the ones that must flip — which is why they are here rather - // than left for the next reader to rediscover. + // Both were pinned here as the measured behaviour of `origin/main`, on the + // note that whichever way #6361 / #6363 were ruled, these assertions are the + // ones that must flip. #6363 has been ruled (2026-08-07, Option A: make the + // declaration true) and its assertion has flipped — it now pins the fix, over + // the wire, which is the only place the whole stack is in play. The `cursor` + // half is unchanged: it is one capability's two halves and is being retired + // with #6361, so it stays pinned as measured until that lands. + // + // The Stage D input stands either way, and is if anything sharper now: the + // ratchet still cannot see EITHER fact. Both had to be written by hand, and + // the fix below would have been just as invisible to it as the defect was. describe('[#6361 / #6363] the gaps the double assertion cannot see', () => { - it('[#6363] `unreadCount` counts the RETURNED WINDOW, not the total the schema describes', async () => { + it('[#6363] `unreadCount` is the TOTAL the schema describes, and survives a smaller window', async () => { const all = await getJson(GAP_USER, '/api/v1/notifications'); expect(all.unreadCount, 'fixture must leave more than one unread for this to mean anything') .toBeGreaterThan(1); const windowed = await getJson(GAP_USER, '/api/v1/notifications?limit=1'); + // The LIST is still windowed — that half never changed. expect(windowed.notifications).toHaveLength(1); - // Declared: 'Total number of unread notifications'. Delivered: the unread - // count within the fetched window. See #6363. - expect(windowed.unreadCount).toBe(1); - expect(windowed.unreadCount).not.toBe(all.unreadCount); - // …and it still parses, which is exactly the blind spot. + // The BADGE is not: declared 'Total number of unread notifications', and + // now delivered as one. Before #6363 this read `1` — the window's size, + // which is what a user with more unread than the page size was told + // forever. The parse was green either way; only this assertion can tell. + expect(windowed.unreadCount).toBe(all.unreadCount); expect(ListNotificationsResponseSchema.safeParse(windowed).success).toBe(true); }); diff --git a/packages/services/service-messaging/src/messaging-service.test.ts b/packages/services/service-messaging/src/messaging-service.test.ts index ee82e3a2e0..7233f978fa 100644 --- a/packages/services/service-messaging/src/messaging-service.test.ts +++ b/packages/services/service-messaging/src/messaging-service.test.ts @@ -592,3 +592,204 @@ describe('MessagingService — inbox read API (ADR-0030)', () => { expect(await svc.listInbox('')).toEqual({ notifications: [], unreadCount: 0 }); }); }); + +/** + * `n` inbox rows for one user, oldest first. `created_at` carries a padded + * millisecond index so the fake engine's lexicographic `desc` sort is the real + * newest-first order for any `n` — the window tests below all depend on + * knowing exactly WHICH rows a truncated window holds. + */ +function seedInbox( + userId: string, + n: number, + topicAt: (i: number) => string = () => 'task.assigned', +): Array> { + return Array.from({ length: n }, (_, i) => ({ + id: `m${i + 1}`, + user_id: userId, + notification_id: `n${i + 1}`, + topic: topicAt(i), + title: `Notification ${i + 1}`, + body_md: 'body', + created_at: `2026-01-01T00:00:00.${String(i).padStart(3, '0')}Z`, + })); +} + +/** An inbox receipt in a read state, for the message `seedInbox` numbered `i`. */ +function readReceipt(userId: string, i: number): Record { + return { id: `r${i}`, notification_id: `n${i}`, user_id: userId, channel: 'inbox', state: 'read' }; +} + +/** + * Record every `find` an existing engine is asked to run, in order. Wraps the + * double rather than declaring another one: the cost claims below are about how + * many reads `listInbox` issues, which is only observable at the call site. + */ +function recordFinds(engine: any): Array<{ object: string; query: any }> { + const calls: Array<{ object: string; query: any }> = []; + const real = engine.find.bind(engine); + engine.find = async (object: string, query: any = {}) => { + calls.push({ object, query }); + return real(object, query); + }; + return calls; +} + +/** + * [#6363] `ListNotificationsResponseSchema.unreadCount` is published into the + * API reference as "Total number of unread notifications". It was counted + * inside `rows.map(...)`, i.e. over the `limit`-truncated window, so the badge + * saturated at the window size forever: measured on a real stack with 60 + * unread, the route answered `unreadCount: 50` unfiltered and `10` at + * `?limit=10`. Maintainer ruling (2026-08-07, Option A): make the declaration + * true — count the total — and leave the list itself windowed. + * + * The fixtures below are the issue's measured shape (60 unread, `limit=10`). + */ +describe('[#6363] listInbox — unreadCount is the TOTAL unread, not the fetched window', () => { + const logger = silentLogger(); + + it('counts every unread message when the window truncates the inbox', async () => { + const engine = inboxEngine({ inbox: seedInbox('u1', 60) }); + const svc = new MessagingService({ logger, getData: () => engine }); + + // No `limit`: the default clamp windows the LIST at 50 — unchanged. + const unfiltered = await svc.listInbox('u1'); + expect(unfiltered.notifications).toHaveLength(50); + expect(unfiltered.unreadCount).toBe(60); // was 50 — the window's size + + // `?limit=10`: the list shrinks with the window, the badge does not. + const windowed = await svc.listInbox('u1', { limit: 10 }); + expect(windowed.notifications).toHaveLength(10); + expect(windowed.unreadCount).toBe(60); // was 10 — the window's size + }); + + it('subtracts read-state across the whole inbox, not just inside the window', async () => { + // The 20 read messages are the OLDEST, so none of them is inside a + // newest-first `limit=10` window: a window-scoped count cannot see + // them, and a total that ignored receipts would answer 60. + const engine = inboxEngine({ + inbox: seedInbox('u1', 60), + receipts: Array.from({ length: 20 }, (_, i) => readReceipt('u1', i + 1)), + }); + const svc = new MessagingService({ logger, getData: () => engine }); + + const res = await svc.listInbox('u1', { limit: 10 }); + expect(res.notifications).toHaveLength(10); + expect(res.notifications.every((n) => n.read === false)).toBe(true); // window is all-unread + expect(res.unreadCount).toBe(40); + }); + + it('counts rows carrying no notification_id — never receipted, so never read', async () => { + const inbox = seedInbox('u1', 60); + // A synthetic/legacy row with no event id keys no receipt at all. + for (const row of inbox.slice(0, 5)) row.notification_id = null; + const engine = inboxEngine({ inbox }); + const svc = new MessagingService({ logger, getData: () => engine }); + + expect((await svc.listInbox('u1', { limit: 10 })).unreadCount).toBe(60); + }); + + it('answers the `type` filter it was asked, over the whole inbox', async () => { + // 60 messages alternating between two topics ⇒ 30 of each. + const engine = inboxEngine({ + inbox: seedInbox('u1', 60, (i) => (i % 2 === 0 ? 'deal.won' : 'task.assigned')), + }); + const svc = new MessagingService({ logger, getData: () => engine }); + + const res = await svc.listInbox('u1', { type: 'deal.won', limit: 10 }); + expect(res.notifications).toHaveLength(10); + expect(res.notifications.every((n) => n.type === 'deal.won')).toBe(true); + expect(res.unreadCount).toBe(30); + }); + + it('counts only the addressed user, at any window size', async () => { + const engine = inboxEngine({ inbox: [...seedInbox('u1', 60), ...seedInbox('u2', 7)] }); + const svc = new MessagingService({ logger, getData: () => engine }); + + expect((await svc.listInbox('u1', { limit: 10 })).unreadCount).toBe(60); + expect((await svc.listInbox('u2', { limit: 10 })).unreadCount).toBe(7); + }); + + it('the `read` filter narrows the list and never the badge', async () => { + const engine = inboxEngine({ + inbox: seedInbox('u1', 60), + receipts: Array.from({ length: 20 }, (_, i) => readReceipt('u1', i + 1)), + }); + const svc = new MessagingService({ logger, getData: () => engine }); + + // Asking for the READ half does not mean the unread badge is zero. + const readOnly = await svc.listInbox('u1', { read: true, limit: 10 }); + expect(readOnly.notifications).toEqual([]); // the newest 10 are all unread + expect(readOnly.unreadCount).toBe(40); + + const unreadOnly = await svc.listInbox('u1', { read: false, limit: 10 }); + expect(unreadOnly.notifications).toHaveLength(10); + expect(unreadOnly.unreadCount).toBe(40); + }); + + it('a window that came back SHORT costs no second read', async () => { + // Nothing was truncated, so the window count already IS the total and + // the reverse join would re-read what the first `find` returned. + const engine = inboxEngine({ inbox: seedInbox('u1', 3) }); + const calls = recordFinds(engine); + const svc = new MessagingService({ logger, getData: () => engine }); + + const res = await svc.listInbox('u1'); + expect(res.unreadCount).toBe(3); + expect(calls.filter((c) => c.object === 'sys_inbox_message')).toHaveLength(1); + }); + + it('the total read is one narrow projection, unwindowed, over the same predicate', async () => { + const engine = inboxEngine({ inbox: seedInbox('u1', 60) }); + const calls = recordFinds(engine); + const svc = new MessagingService({ logger, getData: () => engine }); + + await svc.listInbox('u1', { type: 'task.assigned', limit: 10 }); + + const inboxReads = calls.filter((c) => c.object === 'sys_inbox_message'); + expect(inboxReads).toHaveLength(2); + const [windowRead, totalRead] = inboxReads; + expect(windowRead.query.limit).toBe(10); + // Same predicate as the windowed read — the count answers the same + // question, minus the window — and one column, so the extra work stays + // the same order as the receipt scan `listInbox` already performs. + expect(totalRead.query.where).toEqual(windowRead.query.where); + expect(totalRead.query.limit).toBeUndefined(); + expect(totalRead.query.fields).toEqual(['notification_id']); + }); + + it('a failing total read is NOT swallowed into a window-sized answer', async () => { + // The receipt read degrades (different object, may be absent from a + // minimal stack); this one re-reads the object whose `find` just + // succeeded, so a failure is a data-layer outage — not a licence to + // quietly re-tell the window-sized lie. + const engine = inboxEngine({ inbox: seedInbox('u1', 60) }); + const real = engine.find.bind(engine); + engine.find = async (object: string, query: any = {}) => { + if (object === 'sys_inbox_message' && query.fields) throw new Error('connection lost'); + return real(object, query); + }; + const svc = new MessagingService({ logger, getData: () => engine }); + + await expect(svc.listInbox('u1', { limit: 10 })).rejects.toThrow('connection lost'); + }); + + /* ------------------------------------------------------------------ */ + /* The other half of the ruling: the LIST window is unchanged. */ + /* ------------------------------------------------------------------ */ + + it('the list keeps its window: default 50, hard cap 200, floor 1, newest first', async () => { + const engine = inboxEngine({ inbox: seedInbox('u1', 250) }); + const svc = new MessagingService({ logger, getData: () => engine }); + + const dflt = await svc.listInbox('u1'); + expect(dflt.notifications).toHaveLength(50); + expect(dflt.notifications[0].id).toBe('n250'); // newest first, still + expect(dflt.unreadCount).toBe(250); + + expect((await svc.listInbox('u1', { limit: 500 })).notifications).toHaveLength(200); + expect((await svc.listInbox('u1', { limit: 0 })).notifications).toHaveLength(1); + expect((await svc.listInbox('u1', { limit: 120 })).notifications).toHaveLength(120); + }); +}); diff --git a/packages/services/service-messaging/src/messaging-service.ts b/packages/services/service-messaging/src/messaging-service.ts index e361a5da85..1525abcfa9 100644 --- a/packages/services/service-messaging/src/messaging-service.ts +++ b/packages/services/service-messaging/src/messaging-service.ts @@ -277,9 +277,26 @@ export class MessagingService { * * A message is unread until its event has a `read`/`clicked`/`dismissed` * receipt; the `read` filter (when given) is applied in-memory after the - * join. `unreadCount` is computed over the fetched window (bounded by - * `limit`, like the Console bell's poll). Returns the REST contract shape - * (`ListNotificationsResponseSchema`): `{ notifications, unreadCount }`. + * join. + * + * Two different bounds, deliberately (#6363): + * + * * `notifications[]` is the fetched WINDOW — `limit` rows, defaulting to + * 50 and hard-capped at 200, newest first. Unchanged: the Console + * bell's poll and every other caller page through this list. + * * `unreadCount` is the **total** unread across the user's whole + * matching inbox, which is what `ListNotificationsResponseSchema` + * publishes into the API reference ("Total number of unread + * notifications"). Counting it over `rows` — the window — made the + * badge saturate at the window size forever: a user with 60 unread was + * told 50, and `?limit=10` told them 10. The declaration was right and + * the implementation was wrong (maintainer ruling, #6363 Option A). + * + * The `read` filter never moves `unreadCount`: asking for the read half of + * the inbox does not mean the badge is zero. A `type` filter does — the + * count answers the query that was asked, as it always has. + * + * Returns the REST contract shape: `{ notifications, unreadCount }`. */ async listInbox( userId: string, @@ -313,12 +330,12 @@ export class MessagingService { } } - let unreadCount = 0; + let windowUnread = 0; const all: InboxNotificationView[] = rows.map((m) => { const nid = m?.notification_id != null ? String(m.notification_id) : null; const state = nid ? stateByNotif.get(nid) : undefined; const read = state ? READ_RECEIPT_STATES.has(state) : false; - if (!read) unreadCount += 1; + if (!read) windowUnread += 1; return { id: nid ?? String(m.id), type: (m.topic as string) ?? 'notification', @@ -330,10 +347,62 @@ export class MessagingService { }; }); + // A window that came back SHORT is the whole matching set — nothing was + // truncated, so the window count already IS the total and the second + // read would be a duplicate of the first. Only a saturated window + // (`rows.length === limit`) can be hiding rows, and that is exactly the + // case #6363 is about. So the common inbox — fewer messages than the + // page size — costs precisely what it cost before this change. + const unreadCount = rows.length < limit + ? windowUnread + : await this.countUnreadTotal(data, where, stateByNotif); + const notifications = opts.read === undefined ? all : all.filter((n) => n.read === opts.read); return { notifications, unreadCount }; } + /** + * Total unread across the user's whole matching inbox — the reverse join + * `unreadCount` is declared to answer (#6363). + * + * Read-state lives on `sys_notification_receipt`, not on the inbox row + * (ADR-0030), so no single `count()` answers this: the predicate spans two + * objects. The receipt side is already fully in memory — `listInbox` reads + * every one of the user's inbox receipts, unbounded, to build the join — + * so all that is missing is the message side, and it is read as a + * PROJECTION of one column with no `orderBy` and no `limit`. That keeps + * this the same order of work as the receipt scan the method already + * performs unconditionally, one column wide, and it stays exact under a + * `type` filter and for rows carrying no `notification_id` (never + * receipted, therefore always unread) — neither of which a + * `count(messages) - count(read receipts)` subtraction survives. + * + * NOT best-effort, unlike the receipt read above. That one degrades because + * receipts are a DIFFERENT object which a minimal stack may not have + * registered at all; this one re-reads the very object whose `find` just + * succeeded with the very same `where`. There is no state in which it fails + * and the caller still holds a trustworthy list — and swallowing it would + * silently restore the window-sized lie this method exists to stop telling. + */ + private async countUnreadTotal( + data: IDataEngine, + where: Record, + stateByNotif: ReadonlyMap, + ): Promise { + const ids = (await data.find(INBOX_OBJECT, { + where, + fields: ['notification_id'], + })) as Array>; + + let unread = 0; + for (const row of ids) { + const nid = row?.notification_id != null ? String(row.notification_id) : null; + const state = nid ? stateByNotif.get(nid) : undefined; + if (!state || !READ_RECEIPT_STATES.has(state)) unread += 1; + } + return unread; + } + /** * Mark specific notifications read by upserting their inbox receipts to * `read`. Updates the existing `delivered` receipt in place (keyed From 2acc6606c9c6b6e28c205ff5b6cbab3608f8454c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 18:39:41 +0000 Subject: [PATCH 2/2] 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 Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD --- .../src/notification-schema-conformance.integration.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/runtime/src/notification-schema-conformance.integration.test.ts b/packages/runtime/src/notification-schema-conformance.integration.test.ts index bcee4381b6..49a47c7009 100644 --- a/packages/runtime/src/notification-schema-conformance.integration.test.ts +++ b/packages/runtime/src/notification-schema-conformance.integration.test.ts @@ -254,6 +254,10 @@ describe('[#5792] the notification wire bodies conform to the schemas the catalo // which is what a user with more unread than the page size was told // forever. The parse was green either way; only this assertion can tell. expect(windowed.unreadCount).toBe(all.unreadCount); + // Stated as the property rather than only as an equality, so the pin + // cannot go quietly vacuous if a future fixture change flattens both + // sides: the count MUST be able to exceed the window it came back in. + expect(windowed.unreadCount).toBeGreaterThan(windowed.notifications.length); expect(ListNotificationsResponseSchema.safeParse(windowed).success).toBe(true); });