From c043d0e3036fae0b5a0b792b27a2132e850b23dd Mon Sep 17 00:00:00 2001 From: kipavy Date: Tue, 18 Aug 2026 22:56:19 +0000 Subject: [PATCH] fix(notifications): re-measure bells when the mounted set changes The notification centre's open state is global, but each bell measured its placement once, on the open transition. The mobile shell unmounts the foreground tab's bell on a tab switch while the SFTP tab's bell stays mounted behind `invisible`, so after a switch the surviving bell still held the `null` position it measured while hidden: the popover stopped painting even though the store said open, and the first tap on the visible bell only closed it. Bells now re-measure whenever any bell mounts or unmounts, so the popover follows the bell the user can actually see. --- .../notifications/NotificationBell.test.tsx | 70 ++++++++++++++++++- .../notifications/NotificationBell.tsx | 30 +++++++- 2 files changed, 97 insertions(+), 3 deletions(-) diff --git a/src/components/notifications/NotificationBell.test.tsx b/src/components/notifications/NotificationBell.test.tsx index e5db931ef..f198b57a2 100644 --- a/src/components/notifications/NotificationBell.test.tsx +++ b/src/components/notifications/NotificationBell.test.tsx @@ -1,5 +1,5 @@ import { test, expect, afterEach, beforeEach, vi } from "vitest"; -import { act, cleanup, render, screen } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; import { NotificationBell } from "./NotificationBell"; import { useNotificationStore } from "@/stores/notificationStore"; import { useUIStore } from "@/stores/uiStore"; @@ -67,3 +67,71 @@ test("an id no longer in the inbox still leaves the popover open", () => { expect(screen.getByText("notifications.bell.clearHistory")).toBeTruthy(); expect(useUIStore.getState().notificationFocusId).toBeNull(); }); + +// The mobile shell swaps the foreground tab's bell while the SFTP tab's bell +// stays mounted behind `invisible`. +function TwoBells({ hostsTab }: { hostsTab: boolean }) { + return ( + <> + {hostsTab &&
} +
+ + ); +} + +function stubHiddenSftpBell(hidden: () => boolean) { + const real = window.getComputedStyle; + window.getComputedStyle = ((el: Element) => + el instanceof HTMLButtonElement && el.closest("[data-testid='sftp']") && hidden() + ? ({ visibility: "hidden" } as CSSStyleDeclaration) + : real(el)) as typeof window.getComputedStyle; + return () => { + window.getComputedStyle = real; + }; +} + +const popovers = () => screen.queryAllByText("notifications.bell.clearHistory").length; + +test("only the visible bell paints while both are mounted", () => { + const restore = stubHiddenSftpBell(() => true); + try { + render(); + act(() => useUIStore.getState().openNotificationCenter(null)); + expect(popovers()).toBe(1); + } finally { + restore(); + } +}); + +test("the popover follows the bell a tab switch makes visible", () => { + let sftpHidden = true; + const restore = stubHiddenSftpBell(() => sftpHidden); + try { + const { rerender } = render(); + act(() => useUIStore.getState().openNotificationCenter(null)); + expect(popovers()).toBe(1); + + sftpHidden = false; + rerender(); + + expect(popovers()).toBe(1); + fireEvent.click(screen.getAllByRole("button")[0]); + expect(useUIStore.getState().notificationCenterOpen).toBe(false); + } finally { + restore(); + } +}); + +test("a link raised with no visible bell paints once one mounts", () => { + const restore = stubHiddenSftpBell(() => true); + try { + const { rerender } = render(); + act(() => useUIStore.getState().openNotificationCenter(null)); + expect(popovers()).toBe(0); + + rerender(); + expect(popovers()).toBe(1); + } finally { + restore(); + } +}); diff --git a/src/components/notifications/NotificationBell.tsx b/src/components/notifications/NotificationBell.tsx index ef8d86475..62a2d6bb0 100644 --- a/src/components/notifications/NotificationBell.tsx +++ b/src/components/notifications/NotificationBell.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState, useSyncExternalStore } from "react"; import { createPortal } from "react-dom"; import { useTranslation } from "react-i18next"; import i18n from "@/i18n"; @@ -21,6 +21,26 @@ const SEVERITY_COLORS: Record = { error: "var(--t-status-error)", }; +// Which bell is on screen changes when bells mount and unmount: the mobile +// shell swaps the foreground tab's bell while the SFTP tab's stays mounted +// behind `invisible`. Bells re-measure on this signal so the popover follows +// the bell the user can actually see instead of staying with the one that was +// visible when it opened. +let mountVersion = 0; +const mountListeners = new Set<() => void>(); + +function bumpMounts() { + mountVersion += 1; + for (const listener of mountListeners) listener(); +} + +function subscribeMounts(listener: () => void) { + mountListeners.add(listener); + return () => { + mountListeners.delete(listener); + }; +} + function relativeTime(ms: number): string { const diff = Date.now() - ms; if (diff < 60_000) return i18n.t("notifications.bell.relativeTime.justNow"); @@ -165,6 +185,12 @@ export function NotificationBell() { const buttonRef = useRef(null); const dropdownRef = useRef(null); + const mounts = useSyncExternalStore(subscribeMounts, () => mountVersion); + useEffect(() => { + bumpMounts(); + return bumpMounts; + }, []); + // Placement is measured on every open, not on click: a deep link opens the // popover with no pointer event to measure from. // @@ -182,7 +208,7 @@ export function NotificationBell() { } const rect = button.getBoundingClientRect(); setPos({ top: rect.bottom + 4, right: window.innerWidth - rect.right }); - }, [open]); + }, [open, mounts]); // A stale id is normal — inbox entries are re-derived, not stored — so a miss // leaves the popover open on the full list rather than reporting anything.