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 No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
Warning Rate limit exceeded@joel-jeremy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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
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.
| async function deleteCategory({ | ||
| id, | ||
| transferId, | ||
| }: { | ||
| id: CategoryEntity['id']; | ||
| transferId: CategoryEntity['id']; | ||
| }): Promise<{ error?: 'no-categories' | 'category-type' }> { | ||
| let result = {}; | ||
| await batchMessages(async () => { | ||
| const row = await db.first( | ||
| 'SELECT is_income FROM categories WHERE id = ?', | ||
| [id], | ||
| ); | ||
| if (!row) { | ||
| result = { error: 'no-categories' }; | ||
| return; | ||
| } | ||
|
|
||
| const transfer = | ||
| transferId && | ||
| (await db.first('SELECT is_income FROM categories WHERE id = ?', [ | ||
| transferId, | ||
| ])); | ||
|
|
||
| if (!row || (transferId && !transfer)) { | ||
| result = { error: 'no-categories' }; | ||
| return; | ||
| } else if (transferId && row.is_income !== transfer.is_income) { | ||
| result = { error: 'category-type' }; | ||
| return; | ||
| } | ||
|
|
||
| // Update spreadsheet values if it's an expense category | ||
| // TODO: We should do this for income too if it's a reflect budget | ||
| if (row.is_income === 0) { | ||
| if (transferId) { | ||
| await budget.doTransfer([id], transferId); | ||
| } | ||
| } | ||
|
|
||
| await db.deleteCategory({ id }, transferId); | ||
| }); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for transferId parameter.
The deleteCategory function should validate transferId before using it.
Add validation at the start of the function:
async function deleteCategory({
id,
transferId,
}: {
id: CategoryEntity['id'];
transferId: CategoryEntity['id'];
}): Promise<{ error?: 'no-categories' | 'category-type' }> {
+ if (transferId && typeof transferId !== 'number') {
+ throw APIError('Invalid transferId parameter');
+ }
let result = {};
await batchMessages(async () => {
// ... rest of the function📝 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.
| async function deleteCategory({ | |
| id, | |
| transferId, | |
| }: { | |
| id: CategoryEntity['id']; | |
| transferId: CategoryEntity['id']; | |
| }): Promise<{ error?: 'no-categories' | 'category-type' }> { | |
| let result = {}; | |
| await batchMessages(async () => { | |
| const row = await db.first( | |
| 'SELECT is_income FROM categories WHERE id = ?', | |
| [id], | |
| ); | |
| if (!row) { | |
| result = { error: 'no-categories' }; | |
| return; | |
| } | |
| const transfer = | |
| transferId && | |
| (await db.first('SELECT is_income FROM categories WHERE id = ?', [ | |
| transferId, | |
| ])); | |
| if (!row || (transferId && !transfer)) { | |
| result = { error: 'no-categories' }; | |
| return; | |
| } else if (transferId && row.is_income !== transfer.is_income) { | |
| result = { error: 'category-type' }; | |
| return; | |
| } | |
| // Update spreadsheet values if it's an expense category | |
| // TODO: We should do this for income too if it's a reflect budget | |
| if (row.is_income === 0) { | |
| if (transferId) { | |
| await budget.doTransfer([id], transferId); | |
| } | |
| } | |
| await db.deleteCategory({ id }, transferId); | |
| }); | |
| return result; | |
| } | |
| async function deleteCategory({ | |
| id, | |
| transferId, | |
| }: { | |
| id: CategoryEntity['id']; | |
| transferId: CategoryEntity['id']; | |
| }): Promise<{ error?: 'no-categories' | 'category-type' }> { | |
| if (transferId && typeof transferId !== 'number') { | |
| throw APIError('Invalid transferId parameter'); | |
| } | |
| let result = {}; | |
| await batchMessages(async () => { | |
| const row = await db.first( | |
| 'SELECT is_income FROM categories WHERE id = ?', | |
| [id], | |
| ); | |
| if (!row) { | |
| result = { error: 'no-categories' }; | |
| return; | |
| } | |
| const transfer = | |
| transferId && | |
| (await db.first('SELECT is_income FROM categories WHERE id = ?', [ | |
| transferId, | |
| ])); | |
| if (!row || (transferId && !transfer)) { | |
| result = { error: 'no-categories' }; | |
| return; | |
| } else if (transferId && row.is_income !== transfer.is_income) { | |
| result = { error: 'category-type' }; | |
| return; | |
| } | |
| // Update spreadsheet values if it's an expense category | |
| // TODO: We should do this for income too if it's a reflect budget | |
| if (row.is_income === 0) { | |
| if (transferId) { | |
| await budget.doTransfer([id], transferId); | |
| } | |
| } | |
| await db.deleteCategory({ id }, transferId); | |
| }); | |
| return result; | |
| } |
Related to #1113