Custom mapping and import settings for bank sync providers #4253
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
matt-fidd
force-pushed
the
bank-sync
branch
2 times, most recently
from
f37d495 to
5118830
Compare
|
/update-vrt |
matt-fidd
changed the title
WalkthroughThis pull request introduces a set of changes across the desktop client and server codebases to enhance bank synchronization functionality. On the desktop client side, a new route ( Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (21)
packages/loot-core/src/server/accounts/sync.ts (4)
355-356: Skipping non-cleared transactions
IfimportPendingis false, non-cleared transactions are skipped altogether. Consider how these pending transactions might be handled if the user later enables pending imports.
369-369: Confirm the strict requirement for a 'date'
Throwing an error ifdateis missing halts the entire sync. Consider skipping or partially applying such transactions instead of stopping the process.
377-377: Ensure empty strings are replaced
Using|| payeeNamewon't replacetrans.imported_payeeif it's an empty string. A trim-based fallback may help unify the logic.
389-389: Assess storing raw synced data
raw_synced_datamay include personal or sensitive data. Consider encrypting it, partially redacting it, or limiting its size to avoid potential data exposure or excessive bloat.
[security]Also applies to: 394-394, 395-395, 400-400
packages/loot-core/src/types/models/account.d.ts (1)
21-21: Consider using a typed date
Iflast_syncis meant for date operations, storing it as a string may limit type-safety. Evaluate using an ISO datetime or a dedicated date type.packages/loot-core/src/types/models/transaction.d.ts (1)
35-35: Simplify the type definition.The type definition can be simplified since the
undefinedtype is redundant for optional properties.- raw_synced_data?: string | undefined; + raw_synced_data?: string;packages/loot-core/src/server/util/custom-sync-mapping.ts (1)
21-38: Refactor default mappings to reduce duplication.The mappings for 'payment' and 'deposit' are identical. Consider extracting the common mappings to reduce duplication.
+const commonFieldMappings = new Map([ + ['date', 'date'], + ['payee', 'payeeName'], + ['notes', 'remittanceInformationUnstructured'], +]); + export const defaultMappings: Mappings = new Map([ - [ - 'payment', - new Map([ - ['date', 'date'], - ['payee', 'payeeName'], - ['notes', 'remittanceInformationUnstructured'], - ]), - ], - [ - 'deposit', - new Map([ - ['date', 'date'], - ['payee', 'payeeName'], - ['notes', 'remittanceInformationUnstructured'], - ]), - ], + ['payment', new Map(commonFieldMappings)], + ['deposit', new Map(commonFieldMappings)], ]);packages/desktop-client/src/components/banksync/AccountsHeader.tsx (2)
14-14: Remove empty style object.The empty style object on TableHeader is unnecessary.
- <TableHeader style={{}}> + <TableHeader>
15-34: Enhance accessibility and reduce style duplication.The table header cells need aria labels for better accessibility. Also, the padding style is duplicated across cells.
+const cellStyle = { paddingLeft: '10px' }; + <Cell value={t('Account')} width={!unlinked ? 250 : 'flex'} - style={{ paddingLeft: '10px' }} + style={cellStyle} + aria-label={t('Account column header')} /> {!unlinked && ( <> <Cell value={t('Bank')} width="flex" - style={{ paddingLeft: '10px' }} + style={cellStyle} + aria-label={t('Bank column header')} /> <Cell value={t('Last sync')} width={160} - style={{ paddingLeft: '10px' }} + style={cellStyle} + aria-label={t('Last sync column header')} /> - <Cell value="" width={100} style={{ paddingLeft: '10px' }} /> + <Cell value="" width={100} style={cellStyle} /> </> )}packages/desktop-client/src/components/banksync/AccountsList.tsx (2)
9-14: Use literal union type for action.The action type can be more strictly typed using a literal union type.
+type AccountAction = 'link' | 'edit'; + type AccountsListProps = { accounts: AccountEntity[]; hoveredAccount?: string | undefined; onHover: (id: AccountEntity['id']) => void; - onAction: (account: AccountEntity, action: 'link' | 'edit') => void; + onAction: (account: AccountEntity, action: AccountAction) => void; };
28-40: Optimize performance for large lists.For better performance with large account lists, consider memoizing the AccountRow components and callback functions.
+import React, { useCallback, useMemo } from 'react'; + +const MemoizedAccountRow = React.memo(AccountRow); + export function AccountsList({ accounts, hoveredAccount, onHover, onAction, }: AccountsListProps) { + const handleHover = useCallback(onHover, [onHover]); + const handleAction = useCallback(onAction, [onAction]); + if (accounts.length === 0) { return null; } + + const accountRows = useMemo( + () => + accounts.map(account => { + const hovered = hoveredAccount === account.id; + return ( + <MemoizedAccountRow + key={account.id} + account={account} + hovered={hovered} + onHover={handleHover} + onAction={handleAction} + /> + ); + }), + [accounts, hoveredAccount, handleHover, handleAction], + ); return ( <View> - {accounts.map(account => { - const hovered = hoveredAccount === account.id; - - return ( - <AccountRow - key={account.id} - account={account} - hovered={hovered} - onHover={onHover} - onAction={onAction} - /> - ); - })} + {accountRows} </View> );packages/loot-core/src/types/prefs.d.ts (1)
31-33: LGTM! Consider adding JSDoc comments.The new preference types follow the existing pattern and naming conventions. To improve maintainability, consider adding JSDoc comments explaining the purpose of each preference type.
+ /** Custom field mappings for bank sync providers */ | `custom-sync-mappings-${string}` + /** Controls import of pending transactions per account */ | `sync-import-pending-${string}` + /** Controls import of transaction notes per account */ | `sync-import-notes-${string}`packages/desktop-client/src/components/banksync/AccountRow.tsx (2)
13-20: Consider movingtsToStringto a shared utility module.This timestamp formatting utility could be useful in other components. Consider moving it to a shared utility module for reusability.
47-48: Use optional chaining for event handlers.The static analysis tool correctly identifies potential null reference issues. Use optional chaining for cleaner code.
- onMouseEnter={() => onHover && onHover(account.id)} - onMouseLeave={() => onHover && onHover(null)} + onMouseEnter={() => onHover?.(account.id)} + onMouseLeave={() => onHover?.(null)}🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/banksync/index.tsx (3)
24-34: Consider making provider list more maintainable.The provider names are currently hardcoded in the hook. Consider moving these to a configuration file or deriving them from the BankSyncProviders type to make future additions easier.
+import { type BankSyncProviders } from 'loot-core/types/models'; + +const SYNC_PROVIDERS: Record<BankSyncProviders, string> = { + goCardless: 'GoCardless', + simpleFin: 'SimpleFIN', +}; + const useSyncSourceReadable = () => { const { t } = useTranslation(); const syncSourceReadable: Record<SyncProviders, string> = { - goCardless: 'GoCardless', - simpleFin: 'SimpleFIN', + ...SYNC_PROVIDERS, unlinked: t('Unlinked'), };
48-74: Optimize memoization of grouped accounts.The
groupedAccountsmemo could be optimized by extracting the reducer function and memoizing it separately.+const useAccountGrouping = () => { + const groupAccounts = useCallback((accounts: AccountEntity[]) => { + return accounts + .filter(a => !a.closed) + .reduce( + (acc, a) => { + const syncSource = a.account_sync_source ?? 'unlinked'; + acc[syncSource] = acc[syncSource] || []; + acc[syncSource].push(a); + return acc; + }, + {} as Record<SyncProviders, AccountEntity[]>, + ); + }, []); + + return { groupAccounts }; +}; export function BankSync() { + const { groupAccounts } = useAccountGrouping(); const groupedAccounts = useMemo(() => { - const unsorted = accounts - .filter(a => !a.closed) - .reduce( - (acc, a) => { - const syncSource = a.account_sync_source ?? 'unlinked'; - acc[syncSource] = acc[syncSource] || []; - acc[syncSource].push(a); - return acc; - }, - {} as Record<SyncProviders, AccountEntity[]>, - ); + const unsorted = groupAccounts(accounts);
93-95: Memoize the onHover callback.The
onHovercallback should be memoized to prevent unnecessary re-renders of child components.- const onHover = useCallback((id: AccountEntity['id']) => { + const onHover = useCallback((id: AccountEntity['id']) => { setHoveredAccount(id); - }, []); + }, [setHoveredAccount]);packages/desktop-client/src/components/banksync/FieldMapping.tsx (1)
16-31: Extract transaction direction options to constants.Consider moving the transaction direction options to a separate constants file for better maintainability.
+const TRANSACTION_DIRECTIONS = { + PAYMENT: 'payment', + DEPOSIT: 'deposit', +} as const; + +type TransactionDirectionType = typeof TRANSACTION_DIRECTIONS[keyof typeof TRANSACTION_DIRECTIONS]; + const useTransactionDirectionOptions = () => { const { t } = useTranslation(); const transactionDirectionOptions = [ { - value: 'payment', + value: TRANSACTION_DIRECTIONS.PAYMENT, label: t('Payment'), }, { - value: 'deposit', + value: TRANSACTION_DIRECTIONS.DEPOSIT, label: t('Deposit'), }, ];packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (2)
42-66: Extract mappable fields configuration.Consider moving the mappable fields configuration to a separate file for better maintainability and reusability.
+// mappableFieldsConfig.ts +export const MAPPABLE_FIELDS_CONFIG: MappableField[] = [ + { + actualField: 'date', + syncFields: ['date', 'bookingDate', 'valueDate'], + }, + // ... other fields +]; -const mappableFields: MappableField[] = [ - { - actualField: 'date', - syncFields: ['date', 'bookingDate', 'valueDate'], - }, - // ... rest of the fields -]; +import { MAPPABLE_FIELDS_CONFIG } from './mappableFieldsConfig'; + +const mappableFields = MAPPABLE_FIELDS_CONFIG;
86-93: Extract preference management to custom hook.Consider creating a custom hook to manage related preferences, improving code organization and reusability.
+const useAccountPreferences = (accountId: string) => { + const [savedMappings, setSavedMappings] = useSyncedPref( + `custom-sync-mappings-${accountId}`, + mappingsToString(defaultMappings) + ); + const [savedImportNotes, setSavedImportNotes] = useSyncedPref( + `sync-import-notes-${accountId}`, + true + ); + const [savedImportPending, setSavedImportPending] = useSyncedPref( + `sync-import-pending-${accountId}`, + true + ); + + return { + savedMappings, + setSavedMappings, + savedImportNotes, + setSavedImportNotes, + savedImportPending, + setSavedImportPending, + }; +};packages/loot-core/src/server/main.ts (1)
1064-1066: Consider using ISO string for better timestamp readability.While storing milliseconds since epoch works, using an ISO string format would make the timestamps more readable in the database while maintaining sortability.
-const ts = new Date().getTime().toString(); +const ts = new Date().toISOString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
packages/desktop-client/e2e/rules.test.ts-snapshots/Rules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-10-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-11-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-12-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-13-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-14-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-15-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.ts-snapshots/Settings-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.ts-snapshots/Settings-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.ts-snapshots/Settings-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/loot-core/src/server/__snapshots__/main.test.ts.snapis excluded by!**/*.snappackages/loot-core/src/server/accounts/__snapshots__/parse-file.test.ts.snapis excluded by!**/*.snappackages/loot-core/src/server/accounts/__snapshots__/sync.test.ts.snapis excluded by!**/*.snappackages/loot-core/src/server/accounts/__snapshots__/transfer.test.ts.snapis excluded by!**/*.snapupcoming-release-notes/4253.mdis excluded by!**/*.md
📒 Files selected for processing (22)
packages/desktop-client/src/components/FinancesApp.tsx(2 hunks)packages/desktop-client/src/components/Modals.tsx(2 hunks)packages/desktop-client/src/components/banksync/AccountRow.tsx(1 hunks)packages/desktop-client/src/components/banksync/AccountsHeader.tsx(1 hunks)packages/desktop-client/src/components/banksync/AccountsList.tsx(1 hunks)packages/desktop-client/src/components/banksync/EditSyncAccount.tsx(1 hunks)packages/desktop-client/src/components/banksync/FieldMapping.tsx(1 hunks)packages/desktop-client/src/components/banksync/index.tsx(1 hunks)packages/desktop-client/src/components/common/Input.tsx(1 hunks)packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx(3 hunks)packages/loot-core/migrations/1738072673000_bank_sync_page.sql(1 hunks)packages/loot-core/src/client/accounts/accountsSlice.ts(1 hunks)packages/loot-core/src/client/state-types/modals.d.ts(1 hunks)packages/loot-core/src/mocks/index.ts(3 hunks)packages/loot-core/src/server/accounts/sync.ts(3 hunks)packages/loot-core/src/server/aql/schema/index.ts(2 hunks)packages/loot-core/src/server/main.ts(4 hunks)packages/loot-core/src/server/util/custom-sync-mapping.ts(1 hunks)packages/loot-core/src/types/models/account.d.ts(1 hunks)packages/loot-core/src/types/models/bank-sync.d.ts(1 hunks)packages/loot-core/src/types/models/transaction.d.ts(1 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
🧰 Additional context used
📓 Learnings (6)
packages/loot-core/src/types/models/bank-sync.d.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.
packages/loot-core/src/types/models/account.d.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.
packages/loot-core/src/server/aql/schema/index.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/loot-core/src/server/aql/schema/index.ts:145-145
Timestamp: 2025-01-12T20:26:15.249Z
Learning: In the schema definitions in `packages/loot-core/src/server/aql/schema/index.ts`, field definitions should follow the existing pattern using only the common properties (`type`, `default`, `required`, `ref`) for consistency.
packages/desktop-client/src/components/banksync/AccountsList.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/banksync/AccountsHeader.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/banksync/AccountRow.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🪛 Biome (1.9.4)
packages/desktop-client/src/components/banksync/AccountRow.tsx
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
packages/loot-core/src/server/accounts/sync.ts (8)
29-32: Looks good
These new imports for custom-sync-mapping appear clean and align with the usage innormalizeBankSyncTransactions.
329-346: Consider stricter validation of preferences for safer defaults
Currently, if the retrieved value is unexpected or absent, the code defaults to'true'. Make sure that's the intended behavior for every scenario, or handle invalid strings more robustly.
347-350: Handle malformed mapping strings
IfcustomMappingsRawis corrupted,mappingsFromStringmay fail or yield inconsistent mappings. Consider adding error handling or a safer fallback todefaultMappings.
353-354: Confirm alignment with user expectations for 'cleared'
Marking every transaction as cleared ifbookedis truthy might override any manually set states. Double-check that this is the desired logic.
361-361: Check for zero-amount edge case
Here, zero-amount transactions are treated as deposits. Verify that the mapping keys for'deposit'and'payment'exist and make sense for such edge cases.
362-366: Validate dynamic mapping fields
Relying onmapping.get('date'), etc., is flexible, but ensure the user-specified mapping keys exist. Otherwise, critical fields (like the date) could be undefined.
373-373: Consider a fallback payee
Some transactions may legitimately lack a payee. IfpayeeNameis null, consider using a placeholder or skipping them to avoid blocking the sync.
386-386: Resolve payees concurrency
This runs within a loop and relies onpayeesToCreateto avoid duplicates. Confirm no race conditions occur in parallel code or multi-transaction syncs.packages/loot-core/src/types/models/bank-sync.d.ts (1)
23-23: Foresee extensibility
DefiningBankSyncProvidersas a union is straightforward; just remember to extend it if more providers are introduced.packages/loot-core/src/types/models/account.d.ts (1)
13-14: Carefully handle null bank fields
WhenTis false,bankNameandbankIdboth become null. Verify that the UI and DB logic gracefully handle those null cases.packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1)
9-9: LGTM! Bank Sync navigation is well integrated.The Bank Sync menu item is appropriately placed in the sidebar navigation, with a suitable icon choice and correct route configuration.
Also applies to: 30-30, 67-72
packages/desktop-client/src/components/common/Input.tsx (1)
57-59: LGTM! Good visual feedback for disabled state.The conditional color styling provides clear visual feedback when inputs are disabled, using appropriate theme variables.
packages/desktop-client/src/components/banksync/AccountRow.tsx (1)
22-27: LGTM! Props interface is well-defined.The TypeScript interface for props is clear and type-safe, using appropriate types from the core package.
packages/desktop-client/src/components/banksync/index.tsx (1)
1-23: LGTM! Well-organized imports and type definitions.The imports are properly structured, and the type definitions are clear and purposeful.
packages/loot-core/src/mocks/index.ts (1)
54-62: LGTM! Consistent null initialization.The emptySyncFields function properly initializes all sync-related fields to null.
packages/loot-core/src/client/accounts/accountsSlice.ts (1)
193-194: LGTM! Ensures account data is refreshed after sync.The addition of
dispatch(getAccounts());improves data consistency by refreshing account data after handling sync responses.packages/loot-core/src/client/state-types/modals.d.ts (1)
189-193: LGTM! Well-defined modal type for editing synced accounts.The new modal type is properly integrated and follows TypeScript best practices.
packages/desktop-client/src/components/FinancesApp.tsx (1)
27-27: LGTM! Clean integration of bank sync feature.The bank sync feature is properly integrated with a new route and follows React Router patterns.
Also applies to: 252-252
packages/loot-core/src/server/aql/schema/index.ts (1)
56-56: LGTM! Schema fields follow consistent pattern.The new fields
raw_synced_dataandlast_syncare properly defined following the existing schema pattern, aligning with the established conventions.Also applies to: 77-77
packages/desktop-client/src/components/Modals.tsx (2)
14-14: LGTM!The import statement follows the project's import style and is used in the new modal case.
388-390: LGTM!The new modal case follows the established pattern, includes proper React key, and correctly passes the account prop.
packages/loot-core/src/server/main.ts (1)
Line range hint
1048-1063: LGTM!The function is correctly marked as async to support awaiting database operations while maintaining the same interface.
packages/loot-core/migrations/1738072673000_bank_sync_page.sql (3)
1-2: LGTM!The migration is properly wrapped in a transaction to ensure atomicity of the schema changes.
Also applies to: 7-7
3-3: LGTM!The
last_synccolumn is appropriately added with text type and no default value.
5-5: LGTM!The
raw_synced_datacolumn is appropriately added with text type and no default value.
packages/desktop-client/src/components/banksync/FieldMapping.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/banksync/EditSyncAccount.tsx
Outdated
Show resolved
Hide resolved
matt-fidd
force-pushed
the
bank-sync
branch
3 times, most recently
from
ec59e51 to
3dc37f1
Compare
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (20)
packages/desktop-client/src/components/banksync/index.tsx (2)
1-23: Consider code splitting or lazy-loading for improved performance.The comprehensive import list is fine, but if the bundle grows significantly, you may consider code-splitting or lazy-loading less frequently used components, especially for modals or config data, to optimize performance.
24-34: Localize all sync provider strings.While "Unlinked" is localized via
t(...), strings like "GoCardless" and "SimpleFIN" are currently hardcoded. Consider wrapping them int(...)for consistent localization support.- goCardless: 'GoCardless', - simpleFin: 'SimpleFIN', + goCardless: t('GoCardless'), + simpleFin: t('SimpleFIN'),packages/desktop-client/src/components/banksync/AccountsHeader.tsx (2)
14-14: Remove unnecessary empty style object.The empty style object
style={{}}onTableHeaderserves no purpose and can be removed.- <TableHeader style={{}}> + <TableHeader>
15-33: Consider extracting hardcoded values to constants.The component has multiple instances of hardcoded padding and width values. Consider extracting these to theme constants or a configuration object for better maintainability and consistency.
+const CELL_STYLES = { + padding: { paddingLeft: '10px' }, + widths: { + account: 250, + lastSync: 160, + actions: 100, + }, +} as const; export function AccountsHeader({ unlinked }: AccountsHeaderProps) { const { t } = useTranslation(); return ( <TableHeader> <Cell value={t('Account')} - width={!unlinked ? 250 : 'flex'} + width={!unlinked ? CELL_STYLES.widths.account : 'flex'} - style={{ paddingLeft: '10px' }} + style={CELL_STYLES.padding} /> {!unlinked && ( <> <Cell value={t('Bank')} width="flex" - style={{ paddingLeft: '10px' }} + style={CELL_STYLES.padding} /> <Cell value={t('Last sync')} - width={160} - style={{ paddingLeft: '10px' }} + width={CELL_STYLES.widths.lastSync} + style={CELL_STYLES.padding} /> - <Cell value="" width={100} style={{ paddingLeft: '10px' }} /> + <Cell value="" width={CELL_STYLES.widths.actions} style={CELL_STYLES.padding} /> </> )} </TableHeader> ); }packages/desktop-client/src/components/banksync/AccountsList.tsx (1)
16-43: Consider memoizing callbacks and mapped components for performance.While the implementation is clean, consider these performance optimizations:
- Memoize the
onHoverandonActioncallbacks usinguseCallback- Memoize the mapped
AccountRowcomponents usinguseMemo+import React, { useCallback, useMemo } from 'react'; export function AccountsList({ accounts, hoveredAccount, onHover, onAction, }: AccountsListProps) { if (accounts.length === 0) { return null; } + const memoizedOnHover = useCallback(onHover, [onHover]); + const memoizedOnAction = useCallback(onAction, [onAction]); + + const accountRows = useMemo( + () => + accounts.map(account => { + const hovered = hoveredAccount === account.id; + + return ( + <AccountRow + key={account.id} + account={account} + hovered={hovered} + onHover={memoizedOnHover} + onAction={memoizedOnAction} + /> + ); + }), + [accounts, hoveredAccount, memoizedOnHover, memoizedOnAction] + ); return ( <View> - {accounts.map(account => { - const hovered = hoveredAccount === account.id; - - return ( - <AccountRow - key={account.id} - account={account} - hovered={hovered} - onHover={onHover} - onAction={onAction} - /> - ); - })} + {accountRows} </View> ); }packages/loot-core/src/server/util/custom-sync-mapping.ts (2)
1-29: Enhance type safety and error handling.While the implementation is good, consider these improvements:
- Add type guards for better type safety
- Add validation for the inner map values
- Make error messages more specific
+type JsonMapping = Record<string, Record<string, string>>; + +function isValidInnerMapping(value: unknown): value is Record<string, string> { + return ( + typeof value === 'object' && + value !== null && + Object.entries(value).every( + ([k, v]) => typeof k === 'string' && typeof v === 'string' + ) + ); +} + export const mappingsFromString = (str: string): Mappings => { try { const parsed = JSON.parse(str); if (typeof parsed !== 'object' || parsed === null) { - throw new Error('Invalid mapping format'); + throw new Error('Mapping must be a JSON object'); } + + for (const [key, value] of Object.entries(parsed)) { + if (!isValidInnerMapping(value)) { + throw new Error( + `Invalid inner mapping for key "${key}": all values must be strings` + ); + } + } + return new Map( - Object.entries(parsed).map(([key, value]) => [ + Object.entries(parsed as JsonMapping).map(([key, value]) => [ key, new Map(Object.entries(value as object)), ]), ); } catch (e) { const message = e instanceof Error ? e.message : e; - throw new Error(`Failed to parse mapping: ${message}`); + throw new Error( + e instanceof SyntaxError + ? 'Invalid JSON format in mapping string' + : `Failed to parse mapping: ${message}` + ); } };
31-48: Reduce duplication in default mappings.The mappings for 'payment' and 'deposit' are identical. Consider extracting the common mapping to reduce duplication.
+const DEFAULT_FIELD_MAPPING = new Map([ + ['date', 'date'], + ['payee', 'payeeName'], + ['notes', 'remittanceInformationUnstructured'], +]); + export const defaultMappings: Mappings = new Map([ - [ - 'payment', - new Map([ - ['date', 'date'], - ['payee', 'payeeName'], - ['notes', 'remittanceInformationUnstructured'], - ]), - ], - [ - 'deposit', - new Map([ - ['date', 'date'], - ['payee', 'payeeName'], - ['notes', 'remittanceInformationUnstructured'], - ]), - ], + ['payment', new Map(DEFAULT_FIELD_MAPPING)], + ['deposit', new Map(DEFAULT_FIELD_MAPPING)], ]);packages/desktop-client/src/components/banksync/AccountRow.tsx (3)
13-18: Consider moving tsToString to a shared utility.The
tsToStringfunction could be useful in other components that need to format timestamps. Consider moving it to a shared utility file.
45-46: Use optional chaining for event handlers.The
onHovercallback checks could be simplified using optional chaining.- onMouseEnter={() => onHover && onHover(account.id)} - onMouseLeave={() => onHover && onHover(null)} + onMouseEnter={() => onHover?.(account.id)} + onMouseLeave={() => onHover?.(null)}🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
75-87: Consider extracting button cell to a separate component.The conditional rendering of edit/link buttons could be extracted to a separate component to improve readability and reusability.
type ActionButtonProps = { account: AccountEntity; onAction: AccountRowProps['onAction']; }; const ActionButton: React.FC<ActionButtonProps> = ({ account, onAction }) => { if (account.account_sync_source) { return ( <Cell name="edit" plain style={{ paddingRight: '10px' }}> <Button onPress={() => onAction(account, 'edit')}> <Trans>Edit</Trans> </Button> </Cell> ); } return ( <Cell name="link" plain style={{ paddingRight: '10px' }}> <Button onPress={() => onAction(account, 'link')}> <Trans>Link account</Trans> </Button> </Cell> ); };packages/desktop-client/src/components/banksync/FieldMapping.tsx (2)
16-31: Consider moving hook to a separate file.The
useTransactionDirectionOptionshook could be moved to a separate hooks file for better organization and reusability.
85-152: Extract field row to a separate component.The field mapping row rendering logic is complex and could be extracted to a separate component for better maintainability.
type FieldRowProps = { field: MappableFieldWithExample; mapping: Map<string, string>; setMapping: (field: string, value: string) => void; }; const FieldRow: React.FC<FieldRowProps> = ({ field, mapping, setMapping }) => { const { t } = useTranslation(); return ( <Row key={field.actualField} style={{ fontSize: 13, backgroundColor: theme.tableRowBackgroundHover, display: 'flex', alignItems: 'center', border: '1px solid var(--color-tableBorder)', minHeight: '40px', }} collapsed={true} > {/* ... rest of the row content ... */} </Row> ); };packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (2)
42-66: Add documentation for sync fields.Consider adding JSDoc comments to explain the purpose and expected format of each sync field, especially for fields like
remittanceInformationUnstructuredandremittanceInformationStructured.
135-141: Add loading state for preference updates.The
onSavefunction updates multiple preferences but doesn't handle the loading state or potential failures.Consider this improvement:
const onSave = async (close: () => void) => { + try { + setIsSaving(true); const mappingsStr = mappingsToString(mappings); - setSavedMappings(mappingsStr); - setSavedImportPending(String(importPending)); - setSavedImportNotes(String(importNotes)); + await Promise.all([ + setSavedMappings(mappingsStr), + setSavedImportPending(String(importPending)), + setSavedImportNotes(String(importNotes)) + ]); close(); + } catch (error) { + console.error('Failed to save preferences:', error); + // Show error notification to user + } finally { + setIsSaving(false); + } };packages/loot-core/src/server/aql/schema/index.ts (1)
56-57: Add documentation for new schema fields.Consider adding comments to explain:
- The purpose and format of
raw_synced_datain transactions- The format and timezone of
last_syncin accountsAlso applies to: 77-77
packages/loot-core/migrations/1738072673000_bank_sync_page.sql (1)
1-7: Consider enhancing the schema with constraints and indexes.While the migration is functionally correct, consider these improvements:
- Add a default value (NULL) for new columns
- Add an index on
last_syncif it will be frequently queried- Consider adding CHECK constraints if there are valid format requirements for these fields
BEGIN TRANSACTION; -ALTER TABLE accounts ADD COLUMN last_sync text; +ALTER TABLE accounts ADD COLUMN last_sync text DEFAULT NULL; +CREATE INDEX idx_accounts_last_sync ON accounts(last_sync); -ALTER TABLE transactions ADD COLUMN raw_synced_data text; +ALTER TABLE transactions ADD COLUMN raw_synced_data text DEFAULT NULL; COMMIT;packages/loot-core/src/types/models/account.d.ts (1)
13-14: Maintain consistent field naming convention.The field naming is inconsistent:
bankNameandbankIduse camelCaselast_syncuses snake_caseBased on the retrieved learning, we should maintain consistency with server field names. Additionally, consider adding JSDoc comments to document the purpose of these fields.
- bankName: T extends true ? string : null; - bankId: T extends true ? number : null; + /** Name of the bank as provided by the sync provider */ + bank_name: T extends true ? string : null; + /** Unique identifier for the bank in the provider's system */ + bank_id: T extends true ? number : null; mask: T extends true ? string : null; // end of bank account number official_name: T extends true ? string : null; balance_current: T extends true ? number : null; balance_available: T extends true ? number : null; balance_limit: T extends true ? number : null; account_sync_source: T extends true ? AccountSyncSource : null; + /** Timestamp of the last successful sync operation */ last_sync: T extends true ? string : null;Also applies to: 21-21
packages/loot-core/src/client/accounts/accountsSlice.ts (1)
193-194: Consider optimizing the account refresh.While refreshing accounts after sync is necessary, consider debouncing multiple sync responses to prevent unnecessary re-renders when batch syncing accounts.
+import { debounce } from '../../shared/util'; + +const debouncedGetAccounts = debounce((dispatch) => { + dispatch(getAccounts()); +}, 1000); + function handleSyncResponse( accountId: AccountEntity['id'], res: SyncResponse, dispatch: AppDispatch, resNewTransactions: Array<TransactionEntity['id']>, resMatchedTransactions: Array<TransactionEntity['id']>, resUpdatedAccounts: Array<AccountEntity['id']>, ) { // ... existing code ... - dispatch(getAccounts()); + debouncedGetAccounts(dispatch);packages/loot-core/src/server/accounts/sync.ts (2)
353-355: Consider adding a preference for cleared status.Currently, the code hardcodes the cleared status based on
booked. Consider making this configurable likeimportPendingandimportNotes.
394-395: Sanitize notes before storage.The code replaces '#' with '##' but should consider other potential markdown characters that might need escaping.
- notes: importNotes ? notes.trim().replace('#', '##') : null, + notes: importNotes && notes ? sanitizeMarkdown(notes.trim()) : null,Consider adding a utility function:
function sanitizeMarkdown(text: string): string { return text .replace(/#/g, '##') .replace(/\*/g, '\\*') .replace(/_/g, '\\_'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
packages/desktop-client/e2e/rules.test.ts-snapshots/Rules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.ts-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-10-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-11-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-12-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-13-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-14-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.ts-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-15-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.ts-snapshots/Settings-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.ts-snapshots/Settings-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.ts-snapshots/Settings-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/loot-core/src/server/__snapshots__/main.test.ts.snapis excluded by!**/*.snappackages/loot-core/src/server/accounts/__snapshots__/parse-file.test.ts.snapis excluded by!**/*.snappackages/loot-core/src/server/accounts/__snapshots__/sync.test.ts.snapis excluded by!**/*.snappackages/loot-core/src/server/accounts/__snapshots__/transfer.test.ts.snapis excluded by!**/*.snapupcoming-release-notes/4253.mdis excluded by!**/*.md
📒 Files selected for processing (22)
packages/desktop-client/src/components/FinancesApp.tsx(2 hunks)packages/desktop-client/src/components/Modals.tsx(2 hunks)packages/desktop-client/src/components/banksync/AccountRow.tsx(1 hunks)packages/desktop-client/src/components/banksync/AccountsHeader.tsx(1 hunks)packages/desktop-client/src/components/banksync/AccountsList.tsx(1 hunks)packages/desktop-client/src/components/banksync/EditSyncAccount.tsx(1 hunks)packages/desktop-client/src/components/banksync/FieldMapping.tsx(1 hunks)packages/desktop-client/src/components/banksync/index.tsx(1 hunks)packages/desktop-client/src/components/common/Input.tsx(1 hunks)packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx(3 hunks)packages/loot-core/migrations/1738072673000_bank_sync_page.sql(1 hunks)packages/loot-core/src/client/accounts/accountsSlice.ts(1 hunks)packages/loot-core/src/client/state-types/modals.d.ts(1 hunks)packages/loot-core/src/mocks/index.ts(3 hunks)packages/loot-core/src/server/accounts/sync.ts(3 hunks)packages/loot-core/src/server/aql/schema/index.ts(2 hunks)packages/loot-core/src/server/main.ts(4 hunks)packages/loot-core/src/server/util/custom-sync-mapping.ts(1 hunks)packages/loot-core/src/types/models/account.d.ts(1 hunks)packages/loot-core/src/types/models/bank-sync.d.ts(1 hunks)packages/loot-core/src/types/models/transaction.d.ts(1 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
🧰 Additional context used
📓 Learnings (6)
packages/loot-core/src/types/models/account.d.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.
packages/loot-core/src/types/prefs.d.ts (1)
Learnt from: MatissJanis
PR: actualbudget/actual#3458
File: packages/loot-core/src/client/state-types/prefs.d.ts:5-5
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In future reviews, ensure that changes related to `PrefsState` in `prefs.d.ts` do not incorrectly assume necessary updates in other parts of the codebase. Verify the impact thoroughly before making suggestions.
packages/desktop-client/src/components/banksync/AccountRow.tsx (2)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/desktop-client/src/components/banksync/AccountRow.tsx:0-0
Timestamp: 2025-01-28T23:24:32.957Z
Learning: In the Actual codebase, timestamp formatting should use date-fns' `format` function with explicit format strings (e.g., 'HH:mm:ss' for time) instead of JavaScript's built-in `toLocaleTimeString()` for consistency.
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/banksync/AccountsList.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/banksync/AccountsHeader.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/loot-core/src/types/models/bank-sync.d.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.
🪛 Biome (1.9.4)
packages/desktop-client/src/components/banksync/AccountRow.tsx
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (20)
packages/loot-core/src/server/main.ts (3)
1048-1048: Adopt consistent async patterns.Changing
handleSyncResponseto an async function ensures you canawaitdatabase updates. Ensure that all callers also leverageawaitor handle promise rejections as needed to avoid unhandled promise rejections.
1063-1066: Validate the last sync timestamp value.You’re converting a timestamp to string before persisting. If future logic depends on strict numeric fields, consider storing numeric timestamps instead or clarifying in docs that
last_syncis a text field.
1203-1203: Await handleSyncResponse in batch flows.Now that
handleSyncResponseis async, awaiting each handle ensures transactions and timestamps are updated before proceeding to subsequent operations. This is good for data consistency.packages/desktop-client/src/components/banksync/AccountsHeader.tsx (1)
1-8: LGTM! Clean imports and type definition.The imports are well-organized, and the type definition is clear and concise.
packages/desktop-client/src/components/banksync/AccountsList.tsx (1)
1-14: LGTM! Well-structured types and imports.The type definitions are clear and follow TypeScript best practices. The action type is properly constrained using a union type.
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1)
9-9: LGTM! Clean integration of bank sync navigation.The changes follow the existing patterns and are well-integrated into the sidebar navigation.
Also applies to: 30-30, 67-72
packages/loot-core/src/types/prefs.d.ts (1)
31-33: LGTM! The new preference types follow established patterns.The new string literal types for sync-related preferences maintain consistency with existing naming conventions and provide granular control over bank sync features.
Let's verify the naming consistency across the codebase:
✅ Verification successful
Naming consistency verified across the codebase
The preference key patterns are consistently implemented in both server and client code, maintaining the established naming conventions throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar preference key patterns rg "custom-sync|sync-import" --type tsLength of output: 1130
packages/loot-core/src/mocks/index.ts (2)
43-43: LGTM! Timestamp stored as string follows schema design.The implementation correctly stores the
last_synctimestamp as a string, which aligns with the database schema design.
54-62: LGTM! Empty sync fields are properly initialized.The
emptySyncFieldsfunction correctly initializes all sync-related fields to null, maintaining consistency with the database schema.packages/desktop-client/src/components/banksync/FieldMapping.tsx (1)
141-149: Add error handling for missing field mappings.The example value lookup could fail silently if the mapping is not found. Consider adding error handling or a fallback value.
const example = field.syncFields.find( f => f.field === mapping.get(field.actualField), )?.example ?? t('No example available');packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (2)
1-25: LGTM! Well-structured type definitions and imports.The type definitions and imports are well-organized, following TypeScript best practices.
124-133: LGTM! Proper error handling for JSON parsing.The JSON parsing is properly wrapped in a try-catch block, following the previous review comment's suggestion.
packages/loot-core/src/client/state-types/modals.d.ts (1)
189-193: LGTM! Well-typed modal definition.The new modal type is properly typed with the required
accountproperty.packages/desktop-client/src/components/FinancesApp.tsx (1)
27-27: LGTM! Proper route integration.The bank sync route is properly integrated into the existing routing structure.
Also applies to: 252-252
packages/desktop-client/src/components/Modals.tsx (1)
14-14: LGTM! Modal integration looks good.The new
EditSyncAccountmodal is properly integrated into the modal system, following the established patterns for modal rendering and prop passing.Also applies to: 388-390
packages/loot-core/src/types/models/bank-sync.d.ts (1)
22-23: LGTM! Type definition is clear and follows conventions.The
BankSyncProviderstype appropriately defines the supported providers using a union type.packages/desktop-client/src/components/common/Input.tsx (1)
57-59: LGTM! Good UX improvement.The conditional styling for disabled state provides better visual feedback to users.
packages/loot-core/src/server/accounts/sync.ts (3)
329-345: Good use of Promise.all for parallel queries.The code efficiently fetches multiple preferences in parallel, improving performance.
399-400: Good practice: Storing raw sync data.Storing the original transaction data enables better debugging and future data recovery if needed.
347-349: Verify mapping string parsing.Ensure that
mappingsFromStringproperly handles invalid mapping strings to prevent runtime errors.✅ Verification successful
✓ Mapping string parsing is properly implemented with error handling
The
mappingsFromStringfunction incustom-sync-mapping.tsincludes comprehensive error handling for invalid inputs:
- JSON parsing errors
- Type validation
- Null checks
- Descriptive error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling in mappingsFromString ast-grep --pattern 'function mappingsFromString($_) { $$$ try { $$$ } catch { $$$ } }'Length of output: 104
Script:
#!/bin/bash # Find mappingsFromString function definition ast-grep --pattern 'mappingsFromString = $_' # Also try const/let/var declaration ast-grep --pattern 'const mappingsFromString = $_' # Search for the function name in the codebase rg "mappingsFromString" -A 5Length of output: 10817
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/update-vrt |
Sorry, not sure what's going on with yours, I can't get it to happen at all on mine even with a completely clean slate. Got a separate change to fix that one, the transactions are returned unsorted. I'm just moving it over from actual-server now. |
|
Looks like that error is actually happening without syncing. There was an error that got thrown a few times after syncing, but maybe that is because of whatever is causing the other issue. |
|
Love the new features! I've been a little wary of adding new full pages to the sidebar, wondering your thoughts. E.g. with the goals UI feature we have potential for yet another page, and I feel like it starts to become overwhelming and run counter to the "incrementally revealing functionality" philosophy. I was thinking that the per-account settings could be an option in the menu for each account instead. But I do see an argument for having a central place for sync settings, not sure. What do you think? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/sync-server/src/app-gocardless/banks/bank.interface.ts (1)
25-37: Add documentation for the neweditedTransactionparameter.While the method signature changes look good, the documentation should be updated to explain the purpose and usage of the new
editedTransactionparameter./** * Returns a normalized transaction object * * The GoCardless integrations with different banks are very inconsistent in * what each of the different date fields actually mean, so this function is * expected to set a `date` field which corresponds to the expected * transaction date. + * + * @param transaction - The original transaction to normalize + * @param booked - Whether the transaction is booked + * @param editedTransaction - Optional transaction with custom field mappings + * @returns The normalized transaction or null if the transaction should be skipped */packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js (1)
17-29: Improve handling of undefined payee names.Using the string literal 'undefined' as a fallback value could cause issues in downstream code that checks for the actual
undefinedvalue.Consider this alternative approach:
if (Number(transaction.transactionAmount.amount) > 0) { editedTrans.payeeName = transaction.debtorName || - transaction.remittanceInformationUnstructured || - 'undefined'; + transaction.remittanceInformationUnstructured || + null; } else { editedTrans.payeeName = transaction.creditorName || extractPayeeNameFromRemittanceInfo( transaction.remittanceInformationUnstructured, ['Betaling met', 'Domiciliëring', 'Overschrijving'], - ); + ) || + null; }This change:
- Uses
nullinstead of 'undefined' string to represent missing values- Adds a fallback for negative amounts when both creditorName and extracted name are missing
packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js (2)
11-13: Update the comment to cover both positive and negative amount cases.The current comment only describes the negative amount case. Consider updating it to document both cases:
/** + * For positive amounts, payee information is extracted from debtorName or + * remittanceInformationUnstructured. + * * For negative amounts, the only payee information we have is returned in * remittanceInformationUnstructured. */
17-27: Consider improving payee name extraction.A few suggestions to enhance the payee name extraction:
- Consider moving the French keywords to a configuration file for better maintainability.
- Add fallback handling when all payee name sources are undefined.
- Consider adding validation for the extracted payee name.
Here's a suggested improvement:
+const PAYMENT_KEYWORDS = ['Paiement', 'Domiciliation', 'Transfert', 'Ordre permanent']; if (Number(transaction.transactionAmount.amount) > 0) { editedTrans.payeeName = - transaction.debtorName || transaction.remittanceInformationUnstructured; + transaction.debtorName || + transaction.remittanceInformationUnstructured || + 'Unknown Payee'; } else { editedTrans.payeeName = transaction.creditorName || extractPayeeNameFromRemittanceInfo( transaction.remittanceInformationUnstructured, - ['Paiement', 'Domiciliation', 'Transfert', 'Ordre permanent'], + PAYMENT_KEYWORDS, ) || + 'Unknown Payee'; }packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (2)
11-12: LGTM! Consider using deep clone for better immutability.Good practice creating a copy to avoid mutating the original transaction. However, since
transactionmight contain nested objects, consider using a deep clone for complete immutability.- const editedTrans = { ...transaction }; + const editedTrans = JSON.parse(JSON.stringify(transaction));
19-25: Consider extracting keywords to a configuration object.The keywords list could grow over time. Consider moving it to a separate configuration object for better maintainability and reusability across different bank integrations.
+const FORTUNEO_KEYWORDS = { + INSTANT_TRANSFER: 'VIR INST', + TRANSFER: 'VIR', + DIRECT_DEBIT: 'PRLV', + CARD_CANCEL: 'ANN CARTE', + CARD_DATE: 'CARTE \\d{2}\\/\\d{2}', +}; + const keywordsToRemove = [ - 'VIR INST', - 'VIR', - 'PRLV', - 'ANN CARTE', - 'CARTE \\d{2}\\/\\d{2}', + FORTUNEO_KEYWORDS.INSTANT_TRANSFER, + FORTUNEO_KEYWORDS.TRANSFER, + FORTUNEO_KEYWORDS.DIRECT_DEBIT, + FORTUNEO_KEYWORDS.CARD_CANCEL, + FORTUNEO_KEYWORDS.CARD_DATE, ];packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (2)
47-68: Consider refactoring the Unicode codepoint conversion logic.The current implementation has a complex loop with multiple string operations. Consider extracting this into a separate utility function for better maintainability.
+function convertUnicodeCodepoints(str) { + let idx = str.indexOf('\\U'); + let start_idx = idx; + let codepoints = []; + while (idx !== -1) { + codepoints.push(parseInt(str.slice(idx + 2, idx + 6), 16)); + const next_idx = str.indexOf('\\U', idx + 6); + if (next_idx == idx + 6) { + idx = next_idx; + continue; + } + str = + str.slice(0, start_idx) + + String.fromCodePoint(...codepoints) + + str.slice(idx + 6); + codepoints = []; + idx = str.indexOf('\\U'); + start_idx = idx; + } + return str; +} if (transaction.proprietaryBankTransactionCode == 'p2p') { - let str = transaction.remittanceInformationUnstructured; - let idx = str.indexOf('\\U'); - let start_idx = idx; - let codepoints = []; - while (idx !== -1) { - codepoints.push(parseInt(str.slice(idx + 2, idx + 6), 16)); - const next_idx = str.indexOf('\\U', idx + 6); - if (next_idx == idx + 6) { - idx = next_idx; - continue; - } - str = - str.slice(0, start_idx) + - String.fromCodePoint(...codepoints) + - str.slice(idx + 6); - codepoints = []; - idx = str.indexOf('\\U'); - start_idx = idx; - } - editedTrans.remittanceInformationUnstructured = str; + editedTrans.remittanceInformationUnstructured = convertUnicodeCodepoints( + transaction.remittanceInformationUnstructured + ); }
70-70: Consider adding a comment explaining the date fallback logic.The date assignment uses a fallback pattern that might not be immediately obvious to other developers.
- editedTrans.date = transaction.valueDate || transaction.bookingDate; + // Use valueDate if available, otherwise fall back to bookingDate + editedTrans.date = transaction.valueDate || transaction.bookingDate;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
packages/sync-server/src/app-gocardless/banks/abnamro_abnanl2a.js(1 hunks)packages/sync-server/src/app-gocardless/banks/american_express_aesudef1.js(0 hunks)packages/sync-server/src/app-gocardless/banks/bank.interface.ts(2 hunks)packages/sync-server/src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js(2 hunks)packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js(3 hunks)packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/danskebank_dabno22.js(1 hunks)packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js(1 hunks)packages/sync-server/src/app-gocardless/banks/entercard_swednokk.js(2 hunks)packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js(2 hunks)packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)packages/sync-server/src/app-gocardless/banks/ing_ingddeff.js(1 hunks)packages/sync-server/src/app-gocardless/banks/ing_pl_ingbplpw.js(1 hunks)packages/sync-server/src/app-gocardless/banks/integration-bank.js(1 hunks)packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/mbank_retail_brexplpw.js(0 hunks)packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/norwegian_xx_norwnok1.js(2 hunks)packages/sync-server/src/app-gocardless/banks/sandboxfinance_sfin0000.js(0 hunks)packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js(1 hunks)packages/sync-server/src/app-gocardless/banks/seb_privat.js(1 hunks)packages/sync-server/src/app-gocardless/banks/spk_karlsruhe_karsde66.js(2 hunks)packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(2 hunks)packages/sync-server/src/app-gocardless/banks/tests/abanca_caglesmm.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js(3 hunks)packages/sync-server/src/app-gocardless/services/tests/gocardless-service.spec.js(1 hunks)packages/sync-server/src/app-simplefin/app-simplefin.js(1 hunks)
💤 Files with no reviewable changes (3)
- packages/sync-server/src/app-gocardless/banks/sandboxfinance_sfin0000.js
- packages/sync-server/src/app-gocardless/banks/american_express_aesudef1.js
- packages/sync-server/src/app-gocardless/banks/mbank_retail_brexplpw.js
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/sync-server/src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js
- packages/sync-server/src/app-simplefin/app-simplefin.js
- packages/sync-server/src/app-gocardless/banks/tests/abanca_caglesmm.spec.js
- packages/sync-server/src/app-gocardless/banks/ing_pl_ingbplpw.js
- packages/sync-server/src/app-gocardless/services/tests/gocardless-service.spec.js
- packages/sync-server/src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js
- packages/sync-server/src/app-gocardless/banks/abnamro_abnanl2a.js
- packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js
- packages/sync-server/src/app-gocardless/banks/ing_ingddeff.js
- packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js
- packages/sync-server/src/app-gocardless/banks/entercard_swednokk.js
- packages/sync-server/src/app-gocardless/banks/danskebank_dabno22.js
- packages/sync-server/src/app-gocardless/banks/integration-bank.js
- packages/sync-server/src/app-gocardless/banks/norwegian_xx_norwnok1.js
🧰 Additional context used
🧠 Learnings (6)
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (2)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/integration-bank.js:68-71
Timestamp: 2025-02-11T17:28:02.196Z
Learning: In the integration-bank.js normalizeTransaction method, mutating the original transaction object by adding remittanceInformationUnstructuredArrayString and remittanceInformationStructuredArrayString properties is acceptable.
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js:19-27
Timestamp: 2025-02-11T17:26:44.539Z
Learning: In SEB bank integration (seb_kort_bank_ab.js), the transaction amount should be modified directly on the original transaction object as it serves as an identifier, and the modified amount is considered the true value. This is an intentional exception to the immutable pattern.
packages/sync-server/src/app-gocardless/banks/seb_privat.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js:19-27
Timestamp: 2025-02-11T17:26:44.539Z
Learning: In SEB bank integration (seb_kort_bank_ab.js), the transaction amount should be modified directly on the original transaction object as it serves as an identifier, and the modified amount is considered the true value. This is an intentional exception to the immutable pattern.
packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/virgin_nrnbgb22.js:25-30
Timestamp: 2025-02-11T17:26:19.364Z
Learning: The regex match validation in virgin_nrnbgb22.js was intentionally left as-is during the code copy to maintain consistency with the source code.
packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (16)
packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js (1)
16-31: Well-structured transaction normalization implementation!The changes follow best practices by:
- Correctly mutating core fields (transactionAmount) directly on the transaction object
- Using editedTrans for display-related fields (remittanceInformationUnstructured)
- Properly leveraging the fallback normalization
packages/sync-server/src/app-gocardless/banks/spk_karlsruhe_karsde66.js (1)
11-12: Well-structured changes that improve data handling!The modifications enhance the transaction normalization process by:
- Creating an immutable copy of the transaction object
- Preserving the original transaction data while making modifications
- Properly utilizing the booked parameter
- Delegating to the Fallback implementation for consistent processing
This aligns well with the PR's objective of preserving raw synchronization data.
Also applies to: 37-39, 41-41
packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js (3)
18-18: LGTM! Improved parameter naming.The removal of the underscore prefix from the
bookedparameter indicates it's now being used, which aligns with JavaScript naming conventions.
19-27: LGTM! Intentional mutation pattern for transaction amount.The code correctly follows the established pattern where:
- Most modifications are made on the
editedTranscopy- Transaction amount is intentionally modified on the original object as it serves as an identifier
29-29: LGTM! Enhanced normalization with fallback support.The change improves code reusability by delegating to the Fallback implementation while preserving bank-specific modifications.
packages/sync-server/src/app-gocardless/banks/bank.interface.ts (1)
7-13: LGTM! Well-structured type definition.The
TransactionExtendedtype effectively extends the baseTransactiontype with optional fields for enhanced transaction data. The type definition aligns with the PR's objective of preserving raw synchronization data and follows TypeScript best practices.packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js (1)
14-16: LGTM! Good use of immutability.Creating a copy of the transaction object using the spread operator is a good practice as it prevents unintended modifications to the original data.
packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js (1)
14-16: LGTM! Good use of immutability.The parameter renaming improves readability, and creating a copy of the transaction object is a good practice for maintaining immutability.
packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (2)
34-40: LGTM! Clear and well-documented logic.The transaction direction determination and payee name assignment is clear, consistent with other bank integrations, and well-documented.
41-44: LGTM! Clean implementation of the fallback pattern.The implementation correctly updates the transaction and delegates to the parent class, maintaining consistency with other bank integrations.
packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js (1)
18-19: LGTM! Test cases correctly verify the new transaction field structure.The test assertions have been updated to verify
payeeNameandnotesfields instead ofcreditorName/debtorName, which aligns with the standardized transaction field structure being implemented across bank sync providers.Also applies to: 34-35, 50-51
packages/sync-server/src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js (1)
11-12: LGTM! Correctly implements the transaction normalization pattern.The implementation follows the established pattern:
- Creates
editedTransfor display fields- Uses
Fallback.normalizeTransactionfor standardizationAlso applies to: 37-41
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (1)
9-11: LGTM! Correctly implements the transaction normalization pattern.The implementation follows the established pattern by using
editedTransfor display fields and maintaining the original transaction object.Also applies to: 17-21, 35-39, 67-68
packages/sync-server/src/app-gocardless/banks/seb_privat.js (2)
12-15: Implementation aligns with SEB bank integration requirements.Based on the retrieved learnings about SEB bank integration, the implementation correctly preserves the original transaction object for amount handling while creating a copy only for non-amount field modifications.
11-17: LGTM! The changes follow good practices.The implementation preserves immutability by creating a copy of the transaction object and properly documents the special case of creditor name storage in
additionalInformation.Let's verify that the
bookedparameter is used in the Fallback implementation:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check how the booked parameter is used in the Fallback implementation ast-grep --pattern 'normalizeTransaction(transaction, booked, $_) { $$$ }' src/app-gocardless/banks/integration-bank.jsLength of output: 215
I'll generate a new shell script to search for the Fallback implementation across the codebase (instead of looking in the non-existent integration-bank.js file) to confirm how the
bookedparameter is used. This should provide more context on whether the updatednormalizeTransactionmethod correctly forwards thebookedparameter.This will help us find the file where
Fallback.normalizeTransactionis defined or referenced so that we can manually verify the usage of thebookedparameter.
🏁 Script executed:
[run_scripts] #!/bin/bash # Locate files that mention Fallback and its normalizeTransaction method. rg "Fallback\.normalizeTransaction" -n .Length of output: 5816
Confirmed: The usage of the
bookedparameter is consistent.Our investigation shows that multiple bank modules invoke
Fallback.normalizeTransactionusing thebookedparameter (e.g., see similar calls in other bank files). The updatedseb_privat.jsimplementation aligns with the established pattern—forwarding thebookedparameter while preserving immutability.packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (1)
31-31: LGTM!The delegation to
Fallback.normalizeTransactionwith all necessary context ensures consistent normalization behavior while maintaining flexibility for bank-specific customizations.
I can see the concern with adding a page for everything, and I did consider making the configuration accessible through the account pages first, but I like the page in this case as it makes it more discoverable and provides an easy and obvious way to set up bank sync and options for a new user. It's hidden in the "More" dropdown, so the use case I'm imagining is that users will set it up and then collapse the menu and it won't add to the overall picture. It provides a central place for first sync setup when linking all of your accounts, and makes it easy to cross reference your custom mappings with another account if you're trying to copy them over. For the record, I'm in favour of a separate page for goal templates too. Compared to lots of applications we've got fairly a fairly small nav bar, along with the collapsable more menu. As long as every item is considered carefully, adding a couple of central pages isn't directly opposing the design choice IMO. That's not by any means to say everything should be added, I agree that inflating the amount of pages for the sake of it is a bad choice. |
|
One thing that maybe could help this menu situation is if the user could choose which pages get to be above the break. That way below the break could be a longer list, but the most used items are available always. |
I like that idea, an feature request to track that would be good |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (1)
70-70: Consider handling invalid dates.The fallback to
bookingDateis good, but consider validating that at least one of these dates exists.- editedTrans.date = transaction.valueDate || transaction.bookingDate; + if (!transaction.valueDate && !transaction.bookingDate) { + throw new Error('Transaction must have either valueDate or bookingDate'); + } + editedTrans.date = transaction.valueDate || transaction.bookingDate;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js(1 hunks)packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)packages/sync-server/src/app-simplefin/app-simplefin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sync-server/src/app-simplefin/app-simplefin.js
🧰 Additional context used
🧠 Learnings (2)
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (3)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js:24-30
Timestamp: 2025-02-12T01:17:09.971Z
Learning: In bank sync implementations, payeeName is not a core field and should be assigned to editedTrans rather than directly modifying the transaction object, as it's handled by the fallback normalization logic.
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/integration-bank.js:68-71
Timestamp: 2025-02-11T17:28:02.196Z
Learning: In the integration-bank.js normalizeTransaction method, mutating the original transaction object by adding remittanceInformationUnstructuredArrayString and remittanceInformationStructuredArrayString properties is acceptable.
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (2)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js:24-30
Timestamp: 2025-02-12T01:17:09.971Z
Learning: In bank sync implementations, payeeName is not a core field and should be assigned to editedTrans rather than directly modifying the transaction object, as it's handled by the fallback normalization logic.
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (6)
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (1)
24-32: LGTM! Well-structured changes with improved immutability.The changes follow best practices by:
- Creating an immutable copy of the transaction object
- Correctly assigning
payeeNametoeditedTrans- Reusing common normalization logic through
Fallback.normalizeTransactionpackages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (5)
9-10: LGTM! Good use of immutable pattern.The change from
_bookedtobookedparameter and the creation ofeditedTransas a shallow copy aligns with best practices for maintaining immutability.
17-18: LGTM! Correctly using editedTrans for payee information.Setting
debtorNameoneditedTransinstead of directly modifying the transaction object aligns with the learned best practice that display-related fields should useeditedTrans.
67-67: LGTM! Correctly using editedTrans for remittance info.Setting
remittanceInformationUnstructuredoneditedTransaligns with the learned best practice for display-related fields.
72-72: LGTM! Correctly using Fallback normalization.Good use of the Fallback normalization to handle common fields while preserving bank-specific customizations.
35-38: Consider adding input validation for remittance info parsing.The code assumes a specific format for remittance info and, as noted in the comment, could break if the payee name contains dashes. Consider adding validation or a more robust parsing strategy.
Please verify the format assumptions by checking the raw transaction data:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for remittance info patterns in test files rg -A 3 'remittanceInformationUnstructured.*-' packages/sync-server/src/app-gocardless/banks/tests/Length of output: 1699
Final Remittance Info Parsing Verification
Based on our investigation:
- The test files only supply basic remittance information strings (e.g.,
"some-creditor-name") that don’t appear to trigger any failure in the current parsing logic.- There is no evidence from the raw data tests that the dash characters in payee names cause the code to break.
That said, while current tests pass with minimal inputs, additional input validation could still be beneficial if more complex formats are encountered in production.
Features
Notes
Testing
Screenshots
Potential TODOs
Issues
Closes #4138
Closes #3838
Closes #2759
Closes #1467