Sub category groups #3772
Sub category groups #3772
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
UnderKoen
force-pushed
the
feat/sub-categories
branch
from
ec2cef0 to
f45e246
Compare
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 |
UnderKoen
force-pushed
the
feat/sub-categories
branch
from
f45e246 to
2a9e296
Compare
WalkthroughThe pull request introduces multiple enhancements across various components related to budget management in the application. Key changes include the introduction of a 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: 7
🧹 Outside diff range and nitpick comments (31)
packages/loot-core/migrations/1726730827000_sub_category_groups.sql (1)
3-3: Consider adding foreign key constraint and index.While the column addition looks good, consider these improvements for better data integrity and query performance:
- Add a foreign key constraint to ensure referential integrity
- Add an index to optimize hierarchical queries
-ALTER TABLE category_groups ADD COLUMN parent_id TEXT DEFAULT null; +ALTER TABLE category_groups ADD COLUMN parent_id TEXT DEFAULT null REFERENCES category_groups(id); +CREATE INDEX idx_category_groups_parent_id ON category_groups(parent_id);Also, consider adding a comment to document the column's purpose:
COMMENT ON COLUMN category_groups.parent_id IS 'References the parent group in the category group hierarchy';packages/loot-core/src/types/models/category-group.d.ts (2)
11-11: Consider adding JSDoc comment for parent_id property.The parent_id property is correctly typed, but adding documentation would help clarify its purpose and relationship model.
+ /** ID of the parent category group. If not set, this is a root-level group. */ parent_id?: string;
Line range hint
1-18: Consider documenting migration strategy and feature flag implications.While the type changes are backward compatible, it would be valuable to:
- Document how existing flat data will be migrated to the new hierarchical structure
- Consider adding a type-level feature flag (conditional type) to align with the runtime feature flag mentioned in the PR objectives
Example approach for feature flag at type level:
type FeatureFlags = { subCategoryGroups: boolean; }; type CategoryGroupEntityWithFeatures<F extends FeatureFlags> = NewCategoryGroupEntity & { id: string; categories?: CategoryEntity[]; } & (F['subCategoryGroups'] extends true ? { parent_id?: string; children?: WithRequired<CategoryGroupEntity, 'parent_id'>[]; } : {});packages/desktop-client/src/hooks/useFeatureFlag.ts (1)
11-11: Consider adding documentation for the feature flag.To improve maintainability, consider adding a JSDoc comment above the
DEFAULT_FEATURE_FLAG_STATEobject describing the purpose and scope of thesubCategoryGroupsflag.const DEFAULT_FEATURE_FLAG_STATE: Record<FeatureFlag, boolean> = { reportBudget: false, goalTemplatesEnabled: false, dashboards: false, actionTemplating: false, upcomingLengthAdjustment: false, + /** Controls the hierarchical category groups feature. + * When enabled, allows creation and management of sub-category groups + * Currently only affects the budget page. + */ subCategoryGroups: false, };packages/desktop-client/src/hooks/useCategories.ts (1)
19-24: Consider improving type safety and naming.A few suggestions to enhance the code:
- The selector type could be more explicit
- The name
groupedHierarchymight be misleading as it only contains root-level groupsConsider applying these improvements:
- const selector = useSelector(state => state.queries.categories); + type CategoriesState = State['queries']['categories']; + const selector = useSelector((state: State) => state.queries.categories); return useMemo( () => ({ ...selector, - groupedHierarchy: selector.grouped.filter(g => g.parent_id === null), + rootGroups: selector.grouped.filter(g => g.parent_id === null), }), [selector], );packages/loot-core/src/server/budget/base.test.ts (1)
Line range hint
1-71: Consider adding test cases for hierarchical groups.Given that this PR introduces hierarchy to category groups, it would be beneficial to add test cases that verify:
- Creation of parent-child relationships between groups
- Budget calculations with nested groups
- Edge cases in the group hierarchy
Would you like me to help generate additional test cases for these scenarios?
packages/desktop-client/src/components/budget/IncomeCategory.tsx (1)
31-31: Consider adding JSDoc documentation for the depth prop.The
depthproperty is used for hierarchical rendering, but its purpose and valid values aren't documented. Adding JSDoc comments would improve maintainability.type IncomeCategoryProps = { + /** Indicates the nesting level of the category in the hierarchy. Undefined means root level. */ depth?: number;Also applies to: 47-47, 84-84
packages/loot-core/src/server/models.ts (2)
92-98: Consider standardizing boolean field handlingWhile the current implementation works correctly, we could make the boolean field handling more consistent and maintainable.
Consider refactoring to handle both boolean fields similarly:
return { ...rest, - hidden: rest.hidden ? 1 : 0, - ...(rest.is_income !== undefined && { - is_income: rest.is_income ? 1 : 0, - }), + ...(rest.hidden !== undefined && { hidden: rest.hidden ? 1 : 0 }), + ...(rest.is_income !== undefined && { is_income: rest.is_income ? 1 : 0 }), };This approach:
- Makes the handling of boolean fields consistent
- Maintains the same functionality
- Improves code maintainability
Model validation needs update for parent_id field
The migration file
1726730827000_sub_category_groups.sqladds aparent_idcolumn to thecategory_groupstable, and the type definition incategory-group.d.tsincludesparent_idin thechildrenfield type. However, thecategoryGroupModel.validate()function inmodels.tsdoesn't handle theparent_idfield in its validation or transformation logic.
packages/loot-core/src/server/models.ts: UpdatecategoryGroupModel.validate()to handle theparent_idfield, similar to how it handles other nullable fields.🔗 Analysis chain
Line range hint
81-98: Verify model alignment with database schemaThe model should handle the
parent_idfield mentioned in the AI summary for hierarchical relationships.Let's verify the database schema changes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for parent_id column in schema and migrations # Expected: Find references to parent_id in schema/migration files # Look for SQL migration files that add parent_id fd -e sql | xargs rg -l "parent_id" # Look for parent_id references in TypeScript schema definitions fd -e ts | xargs rg "parent_id.*category_groups"Length of output: 336
Script:
#!/bin/bash # Let's examine the migration files and schema definitions more thoroughly # Check the content of the migration file that adds sub_category_groups cat packages/loot-core/migrations/1726730827000_sub_category_groups.sql # Look for category group schema/type definitions rg -A 5 "interface.*CategoryGroup" # Check for parent_id in models.ts cat packages/loot-core/src/server/models.tsLength of output: 4125
packages/desktop-client/src/components/budget/ExpenseCategory.tsx (2)
38-38: Consider adding depth validation.While the type definition is correct, consider adding runtime validation to ensure depth is non-negative, as negative depths wouldn't make sense in a hierarchy.
+const validateDepth = (depth?: number) => { + if (depth !== undefined && depth < 0) { + console.warn('Depth should not be negative'); + } +};
Line range hint
38-102: Well-structured approach to implementing hierarchy.The implementation of depth through prop passing is a clean and maintainable approach to handling hierarchical structures. It maintains good separation of concerns by delegating the actual rendering logic to the SidebarCategory component while keeping the ExpenseCategory component focused on its core responsibilities.
packages/loot-core/src/client/reducers/queries.ts (1)
114-124: Consider a more functional approach to avoid direct mutations.The current implementation mutates the outer
resobject directly. Consider using a more functional approach that builds and returns a new object:-function addGroups(groups) { - groups.forEach(group => { - group.categories.forEach(cat => { - res[cat.id] = cat; - }); - if (group.children) { - addGroups(group.children); - } - }); -} -addGroups(categoryGroups); +function addGroups(groups: CategoryGroup[]): Record<string, Category> { + return groups.reduce((acc, group) => { + const categoryMap = group.categories.reduce( + (catAcc, cat) => ({ ...catAcc, [cat.id]: cat }), + {} + ); + const childrenMap = group.children ? addGroups(group.children) : {}; + return { ...acc, ...categoryMap, ...childrenMap }; + }, {}); +} +return addGroups(categoryGroups);packages/desktop-client/src/components/budget/ExpenseGroup.tsx (2)
36-37: Add JSDoc comments for new propsThe new props are well-typed, but would benefit from documentation explaining their purpose.
+/** Callback fired when creating a new subgroup */ onShowNewGroup?: ComponentProps<typeof SidebarGroup>['onShowNewGroup']; +/** Hierarchical depth of the group (0 for top-level groups) */ depth?: number;
89-90: Consider making depth check more explicitWhile the logic is correct, the depth check could be more explicit to better convey intent.
- depth > 0 ? theme.tableRowHeaderBackground : colorPalette.navy900, + depth && depth > 0 ? theme.tableRowHeaderBackground : colorPalette.navy900,packages/desktop-client/src/components/settings/Experimental.tsx (1)
118-120: LGTM! Feature toggle implementation follows best practices.The implementation:
- Correctly uses the
FeatureTogglecomponent- Properly handles internationalization with
Trans- Is appropriately placed in the experimental features section
- Is gated behind user acknowledgment of risks
However, consider adding a feedback link like other experimental features to gather user input during the beta phase.
<FeatureToggle flag="subCategoryGroups"> <Trans>Sub category groups</Trans> </FeatureToggle> +<FeatureToggle + flag="subCategoryGroups" + feedbackLink="https://github.com/actualbudget/actual/pull/3772" +> + <Trans>Sub category groups</Trans> +</FeatureToggle>packages/desktop-client/src/components/budget/SidebarCategory.tsx (1)
185-188: Consider improving the padding calculation readability and maintainability.The current padding calculation uses magic numbers and a complex formula that might be difficult to maintain. Consider:
- Extracting the constants to named variables
- Simplifying the formula
- Documenting the reasoning behind the -5px adjustment
+ // Constants for hierarchical indentation + const BASE_PADDING = 13; + const DEPTH_INDENT = 13; + const DEPTH_ADJUSTMENT = 5; + style={{ - paddingLeft: 13 + (depth ?? 0) * 13 - (depth ? 5 : 0), + paddingLeft: BASE_PADDING + (depth ?? 0) * DEPTH_INDENT - (depth ? DEPTH_ADJUSTMENT : 0), ...(isLast && { borderBottomWidth: 0 }), }}packages/desktop-client/src/components/budget/SidebarGroup.tsx (2)
73-73: Consider extracting padding calculation constantsThe padding calculation uses magic numbers that could benefit from named constants for better maintainability and documentation.
Consider refactoring like this:
- paddingLeft: (depth ?? 0) * 13 - (depth ? 5 : 0), + const INDENT_SIZE = 13; + const DEPTH_OFFSET = 5; + paddingLeft: (depth ?? 0) * INDENT_SIZE - (depth ? DEPTH_OFFSET : 0),
206-206: Consider removing redundant id spreadThe
idproperty is being explicitly spread while already present in the group object spread.Consider simplifying:
- onSave({ ...group, id: group.id, name: value }); + onSave({ ...group, name: value });Also applies to: 209-209
packages/loot-core/src/types/server-handlers.d.ts (2)
94-94: Consider explicitly typing parentId for better type safety.The optional parentId parameter correctly enables hierarchical relationships while maintaining backward compatibility.
Consider explicitly typing parentId for better type safety:
- parentId?: string; + parentId?: CategoryGroupEntity['id'];
97-99: LGTM! Consider adjusting formatting.The use of
Partial<CategoryGroupEntity>improves type safety for update operations, which is particularly important when dealing with hierarchical structures.Consider adjusting the formatting to be more consistent with the rest of the file:
- 'category-group-update': ( - group: Partial<CategoryGroupEntity>, - ) => Promise<unknown>; + 'category-group-update': (group: Partial<CategoryGroupEntity>) => Promise<unknown>;packages/loot-core/src/client/actions/queries.ts (2)
217-217: Add JSDoc documentation for the new parameter.Consider adding JSDoc documentation to describe the purpose of the
parentIdparameter and its relationship to the hierarchical group structure.+/** + * Creates a new category group + * @param name The name of the group + * @param parentId Optional ID of the parent group for hierarchical structure + * @returns Promise<string> The ID of the newly created group + */ export function createGroup(name: string, parentId?: string) {
217-219: Consider updating related group management functions.While the
createGroupchanges look good, consider reviewing the following functions to ensure they properly handle the new hierarchical structure:
deleteGroup: Consider how deletion should handle nested groups (cascade vs. prevent)moveCategoryGroup: Ensure it respects parent-child relationships when moving groupspackages/desktop-client/src/components/budget/BudgetCategories.jsx (3)
47-85: Consider optimizing filter operationsThe
expandGroupfunction's logic for handling hierarchical groups is well-implemented. However, the separate filter operations forgroupCategoriesandgroupChildrencould be optimized.Consider combining the filter operations:
- const groupCategories = group.categories.filter( - cat => showHiddenCategories || !cat.hidden, - ); - - const groupChildren = group.children.filter( - child => showHiddenCategories || !child.hidden, - ); + const isVisible = item => showHiddenCategories || !item.hidden; + const groupCategories = group.categories.filter(isVisible); + const groupChildren = group.children.filter(isVisible);This refactoring:
- Reduces code duplication
- Makes the visibility logic more maintainable
- Improves readability
206-206: Consider adding depth validationThe depth property has been consistently added to all relevant components. However, consider adding validation to:
- Enforce a maximum nesting depth
- Prevent potential UI issues with deeply nested items
Consider defining a constant for maximum depth and adding validation:
const MAX_GROUP_DEPTH = 3; // or whatever is appropriate for your UI // Add to expandGroup function if (depth >= MAX_GROUP_DEPTH) { console.warn(`Maximum group depth of ${MAX_GROUP_DEPTH} exceeded`); return []; }Also applies to: 252-253, 273-273, 303-304, 323-323
206-206: Add type validation for parent_idThe
parent_idis passed to new groups without type validation. Consider adding PropTypes or TypeScript types to ensure type safety.Example with PropTypes:
SidebarGroup.propTypes = { group: PropTypes.shape({ id: PropTypes.string.isRequired, name: PropTypes.string.isRequired, parent_id: PropTypes.string }).isRequired, // ... other props };packages/loot-core/src/server/main.test.ts (1)
310-310: LGTM! Consider additional test scenarios.The boolean type for
is_incomemaintains consistency. However, with the introduction of hierarchical category groups, consider adding test scenarios for:
- Deleting a parent category group and verifying the impact on child groups
- Transferring categories between parent and child groups
- Validating that income/expense type constraints are enforced across the hierarchy
Here's a suggested test structure:
test('hierarchical category groups maintain proper relationships', async () => { await sheet.loadSpreadsheet(db); // Create parent and child groups await runMutator(async () => { await db.insertCategoryGroup({ id: 'parent', name: 'Parent Group', is_income: false }); await db.insertCategoryGroup({ id: 'child', name: 'Child Group', is_income: false, parent_id: 'parent' }); // Add test categories and verify relationships }); // Test deletion cascades // Test category transfers // Test income/expense constraints });packages/loot-core/src/server/main.ts (1)
370-376: LGTM! Consider adding type annotations for better type safety.The changes effectively support hierarchical category groups through the
parentIdparameter and provide good default values using nullish coalescing.Consider adding TypeScript type annotations to explicitly define the parameter types:
- parentId, + parentId?: string, }) { return withUndo(async () => { return db.insertCategoryGroup({ name, is_income: isIncome ?? false, parent_id: parentId ?? null,packages/loot-core/src/server/db/index.ts (4)
334-334: Simplify filter condition for clarityThe filter condition
!hierarchical || !g.parent_idmay be confusing.Refactor the return statement for better readability:
-return mappedGroups.filter(g => !hierarchical || !g.parent_id); +if (hierarchical) { + return mappedGroups.filter(g => !g.parent_id); +} else { + return mappedGroups; +}
337-341: Consider combining or documenting hierarchical functionsThe
getCategoriesGroupedHierarchicalfunction is a simple wrapper that setshierarchicaltotrue.If both hierarchical and non-hierarchical versions are required, consider documenting their usage. Otherwise, you might combine them to simplify the API.
Line range hint
343-369: Validate existence of parent group before insertionWhen inserting a category group with a
parent_id, there's no check to confirm that the parent group exists.Add validation to ensure the
parent_idcorresponds to an existing group:+if (group.parent_id) { + const parentGroup = await first( + 'SELECT id FROM category_groups WHERE id = ? AND tombstone = 0', + [group.parent_id], + ); + if (!parentGroup) { + throw new Error('Parent category group does not exist.'); + } +}
372-374: Ensure partial updates are correctly validatedThe
updateCategoryGroupfunction usescategoryGroupModel.validatewith{ update: true }, but it's important to confirm that partial updates are handled correctly.Review the
validatemethod to ensure it accommodates partial updates without overriding unset fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3772.mdis excluded by!**/*.md
📒 Files selected for processing (28)
packages/desktop-client/src/components/budget/BudgetCategories.jsx(7 hunks)packages/desktop-client/src/components/budget/BudgetTable.jsx(1 hunks)packages/desktop-client/src/components/budget/ExpenseCategory.tsx(3 hunks)packages/desktop-client/src/components/budget/ExpenseGroup.tsx(5 hunks)packages/desktop-client/src/components/budget/IncomeCategory.tsx(3 hunks)packages/desktop-client/src/components/budget/IncomeGroup.tsx(3 hunks)packages/desktop-client/src/components/budget/IncomeHeader.tsx(2 hunks)packages/desktop-client/src/components/budget/SidebarCategory.tsx(3 hunks)packages/desktop-client/src/components/budget/SidebarGroup.tsx(7 hunks)packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx(2 hunks)packages/desktop-client/src/components/budget/index.tsx(2 hunks)packages/desktop-client/src/components/settings/Experimental.tsx(1 hunks)packages/desktop-client/src/hooks/useCategories.ts(2 hunks)packages/desktop-client/src/hooks/useFeatureFlag.ts(1 hunks)packages/loot-core/migrations/1726730827000_sub_category_groups.sql(1 hunks)packages/loot-core/src/client/actions/queries.ts(1 hunks)packages/loot-core/src/client/reducers/queries.ts(1 hunks)packages/loot-core/src/server/accounts/sync.test.ts(1 hunks)packages/loot-core/src/server/accounts/transfer.test.ts(1 hunks)packages/loot-core/src/server/budget/base.test.ts(1 hunks)packages/loot-core/src/server/budget/base.ts(3 hunks)packages/loot-core/src/server/db/index.ts(4 hunks)packages/loot-core/src/server/main.test.ts(2 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/server/models.ts(2 hunks)packages/loot-core/src/types/models/category-group.d.ts(2 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/budget/SidebarGroup.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/SidebarGroup.tsx:69-76
Timestamp: 2024-10-13T11:17:59.711Z
Learning: In the `SidebarGroup` component (`packages/desktop-client/src/components/budget/SidebarGroup.tsx`), context menu state management is handled in another file, ensuring that only one context menu is open at a time.
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.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.
🔇 Additional comments (51)
packages/loot-core/migrations/1726730827000_sub_category_groups.sql (1)
1-5: LGTM! Transaction ensures safe schema modification.
The migration is properly wrapped in a transaction, ensuring atomicity of the schema change.
packages/loot-core/src/types/models/category-group.d.ts (2)
1-1: LGTM! Import of WithRequired utility type.
The WithRequired utility type is appropriately imported to enforce type safety for parent-child relationships.
18-18: Consider adding runtime validation for hierarchical constraints.
The type definition correctly enforces parent-child relationships. However, consider implementing runtime validations to:
- Prevent circular references (a group being its own ancestor)
- Enforce a reasonable maximum depth for nested groups
Let's check if there are any existing validation utilities we can leverage:
packages/desktop-client/src/hooks/useFeatureFlag.ts (1)
11-11: Implementation looks good!
The feature flag is correctly implemented, following the existing patterns and maintaining type safety. The default value of false is appropriate for a gradual rollout of the sub-category groups feature.
packages/desktop-client/src/hooks/useCategories.ts (3)
1-1: LGTM: Import changes are correct.
The addition of useMemo import is appropriate for the new memoization implementation.
19-26: LGTM: Memoization implementation is correct.
The use of useMemo is appropriate here to prevent unnecessary recalculations of the filtered groups.
22-23: Verify usage and performance implications.
The new groupedHierarchy property introduces filtering on each render when the selector changes. Let's verify its usage and potential performance impact.
✅ Verification successful
Let me try another search strategy to verify the usage of groupedHierarchy.
Let me try one more search with different commands to find the usage patterns.
Based on the gathered information, I can now provide the final response:
Performance impact is minimal; usage is well-contained
The groupedHierarchy property is only used within the useCategories hook and is properly memoized using useMemo. The filtering operation is simple and only runs when the selector updates. The data structure maintains only top-level categories (where parent_id === null), which is an efficient way to represent the hierarchy.
- The property is only referenced in one file:
packages/desktop-client/src/hooks/useCategories.ts - The hook itself is used across various UI components, but they receive the memoized result
- The filtering operation is performed on the
groupedarray which is already available in the selector, not causing additional database queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of groupedHierarchy and potential performance implications
# Check where groupedHierarchy is being used
echo "Checking usage of groupedHierarchy:"
rg "groupedHierarchy" --type ts --type tsx
# Look for large datasets that might impact performance
echo "\nChecking for potential large datasets:"
rg "INSERT INTO category_groups" --type sql
Length of output: 568
Script:
#!/bin/bash
# Search for groupedHierarchy with correct file type
rg "groupedHierarchy" -t typescript
# Check the component/hook usage where this selector is used
rg "useCategories" -t typescript -A 3
# Look at the actual implementation to understand the data flow
ast-grep --pattern 'const useCategories = $_'
Length of output: 3090
Script:
#!/bin/bash
# Search for files containing groupedHierarchy
fd -e ts -e tsx -x grep -l "groupedHierarchy" {}
# Search for useCategories hook usage
fd -e ts -e tsx -x grep -l "useCategories" {}
# Look at the content of the useCategories hook file
cat packages/desktop-client/src/hooks/useCategories.ts
Length of output: 2638
packages/desktop-client/src/components/budget/IncomeHeader.tsx (2)
27-30: LGTM! Implementation correctly handles the new optional parameter.
The Button implementation appropriately calls onShowNewGroup without a parent parameter, which is the expected behavior for adding a root-level group.
11-11: Verify all implementations of IncomeHeaderProps.
The type change for onShowNewGroup is correctly implemented to support hierarchical groups. However, this is a breaking change that requires verification of all components implementing IncomeHeaderProps.
✅ Verification successful
The type change is correctly implemented across the codebase
The verification shows that onShowNewGroup is properly implemented in BudgetCategories.jsx with the parent group ID parameter:
function onShowNewGroup(parentId) {
setIsAddingGroup(true);
setNewGroupForGroup(parentId);
}This is the only implementation of IncomeHeader in the codebase, and it correctly handles the optional parent parameter as specified in the type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might be affected by the IncomeHeaderProps change
# Look for files that import or use IncomeHeader component
# Search for imports of IncomeHeader
echo "Files importing IncomeHeader:"
rg -l "import.*IncomeHeader.*from"
# Search for usage of IncomeHeader component
echo -e "\nFiles using IncomeHeader component:"
rg -l "<IncomeHeader"
Length of output: 350
Script:
#!/bin/bash
# Let's examine how BudgetCategories.jsx uses IncomeHeader to verify the prop usage
# Show the context around IncomeHeader usage
echo "IncomeHeader usage in BudgetCategories.jsx:"
rg -A 5 -B 5 "<IncomeHeader" packages/desktop-client/src/components/budget/BudgetCategories.jsx
# Also check the onShowNewGroup implementation
echo -e "\nonShowNewGroup implementation:"
ast-grep --pattern 'onShowNewGroup = $_' packages/desktop-client/src/components/budget/BudgetCategories.jsx
Length of output: 782
Script:
#!/bin/bash
# Let's find the onShowNewGroup function declaration in BudgetCategories.jsx
echo "Looking for onShowNewGroup function declaration:"
rg -A 10 "onShowNewGroup" packages/desktop-client/src/components/budget/BudgetCategories.jsx
# Also check if there are any other method declarations or assignments
echo -e "\nChecking for other onShowNewGroup declarations:"
ast-grep --pattern 'const onShowNewGroup = $_'
Length of output: 4627
packages/desktop-client/src/components/budget/IncomeGroup.tsx (3)
27-28: LGTM: Type definitions are well-structured.
The optional props onShowNewGroup and depth are properly typed and align with the PR's objective of introducing hierarchical groups while maintaining backward compatibility.
40-41: LGTM: Component parameters properly updated.
The new props are correctly destructured and align with the type definitions.
63-64: Verify SidebarGroup component prop types.
While the props are correctly forwarded, let's verify that the SidebarGroup component properly declares these props in its type definition.
✅ Verification successful
Props are properly typed in SidebarGroup component
The verification shows that both onShowNewGroup and depth props are correctly declared in the SidebarGroupProps type definition:
onShowNewGroup?: (parent?: string) => void(line 37)depth?: number(line 40)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SidebarGroup component properly declares the new props
# Expected: Find type definition of SidebarGroup with onShowNewGroup and depth props
# Search for SidebarGroup type/interface definition
ast-grep --pattern 'type SidebarGroupProps = {
$$$
}'
# Alternative pattern if it's an interface
ast-grep --pattern 'interface SidebarGroupProps {
$$$
}'
Length of output: 2332
packages/loot-core/src/server/budget/base.test.ts (1)
20-20: LGTM! Type improvement from integer to boolean.
The change from integer to boolean for is_income improves type safety and makes the code more semantically correct.
packages/loot-core/src/types/prefs.d.ts (2)
7-7: LGTM! Addition of 'subCategoryGroups' feature flag.
The addition of 'subCategoryGroups' feature flag aligns with the PR objectives and enables controlled rollout of the hierarchical category groups feature.
#!/bin/bash
# Description: Verify the implementation of the new feature flag
# Test 1: Check feature flag usage in components
echo "Checking feature flag usage in components..."
ast-grep --pattern 'useFeatureFlag($_, "subCategoryGroups")'
# Test 2: Check for feature toggle in settings
echo "Checking feature toggle implementation..."
rg -A 5 "subCategoryGroups.*FeatureToggle"6-6: Verify removal of 'upcomingLengthAdjustment' feature flag.
The removal of this feature flag seems unrelated to the PR's objective of introducing sub-category groups. Please verify if this removal is intentional and won't break any existing functionality.
packages/desktop-client/src/components/budget/IncomeCategory.tsx (1)
31-31: LGTM! Type definition is correct.
The optional depth property is properly typed as number | undefined.
packages/loot-core/src/server/models.ts (1)
81-81: LGTM: Type change improves flexibility
The change to Partial<CategoryGroupEntity> is appropriate as it allows for partial objects while still maintaining type safety. The required fields are still properly enforced through the requiredFields function.
packages/desktop-client/src/components/budget/ExpenseCategory.tsx (2)
55-55: LGTM!
The depth prop is correctly destructured from the component props.
102-102: Verify SidebarCategory depth prop implementation.
The depth prop is correctly passed to SidebarCategory, but we should verify its implementation.
✅ Verification successful
SidebarCategory depth prop implementation is correct
The depth prop is properly typed as an optional number and correctly used for indentation in the component's styling (paddingLeft calculation). The implementation follows the hierarchical rendering requirement by applying appropriate left padding based on the depth value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SidebarCategory component handles the depth prop correctly
# Check SidebarCategory implementation and its prop types
ast-grep --pattern 'type SidebarCategoryProps = {
$$$
depth?: number;
$$$
}'
# Check for depth usage in SidebarCategory
rg -A 5 'depth.*?' packages/desktop-client/src/components/budget/SidebarCategory
Length of output: 1819
Script:
#!/bin/bash
# Let's check how depth prop is used in the implementation
ast-grep --pattern 'const SidebarCategory = ({ depth, $$$ }: SidebarCategoryProps) => {
$$$
}'
# Also search for any usage of depth in the file
rg -A 5 'depth' packages/desktop-client/src/components/budget/SidebarCategory.tsx
Length of output: 655
packages/loot-core/src/client/reducers/queries.ts (1)
114-124: LGTM! The implementation aligns with PR objectives.
The recursive implementation correctly handles the hierarchical category groups while maintaining backward compatibility through a flattened structure. This aligns well with the PR objectives of introducing hierarchy while ensuring existing group usages remain functional.
packages/desktop-client/src/components/budget/ExpenseGroup.tsx (4)
5-5: LGTM: Palette import added correctly
The palette import is properly added and used for the new background color logic.
54-55: LGTM: Parameters properly added
The new parameters are correctly destructured from props and match the interface definition.
135-136: LGTM: Props correctly passed to SidebarGroup
The new props are properly passed through to the SidebarGroup component.
Line range hint 1-141: Verify feature flag implementation
The PR objectives mention feature flag usage, but there's no visible feature flag check in this component.
packages/desktop-client/src/components/budget/SidebarCategory.tsx (2)
33-33: LGTM: Type definition for depth property.
The optional depth property is correctly typed and aligns with the hierarchical group implementation objective.
49-49: LGTM: Props destructuring.
The depth property is correctly destructured from the component props.
packages/desktop-client/src/components/budget/SidebarGroup.tsx (4)
6-6: LGTM: Clean prop type additions and imports
The new feature flag import and prop type additions are well-structured and properly typed.
Also applies to: 37-40
54-57: LGTM: Clean feature flag implementation
The feature flag initialization and prop destructuring are properly implemented, following existing patterns.
Also applies to: 64-64
128-129: LGTM: Clean menu handling implementation
The menu changes are well-implemented with proper feature flag control and consistent with the existing menu handling pattern.
Also applies to: 139-142
Line range hint 66-74: Verify accessibility of hierarchical navigation
The nested group structure might affect keyboard navigation and screen reader experience. Please ensure:
- Keyboard users can navigate the hierarchy effectively
- Screen readers announce the group depth/level correctly
- ARIA attributes are properly set for the hierarchical structure
packages/desktop-client/src/components/budget/BudgetTable.jsx (2)
Line range hint 41-43: LGTM: Fixed typo in setter function name.
Good catch on fixing the typo in the setter function name from Pef to Pref.
37-37: Verify that existing group operations work with hierarchical data.
The switch from grouped to groupedHierarchy aligns with the PR's goal of introducing hierarchical category groups. However, we should verify that existing group operations continue to work correctly with the new data structure.
Let's verify the implementation of groupedHierarchy and its usage:
✅ Verification successful
The change from grouped to groupedHierarchy is safe and correctly implemented
The implementation is verified to be working correctly for the following reasons:
groupedHierarchyis simply filtering the existinggroupeddata to show only top-level groups (parent_id === null)- The original group operations (
_onReorderGroup,_onReorderCategory) continue to work with the hierarchical structure as they:- Still use the full
categoryGroupsdata for finding and manipulating categories - Maintain existing group operations logic without requiring changes
- Still use the full
- The component properly handles all group-related operations including collapsing, expanding, and keyboard navigation with the filtered hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of groupedHierarchy
# Test 1: Check the implementation of groupedHierarchy in useCategories hook
echo "Checking useCategories implementation..."
rg -A 10 "groupedHierarchy" "packages/desktop-client/src/hooks/useCategories.ts"
# Test 2: Verify all components using the useCategories hook
echo "Checking other components using useCategories..."
rg "useCategories.*grouped" "packages/desktop-client/src/components"
Length of output: 575
Script:
#!/bin/bash
# Let's gather more comprehensive information about the category hierarchy implementation
# Check the complete implementation of useCategories
echo "Checking complete useCategories implementation..."
rg -l "useCategories" packages/desktop-client/src --type ts --type tsx --type jsx
# Once we find the file, let's look at its content
cat packages/desktop-client/src/hooks/useCategories.ts
# Check how BudgetTable uses the category groups
echo "Checking BudgetTable implementation..."
cat packages/desktop-client/src/components/budget/BudgetTable.jsx
# Check for any group operations in BudgetTable
echo "Checking group operations..."
rg "_onReorderGroup|_onReorderCategory" packages/desktop-client/src/components/budget/BudgetTable.jsx -A 5
Length of output: 8648
packages/loot-core/src/types/server-handlers.d.ts (1)
89-90: LGTM! Verify usage consistency across components.
The new handler aligns well with the PR objectives, returning a flattened list of category groups that supports the hierarchical structure.
Let's verify the consistent usage of this handler across components:
✅ Verification successful
Implementation and usage of get-category-groups handler is consistent and well-structured
The handler is properly:
- Implemented in
main.tsusing the database layer'sgetCategoriesGrouped - Used in
api.tswith appropriate model transformation viacategoryGroupModel.toExternal - Type definitions match the implementation and usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of the new handler
rg "get-category-groups" --type ts --type tsx --type jsx
Length of output: 84
Script:
#!/bin/bash
# Search for all usages of the new handler with correct ripgrep options
rg "get-category-groups" -t ts -t js -t jsx
Length of output: 71
Script:
#!/bin/bash
# Search for all usages of the new handler with basic ripgrep
rg "get-category-groups"
Length of output: 324
Script:
#!/bin/bash
# Let's check the implementation and usage context
echo "=== Handler Implementation ==="
rg -A 5 "get-category-groups" packages/loot-core/src/server/main.ts
echo -e "\n=== API Usage Context ==="
rg -A 5 "get-category-groups" packages/loot-core/src/server/api.ts
Length of output: 636
packages/loot-core/src/client/actions/queries.ts (1)
217-219: Verify backend endpoint compatibility.
The implementation looks good and aligns with the PR objectives for hierarchical groups. However, we should verify that the backend endpoint 'category-group-create' has been updated to handle the new parentId parameter.
✅ Verification successful
Let me search for the backend handler implementation to verify it properly handles the parentId parameter.
Let me check the migration file and also try a different pattern to find the handler implementation.
Let me try one more search pattern to locate the handler implementation.
Backend endpoint is properly updated to handle parentId parameter
The implementation is safe to use as verified by:
- The backend handler in
src/server/main.tscorrectly accepts and processes theparentIdparameter - The database schema has been updated with
parent_idcolumn incategory_groupstable - The type definitions in
server-handlers.d.tsproperly include the optionalparentIdparameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the backend endpoint implementation
rg -A 10 "category-group-create.*parentId" --type ts
Length of output: 949
Script:
#!/bin/bash
# Search for the backend handler implementation
ast-grep --pattern 'handlers["category-group-create"] = $_'
# Also search for any related database schema or migration files
rg -l "category.*group.*parent" --type sql
Length of output: 174
Script:
#!/bin/bash
# Check the migration file
cat packages/loot-core/migrations/1726730827000_sub_category_groups.sql
# Try different pattern to find handler
rg -A 10 '"category-group-create"' --type ts
Length of output: 217
Script:
#!/bin/bash
# Search for category group related handlers
rg -A 10 "category.*group.*create" --type ts
# Also search for any tests that might verify this functionality
rg -l "category.*group.*create.*test" --type ts
Length of output: 5526
packages/desktop-client/src/components/budget/BudgetCategories.jsx (2)
43-43: LGTM: Clean state management addition
The new state variable newGroupForGroup is well-named and properly integrated for managing parent-child relationships in category groups.
153-161: Verify parent-child relationship integrity
While the group creation logic handles the basic parent-child relationship, it might be vulnerable to circular references.
Let's verify if there are any existing checks for circular references:
Consider adding validation to prevent:
- A group becoming its own ancestor
- Deep nesting beyond reasonable limits
packages/loot-core/src/server/main.test.ts (1)
161-161: LGTM! Consider adding hierarchy tests.
The change from implicit integer to explicit boolean for is_income improves type safety. However, given that this PR introduces category group hierarchy, consider adding test cases to verify the parent-child relationships between category groups.
Let's verify if there are any existing tests for hierarchical groups:
packages/desktop-client/src/components/budget/index.tsx (2)
189-191: Consider extending duplicate name check for hierarchical groups.
The current implementation only checks for duplicate category names within the same immediate group. With the introduction of hierarchical groups, consider extending this check to prevent duplicate names across all subgroups under the same parent group to avoid potential confusion.
Let's verify the current behavior:
Consider implementing a utility function that handles category name validation consistently across the codebase, taking into account the full group hierarchy.
235-235:
Add validation for parent_id in group creation.
The addition of parent_id enables hierarchical groups, but consider adding the following validations:
- Ensure the parent group exists
- Prevent circular references (a group cannot be its own ancestor)
- Consider implementing a maximum depth limit for group hierarchies
Let's verify the current validation:
These validations are essential to prevent data integrity issues and potential rendering problems in the UI.
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (2)
424-426: LGTM! Type definition is consistent with existing patterns.
The addition of the group prop to IncomeGroupMonthProps maintains consistency with ExpenseGroupMonthProps and supports the hierarchical group structure.
442-442: Verify the binding change maintains existing functionality.
The change from groupIncomeReceived to groupSumAmount(group.id) looks correct and aligns with the hierarchical group structure. However, let's verify that this change maintains the same behavior for income groups.
✅ Verification successful
Let me gather more context about the income-related functionality to ensure the change maintains the same behavior.
Based on the gathered information, I can now provide a final response:
The binding change from groupIncomeReceived to groupSumAmount is correct and safe
The codebase shows that groupIncomeReceived is just an alias for total-income, and total-income is internally bound to group-sum-amount-${incomeGroup.id} in both envelope and report budgets. The new binding groupSumAmount(group.id) directly uses the same underlying value, making this change an improvement that:
- Maintains identical functionality
- Provides better consistency with how other group amounts are handled
- Properly supports the hierarchical group structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other usages of groupIncomeReceived to ensure all instances are updated
# Test: Search for any remaining usage of groupIncomeReceived
echo "Searching for remaining usages of groupIncomeReceived..."
rg "groupIncomeReceived"
# Test: Search for similar patterns to understand the context
echo "Searching for similar group sum patterns..."
rg "groupSumAmount|groupIncomeReceived"
Length of output: 1734
Script:
#!/bin/bash
# Search for the implementation of group-sum-amount and total-income
echo "Searching for group-sum-amount implementation..."
rg "group-sum-amount" -A 5
# Search for total-income implementation
echo "Searching for total-income implementation..."
rg "total-income" -A 5
# Search for income-related group handling
echo "Searching for income group handling..."
ast-grep --pattern 'type === "income"'
Length of output: 19228
packages/loot-core/src/server/accounts/sync.test.ts (2)
Line range hint 1-585: Test coverage looks comprehensive!
The test suite thoroughly covers various scenarios including:
- Transaction reconciliation with different imported_id values
- Fuzzy search functionality with and without imported_id
- Edge cases in transaction matching
- Payee handling and merging scenarios
This robust test coverage helps ensure the stability of the category groups feature.
37-41:
Fix inconsistent is_income type usage
While insertCategoryGroup now uses a boolean value for is_income, the subsequent insertCategory call still uses an integer value. This inconsistency should be addressed to maintain type consistency across the codebase.
Apply this diff to fix the inconsistency:
await db.insertCategory({
name: 'income',
cat_group: 'group1',
- is_income: 1,
+ is_income: true,
});Likely invalid or redundant comment.
packages/loot-core/src/server/budget/base.ts (7)
66-67: Recursively process child category groups
The addition of recursively calling createCategoryGroup on children allows for hierarchical category groups to be processed correctly.
71-74: Include child groups in sum amount dependencies
By adding child group sums to the dependencies array, the total amounts now include contributions from both categories and child groups, ensuring accurate aggregation.
81-84: Include child groups in budget dependencies
Including child group budgets in the dependencies ensures that the total budget for a group accounts for the budgets of all nested child groups.
90-93: Include child groups in leftover calculations
Adding child groups to the dependencies for leftovers ensures that leftover amounts accurately reflect all nested categories and groups.
332-335: Avoid adding dependencies for child groups to prevent duplication
By checking if (!group.parent_id), dependencies are added only for top-level groups. This prevents duplication because child groups' contributions are already included through parent group dependencies.
430-430: Update to hierarchical category groups in createBudget
Using getCategoriesGroupedHierarchical() ensures that the budget creation process now correctly handles hierarchical category groups, aligning with the new data structure.
98-117:
Ensure parent group dependencies are acyclic
When adding dependencies to parent groups based on group.parent_id, ensure that the category group hierarchy does not contain circular references. Circular dependencies could lead to infinite loops or stack overflows during calculations.
Please verify that the category groups do not form cycles. You can perform this check by ensuring that no group's parent_id references its own id, either directly or through a chain of parent groups.
packages/loot-core/src/server/db/index.ts (1)
295-295: Verify default value impact for 'hierarchical' parameter
The hierarchical parameter in getCategoriesGrouped defaults to false. Ensure that existing calls to this function are not unintentionally affected by this default value, and that they explicitly set hierarchical when necessary.
You can run the following script to find all calls to getCategoriesGrouped that may need to be updated:
✅ Verification successful
Let me try a different approach to find the usage of getCategoriesGrouped.
Based on the search results, I can now generate the final response:
Default value for 'hierarchical' parameter is safe
The codebase shows consistent usage of getCategoriesGrouped(). All direct calls use it without the hierarchical parameter, intentionally relying on the false default. When hierarchical grouping is needed, the code explicitly uses the dedicated getCategoriesGroupedHierarchical() wrapper function, which calls getCategoriesGrouped(ids, true). This demonstrates a well-structured pattern that makes the code's intent clear.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to 'getCategoriesGrouped' without the 'hierarchical' parameter.
# Expected: Identify calls with less than two arguments.
ast-grep --pattern $'getCategoriesGrouped($_)' --lang ts
Length of output: 57
Script:
#!/bin/bash
# Search for getCategoriesGrouped function calls using ripgrep for better context
rg "getCategoriesGrouped" -A 2 -B 2
Length of output: 4070
|
I generally like that the sub groups show up as their own groups everywhere else in the app. That way these sub groups should "just work" everywhere else. Personally I think I would prefer having sub categories instead of sub groups, but this is probably a lot easier to add. |
Similar idea I guess. It would be nice to be able to budget to a collection of categories and then each of the underlying categories then utilize the overall budget instead of having to be budgeted for individually. |
I'd say we definitely don't want to be budgeting to anything other than a specific category, otherwise it removes the whole idea of envelope budgeting and being specific to what the money is for. We should encourage good habits, not bad ones |
|
Can the "add group" option be listed as "add sub-group" to differentiate between this and the "Add Group" button at the bottom of the budget? |
# Conflicts: # packages/desktop-client/src/components/budget/BudgetCategories.jsx # packages/desktop-client/src/components/budget/SidebarGroup.tsx # packages/desktop-client/src/components/settings/Experimental.tsx # packages/desktop-client/src/hooks/useFeatureFlag.ts # packages/loot-core/src/types/prefs.d.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/budget/SidebarGroup.tsx (3)
64-64: Consider extracting padding constants.The padding calculation logic is correct but could be more maintainable. Consider extracting the magic numbers into named constants:
+ const INDENT_PER_LEVEL = 13; + const DEPTH_ADJUSTMENT = 5; paddingLeft: (depth ?? 0) * INDENT_PER_LEVEL - (depth ? DEPTH_ADJUSTMENT : 0),Also applies to: 73-73
206-209: Remove redundant id spread.The
idproperty is already included in the spread ofgroup, making its explicit inclusion redundant:- onSave({ ...group, id: group.id, name: value }); + onSave({ ...group, name: value });
Line range hint
65-97: Consider enhancing keyboard accessibility.The hierarchical structure would benefit from proper ARIA attributes and keyboard navigation support:
- Add
role="treeitem"to group items- Add
aria-expandedattribute for collapse state- Implement keyboard navigation (arrow keys) for the hierarchy
Would you like me to provide a detailed example of these accessibility enhancements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/budget/SidebarGroup.tsx(7 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/budget/SidebarGroup.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/SidebarGroup.tsx:69-76
Timestamp: 2024-10-13T11:17:59.711Z
Learning: In the `SidebarGroup` component (`packages/desktop-client/src/components/budget/SidebarGroup.tsx`), context menu state management is handled in another file, ensuring that only one context menu is open at a time.
🔇 Additional comments (2)
packages/desktop-client/src/components/budget/SidebarGroup.tsx (2)
6-6: LGTM: New imports and props are well-defined.
The new feature flag import and additional props (onShowNewGroup and depth) are properly typed and align well with the PR's objective of implementing hierarchical category groups.
Also applies to: 37-40
Line range hint 128-142: LGTM: Menu handling is well-implemented.
The sub-group menu item is properly integrated with:
- Correct feature flag gating
- Proper translation support
- Consistent menu structure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/budget/BudgetCategories.jsx (3)
48-86: Consider memoizing expanded groups for better performanceThe recursive
expandGroupimplementation correctly handles the group hierarchy, but could benefit from performance optimization when dealing with large category structures.Consider memoizing the expanded groups:
+ const memoizedExpandGroup = useMemo(() => { function expandGroup(group, depth = 0, type = 'expense') { // ... existing implementation ... } return expandGroup; + }, [newCategoryForGroup, isAddingGroup, newGroupForGroup, collapsedGroupIds, showHiddenCategories]);
154-161: Add error handling for group creationWhile the group creation handlers are well-implemented, they could benefit from error handling to manage failed group creation scenarios.
Consider adding error handling:
function onShowNewGroup(parentId) { + try { setIsAddingGroup(true); setNewGroupForGroup(parentId); + } catch (error) { + console.error('Failed to initiate group creation:', error); + onHideNewGroup(); + } }
254-254: Add prop-types validation for depth propertyThe depth property is consistently implemented across components, but would benefit from prop-types validation to ensure type safety.
Add prop-types validation:
+ import PropTypes from 'prop-types'; ExpenseGroup.propTypes = { + depth: PropTypes.number.isRequired, // ... other prop types }; ExpenseCategory.propTypes = { + depth: PropTypes.number.isRequired, // ... other prop types }; // Similar for IncomeGroup and IncomeCategoryAlso applies to: 275-275, 306-306, 325-325
packages/loot-core/src/server/main.ts (1)
368-368: Add validation for parent-child relationships.The implementation correctly supports hierarchical groups by adding the
parentIdparameter. However, consider adding these validations:
- Verify that the parent group exists
- Prevent circular references in the hierarchy
- Ensure parent and child groups have consistent income status
Example validation implementation:
handlers['category-group-create'] = mutator(async function ({ name, isIncome, parentId, }) { return withUndo(async () => { + if (parentId) { + const parent = await db.first( + 'SELECT is_income FROM category_groups WHERE id = ? AND tombstone = 0', + [parentId] + ); + + if (!parent) { + throw new Error('Parent group not found'); + } + + if (parent.is_income !== (isIncome ?? false)) { + throw new Error('Parent and child groups must have the same income status'); + } + } + return db.insertCategoryGroup({ name, is_income: isIncome ?? false, parent_id: parentId ?? null, }); }); });Also applies to: 373-374
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/desktop-client/src/components/budget/BudgetCategories.jsx(7 hunks)packages/desktop-client/src/components/budget/BudgetTable.jsx(1 hunks)packages/desktop-client/src/components/budget/ExpenseGroup.tsx(5 hunks)packages/desktop-client/src/components/budget/SidebarCategory.tsx(3 hunks)packages/desktop-client/src/components/budget/SidebarGroup.tsx(7 hunks)packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx(2 hunks)packages/desktop-client/src/components/budget/index.tsx(2 hunks)packages/desktop-client/src/components/settings/Experimental.tsx(1 hunks)packages/desktop-client/src/hooks/useFeatureFlag.ts(1 hunks)packages/loot-core/src/client/actions/queries.ts(1 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/types/prefs.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 (10)
- packages/desktop-client/src/components/budget/BudgetTable.jsx
- packages/desktop-client/src/components/budget/SidebarCategory.tsx
- packages/desktop-client/src/components/budget/SidebarGroup.tsx
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
- packages/desktop-client/src/components/budget/index.tsx
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/loot-core/src/client/actions/queries.ts
- packages/loot-core/src/types/prefs.d.ts
- packages/loot-core/src/types/server-handlers.d.ts
🔇 Additional comments (7)
packages/desktop-client/src/components/budget/ExpenseGroup.tsx (4)
39-40: LGTM: Type definitions are well-structured
The new type definitions for onShowNewGroup and depth are properly typed and align with the hierarchical group structure requirements.
58-59: LGTM: Props are properly implemented
The new props are correctly destructured and follow the established pattern in the component.
93-94: Review the visual hierarchy implementation
The current implementation makes deeper levels (depth > 0) lighter by using tableRowHeaderBackground, while root level uses the darker navy900. This seems counterintuitive to typical visual hierarchy patterns where nested items are usually more distinct/darker to show depth.
Consider reviewing if this is the intended visual hierarchy:
- backgroundColor:
- depth > 0 ? theme.tableRowHeaderBackground : colorPalette.navy900,
+ backgroundColor:
+ depth > 0 ? colorPalette.navy900 : theme.tableRowHeaderBackground,140-141: LGTM: SidebarGroup props properly forwarded
The new hierarchy-related props are correctly passed down to the SidebarGroup component.
packages/desktop-client/src/components/budget/BudgetCategories.jsx (2)
207-207: LGTM: Parent-child relationship implementation
The addition of parent_id to new group creation correctly implements the hierarchical relationship between groups.
44-44: LGTM: State management for subgroups
The new state variable newGroupForGroup is well-implemented for tracking parent-child relationships in the group hierarchy.
Let's verify the state management implementation:
✅ Verification successful
Let me gather more context about the state management implementation to ensure proper verification.
Let me check one more aspect to verify the cleanup handling in error scenarios and component unmounting.
Based on the gathered information, I can now provide the final response:
State management for subgroups is properly implemented
The newGroupForGroup state is well-managed with appropriate cleanup:
- Properly initialized as null
- Reset after group creation via
onHideNewGroup - Correctly used in the component hierarchy for new group creation
- Properly handled in save scenarios through
_onSaveGroup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper state cleanup in error scenarios
rg "setNewGroupForGroup\(.*\)" --type js
Length of output: 246
Script:
#!/bin/bash
# Check the usage context of newGroupForGroup and its cleanup
rg "newGroupForGroup" -B 2 -A 2 --type js
Length of output: 2220
Script:
#!/bin/bash
# Check error handling and cleanup scenarios
rg "_onSaveGroup|onSave.*Group" -B 3 -A 5 --type js
Length of output: 8227
packages/loot-core/src/server/main.ts (1)
373-373: Good use of nullish coalescing for type safety.
The change from direct assignment to using the nullish coalescing operator (??) for is_income improves type safety by ensuring a boolean value.
|
Neat, thanks for implementing this. Dragging and dropping budget categories doesn't work correctly anymore, tho. For the existing first level of categories: When you add a subgroup to an existing group, if there's been existing categories, you can shuffle them around at the level they're currently are (i.e. level 1), you can drag new ones into said level and drag them back out. Once the last category was removed, you can't drag anything back into that group at level 1 anymore. Until you manually create a new category. I figure the drag-drop code looks for one to latch to or whatever. For subgroups: No drag-drop whatsoever works here. So you can't reorder anything you've created. Also, in regards to organisation, I feel like the subgroups should either trail the categories at said level or there need to be vertical dotted line (or something) from the header of the group to the first category of said group, because things might eventually get hard to read if there's quite some hierarchy and everything is expanded:
|
|
related #1320 |
Previous: #3589
Add hierachy to category groups.
What is changed compared to the previous is: