From 0469812499c213e63be6e480fbca2a9248af8a7a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:54:00 +0100 Subject: [PATCH] fix(notifications): Mark all read empties a band of read rows, and a read landing mid-write cannot refill it --- .../NotificationsPanel.order.dom.test.tsx | 57 ++++++++++++++++++- .../notifications/NotificationsPanel.tsx | 26 ++++++--- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx index 1d821ae9..0f489b27 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx @@ -97,7 +97,7 @@ function mount() { ); const { rerender } = render(tree()); - return { rerender: () => rerender(tree()) }; + return { qc, rerender: () => rerender(tree()) }; } // Every test has a "Ready 1" on screen once the server list has landed. const loaded = () => screen.findByText("Ready 1"); @@ -211,4 +211,59 @@ describe("NotificationsPanel ยท order", () => { expect(row.isConnected).toBe(true); expect(document.activeElement).toBe(row); }); + + it("a band of rows already read still offers Mark all read, which empties it", async () => { + // Opening the center marked this browser's notices read: the band holds them, and nothing on + // the page is unread any more. + h.server = [server(1, 1 * H, true)]; + h.client = [client(2, 2 * H, { read: false })]; + const { rerender } = mount(); + await loaded(); + h.client = [client(2, 2 * H, { read: true })]; // what the mount's markAllRead leaves + await act(async () => rerender()); + expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Activity 2"]); + const button = screen.getByRole("button", { name: "inbox.markAllRead" }); + expect(button.hasAttribute("disabled")).toBe(false); + fireEvent.click(button); + await act(async () => rerender()); + expect(screen.queryByText(/inbox\.sectionNeedsYou/)).toBeNull(); + }); + + it("a read that lands before Mark all read commits does not put the rows back", async () => { + let commit!: () => void; + vi.spyOn(api, "markAllNotificationsRead").mockImplementation( + () => new Promise((resolve) => (commit = () => resolve({ ok: true } as never))), + ); + h.server = [server(1, 1 * H, false)]; + h.client = []; + const { qc } = mount(); + await loaded(); + fireEvent.click(screen.getByRole("button", { name: "inbox.markAllRead" })); + await waitFor(() => expect(api.markAllNotificationsRead).toHaveBeenCalled()); + // The poll (or another row's write) reads the list before the server has committed the write. + void qc.invalidateQueries({ queryKey: ["notifications"] }); + await waitFor(() => + expect(qc.getQueryData<{ items: AppNotification[] }>(["notifications"])?.items[0]?.read).toBe( + false, + ), + ); + h.server = [server(1, 1 * H, true)]; + await act(async () => commit()); + await waitFor(() => expect(screen.queryByText(/inbox\.sectionNeedsYou/)).toBeNull()); + expect(listUnder("inbox.sectionRecent")).toEqual(["Ready 1"]); + }); + + it("a refused Mark all read brings the rows back at once, not at the next poll", async () => { + vi.spyOn(api, "markAllNotificationsRead").mockRejectedValue(new Error("offline")); + h.server = [server(1, 1 * H, false)]; + h.client = []; + mount(); + await loaded(); + const reads = vi.mocked(api.notifications).mock.calls.length; + fireEvent.click(screen.getByRole("button", { name: "inbox.markAllRead" })); + await waitFor(() => + expect(vi.mocked(api.notifications).mock.calls.length).toBeGreaterThan(reads), + ); + await waitFor(() => expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Ready 1"])); + }); }); diff --git a/frontend/src/modules/notifications/NotificationsPanel.tsx b/frontend/src/modules/notifications/NotificationsPanel.tsx index 162a07ec..a02c8b44 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.tsx @@ -108,9 +108,6 @@ export default function NotificationsPanel() { ); const [openedAt] = useState(() => Date.now()); const [earlierOpen, setEarlierOpen] = useState(false); - const order = orderInbox(serverItems, clientItems, { now: openedAt, held }); - const asking = order.needsYou.map(entryKey).filter((k) => !held.has(k)); - if (asking.length > 0) setHeld(new Set([...held, ...asking])); // Opening the center marks the (ephemeral) client items read, matching the old bell. Server // items stay unread until acted on individually. @@ -129,7 +126,9 @@ export default function NotificationsPanel() { }); const readAllMut = useMutation({ mutationFn: api.markAllNotificationsRead, - onSuccess: refresh, + // Settled, not only succeeded: a refused write brings the rows back unread at once, rather than + // leaving them read until the next poll moves them under the reader. + onSettled: refresh, meta: { failTitle: "errors.writeFailed.notifReadAll" }, }); const dismissMut = useMutation({ @@ -143,14 +142,25 @@ export default function NotificationsPanel() { meta: { failTitle: "errors.writeFailed.notifClear" }, }); + // While Mark all read is on its way, a read that lands before the server commits it (the poll, or + // another row's write refreshing the list) still says "unread": those rows are read already, or + // the band would take them back and hold them for the rest of the visit. + const shownServer = readAllMut.isPending + ? serverItems.map((n) => (n.read ? n : { ...n, read: true })) + : serverItems; + const order = orderInbox(shownServer, clientItems, { now: openedAt, held }); + const asking = order.needsYou.map(entryKey).filter((k) => !held.has(k)); + if (asking.length > 0) setHeld(new Set([...held, ...asking])); + const serverUnread = serverItems.some((n) => !n.read); const hasAny = serverItems.length > 0 || clientItems.length > 0; - const hasUnread = serverUnread || clientUnread > 0; + // A band of rows already read still offers the gesture that empties it. + const canMarkAll = serverUnread || clientUnread > 0 || order.needsYou.length > 0; function markAll() { markAllClientRead(); // Read at once, not when the write lands: until then the rows still read unread, and the band - // would take them straight back. A failed write refetches the truth (refresh, then the poll). + // would take them straight back. A failed write refetches the truth when it settles. void qc.cancelQueries({ queryKey: qk.notifications() }); qc.setQueryData<{ items: AppNotification[]; total: number }>(qk.notifications(), (d) => d ? { ...d, items: d.items.map((n) => (n.read ? n : { ...n, read: true })) } : d, @@ -404,7 +414,7 @@ export default function NotificationsPanel() { id: "read-all", label: t("inbox.markAllRead"), icon: icon(CheckCheck), - disabled: !hasUnread || readAllMut.isPending, + disabled: !canMarkAll || readAllMut.isPending, onSelect: markAll, }, { @@ -438,7 +448,7 @@ export default function NotificationsPanel() { variant="ghost" size="sm" onClick={markAll} - disabled={!hasUnread || readAllMut.isPending} + disabled={!canMarkAll || readAllMut.isPending} icon={} > {t("inbox.markAllRead")}