diff --git a/app/components/NotificationDot.tsx b/app/components/NotificationDot.tsx index 26f85d0c7..fce3476df 100644 --- a/app/components/NotificationDot.tsx +++ b/app/components/NotificationDot.tsx @@ -1,9 +1,15 @@ import clsx from "clsx"; import styles from "./NotificationDot.module.css"; -export function NotificationDot({ className }: { className?: string }) { +export function NotificationDot({ + className, + testId, +}: { + className?: string; + testId?: string; +}) { return ( - + diff --git a/app/components/layout/NotificationPopover.tsx b/app/components/layout/NotificationPopover.tsx index cd331dd4e..67b06ad6f 100644 --- a/app/components/layout/NotificationPopover.tsx +++ b/app/components/layout/NotificationPopover.tsx @@ -11,7 +11,10 @@ import { import { NOTIFICATIONS } from "~/features/notifications/notifications-contants"; import type { RootLoaderData } from "~/root"; import { NOTIFICATIONS_URL } from "~/utils/urls"; -import { useMarkNotificationsAsSeen } from "../../features/notifications/notifications-hooks"; +import { + useMarkNotificationsAsSeen, + useStickyUnseenIds, +} from "../../features/notifications/notifications-hooks"; import { SendouButton } from "../elements/Button"; import styles from "./NotificationPopover.module.css"; @@ -45,6 +48,7 @@ export function NotificationContent({ }) { const { t } = useTranslation(["common"]); const { refresh, isRefreshing } = useLayoutData(); + const stickyUnseenIds = useStickyUnseenIds(notifications); useMarkNotificationsAsSeen(unseenIds); @@ -73,7 +77,10 @@ export function NotificationContent({ {i !== notifications.length - 1 && } diff --git a/app/components/layout/index.tsx b/app/components/layout/index.tsx index 07d4dc926..64eecf57e 100644 --- a/app/components/layout/index.tsx +++ b/app/components/layout/index.tsx @@ -691,6 +691,7 @@ function SideNavUserPanel() { {unseenIds.length > 0 ? ( ) : null} load(LAYOUT_DATA_ROUTE), [load]); + const newest = useNewestOf(data, fetcher.data); useReloadOnNewDeploy(newest.buildCommit ?? ""); @@ -79,7 +82,7 @@ export function LayoutDataProvider({ const value: LayoutDataContextValue = { ...newest, - refresh: () => load(LAYOUT_DATA_ROUTE), + refresh, isRefreshing: state !== "idle", }; diff --git a/app/features/notifications/components/NotificationList.tsx b/app/features/notifications/components/NotificationList.tsx index 2d13f7d0b..3ef05c437 100644 --- a/app/features/notifications/components/NotificationList.tsx +++ b/app/features/notifications/components/NotificationList.tsx @@ -33,7 +33,12 @@ export function NotificationItem({ onClick={onClose} > - {!notification.seen ?
: null} + {!notification.seen ? ( +
+ ) : null}
{t( diff --git a/app/features/notifications/notifications-hooks.ts b/app/features/notifications/notifications-hooks.ts index de61c63ae..0dd3bcb3e 100644 --- a/app/features/notifications/notifications-hooks.ts +++ b/app/features/notifications/notifications-hooks.ts @@ -1,10 +1,13 @@ import * as React from "react"; import { useFetcher } from "react-router"; +import { useLayoutData } from "~/features/layout/LayoutDataProvider"; import { NOTIFICATIONS_MARK_AS_SEEN_ROUTE } from "~/utils/urls"; export function useMarkNotificationsAsSeen(unseenIds: number[]) { const fetcher = useFetcher(); + const { refresh } = useLayoutData(); const submittedIdsRef = React.useRef(new Set()); + const refreshPendingRef = React.useRef(false); const { submit } = fetcher; React.useEffect(() => { @@ -12,6 +15,13 @@ export function useMarkNotificationsAsSeen(unseenIds: number[]) { // get submitted when the fetcher returns to idle if (fetcher.state !== "idle") return; + // the bell dot reads from layout data, which the root loader does not + // revalidate for this action, so it has to be refetched by hand + if (refreshPendingRef.current) { + refreshPendingRef.current = false; + refresh(); + } + const idsToSubmit = unseenIds.filter( (id) => !submittedIdsRef.current.has(id), ); @@ -20,6 +30,7 @@ export function useMarkNotificationsAsSeen(unseenIds: number[]) { for (const id of idsToSubmit) { submittedIdsRef.current.add(id); } + refreshPendingRef.current = true; submit( { notificationIds: idsToSubmit }, @@ -29,5 +40,45 @@ export function useMarkNotificationsAsSeen(unseenIds: number[]) { action: NOTIFICATIONS_MARK_AS_SEEN_ROUTE, }, ); - }, [submit, unseenIds, fetcher.state]); + }, [submit, unseenIds, fetcher.state, refresh]); +} + +/** + * Ids of the notifications to show an unseen dot for, keeping the dot for as + * long as the list stays open. Opening the list marks its notifications as + * seen right away so the bell stops claiming there is something new, and this + * keeps the reader from losing track of which ones those were. + */ +export function useStickyUnseenIds( + notifications: Array<{ id: number; seen: number }>, +) { + const [unseenIds, setUnseenIds] = React.useState( + () => new Set(unseenIdsOf(notifications)), + ); + const [prevNotifications, setPrevNotifications] = + React.useState(notifications); + + if (prevNotifications !== notifications) { + setPrevNotifications(notifications); + setUnseenIds((prevUnseenIds) => { + const newUnseenIds = new Set(prevUnseenIds); + + for (const id of unseenIdsOf(notifications)) { + newUnseenIds.add(id); + } + + // optimize render by not updating state if nothing changed + if (newUnseenIds.size === prevUnseenIds.size) return prevUnseenIds; + + return newUnseenIds; + }); + } + + return unseenIds; +} + +function unseenIdsOf(notifications: Array<{ id: number; seen: number }>) { + return notifications + .filter((notification) => !notification.seen) + .map((notification) => notification.id); } diff --git a/app/features/notifications/routes/notifications.tsx b/app/features/notifications/routes/notifications.tsx index 83f2f64e7..b4c937055 100644 --- a/app/features/notifications/routes/notifications.tsx +++ b/app/features/notifications/routes/notifications.tsx @@ -11,7 +11,10 @@ import { NotificationsList, } from "../components/NotificationList"; import { loader } from "../loaders/notifications.server"; -import { useMarkNotificationsAsSeen } from "../notifications-hooks"; +import { + useMarkNotificationsAsSeen, + useStickyUnseenIds, +} from "../notifications-hooks"; export { loader }; @@ -27,36 +30,7 @@ export const meta: MetaFunction = (args) => { export default function NotificationsPage() { const { t } = useTranslation(["common"]); const data = useLoaderData(); - const [unseenIds, setUnseenIds] = React.useState( - () => - new Set( - data.notifications - .filter((notification) => !notification.seen) - .map((notification) => notification.id), - ), - ); - const [prevNotifications, setPrevNotifications] = React.useState( - data.notifications, - ); - - // persist unseen dots for the duration of the page being viewed - if (prevNotifications !== data.notifications) { - setPrevNotifications(data.notifications); - setUnseenIds((prevUnseenIds) => { - const newUnseenIds = new Set(prevUnseenIds); - - for (const notification of data.notifications) { - if (!notification.seen) { - newUnseenIds.add(notification.id); - } - } - - // optimize render by not updating state if nothing changed - if (newUnseenIds.size === prevUnseenIds.size) return prevUnseenIds; - - return newUnseenIds; - }); - } + const unseenIds = useStickyUnseenIds(data.notifications); const unSeenIdsArr = React.useMemo(() => Array.from(unseenIds), [unseenIds]); diff --git a/e2e/notifications.spec.ts b/e2e/notifications.spec.ts new file mode 100644 index 000000000..b53fda90c --- /dev/null +++ b/e2e/notifications.spec.ts @@ -0,0 +1,38 @@ +import { ADMIN_ID } from "~/features/admin/admin-constants"; +import { expect, impersonate, navigate, test } from "./helpers/playwright"; +import { NotificationPopover } from "./pages/layout/notification-popover"; + +const UNSEEN_COUNT = 2; + +test.describe("Notifications", () => { + test("opening the popover clears the bell dot but keeps the unseen dots listed", async ({ + page, + factories, + }) => { + for (let seasonNth = 1; seasonNth <= UNSEEN_COUNT; seasonNth++) { + await factories.NotificationFactory.create({ + notification: { type: "SEASON_STARTED", meta: { seasonNth } }, + users: [{ userId: ADMIN_ID, seen: 0 }], + }); + } + + await impersonate(page); + await navigate({ page, url: "/" }); + + const notifications = new NotificationPopover(page); + await expect(notifications.locators.bellDot).toBeVisible(); + + await notifications.open(); + + await expect(notifications.locators.bellDot).toBeHidden(); + await expect(notifications.locators.unseenDots).toHaveCount(UNSEEN_COUNT); + + await notifications.close(); + await expect(notifications.locators.items).toHaveCount(0); + + await notifications.open(); + + await expect(notifications.locators.items).toHaveCount(UNSEEN_COUNT); + await expect(notifications.locators.unseenDots).toHaveCount(0); + }); +}); diff --git a/e2e/pages/layout/notification-popover.ts b/e2e/pages/layout/notification-popover.ts index 11386eae7..fd746767b 100644 --- a/e2e/pages/layout/notification-popover.ts +++ b/e2e/pages/layout/notification-popover.ts @@ -12,6 +12,10 @@ export class NotificationPopover { openButton: this.page.getByTestId("notifications-button"), items: this.page.getByTestId("notification-item"), seeAllLink: this.page.getByTestId("notifications-see-all-button"), + /** Shown on the bell while unseen notifications exist. */ + bellDot: this.page.getByTestId("notifications-bell-dot"), + /** Per notification, marking it as one the user has not read yet. */ + unseenDots: this.page.getByTestId("notification-unseen-dot"), }; } @@ -19,6 +23,10 @@ export class NotificationPopover { await this.locators.openButton.click(); } + async close() { + await this.page.keyboard.press("Escape"); + } + notification(text: string) { return this.locators.items.filter({ hasText: text }); }