diff --git a/components/transactions/BankSyncNowButton.tsx b/components/transactions/BankSyncNowButton.tsx index a6a2c177..77d74e0b 100644 --- a/components/transactions/BankSyncNowButton.tsx +++ b/components/transactions/BankSyncNowButton.tsx @@ -1,6 +1,6 @@ 'use client' -import { useEffect, useState } from 'react' +import { useEffect, useSyncExternalStore } from 'react' import { useRouter } from 'next/navigation' import { useTranslations } from 'next-intl' import { Loader2, RefreshCw } from 'lucide-react' @@ -15,17 +15,23 @@ import { } from '@/components/ui/dropdown-menu' import { createClient } from '@/lib/supabase/client' import { notifyBankSyncUpdated } from '@/lib/transactions/bank-sync-signal' +import { + claimConnectionsLoad, + clearBusyConnection, + getBankSyncSnapshot, + markConnectionStatus, + publishConnections, + releaseConnectionsLoad, + setBusyConnection, + setSyncingAll, + subscribeBankSync, + type BankConn, +} from '@/lib/transactions/bank-sync-store' import { useCompany, useCapability } from '@/contexts/CompanyContext' import { CAPABILITY } from '@/lib/entitlements/keys' import { getErrorMessage as getUserErrorMessage } from '@/lib/errors/get-error-message' -export interface BankConn { - id: string - bank_name: string - status: string - provider: string - last_synced_at: string | null -} +export type { BankConn } /** * Shared on-demand bank sync state + actions. Powers the footer "Synka nu" @@ -44,15 +50,20 @@ export function useBankSync() { const router = useRouter() const { company } = useCompany() const hasBankSync = useCapability(CAPABILITY.bank_sync) - const [connections, setConnections] = useState(null) - const [busyId, setBusyId] = useState(null) - // Holds isBusy true across the whole syncAll loop so the spinner doesn't - // flicker off between per-connection syncs. - const [syncingAll, setSyncingAll] = useState(false) + // Busy state and the connection list live in a module-level store so every + // useBankSync() instance (header split button, footer button) sees the same + // sync in flight and cannot start a concurrent one (#1162). + const store = useSyncExternalStore(subscribeBankSync, getBankSyncSnapshot, getBankSyncSnapshot) + // Never present another company's cached list while a switch is loading. + const connections = store.companyId === company?.id ? store.connections : null useEffect(() => { if (!company?.id) return - let cancelled = false + // First instance to mount claims the fetch; the rest read the store. The + // store outlives components, so the result publishes even if this + // instance unmounts mid-flight. + if (!claimConnectionsLoad(company.id)) return + const companyId = company.id const supabase = createClient() supabase .from('bank_connections') @@ -60,19 +71,20 @@ export function useBankSync() { // Include expired/error so the reconnect entry point survives a reload: // not just active connections that can sync. .in('status', ['active', 'expired', 'error']) - .eq('company_id', company.id) - .then(({ data }) => { - if (!cancelled) setConnections((data as BankConn[]) ?? []) + .eq('company_id', companyId) + .then(({ data, error }) => { + if (error) { + releaseConnectionsLoad(companyId) + return + } + publishConnections(companyId, (data as BankConn[]) ?? []) }) - return () => { - cancelled = true - } }, [company?.id]) // Re-authorize an existing connection in place: posts the connection_id so // the server reuses the same row, then hands off to the bank's consent screen. async function reconnect(conn: BankConn) { - setBusyId(conn.id) + setBusyConnection(conn.id) try { const country = conn.provider?.split('-').pop()?.toUpperCase() || 'SE' const res = await fetch('/api/extensions/ext/enable-banking/connect', { @@ -93,12 +105,12 @@ export function useBankSync() { description: error instanceof Error ? getUserErrorMessage(error) : 'Reconnect failed', variant: 'destructive', }) - setBusyId(null) + setBusyConnection(null) } } async function syncConnection(conn: BankConn) { - setBusyId(conn.id) + setBusyConnection(conn.id) try { const res = await fetch('/api/extensions/ext/enable-banking/sync', { method: 'POST', @@ -120,10 +132,9 @@ export function useBankSync() { ), }) - // Reflect the now-expired status so the button flips to reconnect. - setConnections((prev) => - (prev ?? []).map((c) => (c.id === conn.id ? { ...c, status: 'expired' } : c)) - ) + // Reflect the now-expired status so the button flips to reconnect + // on every surface at once. + markConnectionStatus(conn.id, 'expired') return } throw new Error(data.error || 'Sync failed') @@ -145,12 +156,16 @@ export function useBankSync() { variant: 'destructive', }) } finally { - setBusyId((prev) => (prev === conn.id ? null : prev)) + clearBusyConnection(conn.id) } } - // Active connections sync; expired/error connections reconnect. + // Active connections sync; expired/error connections reconnect. Reads the + // live snapshot, not the render closure, so a click racing a sync started + // from the other surface is a no-op instead of a concurrent PSD2 call. function runFor(conn: BankConn) { + const { busyId, syncingAll } = getBankSyncSnapshot() + if (busyId !== null || syncingAll) return if (conn.status === 'active') return syncConnection(conn) return reconnect(conn) } @@ -159,6 +174,8 @@ export function useBankSync() { // active connection in turn; with only dead connections it jumps straight // to re-authorizing the first one (a retry can't revive a closed session). async function syncAll() { + const { busyId, syncingAll } = getBankSyncSnapshot() + if (busyId !== null || syncingAll) return setSyncingAll(true) try { const conns = connections ?? [] @@ -184,8 +201,8 @@ export function useBankSync() { return { connections, - busyId, - isBusy: busyId !== null || syncingAll, + busyId: store.busyId, + isBusy: store.busyId !== null || store.syncingAll, hasBankSync, reconnect, syncConnection, diff --git a/lib/transactions/__tests__/bank-sync-store.test.ts b/lib/transactions/__tests__/bank-sync-store.test.ts new file mode 100644 index 00000000..45cdbcf5 --- /dev/null +++ b/lib/transactions/__tests__/bank-sync-store.test.ts @@ -0,0 +1,139 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { + claimConnectionsLoad, + clearBusyConnection, + getBankSyncSnapshot, + markConnectionStatus, + publishConnections, + releaseConnectionsLoad, + resetBankSyncStore, + setBusyConnection, + setSyncingAll, + subscribeBankSync, + type BankConn, +} from '@/lib/transactions/bank-sync-store' + +const conn = (id: string, status = 'active'): BankConn => ({ + id, + bank_name: 'Testbanken', + status, + provider: 'enablebanking-se', + last_synced_at: null, +}) + +describe('bank-sync-store', () => { + beforeEach(() => { + resetBankSyncStore() + }) + + describe('connections load claiming', () => { + it('lets only the first instance claim the fetch for a company', () => { + expect(claimConnectionsLoad('co-1')).toBe(true) + // Second surface mounting while the fetch is in flight must not refetch. + expect(claimConnectionsLoad('co-1')).toBe(false) + }) + + it('does not re-claim once connections are published', () => { + expect(claimConnectionsLoad('co-1')).toBe(true) + publishConnections('co-1', [conn('c1')]) + expect(claimConnectionsLoad('co-1')).toBe(false) + }) + + it('allows a retry after a failed load is released', () => { + expect(claimConnectionsLoad('co-1')).toBe(true) + releaseConnectionsLoad('co-1') + expect(claimConnectionsLoad('co-1')).toBe(true) + }) + + it('claims independently per company (company switch refetches)', () => { + expect(claimConnectionsLoad('co-1')).toBe(true) + publishConnections('co-1', [conn('c1')]) + expect(claimConnectionsLoad('co-2')).toBe(true) + publishConnections('co-2', [conn('c2')]) + expect(getBankSyncSnapshot().companyId).toBe('co-2') + expect(getBankSyncSnapshot().connections).toEqual([conn('c2')]) + }) + + it('discards a stale resolve after the company switched mid-flight', () => { + expect(claimConnectionsLoad('co-1')).toBe(true) // fetch for co-1 in flight + expect(claimConnectionsLoad('co-2')).toBe(true) // switch re-claims the slot + publishConnections('co-2', [conn('c2')]) + // co-1's fetch resolves late: it no longer owns the claim and must not + // clobber co-2's published list. + publishConnections('co-1', [conn('c1')]) + expect(getBankSyncSnapshot().companyId).toBe('co-2') + expect(getBankSyncSnapshot().connections).toEqual([conn('c2')]) + }) + + it('ignores a publish that never claimed the load', () => { + publishConnections('co-1', [conn('c1')]) + expect(getBankSyncSnapshot().connections).toBeNull() + }) + }) + + describe('busy state', () => { + it('shares busyId through the snapshot and notifies subscribers', () => { + const listener = vi.fn() + subscribeBankSync(listener) + setBusyConnection('c1') + expect(getBankSyncSnapshot().busyId).toBe('c1') + expect(listener).toHaveBeenCalledTimes(1) + }) + + it('clearBusyConnection only clears when that connection owns busy', () => { + setBusyConnection('c1') + clearBusyConnection('c2') + expect(getBankSyncSnapshot().busyId).toBe('c1') + clearBusyConnection('c1') + expect(getBankSyncSnapshot().busyId).toBeNull() + }) + + it('does not notify on a no-op write (stable snapshot identity)', () => { + setBusyConnection('c1') + const listener = vi.fn() + subscribeBankSync(listener) + const before = getBankSyncSnapshot() + setBusyConnection('c1') + setSyncingAll(false) + expect(listener).not.toHaveBeenCalled() + expect(getBankSyncSnapshot()).toBe(before) + }) + + it('tracks syncingAll independently of busyId', () => { + setSyncingAll(true) + expect(getBankSyncSnapshot().syncingAll).toBe(true) + expect(getBankSyncSnapshot().busyId).toBeNull() + setSyncingAll(false) + expect(getBankSyncSnapshot().syncingAll).toBe(false) + }) + }) + + describe('markConnectionStatus', () => { + it('updates one connection immutably', () => { + claimConnectionsLoad('co-1') + publishConnections('co-1', [conn('c1'), conn('c2')]) + const before = getBankSyncSnapshot().connections + markConnectionStatus('c1', 'expired') + const after = getBankSyncSnapshot().connections + expect(after).not.toBe(before) + expect(after?.find((c) => c.id === 'c1')?.status).toBe('expired') + expect(after?.find((c) => c.id === 'c2')?.status).toBe('active') + }) + + it('is a no-op before any connections are loaded', () => { + const listener = vi.fn() + subscribeBankSync(listener) + markConnectionStatus('c1', 'expired') + expect(listener).not.toHaveBeenCalled() + expect(getBankSyncSnapshot().connections).toBeNull() + }) + }) + + it('unsubscribe stops notifications', () => { + const listener = vi.fn() + const unsubscribe = subscribeBankSync(listener) + unsubscribe() + setBusyConnection('c1') + expect(listener).not.toHaveBeenCalled() + }) +}) diff --git a/lib/transactions/bank-sync-store.ts b/lib/transactions/bank-sync-store.ts new file mode 100644 index 00000000..2f7244f9 --- /dev/null +++ b/lib/transactions/bank-sync-store.ts @@ -0,0 +1,125 @@ +/** + * Module-level store for on-demand bank sync state, shared by every + * `useBankSync()` instance via `useSyncExternalStore`. + * + * Two surfaces on the transactions page trigger syncs independently: the + * "Synka bank nu" row in the Importera split button (TransactionStatusBar) + * and the footer "Synka nu" button (BankSyncNowButton). With hook-local + * state, a sync started from one surface left the other enabled and + * spinner-less, so a second concurrent sync of the same connection could be + * started (#1162). Hoisting the state here makes busy/connections identical + * across all instances, and lets the first mounted instance's fetch serve + * the rest (no duplicate `bank_connections` query per surface). + * + * Pure state + pub/sub only: the Supabase fetch stays in the hook so this + * module is trivially unit-testable. + */ + +export interface BankConn { + id: string + bank_name: string + status: string + provider: string + last_synced_at: string | null +} + +export interface BankSyncState { + /** null until the first load for `companyId` has completed */ + connections: BankConn[] | null + /** company the loaded connections belong to */ + companyId: string | null + /** id of the connection currently syncing or reconnecting */ + busyId: string | null + /** true across a whole "sync everything" run, including between connections */ + syncingAll: boolean +} + +const INITIAL_STATE: BankSyncState = { + connections: null, + companyId: null, + busyId: null, + syncingAll: false, +} + +let state: BankSyncState = INITIAL_STATE +let loadingCompanyId: string | null = null +const listeners = new Set<() => void>() + +function emit(next: BankSyncState): void { + state = next + for (const listener of listeners) listener() +} + +export function subscribeBankSync(listener: () => void): () => void { + listeners.add(listener) + return () => { + listeners.delete(listener) + } +} + +/** Stable-identity snapshot: only replaced when something actually changed. */ +export function getBankSyncSnapshot(): BankSyncState { + return state +} + +/** + * Claim the connections fetch for a company. Returns true when the caller + * should run the query (first instance to mount, or a retry after a failed + * load); false when the list is already loaded for that company or another + * instance's fetch is in flight. + */ +export function claimConnectionsLoad(companyId: string): boolean { + if (state.companyId === companyId && state.connections !== null) return false + if (loadingCompanyId === companyId) return false + loadingCompanyId = companyId + return true +} + +export function publishConnections(companyId: string, connections: BankConn[]): void { + // Only the fetch that still owns the load claim may publish. A resolve + // arriving after the active company switched (and re-claimed the slot) + // would otherwise clobber the newer company's list with stale data. + if (loadingCompanyId !== companyId) return + loadingCompanyId = null + emit({ ...state, companyId, connections }) +} + +/** Fetch failed: release the claim so a later mount can retry. */ +export function releaseConnectionsLoad(companyId: string): void { + if (loadingCompanyId === companyId) loadingCompanyId = null +} + +export function setBusyConnection(connectionId: string | null): void { + if (state.busyId === connectionId) return + emit({ ...state, busyId: connectionId }) +} + +/** Clear busy only if this connection still owns it (mirrors the old + * `setBusyId(prev => prev === id ? null : prev)` guard). */ +export function clearBusyConnection(connectionId: string): void { + if (state.busyId !== connectionId) return + emit({ ...state, busyId: null }) +} + +export function setSyncingAll(syncingAll: boolean): void { + if (state.syncingAll === syncingAll) return + emit({ ...state, syncingAll }) +} + +/** Reflect a status change (e.g. a sync hit a dead PSD2 session) on every + * surface at once. */ +export function markConnectionStatus(connectionId: string, status: string): void { + if (!state.connections) return + emit({ + ...state, + connections: state.connections.map((c) => + c.id === connectionId ? { ...c, status } : c, + ), + }) +} + +/** Reset to the initial state (tests, sign-out). */ +export function resetBankSyncStore(): void { + loadingCompanyId = null + emit(INITIAL_STATE) +}