perf(app-shell): scope the inbox receipt read to the listed messages (objectui#7392) - #10047
Merged
Merged
Conversation
…(objectui#7392)
The inbox poll re-read `sys_notification_receipt` in full on every tick —
filtered by `user_id` + `channel:'inbox'`, `$top: 200`, no cursor — beside
the `sys_inbox_message` read's `$top: 20`, at 10s foregrounded and 60s
hidden. `mergeInboxRows` uses the receipts as a lookup keyed by
`notification_id` while mapping over the MESSAGE rows, so every receipt
outside that window of 20 was fetched, indexed and dropped, six times a
minute.
The read now names those messages' notification ids as an `$in` comparand
and is skipped outright when the window lists nothing a receipt could
belong to. `$top` becomes the id count, which is an exact bound rather
than a guessed headroom: the object declares its key
`{ fields: ['notification_id', 'user_id', 'channel'], unique: true }`, so
at most one row per named id can match.
No number the user sees moves. The bell badge is `unreadTopics +
pendingApprovalsCount` and `unreadTopics` folds this feed's rows, which
are one per listed message — bounded by the message read's `$top: 20`
since #4225 gave the bell and Home one feed, never by the receipt set.
Home's `unreadTopicCount` folds the same rows the same way (#4329). The
receipts only ever supplied read-state to rows the message query had
already chosen.
The two reads are sequential where they were a `Promise.all`: the receipt
query cannot be written until the message read says which notifications
are in the window. One extra round trip per tick on a background poll,
against ~180 discarded rows saved on each of them.
The #7249 cadence pin goes red on this change and that red is correct: it
asserts the receipt read is exactly as frequent as the message read, on a
fixture whose inbox was empty — a window where no receipt read is now due.
Its fixture now lists rows so the equality is exercised where it means
something, the equality itself is unchanged, and the empty window is
pinned separately.
Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018HrVaotisyhgmot9o2MLRq
Contributor
|
changeset-claim-re-read
|
…ediction The pin shipped with the direction it predicted. Running it turned in a different count — 6 failed, 5 passed across this file and the #7249 pin, not the three-red/two-green that was written down — and the gap is the informative part: the `badge` case's `unreadTopics` equality passed under the old, wider read too, so what went red there is the row arithmetic beside it, not the number a narrowing could have moved. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HrVaotisyhgmot9o2MLRq
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Brings objectui#7394 (PR #10045, 237e5b8) in. It moves packages/plugin-list, which this branch does not touch — no conflict, and the package checks are re-run after this merge rather than before it. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HrVaotisyhgmot9o2MLRq
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Collaborator
Author
Post-merge verification (
|
| reading, all AFTER the merge | result |
|---|---|
pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' build then pnpm --filter @object-ui/app-shell type-check (tsc --noEmit && tsc -p tsconfig.test.json) |
VERDICT command-exit 0, 185s held |
pnpm exec vitest run packages/app-shell/ |
Test Files 736 passed (736) · Tests 7302 passed, 1 skipped (7303), VERDICT command-exit 0, 781s held |
check:control-bytes, check:new-line-citations, check:changeset-claims, check:pending-changeset-literals, check-changeset-no-major |
all exit 0 |
check-changeset-presence, re-derived against the new merge-base 237e5b8d2 |
exit 0 — "3 source file(s) of 1 released package(s) changed … declares 1 changeset(s)" |
| conflicts | none; the merge was clean and the PR's diff against main is still the same 4 files |
Both long runs exceeded the 600s foreground cap and were collected in-round by blocking on their pid, never by polling.
mergeable_state moved behind to blocked, mergeable: true — blocked is the draft plus checks still in flight on the new head. Snapshot at d8cf07584, a read and not a wait: 36 check runs, total_count 36 equals the returned array length so nothing is truncated — 12 success, 3 skipped, 20 in_progress, 1 queued, 0 failing. Convergence and landing belong to the claiming seat.
Generated by Claude Code
Generated by Claude Code
os-tesla
marked this pull request as ready for review
September 19, 2026 22:59
os-tesla
deleted the
claude/issue-7392-inbox-receipt-incremental-read
branch
September 19, 2026 23:38
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.
Fixes #7392
Clause-②: no
The inbox poll re-read
sys_notification_receiptin full on every tick — filtered byuser_id+channel:'inbox',$top: 200, no cursor of any kind — beside thesys_inbox_messageread's$top: 20, at 10s foregrounded and 60s hidden.mergeInboxRowsuses the receipts as a lookup table keyed bynotification_idwhile mapping over the MESSAGE rows, so every receipt outside that window of 20 was fetched, indexed and dropped, six times a minute.The read now names those messages' notification ids as an
$incomparand, and is skipped outright when the window lists nothing a receipt could belong to.1. The badge verification — taken FIRST, because it decides the shape
The dispatch named this as the one way the change could go silently wrong: the receipt set feeds the bell's unread badge, and a receipt is not a "read" marker (
deliveredis a receipt and is NOT read). So "read only the receipts of the listed messages" invites turning an unread TOTAL into "unread among the newest 20" with no error and no red test.Where the number actually comes from, expression by expression:
layout/InboxPopover.tsxconst totalBadge = unreadTopics + pendingApprovalsCount;— what theinbox-bell-badgetestid rendersconst unreadTopics = allGroups.reduce((sum, g) =thensum + (g.unreadCountgreater-than0 ? 1 : 0), 0), overgroupNotifications(notifications)hooks/useInboxBell.tsnotificationsisuseSharedInboxFeed().valuewith the local read overlay applied; it also exposesunreadCount = notifications.reduce(...)hooks/sharedUserFeeds.tsmergeInboxRows(rows, receipts), whose body isreturn rows.map((raw) =...)— one output row persys_inbox_messagerowhooks/useHomeInbox.tsunreadTopicCountappliesgroupNotificationsto the same rows and reduces it the same way (#4329)⭐ Reading: the badge's inbox addend is bounded by the MESSAGE read's
$top: 20, never by the receipt set.mergeInboxRowsmaps over the message rows; a receipt whosenotification_idis not among them is never looked up and contributes nothing. The inbox addend has therefore been "unread within the$top: 20window" since #4225 gave the bell and Home one feed — the receipts only ever supplied read-state to rows the message query had already chosen.⇒ The fall-back clause does not fire. The count stays whole because it was never derived from the receipt set's size, and the second candidate shape is on the table.
Shape chosen: receipts for the listed messages. The since-cursor shape owes missed-update handling and cursor persistence for an answer that is discarded every tick anyway — the feed keeps no receipt state between ticks, so a cursor would have had to grow one.
2. Measured before/after — the card's closing criterion
Measured with a fake backend that honours
$filter(equality and$in) and$top, so the row count is a reading of the query the feed wrote rather than an echo of the fixture. Fixture: a user with 200 inbox receipts, a$top: 20message window.sys_notification_receiptrows delivered$topsentThe "after" row count is the number of receipts among the listed messages, so it is at most the window size and in ordinary use well under it — the card's "a few". The two numbers above are produced by the same assertions in
sharedInboxFeed.receiptScope-7392.test.tsx: the before figure is thewouldHaveDeliveredcontrol (the replaced filter, run against the same store), and it is re-measured by the reverse verification in section 4.$topis now the id count, and that bound is exact rather than guessed headroom:sys_notification_receiptdeclares its key{ fields: ['notification_id', 'user_id', 'channel'], unique: true }, so at most one row per named id can match.3. The existing #7249 pin — it went red, and that red was correct
It pins frequency, not payload: the
top=200mentions in its docblock and in one comment are prose, and the assertion isexpect(receiptReads()).toBe(inboxReads()). It went red anyway, atexpected +0 to be 7, because its fixture answers the message read with an empty inbox — the one window where a receipt read is now legitimately not due, so the old unconditional read was what made that equality hold there.Updated deliberately, and strengthened rather than loosened:
notification_id, so it is exercised on a window that has something to join;$top: 200was not kept to keep anything green, and no assertion was weakened. The historical sentence recording what app-shell: on the console HOME the inbox feed polls every 2 s (sys_inbox_message + sys_notification_receipt top=200) instead of the declared 10 s; realtime is loaded but unused for the bell #7249 measured is left standing as the record of that measurement, with a section noting what changed since.4. Reverse verification — predicted, then measured
Run against the committed fix (
git checkout HEAD --to restore, tree confirmed identical by blob hash afterwards,git diff HEADempty), by putting the pre-change read back:badgeandread-stategreen.6 failed | 5 passedacross the two files. Right about direction, wrong about the count, and the gap is the interesting part:read-statestayed GREEN — the old read carried the same answer, it just carried more rows to get there. That is the behaviour-preservation half, and it holds under both reads.badgewent red but not on the badge: itsunreadTopicsequality is evaluated first and passed under the old read too; what failed after it is the row arithmetic beside it (expected +0 to be 180— nothing dropped, because nothing was narrowed). The number is invariant, the payload is not.expected 200 to be 20; the$incomparandexpected undefined), and the new empty-window case failed withexpected 7 to be +0.$top: 200,occurrence counted 1, the removed$in: notificationIdscounted 0. No dist is involved — the pins import../sharedUserFeedsrelatively, so the mutation lands on the module under test with noexportsresolution in between.5. Side effect worth naming: a latent truncation hazard goes with it
The old read carried no
$orderbyunder a$top: 200ceiling. A user holding more than 200 inbox receipts could therefore have the newest messages' receipts fall outside the 200 returned, and already-read messages would render unread and inflate the badge. Narrowing the filter to the listed ids removes that failure mode rather than merely shrinking it; it is not a separate card because it is the same read.6. Cost paid
The two reads are sequential where they were a
Promise.all: the receipt query cannot be written until the message read says which notifications are in the window. One extra round trip per tick, on a background poll, against ~180 discarded rows saved on each of them. Stated here rather than buried — it is the only thing this change makes worse.Verification
pnpm exec vitest run— the two inbox pinspnpm exec vitest run— 7 consumer suites (twoSurfaces, rowShape, transient404, arrival notifier, inboxArrivals, global page blocks, AppHeader inbox variant, Home action centre, console HMR reloader)pnpm --filter '@object-ui/app-shell^...' build(dependency closure, built before any typecheck)pnpm --filter @object-ui/app-shell type-check(tsc --noEmit && tsc -p tsconfig.test.json), run after the last edittsc -p tsconfig.test.json --listFileshitsreceiptScope-7392.test.tsx1,cadence-7249.test.tsx1, controlhooks/sharedUserFeeds.ts1, 4114 files in the programnode scripts/check-changeset-presence.mjsAcceptance notes
noted, not filed:mergeInboxRowswritesreceipt_id: String(r?.id), which yields the literal string"undefined"for a receipt row carrying noid. Harmless today —receipt_idis written by that producer and read by no consumer (only the type declaration and test fixtures mention it). Not filed because the question "which PR or person meets this next" has no answer: the successor would be whoever first wiresreceipt_idup, and nobody is. Successor: none.skip-changeset, which AGENTS.md forbids applying here, and no dispatch clause named another; a changeset is declared instead.packages/plugin-list; this branch does not, so there is no overlap to reconcile. Neither app-shell: on the console HOME the inbox feed polls every 2 s (sys_inbox_message + sys_notification_receipt top=200) instead of the declared 10 s; realtime is loaded but unused for the bell #7249 nor Home readssys_inbox_messagetwice — the bell's poll anduseHomeInboxeach issue their own query #4225 is addressed here.Generated by Claude Code