Integrate Enable Banking as a bank sync provider#7345
Integrate Enable Banking as a bank sync provider#7345AurelDemiri wants to merge 15 commits intoactualbudget:masterfrom
Conversation
Rewrite Enable Banking modal to match GoCardless pattern Resolve Enable Banking bugs and improve auth flow
Bug fixes:
- Fix double-negative for DBIT transaction amounts (e.g. '--25.99')
- Fix payeeName counterparty mapping (CRDT→debtor, DBIT→creditor)
- Add missing state validation in EnableBankingCallback and /auth_callback
- Fix stuck loading state in useEnableBankingStatus with try/catch/finally
- Make session-expiry error matching case-insensitive
- Prefer CLAV balance type for startingBalance in /transactions route
- Guard setTimeout in post/del/patch when timeout is null
- Distinguish abort from network failure in post() catch
Credential handling:
- Add validateCredentials() to validate before persisting secrets
- Refactor client to use enablebanking-configure instead of manual secret-set
- Distinguish null (loading) from false (not configured) in setup checks
Poll-auth robustness:
- Add unique waiter IDs to prevent superseded waiter cleanup race
- Always cache results in completedAuths for retry resilience
- Add client disconnect cleanup via res.on('close')
- Cancel poll when Enable Banking modal closes via AbortController
- Prevent concurrent poll controller race with local reference check
Code quality:
- Extract buildSessionResult() to deduplicate auth_callback/complete-auth
- Add enabled parameter to useEnableBankingStatus to skip unused requests
- Add re-entrancy guard on onJump, reset bank on country change
- Refetch bank list after Enable Banking setup completes
- Type enableBankingConfigure config, make state required in completeAuth
- Add AbortError→TIMED_OUT test, fix startAuth test assertion
- Add afterAll vi.unstubAllGlobals() for test cleanup
- Add explanatory comments for bank-per-account model and in-memory maps
- 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 actualbudget#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
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
✅ Deploy Preview for actualbudget-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Expected "sign" (value-import) to come before "Algorithm"
📝 WalkthroughWalkthroughThis PR integrates Enable Banking as a new bank synchronization provider, offering an alternative to the discontinued GoCardless service. It includes OAuth-like authentication flows, account linking interfaces, transaction synchronization handlers, and supporting infrastructure across the desktop client and sync server. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Client as Desktop Client
participant EB as Enable Banking API
participant Server as Sync Server
participant Callback as Auth Callback
participant Bank as User's Bank
User->>Client: Click "Link Bank (Enable Banking)"
Client->>Server: authorizeEnableBanking(dispatch)
Server->>Server: Open enablebanking-external-msg modal
User->>Client: Select country & bank (ASPSP)
Client->>Server: GET /enablebanking/aspsps?country=XX
Server->>EB: Fetch available ASPSPs
EB-->>Server: Return ASPSP list
Server-->>Client: Return bank options
User->>Client: Click "Link bank in browser"
Client->>Server: POST /enablebanking/start-auth<br/>{aspspId, country, redirectUrl, state}
Server->>EB: Create auth request
EB-->>Server: Return auth URL
Server-->>Client: Return auth URL & state
Client->>Client: Store state in localStorage
Client->>Client: window.open(authUrl)
Client->>Server: Begin polling POST /enablebanking/poll-auth<br/>{state}
Bank->>Bank: User authorizes consent
Bank->>Callback: Redirect to /enablebanking/auth_callback<br/>?code=AUTH_CODE&state=STATE
Callback->>Server: POST /enablebanking/complete-auth<br/>{code, state}
Server->>EB: Create session with code
EB-->>Server: Return session with accounts
Server-->>Server: Store session result for state
Server-->>Callback: Return success
Callback->>Callback: Close popup window
Server-->>Client: Poll returns account list
Client->>Client: Close polling modal
Client->>Client: Open select-linked-accounts modal
User->>Client: Review & select accounts to link
Client->>Server: POST /enablebanking/enablebanking-accounts-link<br/>{externalAccount, ...}
Server->>Server: Create/link account to Actual
Server-->>Client: Return success
Client->>Client: Navigate away, accounts synced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
packages/sync-server/src/app-enablebanking/utils/jwt.ts (1)
10-18: Consider adding explicit return type annotation.The
getJWTBodyfunction lacks an explicit return type, unlike thegetJWTHeaderfunction which has theHeadertype.♻️ Suggested improvement
+type JWTPayload = { + iss: string; + aud: string; + iat: number; + exp: number; +}; + -function getJWTBody(exp = 3600) { +function getJWTBody(exp = 3600): JWTPayload { const timestamp = Math.floor(Date.now() / 1000); return { iss: 'enablebanking.com', aud: 'api.enablebanking.com', iat: timestamp, exp: timestamp + exp, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sync-server/src/app-enablebanking/utils/jwt.ts` around lines 10 - 18, The getJWTBody function should have an explicit return type like getJWTHeader does; update getJWTBody to declare and use a matching JWT body type (e.g., JWTBody or an inline type/interface) instead of relying on inferred typing so the function signature explicitly returns that type; reference getJWTBody (and getJWTHeader/Header if needed) to locate the implementation and adjust the function signature to return the defined type that includes iss, aud, iat, and exp.packages/desktop-client/src/enablebanking.ts (1)
6-7: Switch these to@desktop-client/...imports.This file lives under
packages/desktop-client/src, so the relative imports here are out of step with the repo convention and the rest of this change.As per coding guidelines, "Use absolute imports in `desktop-client` - relative imports are enforced against by ESLint."♻️ Suggested import update
-import { pushModal } from './modals/modalsSlice'; -import type { AppDispatch } from './redux/store'; +import { pushModal } from '@desktop-client/modals/modalsSlice'; +import { type AppDispatch } from '@desktop-client/redux/store';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/enablebanking.ts` around lines 6 - 7, Update the two relative imports to use the repo's desktop-client absolute import prefix: replace the import of pushModal from './modals/modalsSlice' with an import from '@desktop-client/modals/modalsSlice', and replace the AppDispatch import from './redux/store' with one from '@desktop-client/redux/store'; keep the imported symbols (pushModal and AppDispatch) unchanged so existing references continue to work and ESLint's absolute-import rule is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-client/src/components/EnableBankingCallback.tsx`:
- Around line 17-18: Ensure the callback state value round-trips exactly by
requiring both the query param state (stateParam) and the stored value
(localStorage.getItem('enablebanking_auth_state')) to exist and be identical
before proceeding; if they match, send that state back to the backend endpoint
(enablebanking-complete-auth) along with the callback data, otherwise treat it
as an error (setStatus('error')) and do not complete the auth. Locate the state
handling logic (variables stateParam and state, and the code path that calls
enablebanking-complete-auth) and replace the current fallback behavior with an
explicit equality check and error handling so stale or injected callbacks cannot
be accepted.
In `@packages/desktop-client/src/components/modals/CreateAccountModal.tsx`:
- Around line 740-742: In CreateAccountModal (look for the JSX using
enableBankingEnabled and isEnableBankingSetupComplete that currently renders
t('Enable Banking')), change the translated provider name to the literal string
"Enable Banking" while keeping the rest of the surrounding sentence using the
translation function; i.e., remove the t(...) wrapper around the provider/brand
token so provider names like "Enable Banking" remain plain strings but keep
using t(...) for the surrounding wording in the component rendering logic.
- Around line 226-229: The Enable Banking CTA is clickable while
useEnableBankingStatus is unresolved (configuredEnableBanking === null), causing
a broken first-click; update the handlers and button props that use
onConnectEnableBanking and isEnableBankingSetupComplete to also read and pass
through the hook's isLoading (or equivalent) flag, disable the button and render
a loading/spinner state (or keep a non-actionable "Loading..." label) until
isLoading is false, and ensure the label switches correctly based on
configuredEnableBanking when resolved; apply the same change to the other
instances referenced (the blocks around the other two button locations).
- Around line 240-247: The addNotification call is creating an action object but
not dispatching it; wrap the addNotification(...) invocation with dispatch(...)
so the action is sent to the Redux store (apply the same fix for the other bare
addNotification usage around CreateAccountModal, e.g., replace
addNotification({...}) with dispatch(addNotification({...})) so the notification
toasts are actually shown).
In
`@packages/desktop-client/src/components/modals/EnableBankingExternalMsgModal.tsx`:
- Around line 154-189: onJump() holds isJumpingRef.current true for the whole
long-poll (onMoveExternal) which prevents the retry/fallback link from reopening
the bank popup; fix by starting onMoveExternal without blocking the ref — call
onMoveExternal and store the returned promise (e.g. const resPromise =
onMoveExternal({...})), then set isJumpingRef.current = false immediately so the
UI can retry, and await the stored promise (const res = await resPromise)
afterwards; keep existing try/finally cleanup and error handling around the
awaited result and use the same symbols (onJump, isJumpingRef, onMoveExternal,
onSuccess, selectedAspsp, bankOptions) to locate and update the code.
In
`@packages/desktop-client/src/components/modals/EnableBankingInitialiseModal.tsx`:
- Around line 87-94: In EnableBankingInitialiseModal.tsx the error branch
currently uses result.data.error_type (machine codes) for user-facing text;
change it to prefer the serialized human message (result.data.message) or a
local mapping from known error_type values to friendly strings, falling back to
the existing generic t(...) fallback; update the setError call that references
result.data.error_type to use result.data.message || mappedMessage || t('Could
not validate the credentials. Please check your Application ID and secret key.')
and ensure setIsValid(false) remains unchanged.
In `@packages/desktop-client/src/enablebanking.ts`:
- Around line 68-87: The code treats any successful HTTP response as success
even when the body contains an error; update the poll handling after
sendCatch('enablebanking-poll-auth', ...) to check pollResp.data or pollData for
body-level errors (e.g. pollData?.data?.error or pollData?.error) before
extracting accounts: if a body error exists handle timeout (return { error:
'timeout' }) or return the same { error: 'unknown', message: ... } shape as the
top-level error branch, otherwise proceed to set accounts from
pollData?.data?.accounts ?? pollData?.accounts ?? []; this ensures sendCatch,
pollResp, pollData and accounts are validated correctly before opening the
account picker.
In `@packages/loot-core/src/server/accounts/app.ts`:
- Around line 980-1019: The global enableBankingPollController causes races
between concurrent auth flows; change to track controllers per flow (e.g., a
Map<string, AbortController> keyed by the incoming state or a generated flowId).
In enableBankingPollAuth create a new AbortController and store it in the map
under the state (or return a flowId to the caller), use controller.signal in the
post call, and in the finally block only delete the map entry for that
state/flowId if it still points to the same controller. Update
stopEnableBankingPollAuth to accept the state/flowId and lookup+abort only that
controller (then remove it from the map); remove the old process-wide
enableBankingPollController variable and replace usages with the per-flow map.
Ensure function names referenced: enableBankingPollController (remove),
enableBankingPollAuth, stopEnableBankingPollAuth are updated accordingly.
- Around line 925-933: The validation treats maxConsentValidity as days but the
rest of the flow (modal forwarding aspsp.maximum_consent_validity and
enableBankingService.startAuth()) uses seconds, so update the check in the
maxConsentValidity validation (the variable maxConsentValidity in this block) to
compare against seconds not days: keep the Number.isFinite/Number.isInteger and
>0 checks, but replace the upper bound 3650 with 3650 * 24 * 60 * 60 (or an
equivalent seconds constant) so values representing 30–90 days in seconds (e.g.,
2_592_000–7_776_000) are accepted. Ensure the error string remains
'invalid_max_consent_validity'.
In
`@packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts`:
- Around line 211-214: The current normalization always strips the original sign
then applies a sign based solely on tx.credit_debit_indicator, which loses a
leading '-' when the indicator is undefined; change the logic around
rawAmount/signedAmount so you first read amtStr =
tx.transaction_amount.amount.trim(), then if tx.credit_debit_indicator is
undefined preserve the original sign (only remove a leading '+' but keep a
leading '-'), otherwise remove any leading sign and apply '-' when
tx.credit_debit_indicator === 'DBIT' and no prefix when it's a credit; update
the variables rawAmount and signedAmount accordingly to use
tx.transaction_amount.amount, tx.credit_debit_indicator, and the adjusted
amtStr.
- Around line 367-393: The loop detects repeated continuation keys one request
too late because it compares result.continuation_key to previousContinuationKey
(which lags two iterations); change the logic to compare the newly returned
result.continuation_key against the current continuationKey (the key used for
this request) immediately after the fetch, and break if they match, then update
continuationKey = result.continuation_key; remove or repurpose
previousContinuationKey since comparing to continuationKey is sufficient. This
involves modifying the do/while in enablebanking-service.ts around
getTransactions, replacing the if that checks result.continuation_key ===
previousContinuationKey with a check result.continuation_key === continuationKey
and moving/updating the assignments so previousContinuationKey is no longer
needed.
In `@upcoming-release-notes/7345.md`:
- Line 6: The sentence "Integrate Enable Banking as bank sync provider" is
missing the article "a"; update that line (the string "Integrate Enable Banking
as bank sync provider") to read "Integrate Enable Banking as a bank sync
provider" to fix the grammatical error.
---
Nitpick comments:
In `@packages/desktop-client/src/enablebanking.ts`:
- Around line 6-7: Update the two relative imports to use the repo's
desktop-client absolute import prefix: replace the import of pushModal from
'./modals/modalsSlice' with an import from '@desktop-client/modals/modalsSlice',
and replace the AppDispatch import from './redux/store' with one from
'@desktop-client/redux/store'; keep the imported symbols (pushModal and
AppDispatch) unchanged so existing references continue to work and ESLint's
absolute-import rule is satisfied.
In `@packages/sync-server/src/app-enablebanking/utils/jwt.ts`:
- Around line 10-18: The getJWTBody function should have an explicit return type
like getJWTHeader does; update getJWTBody to declare and use a matching JWT body
type (e.g., JWTBody or an inline type/interface) instead of relying on inferred
typing so the function signature explicitly returns that type; reference
getJWTBody (and getJWTHeader/Header if needed) to locate the implementation and
adjust the function signature to return the defined type that includes iss, aud,
iat, and exp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 258a7a19-91d0-4e21-96cb-6afbf1c9f709
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
packages/desktop-client/src/accounts/mutations.tspackages/desktop-client/src/components/EnableBankingCallback.tsxpackages/desktop-client/src/components/FinancesApp.tsxpackages/desktop-client/src/components/Modals.tsxpackages/desktop-client/src/components/accounts/AccountSyncCheck.tsxpackages/desktop-client/src/components/banksync/index.tsxpackages/desktop-client/src/components/mobile/banksync/BankSyncAccountsList.tsxpackages/desktop-client/src/components/mobile/banksync/MobileBankSyncPage.tsxpackages/desktop-client/src/components/modals/CreateAccountModal.tsxpackages/desktop-client/src/components/modals/EnableBankingExternalMsgModal.tsxpackages/desktop-client/src/components/modals/EnableBankingInitialiseModal.tsxpackages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/enablebanking.tspackages/desktop-client/src/hooks/useEnableBankingStatus.tspackages/desktop-client/src/hooks/useFeatureFlag.tspackages/desktop-client/src/modals/modalsSlice.tspackages/desktop-client/vite.config.tspackages/loot-core/src/server/accounts/app.tspackages/loot-core/src/server/accounts/sync.tspackages/loot-core/src/server/post.tspackages/loot-core/src/server/server-config.tspackages/loot-core/src/types/models/account.tspackages/loot-core/src/types/models/bank-sync.tspackages/loot-core/src/types/models/enablebanking.tspackages/loot-core/src/types/models/index.tspackages/loot-core/src/types/prefs.tspackages/sync-server/package.jsonpackages/sync-server/src/app-enablebanking/app-enablebanking.tspackages/sync-server/src/app-enablebanking/services/enablebanking-service.tspackages/sync-server/src/app-enablebanking/services/tests/enablebanking-service.spec.tspackages/sync-server/src/app-enablebanking/services/tests/fixtures.tspackages/sync-server/src/app-enablebanking/services/tests/normalization.spec.tspackages/sync-server/src/app-enablebanking/tests/poll-auth.spec.tspackages/sync-server/src/app-enablebanking/utils/errors.tspackages/sync-server/src/app-enablebanking/utils/jwt.tspackages/sync-server/src/app-enablebanking/utils/tests/errors.spec.tspackages/sync-server/src/app-enablebanking/utils/tests/jwt.spec.tspackages/sync-server/src/app.tspackages/sync-server/src/services/secrets-service.jsupcoming-release-notes/7345.md
packages/desktop-client/src/components/EnableBankingCallback.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/CreateAccountModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/CreateAccountModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/EnableBankingExternalMsgModal.tsx
Show resolved
Hide resolved
packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts
Outdated
Show resolved
Hide resolved
packages/sync-server/src/app-enablebanking/services/enablebanking-service.ts
Show resolved
Hide resolved
|
Thanks for taking this on! Looking forward to reviewing it and merging! But first things first: can you go through the coderabbit comments? Some of those are valid things to fix. Others - you can disregard (but then reply back to the bot and tell him why; he will then resolve the comment). Eventually coderabbitai will approve and that's when I (we) step in for a final review :) |
5c7c70d to
d262f7d
Compare
a04e74e to
0cafb4a
Compare
|
One thing I'd like us to test before merging: what happens when switching an account from GoCardless to Enable Banking? Specifically, this involves importing transactions into an account that already has GoCardless-imported data. I don't have a GoCardless account to test this myself. Could someone with access verify that the transition works cleanly (no duplicate transactions, correct balancing, etc.)? |
This shouldn't be much of a problem, and only needs fixed once. Reconcile the account, protecting the old transacitons from being changed, then delete any duplicate transactions that come in. The bank sync settings will have the option to disable reimporting deleted transactions. |
Description
Alternative implementation of Enable Banking as a bank sync provider (EU banks), replacing the approach in #5570 with a leaner architecture and fewer dependencies. This implementation tries its best to copy the patterns and behavior from the existing bank sync integrations.
Improvements over PR #5570 are:
My React knowledge is fairly limited. I’m more comfortable with Vue or Angular, so I’d appreciate any comments.
Related issue(s)
Fixes #5445, Fixes #5505
Supersedes PR #5570
Testing
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
src/components/modals/EnableBankingExternalMsgModal.tsxsrc/components/modals/EnableBankingInitialiseModal.tsxsrc/components/EnableBankingCallback.tsxsrc/enablebanking.tssrc/hooks/useEnableBankingStatus.tssrc/components/budget/goals/actions.tssrc/components/modals/CreateAccountModal.tsxsrc/accounts/mutations.tssrc/hooks/useFeatureFlag.tssrc/components/Modals.tsxsrc/components/modals/SelectLinkedAccountsModal.tsxsrc/components/settings/Experimental.tsxsrc/components/FinancesApp.tsxsrc/components/accounts/AccountSyncCheck.tsxsrc/components/banksync/index.tsxsrc/components/mobile/banksync/MobileBankSyncPage.tsxsrc/components/alerts.tsxView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/accounts/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/post.tshome/runner/work/actual/actual/packages/loot-core/src/server/server-config.tshome/runner/work/actual/actual/packages/loot-core/src/server/accounts/sync.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/accounts/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/post.tshome/runner/work/actual/actual/packages/loot-core/src/server/server-config.tshome/runner/work/actual/actual/packages/loot-core/src/server/accounts/sync.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged