From 54c495983078ce8ae949ca86dc715bcdf3793064 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 6 Jul 2026 21:28:09 -0700 Subject: [PATCH] perf(mobile): cap the scheduled-notification map + tap-dedup set (#7646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scheduledNotificationsByHostAndNotificationId (mobile-notifications.ts) retained one entry per scheduled desktop notification. The key embeds notificationId, which carries a per-completion timestamp (buildAgentNotificationId), so every agent-task-complete inserts a new, never-reused key. Entries are removed only when the desktop sends a matching dismiss — which a remote mobile user (not sitting at the desktop) frequently never receives — so the module-level map grew for the app's whole lifetime. Small per entry, but genuinely unbounded. Fix: bound the map to the 256 most-recent SETTLED entries (never evict one mid-schedule). A settled entry only retains a small identifier used for later programmatic dismissal, which is unnecessary for long-past completions, so eviction has no user-visible effect. Also FIFO-cap RootLayout's handledNotificationIdsRef tap-dedup Set (RootLayout never unmounts, so it otherwise grew one id per tapped notification forever). Test (red->green): with the cap at 1, scheduling a second notification evicts the first, so a later dismiss for the evicted id is a no-op while the retained one still dismisses; without the cap the old entry survives. --- mobile/app/_layout.tsx | 8 +++ .../mobile-notifications.test.ts | 49 ++++++++++++++++++- .../src/notifications/mobile-notifications.ts | 33 +++++++++++++ 3 files changed, 89 insertions(+), 1 deletion(-) diff --git a/mobile/app/_layout.tsx b/mobile/app/_layout.tsx index ad94114bd..f47ec5910 100644 --- a/mobile/app/_layout.tsx +++ b/mobile/app/_layout.tsx @@ -102,6 +102,14 @@ export default function RootLayout() { return } handledNotificationIdsRef.current.add(notificationId) + // Why: RootLayout never unmounts, so cap this tap-dedup set (FIFO) rather + // than letting it grow one id per notification tapped for the app's life. + if (handledNotificationIdsRef.current.size > 256) { + const oldest = handledNotificationIdsRef.current.values().next().value + if (oldest !== undefined) { + handledNotificationIdsRef.current.delete(oldest) + } + } const path = await getNavigationPath(response.notification.request.content.data) clearLastNotificationResponse() diff --git a/mobile/src/notifications/mobile-notifications.test.ts b/mobile/src/notifications/mobile-notifications.test.ts index 46eab52ec..7ae431508 100644 --- a/mobile/src/notifications/mobile-notifications.test.ts +++ b/mobile/src/notifications/mobile-notifications.test.ts @@ -1,6 +1,9 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import * as Notifications from 'expo-notifications' -import { subscribeToDesktopNotifications } from './mobile-notifications' +import { + setScheduledNotificationsMaxForTests, + subscribeToDesktopNotifications +} from './mobile-notifications' import type { RpcClient } from '../transport/rpc-client' import { loadPushNotificationsEnabled } from '../storage/preferences' @@ -268,4 +271,48 @@ describe('subscribeToDesktopNotifications', () => { expect(Notifications.dismissNotificationAsync).not.toHaveBeenCalled() }) + + // Why: notificationId is unique per completion, so the map grew unbounded when + // the desktop never sent a dismiss (the remote-mobile case). It is now capped. + it('evicts the oldest scheduled entry once the cap is exceeded', async () => { + setScheduledNotificationsMaxForTests(1) + try { + vi.mocked(loadPushNotificationsEnabled).mockResolvedValue(true) + vi.mocked(Notifications.getPermissionsAsync).mockResolvedValue({ + status: 'granted', + canAskAgain: true + } as never) + vi.mocked(Notifications.scheduleNotificationAsync) + .mockResolvedValueOnce('scheduled-old') + .mockResolvedValueOnce('scheduled-new') + vi.mocked(Notifications.dismissNotificationAsync).mockResolvedValue(undefined) + let onEvent: ((data: unknown) => void) | null = null + const client = { + subscribe: vi.fn((_method, _params, callback: (data: unknown) => void) => { + onEvent = callback + return vi.fn() + }), + getState: vi.fn(() => 'connected'), + sendRequest: vi.fn() + } as unknown as RpcClient + + subscribeToDesktopNotifications(client, 'host-1') + onEvent?.({ type: 'notification', title: 't', body: 'b', notificationId: 'agent:old' }) + await flushAsync() + onEvent?.({ type: 'notification', title: 't', body: 'b', notificationId: 'agent:new' }) + await flushAsync() + + // The older entry was evicted by the cap: dismissing it is a no-op... + onEvent?.({ type: 'dismiss', notificationId: 'agent:old' }) + await flushAsync() + expect(Notifications.dismissNotificationAsync).not.toHaveBeenCalledWith('scheduled-old') + + // ...while the most-recent entry is retained and still dismissable. + onEvent?.({ type: 'dismiss', notificationId: 'agent:new' }) + await flushAsync() + expect(Notifications.dismissNotificationAsync).toHaveBeenCalledWith('scheduled-new') + } finally { + setScheduledNotificationsMaxForTests() + } + }) }) diff --git a/mobile/src/notifications/mobile-notifications.ts b/mobile/src/notifications/mobile-notifications.ts index 83abea6fb..9ab936f75 100644 --- a/mobile/src/notifications/mobile-notifications.ts +++ b/mobile/src/notifications/mobile-notifications.ts @@ -31,10 +31,42 @@ type ScheduledNotificationState = { const scheduledNotificationsByHostAndNotificationId = new Map() +// Why: notificationId embeds a per-completion timestamp (buildAgentNotificationId), +// so every agent-task-complete inserts a new, never-reused key. Entries are only +// removed when the desktop sends a matching dismiss — which a remote mobile user +// (not at the desktop) frequently never gets — so the map grew for the app's whole +// life. Bound it; a settled entry only retains a small identifier used for later +// programmatic dismissal, unnecessary for long-past completions. +const MAX_SCHEDULED_NOTIFICATIONS = 256 +let maxScheduledNotifications = MAX_SCHEDULED_NOTIFICATIONS + function getStoredNotificationKey(hostId: string, notificationId: string): string { return `${encodeURIComponent(hostId)}:${encodeURIComponent(notificationId)}` } +// Evict the oldest SETTLED entries (never one mid-schedule) until within the cap. +// Map iteration is insertion order, so the first match is the oldest. +function boundScheduledNotifications(): void { + while (scheduledNotificationsByHostAndNotificationId.size > maxScheduledNotifications) { + let evicted = false + for (const [key, state] of scheduledNotificationsByHostAndNotificationId) { + if (!state.pending) { + scheduledNotificationsByHostAndNotificationId.delete(key) + evicted = true + break + } + } + if (!evicted) { + break + } + } +} + +/** Test-only: override the cap (pass no arg to restore the default). */ +export function setScheduledNotificationsMaxForTests(max?: number): void { + maxScheduledNotifications = max ?? MAX_SCHEDULED_NOTIFICATIONS +} + export type NotificationPermissionState = { granted: boolean status: string @@ -155,6 +187,7 @@ async function showLocalNotification(event: NotificationEvent, hostId: string): return } notificationState.identifier = scheduledIdentifier + boundScheduledNotifications() } finally { if (notificationState.pending === pending) { notificationState.pending = undefined