From 6c8d394f869f69fef0557537a6efc86bc20b6529 Mon Sep 17 00:00:00 2001 From: Nathan Disidore Date: Wed, 12 Aug 2026 10:10:58 -0500 Subject: [PATCH] fix(frontend): dispose pending account subscriptions --- .../src/BlueprintLandingPage.tsx | 19 +++++-------- .../workshop-frontend/src/GatekeeperModal.tsx | 20 +++++--------- .../src/ObserverConfigModal.test.tsx | 27 ++++++++++++++++--- .../src/ObserverConfigModal.tsx | 27 +++++++++---------- .../src/OnboardingWizard.tsx | 19 +++++-------- .../src/components/ConnectionChips.tsx | 13 +++------ .../src/routes/gatekeepers.tsx | 25 ++++++----------- 7 files changed, 66 insertions(+), 84 deletions(-) diff --git a/packages/workshop-frontend/src/BlueprintLandingPage.tsx b/packages/workshop-frontend/src/BlueprintLandingPage.tsx index 646dc0489..0afa6c67c 100644 --- a/packages/workshop-frontend/src/BlueprintLandingPage.tsx +++ b/packages/workshop-frontend/src/BlueprintLandingPage.tsx @@ -156,7 +156,6 @@ export default function BlueprintLandingPage({ rpcStub }: Props) { } let cancelled = false const accountMap = new Map() - let subStub: { [Symbol.dispose](): void } | null = null const subscriber = new AccountsSubscriberAdapter({ add({ id: accountId, description, vendor, supportedResources, credentialsValid, vendorId }) { @@ -177,21 +176,15 @@ export default function BlueprintLandingPage({ rpcStub }: Props) { }, }) - authenticatedApi.subscribeConnectedAccounts(subscriber) - .then(stub => { - if (cancelled) { - stub[Symbol.dispose]() - } else { - subStub = stub - } - }) - .catch(err => { - logRpcFailure('Failed to subscribe to connected accounts:', err) - }) + const subscription = authenticatedApi.subscribeConnectedAccounts(subscriber) + subscription.catch(err => { + if (cancelled) return + logRpcFailure('Failed to subscribe to connected accounts:', err) + }) return () => { cancelled = true - subStub?.[Symbol.dispose]() + subscription[Symbol.dispose]() } }, [isAuthenticated, authenticatedApi]) diff --git a/packages/workshop-frontend/src/GatekeeperModal.tsx b/packages/workshop-frontend/src/GatekeeperModal.tsx index b7edddcd8..50bd57126 100644 --- a/packages/workshop-frontend/src/GatekeeperModal.tsx +++ b/packages/workshop-frontend/src/GatekeeperModal.tsx @@ -216,7 +216,6 @@ export default function GatekeeperModal({ const spawnerEnvCandidatesRef = useRef(spawnerEnvCandidates) spawnerEnvCandidatesRef.current = spawnerEnvCandidates - const accountSubscriptionRef = useRef<{ [Symbol.dispose](): void } | null>(null) const configuratorFrameRef = useRef(null) const configuratorCollectResourceUrlRef = useRef<(() => Promise) | null>(null) const nextConfiguratorFrameKeyRef = useRef(0) @@ -420,22 +419,15 @@ export default function GatekeeperModal({ setAccounts(Array.from(accountMap.values())) }, }) - authenticatedApi.subscribeConnectedAccounts(subscriber) - .then(stub => { - if (cancelled) { - stub[Symbol.dispose]() - } else { - accountSubscriptionRef.current = stub - } - }) - .catch(error => { - logRpcFailure('Failed to subscribe to connected accounts:', error) - }) + const subscription = authenticatedApi.subscribeConnectedAccounts(subscriber) + subscription.catch(error => { + if (cancelled) return + logRpcFailure('Failed to subscribe to connected accounts:', error) + }) return () => { cancelled = true - accountSubscriptionRef.current?.[Symbol.dispose]() - accountSubscriptionRef.current = null + subscription[Symbol.dispose]() } }, [open, authenticatedApi]) diff --git a/packages/workshop-frontend/src/ObserverConfigModal.test.tsx b/packages/workshop-frontend/src/ObserverConfigModal.test.tsx index 25501585d..83226dd67 100644 --- a/packages/workshop-frontend/src/ObserverConfigModal.test.tsx +++ b/packages/workshop-frontend/src/ObserverConfigModal.test.tsx @@ -79,6 +79,9 @@ function account(id: number, uniqueName: string, grantedResourceUrlPatterns?: st } type ApiOverrides = { + subscribeConnectedAccounts?: Mock<( + subscriber: ConnectedAccountsSubscriber, + ) => Promise<{ [Symbol.dispose](): void }>> connectAccount?: Mock<(vendorId: string, resourceUrlPatterns?: string[]) => Promise<{ url: string }>> ensureAccountResources?: Mock<( accountId: number, @@ -92,13 +95,15 @@ function fakeApi( overrides: ApiOverrides = {}, ): RpcStub { return { - subscribeConnectedAccounts: async (subscriber: ConnectedAccountsSubscriber) => { + subscribeConnectedAccounts: overrides.subscribeConnectedAccounts ?? ((subscriber: ConnectedAccountsSubscriber) => { for (const entry of accountEntries) { subscriber.add(entry.id, entry.description, VENDOR, [DOC_RESOURCE], true, 'google') } subscriber.ready() - return { [Symbol.dispose]() {} } - }, + return Object.assign(Promise.resolve({ [Symbol.dispose]() {} }), { + [Symbol.dispose]() {}, + }) + }), listGatekeeperVendors: async () => [{ id: 'google', description: VENDOR, @@ -157,6 +162,22 @@ describe('ObserverConfigModal account selection', () => { expect(rendered.querySelector('[data-testid="account-select"]')).toBeNull() }) + it('disposes a pending account subscription on unmount', async () => { + const dispose = vi.fn<() => void>() + const pendingSubscription = Object.assign(new Promise<{ [Symbol.dispose](): void }>(() => {}), { + [Symbol.dispose]: dispose, + }) + const subscribeConnectedAccounts = vi.fn< + (subscriber: ConnectedAccountsSubscriber) => Promise<{ [Symbol.dispose](): void }> + >().mockReturnValue(pendingSubscription) + await render([], { api: fakeApi([], { subscribeConnectedAccounts }) }) + + act(() => root!.unmount()) + root = undefined + + expect(dispose).toHaveBeenCalledOnce() + }) + it('keeps the account dropdown when multiple accounts match', async () => { const rendered = await render([ account(1, 'dan@cloudflare.com'), diff --git a/packages/workshop-frontend/src/ObserverConfigModal.tsx b/packages/workshop-frontend/src/ObserverConfigModal.tsx index 26cad4eaa..17055766b 100644 --- a/packages/workshop-frontend/src/ObserverConfigModal.tsx +++ b/packages/workshop-frontend/src/ObserverConfigModal.tsx @@ -100,11 +100,11 @@ export default function ObserverConfigModal({ // ── subscribe to the user's connected accounts ──────────────────────────────── useEffect(() => { - let subStub: { [Symbol.dispose](): void } | null = null let cancelled = false const subscriber = new AccountsSubscriberAdapter({ add({ id, description, vendor, supportedResources, credentialsValid, vendorId }) { + if (cancelled) return setAccounts(prev => { const next = new Map(prev) next.set(id, { id, description, vendor, vendorId, supportedResources, credentialsValid }) @@ -121,6 +121,7 @@ export default function ObserverConfigModal({ } }, remove(id) { + if (cancelled) return setAccounts(prev => { if (!prev.has(id)) return prev const next = new Map(prev) @@ -129,26 +130,24 @@ export default function ObserverConfigModal({ }) }, ready() { + if (cancelled) return setReady(true) }, }) - authenticatedApi - .subscribeConnectedAccounts(subscriber, { includeForcedAutoProvisionedAccounts: true }) - .then(stub => { - if (cancelled) { stub[Symbol.dispose](); return } - subStub = stub - }) - .catch(err => { - // Loud on purpose: the modal has no retry path, so a quieted transient failure would - // strand the user on a permanent loader. - console.error('Failed to subscribe to connected accounts:', err) - toasts.add({ title: 'Failed to load your connected accounts', variant: 'error' }) - }) + const subscription = authenticatedApi.subscribeConnectedAccounts( + subscriber, { includeForcedAutoProvisionedAccounts: true }) + subscription.catch(err => { + if (cancelled) return + // Loud on purpose: the modal has no retry path, so a quieted transient failure would + // strand the user on a permanent loader. + console.error('Failed to subscribe to connected accounts:', err) + toasts.add({ title: 'Failed to load your connected accounts', variant: 'error' }) + }) return () => { cancelled = true - subStub?.[Symbol.dispose]() + subscription[Symbol.dispose]() } }, [authenticatedApi]) diff --git a/packages/workshop-frontend/src/OnboardingWizard.tsx b/packages/workshop-frontend/src/OnboardingWizard.tsx index 9fab36e29..b4129adca 100644 --- a/packages/workshop-frontend/src/OnboardingWizard.tsx +++ b/packages/workshop-frontend/src/OnboardingWizard.tsx @@ -206,23 +206,16 @@ export default function OnboardingWizard({ } }, }) - let subscriptionStub: { [Symbol.dispose](): void } | null = null - authenticatedApi.subscribeConnectedAccounts(subscriber) - .then((stub) => { - if (cancelled) { - stub[Symbol.dispose]() - } else { - subscriptionStub = stub - } - }) - .catch((err) => { - logRpcFailure('Failed to subscribe to connected accounts:', err) - }) + const subscription = authenticatedApi.subscribeConnectedAccounts(subscriber) + subscription.catch((err) => { + if (cancelled) return + logRpcFailure('Failed to subscribe to connected accounts:', err) + }) return () => { cancelled = true - subscriptionStub?.[Symbol.dispose]() + subscription[Symbol.dispose]() } }, [authenticatedApi]) diff --git a/packages/workshop-frontend/src/components/ConnectionChips.tsx b/packages/workshop-frontend/src/components/ConnectionChips.tsx index fd8879f98..c3dba2367 100644 --- a/packages/workshop-frontend/src/components/ConnectionChips.tsx +++ b/packages/workshop-frontend/src/components/ConnectionChips.tsx @@ -20,7 +20,6 @@ export default function ConnectionChips() { useEffect(() => { let cancelled = false - let subscriptionStub: { [Symbol.dispose](): void } | null = null const accountMap = new Map() @@ -39,18 +38,12 @@ export default function ConnectionChips() { if (!cancelled) setAccounts(Array.from(accountMap.values())) }, }) - const subPromise = authenticatedApi.subscribeConnectedAccounts(subscriber) - subPromise.then((stub) => { - if (cancelled) { - stub[Symbol.dispose]() - } else { - subscriptionStub = stub - } - }).catch(() => {}) + const subscription = authenticatedApi.subscribeConnectedAccounts(subscriber) + subscription.catch(() => {}) return () => { cancelled = true - subscriptionStub?.[Symbol.dispose]() + subscription[Symbol.dispose]() } }, [authenticatedApi]) diff --git a/packages/workshop-frontend/src/routes/gatekeepers.tsx b/packages/workshop-frontend/src/routes/gatekeepers.tsx index 5bb557658..5902f17ea 100644 --- a/packages/workshop-frontend/src/routes/gatekeepers.tsx +++ b/packages/workshop-frontend/src/routes/gatekeepers.tsx @@ -1,6 +1,6 @@ import { logRpcFailure } from '../rpcErrors' import { createFileRoute } from '@tanstack/react-router' -import { useEffect, useMemo, useRef, useState } from 'react' +import { useEffect, useMemo, useState } from 'react' import { useKumoToastManager } from '@cloudflare/kumo' import { MagnifyingGlass, @@ -474,8 +474,6 @@ function ConnectorsPage() { localStorage.setItem('gatekeepers-view', view) }, [view]) - const subscriptionRef = useRef<{ [Symbol.dispose](): void } | null>(null) - useEffect(() => { let cancelled = false const accountMap = new Map() @@ -539,23 +537,16 @@ function ConnectorsPage() { }, }) - authenticatedApi.subscribeConnectedAccounts(subscriber) - .then((stub) => { - if (cancelled) { - stub[Symbol.dispose]() - } else { - subscriptionRef.current = stub - } - }) - .catch((err) => { - logRpcFailure('Failed to subscribe to connected accounts:', err) - if (!cancelled) setLoadError(true) - }) + const subscription = authenticatedApi.subscribeConnectedAccounts(subscriber) + subscription.catch((err) => { + if (cancelled) return + logRpcFailure('Failed to subscribe to connected accounts:', err) + setLoadError(true) + }) return () => { cancelled = true - subscriptionRef.current?.[Symbol.dispose]() - subscriptionRef.current = null + subscription[Symbol.dispose]() } }, [authenticatedApi])