From 35387d09f63f2be0b56eded5547571ffb814bd00 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 20:21:05 +0000 Subject: [PATCH] fix(app-shell): the bell polls the inbox on every console surface, not only inside an app (#4110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The top-bar bell's `sys_inbox_message` + `sys_notification_receipt` poll was gated on the header's `isApp` variant flag — the flag that exists to hide the app-only presence avatars and connection dot. The bell itself renders in every variant, so on Home, Organizations and the full-page AI screen its Notifications tab held `[]` forever: "Unread" 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 row from the same object. The popover's own filter and its (topic, title) coalescing (#2765) are innocent: given the row they render it. The inbox is scoped to the signed-in user, not to the app in the URL, so the poll is now scoped by `user?.id` only. Regression test renders the header against a fake adapter holding one canonical MessagingService-emitted row and asserts the bell lists it in every variant; the `variant="app"` case is the control that never broke. --- .../inbox-popover-bell-polls-off-app-4110.md | 14 + packages/app-shell/src/layout/AppHeader.tsx | 14 +- .../__tests__/AppHeader.inboxVariant.test.tsx | 263 ++++++++++++++++++ 3 files changed, 289 insertions(+), 2 deletions(-) create mode 100644 .changeset/inbox-popover-bell-polls-off-app-4110.md create mode 100644 packages/app-shell/src/layout/__tests__/AppHeader.inboxVariant.test.tsx diff --git a/.changeset/inbox-popover-bell-polls-off-app-4110.md b/.changeset/inbox-popover-bell-polls-off-app-4110.md new file mode 100644 index 0000000000..63ba2e4a74 --- /dev/null +++ b/.changeset/inbox-popover-bell-polls-off-app-4110.md @@ -0,0 +1,14 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(app-shell): the top-bar bell polls the inbox on every console surface, not only inside an app (#4110) + +The bell's `sys_inbox_message` + `sys_notification_receipt` poll was gated on the +header's `isApp` variant flag — the flag that exists to hide the app-only presence +avatars and connection dot. The bell itself renders in every variant, so on Home, +Organizations and the full-page AI screen its Notifications tab held `[]` forever: +"Unread" read "You're all caught up" and "All" — which applies no predicate at all — +read "No notifications", while Home's own To-do card listed the same row from the +same object. The inbox is scoped to the signed-in user, not to the app in the URL, +so the poll is now scoped by `user?.id` only. diff --git a/packages/app-shell/src/layout/AppHeader.tsx b/packages/app-shell/src/layout/AppHeader.tsx index 82fb3cd7ad..96e47e5f6d 100644 --- a/packages/app-shell/src/layout/AppHeader.tsx +++ b/packages/app-shell/src/layout/AppHeader.tsx @@ -333,9 +333,19 @@ export function AppHeader({ * 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 + * 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. */ useEffect(() => { - if (!dataSource || !isApp || !user?.id) return; + if (!dataSource || !user?.id) return; if (notificationsUnavailableRef.current) return; let cancelled = false; let timer: ReturnType | null = null; @@ -431,7 +441,7 @@ export function AppHeader({ document.removeEventListener('visibilitychange', onVisibilityChange); } }; - }, [dataSource, isApp, user?.id]); + }, [dataSource, user?.id]); /** * M11.C15: poll pending-approvals count for the topbar shortcut badge. diff --git a/packages/app-shell/src/layout/__tests__/AppHeader.inboxVariant.test.tsx b/packages/app-shell/src/layout/__tests__/AppHeader.inboxVariant.test.tsx new file mode 100644 index 0000000000..0e3a3aa2fe --- /dev/null +++ b/packages/app-shell/src/layout/__tests__/AppHeader.inboxVariant.test.tsx @@ -0,0 +1,263 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * #4110 — the top-bar bell's Notifications tab was permanently empty on every + * console surface that is not an app page (Home, Organizations, the full-page + * AI screen). The bell renders in all of those variants, but the poller that + * fills it (`sys_inbox_message` + `sys_notification_receipt`, ADR-0030 L5 / + * #1429) was gated on `isApp` — the same flag that hides the app-only presence + * avatars and connection dot. So on Home the popover held `[]` forever: the + * "Unread" sub-filter showed "You're all caught up" and "All" — which filters + * nothing at all — showed "No notifications", while Home's own To-do card + * (`useHomeInbox`, ungated) listed the very same row from the very same object. + * + * These render the header against a fake adapter holding one canonical + * MessagingService-emitted inbox row and assert the bell lists it in EVERY + * variant. The `variant="app"` case is the control: it passed before the fix + * too, and is what made the defect look like a popover-side filter bug. + */ +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; + +// ── Chrome the header pulls in but that this test does not exercise ────────── + +vi.mock('react-router-dom', () => ({ + useLocation: () => ({ pathname: '/', search: '', hash: '', state: null, key: 'test' }), + useParams: () => ({}), + useNavigate: () => vi.fn(), + // Reached through `useUrlOverlay` (the ⌘K / shortcuts overlays). + useSearchParams: () => [new URLSearchParams(), vi.fn()] as const, + Link: ({ children, to, ...p }: any) => {children}, +})); + +// Interpolating stub — the real packs carry `{{unread}}` / `{{total}}` holes, +// so returning `defaultValue` verbatim would make the copy assertions vacuous. +// `formatRelativeTime` is reached through `utils/relativeTime` for every row, +// so keep the rest of the module real. +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, + }), +})); + +// Passthrough primitives: the popover body renders inline instead of driving +// Radix open/close in jsdom (i.e. "as if the user clicked the bell"). Menus +// that Radix would keep unmounted while closed render as nothing. +vi.mock('@object-ui/components', () => { + const Pass = ({ children, ...p }: any) =>
{children}
; + const stripProps = (p: any) => { + const { asChild, variant, size, align, sideOffset, ...rest } = p ?? {}; + return rest; + }; + 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(' '), + }; +}); + +// Every lucide glyph renders as an inert span — the header imports 14 of them +// and the popover 4 more, and the set churns. +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; + }, + // Vitest validates that each imported name exists on the mock, so the + // proxy has to claim every glyph name, not just answer for it. + has: (_target, prop) => prop !== 'then', + }, + ); +}); + +vi.mock('@object-ui/react', async (importOriginal) => ({ + ...(await importOriginal>()), + useOffline: () => ({ isOnline: true }), +})); + +vi.mock('@object-ui/collaboration', () => ({ + PresenceAvatars: () => null, + useTenantPresence: () => [], +})); + +vi.mock('../ModeToggle', () => ({ ModeToggle: () => null })); +vi.mock('../WorkspaceSwitcher', () => ({ WorkspaceSwitcher: () => null })); +vi.mock('../LocaleSwitcher', () => ({ LocaleSwitcher: () => null })); +vi.mock('../ConnectionStatus', () => ({ ConnectionStatus: () => null })); +vi.mock('../AppSwitcher', () => ({ AppSwitcher: () => null })); +vi.mock('../LocalizedSidebarTrigger', () => ({ LocalizedSidebarTrigger: () => null })); +vi.mock('../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 inbox fixture: exactly what MessagingService materializes ──────────── + +/** + * One `sys_inbox_message` row as the L5 materialization writes it for an emit + * of `{topic, audience:[userId], payload:{title, body, url}, dedupKey, source}` + * — the shape #4110 reports as invisible in the bell. + */ +const INBOX_ROW = { + id: 'ibx_1', + user_id: 'u1', + notification_id: 'ntf_1', + topic: 'hr.contract.expiring', + title: 'Contract expiring: Zhang San', + body_md: "Zhang San's labour contract expires in 30 days.", + action_url: '/apps/ehr/hr_contract/record/c_1', + created_at: '2026-08-10T09:00:00Z', +}; + +/** `state: 'delivered'` — delivered is NOT read, so the row must show unread. */ +const DELIVERED_RECEIPT = { + id: 'rcp_1', + notification_id: 'ntf_1', + user_id: 'u1', + channel: 'inbox', + state: 'delivered', +}; + +const finds: Array<{ object: string; query: unknown }> = []; +/** What the fake `sys_inbox_message` collection holds for the current test. */ +let inboxRows: Array> = []; + +const fakeAdapter = { + find: (object: string, query: unknown) => { + finds.push({ object, query }); + if (object === 'sys_inbox_message') return Promise.resolve({ data: inboxRows }); + if (object === 'sys_notification_receipt') { + return Promise.resolve({ data: inboxRows.length ? [DELIVERED_RECEIPT] : [] }); + } + return Promise.resolve({ data: [] }); + }, + getClient: () => undefined, +}; + +vi.mock('../../providers/AdapterProvider', () => ({ + useAdapter: () => fakeAdapter, +})); + +import { AppHeader } from '../AppHeader'; + +beforeEach(() => { + finds.length = 0; + inboxRows = [INBOX_ROW]; + // The approvals count + auth-config reads are not under test; keep them from + // reaching the network (both are already soft-degrading). + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response('{}', { status: 404 }))); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.useRealTimers(); +}); + +/** Objects the bell's poller reads, in the order it issued them. */ +const inboxReads = () => finds.filter((f) => f.object === 'sys_inbox_message'); + +describe('AppHeader — the bell polls the inbox in every variant (#4110)', () => { + it('lists a canonical inbox row on Home, where the To-do card already shows it', async () => { + render(); + + await waitFor(() => expect(inboxReads().length).toBeGreaterThan(0)); + expect(await screen.findByText('Contract expiring: Zhang San')).toBeInTheDocument(); + // …and the empty states are gone: "Unread" is the default sub-filter, and + // "All" — which applies no predicate — must never be empty while a row is + // in hand. + expect(screen.queryByText("You're all caught up")).not.toBeInTheDocument(); + expect(screen.queryByText('No notifications')).not.toBeInTheDocument(); + }); + + it('scopes that read to the signed-in user, newest first (ADR-0030 `mine`)', async () => { + render(); + + await waitFor(() => expect(inboxReads().length).toBeGreaterThan(0)); + expect(inboxReads()[0].query).toMatchObject({ + $filter: { user_id: 'u1' }, + $orderby: { created_at: 'desc' }, + }); + }); + + it('counts a `delivered` receipt as unread, so the badge shows the row', async () => { + render(); + + expect(await screen.findByTestId('inbox-bell-badge')).toHaveTextContent('1'); + }); + + it('lists it on the Organizations variant too', async () => { + render(); + + expect(await screen.findByText('Contract expiring: Zhang San')).toBeInTheDocument(); + }); + + it('still lists it inside an app (the control — this half never broke)', async () => { + render(); + + expect(await screen.findByText('Contract expiring: Zhang San')).toBeInTheDocument(); + }); + + it('shows the empty state only when the inbox is genuinely empty', async () => { + inboxRows = []; + render(); + + // The read still happens — "empty" is now an answer, not an unasked question. + await waitFor(() => expect(inboxReads().length).toBeGreaterThan(0)); + expect(await screen.findByText("You're all caught up")).toBeInTheDocument(); + expect(screen.queryByTestId('inbox-bell-badge')).not.toBeInTheDocument(); + }); +});