Breaking: Add support for multiple YNAB budgets #647
Conversation
WalkthroughThe changes update the system to support multiple YNAB budgets by modifying the data structure for account mappings. The updates include new interfaces and function types to handle account-specific transaction exports, along with changes to several methods for budget-specific processing. User interface components are updated to display the new budget identifier field. In addition, test and configuration files are adjusted to reflect the new nested object structure for mapping account numbers to YNAB budget and account IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as ExportTransactionsForAccountFunction
participant M as Mapping Service
participant Y as YNAB Service
participant T as Transaction Creator
U->>F: Request export for specific account
F->>M: Retrieve mapping for accountNumber
M-->>F: Return { ynabBudgetId, ynabAccountId }
F->>Y: getTransactions(ynabBudgetId, startDate)
Y-->>F: Transactions data
F->>T: Filter & create transactions
T-->>F: Return creation result
F-->>U: Export result delivered
Possibly related PRs
Suggested labels
Poem
✨ 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: 1
🧹 Nitpick comments (10)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts (5)
43-67: Ensure categories are initialized correctly before use.
IncreateTransactions, you callawait initCategories()only ifbudgetCategoriesMapis empty. This might skip re-initializing categories if they are outdated or if budgets have changed since the last fetch. Consider adding logic or a timestamp-based check to ensure the map is up to date or forcibly reinitialized if needed.
148-156: Check for missing categories.
You retrievebudgetIdfromoriginalTransaction.accountNumberand then callgetYnabCategoryIdFromCategoryName(budgetId, categoryName). IfbudgetCategoriesMapwas never populated due to concurrency or timing issues, the function returnsnull. Ensure downstream logic can handle anullcategory gracefully.
180-186: Consolidate error handling for missing budget ID.
If the user’s config lacks a validynabBudgetId, you throw an error. For better UX, consider a user-friendly fallback or a combined error message describing both missing account ID and missing budget ID.
192-196: Confirm category name collisions are handled.
getYnabCategoryIdFromCategoryNamefetches a category by label. If two categories share the same name in the same budget, it will pick the first. Consider clarifying or documenting the expected behavior if multiple categories share a name.
223-243: Separate transaction fetch and filtering logic.
You now filter out existing YNAB transactions by first retrieving them withgetTransactions(...). This is fine, but consider splitting into smaller helper methods or caching logic for clarity and testability. Making the function do less (fetch then filter in distinct steps) can streamline future debugging.packages/main/src/backend/commonTypes.ts (1)
103-111: Improve code discoverability.
ExportTransactionsForAccountParamsandExportTransactionsForAccountFunctionare well-defined to handle per-account transactions. It might be helpful to add short JSDoc comments describing real-world usage or how these new types integrate with multi-budget logic, especially for maintainers unfamiliar with the domain.packages/renderer/src/components/exporters/YnabAccountMappingTable.tsx (3)
50-64: Expose helpful default or hint text.
The new column forynabBudgetIduses a dropdown, but if the budgets list is empty, the dropdown might be blank. Consider showing a placeholder option or a message if no budgets are available to guide the user.
71-82: Filter accounts by active budget.
When retrieving account options, you filter only active YNAB accounts for the chosenynabBudgetId. This makes sense, but ensure that closed or deleted YNAB accounts are intentionally excluded, and consider whether a user might need to see them for historical transactions.
127-135: Preserve data consistency on array-to-object transformation.
You correctly build the newmappingObjectwith bothynabBudgetIdandynabAccountId. Confirm that the row editing logic prevents duplicates or collisions on the sameaccountNumber. If collisions are possible, you may need an additional check.packages/renderer/src/store/Store.test.tsx (1)
159-162: LGTM! Consider adding test cases for multiple budgets.The test data structure correctly implements the new YNAB account mapping format. However, since this PR adds support for multiple budgets, consider adding test cases where accounts map to different budget IDs to ensure the feature works as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/renderer/src/store/__snapshots__/Store.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/main/src/backend/commonTypes.ts(2 hunks)packages/main/src/backend/export/outputVendors/ynab/ynab.ts(9 hunks)packages/preload/src/commonTypes.ts(1 hunks)packages/renderer/src/components/exporters/YnabAccountMappingTable.tsx(4 hunks)packages/renderer/src/store/Store.test.tsx(1 hunks)packages/renderer/src/types.tsx(1 hunks)
🔇 Additional comments (11)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts (5)
22-24: Use descriptive naming or add clarifying comments.
These lines introduce a newYnabCategorytype alias and a globalbudgetCategoriesMap. The naming is clear, but consider adding clarifying comments explaining whybudgetCategoriesMapis structured as a nested map keyed bybudgetIdand thencategoryName.
69-130: Improve resilience for unmapped accounts.
The newcreateTransactionsForAccountfunction throws an error if the account is unmapped and then logs a progress event. This flow is fine for clarity, but consider whether you want to offer a default fallback or automatically skip the account silently depending on user needs. Also, verify that the logic out of the try/catch always sets the correct budget ID, or else the call tocreateTransactionsmight fail silently if a mismatch occurs.
132-134: Validate thebudgetIdusage.
getTransactions(budgetId, startDate)is now budget-specific, which aligns well with multi-budget support. Double-check any callers of this function to confirm they always pass a validbudgetId, especially for legacy workflows that might assume a single budget.
177-178: Consider fallback for legacy configurations.
Now returningynabAccountIdfrom an object, but older configurations might only store a single string. If this is a hard-breaking change, confirm you’ve accounted for existing data migrations or user instructions for manual updates.
357-361: Refresh categories from a single budget.
getYnabCategories(budgetId)retrieves category names for the provided budget ID. Ensure that in multi-budget contexts, the UI or caller logic re-calls this function for the correct budget and merges or replaces categories as needed.packages/main/src/backend/commonTypes.ts (1)
68-68: Breaking change for existing Config.
accountNumbersToYnabAccountIdsnow maps each string to an object with bothynabBudgetIdandynabAccountId. This is a meaningful schema change, so confirm that existing configurations are migrated or that you have instructions for users to manually update.packages/renderer/src/components/exporters/YnabAccountMappingTable.tsx (2)
18-25: Clarify required vs. optional fields.
interface AccountMappingincludesynabBudgetIdandynabAccountId, which must be chosen by the user. If there’s a permitted scenario where either is undefined, consider marking them optional or adding validation.
102-103: Initialize new rows with correct default.
ynabBudgetId: budgetIdon new rows might not always reflect the user’s actual desired budget, especially if the user wants to fill out multiple budgets. A default is fine, but consider whether to prompt the user to choose from a list if you anticipate more than one valid budget.packages/preload/src/commonTypes.ts (2)
70-70: LGTM! Interface update aligns with multi-budget support.The updated
accountNumbersToYnabAccountIdstype correctly supports the new multi-budget feature by including both budget and account IDs.
235-235: Verify the purpose of theuseReactUIproperty.This property appears unrelated to the YNAB multi-budget feature. Please clarify its purpose and consider moving it to a separate PR if unrelated.
packages/renderer/src/types.tsx (1)
73-73: LGTM! Consistent interface implementation.The YnabConfig interface update matches the changes in other files, maintaining type consistency throughout the application.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| getOptions: (_: any, { row }: { row: AccountMapping }) => { | ||
| const budgetId = (row as AccountMapping).ynabBudgetId; |
There was a problem hiding this comment.
I have no idea how to properly type this.
getOptions isn't covered by DefinitelyTyped, the documentation doesn't indicate the type, and the source code is untyped JavaScript. The only hint was the example here.
There was a problem hiding this comment.
I passed the same way you went through (if I was reading your comment completely before trying to solve this... 😂), and then I found out we don't need to research this.
You can read about this here:
https://stackoverflow.com/a/63254304/839513
We use any to ignore type checking, which may hide errors, and we try to avoid it.
But if you use unknown, it will prevent you from accessing and manipulating the object, which is exactly the situation here, we don't know the type of _ and we don't want to use it.
There was a problem hiding this comment.
Sounds good, I'll change this tomorrow
| @@ -70,7 +70,7 @@ export interface GoogleSheetsConfig extends OutputVendorConfigBase { | |||
| export interface YnabConfig extends OutputVendorConfigBase { | |||
| options: { | |||
| accessToken: string; | |||
| accountNumbersToYnabAccountIds: Record<string, string>; | |||
| accountNumbersToYnabAccountIds: Record<string, { ynabBudgetId: string; ynabAccountId: string }>; | |||
| budgetId: string; | |||
There was a problem hiding this comment.
The reason I'm keeping the "original" budgetId is to pre-populate new rows in the config with it. This way users do not have to reselect a budget for every account if they just use a single budget. This budgetId is not used at all other than that.
However, I think it might be a bit confusing for the user that they have to define a "main" budget Id and an individual Ynab budget id per account.
I can see 2 alternative:
- Rename the title in the UI from
budget idtodefault budget id, maybe even add a tooltip that explains it's only used for pre-populating new rows. - Remove it, and use the budget id of the last row (if exists) for pre-populating new rows.
- This might be troublesome, because in my experience, the rows do not retain their original order on changes.
whatuserever
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts (4)
43-67: Consider adding error handling for Promise.all.While the parallel execution of transactions is good, if one account fails, it could cause all accounts to fail. Consider handling errors per account to ensure partial success.
- const results = await Promise.all(promises); - const exportedTransactionsNums = results.map((v) => v.exportedTransactionsNum); + const results = await Promise.allSettled(promises); + const exportedTransactionsNums = results + .filter((result): result is PromiseFulfilledResult<{ exportedTransactionsNum: number }> => + result.status === 'fulfilled') + .map((result) => result.value.exportedTransactionsNum);
69-130: Enhance error messages for better debugging.The error messages could be more specific about which step failed during transaction creation.
- throw new Error(`Unhandled account number ${transactionAccountNumber}`); + throw new Error(`Failed to find YNAB mapping for account number ${transactionAccountNumber}. Please check your configuration.`);- console.error('Failed to create transactions in ynab', e); + console.error(`Failed to create ${transactionsThatDontExistInYnab.length} transactions in YNAB for account ${accountNumber}:`, e);
24-25: Consider implementing cache expiration for transactionsFromYnab.The
transactionsFromYnabMap caches transactions by date but doesn't have an expiration mechanism. This could lead to memory issues over time.Consider implementing a simple LRU cache or adding an expiration mechanism:
-const transactionsFromYnab = new Map<Date, ynab.TransactionDetail[]>(); +const transactionsFromYnab = new Map<Date, { transactions: ynab.TransactionDetail[], timestamp: number }>(); if (transactionsFromYnab.has(startDate)) { - transactionsInYnabBeforeCreatingTheseTransactions = transactionsFromYnab.get(startDate)!; + const cached = transactionsFromYnab.get(startDate)!; + if (Date.now() - cached.timestamp < 3600000) { // 1 hour expiration + transactionsInYnabBeforeCreatingTheseTransactions = cached.transactions; + } else { + transactionsFromYnab.delete(startDate); + } }Also applies to: 230-237
247-263: Improve type safety and readability in transaction comparison.
- The
@ts-expect-errorsuggests a type safety issue that should be addressed.- The magic number
1000in the amount comparison could be more readable with a named constant.+const AMOUNT_COMPARISON_THRESHOLD = 1000; // Represents 1 unit in YNAB's milliunits + export function isSameTransaction( transactionToCreate: ynab.SaveTransaction, transactionFromYnab: ynab.TransactionDetail, ) { const isATransferTransaction = !!transactionFromYnab.transfer_account_id; const isTransactionsImportIdEqual = isSameImportId(transactionToCreate, transactionFromYnab); + const transactionAmount = transactionToCreate.amount ?? 0; return ( transactionToCreate.account_id === transactionFromYnab.account_id && transactionToCreate.date === transactionFromYnab.date && - // @ts-expect-error error TS18049: 'transactionToCreate.amount' is possibly 'null' or 'undefined' - Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000 && + Math.abs(transactionAmount - transactionFromYnab.amount) < AMOUNT_COMPARISON_THRESHOLD && (areStringsEqualIgnoreCaseAndWhitespace(transactionToCreate.payee_name, transactionFromYnab.payee_name) || isATransferTransaction || isTransactionsImportIdEqual) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (1)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts (1)
1-25: LGTM! Well-structured type definitions and data structures.The new type definitions and the introduction of
budgetCategoriesMapprovide a clean foundation for handling multiple budgets.
| @@ -70,7 +70,7 @@ export interface GoogleSheetsConfig extends OutputVendorConfigBase { | |||
| export interface YnabConfig extends OutputVendorConfigBase { | |||
| options: { | |||
| accessToken: string; | |||
| accountNumbersToYnabAccountIds: Record<string, string>; | |||
| accountNumbersToYnabAccountIds: Record<string, { ynabBudgetId: string; ynabAccountId: string }>; | |||
There was a problem hiding this comment.
Maybe instead of declaring a Record, you can declare an object:
accountNumbersToYnabAccountIds: {
[budgetId: string]: {
ynabBudgetId: string;
ynabAccountId: string
}
}I'm using this structure when it is not clear what is the key, and this give me the option to give a name to the key.
There was a problem hiding this comment.
Sure, will do.
btw, the key is accountNumber, not budgetId. Just further emphasizing your point 😅
There was a problem hiding this comment.
The linter does not allow this :/
error A record is preferred over an index signature @typescript-eslint/consistent-indexed-object-style
There was a problem hiding this comment.
I can do type AccountNumber = string and Record<AccountNumber, ...> if you find that better
This PR makes the following changes to support multiple YNAB accounts:
Ynab budget idcolumn to the configYnabConfigto also storeynabBudgetIdalongsideynabAccountIdYnab account idcolumn based on the budget selected on that rowYnab budget idon new rowsynab.tsto be based on the relevantYnab budget idImportant
This change will not work with existing configs!
I'm not sure how to support backward compatibility here due to the
YnabConfigchanges.If you have any suggestions do let me know.
Note that this PR uses #646 as a base.
I suggest going through the commits one-by-one when reviewing :)
Closes #645
Screenshot
Summary by CodeRabbit
These changes streamline transaction management and upgrade the account mapping workflow for better usability and integration.