perf(mobile): cap the scheduled-notification map + tap-dedup set (#7646)
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.
This commit is contained in:
parent
d0403d808e
commit
54c4959830
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -31,10 +31,42 @@ type ScheduledNotificationState = {
|
|||
|
||||
const scheduledNotificationsByHostAndNotificationId = new Map<string, ScheduledNotificationState>()
|
||||
|
||||
// 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
|
||||
|
|
|
|||
Loading…
Reference in New Issue