diff --git a/mobile/src/notifications/mobile-notifications.ts b/mobile/src/notifications/mobile-notifications.ts index c451fff3a..0043762e3 100644 --- a/mobile/src/notifications/mobile-notifications.ts +++ b/mobile/src/notifications/mobile-notifications.ts @@ -15,9 +15,12 @@ import { } from './local-notification-scheduling' import { adoptNotificationEpoch, + catchUpWatermarkSeq, enqueueHostDelivery, getHostNotificationSession, + quarantineCatchUpWatermark, releaseQueuedShowNotificationId, + resolveCatchUpQuarantine, saveWatermark, seedWatermarkFromStorage, seenKeyForEvent, @@ -68,7 +71,10 @@ export function subscribeToDesktopNotifications(client: RpcClient, hostId: strin releaseQueuedShowNotificationId(session, event.notificationId) } } - }) + // Why swallowed: the caller is an un-awaited handler, so a rejected show would + // surface as an unhandled rejection (a RN redbox) instead of being retried by + // the next catch-up — which is now possible, since `seen` is marked after the show. + }).catch(() => {}) } async function deliverLive( @@ -76,87 +82,127 @@ export function subscribeToDesktopNotifications(client: RpcClient, hostId: strin event: NotificationEvent | DismissNotificationEvent ): Promise { adoptNotificationEpoch(session, hostId, event.notificationEpoch) - // Why (#8129): mark seen on the live path too, so a later replay of an already-pushed id dedups instead of double-pushing. - const key = seenKeyForEvent(event) - if (key) { - session.seen.add(key) - } + const epochAtDelivery = session.lastDeliveredEpoch if (type === 'notification') { await showLocalNotification(event as NotificationEvent, hostId) } else { await dismissLocalNotification(event as DismissNotificationEvent, hostId) } + // Why after the await, exactly like the watermark below: `seen` asserts this event + // reached the user (#8129). Marked before, a rejected show leaves the key behind and + // every later replay is dropped as a duplicate — loss the quarantine cannot recover, + // since the first event to drain a batch lifts it past the one never shown. + const key = seenKeyForEvent(event) + // A mid-flight epoch adoption already cleared the counter lifetime this key indexes. + if (key && session.lastDeliveredEpoch === epochAtDelivery) { + session.seen.add(key) + } // Why after the await (#8591): the watermark is a promise that everything up // to this seq has been shown. Advancing it before the local notification lands // means a process death in between silently drops it — the next launch asks the // desktop for seq greater than one the user never saw. if (event.notificationSeq != null && event.notificationSeq > session.lastDeliveredSeq) { session.lastDeliveredSeq = event.notificationSeq - // Persisted as a pair: a seq is only trustworthy alongside the epoch it indexes. + // Why clamped: while a failed catch-up's range is still unrecovered, persisting + // the live seq would let the next catch-up ask from above the gap and the desktop + // would cut it. resolveCatchUpQuarantine writes the held-back value on success. void saveWatermark(hostId, { - seq: session.lastDeliveredSeq, + seq: catchUpWatermarkSeq(session), epoch: session.lastDeliveredEpoch }) } } + // Claimed inline rather than via queueDelivery: the batch is already one queue + // entry, and re-enqueueing per item is what let a live event cut in. + async function deliverMissedEvent( + event: NotificationEvent | DismissNotificationEvent + ): Promise { + // No pre-marking here either: deliverLive marks the key once the show lands. + const key = seenKeyForEvent(event) + if (key && session.seen.has(key)) { + return + } + if (event.type === 'notification') { + if (!shouldQueueShowForNotificationId(session, event.notificationId)) { + return + } + try { + await deliverLive('notification', event) + } finally { + releaseQueuedShowNotificationId(session, event.notificationId) + } + return + } + if (event.type === 'dismiss') { + await deliverLive('dismiss', event) + } + } + // Why: desktop cuts by seq > lastSeenSeq, so re-fetching from the watermark is idempotent (session.seen guards residual overlap). async function fetchMissed(): Promise { if (disposed) { return } + // Captured before the request: everything at or below it is known delivered, so + // it is the floor the watermark falls back to if this catch-up never completes. + const askFrom = catchUpWatermarkSeq(session) const missed = await client .sendRequest('notifications.getMissedSince', { - lastSeenSeq: session.lastDeliveredSeq, + lastSeenSeq: askFrom, // Why: sending the epoch lets the desktop reject a watermark from a counter // it no longer has and return the whole retained buffer instead of nothing. ...(session.lastDeliveredEpoch != null ? { epoch: session.lastDeliveredEpoch } : {}) }) .then((response) => { if (!response.ok) { - return [] + return null } const result = response.result as { notifications?: unknown[]; epoch?: string } | undefined adoptNotificationEpoch(session, hostId, result?.epoch) return Array.isArray(result?.notifications) ? result.notifications : [] }) - .catch(() => []) + .catch(() => null) + if (missed == null) { + // Why quarantine rather than retry: the range this catch-up abandoned stays + // unrecovered until SOME later one succeeds, and a live seq persisting past it + // meanwhile would make the desktop cut it forever. + quarantineCatchUpWatermark(session, hostId, askFrom) + return + } // Why the whole batch is ONE queue entry (#8591): awaiting per event returns to // the event loop between replays, so a live seq 11 slots into the chain between // seq 6 and 7 and persists a watermark past a notification still unshown. Why the // request stays OUTSIDE the queue: sendRequest waits up to 30s, and holding the // chain for that would stall live delivery on a slow link. await enqueueHostDelivery(session, async () => { - for (const raw of missed) { - // Re-checked per event: the batch can start before a teardown and still be - // draining after it, and a torn-down host must stop pushing. - if (disposed) { - return - } - const event = raw as NotificationEvent | DismissNotificationEvent - const key = seenKeyForEvent(event) - if (key && session.seen.has(key)) { - continue - } - if (key) { - session.seen.add(key) - } - // Claimed inline rather than via queueDelivery: the batch is already one - // queue entry, and re-enqueueing per item is what let a live event cut in. - if (event.type === 'notification') { - if (!shouldQueueShowForNotificationId(session, event.notificationId)) { - continue + // Advances only past events this batch settled, so a teardown or a failing show + // quarantines the true contiguous point instead of the range it never reached. + let contiguousSeq = askFrom + let drained = false + try { + for (const raw of missed) { + // Re-checked per event: the batch can start before a teardown and still be + // draining after it, and a torn-down host must stop pushing. + if (disposed) { + return } - try { - await deliverLive('notification', event) - } finally { - releaseQueuedShowNotificationId(session, event.notificationId) - } - } else if (event.type === 'dismiss') { - await deliverLive('dismiss', event) + const event = raw as NotificationEvent | DismissNotificationEvent + await deliverMissedEvent(event) + contiguousSeq = event.notificationSeq ?? contiguousSeq + } + drained = true + } finally { + if (drained) { + resolveCatchUpQuarantine(session, hostId) + } else { + quarantineCatchUpWatermark(session, hostId, contiguousSeq) } } - }) + // Why swallowed here: the `finally` above already recorded the contiguous point, + // and the only caller is an un-awaited 'ready' continuation — letting a failed + // show escape turns every one into an unhandled rejection (a RN redbox). + }).catch(() => {}) } seedWatermarkFromStorage(session, hostId) diff --git a/mobile/src/notifications/notification-catchup-failure-quarantine.test.ts b/mobile/src/notifications/notification-catchup-failure-quarantine.test.ts new file mode 100644 index 000000000..997b9fce9 --- /dev/null +++ b/mobile/src/notifications/notification-catchup-failure-quarantine.test.ts @@ -0,0 +1,316 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import * as Notifications from 'expo-notifications' +import { subscribeToDesktopNotifications } from './mobile-notifications' +import { resetHostNotificationSessionsForTests } from './notification-reconnect-catchup' +import type { RpcClient } from '../transport/rpc-client' +import { loadPushNotificationsEnabled } from '../storage/preferences' + +vi.mock('expo-notifications', () => ({ + AndroidImportance: { HIGH: 'high' }, + setNotificationChannelAsync: vi.fn(), + getPermissionsAsync: vi.fn(), + requestPermissionsAsync: vi.fn(), + scheduleNotificationAsync: vi.fn(), + dismissNotificationAsync: vi.fn() +})) + +vi.mock('react-native', () => ({ + Platform: { OS: 'ios', Version: 18 } +})) + +const WATERMARK_KEY = 'orca:mobileNotificationsWatermark:host-1' +const storage = new Map() + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: { + getItem: vi.fn(async (key: string) => storage.get(key) ?? null), + setItem: vi.fn(async (key: string, value: string) => { + storage.set(key, value) + }) + } +})) + +vi.mock('../storage/preferences', () => ({ + loadPushNotificationsEnabled: vi.fn() +})) + +function flushAsync(): Promise { + return new Promise((resolve) => { + setTimeout(resolve, 10) + }) +} + +function persistedSeq(): number { + return (JSON.parse(storage.get(WATERMARK_KEY) ?? '{}') as { seq?: number }).seq ?? 0 +} + +type MissedOutcome = + | { kind: 'reject' } + | { kind: 'notOk' } + | { kind: 'ok'; notifications: unknown[] } + // Rejects only once `settle()` is called, so a live event can land mid-request. + | { kind: 'heldReject' } + +function makeHostClient() { + let onData: ((data: unknown) => void) | null = null + const askedFrom: number[] = [] + let outcome: MissedOutcome = { kind: 'ok', notifications: [] } + let releaseHeld: (() => void) | null = null + const client = { + subscribe: vi.fn((_m: string, _p: unknown, cb: (data: unknown) => void) => { + onData = cb + return vi.fn(() => { + onData = null + }) + }), + getState: vi.fn(() => 'connected'), + sendRequest: vi.fn(async (method: string, params: unknown = {}) => { + if (method !== 'notifications.getMissedSince') { + return { ok: true, result: undefined } as never + } + askedFrom.push((params as { lastSeenSeq: number }).lastSeenSeq) + if (outcome.kind === 'heldReject') { + await new Promise((resolve) => { + releaseHeld = resolve + }) + throw new Error('socket closed') + } + if (outcome.kind === 'reject') { + throw new Error('socket closed') + } + if (outcome.kind === 'notOk') { + return { ok: false, error: { message: 'timeout' } } as never + } + return { ok: true, result: { notifications: outcome.notifications } } as never + }) + } + return { + client: client as unknown as RpcClient, + get onData() { + return onData + }, + askedFrom, + setOutcome(next: MissedOutcome) { + outcome = next + }, + settleHeld() { + releaseHeld?.() + } + } +} + +function notification(seq: number) { + return { + type: 'notification', + title: `m${seq}`, + body: 'b', + notificationId: `agent:${seq}`, + notificationSeq: seq + } +} + +describe('#8591 catch-up failure quarantines the watermark', () => { + beforeEach(() => { + vi.clearAllMocks() + storage.clear() + resetHostNotificationSessionsForTests() + vi.mocked(loadPushNotificationsEnabled).mockResolvedValue(true) + vi.mocked(Notifications.getPermissionsAsync).mockResolvedValue({ + status: 'granted', + canAskAgain: true + } as never) + vi.mocked(Notifications.scheduleNotificationAsync).mockResolvedValue('sched-1') + vi.mocked(Notifications.dismissNotificationAsync).mockResolvedValue(undefined) + }) + + it('keeps asking from the abandoned range until a catch-up actually succeeds', async () => { + // The phone was offline while seqs 6-7 dispatched. The catch-up that would have + // replayed them dies (socket close / timeout / ok:false), and live traffic keeps + // flowing. If a live seq is allowed to persist past 6-7, the desktop cuts by + // `seq > lastSeenSeq` on the next catch-up and they are gone for good — and the + // window stays open until some catch-up succeeds, not for one round trip. + storage.set(WATERMARK_KEY, JSON.stringify({ seq: 5, epoch: 'epoch-1' })) + const host = makeHostClient() + host.setOutcome({ kind: 'reject' }) + + subscribeToDesktopNotifications(host.client, 'host-1') + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + expect(host.askedFrom).toEqual([5]) + + host.onData?.({ ...notification(11), notificationEpoch: 'epoch-1' }) + await flushAsync() + expect(persistedSeq()).toBe(5) + + // Second catch-up also fails; the gap is still open. + host.setOutcome({ kind: 'notOk' }) + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + host.onData?.({ ...notification(12), notificationEpoch: 'epoch-1' }) + await flushAsync() + expect(host.askedFrom).toEqual([5, 5]) + expect(persistedSeq()).toBe(5) + + // Third succeeds and replays the abandoned range. + host.setOutcome({ kind: 'ok', notifications: [notification(6), notification(7)] }) + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + + expect(host.askedFrom).toEqual([5, 5, 5]) + const titles = vi + .mocked(Notifications.scheduleNotificationAsync) + .mock.calls.map((call) => (call[0] as { content: { title: string } }).content.title) + // Exact, not arrayContaining: a duplicate here is the double-push `seen` prevents. + // m11/m12 are the live events that kept flowing while the gap stayed open. + expect(titles).toEqual(['m11', 'm12', 'm6', 'm7']) + + // Only now may the watermark move past the recovered range. + expect(persistedSeq()).toBe(12) + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + expect(host.askedFrom).toEqual([5, 5, 5, 12]) + }) + + it('rolls back a watermark a live event stored while the catch-up was in flight', async () => { + // getMissedSince waits up to 30s, so live traffic routinely persists during it. + // Clamping only writes made AFTER the failure leaves that higher seq on disk, and + // the next launch reads it back and resumes past the range this catch-up abandoned. + storage.set(WATERMARK_KEY, JSON.stringify({ seq: 5, epoch: 'epoch-1' })) + const host = makeHostClient() + host.setOutcome({ kind: 'heldReject' }) + + subscribeToDesktopNotifications(host.client, 'host-1') + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + expect(host.askedFrom).toEqual([5]) + + host.onData?.({ ...notification(11), notificationEpoch: 'epoch-1' }) + await flushAsync() + expect(persistedSeq()).toBe(11) + + host.settleHeld() + await flushAsync() + expect(persistedSeq()).toBe(5) + }) + + it('quarantines at the last replayed seq when a teardown cuts the batch short', async () => { + // The batch can start before a teardown and still be draining after it, so the + // events past the interruption were never shown. A live seq arriving on the next + // connection must not persist over them. + storage.set(WATERMARK_KEY, JSON.stringify({ seq: 5, epoch: 'epoch-1' })) + const host = makeHostClient() + host.setOutcome({ + kind: 'ok', + notifications: [notification(6), notification(7), notification(8)] + }) + + let unsubscribe: (() => void) | null = null + vi.mocked(Notifications.scheduleNotificationAsync).mockImplementation(async (request) => { + if ((request as { content: { title: string } }).content.title === 'm6') { + unsubscribe?.() + } + return 'sched-1' + }) + + unsubscribe = subscribeToDesktopNotifications(host.client, 'host-1') + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + + const titles = vi + .mocked(Notifications.scheduleNotificationAsync) + .mock.calls.map((call) => (call[0] as { content: { title: string } }).content.title) + expect(titles).toEqual(['m6']) + + // A fresh subscription on the same module-scope session takes a live seq 20 before + // its own catch-up, then resumes from 6 rather than from 20. + vi.mocked(Notifications.scheduleNotificationAsync).mockResolvedValue('sched-1') + const host2 = makeHostClient() + host2.setOutcome({ kind: 'ok', notifications: [notification(7), notification(8)] }) + subscribeToDesktopNotifications(host2.client, 'host-1') + host2.onData?.({ ...notification(20), notificationEpoch: 'epoch-1' }) + await flushAsync() + expect(persistedSeq()).toBe(6) + + host2.onData?.({ type: 'ready', subscriptionId: 'sub-2', epoch: 'epoch-1' }) + await flushAsync() + + expect(host2.askedFrom).toEqual([6]) + expect( + vi + .mocked(Notifications.scheduleNotificationAsync) + .mock.calls.map((call) => (call[0] as { content: { title: string } }).content.title) + ).toEqual(['m6', 'm20', 'm7', 'm8']) + expect(persistedSeq()).toBe(20) + }) + + it('re-shows a replay whose show threw, instead of dropping it as already seen', async () => { + // The quarantine only holds the RANGE. If the failing event is also marked seen, + // the next catch-up re-fetches it and the dedup guard drops it — the banner is + // never shown, and the first later event to drain the batch lifts the quarantine + // past it. Silent loss with the watermark looking healthy. + storage.set(WATERMARK_KEY, JSON.stringify({ seq: 5, epoch: 'epoch-1' })) + const host = makeHostClient() + host.setOutcome({ kind: 'ok', notifications: [notification(6), notification(7)] }) + + let failNext = true + vi.mocked(Notifications.scheduleNotificationAsync).mockImplementation(async (request) => { + const title = (request as { content: { title: string } }).content.title + if (title === 'm6' && failNext) { + failNext = false + throw new Error('scheduling rejected') + } + return 'sched-1' + }) + + subscribeToDesktopNotifications(host.client, 'host-1') + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + expect(persistedSeq()).toBe(5) + + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + + const titles = vi + .mocked(Notifications.scheduleNotificationAsync) + .mock.calls.map((call) => (call[0] as { content: { title: string } }).content.title) + expect(titles).toEqual(['m6', 'm6', 'm7']) + expect(host.askedFrom).toEqual([5, 5]) + expect(persistedSeq()).toBe(7) + }) + + it('re-shows a live event whose show threw, instead of dropping it as already seen', async () => { + // The same hole without any catch-up failing: the live path marks seen before the + // show, so a rejected show leaves the key behind while the watermark stays put. + // The next catch-up dutifully re-fetches the seq and the guard eats it. + storage.set(WATERMARK_KEY, JSON.stringify({ seq: 5, epoch: 'epoch-1' })) + const host = makeHostClient() + host.setOutcome({ kind: 'ok', notifications: [] }) + + let failNext = true + vi.mocked(Notifications.scheduleNotificationAsync).mockImplementation(async () => { + if (failNext) { + failNext = false + throw new Error('scheduling rejected') + } + return 'sched-1' + }) + + subscribeToDesktopNotifications(host.client, 'host-1') + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + + host.onData?.({ ...notification(6), notificationEpoch: 'epoch-1' }) + await flushAsync() + expect(persistedSeq()).toBe(5) + + host.setOutcome({ kind: 'ok', notifications: [notification(6)] }) + host.onData?.({ type: 'ready', subscriptionId: 'sub-1', epoch: 'epoch-1' }) + await flushAsync() + + const titles = vi + .mocked(Notifications.scheduleNotificationAsync) + .mock.calls.map((call) => (call[0] as { content: { title: string } }).content.title) + expect(titles).toEqual(['m6', 'm6']) + expect(persistedSeq()).toBe(6) + }) +}) diff --git a/mobile/src/notifications/notification-reconnect-catchup.ts b/mobile/src/notifications/notification-reconnect-catchup.ts index 4b657f23e..de05ed695 100644 --- a/mobile/src/notifications/notification-reconnect-catchup.ts +++ b/mobile/src/notifications/notification-reconnect-catchup.ts @@ -135,6 +135,9 @@ export type HostNotificationSession = { // Counter lifetime lastDeliveredSeq belongs to; null until one is known. A // mismatch on reconnect means the desktop restarted and the watermark is void. lastDeliveredEpoch: string | null + // Highest seq known delivered CONTIGUOUSLY, frozen here while a catch-up is + // outstanding; null when none has failed. See quarantineCatchUpWatermark. + catchUpQuarantineSeq: number | null seen: ReturnType // False only until the host's first subscription reaches 'ready' — a true cold open. connectedBefore: boolean @@ -161,6 +164,7 @@ export function getHostNotificationSession(hostId: string): HostNotificationSess session = { lastDeliveredSeq: 0, lastDeliveredEpoch: null, + catchUpQuarantineSeq: null, seen: createSeenNotificationGuard(), connectedBefore: false, hadStoredWatermark: false, @@ -239,6 +243,55 @@ export function resetHostNotificationSessionsForTests(): void { sessionsByHost.clear() } +/** + * Freeze the catch-up watermark at the last seq known delivered contiguously, + * after a catch-up that did not complete. + * + * Why: live delivery advances lastDeliveredSeq unconditionally, so an abandoned + * catch-up otherwise lets the NEXT one ask from above the range it gave up on — + * the desktop cuts by seq, so those notifications are never replayed and are + * gone. Lowest wins: an earlier failure's gap is still open. + */ +export function quarantineCatchUpWatermark( + session: HostNotificationSession, + hostId: string, + contiguousSeq: number +): void { + session.catchUpQuarantineSeq = + session.catchUpQuarantineSeq == null + ? contiguousSeq + : Math.min(session.catchUpQuarantineSeq, contiguousSeq) + // Why re-persist: a live event delivered while the catch-up was still in flight + // already stored a seq above the gap. Clamping only later writes would leave that + // value on disk, so a restart still resumes past the abandoned range. + void saveWatermark(hostId, { + seq: catchUpWatermarkSeq(session), + epoch: session.lastDeliveredEpoch + }) +} + +/** Lift the quarantine once a catch-up completes, persisting what it held back. */ +export function resolveCatchUpQuarantine(session: HostNotificationSession, hostId: string): void { + if (session.catchUpQuarantineSeq == null) { + return + } + session.catchUpQuarantineSeq = null + void saveWatermark(hostId, { + seq: session.lastDeliveredSeq, + epoch: session.lastDeliveredEpoch + }) +} + +/** + * The seq a catch-up may ask from and the highest seq safe to persist — the live + * watermark, clamped to any open gap. + */ +export function catchUpWatermarkSeq(session: HostNotificationSession): number { + return session.catchUpQuarantineSeq == null + ? session.lastDeliveredSeq + : Math.min(session.catchUpQuarantineSeq, session.lastDeliveredSeq) +} + // Why (#8591): the desktop's seq counter restarts at 0 every launch, so a watermark // from a previous lifetime indexes a counter that no longer exists. Comparing it // against the fresh counter makes `lastSeenSeq >= seq` true for everything and @@ -262,6 +315,8 @@ export function adoptNotificationEpoch( // counter re-issues those same low seqs, so a stale `seq:1` would silently drop // the new counter's first bell. The dedup window belongs to one counter lifetime. session.seen.clear() + // The quarantined gap indexed the dead counter; the watermark it guarded is gone too. + session.catchUpQuarantineSeq = null session.lastDeliveredEpoch = epoch void saveWatermark(hostId, { seq: 0, epoch }) } diff --git a/src/main/jira/attachment-image-cache.test.ts b/src/main/jira/attachment-image-cache.test.ts index 4f213327c..5612f95a2 100644 --- a/src/main/jira/attachment-image-cache.test.ts +++ b/src/main/jira/attachment-image-cache.test.ts @@ -58,4 +58,40 @@ describe('attachment image cache', () => { expect(await p3).toBe('data:image/png;base64,OK==') expect(_getAttachmentImageCacheSize()).toBe(1) }) + + it('does not repopulate after "disconnect all" when the site was cleared before', async () => { + // Summed epochs read the same before a global clear (1 + 0) and after it (0 + 1). + clearAttachmentImagesForSite('site-a') + + let resolveLoad: (value: { dataUrl: string; byteSize: number } | null) => void = () => {} + const inFlight = loadAttachmentDataUrlWithCache({ + siteId: 'site-a', + attachmentId: '1', + load: () => + new Promise<{ dataUrl: string; byteSize: number } | null>((resolve) => { + resolveLoad = resolve + }) + }) + + clearAttachmentImagesForSite() + resolveLoad({ dataUrl: 'data:image/png;base64,SECRET==', byteSize: 4 }) + + // The waiter still gets its bytes; nothing survives in the cache. + expect(await inFlight).toBe('data:image/png;base64,SECRET==') + expect(getCachedAttachmentDataUrl('site-a', '1')).toBeNull() + expect(_getAttachmentImageCacheSize()).toBe(0) + }) + + it('still caches a load that spans no clear at all', async () => { + clearAttachmentImagesForSite('site-a') + + const dataUrl = await loadAttachmentDataUrlWithCache({ + siteId: 'site-a', + attachmentId: '1', + load: async () => ({ dataUrl: 'data:image/png;base64,OK==', byteSize: 2 }) + }) + + expect(dataUrl).toBe('data:image/png;base64,OK==') + expect(getCachedAttachmentDataUrl('site-a', '1')).toBe('data:image/png;base64,OK==') + }) }) diff --git a/src/main/jira/attachment-image-cache.ts b/src/main/jira/attachment-image-cache.ts index d1e0f42a5..f9fdbfe3f 100644 --- a/src/main/jira/attachment-image-cache.ts +++ b/src/main/jira/attachment-image-cache.ts @@ -15,15 +15,23 @@ type CacheEntry = { const cache = new Map() const inFlight = new Map>() // Why: mid-flight downloads must not repopulate cache after disconnect/clearToken. -let cacheEpoch = 0 +// Why ONE ticker across both scopes: summing separate counters lets distinct clear +// states collide, passing the guard and re-inserting credentialed bytes. +let epochTicker = 0 +let globalEpoch = 0 const siteEpoch = new Map() function cacheKey(siteId: string, attachmentId: string): string { return `${siteId}::${attachmentId}` } +function nextEpoch(): number { + epochTicker += 1 + return epochTicker +} + function currentEpoch(siteId: string): number { - return (siteEpoch.get(siteId) ?? 0) + cacheEpoch + return Math.max(globalEpoch, siteEpoch.get(siteId) ?? 0) } function pruneExpired(now = Date.now()): void { @@ -137,11 +145,11 @@ export function clearAttachmentImagesForSite(siteId?: string): void { if (siteId == null || siteId === '') { cache.clear() inFlight.clear() - cacheEpoch += 1 + globalEpoch = nextEpoch() siteEpoch.clear() return } - siteEpoch.set(siteId, (siteEpoch.get(siteId) ?? 0) + 1) + siteEpoch.set(siteId, nextEpoch()) const prefix = `${siteId}::` for (const key of cache.keys()) { if (key.startsWith(prefix)) { @@ -159,7 +167,8 @@ export function clearAttachmentImagesForSite(siteId?: string): void { export function _resetAttachmentImageCache(): void { cache.clear() inFlight.clear() - cacheEpoch = 0 + epochTicker = 0 + globalEpoch = 0 siteEpoch.clear() } diff --git a/src/renderer/src/web/web-clipboard-copy-fallback.test.ts b/src/renderer/src/web/web-clipboard-copy-fallback.test.ts index fe5e0a76e..e6242c11a 100644 --- a/src/renderer/src/web/web-clipboard-copy-fallback.test.ts +++ b/src/renderer/src/web/web-clipboard-copy-fallback.test.ts @@ -11,6 +11,7 @@ type FakeDocOptions = { function createFakeDocument(options?: FakeDocOptions) { const listeners: ((event: unknown) => void)[] = [] const clipboardData = { setData: vi.fn() } + const stopImmediatePropagation = vi.fn() const createElement = vi.fn() const appendChild = vi.fn() const execCommand = vi.fn((command: string) => { @@ -24,7 +25,8 @@ function createFakeDocument(options?: FakeDocOptions) { for (const listener of listeners.slice()) { listener({ clipboardData: (options?.withClipboardData ?? true) ? clipboardData : undefined, - preventDefault: vi.fn() + preventDefault: vi.fn(), + stopImmediatePropagation }) } } @@ -50,7 +52,15 @@ function createFakeDocument(options?: FakeDocOptions) { body: { appendChild } } as unknown as Document - return { doc, clipboardData, createElement, appendChild, execCommand, listeners } + return { + doc, + clipboardData, + stopImmediatePropagation, + createElement, + appendChild, + execCommand, + listeners + } } describe('copyClipboardTextViaExecCommand', () => { diff --git a/src/renderer/src/web/web-clipboard-copy-fallback.ts b/src/renderer/src/web/web-clipboard-copy-fallback.ts index 6431cf7a7..f9a2b7267 100644 --- a/src/renderer/src/web/web-clipboard-copy-fallback.ts +++ b/src/renderer/src/web/web-clipboard-copy-fallback.ts @@ -11,17 +11,19 @@ export function copyClipboardTextViaExecCommand(text: string, doc: Document = do return } event.clipboardData.setData('text/plain', text) + // Why bubble + stop: xterm's listener on terminal.element overwrites text/plain, + // and preventDefault alone does not stop it or any later window-level handler. + event.stopImmediatePropagation() event.preventDefault() served = true } - // Why capture: run before any app-level copy handler so the terminal text wins. - doc.addEventListener('copy', onCopy, true) + doc.addEventListener('copy', onCopy) try { // Chromium can return true even when no handler supplied clipboard data. return doc.execCommand('copy') === true && served } catch { return false } finally { - doc.removeEventListener('copy', onCopy, true) + doc.removeEventListener('copy', onCopy) } } diff --git a/src/renderer/src/web/web-clipboard-copy-terminal-selection.test.ts b/src/renderer/src/web/web-clipboard-copy-terminal-selection.test.ts new file mode 100644 index 000000000..35312a7e7 --- /dev/null +++ b/src/renderer/src/web/web-clipboard-copy-terminal-selection.test.ts @@ -0,0 +1,113 @@ +// @vitest-environment happy-dom +import { afterEach, describe, expect, it } from 'vitest' +import { copyClipboardTextViaExecCommand } from './web-clipboard-copy-fallback' + +// Why a real DOM: only genuine capture/bubble propagation reproduces the ordering bug. + +type ClipboardDataStub = { + setData: (format: string, value: string) => void + getData: (format: string) => string +} + +function createClipboardDataStub(): ClipboardDataStub { + const store = new Map() + return { + setData(format, value) { + store.set(format, value) + }, + getData(format) { + return store.get(format) ?? '' + } + } +} + +/** Stands in for xterm's copyHandler: bubble-phase, overwrites text/plain. */ +function mountTerminalWithSelection(selectionText: string): HTMLElement { + const terminalElement = document.createElement('div') + document.body.appendChild(terminalElement) + terminalElement.addEventListener('copy', (event) => { + const clipboardData = (event as unknown as { clipboardData?: ClipboardDataStub }).clipboardData + clipboardData?.setData('text/plain', selectionText) + event.preventDefault() + }) + return terminalElement +} + +/** Stands in for execCommand('copy'): dispatches from the DOM selection's anchor. */ +function stubExecCommand( + source: HTMLElement, + clipboardData: ClipboardDataStub, + // Runs once the fallback has registered, so a handler added here is ordered after it. + beforeDispatch?: () => void +): void { + ;(document as unknown as { execCommand: (command: string) => boolean }).execCommand = ( + command + ) => { + if (command !== 'copy') { + return false + } + beforeDispatch?.() + const event = new Event('copy', { bubbles: true, cancelable: true }) + Object.defineProperty(event, 'clipboardData', { value: clipboardData }) + source.dispatchEvent(event) + return true + } +} + +describe('web copy fallback vs. the terminal selection', () => { + afterEach(() => { + document.body.innerHTML = '' + }) + + it('copies the requested text even when the selection anchor is inside a terminal', () => { + // Copy Path / Copy Pane ID leave the selection in the terminal, so the copy event + // dispatches from there and a capture-phase write loses to xterm's bubble handler. + const terminalElement = mountTerminalWithSelection('rm -rf ./secret-dir') + const clipboardData = createClipboardDataStub() + stubExecCommand(terminalElement, clipboardData) + + expect(copyClipboardTextViaExecCommand('/Users/me/repo/src/index.ts', document)).toBe(true) + expect(clipboardData.getData('text/plain')).toBe('/Users/me/repo/src/index.ts') + }) + + it('wins over a later document-level copy handler', () => { + // Same target as the fallback's own listener, so only stopImmediatePropagation + // suppresses it — stopPropagation would still let it run and clobber text/plain. + const source = document.createElement('div') + document.body.appendChild(source) + const clipboardData = createClipboardDataStub() + const clobber = (event: Event): void => { + const data = (event as unknown as { clipboardData?: ClipboardDataStub }).clipboardData + data?.setData('text/plain', 'later document handler') + } + stubExecCommand(source, clipboardData, () => document.addEventListener('copy', clobber)) + + try { + expect(copyClipboardTextViaExecCommand('pane-42', document)).toBe(true) + expect(clipboardData.getData('text/plain')).toBe('pane-42') + } finally { + document.removeEventListener('copy', clobber) + } + }) + + it('wins over a copy handler that still runs after the document', () => { + // Bubbling hits the document before the window, so only stopImmediatePropagation + // protects against a window-level listener. + const source = document.createElement('div') + document.body.appendChild(source) + const clipboardData = createClipboardDataStub() + stubExecCommand(source, clipboardData) + const clobber = (event: Event): void => { + const data = (event as unknown as { clipboardData?: ClipboardDataStub }).clipboardData + data?.setData('text/plain', 'later window handler') + } + window.addEventListener('copy', clobber) + + try { + expect(copyClipboardTextViaExecCommand('pane-42', document)).toBe(true) + expect(clipboardData.getData('text/plain')).toBe('pane-42') + } finally { + window.removeEventListener('copy', clobber) + } + }) +}) diff --git a/src/renderer/src/web/web-preload-api.test.ts b/src/renderer/src/web/web-preload-api.test.ts index cfe578f7b..07cd4c7cd 100644 --- a/src/renderer/src/web/web-preload-api.test.ts +++ b/src/renderer/src/web/web-preload-api.test.ts @@ -70,7 +70,11 @@ function installExecCommandClipboardDocument(execCommandResult = true): { const execCommand = vi.fn((command: string) => { if (command === 'copy') { for (const listener of listeners.slice()) { - listener({ clipboardData: { setData }, preventDefault: vi.fn() }) + listener({ + clipboardData: { setData }, + preventDefault: vi.fn(), + stopImmediatePropagation: vi.fn() + }) } } return execCommandResult