diff --git a/.changeset/shared-inbox-feed-4225.md b/.changeset/shared-inbox-feed-4225.md new file mode 100644 index 000000000..882ce93b0 --- /dev/null +++ b/.changeset/shared-inbox-feed-4225.md @@ -0,0 +1,35 @@ +--- +'@object-ui/app-shell': patch +--- + +Home's action centre stops counting messages the user has already read, and the inbox is read once per page instead of twice (#4316, #4225) + +`useHomeInbox` read `sys_inbox_message` and nothing else — it never joined +`sys_notification_receipt`, where ADR-0030 (resolved decision 2) puts read-state. +So Home's "Needs your attention" card could not tell a read message from an +unread one: it listed the five most recent unconditionally and badged them. A +user who opened the bell, read all nine messages and returned to Home still found +up to five of them filed as work waiting on them — while the bell two hundred +pixels above correctly showed zero, because its own poll did join the receipts. +One page load, two panels, opposite claims about the same rows (#4316). + +The fix is the one #4225 sketched: `hooks/sharedUserFeeds.ts` gains an inbox feed +holding the bell's already-joined 20-row window, polled once at the bell's 10s +cadence, and BOTH consumers derive from it — the bell lists the window and badges +its unread topics, Home takes the unread ones newest-first and caps them at its +own smaller limit. Home's second query is gone (one `sys_inbox_message` read and +one `sys_notification_receipt` read per page, not two and one), and the two +surfaces can no longer disagree about a row's read-state, because there is no +second read left to drift from the first. + +Two supporting changes travel with it, both visible only when something goes +wrong. The shared store now reports per-feed status in the same four words the +rest of the console uses (`idle` / `loading` / `ready` / `error`, per #4300's +one-dialect ruling): it used to swallow every failure into "keep the last value" +and say nothing, which is indistinguishable from a successful re-read, and would +have turned #4235's hard-won `error` state back into stale-but-confident data on +its way through the store. A missing object (404 / `OBJECT_NOT_FOUND`) is still +an answer — the deployment has no inbox, so nothing is waiting — and a denial +still is not. The bell's hidden-tab throttle, its return-to-tab refetch and its +failure backoff moved into the store with the poll rather than being dropped, and +now apply to every shared feed. diff --git a/packages/app-shell/src/console/home/__tests__/HomeActionCenter.unansweredInbox.test.tsx b/packages/app-shell/src/console/home/__tests__/HomeActionCenter.unansweredInbox.test.tsx index ef5f8abae..52122a3d7 100644 --- a/packages/app-shell/src/console/home/__tests__/HomeActionCenter.unansweredInbox.test.tsx +++ b/packages/app-shell/src/console/home/__tests__/HomeActionCenter.unansweredInbox.test.tsx @@ -85,14 +85,22 @@ vi.mock('../../../providers/AdapterProvider', () => ({ useAdapter: () => adapter /** * The other two feeds are #4197's shared store and are not this card's subject; * stubbing them keeps the approvals addend a dial this suite can set directly. + * + * Partial, not wholesale (#4225): the inbox read now lives in this same module + * (`useSharedInboxFeed`) and it IS this suite's subject, so it has to stay + * real. Keeping the original module underneath leaves the hop under test + * exactly where it was — `dataSource.find` → rejected promise → status — while + * still holding the approvals addend as a dial. */ let approvalsFixture = 0; -vi.mock('../../../hooks/sharedUserFeeds', () => ({ +vi.mock('../../../hooks/sharedUserFeeds', async (importOriginal) => ({ + ...(await importOriginal>()), useSharedPendingApprovalsCount: () => approvalsFixture, useHumanActivityFeed: () => [], })); import { useHomeInbox } from '../../../hooks/useHomeInbox'; +import { __resetSharedUserFeeds } from '../../../hooks/sharedUserFeeds'; import { HomeActionCenter } from '../HomeRail'; /** Exactly `HomePage`'s wiring of the two — the seam the card indicts. */ @@ -163,6 +171,9 @@ beforeEach(() => { approvalsFixture = 0; inboxBehaviour = async () => ({ data: [] }); findCalls.length = 0; + // The inbox feed is a module-scoped store that deliberately outlives any one + // render tree, so cases would otherwise inherit each other's rows and status. + __resetSharedUserFeeds(); }); describe('Home action centre — the affirmative empty state needs an ANSWER (#4235)', () => { @@ -254,8 +265,16 @@ describe('Home action centre — nine unread rows are listed and badged (#4235)' await waitFor(() => expect(screen.getByText('Approval request 1 needs your decision')).toBeInTheDocument(), ); - expect(screen.getAllByText(/Approval request \d+ needs your decision/)).toHaveLength(9); - expect(badgeText()).toBe('9'); + // Five, not nine — and five is what a real deployment always showed. The + // card's cap used to travel as the read's own `$top: 5`, which THIS fake + // adapter ignores (it answers every query with the full fixture), so the + // case measured nine only because nothing here enforced the server's cut. + // #4225 moved the read to the shared feed, whose `$top` is the bell's 20, + // and the cap became a client-side slice — enforced in the test exactly as + // the server enforced it in production. The rows-are-listed-and-badged + // claim this case exists to make is unchanged. + expect(screen.getAllByText(/Approval request \d+ needs your decision/)).toHaveLength(5); + expect(badgeText()).toBe('5'); expect(screen.queryByText(CAUGHT_UP)).not.toBeInTheDocument(); expect(screen.queryByTestId('home-action-unanswered')).not.toBeInTheDocument(); }); @@ -275,7 +294,10 @@ describe('Home action centre — nine unread rows are listed and badged (#4235)' expect(inboxRead?.query).toMatchObject({ $filter: { user_id: 'u1' }, $orderby: { created_at: 'desc' }, - $top: 5, + // 20, not 5: this is the bell's read now, and Home cuts its five from the + // superset (#4225). The `mine` scope and the ordering — what ADR-0030 + // actually names, and what this case is pinning — are untouched. + $top: 20, }); }); diff --git a/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx b/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx new file mode 100644 index 000000000..d4f1dd457 --- /dev/null +++ b/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx @@ -0,0 +1,511 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * #4225 / #4316 — the bell and Home's action centre, driven from ONE inbox feed. + * + * ## What was wrong + * + * Two consumers read `sys_inbox_message` on `/home`, and they asked different + * questions of it. The bell's poll joined `sys_notification_receipt` for + * read-state; `useHomeInbox` did not join anything. So Home could not tell a + * read message from an unread one and listed the five most recent + * unconditionally, badged — while the bell two hundred pixels above, reading + * the same rows through the join, correctly showed zero unread (#4316). One + * page load, two panels, opposite claims about the same messages. + * + * ## What these pin + * + * Both surfaces now derive from `sharedUserFeeds`' inbox feed, so the pins here + * are deliberately *joint*: every case mounts the real `AppHeader` and the real + * `HomeActionCenter` in ONE tree over ONE fake adapter and asserts what BOTH + * show. That is the difference between "the bug is fixed" and "the bug has no + * representable state left" — a future change that re-splits the read has to + * make these two panels disagree to get past them, which is exactly the failure + * mode being locked out. + * + * Reverse verification (predictions first, measured in PR #4319): + * - restore `useHomeInbox`'s own unjoined read ⇒ the #4316 block goes red + * (Home lists read messages the bell is not badging) and the one-read block + * goes red (two `sys_inbox_message` reads instead of one); + * - drop only the `!m.is_read` filter ⇒ the #4316 block goes red, the + * one-read block stays GREEN — which is why the read-count pin cannot stand + * in for the read-state pin, and both are here. + */ +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, within } from '@testing-library/react'; + +// ── Chrome the header pulls in but that this test does not exercise ────────── +// (the mock set `AppHeader.inboxVariant.test.tsx` established for mounting the +// real header in jsdom — kept in step with it deliberately.) + +vi.mock('react-router-dom', () => ({ + useLocation: () => ({ pathname: '/', search: '', hash: '', state: null, key: 'test' }), + useParams: () => ({}), + useNavigate: () => vi.fn(), + useSearchParams: () => [new URLSearchParams(), vi.fn()] as const, + Link: ({ children, to, ...p }: any) => {children}, +})); + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => ({ + language: 'en', + t: (key: string, options?: Record) => + String(options?.defaultValue ?? key).replace(/\{\{(\w+)\}\}/g, (_m, name: string) => + String(options?.[name] ?? ''), + ), + }), + useObjectLabel: () => ({ + objectLabel: (n: string) => n, + dashboardLabel: (n: string) => n, + pageLabel: (n: string) => n, + reportLabel: (n: string) => n, + viewLabel: (n: string) => n, + appLabel: (n: string) => n, + }), +})); + +vi.mock('@object-ui/components', () => { + const stripProps = (p: any) => { + const { asChild, variant, size, align, sideOffset, ...rest } = p ?? {}; + return rest; + }; + const Pass = ({ children, ...p }: any) =>
{children}
; + return { + Button: ({ children, asChild, variant, size, ...p }: any) => ( + + ), + DropdownMenu: Pass, + DropdownMenuTrigger: Pass, + DropdownMenuContent: () => null, + DropdownMenuItem: Pass, + DropdownMenuLabel: Pass, + DropdownMenuSeparator: () => null, + DropdownMenuGroup: Pass, + Avatar: Pass, + AvatarImage: () => null, + AvatarFallback: Pass, + Popover: Pass, + PopoverTrigger: Pass, + PopoverContent: Pass, + Tabs: Pass, + TabsList: Pass, + TabsTrigger: ({ children }: any) => , + TabsContent: Pass, + cn: (...c: any[]) => c.filter(Boolean).join(' '), + }; +}); + +vi.mock('lucide-react', () => { + const Icon = () => ; + return new Proxy({ __esModule: true } as Record, { + get: (target, prop) => { + if (prop === 'then' || prop === '__esModule' || typeof prop === 'symbol') { + return target[prop]; + } + return Icon; + }, + has: (_target, prop) => prop !== 'then', + }); +}); + +vi.mock('@object-ui/react', async (importOriginal) => ({ + ...(await importOriginal>()), + useOffline: () => ({ isOnline: true }), +})); + +vi.mock('@object-ui/collaboration', () => ({ + PresenceAvatars: () =>
, + useTenantPresence: () => [], +})); + +vi.mock('../../layout/ModeToggle', () => ({ ModeToggle: () => null })); +vi.mock('../../layout/WorkspaceSwitcher', () => ({ WorkspaceSwitcher: () => null })); +vi.mock('../../layout/LocaleSwitcher', () => ({ LocaleSwitcher: () => null })); +vi.mock('../../layout/ConnectionStatus', () => ({ ConnectionStatus: () => null })); +vi.mock('../../layout/AppSwitcher', () => ({ AppSwitcher: () => null })); +vi.mock('../../layout/LocalizedSidebarTrigger', () => ({ LocalizedSidebarTrigger: () => null })); +vi.mock('../../layout/PreviewBadge', () => ({ PreviewBadge: () => null })); + +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ + user: { id: 'u1', name: 'Zhang San', email: 'zs@example.com' }, + signOut: vi.fn(), + isAuthEnabled: true, + organizations: [], + activeOrganization: null, + isOrganizationsLoading: false, + getAuthConfig: undefined, + }), + getUserInitials: () => 'ZS', + useIsWorkspaceAdmin: () => false, +})); + +vi.mock('../../providers/MetadataProvider', () => ({ + useMetadata: () => ({ apps: [], dashboards: [], pages: [], reports: [] }), +})); + +// ── The fixture: one user's inbox, read-state carried by the receipts ──────── + +/** + * Nine messages for one user — #4316's exact reported shape ("a user who opens + * the bell and reads all nine messages, then returns to Home"). + */ +const NINE = Array.from({ length: 9 }, (_, i) => ({ + id: `ibx_${i + 1}`, + user_id: 'u1', + notification_id: `ntf_${i + 1}`, + topic: 'approval.reminder', + title: `Approval request ${i + 1} needs your decision`, + action_url: `/apps/crm/sys_approval_request/record/a_${i + 1}`, + created_at: `2026-08-0${i + 1}T09:00:00Z`, +})); + +/** + * Ten messages, five of them read — the mixed cut. Three share a `(topic, + * title)` so BOTH consumers' coalescing passes are exercised: the bell folds by + * `(topic, title)`, Home by title, and the unread subset has to survive each. + */ +const MIXED = [ + { id: 'ibx_01', notification_id: 'ntf_01', topic: 'approval.reminder', title: 'Approval reminder: INV-1008', created_at: '2026-08-11T04:30:00Z' }, + { id: 'ibx_02', notification_id: 'ntf_02', topic: 'hr.contract.expiring', title: 'Contract expiring: Zhang San', created_at: '2026-08-11T04:00:00Z' }, + { id: 'ibx_03', notification_id: 'ntf_03', topic: 'project.digest', title: 'Scheduled project digest', created_at: '2026-08-11T03:00:00Z' }, + { id: 'ibx_04', notification_id: 'ntf_04', topic: 'project.digest', title: 'Scheduled project digest', created_at: '2026-08-11T02:00:00Z' }, + { id: 'ibx_05', notification_id: 'ntf_05', topic: 'task.mention', title: 'You were mentioned in Task T-42', created_at: '2026-08-11T01:00:00Z' }, + { id: 'ibx_06', notification_id: 'ntf_06', topic: 'project.digest', title: 'Scheduled project digest', created_at: '2026-08-11T00:00:00Z' }, + { id: 'ibx_07', notification_id: 'ntf_07', topic: 'crm.lead.assigned', title: 'Lead assigned: Acme Corp', created_at: '2026-08-10T23:00:00Z' }, + { id: 'ibx_08', notification_id: 'ntf_08', topic: 'invoice.paid', title: 'Invoice INV-1004 paid', created_at: '2026-08-10T22:00:00Z' }, + { id: 'ibx_09', notification_id: 'ntf_09', topic: 'deal.won', title: 'Deal won: Globex renewal', created_at: '2026-08-10T21:00:00Z' }, + { id: 'ibx_10', notification_id: 'ntf_10', topic: 'system.maintenance', title: 'Scheduled maintenance Sunday 02:00', created_at: '2026-08-10T20:00:00Z' }, +].map((r) => ({ ...r, user_id: 'u1', action_url: `/apps/showcase/x/record/${r.id}` })); + +/** Rows 06-10 are read; 01-05 are not. */ +const MIXED_READ_IDS = ['ntf_06', 'ntf_07', 'ntf_08', 'ntf_09', 'ntf_10']; +/** The four distinct UNREAD titles (the two unread digests fold into one). */ +const UNREAD_TITLES = [ + 'Approval reminder: INV-1008', + 'Contract expiring: Zhang San', + 'Scheduled project digest', + 'You were mentioned in Task T-42', +]; +/** Titles that are read — neither surface may present them as waiting. */ +const READ_TITLES = [ + 'Lead assigned: Acme Corp', + 'Invoice INV-1004 paid', + 'Deal won: Globex renewal', + 'Scheduled maintenance Sunday 02:00', +]; + +/** `read` for the listed notification ids, `delivered` (= NOT read) for the rest. */ +const receiptsFor = (rows: Array<{ notification_id: string }>, readIds: string[]) => + rows.map((r, i) => ({ + id: `rcp_${i + 1}`, + notification_id: r.notification_id, + user_id: 'u1', + channel: 'inbox', + state: readIds.includes(r.notification_id) ? 'read' : 'delivered', + })); + +const finds: Array<{ object: string; query: unknown }> = []; +let inboxRows: Array> = []; +let receiptRows: Array> = []; +/** Overrides the inbox read outright (rejections, for the status cases). */ +let inboxBehaviour: (() => Promise) | null = null; + +const fakeAdapter = { + find: (object: string, query: unknown) => { + finds.push({ object, query }); + if (object === 'sys_inbox_message') { + return inboxBehaviour ? inboxBehaviour() : Promise.resolve({ data: inboxRows }); + } + if (object === 'sys_notification_receipt') return Promise.resolve({ data: receiptRows }); + return Promise.resolve({ data: [] }); + }, + getClient: () => undefined, +}; + +vi.mock('../../providers/AdapterProvider', () => ({ useAdapter: () => fakeAdapter })); + +import { AppHeader } from '../../layout/AppHeader'; +import { HomeActionCenter } from '../../console/home/HomeRail'; +import { useHomeInbox } from '../useHomeInbox'; +import { __resetSharedUserFeeds } from '../sharedUserFeeds'; + +/** + * Home's action centre wired exactly as `HomePage` wires it, fenced behind a + * testid: both panels render the same strings by construction when the fix + * works, so every assertion has to say WHICH panel it is talking about. + */ +function HomeProbe() { + const { pendingApprovalsCount, notifications, notificationsStatus } = useHomeInbox(); + return ( +
+ {}} + onOpenNotification={() => {}} + t={(key: string, options?: any) => + String(options?.defaultValue ?? key).replace(/\{\{(\w+)\}\}/g, (_m, name: string) => + String(options?.[name] ?? ''), + ) + } + /> +
+ ); +} + +/** The page as a user meets it: the bell above, the action centre below. */ +function HomeSurfaces() { + return ( + <> + + + + ); +} + +const home = () => screen.getByTestId('home-cards'); +/** Home's "Needs your attention" badge, or null when the card shows none. */ +const homeBadge = (): string | null => { + const heading = within(home()).getByText('Needs your attention'); + const badge = heading.parentElement?.querySelector('span.tabular-nums'); + return badge ? badge.textContent : null; +}; +/** Matches inside the bell only — i.e. anywhere that is not Home's card. */ +const inBell = (title: string) => + screen.queryAllByText(title).filter((el) => !home().contains(el)); +const inHome = (title: string) => + screen.queryAllByText(title).filter((el) => home().contains(el)); + +const inboxReads = () => finds.filter((f) => f.object === 'sys_inbox_message'); +const receiptReads = () => finds.filter((f) => f.object === 'sys_notification_receipt'); + +beforeEach(() => { + finds.length = 0; + inboxRows = []; + receiptRows = []; + inboxBehaviour = null; + // The feeds are module-scoped stores that outlive any one render tree. + __resetSharedUserFeeds(); + // Approvals soft-degrade to 0 (404), so every badge below is the inbox's + // contribution alone and the two surfaces' numbers are directly comparable. + vi.stubGlobal('fetch', vi.fn(() => Promise.resolve(new Response('{}', { status: 404 })))); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('#4316 — an already-read message is not "needs your attention"', () => { + it('nine messages, all read: the bell badges nothing AND Home lists nothing', async () => { + // #4316's headline scenario, verbatim: the user opened the bell and read + // all nine, then came back to Home. Before the join, Home listed five of + // them under "Needs your attention" with a badge, directly contradicting + // the bell above it. + inboxRows = NINE; + receiptRows = receiptsFor(NINE, NINE.map((r) => r.notification_id)); + + render(); + + await waitFor(() => expect(inboxReads().length).toBeGreaterThan(0)); + await waitFor(() => + expect(within(home()).getByText("You're all caught up")).toBeInTheDocument(), + ); + // Home: no rows, no badge. + expect(within(home()).queryAllByText(/Approval request \d+ needs your decision/)).toHaveLength(0); + expect(homeBadge()).toBeNull(); + // The bell agrees, and it is the same nine rows it is agreeing about. + expect(screen.queryByTestId('inbox-bell-badge')).not.toBeInTheDocument(); + }); + + it('one unread among nine: both surfaces name that one, and only that one', async () => { + // The sharpest cut — everything read except row 3. A card that lists "the + // five most recent" regardless of read-state shows four wrong rows here. + inboxRows = NINE; + receiptRows = receiptsFor( + NINE, + NINE.map((r) => r.notification_id).filter((id) => id !== 'ntf_3'), + ); + + render(); + + await waitFor(() => + expect(inHome('Approval request 3 needs your decision')).toHaveLength(1), + ); + // Nothing else reached Home's card … + expect(within(home()).queryAllByText(/Approval request \d+ needs your decision/)).toHaveLength(1); + expect(homeBadge()).toBe('1'); + // … and the bell counts exactly the same single unread message. + await waitFor(() => + expect(screen.getByTestId('inbox-bell-badge')).toHaveTextContent('1'), + ); + }); + + it('mixed read-state: the two surfaces agree on the unread subset', async () => { + inboxRows = MIXED; + receiptRows = receiptsFor(MIXED, MIXED_READ_IDS); + + render(); + + // Home lists the unread titles … + for (const title of UNREAD_TITLES) { + await waitFor(() => expect(inHome(title).length).toBeGreaterThan(0)); + } + // … and not one read title, on either surface's "waiting" reading. The + // bell's unread filter is its default, so a read row must not be there + // either — the same rows, the same verdict, from the same feed. + for (const title of READ_TITLES) { + expect(inHome(title)).toHaveLength(0); + expect(inBell(title)).toHaveLength(0); + } + }); + + it('reads the SAME badge number on both surfaces — 4 unread topics', async () => { + // Five unread rows folding into four distinct topics. The bell has always + // badged unread topics; Home now counts the same fold of the same rows, so + // the two numbers a user can see at once are one number. + inboxRows = MIXED; + receiptRows = receiptsFor(MIXED, MIXED_READ_IDS); + + render(); + + await waitFor(() => expect(screen.getByTestId('inbox-bell-badge')).toHaveTextContent('4')); + expect(homeBadge()).toBe('4'); + }); + + it('an unread message with no receipt at all still counts (delivered ≠ read)', async () => { + // The other polarity: read-state is asserted by a receipt in a READ state, + // never inferred from a receipt's absence — a fresh message has no receipt + // and is unread, which is what makes the join safe to gate the card on. + inboxRows = NINE; + receiptRows = []; + + render(); + + await waitFor(() => expect(homeBadge()).toBe('5')); + // Home caps its list at `limit` (5) … + expect(within(home()).queryAllByText(/Approval request \d+ needs your decision/)).toHaveLength(5); + // … while the bell, which lists the full window, badges all nine topics. + expect(screen.getByTestId('inbox-bell-badge')).toHaveTextContent('9'); + }); +}); + +describe('#4225 — one feed, one read, however many consumers mount', () => { + beforeEach(() => { + inboxRows = MIXED; + receiptRows = receiptsFor(MIXED, MIXED_READ_IDS); + }); + + it('issues ONE sys_inbox_message read for the bell and Home together', async () => { + render(); + + // Both surfaces have rendered from it … + await waitFor(() => expect(inHome('Approval reminder: INV-1008').length).toBeGreaterThan(0)); + expect(inBell('Approval reminder: INV-1008').length).toBeGreaterThan(0); + // … and exactly one read went out for the two of them. Two consumers, one + // query: the duplicate-read the card was filed for. + expect(inboxReads()).toHaveLength(1); + }); + + it('issues ONE sys_notification_receipt read too — the join is shared', async () => { + render(); + + await waitFor(() => expect(receiptReads().length).toBeGreaterThan(0)); + expect(receiptReads()).toHaveLength(1); + }); + + it('reads the ADR-0030 `mine` window once, as the bell\'s superset', async () => { + render(); + + await waitFor(() => expect(inboxReads().length).toBeGreaterThan(0)); + expect(inboxReads()[0].query).toMatchObject({ + $filter: { user_id: 'u1' }, + $orderby: { created_at: 'desc' }, + $top: 20, + }); + expect(receiptReads()[0].query).toMatchObject({ + $filter: { user_id: 'u1', channel: 'inbox' }, + }); + }); + + it('serves a second consumer mounting later from the same feed, without re-reading', async () => { + const view = render(); + await waitFor(() => expect(inboxReads()).toHaveLength(1)); + + // Home mounts afterwards — the common case, the page body settling after + // the header — and is served the cached rows rather than issuing its own. + view.rerender(); + + await waitFor(() => expect(inHome('Approval reminder: INV-1008').length).toBeGreaterThan(0)); + expect(inboxReads()).toHaveLength(1); + }); +}); + +describe('#4225 — a failed inbox read reaches Home as an error, not as stale data', () => { + /** objectstack#7344's rejection: the object exists, this caller may not read it. */ + const denied = () => { + const err = new Error( + "Access denied: operation 'find' on object 'sys_inbox_message' is not permitted", + ) as Error & { httpStatus?: number; code?: string }; + err.httpStatus = 403; + err.code = 'PERMISSION_DENIED'; + return Promise.reject(err); + }; + + it('does not let the store swallow a denial into stale-but-ready rows', async () => { + // The store's pre-#4225 contract was "on error keep the last value" and say + // nothing — which for a consumer is indistinguishable from a successful + // re-read returning the same thing. Here the feed HAS rows in hand and then + // the next read fails: the rows may stay, but the card must stop claiming + // they are an answer. + inboxRows = MIXED; + receiptRows = receiptsFor(MIXED, MIXED_READ_IDS); + render(); + await waitFor(() => expect(inHome('Approval reminder: INV-1008').length).toBeGreaterThan(0)); + expect(within(home()).queryByTestId('home-action-unanswered')).not.toBeInTheDocument(); + + // Re-read the same feed, in the one way a mounted page really does it: the + // tab regains focus. (A remount would NOT do — it lands inside the store's + // freshness window and is served the cache, which is the dedupe working.) + inboxBehaviour = denied; + document.dispatchEvent(new Event('visibilitychange')); + + await waitFor(() => + expect(within(home()).getByTestId('home-action-unanswered')).toBeInTheDocument(), + ); + expect(within(home()).getByText('An unexpected error occurred.')).toBeInTheDocument(); + expect(within(home()).queryByText("You're all caught up")).not.toBeInTheDocument(); + // …and the rows are still on screen. "Not an answer any more" is the claim, + // not "throw the user's inbox away" — the two are different states and the + // status is what separates them. + expect(inHome('Approval reminder: INV-1008').length).toBeGreaterThan(0); + }); + + it('still treats a MISSING inbox object as an answer, for both surfaces', async () => { + // The 404-is-an-answer split, now made once inside the store instead of + // three times across three call sites (#4225 rider). A community build + // without service-messaging has no inbox, so nothing is waiting — get this + // wrong and every such deployment reads an error on Home forever. + inboxBehaviour = () => { + const err = new Error('Object not found: sys_inbox_message') as Error & { + httpStatus?: number; + code?: string; + }; + err.httpStatus = 404; + err.code = 'OBJECT_NOT_FOUND'; + return Promise.reject(err); + }; + + render(); + + await waitFor(() => + expect(within(home()).getByText("You're all caught up")).toBeInTheDocument(), + ); + expect(within(home()).queryByTestId('home-action-unanswered')).not.toBeInTheDocument(); + expect(screen.queryByTestId('inbox-bell-badge')).not.toBeInTheDocument(); + }); +}); diff --git a/packages/app-shell/src/hooks/__tests__/sharedUserFeeds.isMissingResource.test.ts b/packages/app-shell/src/hooks/__tests__/sharedUserFeeds.isMissingResource.test.ts new file mode 100644 index 000000000..6b6a81c64 --- /dev/null +++ b/packages/app-shell/src/hooks/__tests__/sharedUserFeeds.isMissingResource.test.ts @@ -0,0 +1,60 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * #4225 rider — one `isMissingResource`, not three. + * + * `sharedUserFeeds`, `AppHeader`'s inbox poller and `useHomeInbox` each carried + * their own copy of this predicate. It is not a formatting detail that they + * did: the predicate decides which of two OPPOSITE things a failed read means — + * + * - true ⇒ this deployment does not have the object. Nothing is waiting on + * the user, the feed degrades to empty, the poll retires, and the + * affirmative empty copy is honest ("You're all caught up"). + * - false ⇒ the read of a present object failed. The empty value is the + * ABSENCE of an answer and the surface must say so (#4235). + * + * Three copies of that split is three chances for one of them to drift, and the + * drift is silent in the worst direction: a denial re-classified as "missing" + * reproduces exactly the objectstack#7344 lie #4235 was filed to kill. These + * pin both polarities on the one surviving definition. + */ +import { describe, it, expect } from 'vitest'; +import { isMissingResource } from '../sharedUserFeeds'; + +/** The ObjectStack client's rejection shape: `httpStatus`, plus an error code. */ +function clientError(over: { httpStatus?: number; status?: number; code?: string }): Error { + return Object.assign(new Error('read failed'), over); +} + +describe('isMissingResource — the 404-is-an-answer split (#4225 rider)', () => { + it('is true for the client\'s `httpStatus` 404', () => { + // The spelling that matters: the ObjectStack client throws `httpStatus`, + // NOT `status`, and a copy that only checked `status` would classify every + // genuinely-missing object as a failure. + expect(isMissingResource(clientError({ httpStatus: 404 }))).toBe(true); + }); + + it('is true for a plain `status` 404 as well', () => { + expect(isMissingResource(clientError({ status: 404 }))).toBe(true); + }); + + it('is true for an OBJECT_NOT_FOUND error code without any status', () => { + expect(isMissingResource(clientError({ code: 'OBJECT_NOT_FOUND' }))).toBe(true); + }); + + it('is FALSE for the objectstack#7344 permission denial', () => { + // The case the whole split exists for. A 403 is a present object this + // caller may not read — an error, never "you have no inbox". + expect(isMissingResource(clientError({ httpStatus: 403, code: 'PERMISSION_DENIED' }))).toBe( + false, + ); + }); + + it('is FALSE for a server error, a network throw and a non-error value', () => { + expect(isMissingResource(clientError({ httpStatus: 500 }))).toBe(false); + expect(isMissingResource(new TypeError('Failed to fetch'))).toBe(false); + expect(isMissingResource(null)).toBe(false); + expect(isMissingResource(undefined)).toBe(false); + }); +}); diff --git a/packages/app-shell/src/hooks/sharedUserFeeds.ts b/packages/app-shell/src/hooks/sharedUserFeeds.ts index 96ed95a28..608e68a6c 100644 --- a/packages/app-shell/src/hooks/sharedUserFeeds.ts +++ b/packages/app-shell/src/hooks/sharedUserFeeds.ts @@ -9,6 +9,8 @@ * | | | tab; Home's To-do card | * | recent activity | `find('sys_activity', top 20, desc)` | AppHeader bell Activity tab; | * | | | Home's activity card | + * | inbox messages | `find('sys_inbox_message', top 20, desc)` | AppHeader bell Notifications tab | + * | | ⋈ `find('sys_notification_receipt')` | + badge; Home's action centre | * * Both consumers live in this package and, on `/home`, mount in the same tree * (`HomeLayout` renders the bell, `HomePage` renders the cards) — so each of @@ -16,6 +18,15 @@ * is exactly the trade-off #4197 refused to accept as the price of un-gating * the bell: the fix is one fetch feeding both, not two fetches agreeing. * + * The inbox feed joined them last (#4225). #4197 had left it out because the + * two consumers asked genuinely different questions of the same object — and + * #4316 measured what that cost: `useHomeInbox` never read the receipts, so + * Home's "Needs your attention" counted messages the user had already read + * while the bell two hundred pixels above correctly showed zero. Two panels, + * one page load, disagreeing about the same rows. Deriving both from ONE feed + * is what makes that disagreement structurally impossible rather than merely + * fixed: there is no second read left to drift. + * * Neither feed is app-scoped, so neither is gated on the header's `isApp` * flag. `isApp` still means something — it hides genuinely app-shell chrome * (presence avatars, the connection dot) — but the approvals inbox and the @@ -42,9 +53,26 @@ import { errorCodeIs } from '@object-ui/types'; import { useAdapter } from '../providers/AdapterProvider'; import { bearerAuthHeaders } from '../utils/authToken'; import type { ActivityItem } from '../layout/ActivityFeed'; +import type { InboxNotification } from '../layout/inboxGrouping'; /** Approvals poll cadence — the bell's original 30s (M11.C15). */ const APPROVALS_POLL_MS = 30_000; +/** Inbox poll cadence — the bell's original 10s (ADR-0030 L5, #4110). */ +const INBOX_POLL_MS = 10_000; +/** + * Cadence while the tab is backgrounded. The bell's own poller carried this + * (60s hidden against 10s foregrounded) plus an immediate refetch when the + * user comes back, and consolidating the poll into the store had to bring + * both along — a shared feed that polled a hidden tab at its foreground rate + * would have been a regression riding in on a de-duplication fix (#4225). + */ +const HIDDEN_POLL_MS = 60_000; +/** + * Ceiling for the failure backoff, likewise lifted from the bell's poller and + * now applied to every feed: a feed whose read keeps failing must not keep + * hammering the server at its foreground cadence. + */ +const MAX_BACKOFF_MS = 120_000; /** * How long a fetched value stays authoritative. It is the dedupe window: a * second consumer mounting inside it is served the cached value instead of @@ -59,14 +87,55 @@ const FRESH_MS = 30_000; * value must be one shared array (cf. `EMPTY_PRESENCE_USERS` in AppHeader). */ const NO_ACTIVITIES: ActivityItem[] = []; +/** The same stable-empty rule, for the inbox feed. */ +const NO_MESSAGES: InboxNotification[] = []; + +/** + * Whether a feed's value is an ANSWER — one dialect for every feed here. + * + * - `idle` — nothing asked yet (no adapter, no signed-in user, post-reset). + * - `loading` — asked, still in flight, no prior answer to show. + * - `ready` — the read answered. Only here does an empty value mean the + * feed is genuinely empty. A MISSING resource (404 / + * `OBJECT_NOT_FOUND`) is `ready` too: this deployment has no + * such object, so nothing is waiting — that is an answer. + * - `error` — the read failed (denied, unreachable, malformed). The value + * is the last one known, or empty; either way it is not an + * answer to the question being asked now. + * + * The same four words as `MetadataTypeStatus` (`providers/MetadataProvider`) + * and `HomeInboxStatus` (`useHomeInbox`), deliberately: #4300 ruled one status + * dialect for this exact question, and #4235 applied it to the inbox. The + * store used to have none — every failure was swallowed into "keep the last + * value", which is indistinguishable from a successful re-read returning the + * same thing. #4225 filled that gap for ALL feeds at once rather than adding a + * second, inbox-only dialect beside it. + */ +export type SharedFeedStatus = 'idle' | 'loading' | 'ready' | 'error'; + +/** A feed's value together with whether that value is an answer. */ +export interface SharedFeedSnapshot { + value: T; + status: SharedFeedStatus; +} /** * The runner produces the feed's next value, or `undefined` to leave the last - * one in place (a transient error, a non-OK response). `markUnavailable()` - * retires the feed for the rest of the page — the deployment does not have the - * approvals plugin / the `sys_activity` object, so retrying is pure noise. + * one in place. Which of the two "no value" cases it is has to be said out + * loud, because the store can no longer guess: + * + * - `markUnavailable()` retires the feed for the rest of the page — the + * deployment does not have the approvals plugin / the `sys_activity` + * object / the messaging pipeline, so retrying is pure noise. That is an + * ANSWER (`ready`). + * - `markFailed()` reports a read that should have worked and did not. The + * last value stays on screen, the status goes `error`, and the next poll + * backs off. A thrown error is equivalent — the runner may just let it fly. */ -type FeedRunner = (ctx: { markUnavailable: () => void }) => Promise; +type FeedRunner = (ctx: { + markUnavailable: () => void; + markFailed: () => void; +}) => Promise; /** * One feed's shared state. Consumers `attach` (from an effect) and read via @@ -74,7 +143,13 @@ type FeedRunner = (ctx: { markUnavailable: () => void }) => Promise { - private value: T; + /** + * The published snapshot. Cached as ONE object and replaced only when the + * value or the status actually changes: `useSyncExternalStore` re-renders in + * a loop if `getSnapshot` hands back a fresh reference each call, so pairing + * the value with its status must not mean building a new pair per read. + */ + private snapshot: SharedFeedSnapshot; private key: string | null = null; private readonly listeners = new Set<() => void>(); private runner: FeedRunner | null = null; @@ -83,13 +158,18 @@ class SharedFeed { private unavailable = false; private fetchedAt = 0; private timer: ReturnType | null = null; + /** Current inter-poll delay: `pollMs`, doubled per failure, capped. */ + private backoffMs: number; + private visibilityBound = false; constructor( private readonly empty: T, /** Re-fetch cadence while at least one consumer is mounted; 0 = fetch once. */ private readonly pollMs: number, ) { - this.value = empty; + this.snapshot = { value: empty, status: 'idle' }; + this.idleSnapshot = this.snapshot; + this.backoffMs = pollMs; } subscribe = (onStoreChange: () => void): (() => void) => { @@ -99,7 +179,15 @@ class SharedFeed { }; }; - getSnapshot = (): T => this.value; + getSnapshot = (): SharedFeedSnapshot => this.snapshot; + + /** + * What a consumer that is not driving a fetch reads: nothing, not asked. + * A cached `ready` belongs to the key that earned it — a signed-out session + * must not inherit the previous user's answer, nor their rows. One frozen + * object so it is snapshot-stable like any other published value. + */ + readonly idleSnapshot: SharedFeedSnapshot; /** * Register a consumer. `key` identifies *whose* feed this is (the approver @@ -114,16 +202,25 @@ class SharedFeed { this.key = key; this.unavailable = false; this.fetchedAt = 0; - this.publish(this.empty); + this.backoffMs = this.pollMs; + // A different key means the cached value belongs to someone else, and + // so does the fact that it was an answer — back to `idle`, not `ready`. + this.publish(this.empty, 'idle'); } // Freshest closure wins — it holds the current adapter / identities. this.runner = runner; this.consumers += 1; - if (this.consumers === 1) this.schedule(); + if (this.consumers === 1) { + this.bindVisibility(); + this.schedule(); + } void this.refresh(); return () => { this.consumers = Math.max(0, this.consumers - 1); - if (this.consumers === 0) this.stopPolling(); + if (this.consumers === 0) { + this.stopPolling(); + this.unbindVisibility(); + } }; } @@ -138,32 +235,88 @@ class SharedFeed { if (!runner || this.unavailable || this.inFlight) return; if (!force && this.fetchedAt && Date.now() - this.fetchedAt < FRESH_MS) return; this.inFlight = true; + // Only announce "loading" when there is no prior answer to show. A poll + // tick over a `ready` feed must not flash its consumers back through the + // loading state ten times a minute. + if (this.snapshot.status === 'idle') this.publish(this.snapshot.value, 'loading'); + let failed = false; try { const next = await runner({ markUnavailable: () => { this.unavailable = true; this.stopPolling(); + // A missing resource IS an answer: this deployment has none, so + // nothing is waiting. Degrading to empty and calling it `ready` is + // the split `useHomeInbox` already applied to its own read (#4235). + this.fetchedAt = Date.now(); + this.publish(this.snapshot.value, 'ready'); + }, + markFailed: () => { + failed = true; }, }); if (next !== undefined) { this.fetchedAt = Date.now(); - this.publish(next); + this.backoffMs = this.pollMs; + this.publish(next, 'ready'); + } else if (failed) { + this.fail(); } } catch { - // Transient — keep the last value; the next poll / mount retries. + // Not swallowed into "keep the last value and say nothing": the value + // stays, but consumers are told it is no longer an answer (#4225). + this.fail(); } finally { this.inFlight = false; } } + /** A read that should have worked did not — report it and back the poll off. */ + private fail(): void { + this.backoffMs = Math.min(Math.max(this.backoffMs, this.pollMs) * 2, MAX_BACKOFF_MS); + this.publish(this.snapshot.value, 'error'); + } + private schedule(): void { if (this.pollMs <= 0 || this.unavailable || this.timer) return; + // Backgrounded tabs poll at the slower cadence — but never FASTER than the + // current backoff, so a failing feed stays backed off either way. + const hidden = typeof document !== 'undefined' && document.hidden; + const delay = hidden ? Math.max(HIDDEN_POLL_MS, this.backoffMs) : this.backoffMs; this.timer = setTimeout(() => { this.timer = null; void this.refresh(true).finally(() => { if (this.consumers > 0) this.schedule(); }); - }, this.pollMs); + }, delay); + } + + /** + * Coming back to the tab refetches immediately rather than waiting out a + * hidden-cadence tick, so the bell is current within a beat of the user + * looking at it — the behaviour its own poller had before #4225 moved it. + */ + private readonly onVisibilityChange = (): void => { + if (typeof document === 'undefined' || document.hidden) return; + if (this.consumers === 0 || this.unavailable) return; + this.stopPolling(); + this.backoffMs = this.pollMs; + void this.refresh(true).finally(() => { + if (this.consumers > 0) this.schedule(); + }); + }; + + private bindVisibility(): void { + if (this.pollMs <= 0 || this.visibilityBound) return; + if (typeof document === 'undefined') return; + document.addEventListener('visibilitychange', this.onVisibilityChange); + this.visibilityBound = true; + } + + private unbindVisibility(): void { + if (!this.visibilityBound || typeof document === 'undefined') return; + document.removeEventListener('visibilitychange', this.onVisibilityChange); + this.visibilityBound = false; } private stopPolling(): void { @@ -173,22 +326,24 @@ class SharedFeed { } } - private publish(next: T): void { - if (Object.is(next, this.value)) return; - this.value = next; + private publish(next: T, status: SharedFeedStatus): void { + if (Object.is(next, this.snapshot.value) && status === this.snapshot.status) return; + this.snapshot = { value: next, status }; for (const listener of [...this.listeners]) listener(); } /** Test seam — drop all cached state between cases. Listeners are left alone. */ reset(): void { this.stopPolling(); - this.value = this.empty; + this.unbindVisibility(); + this.snapshot = { value: this.empty, status: 'idle' }; this.key = null; this.runner = null; this.consumers = 0; this.inFlight = false; this.unavailable = false; this.fetchedAt = 0; + this.backoffMs = this.pollMs; } } @@ -197,7 +352,11 @@ class SharedFeed { * signed-in user, no adapter) — the consumer still reads the snapshot, it just * does not drive a fetch. */ -function useSharedFeed(feed: SharedFeed, key: string | null, runner: FeedRunner): T { +function useSharedFeed( + feed: SharedFeed, + key: string | null, + runner: FeedRunner, +): SharedFeedSnapshot { const value = useSyncExternalStore(feed.subscribe, feed.getSnapshot, feed.getSnapshot); // Latest-ref: the runner closes over values that change every render, but // only `key` may re-drive the attach effect. Declared first so it lands @@ -210,7 +369,9 @@ function useSharedFeed(feed: SharedFeed, key: string | null, runner: FeedR if (!key) return; return feed.attach(key, (ctx) => runnerRef.current(ctx)); }, [feed, key]); - return value; + // No key ⇒ this consumer has asked nothing, so it is told nothing — never + // the cached answer to a question somebody else asked (#4235's `idle`). + return key ? value : feed.idleSnapshot; } // ── Pending approvals ──────────────────────────────────────────────────────── @@ -244,7 +405,7 @@ export function useSharedPendingApprovalsCount(): number { // `user?.id` is the sign-in gate; identities is what the query needs. const key = user?.id && identities.length > 0 ? identities.join(',') : null; - return useSharedFeed(approvalsFeed, key, async ({ markUnavailable }) => { + return useSharedFeed(approvalsFeed, key, async ({ markUnavailable, markFailed }) => { const serverUrl = (import.meta.env?.VITE_SERVER_URL || '').replace(/\/$/, ''); const qs = new URLSearchParams({ status: 'pending', approverId: identities.join(',') }); const res = await fetch(`${serverUrl}/api/v1/approvals/requests?${qs}`, { @@ -256,12 +417,15 @@ export function useSharedPendingApprovalsCount(): number { markUnavailable(); return undefined; } - if (!res.ok) return undefined; + if (!res.ok) { + markFailed(); + return undefined; + } const payload = await res.json().catch(() => null); const seen = new Set(); for (const row of (payload?.data || []) as { id: string }[]) seen.add(row.id); return seen.size; - }); + }).value; } // ── Recent activity ────────────────────────────────────────────────────────── @@ -271,6 +435,8 @@ const activityFeed = new SharedFeed(NO_ACTIVITIES, 0); /** * Stable string id per adapter instance, so swapping the adapter (tenant * switch) drops the previous tenant's rows instead of serving them from cache. + * Feed-neutral: each feed composes it with whatever else scopes its rows (the + * inbox adds the signed-in user id, since its query is `mine`). */ const adapterKeys = new WeakMap(); let adapterSeq = 0; @@ -278,7 +444,7 @@ function adapterKey(adapter: unknown): string | null { if (!adapter || typeof adapter !== 'object') return null; let key = adapterKeys.get(adapter as object); if (!key) { - key = `sys_activity@${++adapterSeq}`; + key = `adapter@${++adapterSeq}`; adapterKeys.set(adapter as object, key); } return key; @@ -326,8 +492,18 @@ function mapActivityRows(rows: unknown[]): ActivityItem[] { }); } -/** The ObjectStack client throws `httpStatus` (not `status`) with an error code. */ -function isMissingResource(err: unknown): boolean { +/** + * A missing OBJECT, as opposed to a failed read of a present one — the split + * that decides whether an empty result is an answer. The ObjectStack client + * throws `httpStatus` (not `status`) with an error code. + * + * Exported because all three inbox-surface readers need exactly this predicate + * and used to carry a copy each — `sharedUserFeeds`, `AppHeader`'s poller and + * `useHomeInbox` (#4225). Three copies of a predicate whose two branches mean + * "degrade quietly" and "say the read failed" is three chances to drift on the + * distinction #4235 exists to protect. + */ +export function isMissingResource(err: unknown): boolean { const e = err as { httpStatus?: number; status?: number } | null; return e?.httpStatus === 404 || e?.status === 404 || errorCodeIs(err, 'OBJECT_NOT_FOUND'); } @@ -342,19 +518,26 @@ function isMissingResource(err: unknown): boolean { export function useSharedActivityFeed(): ActivityItem[] { const dataSource = useAdapter(); - return useSharedFeed(activityFeed, adapterKey(dataSource), async ({ markUnavailable }) => { - if (!dataSource) return undefined; - const res = await Promise.resolve( - dataSource.find('sys_activity', { $orderby: { timestamp: 'desc' }, $top: 20 }) as Promise<{ - data?: unknown[]; - }>, - ).catch((err: unknown) => { - if (isMissingResource(err)) markUnavailable(); - return null; - }); - if (!res) return undefined; - return mapActivityRows(Array.isArray(res.data) ? res.data : []); - }); + return useSharedFeed( + activityFeed, + adapterKey(dataSource), + async ({ markUnavailable, markFailed }) => { + if (!dataSource) return undefined; + const res = await Promise.resolve( + dataSource.find('sys_activity', { $orderby: { timestamp: 'desc' }, $top: 20 }) as Promise<{ + data?: unknown[]; + }>, + ).catch((err: unknown) => { + // No `sys_activity` object ⇒ this deployment has no audit plugin, which + // is an answer. Anything else is a read that failed and must say so. + if (isMissingResource(err)) markUnavailable(); + else markFailed(); + return null; + }); + if (!res) return undefined; + return mapActivityRows(Array.isArray(res.data) ? res.data : []); + }, + ).value; } /** @@ -373,6 +556,118 @@ export function useHumanActivityFeed(limit: number): ActivityItem[] { }, [all, limit]); } +// ── Inbox messages ─────────────────────────────────────────────────────────── + +const inboxFeed = new SharedFeed(NO_MESSAGES, INBOX_POLL_MS); + +/** + * Receipt states that count as READ (ADR-0030). `delivered` is not one of them + * — a message can carry a receipt and still be unread, which is the whole + * reason read-state cannot be inferred from the receipt's mere existence. + */ +const READ_STATES = new Set(['read', 'clicked', 'dismissed']); + +/** + * Join the `mine` inbox rows to their read-state receipts — the merge the + * bell's poller did inline, now the shared feed's single definition of what a + * message IS. `useHomeInbox` never had this join at all (#4316), which is why + * Home counted already-read messages as needing attention. + */ +function mergeInboxRows(rows: unknown[], receipts: unknown[]): InboxNotification[] { + // notification_id → { id, state } (most-advanced receipt wins). + const receiptByNotif = new Map(); + for (const raw of receipts) { + const r = raw as Record | null; + const nid = r?.notification_id != null ? String(r.notification_id) : ''; + if (!nid) continue; + const state = String(r?.state ?? ''); + const prev = receiptByNotif.get(nid); + // Prefer a read/clicked/dismissed receipt over a plain delivered one. + if (!prev || (!READ_STATES.has(prev.state) && READ_STATES.has(state))) { + receiptByNotif.set(nid, { id: String(r?.id), state }); + } + } + return rows.map((raw) => { + const m = raw as Record; + const nid = m?.notification_id != null ? String(m.notification_id) : null; + const rec = nid ? receiptByNotif.get(nid) : undefined; + return { + id: String(m.id), + notification_id: nid, + receipt_id: rec?.id ?? null, + type: (m.topic as string) ?? 'notification', + title: (m.title as string) ?? '', + body: (m.body_md as string) ?? null, + action_url: (m.action_url as string) ?? null, + is_read: rec ? READ_STATES.has(rec.state) : false, + created_at: m.created_at as string | undefined, + } satisfies InboxNotification; + }); +} + +/** + * The signed-in user's 20 most recent in-app inbox messages, joined with their + * read-state receipts (ADR-0030 L5, the `mine` materialization). + * + * Two scoped reads, joined client-side, polled at 10s while the tab is + * foregrounded — the bell's cadence, now the store's: + * - `sys_inbox_message` filtered by `user_id`, newest first, `$top: 20`. + * - `sys_notification_receipt` filtered by `user_id` + `channel:'inbox'`. + * Best-effort: if receipts are unavailable the inbox still renders + * (everything shows unread) rather than erroring. + * + * This is the SUPERSET both consumers cut from. The bell lists all 20 and + * badges the unread topics; Home's action centre takes the unread ones, newest + * first, capped at its own smaller limit. Neither issues a read of its own, so + * the two cannot disagree about a row's read-state — the #4316 defect is not + * merely fixed here, it is unreachable. + * + * Degrades to empty when the messaging pipeline is absent (404 / + * `OBJECT_NOT_FOUND`) and retires the poll; every other failure is reported as + * `error` so a denial cannot reach a consumer wearing the shape of an empty + * inbox (#4235, objectstack#7344). + */ +export function useSharedInboxFeed(): SharedFeedSnapshot { + const dataSource = useAdapter(); + const { user } = useAuth(); + const userId = user?.id; + // Scoped by adapter AND user: the query is `mine`, so another user's rows + // must never be served from cache after a session switch. + const adapter = adapterKey(dataSource); + const key = adapter && userId ? `${adapter}:${userId}` : null; + + return useSharedFeed(inboxFeed, key, async ({ markUnavailable, markFailed }) => { + if (!dataSource || !userId) return undefined; + try { + const [inboxRes, receiptRes] = await Promise.all([ + Promise.resolve( + dataSource.find('sys_inbox_message', { + $filter: { user_id: userId }, + $orderby: { created_at: 'desc' }, + $top: 20, + }) as Promise<{ data?: unknown[] }>, + ), + Promise.resolve( + dataSource.find('sys_notification_receipt', { + $filter: { user_id: userId, channel: 'inbox' }, + $top: 200, + }) as Promise<{ data?: unknown[] }>, + ).catch(() => ({ data: [] as unknown[] })), + ]); + return mergeInboxRows( + Array.isArray(inboxRes?.data) ? inboxRes.data : [], + Array.isArray(receiptRes?.data) ? receiptRes.data : [], + ); + } catch (err: unknown) { + // No inbox object ⇒ no messaging pipeline in this deployment, so nothing + // is waiting: an answer. A denial / outage / malformed reply is not. + if (isMissingResource(err)) markUnavailable(); + else markFailed(); + return undefined; + } + }); +} + /** * Test seam: drop every shared feed's cached value, key and in-flight state so * cases do not inherit each other's reads. Not part of the public surface. @@ -380,4 +675,5 @@ export function useHumanActivityFeed(limit: number): ActivityItem[] { export function __resetSharedUserFeeds(): void { approvalsFeed.reset(); activityFeed.reset(); + inboxFeed.reset(); } diff --git a/packages/app-shell/src/hooks/useHomeInbox.ts b/packages/app-shell/src/hooks/useHomeInbox.ts index f493c7863..4fbf81e1e 100644 --- a/packages/app-shell/src/hooks/useHomeInbox.ts +++ b/packages/app-shell/src/hooks/useHomeInbox.ts @@ -28,21 +28,34 @@ * #4300, which fixed the same class ("an unloadable app list is UNKNOWN, not * 'no default app'") and ruled one source of truth, no second dialect. * - * Approvals and activity are NOT fetched here (#4197). Both come from + * NOTHING is fetched here (#4197, #4225). All three streams come from * `sharedUserFeeds`, which the top-bar bell reads too — on `/home` the bell and * these cards mount in one tree, so two owners meant the same read went out * twice per page. One fetch now feeds both, which is also what makes the bell's * badge and this card structurally incapable of showing different numbers. - * What is still fetched here is the inbox-message list, whose query is Home's - * own (top-`limit` titles, no read-state receipts). + * + * The inbox list was the last hold-out (#4225) and its own read carried the + * #4316 defect: it queried `sys_inbox_message` alone, never joining + * `sys_notification_receipt`, so it could not tell a read message from an + * unread one and listed the five most recent unconditionally. A user who had + * just read all nine in the bell came back to Home and found up to five of them + * still filed under "Needs your attention", badged — while the bell two hundred + * pixels above correctly showed zero. Read-state is not on the message row; + * ADR-0030 resolved decision 2 puts it in the receipt, per recipient×channel. + * + * Both surfaces now cut from the shared feed's already-joined superset: the + * bell lists all 20 and badges its unread topics, this card takes the UNREAD + * ones newest-first and caps them at `limit`. The two cannot disagree about a + * row's read-state because there is no second read left to drift. * * @module */ -import { useEffect, useRef, useState } from 'react'; -import { useAdapter } from '../providers/AdapterProvider'; -import { useAuth } from '@object-ui/auth'; -import { errorCodeIs } from '@object-ui/types'; -import { useHumanActivityFeed, useSharedPendingApprovalsCount } from './sharedUserFeeds'; +import { useMemo } from 'react'; +import { + useHumanActivityFeed, + useSharedInboxFeed, + useSharedPendingApprovalsCount, +} from './sharedUserFeeds'; import type { ActivityItem } from '../layout/ActivityFeed'; export interface HomeNotification { @@ -62,8 +75,12 @@ export interface HomeNotification { * - `error` — the read failed (denied, unreachable, malformed). The empty * array is the absence of an answer, not an empty inbox. * - * Same four words as `MetadataTypeStatus` (`providers/MetadataProvider`), on - * purpose: #4300 ruled one status dialect for this exact question. + * Same four words as `MetadataTypeStatus` (`providers/MetadataProvider`) and + * `SharedFeedStatus` (`sharedUserFeeds`), on purpose: #4300 ruled one status + * dialect for this exact question, and #4225 made the store speak it too + * rather than adding a second dialect beside it. This is now that store's + * status, passed through — a failed inbox read reaches this card as `error` + * instead of being swallowed into stale-but-`ready` data. */ export type HomeInboxStatus = 'idle' | 'loading' | 'ready' | 'error'; @@ -75,78 +92,39 @@ export interface HomeInboxData { activities: ActivityItem[]; } -/** - * A missing OBJECT, as opposed to a failed read of a present one. The - * ObjectStack client throws `httpStatus` (not `status`) with an error code — - * same predicate `sharedUserFeeds` and `AppHeader` apply to their own reads. - */ -function isMissingResource(err: unknown): boolean { - const e = err as { httpStatus?: number; status?: number } | null; - return e?.httpStatus === 404 || e?.status === 404 || errorCodeIs(err, 'OBJECT_NOT_FOUND'); -} - export function useHomeInbox(limit = 5): HomeInboxData { - const dataSource = useAdapter(); - const { user } = useAuth(); - const [notifications, setNotifications] = useState([]); - const [notificationsStatus, setNotificationsStatus] = useState('idle'); - const mountedRef = useRef(true); - - // Shared with the top-bar bell — one read each, not one per consumer (#4197). - // `useHumanActivityFeed` is Home's narrower cut of the bell's rows: real - // human actions only, dropping the sys_*/ai_* churn (actor "System"). + // Shared with the top-bar bell — one read each, not one per consumer + // (#4197, #4225). `useHumanActivityFeed` is Home's narrower cut of the bell's + // activity rows: real human actions only, dropping the sys_*/ai_* churn + // (actor "System"). `useSharedInboxFeed` is the bell's already-joined inbox + // superset, cut here to what is actually waiting on the user. const pendingApprovalsCount = useSharedPendingApprovalsCount(); const activities = useHumanActivityFeed(limit); + const { value: messages, status: notificationsStatus } = useSharedInboxFeed(); - useEffect(() => { - mountedRef.current = true; - return () => { mountedRef.current = false; }; - }, []); - - // Latest in-app inbox messages (assignments / @mentions / alerts). - useEffect(() => { - // Nothing asked yet — NOT an empty inbox. A console still settling its - // adapter or its session must not be reported as "all caught up". - if (!dataSource || !user?.id) { - setNotificationsStatus('idle'); - return; - } - let cancelled = false; - setNotificationsStatus('loading'); - Promise.resolve( - dataSource.find('sys_inbox_message', { - $filter: { user_id: user.id }, - $orderby: { created_at: 'desc' }, - $top: limit, - }) as Promise, - ) - .then((res) => { - if (cancelled || !mountedRef.current) return; - const rows: any[] = Array.isArray(res?.data) ? res.data : []; - const seenTitles = new Set(); - const deduped = rows - .filter((m) => m && (m.title ?? '').toString().trim()) - .map((m) => ({ - id: String(m.id), - title: String(m.title), - actionUrl: m.action_url ?? undefined, - createdAt: m.created_at ?? undefined, - })) - // Collapse repeated identical notifications (e.g. recurring digests) - // — keep the most recent of each title (rows are newest-first). - .filter((n) => (seenTitles.has(n.title) ? false : (seenTitles.add(n.title), true))); - setNotifications(deduped); - setNotificationsStatus('ready'); - }) - .catch((err: unknown) => { - if (cancelled || !mountedRef.current) return; - // A missing object is an answer: this deployment has no inbox pipeline, - // so nothing is waiting on the user and the empty state is honest. - // Every other failure — the objectstack#7344 denial included — is not. - setNotificationsStatus(isMissingResource(err) ? 'ready' : 'error'); - }); - return () => { cancelled = true; }; - }, [dataSource, user?.id, limit]); + const notifications = useMemo(() => { + const seenTitles = new Set(); + return messages + // #4316: "Needs your attention" means UNREAD. Read-state comes from the + // receipts join in the shared feed — the read this hook used to issue + // had none, so an already-read message was indistinguishable from a + // waiting one and got listed and badged all the same. + .filter((m) => !m.is_read) + .filter((m) => (m.title ?? '').trim().length > 0) + .map((m) => ({ + id: m.id, + title: String(m.title), + actionUrl: m.action_url ?? undefined, + createdAt: m.created_at ?? undefined, + })) + // Collapse repeated identical notifications (e.g. recurring digests) + // — keep the most recent of each title (rows are newest-first). + .filter((n) => (seenTitles.has(n.title) ? false : (seenTitles.add(n.title), true))) + // The cap is applied here rather than as the read's `$top`, because the + // read is the bell's now and holds its 20. Same shape as `activities`, + // which has sliced the shared feed at its own call site since #4197. + .slice(0, limit); + }, [messages, limit]); return { pendingApprovalsCount, notifications, notificationsStatus, activities }; } diff --git a/packages/app-shell/src/layout/AppHeader.tsx b/packages/app-shell/src/layout/AppHeader.tsx index c745b89db..716275d46 100644 --- a/packages/app-shell/src/layout/AppHeader.tsx +++ b/packages/app-shell/src/layout/AppHeader.tsx @@ -50,7 +50,7 @@ import { Hammer, } from 'lucide-react'; -import { useState, useEffect, useCallback, useRef } from 'react'; +import { useState, useEffect, useCallback, useMemo } from 'react'; import { useOffline } from '@object-ui/react'; import { PresenceAvatars, useTenantPresence, type PresenceUser } from '@object-ui/collaboration'; import { ModeToggle } from './ModeToggle'; @@ -75,11 +75,14 @@ import { useCommandPalette } from '../context/CommandPaletteProvider'; import { useUrlOverlay } from '../hooks/useUrlOverlay'; import { KEYBOARD_SHORTCUTS_PARAM, RECORD_TRAIL_PARAM, decodeRecordTrail, buildRecordTrailHref } from '../urlParams'; import { useAiSurfaceEnabled } from '../hooks/useAiSurface'; -import { useSharedActivityFeed, useSharedPendingApprovalsCount } from '../hooks/sharedUserFeeds'; +import { + useSharedActivityFeed, + useSharedInboxFeed, + useSharedPendingApprovalsCount, +} from '../hooks/sharedUserFeeds'; import { getProductName, getLogoUrl } from '../runtime-config'; import { LocalizedSidebarTrigger } from './LocalizedSidebarTrigger'; import { PreviewBadge } from './PreviewBadge'; -import { errorCodeIs } from '@object-ui/types'; function humanizeSlug(slug: string): string { return slug.replace(/[-_]/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase()); @@ -99,6 +102,10 @@ function PathSep() { // header doesn't ship phantom collaborators in production. const EMPTY_PRESENCE_USERS: PresenceUser[] = []; +// Same stable-reference rule, for the optimistic mark-read overlay: a fresh +// empty Set per render would re-run every memo that depends on it. +const EMPTY_READ_IDS: ReadonlySet = new Set(); + export type AppHeaderVariant = 'app' | 'home' | 'orgs'; export interface AppHeaderProps { @@ -206,55 +213,39 @@ export function AppHeader({ */ const apiActivities = useSharedActivityFeed(); /** - * In-header notifications (ADR-0030). Polled from `sys_inbox_message` (the L5 - * in-app materialization, `mine` scope) joined with `sys_notification_receipt` - * for read-state — the bell no longer reads the re-modeled `sys_notification` - * L2 event (which carries no recipient/read columns). + * In-header notifications (ADR-0030), from the shared user feed (#4225). + * + * The rows are `sys_inbox_message` (the L5 in-app materialization, `mine` + * scope) joined with `sys_notification_receipt` for read-state — the bell + * does not read the re-modeled `sys_notification` L2 event (which carries no + * recipient/read columns). That query, its 10s cadence, its hidden-tab + * throttle, its visibility refetch and its failure backoff all moved into + * `sharedUserFeeds` unchanged; what was lost is only the SECOND copy of it. + * + * Home's action centre reads the same feed, so the two surfaces can no + * longer disagree about whether a message is read — the #4316 defect, where + * this bell showed zero unread while the card below listed five already-read + * messages as needing attention, has no representable state to occur in. */ - const [notifications, setNotifications] = useState>([]); - // Once the server returns 404 for these collections we stop retrying for - // the lifetime of the page — they're optional features and re-requesting - // on every navigation creates console noise + wasted round trips. - // (`sys_activity` and the approvals endpoint carry the same rule inside - // `sharedUserFeeds`, which retires a feed for every consumer at once.) - const notificationsUnavailableRef = useRef(false); + const { value: inboxMessages } = useSharedInboxFeed(); - // Tracks whether the component is still mounted. Used by the pollers to - // decide whether to apply an in-flight fetch's result, independent of any - // single effect run's `cancelled` flag — so a fetch that outlives the - // effect run that started it (because deps settled mid-flight during - // bootstrap) still populates state instead of being silently dropped. - const mountedRef = useRef(true); - useEffect(() => { - // Reset on (re)mount too, so StrictMode's mount→cleanup→mount cycle - // doesn't leave it latched false and silence the pollers. - mountedRef.current = true; - return () => { mountedRef.current = false; }; - }, []); - - // In-flight guard: during bootstrap the poller effect re-runs several times - // as `dataSource` / `user.id` settle, and each run kicks an immediate fetch. - // Without it the same query fired 5× concurrently (nothing cached yet) and - // flooded the backend. It coalesces them to one. (The approvals and activity - // feeds carry the equivalent guard inside `sharedUserFeeds`, where it also - // collapses the *other* consumer's mount, not just this one's re-runs.) - const notifInFlightRef = useRef(false); + /** + * Optimistic read-state, layered over the shared rows. + * + * Mark-read used to mutate this component's own `notifications` state; the + * rows are shared now, so a consumer may not write to them — one surface's + * optimistic flip must not become another's fact before the server agrees. + * Holding the flipped ids locally keeps the click instant while the next + * poll (which reads the persisted receipt) supersedes it. + */ + const [locallyRead, setLocallyRead] = useState>(EMPTY_READ_IDS); + const notifications = useMemo( + () => + locallyRead.size === 0 + ? inboxMessages + : inboxMessages.map((n) => (locallyRead.has(n.id) ? { ...n, is_read: true } : n)), + [inboxMessages, locallyRead], + ); /** * M11.C15: pending approvals count — the topbar shortcut, and the second @@ -280,134 +271,19 @@ export function AppHeader({ */ /** - * Poll the signed-in user's in-app inbox (ADR-0030 L5). - * - * Two scoped reads, joined client-side: - * - `sys_inbox_message` filtered by `user_id` (the `mine` materialization), - * 20 most-recent — the notification rows themselves. - * - `sys_notification_receipt` filtered by `user_id` + `channel:'inbox'` — - * the read-state spine. A message is unread until its event has a - * `read`/`clicked`/`dismissed` receipt; the unread count drives the badge. - * - * - Adaptive interval: 10s while the tab is foregrounded so the bell reflects - * mentions / assignments within seconds without a server-push transport. - * - Immediate refetch on `visibilitychange` when the user returns to the tab. - * - On transient errors, exponential backoff (cap 2 min), reset on success. - * - Tolerates 404 so deployments without the messaging pipeline degrade - * silently. - * - * Full server-push (SSE / WebSocket) is tracked separately; this adaptive - * poll keeps perceived latency ~5s and is sufficient for pilots up to ~50 - * concurrent users. - * - * ⚠️ Deliberately NOT gated on `isApp` (#4110). The bell renders in every - * header variant, and its inbox is scoped to the *user*, not to the app in - * the URL — unlike the presence avatars and the connection dot, which are - * app-shell chrome and are the reason that flag exists. While this poll was - * gated the popover held `[]` on Home / Organizations / the full-page AI + * ⚠️ The bell's inbox is deliberately NOT gated on `isApp` (#4110), and the + * shared feed keeps it that way: the read is scoped to the *user*, not to the + * app in the URL — unlike the presence avatars and the connection dot, which + * are app-shell chrome and are the reason that flag exists. While this poll + * was gated the popover held `[]` on Home / Organizations / the full-page AI * screen forever: the "Unread" sub-filter read "You're all caught up" and * "All" — which applies no predicate at all — read "No notifications", on the - * very page whose To-do card (`useHomeInbox`, ungated) was listing the same - * `sys_inbox_message` row. Scope the read by `user?.id` only. + * very page whose To-do card was listing the same `sys_inbox_message` row. + * + * Full server-push (SSE / WebSocket) is tracked separately; the shared feed's + * adaptive poll keeps perceived latency ~5s and is sufficient for pilots up + * to ~50 concurrent users. */ - useEffect(() => { - if (!dataSource || !user?.id) return; - if (notificationsUnavailableRef.current) return; - let cancelled = false; - let timer: ReturnType | null = null; - const ACTIVE_INTERVAL_MS = 10_000; - const HIDDEN_INTERVAL_MS = 60_000; - const MAX_BACKOFF_MS = 120_000; - let backoffMs = ACTIVE_INTERVAL_MS; - const isMissingResource = (err: any): boolean => - err?.httpStatus === 404 || err?.status === 404 || errorCodeIs(err, 'OBJECT_NOT_FOUND'); - const READ_STATES = new Set(['read', 'clicked', 'dismissed']); - const fetchOnce = async () => { - if (notifInFlightRef.current) return; - notifInFlightRef.current = true; - try { - const [inboxRes, receiptRes] = await Promise.all([ - dataSource.find('sys_inbox_message', { - $filter: { user_id: user.id }, - $orderby: { created_at: 'desc' }, - $top: 20, - }) as Promise, - // Read-state spine. Best-effort: if receipts are unavailable the - // inbox still renders (everything shows unread) rather than erroring. - (dataSource.find('sys_notification_receipt', { - $filter: { user_id: user.id, channel: 'inbox' }, - $top: 200, - }) as Promise).catch(() => ({ data: [] })), - ]); - if (!mountedRef.current) return; - const rows: any[] = Array.isArray(inboxRes?.data) ? inboxRes.data : []; - const receipts: any[] = Array.isArray(receiptRes?.data) ? receiptRes.data : []; - // notification_id → { id, state } (most-advanced receipt wins). - const receiptByNotif = new Map(); - for (const r of receipts) { - const nid = r?.notification_id != null ? String(r.notification_id) : ''; - if (!nid) continue; - const prev = receiptByNotif.get(nid); - // Prefer a read/clicked/dismissed receipt over a plain delivered one. - if (!prev || (!READ_STATES.has(prev.state) && READ_STATES.has(r.state))) { - receiptByNotif.set(nid, { id: String(r.id), state: String(r.state) }); - } - } - const merged = rows.map((m) => { - const nid = m?.notification_id != null ? String(m.notification_id) : null; - const rec = nid ? receiptByNotif.get(nid) : undefined; - return { - id: String(m.id), - notification_id: nid, - receipt_id: rec?.id ?? null, - type: m.topic ?? 'notification', - title: m.title ?? '', - body: m.body_md ?? null, - action_url: m.action_url ?? null, - is_read: rec ? READ_STATES.has(rec.state) : false, - created_at: m.created_at, - }; - }); - setNotifications(merged); - backoffMs = ACTIVE_INTERVAL_MS; - } catch (err: any) { - if (isMissingResource(err)) { - notificationsUnavailableRef.current = true; - return; - } - backoffMs = Math.min(backoffMs * 2, MAX_BACKOFF_MS); - } finally { - notifInFlightRef.current = false; - } - }; - const scheduleNext = () => { - if (cancelled || notificationsUnavailableRef.current) return; - const hidden = typeof document !== 'undefined' && document.hidden; - const delay = hidden ? HIDDEN_INTERVAL_MS : backoffMs; - timer = setTimeout(async () => { - await fetchOnce(); - scheduleNext(); - }, delay); - }; - const onVisibilityChange = () => { - if (cancelled) return; - if (typeof document === 'undefined' || document.hidden) return; - if (timer) { clearTimeout(timer); timer = null; } - backoffMs = ACTIVE_INTERVAL_MS; - fetchOnce().finally(scheduleNext); - }; - fetchOnce().finally(scheduleNext); - if (typeof document !== 'undefined') { - document.addEventListener('visibilitychange', onVisibilityChange); - } - return () => { - cancelled = true; - if (timer) clearTimeout(timer); - if (typeof document !== 'undefined') { - document.removeEventListener('visibilitychange', onVisibilityChange); - } - }; - }, [dataSource, user?.id]); const unreadCount = notifications.reduce((n, x) => n + (x.is_read ? 0 : 1), 0); @@ -432,19 +308,29 @@ export function AppHeader({ }); }, []); + /** Flip rows read in the local overlay — never in the shared feed's rows. */ + const markLocallyRead = useCallback((ids: readonly string[]) => { + if (ids.length === 0) return; + setLocallyRead((prev) => { + const next = new Set(prev); + for (const id of ids) next.add(id); + return next; + }); + }, []); + const markNotificationRead = useCallback(async (id: string) => { const target = notifications.find(n => n.id === id); - setNotifications(prev => prev.map(n => n.id === id ? { ...n, is_read: true } : n)); + markLocallyRead([id]); if (!target?.notification_id) return; try { await postMarkRead('read', [target.notification_id]); } catch { /* best-effort */ } - }, [notifications, postMarkRead]); + }, [notifications, markLocallyRead, postMarkRead]); const markAllRead = useCallback(async () => { const unread = notifications.filter(n => !n.is_read); if (!unread.length) return; - setNotifications(prev => prev.map(n => ({ ...n, is_read: true }))); + markLocallyRead(notifications.map(n => n.id)); try { await postMarkRead('read/all'); } catch { /* best-effort */ } - }, [notifications, postMarkRead]); + }, [notifications, markLocallyRead, postMarkRead]); // Per-group "mark all of this type read" (#2765): the inbox coalesces // repeats of the same (topic, title) into one expandable row, and this marks @@ -457,10 +343,10 @@ export function AppHeader({ .filter(n => idSet.has(n.id) && !n.is_read) .map(n => n.notification_id) .filter((v): v is string => !!v); - setNotifications(prev => prev.map(n => idSet.has(n.id) ? { ...n, is_read: true } : n)); + markLocallyRead(ids); if (!notifIds.length) return; try { await postMarkRead('read', notifIds); } catch { /* best-effort */ } - }, [notifications, postMarkRead]); + }, [notifications, markLocallyRead, postMarkRead]); const tenantPresence = useTenantPresence(); const activeUsers = presenceUsers ?? (tenantPresence.length > 0 ? tenantPresence : EMPTY_PRESENCE_USERS);