Extract budget category related server handlers from main.ts to server/budget/app.ts #4442
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. |
joel-jeremy
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
WalkthroughThe changes update various aspects of the budget management functionality. In the test budget module, the code now assigns a boolean value directly to Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/loot-core/src/server/budget/app.ts (8)
17-53: Consider grouping or splitting the interface
This interface houses many budget-related handlers in one place. If it grows further, consider grouping related handlers or splitting them into smaller interfaces to maintain readability.
146-198: Large function with repetitive array pushes
The repeated pattern of pushing cell values could be extracted into a helper function to reduce duplication and improve clarity, especially as more values get added.
200-244: Similar logic to envelopeBudgetMonth
This function’s structure largely mirrorsenvelopeBudgetMonth. Refactor common code to a shared utility to minimize duplication and improve maintainability.
269-287: Use robust error identification
Catching a unique constraint violation by checking the error message text is fragile. If possible, use a dedicated error code or numeric identifier to minimize false positives.
303-347: Refactor for clarity
This deletion logic (checking category existence, transferring items, deleting) is lengthy. Consider breaking it into smaller helper functions to handle separate responsibilities more cleanly.
353-367: Validate group name
You may consider validating the uniqueness of the group name or verifying it doesn’t conflict with existing groups.
385-406: Consistent with deleteCategory
This function handles optional transfer similarly. Ensure the transfer group is valid and consistent with the group’s income/expense type if that becomes a requirement.
408-435: Performance consideration
Scanning all months for non-zero budget allocations is acceptable for typical use. If the set of months grows significantly, consider caching or partial queries for better performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4442.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/loot-core/src/mocks/budget.ts(1 hunks)packages/loot-core/src/server/budget/app.ts(2 hunks)packages/loot-core/src/server/budget/types/handlers.d.ts(0 hunks)packages/loot-core/src/server/db/index.ts(3 hunks)packages/loot-core/src/server/main.ts(0 hunks)packages/loot-core/src/types/handlers.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/loot-core/src/server/budget/types/handlers.d.ts
- packages/loot-core/src/server/main.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
🔇 Additional comments (14)
packages/loot-core/src/types/handlers.d.ts (1)
3-3: LGTM! Import path updated correctly.The import path for
BudgetHandlershas been updated to reflect its new location inserver/budget/app.ts, which aligns with the PR's objective of moving budget category related server handlers.packages/loot-core/src/mocks/budget.ts (1)
706-706: LGTM! Simplified boolean assignment.The code now directly assigns the boolean value to
isIncomeinstead of converting it to an integer, which is a cleaner approach.packages/loot-core/src/server/db/index.ts (2)
354-382: LGTM! Enhanced type safety for insertCategoryGroup.The function now has an explicit return type
Promise<CategoryGroupEntity['id']>and properly typed variables, which improves type safety and code clarity.
411-464: LGTM! Enhanced type safety for insertCategory.The function now has an explicit return type
Promise<CategoryEntity['id']>and properly typed variables, which improves type safety and code clarity.packages/loot-core/src/server/budget/app.ts (10)
1-9: Imports look good
No issues found with these new import statements.
13-16: No revision needed
These lines solely introduce new module imports or whitespace. Nothing further to address.
120-134: Consistent method registration
All newly added method registrations properly distinguish between read-only and mutating operations by wrapping only the latter withundoable. Good job preserving consistency.
135-140: Check for empty results
If no categories exist, this function currently returns empty arrays forgroupedandlist. That behavior might be acceptable, but consider whether a special response or additional error handling is needed if the database call fails unexpectedly.
142-144: Consider error boundary
Returningawait budget.createAllBudgets()is straightforward, but if budget creation fails, ensure the caller handles the exception gracefully.
246-267: Verify group identity
Currently, only the presence ofgroupIdis checked before inserting a new category. For added robustness, consider verifying that the referenced group actually exists and is valid.
289-301: All good
This moveCategory function cleanly callsdb.moveCategorywithin batchMessages, ensuring batched updates.
349-351: All good
Straightforward retrieval of category groups from the database.
369-371: No immediate concerns
Callingdb.updateCategoryGroup(group)directly is concise and consistent with your existing pattern.
373-383: All good
Moving the category group remains consistent with the pattern of wrapping DB interactions inbatchMessages.
joel-jeremy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/loot-core/src/server/budget/app.ts (2)
137-142: Consider adding error handling for database operations.The getCategories function should handle potential database errors.
Consider wrapping the database calls in try-catch:
async function getCategories() { + try { return { grouped: await getCategoryGroups(), list: (await db.getCategories()).map(categoryModel.fromDb), }; + } catch (error) { + throw APIError('Failed to fetch categories', error); + } }
412-439: Add documentation for the isCategoryTransferRequired function.The function's purpose and return value should be documented.
Add JSDoc documentation:
+/** + * Checks if a category needs to be transferred before deletion. + * @param {Object} params - The parameters object + * @param {number} params.id - The category ID to check + * @returns {Promise<boolean>} True if the category has transactions or non-zero budget values + */ async function isCategoryTransferRequired({ id, }: { id: CategoryEntity['id']; })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/loot-core/src/server/api.ts(2 hunks)packages/loot-core/src/server/budget/app.ts(2 hunks)packages/loot-core/src/server/db/index.ts(4 hunks)packages/loot-core/src/server/models.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/db/index.ts
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/loot-core/src/server/models.ts
[failure] 89-89:
Type '{ [k: string]: any; }' is missing the following properties from type 'DbCategory': id, name, is_income, cat_group, and 3 more.
[failure] 122-122:
Type '{ [k: string]: any; }' is missing the following properties from type 'DbCategoryGroup': id, name, is_income, sort_order, and 2 more.
🪛 GitHub Actions: Test
packages/loot-core/src/server/models.ts
[error] 89-89: Type '{ [k: string]: any; }' is missing the following properties from type 'DbCategory': id, name, is_income, cat_group, and 3 more.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (5)
packages/loot-core/src/server/api.ts (2)
36-36: LGTM! Improved model separation with renamed import.The renaming of
categoryGroupModeltoserverCategoryGroupModelimproves clarity by explicitly indicating its server-side usage.
358-359: LGTM! Enhanced data transformation with proper model conversion.The changes properly transform database entities to server models using the dedicated conversion method.
packages/loot-core/src/server/models.ts (1)
131-150: LGTM! Well-structured category group transformation.The fromDb method properly handles the transformation of category groups and their associated categories.
packages/loot-core/src/server/budget/app.ts (2)
18-54: LGTM! Well-defined comprehensive budget handlers interface.The BudgetHandlers interface provides a clear contract for all budget-related operations.
271-289: LGTM! Robust error handling for category updates.The updateCategory function properly handles unique constraint violations and other errors.
| toDb( | ||
| category: CategoryEntity, | ||
| { update }: { update?: boolean } = {}, | ||
| ): DbCategory { | ||
| return update | ||
| ? convertForUpdate(schema, schemaConfig, 'categories', category) | ||
| : convertForInsert(schema, schemaConfig, 'categories', category); | ||
| }, |
There was a problem hiding this comment.
Fix type safety issues in toDb method.
The method needs proper type handling to satisfy the DbCategory interface requirements.
Apply this fix to ensure type safety:
toDb(
category: CategoryEntity,
{ update }: { update?: boolean } = {},
): DbCategory {
- return update
+ const result = update
? convertForUpdate(schema, schemaConfig, 'categories', category)
: convertForInsert(schema, schemaConfig, 'categories', category);
+ return result as DbCategory;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toDb( | |
| category: CategoryEntity, | |
| { update }: { update?: boolean } = {}, | |
| ): DbCategory { | |
| return update | |
| ? convertForUpdate(schema, schemaConfig, 'categories', category) | |
| : convertForInsert(schema, schemaConfig, 'categories', category); | |
| }, | |
| toDb( | |
| category: CategoryEntity, | |
| { update }: { update?: boolean } = {}, | |
| ): DbCategory { | |
| const result = update | |
| ? convertForUpdate(schema, schemaConfig, 'categories', category) | |
| : convertForInsert(schema, schemaConfig, 'categories', category); | |
| return result as DbCategory; | |
| }, |
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 89-89:
Type '{ [k: string]: any; }' is missing the following properties from type 'DbCategory': id, name, is_income, cat_group, and 3 more.
🪛 GitHub Actions: Test
[error] 89-89: Type '{ [k: string]: any; }' is missing the following properties from type 'DbCategory': id, name, is_income, cat_group, and 3 more.
| toDb( | ||
| categoryGroup: CategoryGroupEntity, | ||
| { update }: { update?: boolean } = {}, | ||
| ): DbCategoryGroup { | ||
| return update | ||
| ? convertForUpdate(schema, schemaConfig, 'category_groups', categoryGroup) | ||
| : convertForInsert( | ||
| schema, | ||
| schemaConfig, | ||
| 'category_groups', | ||
| categoryGroup, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Fix type safety issues in categoryGroupModel.toDb method.
Similar to categoryModel, this method needs proper type handling.
Apply this fix:
toDb(
categoryGroup: CategoryGroupEntity,
{ update }: { update?: boolean } = {},
): DbCategoryGroup {
- return update
+ const result = update
? convertForUpdate(schema, schemaConfig, 'category_groups', categoryGroup)
: convertForInsert(
schema,
schemaConfig,
'category_groups',
categoryGroup,
);
+ return result as DbCategoryGroup;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toDb( | |
| categoryGroup: CategoryGroupEntity, | |
| { update }: { update?: boolean } = {}, | |
| ): DbCategoryGroup { | |
| return update | |
| ? convertForUpdate(schema, schemaConfig, 'category_groups', categoryGroup) | |
| : convertForInsert( | |
| schema, | |
| schemaConfig, | |
| 'category_groups', | |
| categoryGroup, | |
| ); | |
| }, | |
| toDb( | |
| categoryGroup: CategoryGroupEntity, | |
| { update }: { update?: boolean } = {}, | |
| ): DbCategoryGroup { | |
| const result = update | |
| ? convertForUpdate(schema, schemaConfig, 'category_groups', categoryGroup) | |
| : convertForInsert( | |
| schema, | |
| schemaConfig, | |
| 'category_groups', | |
| categoryGroup, | |
| ); | |
| return result as DbCategoryGroup; | |
| }, |
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 122-122:
Type '{ [k: string]: any; }' is missing the following properties from type 'DbCategoryGroup': id, name, is_income, sort_order, and 2 more.
|
Looks like the category autocomplete's aren't working. To repro try transferring money from one category to another, or deleting a funded category and transferring to another. There are some tests failing too |
joel-jeremy
force-pushed
the
server-budget-category-handlers
branch
2 times, most recently
from
d6fa68d to
c816e80
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/loot-core/src/server/models.ts (2)
90-107: New toDb and fromDb methods increase clarity.The separation between the domain entity (
CategoryEntity) and the DB representation (DbCategory) is consistent. Consider adding basic type or field checks after the conversion for stronger guarantees, especially for fields likename,cat_group, etc.
125-164: Ensure category references belong to the correct group.Filtering by
cat_group === categoryGroup.idinfromDbprotects against unrelated categories. Since the query presumably already limits categories to this group, the filter is likely redundant. Removing it might reduce overhead. However, retaining it is safer if the data constraints are uncertain.packages/loot-core/src/server/budget/app.ts (9)
14-17: Base, cleanup-template, and goaltemplates usage.Bringing in these modules helps isolate specific budget-related operations. This separation aligns well with the PR's objective to refactor budget logic out of
main.ts. Check that all references to older budget code have been removed or properly migrated.
136-142: getCategories method returns grouped and list formats.Returning both grouped and flat lists is convenient for UIs, but be mindful of performance overhead with large data sets. If expansions grow, consider lazy-loading or pagination.
144-146: getBudgetBounds usage.
budget.createAllBudgets()can be an expensive call if it calculates across large datasets. Monitor performance in production to ensure it scales for many months/categories.
148-200: envelopeBudgetMonth function handles multi-group iteration.Collecting budget values from the spreadsheet for each group and category is thorough. Consider caching repeated calls to
value(...)orsheet.getCellValue(...)if performance becomes an issue at scale.
271-291: updateCategory handles unique constraint errors.Gracefully returning
{ error: { type: 'category-exists' } }is user friendly. Consider adding more context to the error, such as which name conflicts.
359-363: getCategoryGroups returns properly converted entities.The code fetches DB rows, then transforms them to domain entities. Ensure local testing includes category groups with no categories to confirm robust conversion logic.
365-379: createCategoryGroup normalizes inputs.As with createCategory, booleans are coerced to integers. This is consistent. If category group naming collisions are possible, consider handling or explicitly ignoring them.
385-395: moveCategoryGroup function uses batchMessages.Like moveCategory, batch operations help keep synchronization events consistent. Double-check that
targetIdis valid to avoid potential ordering conflicts.
420-447: isCategoryTransferRequired checks transactions and budget cells.This thorough approach ensures that categories with existing transactions or non-zero budget values cannot be deleted without a transfer. Confirm that tombstoned transactions or archived months are handled as intended in all edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4442.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/loot-core/src/mocks/budget.ts(1 hunks)packages/loot-core/src/server/api.ts(2 hunks)packages/loot-core/src/server/budget/app.ts(2 hunks)packages/loot-core/src/server/budget/types/handlers.d.ts(0 hunks)packages/loot-core/src/server/db/index.ts(7 hunks)packages/loot-core/src/server/main.ts(0 hunks)packages/loot-core/src/server/models.ts(4 hunks)packages/loot-core/src/types/handlers.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/loot-core/src/server/main.ts
- packages/loot-core/src/server/budget/types/handlers.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/loot-core/src/types/handlers.d.ts
- packages/loot-core/src/mocks/budget.ts
- packages/loot-core/src/server/api.ts
- packages/loot-core/src/server/db/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (12)
packages/loot-core/src/server/models.ts (4)
7-13: Imports look good.These imports from
./aqland./dbhelp cleanly separate concerns for database conversion utilities and DB types. Ensure there are no circular dependencies resulting from these additions, and verify that the schema references inschema&schemaConfigremain stable when used elsewhere.
76-89: Check for omitted sort_order in the returned object.
sort_orderis destructured and discarded. IfDbCategoryrequiressort_order, it might cause type or functional discrepancies. Confirm that it's genuinely optional inDbCategoryor consider preserving it if required.
112-124: Validate thatis_incomeis only optional for insert.
is_incomeis checked only on update in therequiredFieldscall. Confirm whether new category groups also requireis_income. If it’s essential for newly created groups, ensure it’s validated consistently.
60-72:❓ Verification inconclusive
Confirm that required fields match the DbAccount definition.
Casting
Partial<DbAccount>toDbAccountrequires validating all mandatory fields. The call torequiredFieldsonly checks'name','offbudget', and'closed'. Confirm whetherDbAccounthas additional compulsory properties (e.g.,id), and ensure their presence.Run the following script to look for properties declared in
DbAccountthat might not be enforced byrequiredFields:
🏁 Script executed:
#!/bin/bash # Searching for DbAccount interface definition and listing its fields: ast-grep --pattern $'interface DbAccount {\n $$$\n}'Length of output: 52
It appears that our initial ast-grep search did not return any output, which makes it difficult to determine if there are additional mandatory fields in the DbAccount interface (such as an
idproperty) that are not being validated by ourrequiredFieldscall. Please run the above command to search for theDbAccountinterface definition using ripgrep. Once we have the output, we can confirm whether the list of required fields in thevalidate()method is complete.Action Required:
- Review the output of the above script: Ensure that the
DbAccountinterface does not specify additional compulsory properties besides'name','offbudget', and'closed'.- Update the code if necessary: If extra mandatory fields are found (e.g.,
id), update therequiredFieldscall accordingly.
🏁 Script executed:
#!/bin/bash # Use ripgrep to locate the definition of DbAccount within the repository. rg "interface DbAccount" -n .Length of output: 31
Manual Verification Required: Confirm Additional Mandatory Fields in DbAccount
The automated searches for the
DbAccountdefinition using bothast-grepandrgdid not yield any output. This makes it unclear whetherDbAccountincludes additional mandatory properties—such asid—that are not validated by the currentrequiredFieldscall. Please manually verify the complete definition ofDbAccountin the codebase and ensure that all required fields are being checked within thevalidate()method inpackages/loot-core/src/server/models.ts.
- Action Items:
- Manually locate the complete definition of
DbAccountin your repository.- Confirm if the interface (or type) includes any mandatory properties beyond
'name','offbudget', and'closed'.- Update the
requiredFieldscall accordingly if additional required properties are found.packages/loot-core/src/server/budget/app.ts (8)
1-10: Imports promote modular design.Importing utilities from
monthUtils,db, andsheetfosters a well-organized codebase. Verify that these dependencies don't produce cyclical imports and confirmmonthUtils.sheetForMonthusage won't break when invoked across different modules.
18-54: New BudgetHandlers interface centralizes budget-related methods.Centralizing these methods under one interface clarifies usage and fosters maintainability. Confirm that external calls referencing old handler names (from
main.ts) have been updated accordingly to avoid runtime errors.
121-135: Registering new budget category endpoints with app.method.These lines allow the server to expose new and refactored budget endpoints. Ensure test coverage on each method, especially for corner cases.
248-269: createCategory enforces groupId existence.This ensures categories cannot exist without a group. Good job on validating user input and normalizing booleans into integers for
is_income/hidden. More advanced validation logic (e.g., checking group existence) can further prevent orphaned records.
293-305: moveCategory grouping logic.
batchMessagesusage is a solid approach for ensuring only one group-level message is emitted for a sequence of category moves. Confirm that reordering logic withtargetIdis stable across different edge cases (e.g.,targetIdequalsid).
307-357: deleteCategory handles is_income matching and optional transfer.The function carefully checks if the target category has the same
is_incometype. This is crucial for preventing invalid merges. Confirm that the UI properly prompts the user to handle transfers for categories with budget history or leftover amounts.
381-383: updateCategoryGroup delegates to categoryGroupModel.toDb.This is consistent with the earlier patterns. Confirm that changes requiring
update-only validations (e.g.,is_income) are tested if needed for partial updates.
397-418: deleteCategoryGroup handles group-wide category transfers.This logic merges categories into another group if
transferIdis provided. Confirm that any transactions tied to the old group remain valid. Testing with partial group merges is recommended.
| async function trackingBudgetMonth({ month }: { month: string }) { | ||
| const groups = await db.getCategoriesGrouped(); | ||
| const sheetName = monthUtils.sheetForMonth(month); | ||
|
|
||
| function value(name: string) { | ||
| const v = sheet.getCellValue(sheetName, name); | ||
| return { value: v === '' ? 0 : v, name: resolveName(sheetName, name) }; | ||
| } | ||
|
|
||
| let values = [ | ||
| value('total-budgeted'), | ||
| value('total-budget-income'), | ||
| value('total-saved'), | ||
| value('total-income'), | ||
| value('total-spent'), | ||
| value('real-saved'), | ||
| value('total-leftover'), | ||
| ]; | ||
|
|
||
| for (const group of groups) { | ||
| values = values.concat([ | ||
| value(`group-budget-${group.id}`), | ||
| value(`group-sum-amount-${group.id}`), | ||
| value(`group-leftover-${group.id}`), | ||
| ]); | ||
|
|
||
| const categories = group.categories ?? []; | ||
|
|
||
| for (const cat of categories) { | ||
| values = values.concat([ | ||
| value(`budget-${cat.id}`), | ||
| value(`sum-amount-${cat.id}`), | ||
| value(`leftover-${cat.id}`), | ||
| value(`goal-${cat.id}`), | ||
| value(`long-goal-${cat.id}`), | ||
| ]); | ||
|
|
||
| if (!group.is_income) { | ||
| values.push(value(`carryover-${cat.id}`)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return values; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
trackingBudgetMonth function similarly iterates across groups.
Much like envelopeBudgetMonth, repeated calls to value(...) might be expensive. If real-time usage is needed, confirm that the overhead is acceptable or implement caching.
joel-jeremy
force-pushed
the
server-budget-category-handlers
branch
from
f27ee4e to
cb9a201
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/loot-core/src/server/models.ts (2)
90-99: Robust conversion to DB.
CallingconvertForUpdateorconvertForInsertbased on theupdateflag is a clean way to handle DB writes. Consider adding light error-handling if these conversion functions may throw.
125-144: toDb method for category groups.
Your approach mirrorscategoryModel.toDb, promoting consistency. As with categories, consider minimal error-handling if the conversion fails instead of letting it bubble unhandled.packages/loot-core/src/server/budget/app.ts (4)
248-269: createCategory with minimal validation.
CheckinggroupIdis required, but consider disallowing empty or whitespace-onlyname.trim()as it may lead to placeholder or default naming issues.
271-291: updateCategory handles unique constraint errors.
Catching and returning an error object is a neat approach to handle DB conflicts. Complement it with checks for empty or invalid category names, mirroring recommendations in createCategory.
307-357: deleteCategory with partial transfer logic.
This function handles the expense category transfer but leaves a TODO for income categories. Ensure the code covers all relevant scenarios or provide a fallback to avoid data inconsistencies.Would you like help implementing that TODO for income categories?
365-379: createCategoryGroup.
As with categories, consider stricter validation (e.g., trim name, etc.) if you want to prevent accidental blank group names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4442.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/loot-core/src/mocks/budget.ts(1 hunks)packages/loot-core/src/server/api.ts(2 hunks)packages/loot-core/src/server/budget/app.ts(2 hunks)packages/loot-core/src/server/budget/types/handlers.d.ts(0 hunks)packages/loot-core/src/server/db/index.ts(7 hunks)packages/loot-core/src/server/main.ts(0 hunks)packages/loot-core/src/server/models.ts(4 hunks)packages/loot-core/src/types/handlers.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/loot-core/src/server/main.ts
- packages/loot-core/src/server/budget/types/handlers.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/loot-core/src/mocks/budget.ts
- packages/loot-core/src/server/api.ts
- packages/loot-core/src/types/handlers.d.ts
- packages/loot-core/src/server/db/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (21)
packages/loot-core/src/server/models.ts (8)
7-14: Imports look appropriate.
The new imports from'./aql'and'./db'provide clarity and consolidate the conversion methods and types in one place. This organization helps maintain cleaner code.
60-63: Partial type validation for DbAccount.
Castingaccountdirectly toDbAccountafter usingrequiredFieldsis a concise approach, but be mindful that any additional or removed fields in the future might not raise compile-time issues ifrequiredFieldsis not updated accordingly. Overall, this aligns well with your type-safety checks.Also applies to: 71-71
76-79: Consistent approach for Partial.
Similar to the account validation, the use ofPartial<DbCategory>here ensures that only necessary fields are validated, which matches the optionalupdateflow.
88-89: Omission of sort_order.
Excludingsort_orderfrom the returned object may be intentional, but verify whether downstream logic or UI relies on this property.
100-107: Straightforward DB-to-entity conversion.
UsingconvertFromSelectto mapDbCategorytoCategoryEntityis consistent and clear. This helps keep model logic centralized.
112-112: Validate method for DbCategoryGroup.
Enforcing required fields throughrequiredFieldsfor both insert and update is consistent with the category model’s pattern.Also applies to: 114-114
123-124: Filtering out sort_order again.
Like incategoryModel, ensure omittingsort_orderhere is truly intended and doesn't break ordering logic in the UI or reporting.
146-163: Transforming DB groups back to entities.
LeveragingconvertFromSelectfor the main group data and then mapping categories is a neat separation of concerns. Good job keeping the logic consistent across models.packages/loot-core/src/server/budget/app.ts (13)
1-14: Refactored imports.
These import statements consolidate references todb,models,sheet, and various utilities. The structure indicates that budget-specific logic is properly isolated here, which aligns with the PR’s goal of organizing budget code.
18-54: New BudgetHandlers interface.
Defining all budget operations within a single interface clarifies code organization and fosters discoverability. This structure should simplify future maintenance.
121-134: Registering budget methods.
Binding your new and existing functions toapp.methodin a single location is consistent with the rest of the codebase and helps keep the route definitions tidy.
136-142: getCategories returns immediate domain objects.
Converting from DB viacategoryModel.fromDbat the server layer ensures that clients (or other layers) receive only domain-level data. This is a clean separation of concerns.
144-146: Straightforward getBudgetBounds.
Simply returningbudget.createAllBudgets()keeps the logic minimal. Verify that any errors fromcreateAllBudgets()are acceptable to bubble up rather than being handled at this level.
148-200: Repeated spreadsheet lookups (envelopeBudgetMonth).
This function callssheet.getCellValuerepeatedly. If performance becomes critical, consider batching retrievals or caching results.
202-246: Parallel logic for trackingBudgetMonth.
LikeenvelopeBudgetMonth, this function also makes multiplesheet.getCellValuecalls in a loop, potentially impacting performance under heavy loads.
293-305: moveCategory.
This is a clean, focused wrapper that delegates logic todb.moveCategory. Maintaining segregation of DB queries promotes modular design.
359-363: getCategoryGroups returns domain objects.
CallingcategoryGroupModel.fromDbensures the response is domain-centric, which is consistent withgetCategories.
381-383: updateCategoryGroup.
This straightforward method is consistent with the rest of the CRUD operations, delegating conversions to the model for type safety.
385-395: moveCategoryGroup.
This block method is similarly minimal, matching the pattern inmoveCategory.
397-418: deleteCategoryGroup with optional transfer.
Transferring all categories in the group prior to deletion is a pragmatic approach. This logic prevents orphaned categories or incomplete references.
420-447: isCategoryTransferRequired checks usage across transactions and budget values.
Efficiently scanning transactions first, then checking budget months is a logical approach. Ensure the coverage aligns with any new budget concepts or additional references that could also force transfers.
Related to #1113