diff --git a/.changeset/home-action-centre-badges-full-unread-count-4329.md b/.changeset/home-action-centre-badges-full-unread-count-4329.md new file mode 100644 index 000000000..cbf209502 --- /dev/null +++ b/.changeset/home-action-centre-badges-full-unread-count-4329.md @@ -0,0 +1,15 @@ +--- +'@object-ui/app-shell': patch +--- + +Home's action centre badges everything that is waiting, not the five rows it has room for + +`/home` showed two numbers for one question. The bell badges distinct unread topics plus pending approvals over the shared feed's full 20-row window; the action centre 200px below badged `pendingApprovalsCount + notifications.length` — and `notifications` is the list it renders, which `useHomeInbox` caps at 5. So nine unread messages read as **9** on the bell and **5** on the card, on one page, about one set of rows. The badge was reporting the size of a preview as if it were a total. + +Before objectui#4225 the card could not have said anything else: its own read was `$top: 5`, so nine was not a number it had. Both surfaces now cut from one already-joined feed, so the true count is in hand at Home's call site and the cap is a presentation slice over data the card already holds. + +`useHomeInbox` grows one additive field, `unreadTopicCount`, and `HomeActionCenter` takes it as a required prop: badge = `pendingApprovalsCount + unreadTopicCount`, list = the same unread set, newest first, still capped at 5. Badge means "how much needs you", list means "the newest few of it" — two semantics, each truthful, one number. + +The count is the bell's own fold (`groupNotifications`, by `(topic, title)`) applied to the bell's own rows, deliberately, rather than the pre-slice length of Home's list. That length is title-folded and drops blank titles, so it would agree with the bell on ordinary data and disagree whenever two topics share a title — and "two derivations of one number that agree usually" is exactly the defect objectui#4316 was. One fold, applied twice, cannot drift. + +Two adjacent behaviours are unchanged and now pinned as such: the approvals addend (distinct pending request ids from the shared REST feed, degrading to 0 on 404) and the list's own cap of five. "You're all caught up" is now gated on the total rather than on the rows on show, so it can no longer contradict the badge above it. diff --git a/packages/app-shell/src/console/home/HomePage.tsx b/packages/app-shell/src/console/home/HomePage.tsx index 49257f869..121f2fb7f 100644 --- a/packages/app-shell/src/console/home/HomePage.tsx +++ b/packages/app-shell/src/console/home/HomePage.tsx @@ -253,7 +253,8 @@ export function HomePage() { const { favorites } = useFavorites(); const { user } = useAuth(); const isAdmin = useIsWorkspaceAdmin(); - const { pendingApprovalsCount, notifications, notificationsStatus, activities } = useHomeInbox(); + const { pendingApprovalsCount, notifications, unreadTopicCount, notificationsStatus, activities } = + useHomeInbox(); // Home renders OUTSIDE the `/apps/:appName/*` router, so there is no // `params.appName` to read — `currentAppName` (published by ConsoleLayout on // every app mount) is the only "which app is the user in" signal available @@ -462,6 +463,9 @@ export function HomePage() { navigate(`/apps/${hostAppSegment}/system/approvals`)} /* The fallback arm runs whenever a notification carries no diff --git a/packages/app-shell/src/console/home/HomeRail.tsx b/packages/app-shell/src/console/home/HomeRail.tsx index 5b4c37967..caca3cd9d 100644 --- a/packages/app-shell/src/console/home/HomeRail.tsx +++ b/packages/app-shell/src/console/home/HomeRail.tsx @@ -113,13 +113,27 @@ function Row({ export function HomeActionCenter({ pendingApprovalsCount, notifications, + unreadTopicCount, notificationsStatus, onOpenApprovals, onOpenNotification, t, }: { pendingApprovalsCount: number; + /** The PREVIEW: newest-first, one row per title, capped by `useHomeInbox`. */ notifications: HomeNotification[]; + /** + * The TOTAL waiting in the inbox — every unread topic, not just the ones this + * card has room for (#4329). + * + * Required, and separate from `notifications` for the same reason + * `notificationsStatus` is: `notifications.length` was the badge until this + * card learned the difference, which made the badge report the size of a + * capped list. Nine unread showed "9" on the bell and "5" here, on one page, + * about one set of rows. A call site that cannot say how much is waiting must + * not be able to badge its own preview length by saying nothing. + */ + unreadTopicCount: number; /** * Required, not optional-with-a-default: a call site that cannot say whether * its rows are an answer must not be able to reach the affirmative copy by @@ -131,7 +145,11 @@ export function HomeActionCenter({ t: TFn; }) { const { language } = useObjectTranslation(); - const total = pendingApprovalsCount + notifications.length; + // "How much needs you", which is the question the badge asks and the question + // the bell answers with the same number. The list below is a preview of it — + // fewer rows than this whenever the cap or the title fold bites, and that + // gap is the point rather than a defect: badge = total, list = preview. + const total = pendingApprovalsCount + unreadTopicCount; const answered = notificationsStatus === 'ready'; return ( @@ -155,6 +173,15 @@ export function HomeActionCenter({ )} )} + {/* + Gated on the TOTAL, not on the rows on show: "You're all caught up" is a + claim about the inbox, so it may only be made when the inbox is empty — + never merely because this card had nothing renderable to list. (The one + state where the two differ is an unread message with no title at all, + which the list cannot render: the card then shows its badge and no row, + rather than telling the user they are caught up while the bell above + badges the same message.) + */} {total === 0 ? ( answered && (
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 52122a3d7..ff8dd1354 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 @@ -105,11 +105,13 @@ import { HomeActionCenter } from '../HomeRail'; /** Exactly `HomePage`'s wiring of the two — the seam the card indicts. */ function HomeActionCenterHost() { - const { pendingApprovalsCount, notifications, notificationsStatus } = useHomeInbox(); + const { pendingApprovalsCount, notifications, notificationsStatus, unreadTopicCount } = + useHomeInbox(); return ( {}} onOpenNotification={() => {}} @@ -265,16 +267,15 @@ describe('Home action centre — nine unread rows are listed and badged (#4235)' await waitFor(() => expect(screen.getByText('Approval request 1 needs your decision')).toBeInTheDocument(), ); - // 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. + // Five ROWS and a badge of nine (#4329). The card's cap used to travel as + // the read's own `$top: 5`; #4225 moved the read to the shared feed (whose + // `$top` is the bell's 20) and the cap became a client-side slice, so the + // list is still five. The badge counted that slice until #4329, which is + // how one page came to show 9 on the bell and 5 here for one question — + // the badge is the total waiting now, the list its newest few. 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(badgeText()).toBe('9'); expect(screen.queryByText(CAUGHT_UP)).not.toBeInTheDocument(); expect(screen.queryByTestId('home-action-unanswered')).not.toBeInTheDocument(); }); diff --git a/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx b/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx index 2b500eb81..9f7484ff7 100644 --- a/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx +++ b/packages/app-shell/src/console/home/__tests__/HomePage.approvalsTarget.test.tsx @@ -86,7 +86,12 @@ vi.mock('../../../context/NavigationContext', () => ({ vi.mock('../../../hooks/useRecentItems', () => ({ useRecentItems: () => ({ recentItems: [] }) })); vi.mock('../../../hooks/useFavorites', () => ({ useFavorites: () => ({ favorites: [] }) })); vi.mock('../../../hooks/useHomeInbox', () => ({ - useHomeInbox: () => ({ pendingApprovalsCount: 3, notifications: [], activities: [] }), + useHomeInbox: () => ({ + pendingApprovalsCount: 3, + notifications: [], + unreadTopicCount: 0, + activities: [], + }), })); vi.mock('../../../hooks/useAiSurface', () => ({ resolveAiApiBase: () => '' })); vi.mock('../../../views/metadata-admin/useMetadata', () => ({ diff --git a/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx b/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx index ecb5e1bd2..f032d8253 100644 --- a/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx +++ b/packages/app-shell/src/console/home/__tests__/HomePage.inboxLinksTarget.test.tsx @@ -91,6 +91,7 @@ vi.mock('../../../hooks/useHomeInbox', () => ({ useHomeInbox: () => ({ pendingApprovalsCount: 0, notifications: notificationsFixture, + unreadTopicCount: notificationsFixture.length, activities: activitiesFixture, }), })); diff --git a/packages/app-shell/src/console/home/__tests__/HomeRail.i18n.test.tsx b/packages/app-shell/src/console/home/__tests__/HomeRail.i18n.test.tsx index c8ef06e01..6e446a192 100644 --- a/packages/app-shell/src/console/home/__tests__/HomeRail.i18n.test.tsx +++ b/packages/app-shell/src/console/home/__tests__/HomeRail.i18n.test.tsx @@ -33,6 +33,7 @@ function renderRail() { notifications={[ { id: 'n1', title: '系统文件已分配给你', createdAt: threeDaysAgo() } as any, ]} + unreadTopicCount={1} notificationsStatus="ready" onOpenApprovals={() => {}} onOpenNotification={() => {}} diff --git a/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx b/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx index d4f1dd457..52c23dc6f 100644 --- a/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx +++ b/packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx @@ -31,6 +31,31 @@ * - 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. + * + * ## #4329 — one question, one number + * + * Read-state was only half of "the two surfaces agree". The other half is HOW + * MANY: Home badged `pendingApprovalsCount + notifications.length`, and + * `notifications` is the list it renders — which `useHomeInbox` caps at 5. So + * with nine unread the bell said 9 and the card two hundred pixels below said + * 5, for the same question about the same rows. The badge was reporting the + * size of a preview as if it were a total. + * + * The badge now counts unread TOPICS through `groupNotifications` — the bell's + * own fold, over the bell's own rows — while the list stays capped. Badge = + * "how much is waiting", list = "the newest few of it": two semantics, each + * truthful, one number. + * + * Reverse verification (predictions first, measured in PR #4344): + * - restore `total = pendingApprovalsCount + notifications.length` ⇒ the + * #4329 block goes red (Home badges its capped 5 against the bell's 9) and + * the whole #4316 read-state block stays GREEN — the cap and the join are + * different defects and neither pin stands in for the other; + * - count the pre-slice list length instead of the topic fold (title-folded, + * blank titles dropped) ⇒ every case here stays green EXCEPT + * "counts unread TOPICS, not the titles the list happens to show", which is + * the only fixture where the two folds disagree — that case is why the + * count is taken from `groupNotifications` rather than re-derived. */ import '@testing-library/jest-dom/vitest'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; @@ -198,6 +223,29 @@ const READ_TITLES = [ 'Scheduled maintenance Sunday 02:00', ]; +/** + * Two unread messages sharing a TITLE across two different topics — the one + * fixture where the surfaces' two folds disagree. The bell folds by + * `(topic, title)` and sees two topics; Home's LIST folds by title alone (its + * digest-collapsing rule, deliberately unchanged here) and renders one row. + * The badge is the bell's question, so it answers the bell's fold: 2. + */ +const TWIN_TITLES = [ + { id: 'ibx_t1', notification_id: 'ntf_t1', topic: 'crm.lead.assigned', title: 'Acme Corp needs you', created_at: '2026-08-11T05:00:00Z' }, + { id: 'ibx_t2', notification_id: 'ntf_t2', topic: 'approval.reminder', title: 'Acme Corp needs you', created_at: '2026-08-11T04:00:00Z' }, +].map((r) => ({ ...r, user_id: 'u1', action_url: `/apps/showcase/x/record/${r.id}` })); + +/** Twelve unread topics — past the bell's "9+" display clamp. */ +const TWELVE = Array.from({ length: 12 }, (_, i) => ({ + id: `ibx_d${i + 1}`, + user_id: 'u1', + notification_id: `ntf_d${i + 1}`, + topic: `topic.${i + 1}`, + title: `Waiting item ${i + 1}`, + action_url: `/apps/crm/x/record/d_${i + 1}`, + created_at: `2026-08-${String(i + 1).padStart(2, '0')}T09:00:00Z`, +})); + /** `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) => ({ @@ -239,12 +287,14 @@ import { __resetSharedUserFeeds } from '../sharedUserFeeds'; * works, so every assertion has to say WHICH panel it is talking about. */ function HomeProbe() { - const { pendingApprovalsCount, notifications, notificationsStatus } = useHomeInbox(); + const { pendingApprovalsCount, notifications, notificationsStatus, unreadTopicCount } = + useHomeInbox(); return (
{}} onOpenNotification={() => {}} @@ -300,6 +350,25 @@ afterEach(() => { vi.unstubAllGlobals(); }); +/** + * Approvals answer with `ids` pending requests; every other fetch still 404s. + * + * The count is the number of DISTINCT request ids (`sharedUserFeeds` dedupes by + * `id` before counting), degrading to 0 on 404 — the second addend BOTH badges + * have always had, and the one rule #4329 deliberately leaves alone. + */ +const approvalsPending = (ids: string[]) => + vi.stubGlobal( + 'fetch', + vi.fn((url: unknown) => + Promise.resolve( + String(url).includes('/api/v1/approvals/requests') + ? new Response(JSON.stringify({ data: ids.map((id) => ({ id })) }), { status: 200 }) + : new Response('{}', { status: 404 }), + ), + ), + ); + 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 @@ -386,14 +455,75 @@ describe('#4316 — an already-read message is not "needs your attention"', () = render(); - await waitFor(() => expect(homeBadge()).toBe('5')); - // Home caps its list at `limit` (5) … + // Nine unread, and both surfaces say nine. Home's LIST is still capped at + // `limit` (5) — the cap is a presentation slice, and #4329 is the pin that + // it may not leak into the badge. + await waitFor(() => expect(homeBadge()).toBe('9')); 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('#4329 — the badge is the total, the list is the preview', () => { + it('counts unread TOPICS, not the titles the list happens to show', async () => { + // The discriminating fixture. Two unread topics that share a title: the + // bell's fold sees two, a title fold sees one. Deriving Home's badge from + // its own (pre-slice) list length would agree with the bell on every other + // case in this file and disagree here — "agrees usually" is precisely the + // shape of the defect #4316 was, so the badge takes the bell's own fold of + // the bell's own rows instead of re-deriving a second one. + inboxRows = TWIN_TITLES; + receiptRows = []; + + render(); + + await waitFor(() => expect(screen.getByTestId('inbox-bell-badge')).toHaveTextContent('2')); + expect(homeBadge()).toBe('2'); + // The LIST still collapses repeats by title — that is its digest-collapsing + // rule and #4329 does not touch it. One row under a badge of two is the + // badge/preview split doing its job, exactly as five rows under nine is. + expect(inHome('Acme Corp needs you')).toHaveLength(1); + expect(inBell('Acme Corp needs you')).toHaveLength(2); + }); + + it('adds the same pending-approvals count on both surfaces', async () => { + // Approvals are the badge's other addend and their rule is unchanged: the + // distinct pending request ids from the shared REST feed, added once on + // each surface. Four unread topics + two approvals = six, twice. + inboxRows = MIXED; + receiptRows = receiptsFor(MIXED, MIXED_READ_IDS); + approvalsPending(['ar_1', 'ar_2']); + + render(); + + await waitFor(() => expect(screen.getByTestId('inbox-bell-badge')).toHaveTextContent('6')); + expect(homeBadge()).toBe('6'); + // …and the approvals row is still listed with its own count, unchanged. + expect(within(home()).getByTestId('home-action-approvals')).toHaveTextContent( + '2 pending approvals', + ); + }); + + it('reports the same count past the bell’s "9+" display clamp', async () => { + // Measured, and pinned as measured: twelve unread topics reach both + // surfaces as twelve. The bell CLAMPS its rendering at "9+" because its + // badge is a 20px circle in the top bar (#2765); Home's card badge has the + // room and prints the number. Same count, two renderings of it — not two + // counts, which is what #4329 was filed about. Left as-is deliberately: + // "9+" and "12" do not contradict each other, and clamping Home would mean + // teaching the shared card badge a display rule it has no other use for. + inboxRows = TWELVE; + receiptRows = []; + + render(); + + await waitFor(() => expect(homeBadge()).toBe('12')); + expect(screen.getByTestId('inbox-bell-badge')).toHaveTextContent('9+'); + // The list is unmoved by any of it — still five rows. + expect(within(home()).queryAllByText(/Waiting item \d+/)).toHaveLength(5); + }); +}); + describe('#4225 — one feed, one read, however many consumers mount', () => { beforeEach(() => { inboxRows = MIXED; diff --git a/packages/app-shell/src/hooks/useHomeInbox.ts b/packages/app-shell/src/hooks/useHomeInbox.ts index 4fbf81e1e..4c4247e05 100644 --- a/packages/app-shell/src/hooks/useHomeInbox.ts +++ b/packages/app-shell/src/hooks/useHomeInbox.ts @@ -48,6 +48,12 @@ * 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. * + * That left HOW MANY (#4329). The card's badge counted the list it renders, so + * the cap leaked into it: nine unread read as "9" on the bell and "5" here. + * `unreadTopicCount` is the count of the whole unread set, folded the way the + * bell folds it — the list stays a capped preview, and the badge stops + * reporting a preview's size as a total. + * * @module */ import { useMemo } from 'react'; @@ -56,6 +62,7 @@ import { useSharedInboxFeed, useSharedPendingApprovalsCount, } from './sharedUserFeeds'; +import { groupNotifications } from '../layout/inboxGrouping'; import type { ActivityItem } from '../layout/ActivityFeed'; export interface HomeNotification { @@ -87,6 +94,23 @@ export type HomeInboxStatus = 'idle' | 'loading' | 'ready' | 'error'; export interface HomeInboxData { pendingApprovalsCount: number; notifications: HomeNotification[]; + /** + * How many distinct unread TOPICS are waiting — the whole unread set, not the + * `limit`-capped slice `notifications` renders (#4329). + * + * `notifications` is a PREVIEW: newest-first, one row per title, five of + * them. Counting it badged the size of that preview as if it were the total, + * so nine unread messages read as "9" on the bell and "5" on the card two + * hundred pixels below — one page, one question, two numbers. + * + * Deliberately the bell's own fold (`groupNotifications`, by `(topic, + * title)`) over the bell's own rows, rather than the pre-slice length of the + * list above: that length is title-folded and drops blank titles, so it + * agrees with the bell on ordinary data and disagrees when two topics share a + * title. "Agrees usually" between two derivations of one number is exactly + * what #4316 was; one fold, applied twice, cannot drift. + */ + unreadTopicCount: number; /** Whether `notifications` is an answer — see {@link HomeInboxStatus}. */ notificationsStatus: HomeInboxStatus; activities: ActivityItem[]; @@ -126,5 +150,20 @@ export function useHomeInbox(limit = 5): HomeInboxData { .slice(0, limit); }, [messages, limit]); - return { pendingApprovalsCount, notifications, notificationsStatus, activities }; + // The badge's inbox addend — see {@link HomeInboxData.unreadTopicCount}. Same + // rows, same fold, same reduce as the bell's `unreadTopics`, so the number + // Home shows is the number the bell shows by construction rather than by + // coincidence. Note it is NOT `notifications.length`: that is the preview. + const unreadTopicCount = useMemo( + () => groupNotifications(messages).reduce((n, g) => n + (g.unreadCount > 0 ? 1 : 0), 0), + [messages], + ); + + return { + pendingApprovalsCount, + notifications, + unreadTopicCount, + notificationsStatus, + activities, + }; }