fix(transactions): share bank-sync busy state across surfaces (#1163)

* fix(transactions): share bank-sync busy state across surfaces

useBankSync() kept busyId/syncingAll/connections as hook-local state, but
the header "Synka bank nu" split-button row and the footer "Synka nu"
button each call the hook independently: 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.

Hoist the state into a module-level store (lib/transactions/
bank-sync-store.ts) consumed via useSyncExternalStore, so every instance
shares busy state and the connection list:

- both surfaces spin and disable while either one syncs
- runFor/syncAll re-check the live snapshot before firing, so a click
  racing a sync from the other surface is a no-op instead of a second
  paid PSD2 call
- the bank_connections query runs once per company instead of once per
  surface (first mounted instance claims the fetch; failures release the
  claim so a later mount retries)
- a sync that hits a dead PSD2 session flips the connection to expired
  on every surface at once, not just the one that ran it

Fixes #1162.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(transactions): discard stale connection loads after company switch

publishConnections now requires the caller to still own the load claim:
a fetch resolving after the active company switched (and re-claimed the
slot) no longer clobbers the newer company's published list.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

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<BankConn[] | null>(null)
-  const [busyId, setBusyId] = useState<string | null>(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() {
               </ToastAction>
             ),
           })
-          // 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,

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()
+  })
+})

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)
+}