Apply Template to All Categories in Group for Mobile #3737
Conversation
…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
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. |
Dreptschar
changed the title
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 |
|
Ive add a new menu to groups. Idk if thats a good solution would love to hear some opinions |
WalkthroughThe pull request introduces several modifications across various components within the desktop client of the application, primarily focused on enhancing budget management functionalities. A new modal component, 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 (
|
Migrations out of sync modal![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (13)
packages/desktop-client/src/components/budget/envelope/BudgetMenuGroup.tsx (1)
39-39: Add translation context for better i18n.The translation key 'Apply budget template' might need additional context for translators to understand its usage in a group menu context.
-text: t('Apply budget template'), +text: t('Apply budget template', { + context: 'budgetGroupMenu' +}),packages/loot-core/src/server/budget/types/handlers.d.ts (1)
83-86: LGTM! Consider adding JSDoc documentation.The new method signature is well-structured and consistent with existing template-related methods. Consider adding JSDoc comments to document the purpose and parameters.
+ /** + * Applies budget templates to multiple categories within a specified month + * @param {Object} arg - The arguments object + * @param {string} arg.month - The target month for applying templates + * @param {string[]} arg.categoryIds - Array of category IDs to apply templates to + * @returns {Promise<Notification>} Notification indicating success or failure + */ 'budget/apply-multiple-templates': (arg: { month: string; categoryIds: string[]; }) => Promise<Notification>;packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (1)
64-71: Add accessibility label for budgeted amount.The "Budgeted" text should be properly associated with its value for screen readers.
<Text + accessibilityRole="label" + accessibilityHint="Shows the total budgeted amount for this group" style={{ fontSize: 17, fontWeight: 400, }} > Budgeted </Text>packages/desktop-client/src/components/budget/ExpenseGroup.tsx (1)
132-132: Consider state management optimization for mobile performance.While the implementation is correct, consider using a state management solution (e.g., Context API or Redux) to avoid prop drilling, especially for mobile performance optimization. This would be particularly beneficial as the application grows and more template-related features are added.
packages/desktop-client/src/components/budget/SidebarGroup.tsx (2)
129-134: Consider adding error handling for empty category lists.While the implementation is correct, consider adding a guard clause or user feedback when there are no visible categories in the group.
} else if (type === 'apply-multiple-category-template') { + const visibleCategories = group.categories + .filter(c => !c['hidden']) + .map(c => c['id']); + if (visibleCategories.length === 0) { + // Show user feedback that no categories are available + return; + } onApplyBudgetTemplatesInGroup?.( - group.categories - .filter(c => !c['hidden']) - .map(c => c['id']), + visibleCategories ); }
146-153: Consider using type-safe menu item structure.The menu items implementation is good, but consider defining a type for menu items to ensure type safety across the application.
type MenuItem = { name: string; text: string; disabled?: boolean; };packages/desktop-client/src/components/budget/BudgetTable.jsx (1)
31-31: LGTM! Consider adding PropTypes validation.The new prop follows consistent naming conventions and is well-placed in the props list.
Consider adding PropTypes validation for the new callback prop:
BudgetTable.propTypes = { // ... other props onApplyBudgetTemplatesInGroup: PropTypes.func };packages/loot-core/src/client/state-types/modals.d.ts (1)
177-181: LGTM! The new modal type is well-structured and consistent.The new
envelope-budget-group-menumodal type follows the established patterns and is properly integrated into the type system. It appropriately extends the budget management functionality to work at the group level.Consider adding JSDoc comments to document:
- The purpose of this modal
- The relationship between
monthandgroup- Expected behavior of
onApplyGroupTemplateThis would improve maintainability and help other developers understand the intended usage.
packages/loot-core/src/client/actions/queries.ts (1)
Line range hint
13-108: LGTM! Consider adding JSDoc comments.The new case integrates well with the existing
applyBudgetActionfunction. The implementation maintains consistency with the overall function structure and follows the established patterns for budget actions.To improve maintainability, consider adding JSDoc comments to document the expected shape of
argsfor each action type.Example documentation:
/** * Applies budget actions based on the specified type * @param month The target month * @param type The action type * @param args The action arguments * @param args.categories For 'apply-multiple-templates': Array of category IDs * @param args.category For single category actions: The target category ID * @param args.amount For amount-related actions: The amount to set/transfer */ export function applyBudgetAction(month, type, args) {packages/desktop-client/src/components/budget/BudgetCategories.jsx (1)
31-31: Consider adding PropTypes definition for the new callback prop.To improve type safety and documentation, consider adding PropTypes definition for
onApplyBudgetTemplatesInGroup.BudgetCategories.propTypes = { + onApplyBudgetTemplatesInGroup: PropTypes.func, };packages/desktop-client/src/components/budget/index.tsx (2)
268-275: Add TypeScript types and error handling.The function implementation looks good but could benefit from some improvements:
- Add type safety for the categories parameter
- Add error handling for failed dispatch
- Add validation for the categories array
Consider this implementation:
- const onApplyBudgetTemplatesInGroup = async categories => { + const onApplyBudgetTemplatesInGroup = async (categories: string[]) => { + if (!Array.isArray(categories) || categories.length === 0) { + return; + } + + try { dispatch( applyBudgetAction(startMonth, 'apply-multiple-templates', { month: startMonth, categories, }), ); + } catch (error) { + dispatch( + addNotification({ + type: 'error', + message: t('Failed to apply budget templates'), + }), + ); + } };
268-275: Add test coverage for the new template application feature.Consider adding tests to verify:
- Successful template application to multiple categories
- Error handling scenarios
- Integration with both mobile and desktop interfaces
Would you like me to help create test cases for this new functionality?
Also applies to: 378-378
packages/loot-core/src/server/budget/goaltemplates.ts (1)
206-206: Ensure consistent variable usage inprocessTemplateAssigning
categories = category;may cause confusion sincecategorycan be either a single category or an array of categories. To improve code clarity, consider ensuring that the variable names accurately reflect their content.If
categorycan be an array, consider renaming the parameter tocategoriesfor consistency.
📜 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 (14)
- packages/desktop-client/src/components/Modals.tsx (2 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/envelope/BudgetMenuGroup.tsx (1 hunks)
- packages/desktop-client/src/components/budget/index.tsx (2 hunks)
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (6 hunks)
- packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (1 hunks)
- packages/loot-core/src/client/actions/queries.ts (1 hunks)
- packages/loot-core/src/client/state-types/modals.d.ts (1 hunks)
- packages/loot-core/src/server/budget/app.ts (1 hunks)
- packages/loot-core/src/server/budget/goaltemplates.ts (4 hunks)
- packages/loot-core/src/server/budget/types/handlers.d.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (2)
Learnt from: UnderKoen PR: actualbudget/actual#3381 File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274 Timestamp: 2024-09-24T20:27:51.684Z Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.Learnt from: UnderKoen PR: actualbudget/actual#3381 File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274 Timestamp: 2024-10-08T15:46:15.739Z Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
🪛 GitHub Check: lint
packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx
[warning] 39-39:
'amountFocused' is assigned a value but never used. Allowed unused vars must match /^(_|React)/u
🔇 Additional comments (20)
packages/loot-core/src/server/budget/app.ts (1)
32-35: LGTM! The new method follows established patterns.The new
budget/apply-multiple-templatesmethod is well-integrated, following the consistent pattern of usingmutator(undoable())for state management and is appropriately positioned among other template-related methods.Let's verify the implementation of the underlying action:
✅ Verification successful
Implementation verified and follows best practices
The
applyMultipleCategoryTemplatesfunction is properly implemented ingoaltemplates.tswith appropriate database handling and follows the expected pattern for template operations. The function:
- Takes required parameters (
monthandcategoryIds)- Uses safe SQL query construction with placeholders
- Integrates with existing template management functions (
storeTemplates,getTemplates)- Is correctly exported and imported in the app configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of applyMultipleCategoryTemplates # Expected: Find the implementation and its usage # Look for the implementation echo "Searching for applyMultipleCategoryTemplates implementation:" ast-grep --pattern 'export const applyMultipleCategoryTemplates = $_' # Look for tests echo -e "\nSearching for related tests:" rg -l "applyMultipleCategoryTemplates" -g "*test*"Length of output: 3225
Script:
#!/bin/bash # Let's try a different approach to find the implementation # Search for the implementation using ripgrep echo "Searching for applyMultipleCategoryTemplates implementation:" rg -A 5 "applyMultipleCategoryTemplates" --type ts # Look for the file where it's imported from echo -e "\nSearching for imports:" rg "import.*applyMultipleCategoryTemplates" # Search for any references to this function echo -e "\nSearching for all references:" rg "applyMultipleCategoryTemplates"Length of output: 1774
packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (2)
1-23: LGTM! Well-organized imports and type definitions.The imports are properly structured and the type definitions are clear and concise.
50-50: Verify modal registration and mobile back handling.Ensure this modal is properly registered in the modal system and handles mobile back button/gesture appropriately.
✅ Verification successful
Let me gather more information about the modal system implementation to verify the mobile back handling.
Let me try to locate the modal component and its implementation first.
Let me check the modal state management and back handling implementation.
Based on the gathered information, I can now provide the final response:
Modal is properly integrated with the system
The modal is correctly registered in the modal system and uses the common Modal component which:
- Is managed through the global modal state system (
useModalState)- Automatically handles modal stack management
- Closes on location changes (route navigation)
- Uses the standard modal lifecycle hooks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the modal is properly registered and handles mobile back rg -A 5 "envelope-budget-group-menu" packages/desktop-client/src/components/Modals.tsxLength of output: 288
Script:
#!/bin/bash # Check modal system implementation and back handling ast-grep --pattern 'function $_($$_) { $$$ case "envelope-budget-group-menu": $$$ }' # Check for mobile back handling patterns rg -B 3 -A 5 "handleBackButton|onBackButtonPress" packages/desktop-client/src/components/modals/ # Check the parent component implementation rg -B 3 -A 10 "class Modal|function Modal" packages/desktop-client/src/components/modals/Modal.tsxLength of output: 388
Script:
#!/bin/bash # Find the Modal component location fd -e tsx -e ts -e js Modal # Check modal system implementation rg -B 3 -A 10 "class Modal|function Modal|const Modal" # Look for back button handling in the codebase rg -B 2 -A 5 "handleBackButton|onBackButtonPress|useBackHandler"Length of output: 11257
Script:
#!/bin/bash # Check modal state management rg -B 3 -A 5 "useModalState|closeModal|pushModal" packages/desktop-client/src/hooks/useModalState.ts # Check if there's a global back handler rg -B 3 -A 5 "BackHandler|useEffect.*back" packages/desktop-client/src/components/common/Modal.tsx # Look for mobile-specific handlers rg -B 3 -A 5 "mobile.*back|platform.*back" packages/desktop-client/src/Length of output: 580
packages/desktop-client/src/components/budget/ExpenseGroup.tsx (3)
28-30: LGTM! Type definition is well-structured.The type definition correctly leverages ComponentProps for type safety and maintains consistency with the existing prop types pattern.
49-49: LGTM! Props interface is properly updated.The new callback prop is well-positioned among other similar props, maintaining the component's interface consistency.
Line range hint
1-137: Verify mobile-specific requirements and responsiveness.Since this change is part of mobile template functionality:
- Ensure the component handles touch events appropriately
- Verify that the layout works well on different screen sizes
- Consider adding mobile-specific styles or breakpoints if needed
packages/desktop-client/src/components/budget/SidebarGroup.tsx (3)
6-6: LGTM: Clean implementation of new feature flag hook and prop type.The new import and prop type are well-structured and properly typed.
Also applies to: 36-36
52-52: LGTM: Props destructuring is clean and well-organized.The new callback prop is properly integrated into the component's props destructuring.
58-58: Verify feature flag configuration.The
goalTemplatesEnabledfeature flag controls the visibility of budget template functionality. Ensure this flag is properly configured in the feature flags system.✅ Verification successful
The feature flag
goalTemplatesEnabledappears to be properly configured in the codebase. Let me break down the findings:
- The flag is defined in the preferences type system (
packages/loot-core/src/types/prefs.d.ts)- It has a default value of
falseinpackages/desktop-client/src/hooks/useFeatureFlag.ts- It's accessible through the Experimental settings UI component (
packages/desktop-client/src/components/settings/Experimental.tsx)- It's consistently used across multiple components to conditionally render goal template functionality
Feature flag configuration is verified and properly implemented.
- Default state is disabled (false) by default
- Configurable through the Experimental settings UI
- Used consistently across the codebase for goal template related features
- Properly typed in the preferences system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for feature flag configuration and usage # Test: Search for goalTemplatesEnabled flag configuration rg -A 5 "goalTemplatesEnabled"Length of output: 7451
packages/desktop-client/src/components/budget/BudgetTable.jsx (1)
239-239: LGTM! Verify BudgetCategories implementation.The prop is correctly forwarded to the BudgetCategories component.
Let's verify that the BudgetCategories component properly implements this prop:
✅ Verification successful
Prop implementation verified successfully
The
onApplyBudgetTemplatesInGroupprop is properly implemented throughout the component hierarchy:
- Correctly declared and passed through BudgetCategories component
- Properly forwarded to ExpenseGroup component
- Finally used in SidebarGroup where it's called with filtered categories
- Implementation in index.tsx shows it dispatches an 'apply-multiple-templates' budget action
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify BudgetCategories component implementation of onApplyBudgetTemplatesInGroup # Test: Check if BudgetCategories component properly declares and uses the prop ast-grep --pattern 'function BudgetCategories({ $$$, onApplyBudgetTemplatesInGroup, $$$ }) { $$$ }' # Test: Look for actual usage of the callback within BudgetCategories rg "onApplyBudgetTemplatesInGroup\(" packages/desktop-client/src/components/budget/BudgetCategories.jsxLength of output: 209
Script:
#!/bin/bash # Let's try a broader search to understand the BudgetCategories component structure # First, let's check if the file exists fd "BudgetCategories.jsx" packages/desktop-client/src/components/budget/ # If it exists, let's look at its content fd "BudgetCategories.jsx" packages/desktop-client/src/components/budget/ -x cat {} # Let's also try a more flexible pattern match for the prop usage rg "onApplyBudgetTemplatesInGroup" packages/desktop-client/src/components/budget/ -A 2Length of output: 15126
packages/loot-core/src/client/state-types/modals.d.ts (1)
177-181: Verify the completeness of the modal interface.The modal interface seems minimal compared to other budget-related modals (e.g.,
envelope-budget-menu). Consider whether additional operations might be needed at the group level.Let's analyze other group-level operations in the codebase:
✅ Verification successful
Let me try a different search strategy to analyze the group-level operations and modal interfaces.
Let me try one more search to specifically look at the modal interfaces and group-level operations.
Based on the analysis of the codebase, I can now provide a final response:
The modal interface is appropriately complete for its purpose.
The
envelope-budget-group-menumodal interface contains all necessary operations at the group level, as evidenced by:
It includes the essential group-level budgeting operations:
- Viewing group's total budgeted amount
- Applying budget templates at the group level
- Month-specific operations (via the month context)
Additional group operations are handled through other interfaces:
- Group visibility toggling
- Group deletion
- Adding categories
- Group notes management
- Group renaming
The current interface aligns well with the envelope budgeting system's group-level operations and doesn't require additional fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for patterns of group-level budget operations echo "Searching for group-level budget operations..." rg -l "group.*budget|budget.*group" --type ts --type tsx # Search for similar modal definitions echo "Searching for similar modal patterns..." ast-grep --pattern 'type FinanceModals = { $$$ $name: { group: $_ $$$ } $$$ }'Length of output: 375
Script:
#!/bin/bash # Search for files containing budget group related code echo "Searching for budget group related files..." rg -l "CategoryGroupEntity|BudgetGroup" --type ts # Look for group-level operations in these files echo "Searching for group operations..." rg "group\." -A 5 --type ts # Search for modal interfaces echo "Looking for modal interfaces..." rg "type.*Modal.*=.*{" -A 10 --type tsLength of output: 49009
Script:
#!/bin/bash # Search for modal interfaces in the codebase echo "Looking for modal interfaces..." rg "envelope-budget.*menu|budget.*menu" -A 5 --type ts # Look for group-level operations in the modal components echo "Looking for group-level operations in modals..." rg -l "EnvelopeBudgetGroupMenuModal|CategoryGroupMenuModal" --type ts # Check the implementation of these modal components echo "Checking modal implementations..." fd -e tsx "EnvelopeBudgetGroupMenuModal|CategoryGroupMenuModal" -x cat {}Length of output: 24770
packages/loot-core/src/client/actions/queries.ts (1)
99-108:⚠️ Potential issueConsider improving error handling and consistency.
The new case for applying multiple templates has some potential improvements to consider:
- Error handling should be added to maintain robustness
- The notification dispatch pattern differs from similar template-related cases
- Type safety could be improved
Consider applying this improvement:
case 'apply-multiple-templates': - dispatch( - addNotification( - await send('budget/apply-multiple-templates', { - month, - categoryIds: args.categories, - }), - ), - ); + const result = await send('budget/apply-multiple-templates', { + month, + categoryIds: args.categories, + }); + if (result.error) { + dispatch(addGenericErrorNotification()); + throw new Error(result.error); + } break;Let's verify the error handling pattern in similar cases:
packages/desktop-client/src/components/budget/BudgetCategories.jsx (2)
249-249: Verify if budget templates should apply to income groups.I notice that the
onApplyBudgetTemplatesInGroupprop is only passed toExpenseGroupand not toIncomeGroup. Please confirm if this is intentional and aligns with the requirements.#!/bin/bash # Description: Check if there are any references to budget templates with income groups # Expected: No references to budget templates with income # Search for any mentions of templates with income rg -i "template.*income|income.*template" # Search for related issues or discussions gh issue list --search "income template"
249-249: Verify ExpenseGroup component's prop definition.The prop is correctly passed down, but let's ensure it's properly defined in the
ExpenseGroupcomponent's interface.✅ Verification successful
Let me generate additional scripts to properly verify the prop definition.
Prop is properly defined and used correctly
The
onApplyBudgetTemplatesInGroupprop is correctly defined in both theExpenseGroupandSidebarGroupcomponents:
- In
ExpenseGroup.tsx, it's defined as an optional prop using TypeScript's ComponentProps- In
SidebarGroup.tsx, it's properly typed as(categories: object[]) => void- The prop is correctly passed through the component hierarchy: BudgetCategories → ExpenseGroup → SidebarGroup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the prop is properly defined in ExpenseGroup component # Expected: Find the prop definition in ExpenseGroup's interface/props # Search for ExpenseGroup's prop definitions ast-grep --pattern 'interface ExpenseGroupProps { $$$ }' # Search for PropTypes definition rg "ExpenseGroup.propTypes" -A 10Length of output: 881
Script:
#!/bin/bash # Search for ExpenseGroup component definition and its props ast-grep --pattern 'const ExpenseGroup = ({ $$ }) => { $$$ }' # Search for ExpenseGroup component with PropTypes rg "ExpenseGroup.*propTypes|PropTypes.*ExpenseGroup" -A 10 # Search for the specific prop usage within ExpenseGroup rg "onApplyBudgetTemplatesInGroup" -B 2 -A 2Length of output: 6322
packages/desktop-client/src/components/budget/index.tsx (1)
378-378: Update TypeScript interface for DynamicBudgetTable.The new prop is correctly added, but we should ensure:
- The
DynamicBudgetTablecomponent's TypeScript interface includes this prop- Document why this prop is only needed for envelope budgets and not tracking budgets
Let's check if the interface exists:
packages/desktop-client/src/components/Modals.tsx (2)
33-33: LGTM!The import statement follows the established patterns and conventions.
407-418: Verify month prop usage in EnvelopeBudgetGroupMenuModal.The implementation follows the established patterns and correctly integrates with the modal system. However, while
monthis used for the namespace context, it's not passed to the modal component.Let's verify if the modal component needs the month prop:
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (2)
Line range hint
753-764: LGTM: Props added for template functionality.The new props
monthandonBudgetActionare properly integrated to support the group template feature.
890-913: LGTM: Well-implemented UI enhancement.The budgeted amount is now properly wrapped in an interactive button that:
- Follows existing UI patterns
- Maintains privacy filtering
- Handles text sizing appropriately
- Improves accessibility
packages/loot-core/src/server/budget/goaltemplates.ts (1)
68-68: UpdateapplySingleCategoryTemplateto passcategoriesThe addition of
categoriesas an argument toprocessTemplateensures consistent handling of category data and aligns with the updated function signature.
| const onMenuSelect = (name: string) => { | ||
| switch (name) { | ||
| case 'apply-group-category-template': | ||
| onApplyGroupTemplate?.(); | ||
| break; | ||
| default: | ||
| throw new Error(`Unrecognized menu item: ${name}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider more graceful error handling in production.
The error throwing for unrecognized menu items might be too aggressive for production. Consider logging the error and providing a fallback behavior.
const onMenuSelect = (name: string) => {
switch (name) {
case 'apply-group-category-template':
onApplyGroupTemplate?.();
break;
default:
- throw new Error(`Unrecognized menu item: ${name}`);
+ console.error(`Unrecognized menu item: ${name}`);
}
};📝 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 onMenuSelect = (name: string) => { | |
| switch (name) { | |
| case 'apply-group-category-template': | |
| onApplyGroupTemplate?.(); | |
| break; | |
| default: | |
| throw new Error(`Unrecognized menu item: ${name}`); | |
| } | |
| }; | |
| const onMenuSelect = (name: string) => { | |
| switch (name) { | |
| case 'apply-group-category-template': | |
| onApplyGroupTemplate?.(); | |
| break; | |
| default: | |
| console.error(`Unrecognized menu item: ${name}`); | |
| } | |
| }; |
| type BudgetMenuGroupProps = Omit< | ||
| ComponentPropsWithoutRef<typeof Menu>, | ||
| 'onMenuSelect' | 'items' | ||
| > & { | ||
| onApplyGroupTemplate: () => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the type definition for better type safety.
The onApplyGroupTemplate callback could be enhanced:
- Make it optional since you use optional chaining in the implementation
- Include error handling in the type signature
type BudgetMenuGroupProps = Omit<
ComponentPropsWithoutRef<typeof Menu>,
'onMenuSelect' | 'items'
> & {
- onApplyGroupTemplate: () => void;
+ onApplyGroupTemplate?: () => void | Promise<void>;
};📝 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.
| type BudgetMenuGroupProps = Omit< | |
| ComponentPropsWithoutRef<typeof Menu>, | |
| 'onMenuSelect' | 'items' | |
| > & { | |
| onApplyGroupTemplate: () => void; | |
| }; | |
| type BudgetMenuGroupProps = Omit< | |
| ComponentPropsWithoutRef<typeof Menu>, | |
| 'onMenuSelect' | 'items' | |
| > & { | |
| onApplyGroupTemplate?: () => void | Promise<void>; | |
| }; |
| items={[ | ||
| ...(isGoalTemplatesEnabled | ||
| ? [ | ||
| { | ||
| name: 'apply-group-category-template', | ||
| text: t('Apply budget template'), | ||
| }, | ||
| ] | ||
| : []), | ||
| ]} |
There was a problem hiding this comment.
💡 Codebase verification
Based on the Menu component implementation and usage context, I can now provide the final response:
Consider conditionally rendering the Menu component when empty
The Menu component doesn't have built-in handling for empty states, and rendering it with an empty array will result in a clickable menu button that opens an empty menu. To improve the user experience:
return isGoalTemplatesEnabled ? (
<Menu
{...props}
onMenuSelect={onMenuSelect}
items={[
{
name: 'apply-group-category-template',
text: t('Apply budget template'),
},
]}
/>
) : null;🔗 Analysis chain
Handle empty menu state when feature is disabled.
When isGoalTemplatesEnabled is false, the menu will be empty. Consider:
- Either hiding the menu completely when empty
- Or providing a disabled state explaining why features are unavailable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other places in the codebase handling empty menus
rg -l "items={(\[\])}" --type ts --type tsx
Length of output: 149
Script:
#!/bin/bash
# Search for empty menu items array patterns with proper escaping
rg "items=\{\[\]\}" --type ts --type tsx
# Search for Menu component usage to understand empty state handling
rg -A 3 "<Menu" --type ts --type tsx
# Look for the Menu component definition
ast-grep --pattern 'interface $_Props {
$$$
items: $_
$$$
}'
Length of output: 206
Script:
#!/bin/bash
# Search for Menu component usage with correct file extension
rg -A 3 "<Menu" .
# Look for the Menu component definition
rg -A 10 "interface.*Menu.*Props" .
# Search for empty menu items array patterns
rg "items=\{(\[\]|[])" .
Length of output: 17212
Script:
#!/bin/bash
# Look for Menu component definition and its props
rg -A 5 "export.*Menu.*=.*" ./packages/desktop-client/src/components/common/Menu.tsx
# Check if there's any handling of empty items
rg -A 5 "items\.length.*===.*0" .
# Look for conditional rendering of Menu component
rg "{\s*items.*length.*[><=].*0.*}" .
Length of output: 963
Script:
#!/bin/bash
# Look at the Menu component implementation
cat ./packages/desktop-client/src/components/common/Menu.tsx
# Look for conditional rendering patterns around Menu
rg -B 2 -A 2 "<Menu.*" ./packages/desktop-client/src/components/budget/envelope/BudgetMenuGroup.tsx
Length of output: 6928
| 'budget/apply-multiple-templates': (arg: { | ||
| month: string; | ||
| categoryIds: string[]; | ||
| }) => Promise<Notification>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type validation for the month parameter.
The month parameter could benefit from a more specific type to ensure it follows a consistent format (e.g., 'YYYY-MM').
+ // Define a specific month format type
+ type YearMonth = `${number}-${number}`;
+
'budget/apply-multiple-templates': (arg: {
- month: string;
+ month: YearMonth;
categoryIds: string[];
}) => Promise<Notification>;Committable suggestion was skipped due to low confidence.
| const budgeted = useEnvelopeSheetValue( | ||
| envelopeBudget.groupBudgeted(group.id), | ||
| ); | ||
| const value = integerToAmount(budgeted || 0); | ||
| const [amountFocused, setAmountFocused] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| setAmountFocused(true); | ||
| }, []); |
There was a problem hiding this comment.
Remove unused state and effect.
The amountFocused state and its corresponding effect are not used in the component. Consider removing them to improve code clarity.
- const [amountFocused, setAmountFocused] = useState(false);
-
- useEffect(() => {
- setAmountFocused(true);
- }, []);📝 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 budgeted = useEnvelopeSheetValue( | |
| envelopeBudget.groupBudgeted(group.id), | |
| ); | |
| const value = integerToAmount(budgeted || 0); | |
| const [amountFocused, setAmountFocused] = useState(false); | |
| useEffect(() => { | |
| setAmountFocused(true); | |
| }, []); | |
| const budgeted = useEnvelopeSheetValue( | |
| envelopeBudget.groupBudgeted(group.id), | |
| ); | |
| const value = integerToAmount(budgeted || 0); |
🧰 Tools
🪛 GitHub Check: lint
[warning] 39-39:
'amountFocused' is assigned a value but never used. Allowed unused vars must match /^(_|React)/u
| <Text | ||
| style={{ | ||
| ...makeAmountFullStyle(value), | ||
| userSelect: 'none', | ||
| fontSize: 30, | ||
| fontWeight: 600, | ||
| textAlign: 'center', | ||
| }} | ||
| > | ||
| {amountToCurrency(Math.abs(value))} | ||
| </Text> | ||
| </View> | ||
| </View> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance amount display accessibility and loading state.
The amount display should be accessible and handle loading states gracefully.
<Text
style={{
...makeAmountFullStyle(value),
userSelect: 'none',
fontSize: 30,
fontWeight: 600,
textAlign: 'center',
}}
+ accessibilityRole="text"
+ accessibilityLabel={`Budgeted amount ${amountToCurrency(Math.abs(value))}`}
>
- {amountToCurrency(Math.abs(value))}
+ {budgeted === undefined ? (
+ <ActivityIndicator size="small" />
+ ) : (
+ amountToCurrency(Math.abs(value))
+ )}
</Text>Committable suggestion was skipped due to low confidence.
| @@ -236,6 +236,7 @@ function BudgetCell({ | |||
| const categoryNotes = useNotes(category.id); | |||
|
|
|||
| const onOpenCategoryBudgetMenu = () => { | |||
| console.log('open'); | |||
There was a problem hiding this comment.
Remove debug logging statement.
Debug logging statements should not be committed to production code.
- console.log('open');📝 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.
| console.log('open'); |
| const dispatch = useDispatch(); | ||
| const onOpenGroupBudgetMenu = () => { | ||
| console.log('OPEN Group header menu' + month); | ||
| dispatch( | ||
| pushModal('envelope-budget-group-menu', { | ||
| month, | ||
| group, | ||
| onApplyGroupTemplate: () => { | ||
| onBudgetAction(month, 'apply-multiple-templates', { | ||
| month, | ||
| categories: group.categories | ||
| .filter(c => !c['hidden']) | ||
| .map(c => c['id']), | ||
| }); | ||
| }, | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Clean up the implementation.
The function has several issues that should be addressed:
- Remove the debug logging statement
- Remove redundant month parameter in the action payload
- Simplify the hidden categories filter
- console.log('OPEN Group header menu' + month);
dispatch(
pushModal('envelope-budget-group-menu', {
month,
group,
onApplyGroupTemplate: () => {
onBudgetAction(month, 'apply-multiple-templates', {
- month,
categories: group.categories
- .filter(c => !c['hidden'])
- .map(c => c['id']),
+ .filter(c => !c.hidden)
+ .map(c => c.id),
});
},
}),
);📝 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 dispatch = useDispatch(); | |
| const onOpenGroupBudgetMenu = () => { | |
| console.log('OPEN Group header menu' + month); | |
| dispatch( | |
| pushModal('envelope-budget-group-menu', { | |
| month, | |
| group, | |
| onApplyGroupTemplate: () => { | |
| onBudgetAction(month, 'apply-multiple-templates', { | |
| month, | |
| categories: group.categories | |
| .filter(c => !c['hidden']) | |
| .map(c => c['id']), | |
| }); | |
| }, | |
| }), | |
| ); | |
| }; | |
| const dispatch = useDispatch(); | |
| const onOpenGroupBudgetMenu = () => { | |
| dispatch( | |
| pushModal('envelope-budget-group-menu', { | |
| month, | |
| group, | |
| onApplyGroupTemplate: () => { | |
| onBudgetAction(month, 'apply-multiple-templates', { | |
| categories: group.categories | |
| .filter(c => !c.hidden) | |
| .map(c => c.id), | |
| }); | |
| }, | |
| }), | |
| ); | |
| }; |
| if (Array.isArray(category)) { | ||
| 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; | ||
| } else if (category) { |
There was a problem hiding this comment.
Use an object instead of an array for multipleCategoryTemplates
multipleCategoryTemplates is initialized as an empty array but is used as an object with category IDs as keys. This can lead to confusion since arrays are intended for sequential numeric indexes. It's better to use an object {} when keys are non-numeric.
Apply this diff to fix the issue:
-const multipleCategoryTemplates = [];
+const multipleCategoryTemplates = {};Update the rest of the code accordingly:
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);
+ multipleCategoryTemplates[categoryId] = templates[categoryId].filter(
+ t => t.directive === directive
+ );
}
}📝 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.
| if (Array.isArray(category)) { | |
| 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; | |
| } else if (category) { | |
| if (Array.isArray(category)) { | |
| const multipleCategoryTemplates = {}; | |
| for (let dd = 0; dd < category.length; dd++) { | |
| const categoryId = category[dd].id; | |
| if (templates[categoryId] !== undefined) { | |
| multipleCategoryTemplates[categoryId] = templates[categoryId].filter( | |
| t => t.directive === directive | |
| ); | |
| } | |
| } | |
| return multipleCategoryTemplates; | |
| } else if (category) { |
| export async function applyMultipleCategoryTemplates({ month, categoryIds }) { | ||
| const placeholders = categoryIds.map(() => '?').join(', '); | ||
| const query = `SELECT * FROM v_categories WHERE id IN (${placeholders})`; | ||
| const categories = await db.all(query, categoryIds); | ||
| await storeTemplates(); | ||
| const category_templates = await getTemplates(categories, 'template'); | ||
| const category_goals = await getTemplates(categories, 'goal'); | ||
| const ret = await processTemplate( | ||
| month, | ||
| true, | ||
| category_templates, | ||
| categories, | ||
| ); | ||
| await processGoals(category_goals, month); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Handle empty categoryIds to prevent SQL syntax errors
If categoryIds is an empty array, the constructed SQL query will be invalid (WHERE id IN ()), leading to a syntax error when executing the query. Please add a check to handle this case appropriately.
Apply this diff to fix the issue:
export async function applyMultipleCategoryTemplates({ month, categoryIds }) {
+ if (categoryIds.length === 0) {
+ return;
+ }
const placeholders = categoryIds.map(() => '?').join(', ');
const query = `SELECT * FROM v_categories WHERE id IN (${placeholders})`;
const categories = await db.all(query, categoryIds);📝 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.
| export async function applyMultipleCategoryTemplates({ month, categoryIds }) { | |
| const placeholders = categoryIds.map(() => '?').join(', '); | |
| const query = `SELECT * FROM v_categories WHERE id IN (${placeholders})`; | |
| const categories = await db.all(query, categoryIds); | |
| await storeTemplates(); | |
| const category_templates = await getTemplates(categories, 'template'); | |
| const category_goals = await getTemplates(categories, 'goal'); | |
| const ret = await processTemplate( | |
| month, | |
| true, | |
| category_templates, | |
| categories, | |
| ); | |
| await processGoals(category_goals, month); | |
| return ret; | |
| } | |
| export async function applyMultipleCategoryTemplates({ month, categoryIds }) { | |
| if (categoryIds.length === 0) { | |
| return; | |
| } | |
| const placeholders = categoryIds.map(() => '?').join(', '); | |
| const query = `SELECT * FROM v_categories WHERE id IN (${placeholders})`; | |
| const categories = await db.all(query, categoryIds); | |
| await storeTemplates(); | |
| const category_templates = await getTemplates(categories, 'template'); | |
| const category_goals = await getTemplates(categories, 'goal'); | |
| const ret = await processTemplate( | |
| month, | |
| true, | |
| category_templates, | |
| categories, | |
| ); | |
| await processGoals(category_goals, month); | |
| return ret; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (1)
20-23: Consider enhancing type safety of the callback prop.The
onApplyGroupTemplatecallback type could be more specific about what data it receives when invoked.Consider updating the type definition:
type EnvelopeBudgetGroupMenuModalProps = { group: CategoryGroupEntity; - onApplyGroupTemplate: () => void; + onApplyGroupTemplate: (groupId: string, templateId?: string) => void; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3737.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (1 hunks)
- packages/loot-core/src/client/actions/queries.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/client/actions/queries.ts
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/EnvelopeBudgetGroupMenuModal.tsx (3)
1-19: LGTM! Well-organized imports.The imports are properly organized, with clear separation between external dependencies, internal modules, and type imports.
29-34: Enhance mobile UI patterns.The current menu item styles could be improved for better mobile user experience.
74-86: Enhance amount display accessibility and loading state.The amount display should handle loading states and be accessible.
| <BudgetMenuGroup | ||
| getItemStyle={() => defaultMenuItemStyle} | ||
| onApplyGroupTemplate={onApplyGroupTemplate} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundary.
The BudgetMenuGroup component might throw errors that should be caught to prevent the entire modal from crashing.
Consider wrapping the component with an error boundary:
+import { ErrorBoundary } from '../common/ErrorBoundary';
- <BudgetMenuGroup
- getItemStyle={() => defaultMenuItemStyle}
- onApplyGroupTemplate={onApplyGroupTemplate}
- />
+ <ErrorBoundary fallback={<Text>Failed to load menu options</Text>}>
+ <BudgetMenuGroup
+ getItemStyle={() => defaultMenuItemStyle}
+ onApplyGroupTemplate={onApplyGroupTemplate}
+ />
+ </ErrorBoundary>Committable suggestion was skipped due to low confidence.
| const budgeted = useEnvelopeSheetValue( | ||
| envelopeBudget.groupBudgeted(group.id), | ||
| ); | ||
| const value = integerToAmount(budgeted || 0); | ||
|
|
||
| if (!group) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Move null check before hook usage.
The null check for group should be before using it in the useEnvelopeSheetValue hook to prevent potential runtime errors.
Apply this change:
export function EnvelopeBudgetGroupMenuModal({
group,
onApplyGroupTemplate,
}: EnvelopeBudgetGroupMenuModalProps) {
+ if (!group) {
+ return null;
+ }
+
const defaultMenuItemStyle: CSSProperties = {
...styles.mobileMenuItem,
color: theme.menuItemText,
borderRadius: 0,
borderTop: `1px solid ${theme.pillBorder}`,
};
const budgeted = useEnvelopeSheetValue(
envelopeBudget.groupBudgeted(group.id),
);
const value = integerToAmount(budgeted || 0);
- if (!group) {
- return null;
- }📝 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 budgeted = useEnvelopeSheetValue( | |
| envelopeBudget.groupBudgeted(group.id), | |
| ); | |
| const value = integerToAmount(budgeted || 0); | |
| if (!group) { | |
| return null; | |
| } | |
| if (!group) { | |
| return null; | |
| } | |
| const defaultMenuItemStyle: CSSProperties = { | |
| ...styles.mobileMenuItem, | |
| color: theme.menuItemText, | |
| borderRadius: 0, | |
| borderTop: `1px solid ${theme.pillBorder}`, | |
| }; | |
| const budgeted = useEnvelopeSheetValue( | |
| envelopeBudget.groupBudgeted(group.id), | |
| ); | |
| const value = integerToAmount(budgeted || 0); |
| const budgeted = useEnvelopeSheetValue( | ||
| envelopeBudget.groupBudgeted(group.id), | ||
| ); | ||
| const value = integerToAmount(budgeted || 0); | ||
|
|
There was a problem hiding this comment.
Add error handling for data fetching.
The component should handle potential errors from useEnvelopeSheetValue and provide appropriate feedback to users.
Consider implementing error handling:
const budgeted = useEnvelopeSheetValue(
envelopeBudget.groupBudgeted(group.id),
);
+const [error, setError] = useState<Error | null>(null);
+
+if (error) {
+ return (
+ <View style={{ padding: 16, alignItems: 'center' }}>
+ <Text style={{ color: theme.errorText }}>
+ Failed to load budget data. Please try again.
+ </Text>
+ </View>
+ );
+}Committable suggestion was skipped due to low confidence.
|
I don't love that its in a new modal vs one of the existing ones |
|
Agreed. But I couldn't think of a way to put the new button in one of the existing menus. It's to big of a word for the remove Menu and when I add a third button to the one below the notes it looks awful. I could adjust the width of the buttons so three can fit in one row. |
|
How should i proceed ? |
|
First off this will need to be updated and conflicts fixed. I would look at adding the action to the existing group modal since you aren't budgeting a group, you are budgeting categories, it doesn't make sense to click on the group budget total. The other modals have a few different options for actions to look at. |
|
I agree that this should not be in a new modal. The button is also not obvious that it can be clicked. We can instead add this on the existing category group modal but have a collapsible |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Adds Feature from #3040 and expands #3666