fix(enable-banking): release ledger claims on disconnect so reconnect lands on the original account (#916) (#955)

Disconnecting a bank left its cash_accounts rows pointing at the revoked connection, so the BAS slot (e.g. 1930) looked taken forever: reconnecting the same bank was shunted to 1939 and the picker save was rejected with a 400 the user never saw. Four coordinated fixes: - DELETE /disconnect now demotes the connection's cash_accounts rows to manual (bank_connection_id = null) after marking the connection revoked. Rows are never deleted: transactions.cash_account_id and ledger history reference them, and upsertFromPsd2 promotes manual holders in place on reconnect. - findFreeLedgerAccount and the PATCH /accounts collision guard no longer count claims held by revoked connections (new getRevokedConnectionIds helper). This is the self-heal path for rows orphaned before this fix: no manual data repair needed. - upsertFromPsd2 promotes a holder row owned by a revoked connection in place (same as the manual seed row), keeping the row id stable so the ledger's transaction history stays attached. A duplicate row for the same connection+uid on an overflow slot (mirrored there by the callback while the slot was wrongly blocked) is merged: deleted when it has no linked transactions, demoted to manual otherwise. Either way a primary duplicate hands the flag to the promoted row, so the __PRIMARY_SEK__ sentinel never resolves to a deleted or stale manual row. The linked-transactions probe is company-scoped (defense in depth on the service-role client). - AccountPickerDialog surfaces rejected saves inline in the picker with the picks intact instead of routing them into the sync-progress modal. It also stops signaling the parent to close before the request resolves: the parent unmounts the whole component on close, which tore down the progress modal mid-flight and made every save outcome (including the 400) invisible. Tests: allocator revoked-exclusion + promote/merge unit tests in lib/cash-accounts, PATCH self-heal case in accounts-route.test.ts, and a new disconnect-route.test.ts covering claim release and its failure mode. Fixes #916 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-09 21:10:37 +02:00
parent 15e5dc1a01
commit 2c5e1ce317
7 changed files with 944 additions and 37 deletions
@@ -7,13 +7,15 @@ vi.mock('../lib/sync', () => ({

 // Mock the cash-accounts service (dynamically imported by the route) so the
 // mirror + allocation passes are deterministic and observable.
-const { mockUpsertFromPsd2, mockAllocate } = vi.hoisted(() => ({
+const { mockUpsertFromPsd2, mockAllocate, mockGetRevokedConnectionIds } = vi.hoisted(() => ({
  mockUpsertFromPsd2: vi.fn(),
  mockAllocate: vi.fn(),
+  mockGetRevokedConnectionIds: vi.fn(),
 }))
 vi.mock('@/lib/cash-accounts/service', () => ({
  upsertFromPsd2: (...args: unknown[]) => mockUpsertFromPsd2(...args),
  allocatePsd2LedgerAccount: (...args: unknown[]) => mockAllocate(...args),
+  getRevokedConnectionIds: (...args: unknown[]) => mockGetRevokedConnectionIds(...args),
 }))

 import { enableBankingExtension } from '../index'
@@ -157,6 +159,9 @@ describe('PATCH /accounts (enable-banking)', () => {
  beforeEach(() => {
    vi.clearAllMocks()
    mockUpsertFromPsd2.mockResolvedValue(undefined)
+    // Default: no revoked connections; individual tests override to exercise
+    // the self-heal path.
+    mockGetRevokedConnectionIds.mockResolvedValue(new Set<string>())
    // Allocator stand-in mirroring the real behavior: currency default first,
    // then the next free 1931–1959 slot (skipping other currency defaults).
    mockAllocate.mockImplementation(
@@ -916,6 +921,57 @@ describe('PATCH /accounts (enable-banking)', () => {
      expect(body.conflicting_accounts).toEqual(['1935'])
    })

+    it('allows a mapping onto a ledger held only by a REVOKED connection (self-heal after disconnect)', async () => {
+      // Issue #916: rows orphaned by a disconnect that predates the ledger
+      // claim release still point at the revoked connection. They must not
+      // count as foreign claims: the save goes through and upsertFromPsd2
+      // promotes the orphaned row in place.
+      mockedSync.mockResolvedValue({ imported: 0, duplicates: 0, errors: 0 })
+      mockGetRevokedConnectionIds.mockResolvedValue(new Set(['conn-REVOKED']))
+
+      const stub: SupabaseStub = {
+        authUser: { id: 'user-1' },
+        chartAccountNumbers: ['1930'],
+        cashAccountRows: [
+          { external_uid: 'old-acc', bank_connection_id: 'conn-REVOKED', ledger_account: '1930' },
+        ],
+        connectionRow: {
+          id: 'conn-1',
+          status: 'pending_selection',
+          accounts_data: [{ uid: 'acc-1', currency: 'SEK', enabled: true }],
+        },
+      }
+      const supabase = buildSupabase(stub)
+      const ctx = makeContext(supabase)
+
+      const res = await accountsRoute.handler(
+        makeRequest({
+          connection_id: 'conn-1',
+          enabled_uids: ['acc-1'],
+          account_mappings: [{ uid: 'acc-1', ledger_account: '1930' }],
+        }),
+        ctx
+      )
+
+      expect(res.status).toBe(200)
+      // The revoked-status lookup was scoped to the foreign connection ids.
+      expect(mockGetRevokedConnectionIds).toHaveBeenCalledWith(
+        expect.anything(),
+        'company-1',
+        ['conn-REVOKED']
+      )
+      // The mirror received the user's pick, not an overflow slot.
+      expect(mockUpsertFromPsd2).toHaveBeenCalledWith(
+        expect.anything(),
+        'company-1',
+        expect.objectContaining({
+          bank_connection_id: 'conn-1',
+          external_uid: 'acc-1',
+          ledger_account: '1930',
+        })
+      )
+    })
+
    it('allocates distinct ledgers for legacy accounts with no mapping at all', async () => {
      mockedSync.mockResolvedValue({ imported: 0, duplicates: 0, errors: 0 })

@@ -0,0 +1,219 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+
+// Mock the Enable Banking API client so revoking the PSD2 session never makes
+// a network call. SessionExpiredError must stay a real class: index.ts uses it
+// in an instanceof check.
+vi.mock('../lib/api-client', () => ({
+  startAuthorization: vi.fn(),
+  getASPSPs: vi.fn(),
+  getPreferredAuthMethod: vi.fn(),
+  deleteSession: vi.fn().mockResolvedValue(undefined),
+  isSandboxMode: vi.fn(() => true),
+  SessionExpiredError: class SessionExpiredError extends Error {},
+}))
+
+import { enableBankingExtension } from '../index'
+import { deleteSession } from '../lib/api-client'
+import type { ExtensionContext } from '@/lib/extensions/types'
+
+const mockedDeleteSession = vi.mocked(deleteSession)
+
+const disconnectRoute = enableBankingExtension.apiRoutes?.find(
+  r => r.method === 'DELETE' && r.path === '/disconnect'
+)
+
+if (!disconnectRoute) {
+  throw new Error('DELETE /disconnect route not registered on enable-banking extension')
+}
+
+interface DisconnectStub {
+  authUser: { id: string } | null
+  connectionRow: {
+    id: string
+    session_id: string | null
+    status: string
+    bank_name?: string | null
+  } | null
+  connectionError?: { message: string } | null
+  connUpdateError?: { message: string } | null
+  cashUpdateError?: { message: string } | null
+  /** Captured bank_connections update payloads. */
+  connUpdates: Array<Record<string, unknown>>
+  /** Captured cash_accounts update payloads + their eq() filters, in order. */
+  cashUpdates: Array<{ payload: Record<string, unknown>; filters: Array<[string, unknown]> }>
+}
+
+function makeStub(partial: Partial<DisconnectStub> = {}): DisconnectStub {
+  return {
+    authUser: { id: 'user-1' },
+    connectionRow: {
+      id: 'conn-1',
+      session_id: 'sess-1',
+      status: 'active',
+      bank_name: 'Lunar',
+    },
+    connUpdates: [],
+    cashUpdates: [],
+    ...partial,
+  }
+}
+
+function buildSupabase(stub: DisconnectStub) {
+  return {
+    auth: {
+      getUser: vi.fn().mockResolvedValue({ data: { user: stub.authUser }, error: null }),
+    },
+    from: vi.fn((table: string) => {
+      if (table === 'cash_accounts') {
+        return {
+          update: vi.fn((payload: Record<string, unknown>) => {
+            const filters: Array<[string, unknown]> = []
+            stub.cashUpdates.push({ payload, filters })
+            const result = Promise.resolve({ error: stub.cashUpdateError ?? null })
+            const builder = {
+              eq: vi.fn((col: string, val: unknown) => {
+                filters.push([col, val])
+                return builder
+              }),
+              then: result.then.bind(result),
+              catch: result.catch.bind(result),
+            }
+            return builder
+          }),
+        }
+      }
+      // bank_connections
+      return {
+        select: vi.fn().mockReturnThis(),
+        eq: vi.fn().mockReturnThis(),
+        single: vi.fn().mockResolvedValue({
+          data: stub.connectionRow,
+          error: stub.connectionError ?? null,
+        }),
+        update: vi.fn((payload: Record<string, unknown>) => {
+          stub.connUpdates.push(payload)
+          return { eq: vi.fn().mockResolvedValue({ error: stub.connUpdateError ?? null }) }
+        }),
+      }
+    }),
+  }
+}
+
+function makeContext(supabase: ReturnType<typeof buildSupabase>): ExtensionContext {
+  return {
+    userId: 'user-1',
+    companyId: 'company-1',
+    extensionId: 'enable-banking',
+    requestId: 'req_test',
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    supabase: supabase as any,
+    emit: vi.fn().mockResolvedValue(undefined),
+    settings: { get: vi.fn(), set: vi.fn(), getAll: vi.fn() } as never,
+    storage: {} as never,
+    log: {
+      info: vi.fn(),
+      warn: vi.fn(),
+      error: vi.fn(),
+      debug: vi.fn(),
+    } as never,
+    services: {} as never,
+  }
+}
+
+function makeRequest(body: unknown): Request {
+  return new Request('http://localhost/api/extensions/ext/enable-banking/disconnect', {
+    method: 'DELETE',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify(body),
+  })
+}
+
+describe('DELETE /disconnect (enable-banking)', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockedDeleteSession.mockResolvedValue(undefined)
+  })
+
+  it('returns 401 when unauthenticated', async () => {
+    const stub = makeStub({ authUser: null })
+    const ctx = makeContext(buildSupabase(stub))
+
+    const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx)
+    expect(res.status).toBe(401)
+  })
+
+  it('returns 404 when the connection is not found', async () => {
+    const stub = makeStub({ connectionRow: null, connectionError: { message: 'not found' } })
+    const ctx = makeContext(buildSupabase(stub))
+
+    const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx)
+    expect(res.status).toBe(404)
+  })
+
+  it('revokes the connection AND releases its cash_accounts ledger claims (issue #916)', async () => {
+    const stub = makeStub()
+    const ctx = makeContext(buildSupabase(stub))
+
+    const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx)
+
+    expect(res.status).toBe(200)
+    const body = await res.json()
+    expect(body.success).toBe(true)
+
+    // PSD2 consent revoked upstream.
+    expect(mockedDeleteSession).toHaveBeenCalledWith('sess-1')
+
+    // Connection marked revoked.
+    expect(stub.connUpdates).toEqual([{ status: 'revoked', session_id: null }])
+
+    // The connection's cash_accounts rows are demoted to manual (NOT deleted):
+    // transactions and ledger history reference them, and upsertFromPsd2
+    // promotes manual holders in place on reconnect so the same bank lands
+    // back on its original BAS account.
+    expect(stub.cashUpdates).toHaveLength(1)
+    expect(stub.cashUpdates[0].payload).toEqual({ bank_connection_id: null })
+    expect(stub.cashUpdates[0].filters).toEqual([
+      ['company_id', 'company-1'],
+      ['bank_connection_id', 'conn-1'],
+    ])
+
+    expect(ctx.emit).toHaveBeenCalledWith(
+      expect.objectContaining({
+        type: 'bank_connection.revoked',
+        payload: expect.objectContaining({ connectionId: 'conn-1', companyId: 'company-1' }),
+      })
+    )
+  })
+
+  it('still succeeds when the ledger claim release fails (self-heal covers it)', async () => {
+    // The connection is already revoked at that point; the allocator and the
+    // picker-save collision guard both skip revoked connections, so orphaned
+    // rows recover on the next picker save.
+    const stub = makeStub({ cashUpdateError: { message: 'transient' } })
+    const ctx = makeContext(buildSupabase(stub))
+
+    const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx)
+
+    expect(res.status).toBe(200)
+    const body = await res.json()
+    expect(body.success).toBe(true)
+    expect(ctx.log.error).toHaveBeenCalledWith(
+      expect.stringContaining('release cash_accounts'),
+      expect.objectContaining({ connectionId: 'conn-1' })
+    )
+  })
+
+  it('skips PSD2 session revocation when the connection has no session', async () => {
+    const stub = makeStub({
+      connectionRow: { id: 'conn-1', session_id: null, status: 'expired', bank_name: 'Lunar' },
+    })
+    const ctx = makeContext(buildSupabase(stub))
+
+    const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx)
+
+    expect(res.status).toBe(200)
+    expect(mockedDeleteSession).not.toHaveBeenCalled()
+    // Ledger claims are still released.
+    expect(stub.cashUpdates).toHaveLength(1)
+  })
+})
@@ -86,6 +86,10 @@ export function AccountPickerDialog({

  const [selected, setSelected] = useState<Set<string>>(new Set())
  const [isSaving, setIsSaving] = useState(false)
+  // Server-side save rejection (validation / ledger conflict). Shown inline in
+  // the dialog: a rejected save persisted nothing and started no sync, so the
+  // user must see why and be able to correct the picks.
+  const [saveError, setSaveError] = useState<string | null>(null)
  const [sieLastDate, setSieLastDate] = useState<string | null>(null)
  const [chartAccounts, setChartAccounts] = useState<ChartAccount[]>([])
  const [chartError, setChartError] = useState(false)
@@ -114,6 +118,7 @@ export function AccountPickerDialog({
        accounts.filter(a => a.enabled !== false).map(a => a.uid)
      )
      setSelected(initial)
+      setSaveError(null)
      setLookbackMode('fiscal-year')
      setCustomSubMode('date')
      setCustomDate('')
@@ -285,6 +290,7 @@ export function AccountPickerDialog({
    }

    setIsSaving(true)
+    setSaveError(null)

    // Cap the client wait at the route's 300s budget so a hung backfill can't
    // leave the progress modal in 'syncing' forever. The save+backfill is one
@@ -295,12 +301,15 @@ export function AccountPickerDialog({

    // For the initial-selection path, open the progress modal up-front so the
    // user has visible feedback during the 30-60s backfill. Selection edits
-    // (no backfill) keep the existing toast-only feedback.
+    // (no backfill) keep the existing toast-only feedback. Do NOT signal the
+    // parent to close here (issue #916): the parent unmounts this component on
+    // close, which would tear down the progress modal too and swallow every
+    // outcome, including a rejected save. The picker Dialog hides itself while
+    // progressOpen is true and comes back if the save is rejected.
    if (isInitialSelection) {
      setSyncAttempt((n) => n + 1)
      setProgressState({ kind: 'syncing' })
      setProgressOpen(true)
-      onOpenChange(false)
    }

    try {
@@ -326,7 +335,17 @@ export function AccountPickerDialog({
      const data = await response.json()

      if (!response.ok) {
-        throw new Error(data.error || 'Kunde inte spara kontoval')
+        // Rejected save (400 conflicting_accounts / duplicate_accounts / other
+        // validation): nothing was persisted and no sync started. Surface the
+        // server's message inside the still-open picker; the progress modal's
+        // failed state would wrongly claim "we retry in the background".
+        setSaveError(
+          typeof data?.error === 'string' && data.error
+            ? data.error
+            : 'Kunde inte spara kontoval'
+        )
+        if (isInitialSelection) setProgressOpen(false)
+        return
      }

      if (isInitialSelection && data.initial_sync) {
@@ -407,16 +426,27 @@ export function AccountPickerDialog({
      open={progressOpen}
      onOpenChange={(next) => {
        setProgressOpen(next)
-        // When the user closes the summary, propagate the saved/refresh
-        // signal to the parent (it would have been emitted on success earlier;
-        // this just guards the failure case where we still want a refresh).
-        if (!next) onSaved()
+        // When the user dismisses the progress modal the initial-selection
+        // flow is over: propagate the saved/refresh signal and close the
+        // picker. The parent unmounts this whole component on close, which is
+        // exactly why the picker must stay open until this point: closing it
+        // earlier would unmount the progress modal mid-flight and swallow the
+        // sync summary or error. (onSaved was already emitted on success;
+        // repeating it just guards the failure case where we still refresh.)
+        if (!next) {
+          onSaved()
+          onOpenChange(false)
+        }
      }}
      bankName={bankName}
      accounts={accounts.filter((a) => selected.has(a.uid))}
      state={progressState}
    />
-    <Dialog open={open} onOpenChange={onOpenChange}>
+    {/* Visually yield to the progress modal while it is up, but WITHOUT
+        signaling the parent (open stays true): the parent unmounts the
+        component on close, and a rejected save must return to a live picker
+        with the user's picks intact. */}
+    <Dialog open={open && !progressOpen} onOpenChange={onOpenChange}>
      <DialogContent className="max-w-2xl">
        <DialogHeader>
          <DialogTitle>Välj konton att synka: {bankName}</DialogTitle>
@@ -596,6 +626,15 @@ export function AccountPickerDialog({
          </div>
        )}

+        {saveError && (
+          <div
+            role="alert"
+            className="rounded-lg border border-destructive/30 bg-destructive/10 p-3 text-xs text-destructive"
+          >
+            Kontovalet sparades inte: {saveError}
+          </div>
+        )}
+
        <div className="max-h-[50vh] overflow-y-auto rounded-lg border border-border divide-y divide-border">
          {sortedAccounts.map(account => {
            const isChecked = selected.has(account.uid)
@@ -837,7 +837,7 @@ export const enableBankingExtension: Extension = {
        // cash_accounts, whose UNIQUE (company_id, ledger_account) constraint
        // would otherwise fail per-account and get swallowed, leaving accounts
        // silently unmirrored.
-        const { allocatePsd2LedgerAccount, upsertFromPsd2 } = await import(
+        const { allocatePsd2LedgerAccount, upsertFromPsd2, getRevokedConnectionIds } = await import(
          '@/lib/cash-accounts/service'
        )

@@ -857,10 +857,32 @@ export const enableBankingExtension: Extension = {
        )
        // Slots held by OTHER connections' PSD2 accounts — an explicit mapping
        // onto one of those would violate the unique constraint. Manual rows
-        // are not foreign: upsertFromPsd2 promotes them in place.
+        // are not foreign: upsertFromPsd2 promotes them in place. Rows held by
+        // a REVOKED connection are not foreign either: those are orphaned
+        // leftovers (disconnect predating the claim release, or a lost demote)
+        // and upsertFromPsd2 promotes them in place too. Excluding them here
+        // is the self-heal path for companies whose bank was disconnected
+        // before disconnect started releasing ledger claims.
+        const foreignConnectionIds = [
+          ...new Set(
+            cashRows
+              .filter(r => r.bank_connection_id !== null && r.bank_connection_id !== connection.id)
+              .map(r => r.bank_connection_id as string)
+          ),
+        ]
+        const revokedConnectionIds = await getRevokedConnectionIds(
+          supabase,
+          companyId,
+          foreignConnectionIds
+        )
        const foreignConnectedLedgers = new Set(
          cashRows
-            .filter(r => r.bank_connection_id !== null && r.bank_connection_id !== connection.id)
+            .filter(
+              r =>
+                r.bank_connection_id !== null &&
+                r.bank_connection_id !== connection.id &&
+                !revokedConnectionIds.has(r.bank_connection_id)
+            )
            .map(r => r.ledger_account)
        )

@@ -1227,6 +1249,30 @@ export const enableBankingExtension: Extension = {
          return NextResponse.json({ error: 'Failed to disconnect' }, { status: 500 })
        }

+        // Release the connection's ledger claims by demoting its cash_accounts
+        // rows to manual (bank_connection_id = null). The rows themselves stay:
+        // transactions.cash_account_id and the ledger history reference them,
+        // and upsertFromPsd2 promotes a manual holder in place on reconnect so
+        // the same bank lands back on its original BAS account (e.g. 1930)
+        // instead of overflowing to the next free slot.
+        const { error: releaseError } = await supabase
+          .from('cash_accounts')
+          .update({ bank_connection_id: null })
+          .eq('company_id', companyId)
+          .eq('bank_connection_id', connection.id)
+
+        if (releaseError) {
+          // Don't fail the disconnect: the connection is already revoked, and
+          // the allocator / collision guard also skip revoked connections, so
+          // the orphaned rows self-heal on the next picker save.
+          log.error('[enable-banking] Failed to release cash_accounts ledger claims on disconnect', {
+            errorMessage: releaseError.message,
+            connectionId: connection.id,
+            userId: user.id,
+            companyId,
+          })
+        }
+
        try {
          const emit = ctx?.emit ?? (await import('@/lib/events/bus')).eventBus.emit.bind((await import('@/lib/events/bus')).eventBus)
          await emit({