implement SimpleFin batch sync #3581
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces substantial modifications to the account synchronization functionality within the On the client side, a new function, Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/types/server-handlers.d.ts (1)
196-204: LGTM! Consider enhancing type safety for the return type.The new
simplefin-batch-syncmethod is well-defined and aligns with the PR objectives. It provides a clear interface for batch synchronization of SimpleFin accounts.To improve type safety, consider explicitly defining the types for
errors,newTransactions,matchedTransactions, andupdatedAccountsin the return type. This will provide better type checking and autocompletion for consumers of this API. For example:'simplefin-batch-sync': ({ ids }: { ids: string[] }) => Promise<Array<{ accountId: string; errors: Array<{ message: string }>; // Define the structure of errors newTransactions: Array<Transaction>; // Define Transaction type matchedTransactions: Array<Transaction>; updatedAccounts: Array<Account>; // Define Account type }>>;Replace
TransactionandAccountwith the appropriate types used in your project.packages/loot-core/src/client/actions/account.ts (2)
Line range hint
153-199: Consider simplifying account filtering logicThe logic for determining
accountIdsToSyncand handling batch sync could be simplified for clarity. Currently, there is complex filtering and sorting, and then additional filtering to excludesimpleFinAccounts.Proposed improvement:
- Perform the filtering of
simpleFinAccountsupfront.- Simplify the
accountIdsToSyncdetermination.- This can enhance readability and maintainability.
🧰 Tools
🪛 GitHub Check: lint
[warning] 193-193:
'isSyncSuccess' is assigned a value but never used. Allowed unused vars must match /^(_|React)/u
100-144: Type annotations for function parametersThe
handleSyncResponsefunction lacks type annotations for its parameters:Suggestion: Add type annotations for better type safety
If you're using TypeScript, consider adding type annotations to enhance code reliability and maintainability.
function handleSyncResponse( accountId: string, res: SyncResponseType, dispatch: Dispatch, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/loot-core/src/client/actions/account.ts (2 hunks)
- packages/loot-core/src/server/accounts/sync.ts (7 hunks)
- packages/loot-core/src/server/main.ts (4 hunks)
- packages/loot-core/src/types/server-handlers.d.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/client/actions/account.ts
[warning] 193-193:
'isSyncSuccess' is assigned a value but never used. Allowed unused vars must match /^(_|React)/upackages/loot-core/src/server/accounts/sync.ts
[warning] 26-26:
'loot-core/types/models' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing fromloot-core/*
[warning] 26-26:
There should be at least one empty line between import groups
[warning] 26-26:
loot-core/types/modelsimport should occur before import of../../platform/server/asyncStorage
[warning] 26-26:
'SimpleFinAccount' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 27-27:
@rschedule/core/rules/ByMonthOfYearimport should occur before import ofdate-fns
[warning] 27-27:
'RevByMonthOfYearRule' is defined but never used. Allowed unused vars must match /^(_|React)/u
[failure] 209-209:
Unexpected any. Specify a different type
[warning] 847-847:
'promises' is never reassigned. Use 'const' instead
🔇 Additional comments (4)
packages/loot-core/src/client/actions/account.ts (3)
100-144: Good use of modularization withhandleSyncResponsefunctionExtracting the response handling into the
handleSyncResponsefunction improves code readability and reusability. It centralizes the logic for processing synchronization responses, reducing duplication and potential errors.
210-212: EnsureisSyncSuccessaccumulates success status correctlyIn the loop over
accountIdsToSync, the success of each account sync is determined:Ensure that
isSyncSuccessaccurately reflects whether any account sync succeeded. With the variable scope corrected, this logic should work as intended.
116-133: Consistent error handling for different error typesIn
handleSyncResponse, errors are dispatched based on their type:... ...Ensure that all error types are appropriately handled, and consider logging or handling additional error properties if necessary.
packages/loot-core/src/server/accounts/sync.ts (1)
66-95:getAccountSyncStartDatefunction implementation looks goodThe
getAccountSyncStartDatefunction correctly calculates the start date for account synchronization based on existing transactions. This helps in optimizing the sync process by avoiding unnecessary data retrieval.
|
Ok so manual testing revealed two possible issues:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
packages/loot-core/src/client/actions/account.ts (3)
100-144: Approve the addition ofhandleSyncResponsefunction with a minor suggestion.The new
handleSyncResponsefunction effectively centralizes the logic for handling sync responses, improving code organization and reusability. It correctly processes errors, new transactions, and matched transactions, dispatching appropriate actions for each case.Consider enhancing the error notification messages to include the account ID or name. This would provide more context to the user about which account encountered an error. For example:
dispatch( addNotification({ type: 'error', message: `Error syncing account ${accountId}: ${error.message}`, internal: error.internal, }), );
Line range hint
153-176: Approve the introduction of batch synchronization with a suggestion for clarity.The changes effectively introduce batch synchronization for SimpleFin accounts, which should improve efficiency. The logic for determining which accounts to sync has been enhanced to consider both off-budget status and sort order.
To improve code clarity, consider extracting the account filtering and sorting logic into a separate function. This would make the main function easier to read and the filtering logic more reusable. For example:
function getAccountsToSync(state) { return state.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); } // Then in syncAccounts: let accountIdsToSync = !batchSync ? [id] : getAccountsToSync(getState());
205-211: Approve the modifications to individual account sync process.The changes to the individual account sync process effectively utilize the new
handleSyncResponsefunction, ensuring consistent processing of sync results across both batch and individual syncs. This improves code maintainability and reduces duplication.For consistency with the batch sync implementation, consider updating the
isSyncSuccessassignment to use the logical OR operator:- if (success) isSyncSuccess = true; + isSyncSuccess = isSyncSuccess || success;This change makes it clear that
isSyncSuccesswill be true if any sync operation (batch or individual) was successful.packages/loot-core/src/server/accounts/sync.ts (2)
64-93: Address TODO comment for handling future transactionsThe function
getAccountSyncStartDatelooks well-implemented, but there's a TODO comment that needs to be addressed:// TODO: Handle the case where transactions exist in the future // (that will make start date after end date)To handle this case, you could add a check to ensure the start date is not in the future:
const startDate = monthUtils.dayFromDate( dateFns.min([ dateFns.max([ monthUtils.parseDate(monthUtils.subDays(monthUtils.currentDay(), 90)), monthUtils.parseDate(startingDate), ]), monthUtils.currentDay() // Ensure start date is not in the future ]) );This change will cap the start date to the current day, preventing issues with future transactions.
Would you like me to implement this change or create a GitHub issue to track this task?
Line range hint
179-226: Improve type safety and structure indownloadSimpleFinTransactionsThe changes to support batch synchronization are good, but there are a few areas that could be improved:
- Type safety: The use of
anyin the type assertion should be avoided.- Structure: The use of a placeholder object for non-batch sync adds unnecessary complexity.
Consider refactoring the function to use a more type-safe approach:
interface AccountData { transactions: { all: TransactionType[] }; balances: BalanceType; startingBalance: number; } type SyncResponse = Record<string, AccountData>; // ... let accounts: SyncResponse; if (batchSync) { accounts = res; } else { accounts = { [acctId as string]: res }; } return accounts;This approach eliminates the need for the placeholder and improves type safety.
Would you like me to implement these changes?
🧰 Tools
🪛 GitHub Check: lint
[failure] 207-207:
Unexpected any. Specify a different type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/loot-core/src/client/actions/account.ts (2 hunks)
- packages/loot-core/src/server/accounts/sync.ts (6 hunks)
- packages/loot-core/src/server/main.ts (4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/accounts/sync.ts
[failure] 207-207:
Unexpected any. Specify a different type
🔇 Additional comments (4)
packages/loot-core/src/client/actions/account.ts (1)
Line range hint
100-211: Overall approval of changes with suggestions for improvement.The changes introduced in this pull request significantly enhance the account synchronization functionality, particularly for SimpleFin accounts. The addition of the
handleSyncResponsefunction and the implementation of batch synchronization improve code organization, reduce duplication, and should lead to better performance for users with multiple SimpleFin accounts.Key improvements:
- Introduction of
handleSyncResponsefor centralized sync result processing.- Implementation of batch synchronization for SimpleFin accounts.
- Consistent error handling and transaction processing across batch and individual syncs.
While the overall changes are positive, there are a few areas for improvement:
- Enhance error notification messages to include account identifiers for better user context.
- Extract account filtering and sorting logic into a separate function for improved readability and reusability.
- Fix the scope issue with the
isSyncSuccessvariable in the batch sync block.- Ensure consistent updating of the
isSyncSuccessvariable across batch and individual syncs.Addressing these minor issues will further improve the code quality and maintainability of this important feature.
packages/loot-core/src/server/accounts/sync.ts (1)
Line range hint
1-868: Overall improvements to account synchronization with opportunities for further enhancementThe changes in this file significantly enhance the account synchronization functionality, particularly with the addition of batch synchronization for SimpleFin accounts. The new functions
getAccountSyncStartDate,processBankSyncDownload, andSimpleFinBatchSyncprovide a more robust and efficient synchronization process.Key improvements:
- Support for batch synchronization of SimpleFin accounts.
- More flexible handling of sync start dates.
- Improved processing of downloaded transactions.
While these changes are positive, there are opportunities for further enhancement:
- Improving type safety, particularly in the
downloadSimpleFinTransactionsfunction.- Enhancing error handling across all new and modified functions.
- Optimizing performance in batch synchronization.
- Addressing the TODO comment in
getAccountSyncStartDate.Implementing the suggested improvements will further increase the robustness and maintainability of the code.
packages/loot-core/src/server/main.ts (2)
1059-1074: Well-implemented synchronization response handlerThe
handleSyncResponsefunction efficiently processes the synchronization response and updates the relevant arrays. It correctly uses the spread operator for array concatenation and checks for changes before updating theupdatedAccountsarray.
Line range hint
1119-1142: Improved structure and error handling in bank syncThe modifications to
handlers['accounts-bank-sync']enhance the code structure and error handling:
- Utilization of
handleSyncResponseandhandleSyncErrorfunctions improves code modularity.- The use of arrays for collecting sync results enables efficient batch processing.
- Error handling is now more consistent and informative.
These changes contribute to better maintainability and error reporting.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/client/actions/account.ts (3)
100-147: Approve the newhandleSyncResponsefunction with suggestions for improvementThe
handleSyncResponsefunction is a well-structured addition that centralizes the processing of sync responses. It effectively handles errors, transactions, and account updates.Consider the following improvements:
- Add TypeScript type annotations to improve type safety and code readability.
- Consider using a more specific error type for sync errors instead of checking the
error.typeproperty.- The error handling for non-SyncErrors (lines 132-138) could be more specific. Consider creating custom error types for different scenarios.
Example of adding type annotations:
function handleSyncResponse( accountId: string, res: { errors: Array<{ type: string; message: string; category?: string; code?: string; internal?: boolean }>; newTransactions: unknown[]; matchedTransactions: unknown[]; updatedAccounts: unknown[]; }, dispatch: Dispatch, resNewTransactions: unknown[], resMatchedTransactions: unknown[], resUpdatedAccounts: unknown[] ): boolean { // ... function body ... }
Line range hint
156-207: Approve the addition of batch sync support with refactoring suggestionsThe implementation of SimpleFin batch sync is a valuable addition that should improve performance for users with multiple SimpleFin accounts. The logic for determining which accounts to sync and handling the batch sync process is sound.
To improve readability and maintainability, consider refactoring the
syncAccountsfunction:
- Extract the batch sync logic into a separate function, e.g.,
performSimpleFinBatchSync.- Use early returns to reduce nesting and improve clarity.
- Add more detailed comments explaining the batch sync process and its integration with the existing sync logic.
Example refactoring:
async function performSimpleFinBatchSync(simpleFinAccounts, dispatch) { console.log('Using SimpleFin batch sync'); const res = await send('simplefin-batch-sync', { ids: simpleFinAccounts.map(a => a.id), }); let isSyncSuccess = false; const newTransactions = []; const matchedTransactions = []; const updatedAccounts = []; for (const account of res) { const success = handleSyncResponse( account.accountId, account.res, dispatch, newTransactions, matchedTransactions, updatedAccounts ); if (success) isSyncSuccess = true; } return { isSyncSuccess, newTransactions, matchedTransactions, updatedAccounts }; } // Use this function in syncAccounts if (batchSync && simpleFinAccounts.length) { const batchSyncResult = await performSimpleFinBatchSync(simpleFinAccounts, dispatch); isSyncSuccess = batchSyncResult.isSyncSuccess; newTransactions.push(...batchSyncResult.newTransactions); matchedTransactions.push(...batchSyncResult.matchedTransactions); updatedAccounts.push(...batchSyncResult.updatedAccounts); accountIdsToSync = accountIdsToSync.filter( id => !simpleFinAccounts.find(sfa => sfa.id === id) ); }
Line range hint
156-207: Add detailed comments to explain batch sync logicWhile the code is generally clear, adding more detailed comments would improve its readability and maintainability, especially for other developers who might work on this code in the future.
Consider adding comments to explain:
- The purpose and functionality of the batch sync feature.
- The logic behind filtering SimpleFin accounts.
- The process of updating
accountIdsToSyncafter batch sync.Example:
// Check if we're performing a batch sync (no specific account ID provided) const batchSync = !id; // If performing a batch sync, filter out SimpleFin accounts for special handling if (batchSync) { const accountsData = await send('accounts-get'); const simpleFinAccounts = accountsData.filter( a => a.account_sync_source === 'simpleFin', ); // Perform batch sync for SimpleFin accounts if any exist if (simpleFinAccounts.length) { console.log('Using SimpleFin batch sync'); // ... batch sync logic ... // Update accountIdsToSync to exclude SimpleFin accounts that were just synced accountIdsToSync = accountIdsToSync.filter( id => !simpleFinAccounts.find(sfa => sfa.id === id), ); } } // Continue with individual account syncs for remaining accountsThese comments provide context and explain the purpose of each section of the code, making it easier for developers to understand and maintain the batch sync feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/loot-core/src/client/actions/account.ts (2 hunks)
- packages/loot-core/src/server/main.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/loot-core/src/server/main.ts (2)
1059-1074: Well-implemented sync response handlerThe
handleSyncResponsefunction efficiently processes the sync response, updating the relevant arrays with new and updated transactions. The use of the spread operator for array updates is a good practice for performance.
Line range hint
1119-1142: Improved structure and error handling in bank syncThe modifications to the
accounts-bank-synchandler significantly improve its structure and error handling:
- Introduction of
newTransactions,matchedTransactions, andupdatedAccountsarrays provides a more comprehensive sync result.- Utilization of
handleSyncResponsefor processing sync results enhances code modularity.- Error handling is now delegated to
handleSyncError, improving consistency and maintainability.These changes make the code more readable, maintainable, and robust.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/accounts/sync.ts (2)
64-94: LGTM! Consider adding error handling.The
getAccountSyncStartDatefunction is well-implemented and logically sound. It correctly handles cases with no transactions and applies a reasonable 90-day limit for synchronization.Consider adding error handling for database queries to make the function more robust. For example:
try { const latestTransaction = await db.first( 'SELECT * FROM v_transactions WHERE account = ? AND date <= ? ORDER BY date DESC LIMIT 1', [id, currentDate], ); // ... rest of the function } catch (error) { console.error('Error fetching account sync start date:', error); throw error; }
Line range hint
786-829: LGTM! Consider refactoring to reduce code duplicationThe changes to the
syncAccountfunction improve its structure and readability. The use ofgetAccountSyncStartDateandprocessBankSyncDownloadenhances the overall logic.To reduce code duplication, consider extracting the download logic into a separate function:
function getDownload(acctRow, startDate, userId, userKey, acctId, bankId) { if (acctRow.account_sync_source === 'simpleFin') { return downloadSimpleFinTransactions(acctId, startDate); } else if (acctRow.account_sync_source === 'goCardless') { return downloadGoCardlessTransactions( userId, userKey, acctId, bankId, startDate, startDate === null ); } throw new Error( `Unrecognized bank-sync provider: ${acctRow.account_sync_source}` ); } // Then use it in syncAccount: const startDate = await getAccountSyncStartDate(id) ?? monthUtils.subDays(monthUtils.currentDay(), 90); const download = await getDownload(acctRow, startDate, userId, userKey, acctId, bankId); return processBankSyncDownload(download, id, acctRow, startDate === null);This refactoring eliminates the need for the
if-elseblock and reduces code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/loot-core/src/server/accounts/sync.ts (6 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/accounts/sync.ts
[failure] 208-208:
Unexpected any. Specify a different type
🔇 Additional comments (1)
packages/loot-core/src/server/accounts/sync.ts (1)
Line range hint
1-869: Overall improvements in account synchronization and transaction processingThe changes in this file significantly enhance the account synchronization and transaction processing capabilities. The introduction of
getAccountSyncStartDate,processBankSyncDownload, andSimpleFinBatchSyncfunctions, along with the refactoring of existing functions, improves code structure and efficiency.Key improvements include:
- Better handling of sync start dates
- Support for batch synchronization of SimpleFin accounts
- More consistent processing of downloaded transactions
- Improved separation of concerns in the sync process
While the overall direction is positive, consider addressing the following points to further improve the code:
- Enhance error handling throughout the file, especially in asynchronous operations
- Improve type safety by replacing
anytypes with more specific interfaces- Refactor some sections to reduce code duplication
- Ensure consistent handling of edge cases across different sync sources
These changes will result in a more robust, maintainable, and efficient synchronization process.
matt-fidd
force-pushed
the
simplefin-batch-requests
branch
from
45eb41c to
4735fe2
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/sync.ts (1)
64-94: LGTM with a minor suggestion for error handlingThe
getAccountSyncStartDatefunction is well-implemented. It correctly handles various scenarios and uses secure SQL practices. However, consider adding error handling for the database queries to make the function more robust.Consider wrapping the database queries in try-catch blocks to handle potential errors:
try { const latestTransaction = await db.first( 'SELECT * FROM v_transactions WHERE account = ? AND date <= ? ORDER BY date DESC LIMIT 1', [id, currentDate], ); // ... rest of the function } catch (error) { console.error('Error fetching transaction data:', error); throw error; // or handle it appropriately }
🛑 Comments failed to post (4)
packages/loot-core/src/server/accounts/sync.ts (4)
200-205: 🛠️ Refactor suggestion
Refactor the placeholder logic for better clarity
The use of a placeholder object for single account sync might be confusing and unnecessarily complex.
Consider simplifying this logic:
let accounts = batchSync ? res : { [acctId]: res };This approach eliminates the need for the placeholder and makes the logic more explicit.
207-222:
⚠️ Potential issueImprove type safety in response processing
The current implementation uses type assertions that might lead to runtime errors if the response structure changes.
Consider defining a more specific type for the account data structure:
interface AccountData { transactions: { all: Array<{ // Define transaction properties here }>; }; balances: { // Define balance properties here }; startingBalance: number; } // Then use it in the type assertion let retVal: Record<string, AccountData> = {}; for (const [accountId, data] of Object.entries(accounts) as [string, AccountData][]) { // ... rest of the code }This change will provide better type checking and autocompletion for the
dataobject properties.🧰 Tools
🪛 GitHub Check: typecheck
[failure] 211-211:
Property 'transactions' does not exist on type 'unknown'.
[failure] 212-212:
Property 'balances' does not exist on type 'unknown'.
[failure] 213-213:
Property 'startingBalance' does not exist on type 'unknown'.
693-774:
⚠️ Potential issuePotential issue with SimpleFin balance calculation during initial sync
The balance calculation for SimpleFin accounts during initial sync might be incorrect. The current implementation subtracts transaction amounts from the current balance, which could lead to an incorrect starting balance.
Consider revising the balance calculation logic:
if (acctRow.account_sync_source === 'simpleFin') { const currentBalance = download.startingBalance; const previousBalance = transactions.reduce((total, trans) => { return total + Math.round(parseFloat(trans.transactionAmount.amount) * 100); }, currentBalance); balanceToUse = previousBalance; }This change adds the transaction amounts to the current balance instead of subtracting them, which should correctly calculate the starting balance. It also uses a more precise method for parsing the transaction amount.
830-867: 🛠️ Refactor suggestion
Enhance error handling and robustness in SimpleFinBatchSync
The
SimpleFinBatchSyncfunction efficiently implements batch synchronization for SimpleFin accounts using parallel processing. However, the error handling can be improved to make the function more robust.Consider the following improvements:
- Use
Promise.allSettledinstead ofPromise.allto prevent a single account failure from stopping the entire batch process:const results = await Promise.allSettled(promises); return results.map(result => { if (result.status === 'fulfilled') { return result.value; } else { return { accountId: result.reason.accountId, error: result.reason.message }; } });
- Add error handling for
downloadSimpleFinTransactions:let res; try { res = await downloadSimpleFinTransactions( accounts.map(a => a.accountId), startDates, ); } catch (error) { console.error('Error downloading SimpleFin transactions:', error); throw error; }
- Wrap the individual account processing in a try-catch block:
promises.push( (async () => { try { const result = await promise; return { accountId: account.id, res: result, }; } catch (error) { console.error(`Error processing account ${account.id}:`, error); return { accountId: account.id, error: error.message, }; } })() );These changes will make the function more resilient to errors and provide better feedback on which accounts failed during the batch sync process.
matt-fidd
force-pushed
the
simplefin-batch-requests
branch
from
4735fe2 to
b2c7edd
Compare
|
@coderabbitai pause |
|
@matt-fidd I get this error now. I think one of my accounts is in an error state: 
|
|
Also note that it is now 'stuck': 
|
matt-fidd
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
packages/loot-core/src/types/models/bank-sync.d.ts (4)
7-9: Add JSDoc comments to explain the purpose of these type aliasesWhile the type aliases create a good abstraction layer over the gocardless-specific types, adding documentation would help explain why this abstraction exists and how it relates to the bank synchronization feature.
Example improvement:
+/** Generic balance information for bank sync operations, provider-agnostic */ export type BankSyncBalance = GoCardlessBalance; +/** Generic amount representation for bank sync operations, provider-agnostic */ export type BankSyncAmount = GoCardlessAmount; +/** Generic transaction data for bank sync operations, provider-agnostic */ export type BankSyncTransaction = GoCardlessTransaction;
19-20: Use camelCase for error fields to maintain TypeScript conventionsThe error fields use snake_case which is inconsistent with TypeScript naming conventions.
- error_type: string; - error_code: string; + errorType: string; + errorCode: string;
18-18: Consider using BankSyncAmount type for startingBalanceFor consistency and type safety, the startingBalance field should probably use the BankSyncAmount type instead of a primitive number type, as it represents a monetary value.
- startingBalance: number; + startingBalance: BankSyncAmount;
11-21: Consider adding metadata for UI state managementBased on the PR comments about UI update issues, consider extending this type to include metadata that could help manage UI states (e.g., lastSyncTimestamp, hasNewTransactions).
Example addition:
export type BankSyncResponse = { // ... existing fields ... metadata?: { lastSyncTimestamp: number; hasNewTransactions: boolean; }; };This could help address the reported issue where "accounts that had new transactions were not bolded to indicate updates."
packages/loot-core/src/types/models/gocardless.d.ts (3)
32-35: Add JSDoc comment to document amount formatConsider adding documentation to specify the expected format of the amount string (e.g., decimal places, thousand separators).
+/** + * Represents a monetary amount with its currency + * @property {string} amount - The monetary value as a string (e.g., "123.45") + * @property {string} currency - The ISO 4217 currency code (e.g., "USD") + */ export type GoCardlessAmount = { amount: string; currency: string; };
37-72: Consider expanding debtorAccount structure for different account typesThe type definition is comprehensive, but the
debtorAccountstructure is limited to IBAN only. Consider expanding it to support other account identifiers.debtorAccount?: { - iban: string; + iban?: string; + bban?: string; + accountNumber?: string; + sortCode?: string; };Also, consider extracting common account structures into a shared type since
creditorAccountmight need similar flexibility.
16-72: Types well-aligned with batch sync requirementsThe comprehensive type definitions provide a solid foundation for the SimpleFin batch sync implementation. The balance and transaction types support all necessary data fields for bulk account synchronization.
Consider creating an abstraction layer that maps these GoCardless-specific types to generic types used by SimpleFin to maintain flexibility for future banking integrations.
packages/loot-core/src/server/accounts/sync.ts (4)
87-99: Consider making the sync period configurable.The function addresses the starting date issue mentioned in the PR comments by using the max of 90 days ago and the oldest transaction date. However, the 90-day limit is hardcoded.
Consider extracting this to a configuration:
+const DEFAULT_SYNC_PERIOD_DAYS = 90; + async function getAccountSyncStartDate(id) { - const dates = [monthUtils.subDays(monthUtils.currentDay(), 90)]; + const dates = [monthUtils.subDays(monthUtils.currentDay(), DEFAULT_SYNC_PERIOD_DAYS)];
Line range hint
185-235: Simplify the response processing logic.The function handles both single and batch responses well, but the response processing could be more concise.
Consider this simplified approach:
- let retVal = {}; - if (batchSync) { - for (const [accountId, data] of Object.entries( - res as SimpleFinBatchSyncResponse, - )) { - if (accountId === 'errors') continue; - - const error = res?.errors?.[accountId]?.[0]; - - retVal[accountId] = { - transactions: data?.transactions?.all, - accountBalance: data?.balances, - startingBalance: data?.startingBalance, - }; - - if (error) { - retVal[accountId].error_type = error.error_type; - retVal[accountId].error_code = error.error_code; - } - } - } else { - const singleRes = res as BankSyncResponse; - retVal = { - transactions: singleRes.transactions.all, - accountBalance: singleRes.balances, - startingBalance: singleRes.startingBalance, - }; - } + const retVal = batchSync + ? Object.entries(res as SimpleFinBatchSyncResponse) + .filter(([key]) => key !== 'errors') + .reduce((acc, [accountId, data]) => ({ + ...acc, + [accountId]: { + transactions: data?.transactions?.all, + accountBalance: data?.balances, + startingBalance: data?.startingBalance, + ...(res?.errors?.[accountId]?.[0] && { + error_type: res.errors[accountId][0].error_type, + error_code: res.errors[accountId][0].error_code, + }), + }, + }), {}) + : { + transactions: (res as BankSyncResponse).transactions.all, + accountBalance: (res as BankSyncResponse).balances, + startingBalance: (res as BankSyncResponse).startingBalance, + };
Line range hint
706-787: Simplify the SimpleFin balance calculation.The balance calculation for SimpleFin initial sync could be more readable.
Consider using
reducewith a more explicit calculation:if (acctRow.account_sync_source === 'simpleFin') { - const currentBalance = download.startingBalance; - const previousBalance = transactions.reduce((total, trans) => { - return ( - total - parseInt(trans.transactionAmount.amount.replace('.', '')) - ); - }, currentBalance); - balanceToUse = previousBalance; + balanceToUse = transactions.reduce( + (balance, trans) => balance - amountToInteger(trans.transactionAmount.amount), + download.startingBalance + ); }
822-869: Enhance error handling in batch sync.While the function effectively handles batch processing, the error handling could be improved to provide more context.
Consider adding error context:
if (download.error_code) { promises.push( Promise.resolve({ accountId: account.id, - res: download, + res: { + ...download, + error_context: `Failed to sync account ${account.id} at index ${i}`, + }, }), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/loot-core/src/server/accounts/sync.ts(7 hunks)packages/loot-core/src/types/models/bank-sync.d.ts(1 hunks)packages/loot-core/src/types/models/gocardless.d.ts(1 hunks)packages/loot-core/src/types/models/index.d.ts(1 hunks)packages/loot-core/src/types/models/simplefin.d.ts(2 hunks)
🔇 Additional comments (5)
packages/loot-core/src/types/models/simplefin.d.ts (1)
1-1: LGTM!
The import statement is clean and imports only the required type.
packages/loot-core/src/types/models/index.d.ts (1)
2-2: LGTM! The type export follows established patterns.
The addition is well-placed alphabetically and correctly uses type-only exports.
Let's verify the referenced types exist:
✅ Verification successful
Verified: The bank-sync.d.ts file exists and contains the required types
The verification confirms:
- The file exists at the expected location
- It exports all the necessary bank sync types (BankSyncBalance, BankSyncAmount, BankSyncTransaction, BankSyncResponse)
- The types are properly defined and aligned with the GoCardless implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify bank-sync.d.ts exists and contains expected types
# Expected: File exists with BankSyncBalance, BankSyncAmount, BankSyncTransaction, and BankSyncResponse types
# Test 1: Check if bank-sync.d.ts exists
fd -t f "bank-sync.d.ts" packages/loot-core/src/types/models
# Test 2: Verify the types exist in the file
rg -A 1 "type (BankSyncBalance|BankSyncAmount|BankSyncTransaction|BankSyncResponse)" packages/loot-core/src/types/models/bank-sync.d.ts
Length of output: 454
packages/loot-core/src/types/models/bank-sync.d.ts (1)
1-6: LGTM: Clean and focused imports
The imports are well-structured and specifically import only the required types from the gocardless module.
packages/loot-core/src/server/accounts/sync.ts (2)
72-85: LGTM! Well-structured query using AQL.
The function effectively retrieves the oldest transaction while properly handling the current date constraint.
789-820: LGTM! Well-structured account sync implementation.
The function effectively handles different sync sources and properly utilizes the new helper functions for determining sync dates.
matt-fidd
force-pushed
the
simplefin-batch-requests
branch
from
9f42a0c to
652061c
Compare
|
@coderabbitai full review |
matt-fidd
force-pushed
the
simplefin-batch-requests
branch
from
652061c to
bd9db8b
Compare
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
packages/loot-core/src/types/models/simplefin.d.ts (1)
15-17: Add JSDoc documentation for the new interface.While the interface implementation is clean and well-typed, adding documentation would improve maintainability and help other developers understand its purpose in the batch sync feature.
Consider adding documentation like this:
+/** + * Represents the response structure for SimpleFin batch synchronization. + * Maps account IDs to their individual sync responses, allowing tracking + * of multiple account synchronizations in a single operation. + */ export interface SimpleFinBatchSyncResponse { [accountId: AccountEntity['account_id']]: BankSyncResponse; }packages/loot-core/src/types/models/bank-sync.d.ts (2)
1-9: Consider defining provider-agnostic base interfaces.While type aliases provide abstraction from the specific provider, consider defining base interfaces that capture the essential properties needed for bank synchronization. This would make it easier to support multiple providers in the future without being tightly coupled to GoCardless's type definitions.
Example approach:
interface BaseBankTransaction { id: string; amount: number; date: string; description: string; // Add other essential properties } export interface BankSyncTransaction extends BaseBankTransaction { // Add GoCardless-specific properties }
11-21: Consider adding sync window configuration to the type definitions.Based on the PR objectives, there's a fixed 90-day sync window that's causing concerns. Consider adding configuration types to make this customizable:
export interface BankSyncConfig { /** Number of days to look back during sync */ syncWindowDays: number; /** Whether to use account's oldest transaction date */ useOldestTransaction?: boolean; } export type BankSyncResponse = { // ... existing fields ... /** Configuration used for this sync */ config?: BankSyncConfig; };packages/loot-core/src/types/models/gocardless.d.ts (2)
50-52: Consider expanding debtorAccount structure.The
debtorAccountstructure only includes IBAN, but banking accounts often have other identifiers (like SWIFT/BIC, account number, etc.). Consider expanding this type to matchcreditorAccount's flexibility.Example expansion:
debtorAccount?: { iban?: string; bic?: string; accountNumber?: string; sortCode?: string; };
49-49: Consider typing array properties more specifically.Properties like
currencyExchangeandremittanceInformationStructuredArrayare typed asstring[], which might be too permissive. Consider creating specific types for these arrays to ensure data consistency.Example for currency exchange:
type CurrencyExchange = { sourceCurrency: string; targetCurrency: string; rate: string; date?: string; }; // Then use it in GoCardlessTransaction currencyExchange?: CurrencyExchange[];Also applies to: 63-64
packages/loot-core/src/types/server-handlers.d.ts (1)
196-204: Add JSDoc documentation for the new method.Consider adding documentation to describe:
- The purpose of the batch sync operation
- Expected behavior and usage
- Possible error scenarios and their handling
- Description of response properties
Example:
/** * Performs batch synchronization of SimpleFin accounts. * @param {Object} params - The parameters for the batch sync operation * @param {string[]} params.ids - Array of account IDs to synchronize * @returns {Promise<Array<{ * accountId: string, // ID of the synchronized account * errors, // Array of errors encountered during sync * newTransactions, // Number of new transactions added * matchedTransactions, // Number of transactions matched with existing ones * updatedAccounts // Array of account IDs that were updated * }>>} */packages/loot-core/src/client/actions/account.ts (1)
92-139: Suggestion to Add Type Annotations for ClarityWhile the implementation is correct, adding TypeScript type annotations to the
handleSyncResponsefunction parameters can improve type safety and code clarity.Consider specifying the parameter types as follows:
function handleSyncResponse( accountId: string, res: { errors: Array<{ type: string; message: string; category?: string; code?: string; internal?: boolean }>; newTransactions: any[]; matchedTransactions: any[]; updatedAccounts: any[]; }, dispatch: Dispatch, resNewTransactions: any[], resMatchedTransactions: any[], resUpdatedAccounts: any[], ): boolean { // function body }packages/loot-core/src/server/accounts/sync.ts (2)
72-85: Add type annotation toidparameter ingetAccountOldestTransactionTo enhance type safety and code clarity, consider specifying the type of the
idparameter in thegetAccountOldestTransactionfunction.Update the function signature:
-async function getAccountOldestTransaction(id): Promise<TransactionEntity> { +async function getAccountOldestTransaction(id: string): Promise<TransactionEntity> {
87-99: Add type annotation toidparameter ingetAccountSyncStartDateTo improve type safety and maintain consistency, consider specifying the type of the
idparameter in thegetAccountSyncStartDatefunction.Update the function signature:
-async function getAccountSyncStartDate(id) { +async function getAccountSyncStartDate(id: string) {packages/loot-core/src/server/main.ts (3)
1163-1225: Refactor to eliminate code duplication between sync handlersBoth
accounts-bank-syncandsimplefin-batch-synchandlers perform similar synchronization logic, including error handling and response processing. Consider refactoring the shared code into a common function or utility module to promote reusability and reduce maintenance overhead.
1059-1074: Document the purpose and usage of 'handleSyncResponse'Adding a docstring or comments to the
handleSyncResponsefunction would improve code readability and maintainability by clarifying its role in the synchronization process and detailing its parameters.
1076-1102: Consider handling unexpected error types in 'handleSyncError'While the function handles specific error types, adding a default case for unexpected errors can prevent potential uncaught exceptions and improve robustness.
Apply this diff to include a default error handling case:
return { accountId: acct.id, message: 'There was an internal error. Please get in touch https://actualbudget.org/contact for support.', internal: err.stack, }; + } else { + return { + accountId: acct.id, + message: `An unexpected error occurred while syncing account “${acct.name}”.`, + internal: err.stack, + }; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3581.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/loot-core/src/client/actions/account.ts(2 hunks)packages/loot-core/src/server/accounts/sync.ts(7 hunks)packages/loot-core/src/server/main.ts(4 hunks)packages/loot-core/src/types/models/bank-sync.d.ts(1 hunks)packages/loot-core/src/types/models/gocardless.d.ts(1 hunks)packages/loot-core/src/types/models/index.d.ts(1 hunks)packages/loot-core/src/types/models/simplefin.d.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/loot-core/src/client/actions/account.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/server/main.ts (3)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/server/main.ts:1188-1198
Timestamp: 2024-10-25T00:12:14.939Z
Learning: In `packages/loot-core/src/server/main.ts`, when handling errors, the error properties `error_code` and `error_type` may already be mapped to `code` and `category` before being passed to `handleSyncError`.
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/server/main.ts:1096-1102
Timestamp: 2024-10-25T00:17:33.196Z
Learning: In this codebase, including `err.stack` in error responses returned to clients is acceptable to maintain consistent behavior.
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.
🔇 Additional comments (5)
packages/loot-core/src/types/models/simplefin.d.ts (1)
1-2: LGTM! Imports are properly defined.
The necessary types are correctly imported, addressing the previous review comment about the missing AccountEntity import.
packages/loot-core/src/types/models/index.d.ts (1)
2-2: LGTM! The type export follows the established pattern.
The addition of bank sync types export is consistent with the file's structure and is appropriately placed in alphabetical order.
Let's verify the existence of the referenced type definition file:
✅ Verification successful
Type export is valid and correctly references the existing file
The verification confirms that bank-sync.d.ts exists at the expected location: packages/loot-core/src/types/models/bank-sync.d.ts, making this a valid type export.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the bank-sync.d.ts file exists
# Expected: File should exist in the same directory
fd -t f "bank-sync.d.ts" "packages/loot-core/src/types/models"
Length of output: 111
packages/loot-core/src/types/models/gocardless.d.ts (1)
32-35: LGTM! Well-structured amount type.
The type correctly uses strings for financial values, which is a good practice to avoid floating-point precision issues.
packages/loot-core/src/client/actions/account.ts (2)
92-139: Well-Structured Error Handling with handleSyncResponse
The new handleSyncResponse function effectively consolidates error handling and response processing. This enhances code readability and maintainability by reducing duplication in the syncAccounts function.
Line range hint 148-199: Efficient Batch Synchronization Implementation
The modifications to syncAccounts introduce batch synchronization for SimpleFin accounts, improving performance by reducing individual network requests. The use of handleSyncResponse ensures consistent handling of synchronization responses.
matt-fidd
force-pushed
the
simplefin-batch-requests
branch
from
c1f38fb to
7297816
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/loot-core/src/server/accounts/sync.ts (3)
208-227: Enhance error handling in batch sync mode.While the error handling is functional, it could be more robust. Consider these improvements:
- Add type checking for the error response structure
- Handle cases where
datais undefined- Add logging for error cases
for (const [accountId, data] of Object.entries( res as SimpleFinBatchSyncResponse, )) { if (accountId === 'errors') continue; + // Log any errors for debugging + const error = res?.errors?.[accountId]?.[0]; + if (error) { + console.log(`Error syncing account ${accountId}:`, error); + } + + // Ensure data exists before accessing properties + if (!data) { + retVal[accountId] = { + error_type: 'DATA_MISSING', + error_code: 'NO_DATA', + }; + continue; + } - const error = res?.errors?.[accountId]?.[0]; retVal[accountId] = { transactions: data?.transactions?.all, accountBalance: data?.balances, startingBalance: data?.startingBalance, };
Line range hint
720-766: Simplify starting balance calculation for SimpleFin.The current implementation of starting balance calculation for SimpleFin is complex and could be simplified. Consider extracting this logic into a separate function for better maintainability.
+ async function calculateSimpleFinStartingBalance(transactions, currentBalance) { + return transactions.reduce( + (total, trans) => total - parseInt(trans.transactionAmount.amount.replace('.', '')), + currentBalance + ); + } + if (initialSync) { const { transactions } = download; let balanceToUse = download.startingBalance; if (acctRow.account_sync_source === 'simpleFin') { - const currentBalance = download.startingBalance; - const previousBalance = transactions.reduce((total, trans) => { - return ( - total - parseInt(trans.transactionAmount.amount.replace('.', '')) - ); - }, currentBalance); - balanceToUse = previousBalance; + balanceToUse = await calculateSimpleFinStartingBalance( + transactions, + download.startingBalance + ); }
826-871: Consider adding rate limiting for batch operations.While parallel processing is efficient, it might be beneficial to add rate limiting to prevent overwhelming the SimpleFin API. Consider using a concurrency limit for the promises.
+ async function processBatchWithRateLimit(accounts, batchSize = 5) { + const results = []; + for (let i = 0; i < accounts.length; i += batchSize) { + const batch = accounts.slice(i, i + batchSize); + const batchResults = await Promise.all( + batch.map(async account => { + const download = res[account.accountId]; + const acctRow = await db.select('accounts', account.id); + const oldestTransaction = await getAccountOldestTransaction(account.id); + const newAccount = oldestTransaction == null; + + if (download.error_code) { + return { + accountId: account.id, + res: download, + }; + } + + return processBankSyncDownload( + download, + account.id, + acctRow, + newAccount + ).then(res => ({ + accountId: account.id, + res, + })); + }) + ); + results.push(...batchResults); + } + return results; + } + // Replace the existing promise handling with rate-limited version - const promises = []; - for (let i = 0; i < accounts.length; i++) { - // ... existing code ... - } - return await Promise.all(promises); + return await processBatchWithRateLimit(accounts);packages/loot-core/src/server/main.ts (3)
1076-1102: Consider adding TypeScript type annotations.The error handling implementation is solid, with proper handling of different error types and user-friendly messages. Consider adding TypeScript type annotations to improve code maintainability:
-function handleSyncError(err, acct) { +function handleSyncError(err: BankSyncError | PostError | Error, acct: { id: string; name: string }) {
1173-1179: Consider using Promise.all for parallel processing.The batch sync could be more efficient by processing accounts in parallel:
- const res = await bankSync.SimpleFinBatchSync( - accounts.map(a => ({ - id: a.id, - accountId: a.account_id, - })), - ); + const res = await Promise.all( + accounts.map(a => + bankSync.SimpleFinBatchSync([{ + id: a.id, + accountId: a.account_id, + }]) + ) + );
1215-1220: Consider using Array.prototype.some with early return.The sync event check could be optimized:
- if (retVal.some(a => a.res.updatedAccounts.length > 0)) { + const hasUpdates = retVal.some(a => a.res.updatedAccounts.length > 0); + if (hasUpdates) { connection.send('sync-event', { type: 'success', tables: ['transactions'], }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/loot-core/src/server/accounts/sync.ts(7 hunks)packages/loot-core/src/server/main.ts(4 hunks)packages/loot-core/src/types/models/gocardless.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/models/gocardless.d.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/main.ts (3)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/server/main.ts:1188-1198
Timestamp: 2024-10-25T00:12:14.939Z
Learning: In `packages/loot-core/src/server/main.ts`, when handling errors, the error properties `error_code` and `error_type` may already be mapped to `code` and `category` before being passed to `handleSyncError`.
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/server/main.ts:1096-1102
Timestamp: 2024-10-25T00:17:33.196Z
Learning: In this codebase, including `err.stack` in error responses returned to clients is acceptable to maintain consistent behavior.
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.
🔇 Additional comments (4)
packages/loot-core/src/server/accounts/sync.ts (2)
72-99: LGTM! Well-structured utility functions.
The new utility functions are well-implemented with proper type safety, date validation, and error handling. The 90-day sync limit is a good practice for compatibility with GoCardless.
792-824: LGTM! Well-structured account sync implementation.
The syncAccount function is well-implemented with:
- Proper use of new utility functions
- Clear provider-specific logic separation
- Good error handling for unknown providers
packages/loot-core/src/server/main.ts (2)
1059-1074: Clean implementation of sync response handling!
The function efficiently processes sync responses by:
- Correctly spreading array elements instead of using concat
- Properly tracking both new and matched transactions
- Updating the accounts list only when changes are detected
1163-1225: Well-structured batch sync implementation!
The handler efficiently implements batch synchronization for SimpleFin accounts with proper error handling and response processing.
|
I don't know if you want to do this now, or save for a future PR, but the function to get the oldest account transaction should exclude deleted transactions. I don't know if it does and I don't see anything in the query here to filter out deleted. |
Digging through the aql code it looks like, unless specified, deleted transactions are excluded - happy to be corrected here though as I'm not 100% |
I was wondering if the aql query would do that by default. We can move forward as is then |
This picks up where actualbudget/actual-server#384 left off, and implements bulk sync in the front end for SimpleFin.
It's definitely not polished, but it does at least work. Please throw any and all suggestions this way, I'm sure there are improvements to be made.
--- update ---
open for review now!