From da32dd4b416d833539ba701046aef547953833de Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 16 Oct 2026 01:37:07 +0000 Subject: [PATCH] refactor: share notification banner subscription and drop unused skip reasons The blocked-notification banner was wiring its own storage, permission, focus, and visibility listeners next to the existing notification store. Reuse that subscribe, persist dismiss in local state, and delete the unused skip-reason analytics surface. Co-authored-by: tyler --- .../CompassUI/TimeGrid/TimeGridView.swift | 6 +- .../NotificationPermissionBannerGate.test.tsx | 97 +++++++++++++++++++ .../NotificationPermissionBannerGate.tsx | 55 ++++------- .../notification-blocked-banner.util.test.ts | 1 - .../notifications/notification-diagnostics.ts | 53 ++-------- .../src/notifications/notification.state.ts | 25 ++++- .../notifications/upcoming-notifier.logic.ts | 6 +- .../notifications/useUpcomingEventNotifier.ts | 2 +- .../src/domain/event-instance-assembly.ts | 17 +--- 9 files changed, 152 insertions(+), 110 deletions(-) create mode 100644 apps/calendar-web/src/notifications/NotificationPermissionBannerGate.test.tsx diff --git a/apps/calendar-macos/CompassUI/Sources/CompassUI/TimeGrid/TimeGridView.swift b/apps/calendar-macos/CompassUI/Sources/CompassUI/TimeGrid/TimeGridView.swift index 0db9177ca..240baa6c7 100644 --- a/apps/calendar-macos/CompassUI/Sources/CompassUI/TimeGrid/TimeGridView.swift +++ b/apps/calendar-macos/CompassUI/Sources/CompassUI/TimeGrid/TimeGridView.swift @@ -311,7 +311,7 @@ public final class TimeGridView: NSView { timedContentView.frame = NSRect(x: 0, y: 0, width: contentWidth, height: gridHeight) syncPinnedHeaderScrollX() - renderHourGutter(snapshot: snapshot, hourHeight: hourHeight, gridHeight: gridHeight) + renderHourGutter(snapshot: snapshot, hourHeight: hourHeight) renderHeaders(snapshot: snapshot, headerHeight: headerHeight) renderAllDayColumns(snapshot: snapshot, allDayHeight: allDayHeight, headerHeight: headerHeight) renderTimedGrid(snapshot: snapshot, hourHeight: hourHeight, gridHeight: gridHeight) @@ -321,8 +321,7 @@ public final class TimeGridView: NSView { private func renderHourGutter( snapshot: GridLayoutSnapshot, - hourHeight: Double, - gridHeight: Double + hourHeight: Double ) { hourGutterView.subviews.forEach { $0.removeFromSuperview() } let palette = themePalette @@ -356,7 +355,6 @@ public final class TimeGridView: NSView { hourHeight: hourHeight, palette: palette, marginWidth: margin) - _ = gridHeight } private func renderHourColumn( diff --git a/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.test.tsx b/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.test.tsx new file mode 100644 index 000000000..385e86005 --- /dev/null +++ b/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.test.tsx @@ -0,0 +1,97 @@ +import { cleanup, render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { createTestNotificationPort } from "@web/__tests__/helpers/web-test-seams"; +import { STORAGE_KEYS } from "@web/common/constants/storage.constants"; +import { persistentBrowserStore } from "@web/common/storage/browser-key-value.store"; +import { NotificationPermissionBannerGate } from "@web/notifications/NotificationPermissionBannerGate"; +import { + registerNotificationPort, + resetNotificationPort, +} from "@web/notifications/notification.port"; +import { + recordNotificationShowFailure, + resetNotificationShowFailureCount, +} from "@web/notifications/notification-diagnostics"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import "@testing-library/jest-dom"; + +const banner = () => + screen.queryByRole("region", { name: "Event notifications blocked" }); + +describe("NotificationPermissionBannerGate", () => { + beforeEach(() => { + persistentBrowserStore.remove(STORAGE_KEYS.NOTIFICATIONS_ENABLED); + persistentBrowserStore.remove( + STORAGE_KEYS.NOTIFICATIONS_BLOCKED_BANNER_DISMISSED, + ); + resetNotificationShowFailureCount(); + resetNotificationPort(); + }); + + afterEach(() => { + cleanup(); + resetNotificationPort(); + resetNotificationShowFailureCount(); + persistentBrowserStore.remove(STORAGE_KEYS.NOTIFICATIONS_ENABLED); + persistentBrowserStore.remove( + STORAGE_KEYS.NOTIFICATIONS_BLOCKED_BANNER_DISMISSED, + ); + }); + + it("stays hidden when notifications are not opted in", () => { + registerNotificationPort( + createTestNotificationPort({ permission: "granted" }).port, + ); + + render(); + + expect(banner()).not.toBeInTheDocument(); + }); + + it("prompts for permission when the pref is on but the grant is missing", () => { + persistentBrowserStore.set(STORAGE_KEYS.NOTIFICATIONS_ENABLED, "true"); + registerNotificationPort( + createTestNotificationPort({ permission: "default" }).port, + ); + + render(); + + expect(banner()).toHaveTextContent("has not granted permission"); + expect( + screen.getByRole("button", { name: "Allow notifications" }), + ).toBeInTheDocument(); + }); + + it("hides after dismiss and stays hidden on remount", async () => { + const user = userEvent.setup(); + persistentBrowserStore.set(STORAGE_KEYS.NOTIFICATIONS_ENABLED, "true"); + registerNotificationPort( + createTestNotificationPort({ permission: "default" }).port, + ); + + const { unmount } = render(); + await user.click(screen.getByRole("button", { name: "Dismiss" })); + + expect(banner()).not.toBeInTheDocument(); + unmount(); + + render(); + expect(banner()).not.toBeInTheDocument(); + }); + + it("warns when the browser keeps dropping constructed notifications", () => { + persistentBrowserStore.set(STORAGE_KEYS.NOTIFICATIONS_ENABLED, "true"); + registerNotificationPort( + createTestNotificationPort({ permission: "granted" }).port, + ); + recordNotificationShowFailure(); + recordNotificationShowFailure(); + + render(); + + expect(banner()).toHaveTextContent("is not showing them"); + expect( + screen.queryByRole("button", { name: "Allow notifications" }), + ).not.toBeInTheDocument(); + }); +}); diff --git a/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.tsx b/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.tsx index fbb4e7421..d747d3a96 100644 --- a/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.tsx +++ b/apps/calendar-web/src/notifications/NotificationPermissionBannerGate.tsx @@ -1,52 +1,31 @@ -import { useCallback, useSyncExternalStore } from "react"; -import { STORAGE_KEYS } from "@web/common/constants/storage.constants"; -import { subscribeToStorageKey } from "@web/common/utils/external-store.util"; +import { useState, useSyncExternalStore } from "react"; import { NotificationPermissionBanner } from "@web/notifications/NotificationPermissionBanner"; -import { getNotificationPort } from "@web/notifications/notification.port"; +import { subscribeToNotificationState } from "@web/notifications/notification.state"; import { dismissNotificationBlockedBanner, + hasDismissedNotificationBlockedBanner, pickNotificationBlockedBannerKind, } from "@web/notifications/notification-blocked-banner.util"; -const subscribe = (onChange: () => void) => { - const unstorage = subscribeToStorageKey( - STORAGE_KEYS.NOTIFICATIONS_ENABLED, - onChange, - ); - const unblocked = subscribeToStorageKey( - STORAGE_KEYS.NOTIFICATIONS_BLOCKED_BANNER_DISMISSED, - onChange, - ); - const unobserve = getNotificationPort().observePermission(onChange); - const onVisibilityChange = () => { - if (document.visibilityState === "visible") onChange(); - }; - document.addEventListener("visibilitychange", onVisibilityChange); - window.addEventListener("focus", onChange); - window.addEventListener("storage", onChange); - - return () => { - unstorage(); - unblocked(); - unobserve(); - document.removeEventListener("visibilitychange", onVisibilityChange); - window.removeEventListener("focus", onChange); - window.removeEventListener("storage", onChange); - }; -}; - export function NotificationPermissionBannerGate() { const kind = useSyncExternalStore( - subscribe, + subscribeToNotificationState, pickNotificationBlockedBannerKind, () => null, ); + const [dismissed, setDismissed] = useState( + hasDismissedNotificationBlockedBanner, + ); - const handleDismiss = useCallback(() => { - dismissNotificationBlockedBanner(); - }, []); - - if (!kind) return null; + if (!kind || dismissed) return null; - return ; + return ( + { + dismissNotificationBlockedBanner(); + setDismissed(true); + }} + /> + ); } diff --git a/apps/calendar-web/src/notifications/notification-blocked-banner.util.test.ts b/apps/calendar-web/src/notifications/notification-blocked-banner.util.test.ts index b7c96166c..06b0799dd 100644 --- a/apps/calendar-web/src/notifications/notification-blocked-banner.util.test.ts +++ b/apps/calendar-web/src/notifications/notification-blocked-banner.util.test.ts @@ -22,7 +22,6 @@ describe("pickNotificationBlockedBannerKind", () => { STORAGE_KEYS.NOTIFICATIONS_BLOCKED_BANNER_DISMISSED, ); resetNotificationShowFailureCount(); - if (typeof sessionStorage !== "undefined") sessionStorage.clear(); resetNotificationPort(); }); diff --git a/apps/calendar-web/src/notifications/notification-diagnostics.ts b/apps/calendar-web/src/notifications/notification-diagnostics.ts index fa0b5ed75..ada4a03f0 100644 --- a/apps/calendar-web/src/notifications/notification-diagnostics.ts +++ b/apps/calendar-web/src/notifications/notification-diagnostics.ts @@ -1,60 +1,21 @@ -import { track } from "@web/auth/posthog/track"; -import { STORAGE_KEYS } from "@web/common/constants/storage.constants"; -import { persistentBrowserStore } from "@web/common/storage/browser-key-value.store"; -import { getNotificationPort } from "@web/notifications/notification.port"; - -const isNotificationsPrefEnabled = (): boolean => - persistentBrowserStore.get(STORAGE_KEYS.NOTIFICATIONS_ENABLED) === "true"; - -export type NotificationSkipReason = - | "not_effective" - | "no_reminders" - | "already_fired" - | "not_due" - | "show_failed"; +import { sessionBrowserStore } from "@web/common/storage/browser-key-value.store"; const SHOW_FAILURE_SESSION_KEY = "compass.notifications.session-show-failures"; export function readNotificationShowFailureCount(): number { - if (typeof sessionStorage === "undefined") return 0; - const raw = sessionStorage.getItem(SHOW_FAILURE_SESSION_KEY); - const parsed = raw ? Number.parseInt(raw, 10) : 0; + const parsed = Number.parseInt( + sessionBrowserStore.get(SHOW_FAILURE_SESSION_KEY) ?? "", + 10, + ); return Number.isFinite(parsed) && parsed > 0 ? parsed : 0; } export function recordNotificationShowFailure(): number { - if (typeof sessionStorage === "undefined") return 0; const next = readNotificationShowFailureCount() + 1; - sessionStorage.setItem(SHOW_FAILURE_SESSION_KEY, String(next)); + sessionBrowserStore.set(SHOW_FAILURE_SESSION_KEY, String(next)); return next; } export function resetNotificationShowFailureCount(): void { - if (typeof sessionStorage === "undefined") return; - sessionStorage.removeItem(SHOW_FAILURE_SESSION_KEY); -} - -export function trackNotificationPermissionState( - source: "palette" | "runtime", -): void { - const port = getNotificationPort(); - track("notifications_permission", { - source, - pref_enabled: isNotificationsPrefEnabled(), - permission: port.getPermission(), - supported: port.isSupported(), - effectively_on: - isNotificationsPrefEnabled() && port.getPermission() === "granted", - }); -} - -export function trackNotificationSkipped( - reason: NotificationSkipReason, - extra?: Record, -): void { - track("notifications_skipped", { reason, ...extra }); -} - -export function trackNotificationScheduled(reminderMinutes: number): void { - track("notifications_scheduled", { reminder_minutes: reminderMinutes }); + sessionBrowserStore.remove(SHOW_FAILURE_SESSION_KEY); } diff --git a/apps/calendar-web/src/notifications/notification.state.ts b/apps/calendar-web/src/notifications/notification.state.ts index 567b74003..e33a5fb1e 100644 --- a/apps/calendar-web/src/notifications/notification.state.ts +++ b/apps/calendar-web/src/notifications/notification.state.ts @@ -9,7 +9,6 @@ import { getNotificationPort, type NotificationPort, } from "@web/notifications/notification.port"; -import { trackNotificationPermissionState } from "@web/notifications/notification-diagnostics"; /** Where a toggle came from for analytics. */ export type NotificationToggleSource = "palette"; @@ -82,6 +81,10 @@ const startExternalSubscriptions = (): (() => void) => { STORAGE_KEYS.NOTIFICATIONS_ENABLED, emit, ); + const unblocked = subscribeToStorageKey( + STORAGE_KEYS.NOTIFICATIONS_BLOCKED_BANNER_DISMISSED, + emit, + ); const onVisibilityChange = () => { if (document.visibilityState === "visible") emit(); }; @@ -91,12 +94,15 @@ const startExternalSubscriptions = (): (() => void) => { return () => { unobserve(); unstorage(); + unblocked(); document.removeEventListener("visibilitychange", onVisibilityChange); window.removeEventListener("focus", emit); }; }; -const subscribe = (onChange: () => void): (() => void) => { +export const subscribeToNotificationState = ( + onChange: () => void, +): (() => void) => { listeners.add(onChange); if (subscriberCount === 0) { stopExternal = startExternalSubscriptions(); @@ -113,9 +119,22 @@ const subscribe = (onChange: () => void): (() => void) => { }; }; +export function trackNotificationPermissionState( + source: NotificationToggleSource | "runtime", +): void { + const port = getNotificationPort(); + track("notifications_permission", { + source, + pref_enabled: isNotificationsPrefEnabled(), + permission: port.getPermission(), + supported: port.isSupported(), + effectively_on: areNotificationsEffectivelyOn(), + }); +} + export function useNotificationsEffectivelyOn(): boolean { return useSyncExternalStore( - subscribe, + subscribeToNotificationState, areNotificationsEffectivelyOn, () => false, ); diff --git a/apps/calendar-web/src/notifications/upcoming-notifier.logic.ts b/apps/calendar-web/src/notifications/upcoming-notifier.logic.ts index 87c90d062..bc71b0a87 100644 --- a/apps/calendar-web/src/notifications/upcoming-notifier.logic.ts +++ b/apps/calendar-web/src/notifications/upcoming-notifier.logic.ts @@ -12,8 +12,6 @@ import { type NotificationPort } from "@web/notifications/notification.port"; import { recordNotificationShowFailure, resetNotificationShowFailureCount, - trackNotificationScheduled, - trackNotificationSkipped, } from "@web/notifications/notification-diagnostics"; import { inEffectiveTimeZone } from "@web/timezone/in-time-zone"; import { dayEventQueryRange } from "@web/views/Day/util/day-window.util"; @@ -145,7 +143,7 @@ export function announceUpcomingEvents( const announced = pruneFiredKeys(firedKeys, now); for (const { event, reminderMinutes } of due) { const key = notificationKey(event, reminderMinutes); - trackNotificationScheduled(reminderMinutes); + track("notifications_scheduled", { reminder_minutes: reminderMinutes }); const shown = showUpcomingEventNotification( port, event, @@ -161,7 +159,7 @@ export function announceUpcomingEvents( } // A silent browser drop must not burn the key: the next tick retries. recordNotificationShowFailure(); - trackNotificationSkipped("show_failed"); + track("notifications_skipped", { reason: "show_failed" }); track("notifications_show_failed"); } return announced; diff --git a/apps/calendar-web/src/notifications/useUpcomingEventNotifier.ts b/apps/calendar-web/src/notifications/useUpcomingEventNotifier.ts index 2e81dec4f..e8e197424 100644 --- a/apps/calendar-web/src/notifications/useUpcomingEventNotifier.ts +++ b/apps/calendar-web/src/notifications/useUpcomingEventNotifier.ts @@ -4,9 +4,9 @@ import { useTodayTimedEvents } from "@web/components/Sidebar/UpNextCard/useUpNex import { getNotificationPort } from "@web/notifications/notification.port"; import { isNotificationsPrefEnabled, + trackNotificationPermissionState, useNotificationsEffectivelyOn, } from "@web/notifications/notification.state"; -import { trackNotificationPermissionState } from "@web/notifications/notification-diagnostics"; import { announceUpcomingEvents, toNotifiableEvents, diff --git a/packages/sync/src/domain/event-instance-assembly.ts b/packages/sync/src/domain/event-instance-assembly.ts index cbecdb660..580f5c7a9 100644 --- a/packages/sync/src/domain/event-instance-assembly.ts +++ b/packages/sync/src/domain/event-instance-assembly.ts @@ -160,19 +160,10 @@ const toPopupReminderMinutes = ( event: EventRecord, seriesMaster?: EventRecord, ): { popupReminderMinutes?: readonly number[] } => { - const own = reminderMinutesFromProviderMetadata(event.providerMetadata); - if (own !== undefined) { - return { popupReminderMinutes: own }; - } - if (seriesMaster) { - const inherited = reminderMinutesFromProviderMetadata( - seriesMaster.providerMetadata, - ); - if (inherited !== undefined) { - return { popupReminderMinutes: inherited }; - } - } - return {}; + const minutes = + reminderMinutesFromProviderMetadata(event.providerMetadata) ?? + reminderMinutesFromProviderMetadata(seriesMaster?.providerMetadata); + return minutes !== undefined ? { popupReminderMinutes: minutes } : {}; }; const toInstanceContent = (event: EventRecord) => {