diff --git a/.changeset/27447-home-notification-marks-read.md b/.changeset/27447-home-notification-marks-read.md new file mode 100644 index 00000000000..27a08251d41 --- /dev/null +++ b/.changeset/27447-home-notification-marks-read.md @@ -0,0 +1,13 @@ +--- +'@object-ui/app-shell': patch +'@object-ui/console': patch +--- + +fix(app-shell): opening a notification from Home's "My work → Notifications" card marks it read, as the bell does + +Clause-②: no +Metadata-protocol: none + +Clicking a notification in the top-bar bell marked it read and then opened its link. Clicking the same notification on the Home page's "My work → Notifications" card only opened the link: no `POST /api/v1/notifications/read` was sent, the server's `unreadCount` did not change, and the bell kept its badge after a reload. + +Both places now use one open action, `openInboxNotification`. It marks the notification read first, then opens its link, or opens the inbox when the notification has no link. The Home card marks read through the bell's own write (`useInboxBell().markRead`), so it sends the same request, keyed by the notification's event id. Once the inbox feed re-reads, the Home card's unread count and the bell's badge drop. diff --git a/ui/packages/app-shell/src/console/home/HomePage.tsx b/ui/packages/app-shell/src/console/home/HomePage.tsx index 231e4b759b1..062c332fc67 100644 --- a/ui/packages/app-shell/src/console/home/HomePage.tsx +++ b/ui/packages/app-shell/src/console/home/HomePage.tsx @@ -46,8 +46,9 @@ import { matchAppBySegment, resolveHostAppSegment, resolveKeyedI18nLabel, - resolveNotificationTarget, } from '../../utils/index.js'; +import { useInboxBell } from '../../hooks/useInboxBell.js'; +import { openInboxNotification } from '../../layout/openInboxNotification.js'; import { resolveSetupAppPath } from '../ConsoleShell.js'; import { Empty, @@ -369,6 +370,11 @@ export function HomePage() { notificationsReadable, activity, } = useHomeInbox(); + // Opening a notification from the My-work card marks it read through the + // bell's own write — `useInboxBell` is the one wiring of mark-read onto the + // shared inbox feed (the header bell and the `global:notifications` block + // mount it too), so the request Home sends is the request the bell sends. + const { markRead: markNotificationRead } = useInboxBell(); // The drafts row of the My-work section — `null` with nothing to publish. const draftsRow = usePendingDraftsRow(t, language); // Home renders OUTSIDE the `/apps/:appName/*` router, so there is no @@ -833,23 +839,20 @@ export function HomePage() { so it has to be hosted under an app before it is a route — navigating it verbatim is objectui#5179, where the console catch-all forwarded the unmatched path to `/` and the landing - resolver dropped the user on the default app's home with the - notification already marked read. `resolveNotificationTarget` - is shared with the top-bar bell so the two cannot answer this - differently. The fallback arm runs whenever a notification - carries no link at all, and opens the inbox. */ - onOpenNotification={(n) => { - const target = resolveNotificationTarget(n.actionUrl, hostAppSegment); - if (!target) { - navigate(`/apps/${hostAppSegment}/sys_inbox_message?view=mine`); - return; - } - if (target.kind === 'external') { - window.open(target.url, '_blank', 'noopener,noreferrer'); - return; - } - navigate(target.path); - }} + resolver dropped the user on the default app's home. + `openInboxNotification` is the top-bar bell's own open action, + so the two doors cannot answer differently: it marks the + notification read first (this row used to skip that, and a + notification opened from Home stayed unread on the server and + badged on the bell), then follows the resolved link — or, when + the notification carries no link at all, opens the inbox. */ + onOpenNotification={(n) => + openInboxNotification(n, hostAppSegment, { + markRead: markNotificationRead, + goTo: (path) => navigate(path), + openInbox: () => navigate(`/apps/${hostAppSegment}/sys_inbox_message?view=mine`), + }) + } t={t} /> diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.aiStudioDisabled.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.aiStudioDisabled.test.tsx index 421f36c108a..3046114c64d 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.aiStudioDisabled.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.aiStudioDisabled.test.tsx @@ -128,6 +128,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ })); import { initRuntimeConfig, resetRuntimeConfigForTesting } from '../../../runtime-config'; +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /** diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx index 103e26eb35d..ecdc4c01614 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx @@ -131,6 +131,10 @@ vi.mock('../../../runtime-config', () => ({ isAiStudioEnabled: () => true, })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /* ── The `_drafts` double (objectui#7307) ───────────────────────────────────── diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.authoringCapabilityGate.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.authoringCapabilityGate.test.tsx index 3a205f826eb..ac04b32b674 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.authoringCapabilityGate.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.authoringCapabilityGate.test.tsx @@ -140,6 +140,10 @@ vi.mock('../../../runtime-config', () => ({ isAiStudioEnabled: () => true, })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /* ── The `_drafts` double (objectui#7307) ───────────────────────────────────── diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.draftsReadGate-25894.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.draftsReadGate-25894.test.tsx index 66e07efb403..734bfcdd4d1 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.draftsReadGate-25894.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.draftsReadGate-25894.test.tsx @@ -86,6 +86,10 @@ vi.mock('../../../runtime-config', () => ({ isAiStudioEnabled: () => true, })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; const DRAFTS_ROUTE = '/api/v1/meta/_drafts'; diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingAndApps-12601.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingAndApps-12601.test.tsx index 4929f9f966f..f35199cdfa6 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingAndApps-12601.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingAndApps-12601.test.tsx @@ -147,6 +147,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ usePublishAllDrafts: () => ({ publishAll: async () => ({ ok: true }), publishing: false }), })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; const CRM = { name: 'crm', label: 'CRM', icon: 'Building2' }; diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingLocale-11689.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingLocale-11689.test.tsx index bd5f9b2a650..3b864bd1d19 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingLocale-11689.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.greetingLocale-11689.test.tsx @@ -100,6 +100,10 @@ vi.mock('../../../runtime-config', () => ({ isAiStudioEnabled: () => true, })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /* ── The `_drafts` double ───────────────────────────────────────────────────── diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx index ff16e1a9e7a..014adca8a49 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx @@ -134,6 +134,10 @@ vi.mock('../../../runtime-config', () => ({ isAiStudioEnabled: () => true, })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /* ── The `_drafts` double (objectui#7307) ───────────────────────────────────── diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.launcherHiddenApps-26838.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.launcherHiddenApps-26838.test.tsx index 3c23c9f11b8..13b9306abdd 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.launcherHiddenApps-26838.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.launcherHiddenApps-26838.test.tsx @@ -133,6 +133,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ usePublishAllDrafts: () => ({ publishAll: async () => ({ ok: true }), publishing: false }), })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; import { LauncherAppsProvider } from '../../../hooks/useLauncherApps'; diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.marketplaceDisabled.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.marketplaceDisabled.test.tsx index 9708c6f94a5..c10b69a3fc2 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.marketplaceDisabled.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.marketplaceDisabled.test.tsx @@ -107,6 +107,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ })); import { initRuntimeConfig, resetRuntimeConfigForTesting } from '../../../runtime-config'; +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /** A `GET /api/v1/runtime/config` answer, as the server sends it. */ diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.myWork-12602.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.myWork-12602.test.tsx index 6d56b21c618..4122ff11775 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.myWork-12602.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.myWork-12602.test.tsx @@ -145,6 +145,10 @@ vi.mock('../../../views/studio-design/packages-io', async (importOriginal) => ({ fetchPackages: () => (state.packages ? Promise.resolve(state.packages) : new Promise(() => {})), })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; const CRM = { name: 'crm', label: 'CRM', icon: 'Building2' }; diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.noAgentsBuildChip-26614.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.noAgentsBuildChip-26614.test.tsx index 17e61371262..bad6d913caa 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.noAgentsBuildChip-26614.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.noAgentsBuildChip-26614.test.tsx @@ -134,6 +134,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ usePublishAllDrafts: () => ({ publishAll: async () => ({ ok: true }), publishing: false }), })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; import { STUDIO_NEW_PACKAGE_PATH } from '../../../views/studio-design/studioNewPackage'; diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.notificationDeepLink.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.notificationDeepLink.test.tsx index ba4e697e12f..fe26877e842 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.notificationDeepLink.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.notificationDeepLink.test.tsx @@ -130,6 +130,10 @@ vi.mock('../../../runtime-config', () => ({ isAiStudioEnabled: () => true, })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; /* ── The `_drafts` double (objectui#7307) ───────────────────────────────────── diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.openMarksRead.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.openMarksRead.test.tsx new file mode 100644 index 00000000000..f74862b6a91 --- /dev/null +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.openMarksRead.test.tsx @@ -0,0 +1,260 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Opening a notification from Home's "My work → Notifications" card marks it + * read, exactly as opening it from the top-bar bell does. + * + * ## The defect + * + * The bell's row click marked the message read and then followed its link. + * Home's row click only followed the link. Nothing else ever marks a message + * read, so a notification opened from Home stayed unread on the server: no + * `POST /api/v1/notifications/read` went out, `unreadCount` did not move, and + * the bell kept its badge after a reload. + * + * Both doors now call `openInboxNotification`, and Home hands it the bell's own + * write (`useInboxBell().markRead`) — so the request below is the bell's + * request by construction, not a second copy of it. + * + * ## What this drives + * + * The REAL `HomePage`, the real `useHomeInbox` and `useInboxBell`, and the real + * shared inbox feed, over one fake data adapter (the inbox rows and their + * receipts) and one recording `fetch` double that plays the server: a + * `POST /api/v1/notifications/read` writes `read` receipts for the ids it + * names, so the next feed read answers with them. What is asserted is the + * request on the wire — method, path and body — its order against the + * navigation, and the card's unread count after the feed re-reads. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import React from 'react'; +import { act, cleanup, render, screen, waitFor, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +/** Every observable step, in the order it happened: requests and navigations. */ +let timeline: string[] = []; + +const navigateMock = vi.fn((to: string) => { + timeline.push(`navigate ${to}`); +}); + +vi.mock('react-router-dom', async () => { + const { createElement } = await import('react'); + return { + useNavigate: () => navigateMock, + // The bell wiring's arrival announcer reads the route's app segment. + useParams: () => ({}), + Link: ({ to, ...rest }: { to: string } & Record) => + createElement('a', { ...rest, href: to }), + }; +}); + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => ({ + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + language: 'en', + }), +})); + +vi.mock('@object-ui/auth', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ user: { id: 'u1', name: 'Ada', email: 'ada@example.com' } }), + useWorkspaceAdminStatus: () => ({ isAdmin: false, isResolved: true }), +})); + +vi.mock('@object-ui/plugin-chatbot/core', async (importOriginal) => ({ + ...(await importOriginal()), + useAgents: () => ({ agents: [] }), + isAskAgent: () => false, + agentHasCapability: () => false, +})); + +vi.mock('../../../providers/MetadataProvider', () => ({ + useMetadata: () => ({ apps: [{ name: 'crm', label: 'crm' }], loading: false }), +})); + +vi.mock('../../../context/NavigationContext', () => ({ + useNavigationContext: () => ({ currentAppName: 'crm' }), +})); + +vi.mock('../../../hooks/useRecentItems', () => ({ useRecentItems: () => ({ recentItems: [] }) })); +vi.mock('../../../hooks/useFavorites', () => ({ useFavorites: () => ({ favorites: [] }) })); +vi.mock('../../../hooks/useAiSurface', () => ({ resolveAiApiBase: () => '' })); +vi.mock('../../../views/metadata-admin/useMetadata', () => ({ + useMetadataClient: () => ({ listDrafts: async () => [] }), +})); +vi.mock('../../../preview/usePublishAllDrafts', () => ({ + usePublishAllDrafts: () => ({ publishAll: async () => ({ ok: true }), publishing: false }), +})); +vi.mock('../../../runtime-config', () => ({ + getRuntimeConfig: () => ({ branding: { productName: 'ObjectStack' } }), + isMarketplaceEnabled: () => true, + isAiStudioEnabled: () => true, +})); + +// ── The server's side of the inbox: messages, and receipts keyed by event id ── + +/** Two unread messages, so the count has somewhere to drop to. */ +const INBOX = [ + { + id: 'ibx_1', + user_id: 'u1', + notification_id: 'ntf_1', + topic: 'task.assigned', + title: 'Task T-1 was assigned to you', + action_url: '/showcase_task/t_1', + created_at: '2026-10-29T09:00:00Z', + }, + { + id: 'ibx_2', + user_id: 'u1', + notification_id: 'ntf_2', + topic: 'task.mention', + title: 'You were mentioned in Task T-2', + action_url: '/showcase_task/t_2', + created_at: '2026-10-29T08:00:00Z', + }, +]; + +/** Event ids the server holds a `read` receipt for. */ +let readIds = new Set(); + +const fakeAdapter = { + find: (object: string) => { + if (object === 'sys_inbox_message') return Promise.resolve({ data: INBOX }); + if (object === 'sys_notification_receipt') { + return Promise.resolve({ + data: INBOX.map((r, i) => ({ + id: `rcp_${i + 1}`, + notification_id: r.notification_id, + user_id: 'u1', + channel: 'inbox', + state: readIds.has(r.notification_id) ? 'read' : 'delivered', + })), + }); + } + return Promise.resolve({ data: [] }); + }, + getClient: () => undefined, +}; + +vi.mock('../../../providers/AdapterProvider', () => ({ useAdapter: () => fakeAdapter })); + +/** The bodies of every mark-read request, in order. */ +let markReadBodies: unknown[] = []; +/** Every URL the global `fetch` was handed that this double does not serve. */ +let unserved: string[] = []; + +const routeOf = (url: string) => new URL(url, 'http://localhost').pathname; + +function installServerDouble() { + vi.stubGlobal( + 'fetch', + vi.fn(async (input: unknown, init?: { method?: string; body?: unknown }) => { + const url = String(input); + const method = (init?.method ?? 'GET').toUpperCase(); + const path = routeOf(url); + if (method === 'POST' && path === '/api/v1/notifications/read') { + const body = JSON.parse(String(init?.body ?? '{}')) as { ids?: string[] }; + timeline.push(`POST ${path}`); + markReadBodies.push(body); + for (const id of body.ids ?? []) readIds.add(id); + return { ok: true, status: 200, json: async () => ({ success: true }) }; + } + if (method === 'GET' && path === '/api/v1/meta/_drafts') { + return { ok: true, status: 200, json: async () => ({ drafts: [] }) }; + } + if (method === 'GET' && path === '/api/v1/approvals/requests') { + return { ok: true, status: 200, json: async () => ({ data: [], total: 0 }) }; + } + unserved.push(`${method} ${url}`); + return { ok: false, status: 404, json: async () => ({}) }; + }), + ); +} + +import { HomePage } from '../HomePage'; +import { __resetSharedUserFeeds } from '../../../hooks/sharedUserFeeds'; + +beforeEach(() => { + timeline = []; + markReadBodies = []; + unserved = []; + readIds = new Set(); + navigateMock.mockClear(); + __resetSharedUserFeeds(); + installServerDouble(); +}); + +afterEach(() => { + // A router, not a sink: a request to anything this double does not serve + // fails here instead of vanishing into a best-effort `catch`. + expect(unserved).toEqual([]); + cleanup(); + vi.unstubAllGlobals(); + __resetSharedUserFeeds(); +}); + +const notificationsCard = () => screen.getByTestId('home-my-work-notifications'); +const unreadPill = () => within(notificationsCard()).queryByTestId('home-my-work-notifications-count'); + +describe('Home "My work → Notifications": opening a notification marks it read', () => { + it('sends the bell mark-read request for the opened row, before following its link', async () => { + const user = userEvent.setup(); + render(); + await screen.findByText('Task T-1 was assigned to you'); + + await user.click(screen.getByText('Task T-1 was assigned to you')); + + // The request the bell sends: one POST, keyed by the notification EVENT + // id (`notification_id`), not the inbox row id. + expect(markReadBodies).toEqual([{ ids: ['ntf_1'] }]); + // Spent first, then followed — the bell's order. + expect(timeline).toEqual([ + 'POST /api/v1/notifications/read', + 'navigate /apps/crm/showcase_task/t_1', + ]); + }); + + it('drops the unread count once the feed re-reads the receipt the request wrote', async () => { + const user = userEvent.setup(); + render(); + await screen.findByText('Task T-1 was assigned to you'); + expect(unreadPill()).toHaveTextContent('2'); + + await user.click(screen.getByText('Task T-1 was assigned to you')); + + // Returning to the tab re-reads the shared feed at once — the same read + // the bell's badge is cut from. + await act(async () => { + document.dispatchEvent(new Event('visibilitychange')); + }); + + await waitFor(() => expect(unreadPill()).toHaveTextContent('1')); + expect(screen.queryByText('Task T-1 was assigned to you')).not.toBeInTheDocument(); + expect(screen.getByText('You were mentioned in Task T-2')).toBeInTheDocument(); + }); + + it('marks a linkless notification read too, then opens the inbox', async () => { + const original = INBOX[0].action_url; + (INBOX[0] as { action_url?: string }).action_url = undefined; + try { + const user = userEvent.setup(); + render(); + await screen.findByText('Task T-1 was assigned to you'); + + await user.click(screen.getByText('Task T-1 was assigned to you')); + + expect(markReadBodies).toEqual([{ ids: ['ntf_1'] }]); + expect(timeline).toEqual([ + 'POST /api/v1/notifications/read', + 'navigate /apps/crm/sys_inbox_message?view=mine', + ]); + } finally { + INBOX[0].action_url = original; + } + }); +}); diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.pendingDraftsPackageName-13136.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.pendingDraftsPackageName-13136.test.tsx index cf0cff76b8b..e046f03e990 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.pendingDraftsPackageName-13136.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.pendingDraftsPackageName-13136.test.tsx @@ -116,6 +116,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ usePublishAllDrafts: () => ({ publishAll: vi.fn(async () => ({ ok: true })), publishing: false }), })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; const DRAFTS_ROUTE = '/api/v1/meta/_drafts'; diff --git a/ui/packages/app-shell/src/console/home/__tests__/HomePage.recentlyAccessed-12604.test.tsx b/ui/packages/app-shell/src/console/home/__tests__/HomePage.recentlyAccessed-12604.test.tsx index bd08513e4c9..a4acdfb8983 100644 --- a/ui/packages/app-shell/src/console/home/__tests__/HomePage.recentlyAccessed-12604.test.tsx +++ b/ui/packages/app-shell/src/console/home/__tests__/HomePage.recentlyAccessed-12604.test.tsx @@ -133,6 +133,10 @@ vi.mock('../../../preview/usePublishAllDrafts', () => ({ usePublishAllDrafts: () => ({ publishAll: async () => ({ ok: true }), publishing: false }), })); +// Home marks an opened notification read through the bell's own write +// (`useInboxBell`); this harness serves no inbox feed, so the write is stubbed. +vi.mock('../../../hooks/useInboxBell', () => ({ useInboxBell: () => ({ markRead: vi.fn() }) })); + import { HomePage } from '../HomePage'; const CRM = { name: 'crm', label: 'HotCRM', icon: 'Building2', _packageId: 'com.acme.crm' }; diff --git a/ui/packages/app-shell/src/layout/InboxPopover.tsx b/ui/packages/app-shell/src/layout/InboxPopover.tsx index 23129d25d35..3c98312cca9 100644 --- a/ui/packages/app-shell/src/layout/InboxPopover.tsx +++ b/ui/packages/app-shell/src/layout/InboxPopover.tsx @@ -38,10 +38,11 @@ import { useObjectTranslation, useDisplayLocale } from '@object-ui/i18n'; import type { ActivityFeedReading } from '../hooks/sharedUserFeeds.js'; import { useNavigationContext } from '../context/NavigationContext.js'; import { useMetadata } from '../providers/MetadataProvider.js'; -import { resolveHostAppSegment, resolveNotificationTarget } from '../utils/appRoute.js'; +import { resolveHostAppSegment } from '../utils/appRoute.js'; import { timeAgo } from '../utils/relativeTime.js'; import { groupNotifications } from './inboxGrouping.js'; import type { InboxNotification, NotificationGroup } from './inboxGrouping.js'; +import { openInboxNotification } from './openInboxNotification.js'; import { ActivityObjectLabel } from './ActivityObjectLabel.js'; import { confirmUnsavedLeave } from '../chrome/unsavedEdits.js'; @@ -279,46 +280,43 @@ export function InboxPopover({ leaveTo(`/apps/${hostAppSegment}/sys_activity`); }; - const handleNotificationClick = (n: InboxNotification) => { - onMarkRead(n.id); - // Prefer the materialization's action_url (ADR-0030). The messaging - // pipeline synthesizes an app-relative `/{object}/{id}` link from the - // event's source when a producer didn't set an explicit url — so it names - // a RECORD, and `hostAppSegment` (already resolved above for the two "see - // all" drills) is what turns it into a route. - // - // This used to host it under a raw `currentAppName ?? params.appName` and - // navigate BARE when neither named an app — which is the state on every - // surface outside `/apps/:appName/*` that mounts this bell (`/home`, - // `/organizations`, the full-page AI screen). The bare path matched no - // route, the console catch-all forwarded it to `/`, and the landing - // resolver dropped the user on the default app's home: objectui#5179. - // Reusing the drills' own resolver also stops an unchecked remembered app - // from addressing a link into an app that has since been deactivated. - const target = resolveNotificationTarget(n.action_url, hostAppSegment); - if (!target) { - // No link at all — a real state, not a defect: the producer leaves - // `action_url` undefined when an emit carries neither a `payload.url` nor - // a `source`. `onMarkRead` above has already consumed the row, so - // returning here spent the notification and left the popover sitting open - // on it, having navigated nowhere (objectui#5190). - // - // Home's action centre answers this by opening the full inbox - // (objectui#4074), and that is the ruled behaviour — so the bell reuses - // the drill it already has one screen up rather than writing a second - // answer to the same question. `goToAllNotifications` closes the popover - // and lands on the same `?view=mine` page the row came from, which is - // where a linkless notification can still be read in full. - goToAllNotifications(); - return; - } - setOpen(false); - if (target.kind === 'external') { - window.open(target.url, '_blank', 'noopener,noreferrer'); - return; - } - leaveTo(target.path); - }; + // Opening a row is `openInboxNotification`, the ONE open action — Home's + // "My work → Notifications" row is the other door onto it, so the two + // cannot disagree about what opening a notification does: mark it read + // first, then follow its link. + // + // The link is the materialization's action_url (ADR-0030). The messaging + // pipeline synthesizes an app-relative `/{object}/{id}` link from the + // event's source when a producer didn't set an explicit url — so it names + // a RECORD, and `hostAppSegment` (already resolved above for the two "see + // all" drills) is what turns it into a route. + // + // This used to host it under a raw `currentAppName ?? params.appName` and + // navigate BARE when neither named an app — which is the state on every + // surface outside `/apps/:appName/*` that mounts this bell (`/home`, + // `/organizations`, the full-page AI screen). The bare path matched no + // route, the console catch-all forwarded it to `/`, and the landing + // resolver dropped the user on the default app's home: objectui#5179. + // Reusing the drills' own resolver also stops an unchecked remembered app + // from addressing a link into an app that has since been deactivated. + // + // No link at all is a real state, not a defect: the producer leaves + // `action_url` undefined when an emit carries neither a `payload.url` nor a + // `source`. The row is already spent by then, and stopping there left the + // popover sitting open on it, having navigated nowhere (objectui#5190). + // Home's action centre answers this by opening the full inbox + // (objectui#4074), and that is the ruled behaviour — so the bell reuses the + // drill it already has one screen up rather than writing a second answer to + // the same question. `goToAllNotifications` closes the popover and lands on + // the same `?view=mine` page the row came from, which is where a linkless + // notification can still be read in full. + const handleNotificationClick = (n: InboxNotification) => + openInboxNotification({ id: n.id, actionUrl: n.action_url }, hostAppSegment, { + markRead: onMarkRead, + goTo: leaveTo, + openInbox: goToAllNotifications, + beforeFollow: () => setOpen(false), + }); const toggleGroup = (key: string) => { setExpandedGroups((prev) => { diff --git a/ui/packages/app-shell/src/layout/__tests__/openInboxNotification.test.ts b/ui/packages/app-shell/src/layout/__tests__/openInboxNotification.test.ts new file mode 100644 index 00000000000..74e328915cf --- /dev/null +++ b/ui/packages/app-shell/src/layout/__tests__/openInboxNotification.test.ts @@ -0,0 +1,59 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `openInboxNotification` — the one open action both notification doors (the + * bell's popover row and Home's My-work row) call. Each arm is asserted as the + * ORDER of what the door was asked to do, because "marked read" and "followed + * the link" are both true of the defect it replaced, only in a different + * sequence or with the first step missing. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { openInboxNotification, type NotificationDoor } from '../openInboxNotification'; + +function recordingDoor(withBeforeFollow = true): { door: NotificationDoor; steps: string[] } { + const steps: string[] = []; + const door: NotificationDoor = { + markRead: (id) => { + steps.push(`markRead ${id}`); + }, + goTo: (path) => steps.push(`goTo ${path}`), + openInbox: () => steps.push('openInbox'), + ...(withBeforeFollow ? { beforeFollow: () => steps.push('beforeFollow') } : {}), + }; + return { door, steps }; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('openInboxNotification', () => { + it('marks read, then follows an app-relative link hosted under the app', () => { + const { door, steps } = recordingDoor(); + openInboxNotification({ id: 'ibx_1', actionUrl: '/showcase_task/t_1' }, 'crm', door); + expect(steps).toEqual(['markRead ibx_1', 'beforeFollow', 'goTo /apps/crm/showcase_task/t_1']); + }); + + it('marks read, then opens the inbox when there is no link — without beforeFollow', () => { + const { door, steps } = recordingDoor(); + openInboxNotification({ id: 'ibx_1', actionUrl: undefined }, 'crm', door); + expect(steps).toEqual(['markRead ibx_1', 'openInbox']); + }); + + it('marks read, then opens an external link in a new tab instead of routing', () => { + // This file runs in the node project: no DOM, so `window` is stubbed. + const open = vi.fn(() => null); + vi.stubGlobal('window', { open }); + const { door, steps } = recordingDoor(); + openInboxNotification({ id: 'ibx_1', actionUrl: 'https://example.com/x' }, 'crm', door); + expect(steps).toEqual(['markRead ibx_1', 'beforeFollow']); + expect(open).toHaveBeenCalledWith('https://example.com/x', '_blank', 'noopener,noreferrer'); + }); + + it('works for a door that has nothing to do before following a link', () => { + const { door, steps } = recordingDoor(false); + openInboxNotification({ id: 'ibx_1', actionUrl: '/apps/acme/invoice/42' }, 'crm', door); + expect(steps).toEqual(['markRead ibx_1', 'goTo /apps/acme/invoice/42']); + }); +}); diff --git a/ui/packages/app-shell/src/layout/openInboxNotification.ts b/ui/packages/app-shell/src/layout/openInboxNotification.ts new file mode 100644 index 00000000000..c7cf00bfeb4 --- /dev/null +++ b/ui/packages/app-shell/src/layout/openInboxNotification.ts @@ -0,0 +1,80 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * openInboxNotification — the one "open a notification" action. + * + * A notification has more than one door: the top-bar bell's popover row and + * the Home page's "My work → Notifications" row open the same message. They + * used to carry two copies of the open logic, and the copies drifted: the bell + * marked the message read before following its link, while the Home row only + * followed the link, so a notification opened from Home stayed unread on the + * server and kept the bell badged. Both doors now call this function, so they + * cannot answer differently about what opening a notification does. + * + * Opening a notification, in order: + * + * 1. Spend it: `markRead(id)`. Always first, and whatever the link turns out + * to be — a linkless notification is still read once the user opened it. + * 2. Resolve its `action_url` with `resolveNotificationTarget` (app-relative + * links are hosted under `hostAppSegment`). + * 3. No link at all ⇒ `openInbox()` — the full inbox page, where a linkless + * notification can still be read in full. + * 4. A link ⇒ `beforeFollow()` (the bell closes its popover), then an + * external URL opens in a new tab and a console route goes to `goTo`. + * + * What differs per door is passed in, never re-decided here: how the door + * leaves the page (the bell asks first while the page holds unsaved edits), + * what it does before following a link, and where its "open the inbox" lands. + * + * @module + */ +import { resolveNotificationTarget } from '../utils/appRoute.js'; + +/** The two fields of a notification row that opening it reads. */ +export interface OpenableNotification { + /** The inbox row id — the id the door's `markRead` is keyed by. */ + id: string; + /** The materialization's `action_url` (ADR-0030 L5); absent ⇒ no link. */ + actionUrl?: string | null; +} + +/** What one door supplies to the shared action. */ +export interface NotificationDoor { + /** Mark the row read — the inbox's own write, never a second copy of it. */ + markRead: (id: string) => void | Promise; + /** Leave for a console route (already hosted under an app). */ + goTo: (path: string) => void; + /** The notification carries no link: open the full inbox instead. */ + openInbox: () => void; + /** Runs once a link is about to be followed (e.g. close a popover). */ + beforeFollow?: () => void; +} + +/** + * Open one notification through `door`: mark it read, then follow its link or, + * without one, open the inbox. See the module docblock for the order. + */ +export function openInboxNotification( + notification: OpenableNotification, + hostAppSegment: string, + door: NotificationDoor, +): void { + void door.markRead(notification.id); + const target = resolveNotificationTarget(notification.actionUrl, hostAppSegment); + if (!target) { + door.openInbox(); + return; + } + door.beforeFollow?.(); + if (target.kind === 'external') { + window.open(target.url, '_blank', 'noopener,noreferrer'); + return; + } + door.goTo(target.path); +}