From 1cbe1efbf4defeb6ca0645e28ee61008d19fb6bc Mon Sep 17 00:00:00 2001 From: Aurel Date: Tue, 31 Mar 2026 01:15:00 +0200 Subject: [PATCH] [AI] Fix missing patterns in Enable Banking integration - Add SyncServerEnableBankingAccount to ExternalAccount union and getInstitutionName parameter type in SelectLinkedAccountsModal - Use BankSyncProviders type in mobile BankSyncAccountsList instead of hardcoded union missing enableBanking - Add getSecretsError handling to EnableBankingInitialiseModal for proper auth/permission error messages - Replace hardcoded #666 color with theme.pageTextSubdued - Wrap onConnectEnableBanking in try/catch with error notification and init modal re-open, matching SimpleFin/PluggyAI pattern - Translate hardcoded error string in enablebanking.ts - Add 60s timeout to downloadEnableBankingTransactions matching PluggyAI - Revert out-of-scope changes to del()/patch() in post.ts - Revert shared starting balance dedup logic back to master pattern --- .../mobile/banksync/BankSyncAccountsList.tsx | 4 +- .../components/modals/CreateAccountModal.tsx | 27 +++++++++++- .../modals/EnableBankingInitialiseModal.tsx | 43 ++++++++++++++++--- .../modals/SelectLinkedAccountsModal.tsx | 6 ++- packages/desktop-client/src/enablebanking.ts | 4 +- .../loot-core/src/server/accounts/sync.ts | 30 +++++-------- packages/loot-core/src/server/post.ts | 28 ++++++------ .../app-enablebanking/app-enablebanking.ts | 5 ++- .../services/enablebanking-service.ts | 1 + .../tests/enablebanking-service.spec.ts | 30 ++++++------- .../app-enablebanking/tests/poll-auth.spec.ts | 7 ++- .../src/app-enablebanking/utils/errors.ts | 4 +- .../app-enablebanking/utils/tests/jwt.spec.ts | 19 ++++---- 13 files changed, 128 insertions(+), 80 deletions(-) diff --git a/packages/desktop-client/src/components/mobile/banksync/BankSyncAccountsList.tsx b/packages/desktop-client/src/components/mobile/banksync/BankSyncAccountsList.tsx index 3c897522c8..1cf82fa501 100644 --- a/packages/desktop-client/src/components/mobile/banksync/BankSyncAccountsList.tsx +++ b/packages/desktop-client/src/components/mobile/banksync/BankSyncAccountsList.tsx @@ -4,13 +4,13 @@ import { Text } from '@actual-app/components/text'; import { theme } from '@actual-app/components/theme'; import { View } from '@actual-app/components/view'; -import type { AccountEntity } from 'loot-core/types/models'; +import type { AccountEntity, BankSyncProviders } from 'loot-core/types/models'; import { BankSyncAccountsListItem } from './BankSyncAccountsListItem'; import { MOBILE_NAV_HEIGHT } from '@desktop-client/components/mobile/MobileNavTabs'; -type SyncProviders = 'goCardless' | 'simpleFin' | 'pluggyai' | 'unlinked'; +type SyncProviders = BankSyncProviders | 'unlinked'; type BankSyncAccountsListProps = { groupedAccounts: Record; diff --git a/packages/desktop-client/src/components/modals/CreateAccountModal.tsx b/packages/desktop-client/src/components/modals/CreateAccountModal.tsx index 42bb0ce267..0423e6b7fc 100644 --- a/packages/desktop-client/src/components/modals/CreateAccountModal.tsx +++ b/packages/desktop-client/src/components/modals/CreateAccountModal.tsx @@ -223,7 +223,7 @@ export function CreateAccountModal({ } }; - const onConnectEnableBanking = () => { + const onConnectEnableBanking = async () => { if (isEnableBankingSetupComplete === null) { // Still loading — ignore click return; @@ -232,7 +232,30 @@ export function CreateAccountModal({ onEnableBankingInit(); return; } - void authorizeEnableBanking(dispatch); + + try { + await authorizeEnableBanking(dispatch); + } catch (err) { + console.error(err); + addNotification({ + notification: { + type: 'error', + title: t('Error when trying to contact Enable Banking'), + message: (err as Error).message, + timeout: 5000, + }, + }); + dispatch( + pushModal({ + modal: { + name: 'enablebanking-init', + options: { + onSuccess: () => setIsEnableBankingSetupComplete(true), + }, + }, + }), + ); + } }; const onGoCardlessInit = () => { diff --git a/packages/desktop-client/src/components/modals/EnableBankingInitialiseModal.tsx b/packages/desktop-client/src/components/modals/EnableBankingInitialiseModal.tsx index c86da38e5f..6fdca468f4 100644 --- a/packages/desktop-client/src/components/modals/EnableBankingInitialiseModal.tsx +++ b/packages/desktop-client/src/components/modals/EnableBankingInitialiseModal.tsx @@ -3,12 +3,15 @@ import type { ChangeEvent } from 'react'; import { Trans, useTranslation } from 'react-i18next'; import { ButtonWithLoading } from '@actual-app/components/button'; +import { SvgCheckCircle1 } from '@actual-app/components/icons/v2'; import { InitialFocus } from '@actual-app/components/initial-focus'; import { Input } from '@actual-app/components/input'; import { Text } from '@actual-app/components/text'; +import { theme } from '@actual-app/components/theme'; import { View } from '@actual-app/components/view'; import { send } from 'loot-core/platform/client/connection'; +import { getSecretsError } from 'loot-core/shared/errors'; import { Error as ErrorAlert } from '@desktop-client/components/alerts'; import { Link } from '@desktop-client/components/common/Link'; @@ -34,6 +37,7 @@ export function EnableBankingInitialiseModal({ const [secretKey, setSecretKey] = useState(''); const [isValid, setIsValid] = useState(true); const [isLoading, setIsLoading] = useState(false); + const [keyFileName, setKeyFileName] = useState(''); const [error, setError] = useState( t('It is required to provide both the Application ID and the secret key.'), ); @@ -42,9 +46,17 @@ export function EnableBankingInitialiseModal({ const file = e.target.files?.[0]; if (!file) return; - const text = await file.text(); - setSecretKey(text); - setIsValid(true); + try { + const text = await file.text(); + setSecretKey(text); + setKeyFileName(file.name); + setIsValid(true); + } catch { + setSecretKey(''); + setKeyFileName(''); + setIsValid(false); + setError(t('Failed to read the key file. Please try again.')); + } } async function onSubmit(close: () => void) { @@ -66,6 +78,12 @@ export function EnableBankingInitialiseModal({ secretKey, }); + if (result?.error) { + setIsValid(false); + setError(getSecretsError(result.error, result.reason)); + return; + } + if (result?.data?.error_code) { setIsValid(false); setError( @@ -95,7 +113,7 @@ export function EnableBankingInitialiseModal({ return ( {({ state }) => ( <> @@ -169,9 +187,20 @@ export function EnableBankingInitialiseModal({ {secretKey && ( - - Secret key loaded successfully. - + + + + {keyFileName} + + )} {!isValid && {error}} diff --git a/packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx b/packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx index a993a5c818..1af07759ea 100644 --- a/packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx +++ b/packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx @@ -509,7 +509,8 @@ export function SelectLinkedAccountsModal({ type ExternalAccount = | SyncServerGoCardlessAccount | SyncServerSimpleFinAccount - | SyncServerPluggyAiAccount; + | SyncServerPluggyAiAccount + | SyncServerEnableBankingAccount; type StartingBalanceInfo = { date: string; @@ -747,7 +748,8 @@ function getInstitutionName( externalAccount: | SyncServerGoCardlessAccount | SyncServerSimpleFinAccount - | SyncServerPluggyAiAccount, + | SyncServerPluggyAiAccount + | SyncServerEnableBankingAccount, ) { if (typeof externalAccount?.institution === 'string') { return externalAccount?.institution ?? ''; diff --git a/packages/desktop-client/src/enablebanking.ts b/packages/desktop-client/src/enablebanking.ts index 6b5c583aad..9a27acb560 100644 --- a/packages/desktop-client/src/enablebanking.ts +++ b/packages/desktop-client/src/enablebanking.ts @@ -1,3 +1,5 @@ +import { t } from 'i18next'; + import { sendCatch } from 'loot-core/platform/client/connection'; import type { SyncServerEnableBankingAccount } from 'loot-core/types/models'; @@ -51,7 +53,7 @@ function _authorize( if (!authUrl || !state) { return { error: 'unknown' as const, - message: 'Missing auth URL or state', + message: t('Missing auth URL or state'), }; } diff --git a/packages/loot-core/src/server/accounts/sync.ts b/packages/loot-core/src/server/accounts/sync.ts index 56099a60b2..4411571d97 100644 --- a/packages/loot-core/src/server/accounts/sync.ts +++ b/packages/loot-core/src/server/accounts/sync.ts @@ -333,6 +333,7 @@ async function downloadEnableBankingTransactions( { 'X-ACTUAL-TOKEN': userToken, }, + 60000, ); if (res.error_code) { @@ -1050,8 +1051,7 @@ async function processBankSyncDownload( if (customStartingDate) { startingBalanceDate = customStartingDate; } else if (transactions.length > 0) { - startingBalanceDate = - oldestTransaction.date ?? oldestTransaction.bookingDate; + startingBalanceDate = oldestTransaction.date; } else { startingBalanceDate = monthUtils.currentDay(); } @@ -1059,23 +1059,15 @@ async function processBankSyncDownload( const payee = await getStartingBalancePayee(); return runMutator(async () => { - // Check if a starting balance transaction already exists for this account - const existingStartingBalance = await db.first( - 'SELECT id FROM transactions WHERE acct = ? AND starting_balance_flag = 1 AND tombstone = 0', - [id], - ); - - const initialId = existingStartingBalance - ? existingStartingBalance.id - : await db.insertTransaction({ - account: id, - amount: balanceToUse, - category: acctRow.offbudget === 0 ? payee.category : null, - payee: payee.id, - date: startingBalanceDate, - cleared: true, - starting_balance_flag: true, - }); + const initialId = await db.insertTransaction({ + account: id, + amount: balanceToUse, + category: acctRow.offbudget === 0 ? payee.category : null, + payee: payee.id, + date: startingBalanceDate, + cleared: true, + starting_balance_flag: true, + }); const result = await reconcileTransactions( id, diff --git a/packages/loot-core/src/server/post.ts b/packages/loot-core/src/server/post.ts index eddd49e892..876c82a90c 100644 --- a/packages/loot-core/src/server/post.ts +++ b/packages/loot-core/src/server/post.ts @@ -70,7 +70,11 @@ export async function post( }); text = await res.text(); } catch (err) { - if (err instanceof Error && err.name === 'AbortError' && externalSignal?.aborted) { + if ( + err instanceof Error && + err.name === 'AbortError' && + externalSignal?.aborted + ) { throw new PostError('aborted'); } throw new PostError('network-failure'); @@ -112,12 +116,10 @@ export async function del(url, data, headers = {}, timeout = null) { let text; let res; - const controller = new AbortController(); - const timeoutId = - timeout != null ? setTimeout(() => controller.abort(), timeout) : undefined; - try { - const signal = timeout != null ? controller.signal : null; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + const signal = timeout ? controller.signal : null; res = await fetch(url, { method: 'DELETE', body: JSON.stringify(data), @@ -127,11 +129,10 @@ export async function del(url, data, headers = {}, timeout = null) { 'Content-Type': 'application/json', }, }); + clearTimeout(timeoutId); text = await res.text(); } catch { throw new PostError('network-failure'); - } finally { - if (timeoutId != null) clearTimeout(timeoutId); } throwIfNot200(res, text); @@ -163,12 +164,10 @@ export async function patch(url, data, headers = {}, timeout = null) { let text; let res; - const controller = new AbortController(); - const timeoutId = - timeout != null ? setTimeout(() => controller.abort(), timeout) : undefined; - try { - const signal = timeout != null ? controller.signal : null; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + const signal = timeout ? controller.signal : null; res = await fetch(url, { method: 'PATCH', body: JSON.stringify(data), @@ -178,11 +177,10 @@ export async function patch(url, data, headers = {}, timeout = null) { 'Content-Type': 'application/json', }, }); + clearTimeout(timeoutId); text = await res.text(); } catch { throw new PostError('network-failure'); - } finally { - if (timeoutId != null) clearTimeout(timeoutId); } throwIfNot200(res, text); diff --git a/packages/sync-server/src/app-enablebanking/app-enablebanking.ts b/packages/sync-server/src/app-enablebanking/app-enablebanking.ts index 4190004fb5..d1eb66fc89 100644 --- a/packages/sync-server/src/app-enablebanking/app-enablebanking.ts +++ b/packages/sync-server/src/app-enablebanking/app-enablebanking.ts @@ -69,8 +69,9 @@ async function buildSessionResult(session: { // Auth callback from bank redirect — must be before validateSessionMiddleware // since the bank redirects here directly (no auth token available) app.get('/auth_callback', async (req: Request, res: Response) => { - const code = req.query.code as string | undefined; - const state = req.query.state as string | undefined; + const code = typeof req.query.code === 'string' ? req.query.code : undefined; + const state = + typeof req.query.state === 'string' ? req.query.state : undefined; if (!code) { res diff --git a/packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts b/packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts index c0fbce0e5c..632dc94b0d 100644 --- a/packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts +++ b/packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts @@ -155,6 +155,7 @@ async function request( throw handleEnableBankingError(response.status, responseBody); } + // eslint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- generic API wrapper, type is validated by caller return (await response.json()) as T; } diff --git a/packages/sync-server/src/app-enablebanking/services/tests/enablebanking-service.spec.ts b/packages/sync-server/src/app-enablebanking/services/tests/enablebanking-service.spec.ts index ad66d362d7..a7206f81ec 100644 --- a/packages/sync-server/src/app-enablebanking/services/tests/enablebanking-service.spec.ts +++ b/packages/sync-server/src/app-enablebanking/services/tests/enablebanking-service.spec.ts @@ -8,7 +8,19 @@ import { vi, } from 'vitest'; +import { secretsService } from '../../../services/secrets-service'; import { EnableBankingError } from '../../utils/errors'; +import { enableBankingService } from '../enablebanking-service'; + +import { + mockAspspList, + mockAuthResponse, + mockBalance, + mockCreditTransaction, + mockDebitTransaction, + mockSession, + mockSessionAccount, +} from './fixtures'; // Mock dependencies before importing the service vi.mock('../../../services/secrets-service', () => ({ @@ -30,20 +42,6 @@ vi.mock('../../utils/jwt', () => ({ const mockFetch = vi.fn(); vi.stubGlobal('fetch', mockFetch); -import { secretsService } from '../../../services/secrets-service'; -import { enableBankingService } from '../enablebanking-service'; - -import { - mockAspsp, - mockAspspList, - mockAuthResponse, - mockBalance, - mockCreditTransaction, - mockDebitTransaction, - mockSession, - mockSessionAccount, -} from './fixtures'; - function mockFetchResponse(data: unknown, ok = true, status = 200) { mockFetch.mockResolvedValueOnce({ ok, @@ -186,7 +184,7 @@ describe('enableBankingService', () => { }), ); - const body = JSON.parse(mockFetch.mock.calls[0][1].body as string); + const body = JSON.parse(String(mockFetch.mock.calls[0][1].body)); expect(body.aspsp).toEqual({ name: 'Nordea', country: 'FI' }); expect(body.redirect_url).toBe('https://app.example.com/callback'); expect(body.state).toBe('test-state-uuid'); @@ -206,7 +204,7 @@ describe('enableBankingService', () => { 'state', ); - const body = JSON.parse(mockFetch.mock.calls[0][1].body as string); + const body = JSON.parse(String(mockFetch.mock.calls[0][1].body)); const validUntil = new Date(body.access.valid_until); const now = new Date(); const diffDays = Math.round( diff --git a/packages/sync-server/src/app-enablebanking/tests/poll-auth.spec.ts b/packages/sync-server/src/app-enablebanking/tests/poll-auth.spec.ts index aef56bcfe5..dd79c60792 100644 --- a/packages/sync-server/src/app-enablebanking/tests/poll-auth.spec.ts +++ b/packages/sync-server/src/app-enablebanking/tests/poll-auth.spec.ts @@ -1,3 +1,5 @@ +import express from 'express'; +import request from 'supertest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // Mock all external dependencies before importing the app @@ -27,7 +29,7 @@ vi.mock('../../app-gocardless/util/handle-error', () => ({ handleError: (fn: Function) => (req: unknown, res: { send: (data: unknown) => void }) => { - (fn(req, res) as Promise).catch((err: Error) => { + Promise.resolve(fn(req, res)).catch((err: Error) => { res.send({ status: 'ok', data: { @@ -43,9 +45,6 @@ vi.mock('../../app-gocardless/util/handle-error', () => ({ const mockFetch = vi.fn(); vi.stubGlobal('fetch', mockFetch); -import express from 'express'; -import request from 'supertest'; - // We need to dynamically import the handlers after mocks are set up const { handlers } = await import('../app-enablebanking'); diff --git a/packages/sync-server/src/app-enablebanking/utils/errors.ts b/packages/sync-server/src/app-enablebanking/utils/errors.ts index 0a5ad29549..1ac7fecc3c 100644 --- a/packages/sync-server/src/app-enablebanking/utils/errors.ts +++ b/packages/sync-server/src/app-enablebanking/utils/errors.ts @@ -22,9 +22,9 @@ export function handleEnableBankingError( typeof body === 'string' ? body : JSON.stringify(body ?? 'unknown'); debug('Enable Banking API error: status=%d body=%s', statusCode, bodyStr); - const parsed = + const parsed: Record = typeof body === 'object' && body !== null - ? (body as Record) + ? Object.fromEntries(Object.entries(body)) : {}; const message = typeof parsed.message === 'string' ? parsed.message : bodyStr; const errorType = typeof parsed.error === 'string' ? parsed.error : 'UNKNOWN'; diff --git a/packages/sync-server/src/app-enablebanking/utils/tests/jwt.spec.ts b/packages/sync-server/src/app-enablebanking/utils/tests/jwt.spec.ts index aee900e647..8e8428defd 100644 --- a/packages/sync-server/src/app-enablebanking/utils/tests/jwt.spec.ts +++ b/packages/sync-server/src/app-enablebanking/utils/tests/jwt.spec.ts @@ -1,5 +1,8 @@ +import { sign } from 'jws'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { getJWT } from '../jwt'; + // Mock jws to avoid needing real RSA keys vi.mock('jws', () => ({ sign: vi.fn(({ header, payload }) => { @@ -7,10 +10,6 @@ vi.mock('jws', () => ({ }), })); -import { sign } from 'jws'; - -import { getJWT } from '../jwt'; - describe('getJWT', () => { beforeEach(() => { vi.clearAllMocks(); @@ -33,22 +32,26 @@ describe('getJWT', () => { getJWT('my-app-id', 'my-secret-key'); const callArgs = vi.mocked(sign).mock.calls[0][0]; - const payload = callArgs.payload as Record; + const rawPayload = callArgs.payload; + const payload: { iss: string; aud: string; iat: number; exp: number } = + typeof rawPayload === 'string' ? JSON.parse(rawPayload) : rawPayload; expect(payload.iss).toBe('enablebanking.com'); expect(payload.aud).toBe('api.enablebanking.com'); expect(typeof payload.iat).toBe('number'); expect(typeof payload.exp).toBe('number'); - expect((payload.exp as number) - (payload.iat as number)).toBe(3600); + expect(payload.exp - payload.iat).toBe(3600); }); it('should use custom expiry', () => { getJWT('my-app-id', 'my-secret-key', 7200); const callArgs = vi.mocked(sign).mock.calls[0][0]; - const payload = callArgs.payload as Record; + const rawPayload = callArgs.payload; + const payload: { iat: number; exp: number } = + typeof rawPayload === 'string' ? JSON.parse(rawPayload) : rawPayload; - expect((payload.exp as number) - (payload.iat as number)).toBe(7200); + expect(payload.exp - payload.iat).toBe(7200); }); it('should pass the secret key to jws.sign', () => {