fix(app-shell): the bell's Approvals and Activity tabs fill in off-app, from one shared fetch (#4197) - #4223
Merged
Merged
Conversation
…p, from one shared fetch (#4197) `AppHeader` gated the pending-approvals poll and the `sys_activity` read on `variant === 'app'`. The bell renders on Home, Organizations and the full-page AI screen too, so off-app both tabs were permanently empty — on the very page whose own To-do and activity cards (`useHomeInbox`, ungated) were listing the same rows from the same endpoint and the same object. Since #4199 un-gated the inbox half, the badge (`unreadTopics + pendingApprovalsCount`) fetched only its first addend off-app: one user with one set of data read 1 on Home and 3 inside an app. Neither feed is app-scoped — approvals are scoped to the user, activity to the tenant — so both are un-gated. To avoid paying for that with duplicate reads (on `/home` the bell and the cards mount in one tree), a module-scoped store `hooks/sharedUserFeeds` now owns each feed: one in-flight request, one 30s approvals poll, one 404-retires-the-feature rule, with the bell and `useHomeInbox` both subscribing. Home keeps its narrower cut of the activity rows (human actors only) by filtering the shared feed at its call site. Presence stays app-scoped: the avatars and the connection dot remain behind `isApp`, and presence was never a read at all — it is a transport-level subscription (`useTenantPresence`), which is why the effect formerly named `fetchPresenceAndActivities` only ever fetched `sys_activity`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
…s issue (#4197) Asserting an absence needs the tree to have settled first, and the case used the `sys_activity` read as that settle point — which the `home` variant does not issue on `origin/main`. So under reverse verification the presence pin went red for a timeout that says nothing about presence, when the whole point of a boundary pin is to be green on BOTH sides. Anchored on the inbox read instead: ungated in every variant since #4199, so it arrives before and after, and what the case reports is the render gate it was written to watch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
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 #4197
AppHeadergated two of the three streams that fill the bell onvariant === 'app': the pending-approvals poll (if (!isApp || !user?.id) return;) and thesys_activityread (if (!dataSource || !isApp) return;). The bell renders on Home, Organizations and the full-page AI screen too, so off-app the Approvals tab read "No pending approvals" and the Activity tab "No recent activity" — on the very page whose own cards were listing both, from the same endpoint and the same object.Since #4199 un-gated the inbox half, the badge (
unreadTopics + pendingApprovalsCount) fetched only its first addend off-app: one user with one set of data read 1 on Home and 3 inside an app, and the popover's own breakdown line disagreed with the number on the bell.Consumer map
Which surfaces read these two feeds, and what each fetches:
sys_activitylayout/AppHeader.tsxisAppisApphooks/useHomeInbox.ts→console/home/HomePage.tsx:256plugin-detail/renderers/record-activity.tsx,record-history.tsxobject_name+record_idviews/RecordDetailView.tsx:1368object_name+record_idhooks/useRecordApprovals.ts?object=…&recordId=…apps/console/src/services/approvalsApi.tsOnly the first two rows read the global feeds, and both live in
packages/app-shell. The record- and page-scoped readers ask different questions and are untouched.The bell renders in four layouts —
HomeLayout(variant="home"),OrganizationsLayoutandOrganizationLayout(variant="orgs"),ConsoleLayout(variant="app") — plusAiChatPage, which rendersAppHeader variant="home"; there is no separateai-chatvariant (AppHeaderVariant = 'app' | 'home' | 'orgs'), so the AI screen is covered by thehomecases.Chosen shape: one shared fetch, module-scoped store
Un-gating alone would have fixed the emptiness by paying for it twice — on
/homethe bell and the cards mount in one tree, so two owners means two approvals requests and twosys_activityreads per page. That is the trade-off the card declined to settle by duplicate polling.New
packages/app-shell/src/hooks/sharedUserFeeds.tsowns each feed: one in-flight request, one 30s approvals poll, one 404-retires-the-feature rule.AppHeaderanduseHomeInboxboth subscribe viauseSyncExternalStore; neither fetches these two streams any more.A module-scoped store rather than a context provider, because both consumers are already inside this package (no new dependency edge either way), and a store needs no provider mounted above every call site — the one-fetch guarantee holds no matter which of the four layouts renders the bell. The dedupe is structural, not agreed: there is no second producer left that could drift, so the badge and the card cannot show different numbers.
The dedupe has two layers:
inFlight, set synchronously before the firstawait, collapses two consumers attaching in the same commit; a 30s freshness window covers the later mount (the header commits a beat before the page body).useHomeInboxkeeps its narrower cut of the rows — human actors only,sys_*/ai_*churn dropped — by filtering the shared feed at its own call site instead of issuing its own query.Its
sys_inbox_messageread is deliberately left alone: Home asks a different question there (top-limittitles, no read-state receipts) than the bell does (top-20 joined withsys_notification_receiptfor unread state). That duplicate predates this card — filed separately as #4225.InboxPopover.tsxneeded no change; it already takespendingApprovalsCountandactivitiesas props, and every empty state it rendered was truthful about the data it was handed.Presence boundary
isAppkeeps the meaning it was introduced for. Measured boundary: presence was never a read at all.useTenantPresenceis a transport-level subscription (PresenceProvider.subscribeTenant), and the effect formerly namedfetchPresenceAndActivitiesexplicitly never probed it — its one and only read wassys_activity. So the split is not "share the fetch, keep the presence fetch gated"; there is no presence fetch. The boundary is a render gate, and it stays exactly where it was:ConnectionStatusrenders only underisApp && connectionStatePresenceAvatarsrenders only underisApp && activeUsers.length > 0Three cases pin it: no avatars and no connection dot off-app, both present inside an app, and no read in any variant names presence. The dividing line is data scope, not surface — user- and tenant-scoped feeds follow the bell wherever it renders; app-shell chrome does not.
Evidence
Reverse verification,
git checkout origin/main -- AppHeader.tsx useHomeInbox.ts, same test file — direction predicted before running:The 10 reds are the six home/orgs Approvals/Activity/badge cases, the empty-approvals case, and the three shared-fetch cases. The 12 greens are controls that must not move: the three
variant="app"cases, the three presence-boundary cases, and #4110's six.Two directions worth stating plainly, because the obvious reading of each is wrong:
main, Home issues exactly one approvals read and onesys_activityread —useHomeInboxalone, the bell being gated — sotoHaveLength(1)passes there. They fail earlier, on the divergence itself:Expected "2 pending approvals", Received "0 pending approvals"while the card next to it reads 2. Those assertions are a forward pin against a future naive un-gating, not evidence about this bug.sys_activityread, which thehomevariant does not issue onmain— so it went red for a timeout that says nothing about presence. Re-anchored on the inbox read (ungated in every variant since fix(app-shell): the bell polls the inbox on every console surface, not only inside an app (#4110) #4199), it is now green before and after, which is what makes it a control. That is the second commit.Local gates, re-run after merging current
main(which brings the@objectstack/specrc.6 bump):The downstream sweep uses the prefix filter (
...@object-ui/app-shell) — the four packages that depend on app-shell, not its dependencies. It neededpnpm --filter '@object-ui/console^...' buildfirst; without it the failures wereCannot find module '@object-ui/plugin-map'and friends, i.e. unbuilt siblings outside app-shell's own closure, not this change.CI status — three red jobs, none of them this PR's
Lint green. Type Check red. Test shards 2 and 3 red. Every one of those failures is
main's, inherited via the rc.6 spec bump (#4169 / tracker #4167), and none is in a package this PR touches:check:spec-symbols— 8 spec-named symbols declared locally@object-ui/react,@object-ui/typesregistry-inputs-spec-parity— stale exemptions,record_pickerkeysapps/consolerecordDetailsInputs.spec-parity— OBJECT sectionspackages/plugin-detailquick-reference-current-release-4143— doc says rc.5, manifests say rc.6scripts/Three independent lines of evidence:
mainis red on its own. The Type Check job failed atVerify spec-named symbols are derived, not hand-writtenon38ab5054fand ona9a67ec5b— the latter being this PR's original base.mainand deletingsharedUserFeeds.ts, then re-running the same tests on the identical tree:Tests 6 failed | 60 passed (66)— byte-identical to the run with the diff in. Same for thetsconfig.typetests.jsonerrors.mainchanged nothing. The failure set before and after the merge commit is the same, so it does not track this branch's staleness either.Measured detail added to the tracker: #4167 (comment). One surface there was previously unrecorded —
packages/app-shell/src/utils/resolveActionParams.test.tsfails only undertsc -p tsconfig.typetests.json, whiletsc --noEmitis clean, so a check that stops at the first command reads green.Changeset:
.changeset/bell-approvals-activity-off-app-4197.md,@object-ui/app-shellpatch.Out of scope
sys_inbox_messageis still read twice on Home — once by the bell's 10s poll (top-20 + receipts, for unread state) and once byuseHomeInbox(top-limittitles). It predates this card: #4199 un-gated the bell's inbox poll without merging it into Home's. It is mergeable — Home's rows are a prefix of the bell's superset — but the queries differ in shape and the merge is its own change. Filed as #4225 (finding, unassigned).The rc.6 fallout above is tracked on #4167, where the three surfaces its checklist did not yet enumerate are now recorded.
Generated by Claude Code