Apply Template to All Categories in Group for Web #3666
Conversation
✅ 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 |
…emplate-apply # Conflicts: # packages/desktop-client/src/components/budget/index.tsx
…emplate-apply # Conflicts: # packages/loot-core/src/server/api.ts # packages/loot-core/src/types/api-handlers.d.ts # packages/loot-core/src/types/server-handlers.d.ts
Dreptschar
changed the title
WalkthroughThis pull request introduces a new prop, Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (14)
packages/desktop-client/src/components/budget/ExpenseGroup.tsx (1)
28-30: LGTM! Consider minor formatting adjustment for readability.The addition of
onApplyBudgetTemplatesInGroupprop is correct and consistent with the changes described in the AI summary. The prop is properly typed as optional and derives its type from theSidebarGroupcomponent, ensuring type consistency.For improved readability, consider formatting the type definition on a single line:
onApplyBudgetTemplatesInGroup?: ComponentProps<typeof SidebarGroup>['onApplyBudgetTemplatesInGroup'];packages/loot-core/src/types/api-handlers.d.ts (1)
152-155: LGTM! Minor typo in comment.The new method
'api/apply-multiple-templates'is a good addition to theApiHandlersinterface. It follows the existing pattern and provides a clear interface for applying multiple templates.Please fix the typo in the comment:
- categoryIds: string[]; //categoy ids + categoryIds: string[]; //category idspackages/api/methods.ts (1)
152-154: LGTM! Consider adding type annotations and error handling.The new
applyMultipleTemplatesfunction is well-integrated and follows the established patterns in the codebase. However, to improve type safety and error handling, consider the following enhancements:
- Add type annotations for the parameters and return value.
- Implement input validation or error handling.
Here's a suggested improvement:
export async function applyMultipleTemplates(month: string, categoryIds: string[]): Promise<void> { if (!month || !Array.isArray(categoryIds) || categoryIds.length === 0) { throw new Error('Invalid input: month and categoryIds are required'); } return send('api/apply-multiple-templates', { month, categoryIds }); }This change adds type annotations, input validation, and makes the function asynchronous to properly handle the Promise returned by
send.packages/desktop-client/src/components/budget/SidebarGroup.tsx (2)
36-36: LGTM: New prop added for applying budget templates to multiple categories.The
onApplyBudgetTemplatesInGroupprop is well-named and correctly typed as an optional function. It's appropriate for the intended functionality of applying budget templates to multiple categories.Consider using a more specific type for the
categoriesparameter:onApplyBudgetTemplatesInGroup?: (categoryIds: string[]) => void;This would make it clear that we're expecting an array of category IDs rather than full category objects.
129-132: LGTM: New menu item for applying multiple category templates added.The implementation of the new menu item for applying multiple category templates is well-done:
- It's conditionally rendered based on the
isGoalTemplatesEnabledfeature flag.- The menu item is appropriately placed in the existing menu structure.
- The
onApplyBudgetTemplatesInGroupcall efficiently extracts category IDs.- The text is correctly internationalized.
Consider adding a null check before calling
onApplyBudgetTemplatesInGroup:if (onApplyBudgetTemplatesInGroup) { onApplyBudgetTemplatesInGroup(group.categories.map(c => c['id'])); }This would prevent potential runtime errors if the prop is not provided.
Also applies to: 144-151
packages/desktop-client/src/components/budget/BudgetTable.jsx (2)
31-31: LGTM! Consider adding prop documentation.The addition of the
onApplyBudgetTemplatesInGroupprop is consistent with the new feature for applying budget templates to a group. This change aligns with the PR objectives and the AI-generated summary.Consider adding a JSDoc comment for this new prop to improve code documentation and maintainability. For example:
/** * @param {Function} onApplyBudgetTemplatesInGroup - Callback function to apply budget templates to a group */
239-239: LGTM! Consider future refactoring for prop drilling.The
onApplyBudgetTemplatesInGroupprop is correctly passed down to theBudgetCategoriescomponent, which is consistent with the earlier change and follows React patterns.As the application grows, consider using React Context or a state management library like Redux to avoid excessive prop drilling if this pattern becomes common across multiple levels of components.
packages/loot-core/src/types/server-handlers.d.ts (1)
90-93: Approve new method with minor improvements.The new
'apply-multiple-templates'method is a good addition to theServerHandlersinterface. It provides functionality to apply multiple templates to categories for a specific month.
- Fix the typo in the comment:
- categoryIds: string[]; //categoy ids + categoryIds: string[]; // category ids
- Consider adding a brief description of the method's purpose:
/** * Applies multiple budget templates to specified categories for a given month. * @param month The target month for applying templates. * @param categoryIds An array of category IDs to apply templates to. * @returns A promise that resolves to a Notification object. */ 'apply-multiple-templates': (arg: { month: string; categoryIds: string[]; }) => Promise<Notification>;packages/loot-core/src/client/actions/queries.ts (1)
240-248: LGTM with a suggestion for error handling.The new
applyBudgetTemplatesInGroupfunction is well-implemented and follows the established patterns in the file. It correctly integrates with the existing Redux architecture and API communication.Consider adding error handling to provide a better user experience:
export function applyBudgetTemplatesInGroup(month, categoryIds) { return async function (dispatch: Dispatch) { - dispatch( - addNotification( - await send('apply-multiple-templates', { month, categoryIds }), - ), - ); + try { + const result = await send('apply-multiple-templates', { month, categoryIds }); + dispatch(addNotification(result)); + } catch (error) { + dispatch(addGenericErrorNotification()); + } }; }This change will ensure that any API errors are caught and a generic error notification is displayed to the user, maintaining consistency with error handling in other functions like
deleteCategory.packages/desktop-client/src/components/budget/index.tsx (3)
269-271: LGTM: New function added for applying budget templates to multiple categories.The
onApplyBudgetTemplatesInGroupfunction is implemented correctly, following the component's existing patterns for dispatching actions. It appropriately uses the currentstartMonthand the providedcategorieswhen dispatching the action.Consider adding error handling to catch and handle any potential errors that might occur during the action dispatch:
const onApplyBudgetTemplatesInGroup = async categories => { try { await dispatch(applyBudgetTemplatesInGroup(startMonth, categories)); } catch (error) { console.error('Error applying budget templates:', error); // Optionally, dispatch an action to show an error notification to the user } };
374-374: LGTM: New prop added correctly to DynamicBudgetTable for envelope budgets.The
onApplyBudgetTemplatesInGroupprop is correctly added to theDynamicBudgetTablecomponent for envelope budgets. This addition is consistent with the implementation of the new function and maintains the existing component structure.Consider updating the prop types for
DynamicBudgetTableto include this new prop. This will improve type safety and documentation. You can do this by updating the component's prop type definition:type DynamicBudgetTableProps = { // ... existing props onApplyBudgetTemplatesInGroup?: (categories: Category[]) => Promise<void>; };Replace
Categorywith the appropriate type for your category objects.
Action Implementation Missing for
applyBudgetTemplatesInGroupThe
applyBudgetTemplatesInGroupaction is not found inpackages/loot-core/src/client/actions, indicating it may not be implemented.Additionally, no test files related to budget functionality were found in
packages/desktop-client/src/components/budget.Please implement the
applyBudgetTemplatesInGroupaction and add corresponding unit tests.🔗 Analysis chain
Line range hint
1-464: Summary: New functionality for applying budget templates to multiple categoriesThe changes in this file successfully implement the ability to apply budget templates to multiple categories within a group for envelope budgets. The implementation is well-integrated into the existing
BudgetInnercomponent and follows established patterns in the codebase.Key points:
- A new action
applyBudgetTemplatesInGroupis imported and used.- A new function
onApplyBudgetTemplatesInGroupis added to handle the action dispatch.- The
DynamicBudgetTablecomponent receives a new prop to support this functionality for envelope budgets.These changes enhance the budget management capabilities of the application, allowing for more efficient template application across multiple categories.
To ensure the new functionality is properly integrated, please verify the following:
- The
applyBudgetTemplatesInGroupaction is correctly implemented in the actions file.- The
DynamicBudgetTablecomponent correctly handles the newonApplyBudgetTemplatesInGroupprop.- There are appropriate unit tests for this new functionality.
You can use the following script to check for the action implementation and potential test files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for action implementation echo "Checking for applyBudgetTemplatesInGroup action implementation:" rg "export const applyBudgetTemplatesInGroup" packages/loot-core/src/client/actions # Check for potential test files echo "\nChecking for potential test files:" fd "budget.*test" packages/desktop-client/src/components/budgetLength of output: 371
packages/loot-core/src/server/api.ts (1)
617-626: LGTM! Consider adding explicit error handling.The new handler
api/apply-multiple-templatesis well-integrated with the existing codebase. It follows the established patterns for mutation handling and file open checks.Consider adding explicit error handling to provide more informative API responses. For example:
handlers['api/apply-multiple-templates'] = withMutation(async function ({ month, categoryIds, }) { checkFileOpen(); - return handlers['apply-multiple-templates']({ - month, - categoryIds, - }); + try { + return await handlers['apply-multiple-templates']({ + month, + categoryIds, + }); + } catch (error) { + throw APIError('Failed to apply multiple templates', error); + } });This change would provide more context in case of failures, making it easier for API consumers to handle errors.
packages/loot-core/src/server/main.ts (1)
395-405: LGTM! Consider adding explicit error handling.The implementation of the
apply-multiple-templateshandler looks good. It correctly uses themutatorandwithUndofunctions, and properly passes the received parameters togoalActions.applyMultipleCategoryTemplates.Consider adding explicit error handling to catch and handle any potential errors from
goalActions.applyMultipleCategoryTemplates. This would improve the robustness of the handler:handlers['apply-multiple-templates'] = mutator(async function ({ month, categoryIds, }) { return withUndo(async () => { - return await goalActions.applyMultipleCategoryTemplates({ - month, - categoryIds, - }); + try { + return await goalActions.applyMultipleCategoryTemplates({ + month, + categoryIds, + }); + } catch (error) { + console.error('Error applying multiple templates:', error); + throw error; // or handle it appropriately + } }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3666.mdis excluded by!**/*.md
📒 Files selected for processing (12)
- packages/api/methods.ts (1 hunks)
- packages/desktop-client/src/components/budget/BudgetCategories.jsx (2 hunks)
- packages/desktop-client/src/components/budget/BudgetTable.jsx (2 hunks)
- packages/desktop-client/src/components/budget/ExpenseGroup.tsx (3 hunks)
- packages/desktop-client/src/components/budget/SidebarGroup.tsx (5 hunks)
- packages/desktop-client/src/components/budget/index.tsx (3 hunks)
- packages/loot-core/src/client/actions/queries.ts (1 hunks)
- packages/loot-core/src/server/api.ts (1 hunks)
- packages/loot-core/src/server/budget/goaltemplates.ts (4 hunks)
- packages/loot-core/src/server/main.ts (2 hunks)
- packages/loot-core/src/types/api-handlers.d.ts (2 hunks)
- packages/loot-core/src/types/server-handlers.d.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (11)
packages/desktop-client/src/components/budget/ExpenseGroup.tsx (3)
49-49: LGTM! Prop correctly added to function parameters.The
onApplyBudgetTemplatesInGroupprop is correctly added to the destructured parameters of theExpenseGroupfunction, consistent with the changes made to theExpenseGroupPropstype.
Line range hint
1-138: Overall, the changes look good and are consistent with the PR objectives.The implementation of the new
onApplyBudgetTemplatesInGroupprop in theExpenseGroupcomponent is correct and follows good practices. The prop is consistently added to the component's props type, function parameters, and passed down to theSidebarGroupcomponent. These changes align with the PR objectives of enhancing template management capabilities.To ensure full compatibility, make sure that the
SidebarGroupcomponent is updated to handle this new prop correctly, as suggested in the verification script provided earlier.
132-132: LGTM! Prop correctly passed to SidebarGroup component.The
onApplyBudgetTemplatesInGroupprop is correctly passed down to theSidebarGroupcomponent, which is consistent with the changes described in the AI summary.To ensure the
SidebarGroupcomponent is prepared to handle this new prop, please run the following script:packages/desktop-client/src/components/budget/SidebarGroup.tsx (4)
6-6: LGTM: Import statement for useFeatureFlag added.The import statement for
useFeatureFlagis correctly placed and uses an appropriate relative path. This hook will be useful for managing feature flags within the component.
52-52: LGTM: onApplyBudgetTemplatesInGroup prop added to SidebarGroup component.The
onApplyBudgetTemplatesInGroupprop is correctly added to the component's destructured props. Its position in the list is appropriate and consistent with the component's interface.
58-58: LGTM: Feature flag for goal templates added.The
isGoalTemplatesEnabledconstant is correctly implemented using theuseFeatureFlaghook. Its placement at the beginning of the component is a good practice for feature flags, making it easy to manage and update feature availability.
Line range hint
1-238: Summary: Well-implemented feature for applying budget templates to multiple categories.The changes to the
SidebarGroupcomponent successfully implement a new feature for applying budget templates to multiple categories. Key points:
- A new prop
onApplyBudgetTemplatesInGroupis added to handle the template application.- The feature is gated behind a feature flag
goalTemplatesEnabled.- A new menu item is conditionally rendered based on the feature flag.
- The implementation is well-integrated into the existing component structure.
The code is clean, follows good practices, and is appropriately internationalized. The use of a feature flag allows for easy enabling/disabling of the feature.
To ensure the feature is properly integrated across the codebase, please run the following verification script:
This script will help ensure that the new feature is consistently implemented and that there are no outstanding tasks related to it.
✅ Verification successful
Verified: Integration of the new feature is consistent and without issues.
- The
onApplyBudgetTemplatesInGroupprop is correctly passed and utilized across relevant components.- The
goalTemplatesEnabledfeature flag is properly implemented throughout the codebase.- No related TODOs or FIXMEs impacting this feature were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new feature across the codebase # Test 1: Check if the onApplyBudgetTemplatesInGroup prop is passed correctly echo "Checking prop usage:" rg "onApplyBudgetTemplatesInGroup" packages/desktop-client/src/components/budget/ # Test 2: Verify the feature flag usage echo "\nChecking feature flag usage:" rg "goalTemplatesEnabled" packages/desktop-client/src/ # Test 3: Look for any TODOs or FIXMEs related to this feature echo "\nChecking for related TODOs or FIXMEs:" rg -i "todo|fixme" -A 3 -B 3 packages/desktop-client/src/components/budget/Length of output: 3961
packages/desktop-client/src/components/budget/BudgetTable.jsx (1)
Line range hint
1-245: Summary: New prop added for applying budget templates to groupsThe changes in this file are minimal and focused, adding a new prop
onApplyBudgetTemplatesInGroupto theBudgetTablecomponent and passing it down to theBudgetCategoriescomponent. These changes are consistent with the PR objectives and the AI-generated summary, implementing the functionality to apply budget templates within a group context.The implementation follows React best practices and doesn't introduce any apparent issues. The new feature is well-integrated into the existing component structure.
packages/desktop-client/src/components/budget/BudgetCategories.jsx (2)
31-31: LGTM: New prop added correctly.The new prop
onApplyBudgetTemplatesInGrouphas been added to the component's prop list. This addition is consistent with the existing structure and naming conventions.
Line range hint
1-365: Summary: New prop added for applying budget templates to groups.The changes in this file are minimal and well-implemented. A new prop
onApplyBudgetTemplatesInGrouphas been added to theBudgetCategoriescomponent and correctly passed down to theExpenseGroupcomponent. These changes enhance the component's functionality without introducing any apparent issues or side effects.To ensure full implementation:
- Verify that the
ExpenseGroupcomponent has been updated to handle this new prop.- Check if any parent components passing props to
BudgetCategorieshave been updated to include this new prop.- Confirm that the actual functionality for applying budget templates to groups has been implemented in the relevant part of the application.
packages/desktop-client/src/components/budget/index.tsx (1)
9-9: LGTM: New action import added correctly.The
applyBudgetTemplatesInGroupaction is imported correctly, following the existing import structure and naming conventions.
| @@ -245,6 +246,7 @@ export const BudgetCategories = memo( | |||
| onReorderCategory={onReorderCategory} | |||
| onToggleCollapse={onToggleCollapse} | |||
| onShowNewCategory={onShowNewCategory} | |||
| onApplyBudgetTemplatesInGroup={onApplyBudgetTemplatesInGroup} | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue: onApplyBudgetTemplatesInGroup prop is not handled in BudgetTable.jsx.
The onApplyBudgetTemplatesInGroup prop is passed to the ExpenseGroup component in BudgetCategories.jsx, but it is not utilized within the BudgetTable.jsx where ExpenseGroup is now defined. This may result in the new functionality not being executed as intended.
- Verify the implementation of
onApplyBudgetTemplatesInGroupwithin theBudgetTable.jsxcomponent to ensure the prop is handled correctly. - Ensure that any related logic for applying budget templates is appropriately integrated after the refactoring.
🔗 Analysis chain
LGTM: New prop correctly passed to ExpenseGroup.
The onApplyBudgetTemplatesInGroup prop is correctly passed down to the ExpenseGroup component. This change is minimal and doesn't affect the existing logic of the BudgetCategories component.
To ensure the new functionality is properly implemented, please verify that the ExpenseGroup component has been updated to handle this new prop. Run the following script to check the ExpenseGroup component:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ExpenseGroup component handles the new prop
# Test: Search for the onApplyBudgetTemplatesInGroup prop in ExpenseGroup component
rg -p 'onApplyBudgetTemplatesInGroup' packages/desktop-client/src/components/budget/ExpenseGroup.jsx
Length of output: 203
Script:
#!/bin/bash
# Description: Search for the ExpenseGroup component across the entire codebase
rg 'ExpenseGroup' --type jsx
Length of output: 56
Script:
#!/bin/bash
# Description: Search for the ExpenseGroup component across all JavaScript and JSX files in the codebase
# Search for 'ExpenseGroup' in all .js and .jsx files
rg 'ExpenseGroup' --glob '*.js' --glob '*.jsx'
Length of output: 1088
Script:
#!/bin/bash
# Description: Verify that ExpenseGroup component in BudgetTable.jsx handles the new prop
# Search for 'onApplyBudgetTemplatesInGroup' in BudgetTable.jsx
rg 'onApplyBudgetTemplatesInGroup' packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
Length of output: 103
| const multipleCategoryTemplates = []; | ||
| for (let dd = 0; dd < category.length; dd++) { | ||
| const categoryId = category[dd].id; | ||
| if (templates[categoryId] !== undefined) { | ||
| multipleCategoryTemplates[categoryId] = templates[categoryId]; | ||
| multipleCategoryTemplates[categoryId] = multipleCategoryTemplates[ | ||
| categoryId | ||
| ].filter(t => t.directive === directive); | ||
| } | ||
| } | ||
| return multipleCategoryTemplates; |
There was a problem hiding this comment.
Use an object instead of an array for multipleCategoryTemplates
In getTemplates, multipleCategoryTemplates is initialized as an empty array [], but it's being used as an object with category IDs as keys. To avoid confusion and potential issues, initialize multipleCategoryTemplates as an empty object {}.
Apply this diff to fix:
- const multipleCategoryTemplates = [];
+ const multipleCategoryTemplates = {};📝 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.
| const multipleCategoryTemplates = []; | |
| for (let dd = 0; dd < category.length; dd++) { | |
| const categoryId = category[dd].id; | |
| if (templates[categoryId] !== undefined) { | |
| multipleCategoryTemplates[categoryId] = templates[categoryId]; | |
| multipleCategoryTemplates[categoryId] = multipleCategoryTemplates[ | |
| categoryId | |
| ].filter(t => t.directive === directive); | |
| } | |
| } | |
| return multipleCategoryTemplates; | |
| const multipleCategoryTemplates = {}; | |
| for (let dd = 0; dd < category.length; dd++) { | |
| const categoryId = category[dd].id; | |
| if (templates[categoryId] !== undefined) { | |
| multipleCategoryTemplates[categoryId] = templates[categoryId]; | |
| multipleCategoryTemplates[categoryId] = multipleCategoryTemplates[ | |
| categoryId | |
| ].filter(t => t.directive === directive); | |
| } | |
| } | |
| return multipleCategoryTemplates; |
| const placeholders = categoryIds.map(() => '?').join(', '); | ||
| const query = `SELECT * FROM v_categories WHERE id IN (${placeholders})`; | ||
| const categories = await db.all(query, categoryIds); |
There was a problem hiding this comment.
Handle empty categoryIds to prevent SQL syntax errors
If categoryIds is an empty array, the generated SQL query will have an empty IN clause (WHERE id IN ()), leading to a SQL syntax error. Add a check to ensure categoryIds is not empty before constructing and executing the query.
Apply this diff to fix the issue:
export async function applyMultipleCategoryTemplates({ month, categoryIds }) {
+ if (categoryIds.length === 0) {
+ return { type: 'error', message: 'No category IDs provided.' };
+ }
const placeholders = categoryIds.map(() => '?').join(', ');
const query = `SELECT * FROM v_categories WHERE id IN (${placeholders})`;Committable suggestion was skipped due to low confidence.
|
This is applying to hidden categories. That needs fixed to work like the regular apply's that ignore the hidden categories |
Got it! Will work on excluding them. |
|
Ok, this seems to work well functionally. Templates like percent of available funds and remainders are a little weird like this, but are weird when running on their own too, so not a big deal. |
| @@ -122,6 +126,12 @@ export function SidebarGroup({ | |||
| onDelete(group.id); | |||
| } else if (type === 'toggle-visibility') { | |||
| onSave({ ...group, hidden: !group.hidden }); | |||
| } else if (type === 'apply-multiple-category-template') { | |||
There was a problem hiding this comment.
Do you really need to mess with the categories both here and the template file? It would be nice if you could stick to just one or the other.
There was a problem hiding this comment.
The other goaltemplate function also work only with categoryIds so thats why i filter them before. I can also send the whole list of categories and use that if you think thats cleaner
There was a problem hiding this comment.
Its fine as is, but if the two side could be consolidated as much as possible to not need the extra steps on both sides, that would be preferred. Could you pass the category group from here and not filter to categories?
There was a problem hiding this comment.
So i thought about it again and i think we should leave it to use categorieIds so we can reuse it. Ive seen a issue where people would like to apply the template to selected categories and that would be easy added with using that function no ?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/server/budget/types/handlers.d.ts (1)
82-86: Add TypeDoc comments for the new method.The method signature looks good and aligns with the existing patterns. Consider adding TypeDoc comments to document the method's purpose and parameters.
+ /** + * Applies budget templates to multiple categories for a specific month. + * @param {Object} arg - The arguments object + * @param {string} arg.month - The target month in YYYY-MM format + * @param {string[]} arg.categoryIds - Array of category IDs to apply templates to + * @returns {Promise<Notification>} A promise that resolves with a notification of the operation result + */ 'budget/apply-multiple-templates': (arg: { month: string; categoryIds: string[]; }) => Promise<Notification>;packages/loot-core/src/types/api-handlers.d.ts (1)
Line range hint
1-150: Add missing API endpoint definition.Based on the PR objectives and AI summary, the
'api/apply-multiple-templates'endpoint is missing from theApiHandlersinterface. This endpoint is crucial for applying budget templates to multiple categories at once.Add the following endpoint definition to the
ApiHandlersinterface:'api/apply-multiple-templates': (arg: { month: string; categoryIds: string[]; }) => Promise<Notification>;🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'Notification' is defined but never used. Allowed unused vars must match /^(_|React)/upackages/loot-core/src/types/server-handlers.d.ts (1)
Line range hint
1-600: Add missing handler for applying multiple templates.Based on the PR objectives and AI summary, a new server handler is needed to support applying templates to all categories in a group. This handler should be added to the
ServerHandlersinterface.Add the following handler definition:
export interface ServerHandlers { + 'apply-multiple-templates': (arg: { + month: string; + categoryIds: string[]; + force?: boolean; + }) => Promise<void>; // ... existing handlers }The new handler:
- Takes a month and an array of category IDs as required parameters
- Includes an optional
forceparameter to support overwriting existing budgets- Returns a Promise that resolves when the operation is complete
🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'Notification' is defined but never used. Allowed unused vars must match /^(_|React)/u
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/desktop-client/src/components/budget/index.tsx (3 hunks)
- packages/loot-core/src/client/actions/queries.ts (1 hunks)
- packages/loot-core/src/server/budget/app.ts (1 hunks)
- packages/loot-core/src/server/budget/types/handlers.d.ts (1 hunks)
- packages/loot-core/src/server/main.ts (1 hunks)
- packages/loot-core/src/types/api-handlers.d.ts (1 hunks)
- packages/loot-core/src/types/server-handlers.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/client/actions/queries.ts
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/budget/index.tsx
[failure] 9-9:
Module '"loot-core/src/client/actions"' has no exported member 'applyBudgetTemplatesInGroup'.
🪛 GitHub Check: lint
packages/desktop-client/src/components/budget/index.tsx
[warning] 9-9:
'applyBudgetTemplatesInGroup' is defined but never used. Allowed unused vars must match /^(_|React)/upackages/loot-core/src/server/main.ts
[warning] 43-43:
'goalActions' is defined but never used. Allowed unused vars must match /^(_|React)/upackages/loot-core/src/types/api-handlers.d.ts
[warning] 1-1:
'Notification' is defined but never used. Allowed unused vars must match /^(_|React)/upackages/loot-core/src/types/server-handlers.d.ts
[warning] 1-1:
'Notification' is defined but never used. Allowed unused vars must match /^(_|React)/u
🔇 Additional comments (4)
packages/loot-core/src/server/budget/app.ts (1)
32-35: Implementation looks good but needs verification of requirements.The new method follows the established patterns and integrates well with existing template-related functionality.
Let's verify the implementation of requirements discussed in PR comments:
Consider adding a comment above the method to document:
- Whether it skips hidden categories
- Whether it overwrites existing budgets
- The behavior regarding budget fluctuations
packages/loot-core/src/types/api-handlers.d.ts (1)
1-1: Skip the unused import warning.The
Notificationtype is used in the return type of the missing API endpoint, so this warning is a false positive.🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'Notification' is defined but never used. Allowed unused vars must match /^(_|React)/upackages/desktop-client/src/components/budget/index.tsx (1)
379-379: Verify template application restriction to envelope budgetsThe template application feature is only added to envelope budgets and not tracking budgets. This appears intentional, but please confirm this aligns with the requirements.
packages/loot-core/src/server/main.ts (1)
43-43:⚠️ Potential issueImplementation appears incomplete
The
goalActionsimport is currently unused. Based on the PR objectives and past review comments:
- The handler for applying multiple templates appears to be missing
- Consider using direct function references from
goalActionsinstead of creating a new API handler, as suggested in the previous review discussionLet's verify the intended usage pattern:
🧰 Tools
🪛 GitHub Check: lint
[warning] 43-43:
'goalActions' is defined but never used. Allowed unused vars must match /^(_|React)/u
|
|
||
| 'budget/apply-multiple-templates': (arg: { | ||
| month: string; | ||
| categoryIds: string[]; | ||
| }) => Promise<Notification>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation constraints for categoryIds.
To ensure type safety and prevent runtime errors, consider adding constraints to validate the categoryIds array.
'budget/apply-multiple-templates': (arg: {
month: string;
- categoryIds: string[];
+ categoryIds: readonly string[] & { length: number };
}) => Promise<Notification>;This change:
- Makes the array readonly to prevent accidental modifications
- Ensures the array is not empty through the length constraint
Committable suggestion was skipped due to low confidence.
| @@ -1,3 +1,4 @@ | |||
| import type { Notification } from '../client/state-types/notifications'; | |||
There was a problem hiding this comment.
Remove unused import.
The Notification type is imported but not used in any method signatures.
-import type { Notification } from '../client/state-types/notifications';📝 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.
| import type { Notification } from '../client/state-types/notifications'; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'Notification' is defined but never used. Allowed unused vars must match /^(_|React)/u
| @@ -6,6 +6,7 @@ | |||
| import { | |||
| addNotification, | |||
| applyBudgetAction, | |||
| applyBudgetTemplatesInGroup, | |||
There was a problem hiding this comment.
Fix missing export in actions module
The import applyBudgetTemplatesInGroup is not exported from 'loot-core/src/client/actions'. This will cause compilation errors.
Please ensure the function is properly exported from the actions module. The implementation might look like:
// In loot-core/src/client/actions/index.ts
export { applyBudgetTemplatesInGroup } from './budget';🧰 Tools
🪛 GitHub Check: typecheck
[failure] 9-9:
Module '"loot-core/src/client/actions"' has no exported member 'applyBudgetTemplatesInGroup'.
🪛 GitHub Check: lint
[warning] 9-9:
'applyBudgetTemplatesInGroup' is defined but never used. Allowed unused vars must match /^(_|React)/u
| const onApplyBudgetTemplatesInGroup = async categories => { | ||
| dispatch( | ||
| applyBudgetAction(startMonth, 'apply-multiple-templates', { | ||
| month: startMonth, | ||
| categories, | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety
The implementation could be enhanced with proper error handling and TypeScript types.
Consider applying these improvements:
- const onApplyBudgetTemplatesInGroup = async categories => {
+ const onApplyBudgetTemplatesInGroup = async (categories: string[]) => {
+ try {
dispatch(
applyBudgetAction(startMonth, 'apply-multiple-templates', {
month: startMonth,
categories,
}),
);
+ dispatch(
+ addNotification({
+ type: 'success',
+ message: t('Successfully applied templates to categories'),
+ })
+ );
+ } catch (error) {
+ dispatch(
+ addNotification({
+ type: 'error',
+ message: t('Failed to apply templates: {{error}}', { error: error.message }),
+ })
+ );
+ }
};Committable suggestion was skipped due to low confidence.
Address missing requirements from PR discussion
Based on the PR discussion, two key requirements are not addressed in the current implementation:
- Excluding hidden categories from template application
- Clarifying the behavior for existing budgets (overwrite vs. skip)
Consider updating the implementation to:
const onApplyBudgetTemplatesInGroup = async (categories: string[]) => {
+ // Filter out hidden categories
+ const visibleCategories = categories.filter(id => {
+ const category = categoryGroups
+ .flatMap(g => g.categories)
+ .find(c => c.id === id);
+ return category && !category.hidden;
+ });
+
+ // Confirm overwrite if any categories have existing budgets
dispatch(
applyBudgetAction(startMonth, 'apply-multiple-templates', {
month: startMonth,
- categories,
+ categories: visibleCategories,
+ overwrite: true, // Based on PR discussion about overwriting existing budgets
}),
);
};Committable suggestion was skipped due to low confidence.
|
@youngcw i would like to add this also for the mobile view but im not sure where to add the button. |
|
All I know is that it will be inside the mobile components somewhere |
No my question was where do you think the button fits better ? :D just from a ui point of view. |
|
This is ready to merge i think i will ad the Mobile part in a new PR if that's okay. |
|
@youngcw Will this be merged soon ? |
|
not until after the merge freeze |
Adds Feature from #3040