add simplefin batch sync to api #3821
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 No assets were 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
changed the title
WalkthroughThe pull request introduces changes to the bank synchronization functionality within the Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/loot-core/src/server/main.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (4)
packages/loot-core/src/server/api.ts (3)
254-260: Simplify error collection for individual account sync.The error collection can be simplified by spreading the errors array directly.
- allErrors.push(errors); + allErrors.push(...errors);
261-281: Optimize account filtering logic.The account filtering can be optimized to avoid multiple array operations.
- const accountIdsToSync = accountsData.map(a => a.id); - const simpleFinAccounts = accountsData.filter( - a => a.account_sync_source === 'simpleFin', - ); - const simpleFinAccountIds = simpleFinAccounts.map(a => a.id); + const [simpleFinAccounts, otherAccounts] = accountsData.reduce( + ([sfAccs, others], acc) => { + if (acc.account_sync_source === 'simpleFin') { + sfAccs.push(acc); + } else { + others.push(acc.id); + } + return [sfAccs, others]; + }, + [[], []] + ); + const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);
283-285: Consider aggregating multiple errors for better error reporting.Currently, only the first error is thrown, which might hide other sync issues. Consider aggregating all errors to provide a complete sync status.
- const errors = allErrors.filter(e => e != null); - if (errors.length > 0) { - throw new Error(getBankSyncError(errors[0])); + const errors = allErrors.filter(e => e != null); + if (errors.length > 0) { + const errorMessages = errors.map(e => getBankSyncError(e)); + throw new Error( + `Multiple sync errors occurred:\n${errorMessages.join('\n')}` + );packages/loot-core/src/server/main.ts (1)
1102-1111: Improve Error Message for Better DebuggingThe error message thrown when both
idandidsare provided or neither is specified could be more informative. Including the received values ofidandidscan aid in debugging.Apply this diff to enhance the error message:
if ((id && ids) || (!id && !ids)) { - throw new Error('Specify either id or ids, but not both'); + throw new Error(`Specify either 'id' or 'ids', but not both. Received id: ${id}, ids: ${ids}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3821.mdis excluded by!**/*.md
📒 Files selected for processing (3)
packages/loot-core/src/server/api.ts(1 hunks)packages/loot-core/src/server/main.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/loot-core/src/server/api.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/client/actions/account.ts:180-194
Timestamp: 2024-11-04T00:34:13.035Z
Learning: In `packages/loot-core/src/client/actions/account.ts`, within the `syncAccounts` function, the batch sync request for SimpleFin accounts handles errors by returning error objects instead of throwing exceptions. Therefore, wrapping this block in a try-catch is unnecessary.
packages/loot-core/src/types/server-handlers.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-04T14:14:10.698Z
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.
🔇 Additional comments (4)
packages/loot-core/src/types/server-handlers.d.ts (1)
199-204: LGTM! Type structure improvement enhances type safety.
The restructured return type for 'simplefin-batch-sync' better represents the relationship between accounts and their sync results. Each array element now clearly associates an accountId with its corresponding sync response data.
packages/loot-core/src/server/api.ts (2)
251-252: LGTM! Clean initialization of batch sync mode.
The code effectively determines the sync mode and prepares for error collection.
251-285: Verify SimpleFin batch sync implementation.
The implementation successfully adds SimpleFin batch sync support as intended. Let's verify the usage of this new functionality across the codebase.
✅ Verification successful
SimpleFin batch sync implementation is correctly integrated
The implementation has been properly integrated across the codebase:
- The batch sync handler is correctly defined in
server-handlers.d.tsand implemented inserver/main.ts - The API layer in
server/api.tscorrectly handles both single account and batch sync scenarios - SimpleFin accounts are properly filtered and processed separately from other sync sources
- Error handling is consistent across both batch and single account syncs
- The implementation maintains compatibility with existing sync patterns while adding the new batch capability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of SimpleFin batch sync functionality
# Check for SimpleFin account configurations
echo "Checking SimpleFin account configurations..."
rg -l "account_sync_source.*simpleFin"
# Check for batch sync handler usage
echo "Checking batch sync handler usage..."
rg -A 5 "simplefin-batch-sync"
# Check for potential migration needs
echo "Checking for old sync patterns that might need updating..."
rg -A 5 "accounts-bank-sync.*id:"
Length of output: 2394
Script:
#!/bin/bash
# Let's verify the implementation more thoroughly
# Check the implementation of accounts-bank-sync handler
echo "Checking accounts-bank-sync implementation..."
ast-grep --pattern 'handlers["accounts-bank-sync"] = async function'
# Check for any other sync implementations that might need updating
echo "Checking other sync implementations..."
rg -A 10 "bank-sync" --type ts
# Check for SimpleFin related code to ensure consistency
echo "Checking SimpleFin related implementations..."
rg -A 5 "account_sync_source.*=.*simpleFin" --type ts
Length of output: 17162
packages/loot-core/src/server/main.ts (1)
1173-1177: Verify Consistent Usage of 'account_sync_source' Values
Ensure that the value 'simpleFin' used in the account_sync_source comparison matches the casing used throughout the codebase to prevent potential mismatches due to case sensitivity.
Run the following script to verify consistent usage of 'account_sync_source' values:
✅ Verification successful
Let me search for more occurrences of 'simpleFin' to ensure consistent casing across the codebase.
Based on the search results, I can now provide the final response as the casing of 'simpleFin' is consistently used throughout the codebase:
'simpleFin' Casing is Consistent Across Codebase
The value 'simpleFin' used in the account_sync_source comparison matches the casing used consistently throughout the codebase, particularly in:
- Type definition:
AccountSyncSource = 'simpleFin' | 'goCardless' - Account creation:
account_sync_source: 'simpleFin' - Account filtering:
account_sync_source === 'simpleFin'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of 'account_sync_source' comparisons to verify consistent casing.
# Search for comparisons where 'account_sync_source' is checked against a string literal.
rg -t ts "account_sync_source\s*=\s*['\"]\w+['\"]"
Length of output: 144
Script:
#!/bin/bash
# Search for any occurrence of 'simpleFin' with different casings
rg -i "simplefin" -t ts -A 2 -B 2
Length of output: 35001
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/main.ts (1)
1102-1122: Consider simplifying the interface to only accept an array of IDs.As suggested in a previous review, consider simplifying this handler to only accept an optional
idsarray parameter. This would:
- Reduce complexity in parameter validation
- Make the interface more consistent
- Allow single account sync through an array with one ID
Example refactor:
-handlers['accounts-bank-sync'] = async function ({ id, ids }) { +handlers['accounts-bank-sync'] = async function ({ ids = [] }) { const [[, userId], [, userKey]] = await asyncStorage.multiGet([ 'user-id', 'user-key', ]); - if ((id && ids) || (!id && !ids)) { - throw new Error('Specify either id or ids, but not both'); - } const accounts = await db.runQuery( ` SELECT a.*, b.bank_id as bankId FROM accounts a LEFT JOIN banks b ON a.bank = b.id WHERE a.tombstone = 0 AND a.closed = 0 - ${id ? 'AND a.id = ?' : ''} - ${ids ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''} + ${ids.length ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''} ORDER BY a.offbudget, a.sort_order `, - id ? [id] : ids || [], + ids, true, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/main.ts(2 hunks)
🔇 Additional comments (3)
packages/loot-core/src/server/main.ts (3)
1108-1110: LGTM! Clear parameter validation.
The validation ensures mutual exclusivity between id and ids parameters with a clear error message.
1175-1179: LGTM! Proper filtering for SimpleFin accounts.
The WHERE clause correctly filters for SimpleFin accounts while maintaining existing filters and proper parameterization.
1175-1179: Consider adding an index for better query performance.
The query filters on account_sync_source which could benefit from an index, especially as the number of accounts grows.
Consider adding an index on the account_sync_source column:
CREATE INDEX idx_accounts_sync_source ON accounts(account_sync_source) WHERE tombstone = 0 AND closed = 0;Let's verify if an index already exists:
matt-fidd
force-pushed
the
simplefin-batch-api
branch
from
524588b to
673f079
Compare
matt-fidd
force-pushed
the
simplefin-batch-api
branch
from
9901245 to
96d9e8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/client/actions/account.ts (2)
Line range hint
164-181: Consider optimizing the account filtering logic.The account filtering and sorting logic could be extracted into a separate utility function for better reusability and testability.
+ function getAccountsToSync(accounts, filterSimpleFin = false) { + const filtered = accounts.filter( + ({ bank, closed, tombstone }) => !!bank && !closed && !tombstone, + ); + if (filterSimpleFin) { + filtered = filtered.filter(a => a.account_sync_source === 'simpleFin'); + } + return filtered.sort((a, b) => + a.offbudget === b.offbudget + ? a.sort_order - b.sort_order + : a.offbudget - b.offbudget, + ).map(({ id }) => id); + } let accountIdsToSync = !batchSync ? [id] - : getState() - .queries.accounts.filter( - ({ bank, closed, tombstone }) => !!bank && !closed && !tombstone, - ) - .sort((a, b) => - a.offbudget === b.offbudget - ? a.sort_order - b.sort_order - : a.offbudget - b.offbudget, - ) - .map(({ id }) => id); + : getAccountsToSync(getState().queries.accounts);
Line range hint
182-196: Consider adding logging for debugging and monitoring.The batch sync implementation would benefit from additional logging to help track sync progress and diagnose issues in production.
if (batchSync && simpleFinAccounts.length > 0) { - console.log('Using SimpleFin batch sync'); + console.log(`Starting SimpleFin batch sync for ${simpleFinAccounts.length} accounts`); const res = await send('simplefin-batch-sync', { ids: simpleFinAccounts.map(a => a.id), }); + console.log(`SimpleFin batch sync completed with ${res.length} responses`);packages/loot-core/src/server/main.ts (1)
1102-1117: LGTM! Consider minor formatting improvements for SQL readability.The changes to support batch synchronization through the
idsparameter are well-implemented. The SQL query is secure and efficient.Consider reformatting the SQL query for better readability:
- SELECT a.*, b.bank_id as bankId - FROM accounts a - LEFT JOIN banks b ON a.bank = b.id - WHERE a.tombstone = 0 AND a.closed = 0 - ${ids.length ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''} - ORDER BY a.offbudget, a.sort_order + SELECT a.*, b.bank_id as bankId + FROM accounts a + LEFT JOIN banks b ON a.bank = b.id + WHERE a.tombstone = 0 + AND a.closed = 0 + ${ids.length ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''} + ORDER BY a.offbudget, a.sort_order
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/loot-core/src/client/actions/account.ts(1 hunks)packages/loot-core/src/server/api.ts(1 hunks)packages/loot-core/src/server/main.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/server-handlers.d.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/api.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/client/actions/account.ts:180-194
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/client/actions/account.ts`, within the `syncAccounts` function, the batch sync request for SimpleFin accounts handles errors by returning error objects instead of throwing exceptions. Therefore, wrapping this block in a try-catch is unnecessary.
🔇 Additional comments (5)
packages/loot-core/src/client/actions/account.ts (2)
207-207: LGTM! Parameter change aligns with batch sync support.
The change from id: accountId to ids: [accountId] correctly adapts the individual account sync to use the new batch-capable endpoint.
Line range hint 182-213: Verify error handling for batch sync scenarios.
The batch sync implementation looks good, but let's verify the error handling when mixing SimpleFin and non-SimpleFin accounts.
✅ Verification successful
Based on the shell script results, I can now provide a final response as the error handling patterns are clear from the codebase analysis.
Error handling is consistent across SimpleFin and non-SimpleFin sync scenarios
The error handling implementation is robust and consistent:
- Both sync paths use the same
handleSyncErrorfunction which normalizes errors into a commonSyncErrorformat - SimpleFin account errors are properly converted via
handleSyncErrorwith appropriate error types and codes - The dispatch handling in the client properly processes these normalized errors through
markAccountFailed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error handling across sync implementations
# Look for error handling patterns in sync-related code
rg -A 5 "SyncError|markAccountFailed"
# Check for any SimpleFin-specific error types
ast-grep --pattern 'error.type === $type'
Length of output: 13639
packages/loot-core/src/server/api.ts (2)
283-285: Review error handling approach.
The current error handling approach might hide subsequent errors by only throwing the first one. This could make debugging harder when multiple accounts fail to sync.
Consider aggregating all errors into a single error message:
- const errors = allErrors.filter(e => e != null);
- if (errors.length > 0) {
- throw new Error(getBankSyncError(errors[0]));
- }
+ const errors = allErrors.filter(e => e != null);
+ if (errors.length > 0) {
+ const errorMessages = errors.map(e => getBankSyncError(e));
+ throw new Error(`Multiple sync errors occurred:\n${errorMessages.join('\n')}`);
+ }Let's verify the error handling implementation across the codebase:
#!/bin/bash
# Description: Check error handling patterns in sync operations
# Look for error handling patterns in sync operations
ast-grep --pattern 'catch ($err) { $$$ }'
# Check for error aggregation patterns
rg -A 5 "errors\.(push|concat)"251-281: 🛠️ Refactor suggestion
Consider optimizing the batch sync implementation.
The current implementation has a few areas that could be improved:
- The code fetches all accounts and then filters them, which could be optimized by filtering at the database level.
- There's potential for race conditions when syncing accounts in parallel.
Consider these improvements:
- const accountsData = await handlers['accounts-get']();
- const accountIdsToSync = accountsData.map(a => a.id);
- const simpleFinAccounts = accountsData.filter(
- a => a.account_sync_source === 'simpleFin',
- );
- const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);
+ const accountsData = await handlers['accounts-get']();
+ const [simpleFinAccounts, otherAccounts] = accountsData.reduce(
+ (acc, account) => {
+ acc[account.account_sync_source === 'simpleFin' ? 0 : 1].push(account);
+ return acc;
+ },
+ [[], []]
+ );
+ const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);
+ const otherAccountIds = otherAccounts.map(a => a.id);Let's verify the potential for race conditions in account syncing:
packages/loot-core/src/server/main.ts (1)
1170-1174: LGTM! SimpleFin account filtering is properly implemented.
The addition of the account_sync_source = 'simpleFin' filter condition is well-integrated and aligns perfectly with the PR's objective of implementing SimpleFin batch synchronization.
As reported in Discord, the API was not updated to use the single call SimpleFin batch update handler