[WIP] Proof of concept for sub categories. #3589
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe pull request introduces several changes across multiple components within the Possibly related PRs
Suggested labels
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: 15
🧹 Outside diff range and nitpick comments (18)
packages/loot-core/src/types/models/category-group.d.ts (1)
17-17: LGTM: Addition of children property completes subcategory structure.The new optional
childrenproperty in theCategoryGroupEntityinterface is a great addition. It allows for the representation of child category groups, completing the hierarchical structure needed for subcategories. The use ofWithRequired<CategoryGroupEntity, 'parent_id'>ensures that child category groups always have aparent_id, which is logically correct.For improved clarity, consider adding a comment explaining the purpose of the
WithRequiredtype:// Ensures that child category groups always have a parent_id children?: WithRequired<CategoryGroupEntity, 'parent_id'>[];This comment would help other developers understand the reasoning behind using
WithRequired.packages/desktop-client/src/components/budget/IncomeHeader.tsx (1)
11-11: Consider updating related components to leverage new subcategory functionalityThe changes to
IncomeHeaderlay the groundwork for implementing subcategories while maintaining backwards compatibility. To fully utilize this new capability:
- Review and update parent components or other related components that use
IncomeHeaderto take advantage of the optionalparentparameter inonShowNewGroup.- Ensure that the logic for handling subcategories is implemented in the component that provides the
onShowNewGroupfunction.These changes are a good start for the "Proof of concept for sub categories" as mentioned in the PR objectives. As you continue to develop this feature, consider how the UI will represent the hierarchy of categories and how the user will interact with subcategories throughout the application.
Also applies to: 27-30
packages/desktop-client/src/components/budget/IncomeGroup.tsx (2)
27-27: LGTM! Consider using a more specific type forparent.The addition of the
onShowNewGroupproperty aligns well with the PR objective of implementing subcategories. It provides the necessary flexibility for creating new groups, potentially as subgroups.For improved type safety, consider using a more specific type for the
parentparameter:onShowNewGroup?: (parent?: string | null) => void;This explicitly allows
nullas a valid value, which might be useful to distinguish between top-level groups and subgroups.
39-39: LGTM! Consider adding a comment for clarity.The implementation of the
onShowNewGroupproperty in theIncomeGroupcomponent is correct and consistent with the type definition. It's properly passed down to theSidebarGroupcomponent, which is likely where the new group creation UI is handled.For consistency and clarity, consider adding a short comment above the
onShowNewGroupprop in the destructured props list, similar to other props:// ... other props onShowNewCategory, // Callback for creating a new group onShowNewGroup, }: IncomeGroupProps) {This helps maintain the documentation style and provides a quick explanation of the prop's purpose.
Also applies to: 61-61
packages/desktop-client/src/components/budget/ExpenseCategory.tsx (1)
55-55: LGTM: Proper integration ofdepthprop inExpenseCategorycomponent.The
depthprop is correctly added to the function signature and passed down to theSidebarCategorycomponent. This change is consistent with the addition of thedepthproperty toExpenseCategoryPropsand supports the implementation of subcategories.For improved clarity, consider adding a brief comment explaining the purpose of the
depthprop, especially if it's used for indentation or visual hierarchy in the UI.Also applies to: 102-102
packages/desktop-client/src/components/budget/ExpenseGroup.tsx (2)
18-18: Consider reordering imports for consistency.The import of
colorPaletteshould be moved before the import of component-specific modules to follow common import ordering conventions (third-party/built-in modules, then local modules).Apply this change:
import React, { type ComponentProps } from 'react'; import { theme } from '../../style'; +import * as colorPalette from '../../style/palette'; import { View } from '../common/View'; import { useDraggable, useDroppable, DropHighlight, type OnDragChangeCallback, type OnDropCallback, type DragState, } from '../sort'; import { Row, ROW_HEIGHT } from '../table'; import { RenderMonths } from './RenderMonths'; import { SidebarGroup } from './SidebarGroup'; -import * as colorPalette from '../../style/palette';🧰 Tools
🪛 GitHub Check: lint
[warning] 18-18:
../../style/paletteimport should occur before import of../common/View
89-90: LGTM: Improved visual hierarchy with depth-based background color.The background color logic now considers the
depthprop, enhancing the visual hierarchy of expense groups. This change aligns well with the PR objective of introducing subcategories.Consider using a ternary operator for improved readability:
- backgroundColor: - depth > 0 ? theme.tableRowHeaderBackground : colorPalette.navy900, + backgroundColor: depth === 0 ? colorPalette.navy900 : theme.tableRowHeaderBackground,packages/desktop-client/src/components/budget/SidebarCategory.tsx (2)
185-188: LGTM: Dynamic padding based on category depth.The modification to calculate
paddingLeftbased on thedepthprop effectively visualizes the hierarchical structure of categories. The use of the nullish coalescing operator ensures a fallback to 0 whendepthis undefined.Consider extracting the padding calculation into a constant for improved readability:
const depthPadding = 13; const basePadding = 13; const calculatedPadding = basePadding + (depth ?? 0) * depthPadding; style={{ paddingLeft: calculatedPadding, ...(isLast && { borderBottomWidth: 0 }), }}This change would make the padding calculation more explicit and easier to adjust in the future.
Line range hint
1-195: Overall: Well-implemented support for subcategories.The changes effectively introduce support for subcategories by adding a
depthproperty and using it for visual representation. The implementation is backward-compatible and well-integrated into the existingSidebarCategorycomponent.Consider extracting the
SidebarCategoryPropstype definition into a separate file (e.g.,types.ts) within the same directory. This would improve code organization and make it easier to reuse or extend these types in the future.Example:
// types.ts export type SidebarCategoryProps = { // ... (existing props) depth?: number; }; // SidebarCategory.tsx import { SidebarCategoryProps } from './types';This change would enhance the modularity of your code and make it easier to manage as the component grows.
packages/desktop-client/src/components/budget/SidebarGroup.tsx (1)
71-71: LGTM! Consider using a named constant for padding calculation.The dynamic
paddingLeftbased on thedepthprop is a good addition for visual hierarchy. However, the magic number13used in the calculation could be replaced with a named constant or theme variable for better maintainability.Consider refactoring the padding calculation:
const INDENT_SIZE = 13; // pixels per depth level // ... paddingLeft: (depth ?? 0) * INDENT_SIZE,packages/loot-core/src/types/server-handlers.d.ts (2)
94-94: LGTM: Updated method signature for creating category groupsThe addition of the optional
parentIdparameter to the'category-group-create'method is a good implementation to support the creation of subcategories. It maintains backward compatibility while allowing for the new hierarchical structure.Consider adding a comment to explain the purpose of the
parentIdparameter for better code documentation.You could add a comment like this:
parentId?: string; // ID of the parent category group for creating subcategories
97-99: LGTM: Improved flexibility for updating category groupsThe modification of the
'category-group-update'method to accept aPartial<CategoryGroupEntity>is a good improvement. It allows for more flexible and efficient updates by permitting partial modifications of category groups.For consistency with other method signatures in the interface, consider placing the parameter and return type on the same line if it doesn't exceed the line length limit.
You could update the method signature like this:
'category-group-update': (group: Partial<CategoryGroupEntity>) => Promise<unknown>;packages/loot-core/src/client/actions/queries.ts (2)
211-216: LGTM! Consider adding a return type for consistency.The changes to the
createGroupfunction look good and align with the PR objective of implementing subcategories. The new optionalparentIdparameter enables the creation of nested category groups while maintaining backward compatibility.For consistency with TypeScript best practices, consider adding a return type to the function:
-export function createGroup(name: string, parentId?: string) { +export function createGroup(name: string, parentId?: string): Promise<string> {This change would make the function signature more explicit about its asynchronous nature and the type of value it resolves to.
211-216: Consider potential updates to other category-related functions.While the changes to
createGroupare good, the introduction of subcategories might require updates to other category-related functions in the future.Consider reviewing and potentially updating the following functions to handle the new hierarchical structure:
deleteGroup: It might need to handle deleting nested groups.updateGroup: It might need to allow changing the parent of a group.getCategories: Ensure it properly retrieves and structures the hierarchical category data.These changes might be planned for future iterations, but it's worth keeping in mind for the overall architecture of the category management system.
packages/desktop-client/src/components/budget/index.tsx (1)
Line range hint
1-438: Overall feedback on subcategory implementationThe changes implemented in this file are a good start towards the subcategory feature. The modifications to the category existence check and group creation logic are well-thought-out and align with the PR objectives.
To further improve the implementation, consider the following suggestions:
- Implement comprehensive error handling and validation for the new
parent_idparameter.- Update relevant components and UI elements to reflect the new subcategory structure.
- Ensure that the changes are consistent across the entire codebase, including actions, reducers, and any affected components.
- Add unit tests to cover the new subcategory functionality.
These changes lay a solid foundation for the subcategory feature, but further work may be needed to fully integrate it into the application.
packages/loot-core/src/server/main.ts (2)
370-376: LGTM! Consider adding type annotations.The changes to
category-group-createhandler look good. The addition of theparentIdparameter and its usage indb.insertCategoryGroupintroduces support for hierarchical category groups. The use of the nullish coalescing operator foris_incomeis a good practice.Consider adding TypeScript type annotations to the function parameters for improved type safety and code readability. For example:
handlers['category-group-create'] = mutator(async function ({ name, isIncome, parentId, }: { name: string; isIncome?: boolean; parentId?: string; }) { // ... rest of the function });
Action Required: Implement Error Handling and Validate
transferIdThe
deleteCategoryGroupfunction inpackages/loot-core/src/server/db/index.tslacks explicit error handling and does not validate thetransferId. To enhance the robustness of this function, please consider the following improvements:
- Error Handling: Add specific error handling for all database operations to manage potential failures gracefully.
- Validation: Implement validation to ensure that
transferIdis valid and exists before proceeding with category transfers.- Documentation: Enhance comments to clarify the logic flow, especially around the transfer and deletion processes.
🔗 Analysis chain
Line range hint
397-422: Improve error handling and add comments for clarityThe changes to the
category-group-deletehandler improve the handling of category deletion, especially when a transfer is involved. However, there are a few areas that could be enhanced:
- Error handling: Consider adding more specific error handling for database operations.
- Comments: Add comments explaining the purpose of each step in the process, especially for the complex logic around transferring categories.
- Validation: Consider adding validation for the
transferIdto ensure it exists and is valid before proceeding with the deletion.Here's a suggested improvement with added comments and error handling:
handlers['category-group-delete'] = mutator(async function ({ id, transferId, }) { return withUndo(async () => { try { // Fetch categories associated with the group const groupCategories = await db.all( 'SELECT id FROM categories WHERE cat_group = ? AND tombstone = 0', [id], ); return batchMessages(async () => { if (transferId) { // Validate transferId exists and is valid const transferGroup = await db.first('SELECT id FROM category_groups WHERE id = ?', [transferId]); if (!transferGroup) { throw new Error('Invalid transfer group ID'); } // Transfer categories to the new group await budget.doTransfer( groupCategories.map(c => c.id), transferId, ); } // Delete the category group and update related data await db.deleteCategoryGroup({ id }, transferId); }); } catch (error) { console.error('Error deleting category group:', error); throw new Error('Failed to delete category group'); } }); });To ensure that the
deleteCategoryGroupfunction is correctly implemented and handles the transfer of categories, let's verify its implementation:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the deleteCategoryGroup function definition ast-grep --lang typescript --pattern $'async function deleteCategoryGroup($_, $_) { $$$ }'Length of output: 1544
Script:
#!/bin/bash # Retrieve the full implementation of deleteCategoryGroup cat packages/loot-core/src/server/db/index.ts | sed -n '/export async function deleteCategoryGroup/,/}/p'Length of output: 684
Script:
#!/bin/bash # Retrieve the deleteCategory function definition sed -n '/async function deleteCategory/,/}/p' packages/loot-core/src/server/db/index.ts # Retrieve the delete_ function definition sed -n '/async function delete_/,/}/p' packages/loot-core/src/server/db/index.tsLength of output: 1463
packages/desktop-client/src/components/budget/BudgetCategories.jsx (1)
98-98: Address the TODO: Implement income subgroupsThere's a TODO comment indicating that income subgroups need to be added. Would you like assistance in implementing this feature or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (3 hunks)
- packages/desktop-client/src/components/budget/BudgetCategories.jsx (7 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/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 (6 hunks)
- packages/desktop-client/src/components/budget/index.tsx (2 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/budget/base.ts (1 hunks)
- packages/loot-core/src/server/db/index.ts (4 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 (1 hunks)
- packages/loot-core/src/types/server-handlers.d.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/budget/ExpenseGroup.tsx
[warning] 18-18:
../../style/paletteimport should occur before import of../common/Viewpackages/loot-core/src/server/db/index.ts
[warning] 40-40:
'loot-core/types/util' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing fromloot-core/*
[warning] 40-40:
loot-core/types/utilimport should occur before import of../../platform/server/fs
[warning] 40-40:
'WithRequired' is defined but never used. Allowed unused vars must match /^(_|React)/upackages/loot-core/src/types/models/category-group.d.ts
[warning] 2-2:
'loot-core/types/util' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing fromloot-core/*
[warning] 2-2:
loot-core/types/utilimport should occur before import of./category
🔇 Additional comments (28)
packages/loot-core/migrations/1726730827000_sub_category_groups.sql (2)
1-5: LGTM: Proper transaction handlingThe migration is correctly wrapped in a transaction, ensuring atomicity of the changes. This is a good practice for database migrations.
1-5: Verify impact on existing data and application logicWhile the migration looks correct, it's important to consider its impact:
- Existing data: The new column will be null for all existing rows. Ensure the application handles this correctly.
- Indexes: Depending on how frequently this column will be queried, consider adding an index to improve query performance.
- Application logic: Verify that all parts of the application that interact with category groups are updated to handle the new hierarchical structure.
To help with verification, you can run the following script:
packages/loot-core/src/types/models/category-group.d.ts (2)
10-10: LGTM: Addition of parent_id enables subcategories.The new optional
parent_idproperty in theNewCategoryGroupEntityinterface is a good addition. It enables the creation of hierarchical relationships between category groups, which is essential for implementing subcategories as per the PR objective. The optional nature of this property ensures backward compatibility with existing code.
1-18: Summary: Solid foundation for subcategory implementation.The changes to this file provide an excellent foundation for implementing subcategories in the category group structure. The additions of
parent_idandchildrenproperties allow for creating hierarchical relationships while maintaining backward compatibility. These modifications align well with the PR objective of implementing subcategories.A few minor adjustments have been suggested:
- Updating the import statement to use relative imports and correct ordering.
- Adding a clarifying comment for the
WithRequiredtype usage.Overall, these changes significantly enhance the type definitions to support the new subcategory feature.
🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
There should be at least one empty line between import groups
[warning] 2-2:
'loot-core/types/util' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing fromloot-core/*
[warning] 2-2:
loot-core/types/utilimport should occur before import of./categorypackages/desktop-client/src/components/budget/IncomeHeader.tsx (2)
11-11: LGTM: Enhanced flexibility for category group creationThe updated
onShowNewGroupprop type now allows for an optionalparentparameter. This change aligns with the PR's objective of implementing subcategories and provides more flexibility in creating nested category groups.This modification is backwards compatible and enhances the component's capabilities without breaking existing usage.
27-30: LGTM: Consistent button behavior with updated prop typeThe
onPresshandler has been updated to callonShowNewGroupwithout arguments, which is consistent with the new prop type that includes an optionalparentparameter. This change maintains the previous behavior while allowing for future extensibility.packages/desktop-client/src/components/budget/IncomeGroup.tsx (1)
27-27: Summary: Good implementation of new group creation capability.The changes in this file successfully introduce the ability to handle new group creation, including potential subgroups. This aligns well with the PR's objective of implementing subcategories. The implementation is clean, consistent with the existing code style, and maintains type safety.
Key points:
- New
onShowNewGroupoptional property added toIncomeGroupProps.onShowNewGroupproperly integrated into theIncomeGroupcomponent.- The new property is correctly passed down to the
SidebarGroupcomponent.These changes lay a good foundation for the subcategories feature. As development progresses, ensure that the
SidebarGroupcomponent and any other relevant components are updated to fully utilize this new capability.Also applies to: 39-39, 61-61
packages/desktop-client/src/components/budget/ExpenseCategory.tsx (1)
38-38: LGTM: Addition ofdepthproperty enhances component flexibility.The introduction of the optional
depthproperty toExpenseCategoryPropsis a good addition. It suggests that the component now supports hierarchical structures or nested categories, which aligns with the PR objective of implementing subcategories. The optional nature ensures backward compatibility with existing usage.packages/desktop-client/src/components/budget/ExpenseGroup.tsx (3)
36-37: LGTM: New props enhance ExpenseGroup flexibility.The addition of
onShowNewGroupanddepthtoExpenseGroupPropsincreases the component's flexibility. These optional props allow for better handling of nested categories and improved UI interactions, as mentioned in the PR objectives.
54-55: LGTM: Function signature updated correctly.The
ExpenseGroupfunction signature has been properly updated to include the newonShowNewGroupanddepthprops, maintaining consistency with theExpenseGroupPropstype definition.
135-136: LGTM: SidebarGroup updated with new props.The
SidebarGroupcomponent now receives theonShowNewGroupanddepthprops, ensuring consistent handling of subcategories throughout the component hierarchy. This change aligns with the overall modifications made to theExpenseGroupcomponent.packages/desktop-client/src/components/budget/SidebarCategory.tsx (2)
33-33: LGTM: Addition ofdepthproperty enhances flexibility.The optional
depthproperty allows for representing hierarchical structures in categories, which is consistent with the PR's objective of implementing subcategories. The optional nature maintains backward compatibility.
49-49: LGTM: Destructuringdepthprop in component signature.The addition of
depthto the destructured props in theSidebarCategoryfunction signature is consistent with the type definition and allows for its use within the component.packages/desktop-client/src/components/budget/SidebarGroup.tsx (5)
36-39: LGTM! New props enhance component flexibility.The addition of
onShowNewGroupanddepthprops improves the component's reusability and allows for more dynamic rendering based on the group's hierarchy level.
53-56: LGTM! Props destructuring updated correctly.The component's props destructuring has been properly updated to include the new
onShowNewGroupanddepthprops, ensuring they are available for use within the component.
126-127: LGTM! New menu item for adding a group.The addition of the "Add group" menu item and its corresponding action enhance the component's functionality. The use of the
tfunction for translation is consistent with the rest of the code.Also applies to: 137-137
201-201: LGTM! Improved data preservation inonSavecalls.The use of the spread operator (
...group) in theonSavefunction calls is an important improvement. It ensures that all existing properties of the group object are preserved when updating, preventing unintended data loss.Also applies to: 204-204
Line range hint
1-218: Overall, the changes align well with the PR objectives.The modifications to the
SidebarGroupcomponent successfully introduce support for subcategories and enhance the component's flexibility. The newdepthprop allows for visual representation of the category hierarchy, while theonShowNewGroupfunctionality enables the creation of nested groups. These changes form a solid foundation for the proof of concept for subcategories mentioned in the PR objectives.packages/loot-core/src/types/server-handlers.d.ts (2)
89-90: LGTM: New method for retrieving category groupsThe addition of the
'get-category-groups'method is a good implementation to support the subcategories feature. It follows the existing pattern of other retrieval methods in the interface and uses appropriate TypeScript types.
89-99: Summary: Good implementation of subcategories supportThe changes made to the
ServerHandlersinterface effectively support the implementation of subcategories. The new'get-category-groups'method and the modifications to'category-group-create'and'category-group-update'methods are well-designed and consistent with existing patterns in the codebase.The use of TypeScript features like optional parameters and
Partial<T>demonstrates good practices for maintaining flexibility and type safety.Consider implementing the minor suggestions for improved documentation and consistency, but overall, these changes are ready for implementation.
packages/desktop-client/src/components/budget/index.tsx (2)
189-191: Improved category existence check logicThe changes to the category existence check logic are well-implemented. By filtering the flat list of categories instead of navigating through a nested structure, the code becomes more straightforward and potentially more efficient. This modification maintains the same functionality while improving readability and possibly performance.
235-235: Enhanced group creation with parent_idThe addition of
group.parent_idto thecreateGroupdispatch is a good step towards implementing subcategories. This change aligns well with the PR objective.However, consider implementing the following improvements:
- Add validation for
group.parent_idto ensure it's a valid group ID.- Implement error handling in case an invalid
parent_idis provided.- Update the relevant action creator and reducer to handle this new parameter correctly.
To ensure the
createGroupaction is properly updated, please run the following script:packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (2)
63-75: LGTM: Improved function signature for better maintainabilityThe refactoring of the
CategoryListfunction to accept a singlepropsobject instead of destructured parameters is a good practice. This change enhances code maintainability and makes it easier to add or remove props in the future without altering the function signature.
93-94: Verify the impact of removing the maxHeight propertyThe removal of the commented-out maxHeight property for non-embedded views is good for code cleanliness. However, please ensure that this removal doesn't negatively impact the layout or functionality, especially for parental categories.
To verify the impact, you can run the following script:
packages/loot-core/src/server/models.ts (1)
81-81: Change to acceptPartial<CategoryGroupEntity>is appropriateChanging the parameter type to
Partial<CategoryGroupEntity>allows thevalidatefunction to handle partial updates, which is suitable for update operations. The existing validation logic withrequiredFieldsensures that all necessary fields are present based on whether it's an update or a creation.packages/loot-core/src/server/budget/base.ts (3)
66-68: Recursive processing of child groups increateCategoryGroupThe addition of recursive calls to
createCategoryGroupfor each child group correctly implements the processing of subcategories.
81-84: Prevent runtime errors by ensuringgroup.categoriesis definedThis issue is similar to the one noted in lines 71-74. Ensure that
group.categoriesis defined to avoid errors when mapping over it in the dependencies.
90-93: Prevent runtime errors by ensuringgroup.categoriesis definedAs with the previous comments, make sure
group.categoriesis initialized to prevent potential errors when usinggroup.categories.map(...).
| @@ -0,0 +1,5 @@ | |||
| BEGIN TRANSACTION; | |||
|
|
|||
| ALTER TABLE category_groups ADD COLUMN parent_id TEXT DEFAULT null; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a foreign key constraint
The new parent_id column is correctly added to support the subcategory feature. However, consider adding a foreign key constraint to ensure referential integrity:
ALTER TABLE category_groups
ADD COLUMN parent_id TEXT DEFAULT null
REFERENCES category_groups(id);This would prevent orphaned subcategories if a parent category is deleted.
| @@ -1,15 +1,18 @@ | |||
| import { CategoryEntity } from './category'; | |||
| import { WithRequired } from 'loot-core/types/util'; | |||
There was a problem hiding this comment.
Adjust import statement to follow project conventions.
The new import statement introduces the WithRequired type, which is used in the CategoryGroupEntity interface. However, there are two issues to address:
- The import from 'loot-core/types/util' is restricted. Please use a relative import instead.
- The import order should be adjusted to follow the project's linting rules.
To resolve these issues, please apply the following changes:
-import { CategoryEntity } from './category';
-import { WithRequired } from 'loot-core/types/util';
+import { WithRequired } from '../util';
+import { CategoryEntity } from './category';This change will use a relative import for WithRequired and correct the import order.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { WithRequired } from 'loot-core/types/util'; | |
| import { WithRequired } from '../util'; | |
| import { CategoryEntity } from './category'; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 2-2:
'loot-core/types/util' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing fromloot-core/*
[warning] 2-2:
loot-core/types/utilimport should occur before import of./category
| @@ -35,6 +35,7 @@ type ExpenseCategoryProps = { | |||
| onBudgetAction: (month: number, action: string, arg: unknown) => void; | |||
| onShowActivity: (id: string, month: string) => void; | |||
| onReorder: OnDropCallback; | |||
| depth?: number; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: depth prop is declared but not utilized in SidebarCategory.
The depth property is included in the SidebarCategory component's props but is not used within the component. To complete the implementation of subcategories, please:
- Utilize the
depthprop for visual hierarchy: Apply indentation or other styling based on thedepthvalue to represent hierarchical relationships. - Update related components: Ensure that parent components calculate and pass the correct
depthvalues toSidebarCategory. - Add unit tests: Verify that the
depthprop correctly influences the component's rendering.
🔗 Analysis chain
Overall impact: Good foundation for subcategories, but implementation incomplete.
The addition of the depth property to ExpenseCategoryProps and its integration into the ExpenseCategory component lays a good foundation for implementing subcategories. These changes align well with the PR objective and are backward-compatible.
However, the actual usage and visual representation of the depth property are not implemented in this file. To complete the feature:
- Implement the logic in
SidebarCategoryto utilize thedepthprop for visual hierarchy (e.g., indentation). - Update the parent components to calculate and pass the correct
depthvalues. - Consider adding unit tests to verify the correct handling of the
depthprop.
To ensure the depth prop is being used correctly, we can check its implementation in the SidebarCategory component:
Also applies to: 55-55, 102-102
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of depth prop in SidebarCategory
# Expected result: We should see how the depth prop is used in the component
rg --type typescript 'export function SidebarCategory' -A 20 packages/desktop-client/src/components/budget/SidebarCategory.tsx
Length of output: 163
Script:
#!/bin/bash
# Description: Check the implementation of depth prop in SidebarCategory without specifying file type
# Expected result: We should see how the depth prop is used in the SidebarCategory component
rg 'export function SidebarCategory' -A 20 packages/desktop-client/src/components/budget/SidebarCategory.tsx
Length of output: 500
Script:
#!/bin/bash
# Description: Extract the full implementation of SidebarCategory to check usage of depth prop
# Expected result: Full component code to analyze the usage of depth
rg 'export function SidebarCategory' -A 100 packages/desktop-client/src/components/budget/SidebarCategory.tsx
Length of output: 2736
| {/* TODO: this is a temporary hack for parental categories*/} | ||
| <CategoryList | ||
| {...props} | ||
| items={ | ||
| item.group.children?.flatMap( | ||
| g => g.categories?.map(c => ({ ...c, group: g })) ?? [], | ||
| ) ?? [] | ||
| } | ||
| /> |
There was a problem hiding this comment.
Reconsider the nested CategoryList implementation
The addition of a nested CategoryList for rendering child categories is a step towards supporting hierarchical categories. However, there are some concerns:
- The TODO comment suggests this is a temporary solution. Consider creating a separate ticket to track the implementation of a more robust solution for parental categories.
- Recursive rendering can lead to performance issues with deeply nested categories. Consider implementing a depth limit or a more efficient rendering method.
- The current implementation might not handle all edge cases, such as circular references in the category hierarchy.
Consider refactoring this implementation to address these concerns:
- Implement a non-recursive solution using a flat list with indentation based on depth.
- Add proper error handling for potential circular references.
- Optimize performance by limiting the depth of rendered categories or implementing virtualization for large lists.
Would you like assistance in creating a more robust implementation or opening a GitHub issue to track this task?
| return { | ||
| ...rest, | ||
| hidden: rest.hidden ? 1 : 0, | ||
| ...(rest.is_income !== undefined && { | ||
| is_income: rest.is_income ? 1 : 0, | ||
| }), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consistent inclusion of 'is_income' in the returned object
Currently, hidden is always included in the returned object and defaults to 0 when rest.hidden is falsy or undefined:
hidden: rest.hidden ? 1 : 0,In contrast, is_income is conditionally included only when rest.is_income is defined:
...(rest.is_income !== undefined && {
is_income: rest.is_income ? 1 : 0,
}),For consistency and to ensure uniform data structures, consider always including is_income in the returned object, defaulting to 0 when it's undefined, similar to how hidden is handled. This approach avoids potential issues where is_income might unexpectedly be missing from the object, and downstream code can reliably expect its presence.
| import { sendMessages, batchMessages } from '../sync'; | ||
|
|
||
| import { shoveSortOrders, SORT_INCREMENT } from './sort'; | ||
| import { WithRequired } from 'loot-core/types/util'; |
There was a problem hiding this comment.
Remove unused import and adjust import path
The import of WithRequired from 'loot-core/types/util' is unused and should be removed. Additionally, according to the project's guidelines, imports within loot-core should use relative paths instead of absolute paths.
Apply this diff to fix the issue:
-import { WithRequired } from 'loot-core/types/util';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { WithRequired } from 'loot-core/types/util'; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 40-40:
'loot-core/types/util' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing fromloot-core/*
[warning] 40-40:
loot-core/types/utilimport should occur before import of../../platform/server/fs
[warning] 40-40:
'WithRequired' is defined but never used. Allowed unused vars must match /^(_|React)/u
| for (const group of mappedGroups) { | ||
| if (group.parent_id) { | ||
| const parent = mappedGroups.find(g => g.id === group.parent_id); | ||
| if (parent) { | ||
| parent.children.push(group); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize parent group lookup with a Map for better performance
Currently, the code uses Array.find to locate the parent group for each group with a parent_id. This approach has a time complexity of O(n²) when dealing with a large number of groups. To improve performance, consider using a Map to store groups by their id for constant-time lookups.
Here is how you can implement this change:
+ const groupMap = new Map(mappedGroups.map(group => [group.id, group]));
for (const group of mappedGroups) {
if (group.parent_id) {
- const parent = mappedGroups.find(g => g.id === group.parent_id);
+ const parent = groupMap.get(group.parent_id);
if (parent) {
parent.children.push(group);
}
}
}📝 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.
| for (const group of mappedGroups) { | |
| if (group.parent_id) { | |
| const parent = mappedGroups.find(g => g.id === group.parent_id); | |
| if (parent) { | |
| parent.children.push(group); | |
| } | |
| } | |
| } | |
| const groupMap = new Map(mappedGroups.map(group => [group.id, group])); | |
| for (const group of mappedGroups) { | |
| if (group.parent_id) { | |
| const parent = groupMap.get(group.parent_id); | |
| if (parent) { | |
| parent.children.push(group); | |
| } | |
| } | |
| } |
| export function updateCategoryGroup(group: Partial<CategoryGroupEntity>) { | ||
| const validated = categoryGroupModel.validate(group, { update: true }); | ||
| return update('category_groups', validated); |
There was a problem hiding this comment.
Ensure 'parent_id' is validated during update
In the updateCategoryGroup function, the parent_id is not explicitly validated. To prevent invalid references or circular dependencies, add validation to check that the parent_id, if provided, refers to an existing category group and is not the same as the group's own id.
Modify the function to include validation:
export function updateCategoryGroup(group: Partial<CategoryGroupEntity>) {
const validated = categoryGroupModel.validate(group, { update: true });
+ if (validated.parent_id) {
+ if (validated.parent_id === validated.id) {
+ throw new Error('A category group cannot be its own parent.');
+ }
+ const parentExists = firstSync(
+ 'SELECT id FROM category_groups WHERE id = ? AND tombstone = 0',
+ [validated.parent_id]
+ );
+ if (!parentExists) {
+ throw new Error('Parent category group does not exist.');
+ }
+ }
return update('category_groups', validated);
}📝 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 function updateCategoryGroup(group: Partial<CategoryGroupEntity>) { | |
| const validated = categoryGroupModel.validate(group, { update: true }); | |
| return update('category_groups', validated); | |
| export function updateCategoryGroup(group: Partial<CategoryGroupEntity>) { | |
| const validated = categoryGroupModel.validate(group, { update: true }); | |
| if (validated.parent_id) { | |
| if (validated.parent_id === validated.id) { | |
| throw new Error('A category group cannot be its own parent.'); | |
| } | |
| const parentExists = firstSync( | |
| 'SELECT id FROM category_groups WHERE id = ? AND tombstone = 0', | |
| [validated.parent_id] | |
| ); | |
| if (!parentExists) { | |
| throw new Error('Parent category group does not exist.'); | |
| } | |
| } | |
| return update('category_groups', validated); | |
| } |
| const children = await all( | ||
| 'SELECT * FROM category_groups WHERE parent_id = ?', | ||
| [group.id], | ||
| ); | ||
|
|
||
| // Delete all the children | ||
| await Promise.all( | ||
| children.map(child => deleteCategoryGroup(child, transferId)), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Prevent potential infinite recursion in 'deleteCategoryGroup'
The deleteCategoryGroup function recursively deletes child category groups. Without safeguards, this could lead to infinite recursion if there's a circular parent-child relationship. Consider adding a mechanism to track visited groups or limit recursion depth.
You could implement a visited set to avoid revisiting groups:
export async function deleteCategoryGroup(group, transferId?: string, visited = new Set()) {
+ if (visited.has(group.id)) {
+ throw new Error('Circular category group reference detected.');
+ }
+ visited.add(group.id);
const children = await all(
'SELECT * FROM category_groups WHERE parent_id = ?',
[group.id],
);
// Delete all the children
await Promise.all(
children.map(child => deleteCategoryGroup(child, transferId, visited)),
);
// Proceed with deleting the group
// ...
}Committable suggestion was skipped due to low confidence.
| export async function insertCategoryGroup(group: { | ||
| id?: string; | ||
| name: string; | ||
| is_income?: boolean; | ||
| parent_id?: string; | ||
| }) { |
There was a problem hiding this comment.
Add validation for 'parent_id' to ensure data integrity
The parent_id in the insertCategoryGroup function is accepted without validation. To maintain data integrity, add validation to ensure that the parent_id, if provided, refers to an existing category group and is not the same as the group's own id.
You can enhance the validation as follows:
const validated = {
...categoryGroupModel.validate(group),
sort_order,
};
+ if (validated.parent_id) {
+ if (validated.parent_id === validated.id) {
+ throw new Error('A category group cannot be its own parent.');
+ }
+ const parentExists = await first(
+ 'SELECT id FROM category_groups WHERE id = ? AND tombstone = 0',
+ [validated.parent_id]
+ );
+ if (!parentExists) {
+ throw new Error('Parent category group does not exist.');
+ }
+ }
return insertWithUUID('category_groups', validated);Committable suggestion was skipped due to low confidence.
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
This is a proof of concept for #1320 it far from perfect but introduces the backend and possible ui. Would appreciate any feedback!