Undoable auto transfer notes + auto notes for cover #3411
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. |
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
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 No assets were bigger Smaller
Unchanged No assets were unchanged |
joel-jeremy
changed the title
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
undoable-auto-transfer-notes
branch
from
af1da13
to
9e896de
Compare
|
Seems to work well. @Teprifer do you see anything else that needs fixed here? |
I feel bad not picking up on it before. :( There's something a bit weird with the undo, it needs to be done twice if the note is viewed, this isn't the case in a few days old edge. To reproduce:
This stacks:
If not clicking the note in between, then only one ctrl+z is required.
Fairly minor overall, but could be come across as ctrl+z not working in niche cases? |
joel-jeremy
force-pushed
the
undoable-auto-transfer-notes
branch
from
d41c002
to
dba54ac
Compare
|
Cheers for fixing the ctrl+z. I, uh, mis-clicked when testing and found a difference in transfer behaviour to Edge: In Edge if you click a category to assign the funds to another category and select the source category as the destination category nothing happens. In this PR it'll allocate the transfer value again to that category, pulling from To Budget (obviously). This PR: Transfer food to food: Result: |
joel-jeremy
force-pushed
the
undoable-auto-transfer-notes
branch
2 times, most recently
from
d04d5af
to
906b218
Compare
Nice catch! I've hidden the source category in the transfer menu and cover menus. |
|
@joel-jeremy Has that change been pushed to github? Or has the netlify build not updated? The source category is still showing for me. Food (transfer) I checked cover too, but trying to cover a negative with itself doesn't result in any note or change in budget, so this menu option is as expected. |
Thank you for the testing it out! Sorry, I missed an if condition on the new utility method. Should be fixed now. |
|
All good, familiar with the "oh that one other bit". Checks out functionality wise so should be good for a code review I think? |
joel-jeremy
force-pushed
the
undoable-auto-transfer-notes
branch
from
a872620
to
7a7c74a
Compare
Had to rebase to resolve a conflict. Should be good for code review :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- packages/desktop-client/src/components/Modals.tsx (2 hunks)
- packages/desktop-client/src/components/budget/rollover/BalanceMovementMenu.tsx (3 hunks)
- packages/desktop-client/src/components/budget/rollover/CoverMenu.tsx (2 hunks)
- packages/desktop-client/src/components/budget/rollover/TransferMenu.tsx (3 hunks)
- packages/desktop-client/src/components/budget/util.ts (2 hunks)
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (2 hunks)
- packages/desktop-client/src/components/modals/CoverModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TransferModal.tsx (2 hunks)
- packages/loot-core/src/client/state-types/modals.d.ts (1 hunks)
- packages/loot-core/src/server/budget/actions.ts (5 hunks)
- packages/loot-core/src/server/db/index.ts (3 hunks)
- packages/loot-core/src/server/notes/app.ts (1 hunks)
- upcoming-release-notes/3411.md (1 hunks)
Additional context used
LanguageTool
upcoming-release-notes/3411.md
[uncategorized] ~6-~6: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...emy] --- Undoable auto tranfer notes + auto notes for cover(AUTO_HYPHEN)
Additional comments not posted (37)
packages/loot-core/src/server/notes/app.ts (2)
9-11: LGTM!The
updateNotesfunction is a clear and concise implementation of a database update operation. The use of async/await and object destructuring is appropriate and follows best practices.
13-13: Nice refactoring!Extracting the database update logic into a separate
updateNotesfunction improves code readability and maintainability. Thenotes-savemethod now has a clear and focused responsibility.packages/desktop-client/src/components/budget/rollover/BalanceMovementMenu.tsx (2)
47-47: LGTM!The direct passing of
categoryIdto theTransferMenucomponent looks good. The removal of theuseBudgetTransferNoteshook and its associated functionality seems to be intentional, as per the AI-generated summary.
63-63: LGTM!The direct passing of
categoryIdto theCoverMenucomponent looks good.packages/desktop-client/src/components/budget/rollover/CoverMenu.tsx (4)
3-3: LGTM!The import path update for
CategoryEntitytype is correct and does not affect the component's functionality.
10-10: LGTM!The import statement for the utility functions
addToBeBudgetedGroupandremoveCategoriesFromGroupsis correct and will be used within the component.
14-15: LGTM!The changes to the
CoverMenuPropsare improvements:
- Renaming
categorytocategoryIdenhances clarity and consistency.- Explicitly specifying the parameter type for
onSubmitimproves type safety and readability.
21-21: LGTM!The changes related to
categoryId,fromCategoryId, and theCategoryAutocompletecomponent are well-implemented:
- Using
categoryIdinstead ofcategorymaintains consistency.- Introducing
fromCategoryIdstate allows for better management of the selected category.- Updating
filteredCategoryGroupsto usecategoryIdandfromCategoryIdensures correct recalculation.- Using
fromCategoryIdin thesubmitfunction ensures the correct category is submitted.- Setting
valuetonullinCategoryAutocompletealigns with the expected prop type.- Updating
fromCategoryIdin theonSelectprop ensures the state is correctly updated.Also applies to: 27-27, 29-37, 40-41, 53-53, 55-55
packages/desktop-client/src/components/modals/CoverModal.tsx (4)
5-5: LGTM!The import statement for the
CategoryEntitytype is correct and necessary for using it in the file.
9-12: LGTM!The import statements for the
addToBeBudgetedGroupandremoveCategoriesFromGroupsutility functions are correct and necessary for using them in the file.
28-61: LGTM!The changes to the
CoverModalcomponent are consistent with the AI-generated summary and the changes to theCoverModalPropsinterface. ThecategoryIdprop is used correctly to filter thecategoryGroupsusing theremoveCategoriesFromGroupsutility function, and theonCategoryClickfunction is updated to use the filteredcategoryGroups.
20-23: Verify the impact of the breaking changes in the codebase.The changes to the
CoverModalPropsinterface are breaking changes:
- The
categoryIdprop is now optional, which could affect the usage of theCoverModalcomponent in other parts of the codebase.- The
onSubmitprop now acceptscategoryIdinstead ofcategory, which could affect the usage of theCoverModalcomponent in other parts of the codebase.Run the following script to verify the usage of the
CoverModalcomponent:Verification successful
CoverModal usage is consistent with the updated interface
The verification process has confirmed that the usage of the CoverModal component in the codebase is consistent with the updated interface:
- The
categoryIdprop is used instead ofcategory.- The
onSubmitprop is correctly implemented.- All other props are present and used as expected.
No issues or inconsistencies were found in the usage of the CoverModal component.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `CoverModal` component in the codebase. # Test: Search for the usage of the `CoverModal` component. Expect: No usage of the old `category` prop or the old `onSubmit` prop signature. rg --type typescript -A 5 $'<CoverModal'Length of output: 76
Script:
#!/bin/bash # Description: Verify the usage of the `CoverModal` component in the codebase. # Find TypeScript files and search for CoverModal usage fd -e ts -e tsx | xargs rg -i -A 5 '<CoverModal'Length of output: 575
Script:
#!/bin/bash # Description: Search for the complete CoverModal usage, including the onSubmit prop. # Find the CoverModal usage in Modals.tsx with more context rg -i -A 15 '<CoverModal' packages/desktop-client/src/components/Modals.tsxLength of output: 593
packages/desktop-client/src/components/budget/rollover/TransferMenu.tsx (6)
16-16: LGTM!The addition of the optional
categoryIdprop enhances the component's flexibility by allowing it to filter categories based on a specific ID. The prop is correctly typed and marked as optional, ensuring backward compatibility.
31-41: LGTM!The use of
useMemoto memoize the filtered category groups is a good performance optimization. The filtering logic correctly handles the inclusion/exclusion of categories based on the provided props (categoryId,originalCategoryGroups, andshowToBeBudgeted). This ensures that the component receives a relevant subset of categories, enhancing its efficiency and maintainability.
45-45: LGTM!The renaming of the state variable from
categoryIdtotoCategoryIdimproves code readability and maintainability. It clarifies the purpose of the variable as the target category for the transfer, reducing potential confusion. This change enhances code clarity without introducing any functional changes.
64-64: LGTM!The modification of the
onEntercallback to pass thetoCategoryIdstate variable to the_onSubmitfunction maintains consistency with the renaming of the state variable. This change ensures that the correct target category ID is passed when the enter key is pressed, preserving the expected behavior of the component.
71-74: LGTM!The changes to the
CategoryAutocompletecomponent enhance its usability and flexibility:
- Passing the
filteredCategoryGroupsas thecategoryGroupsprop ensures that the component receives the relevant subset of categories based on the filtering logic.- Setting the
valueprop tonullallows the component to handle its own state internally, promoting a more modular design.- The
onSelectcallback correctly updates thetoCategoryIdstate variable when a category is selected, maintaining the state of the selected target category in the parent component.These modifications improve the component's integration and state management within the
TransferMenu.
19-19: Verify the impact on the parent component.The change in the
onSubmitprop'scategoryIdparameter type fromstringtoCategoryEntity['id']enhances type safety. However, ensure that the parent component or wherever theonSubmitcallback is defined has been updated to match the new type signature.Run the following script to verify the
onSubmitprop usage:packages/desktop-client/src/components/modals/TransferModal.tsx (5)
5-5: LGTM!The import statement for the
CategoryEntitytype is correct and necessary for using it in the file.
9-12: LGTM!The import statements for the
addToBeBudgetedGroupandremoveCategoriesFromGroupsutility functions are correct and necessary for using them in the file.
22-22: LGTM!The addition of the optional
categoryIdprop of typeCategoryEntity['id']to theTransferModalPropstype is correct and allows the component to receive a specific category identifier.
26-26: LGTM!The modification of the
onSubmitprop in theTransferModalPropstype to accept atoCategoryIdparameter of typeCategoryEntity['id']is correct and ensures type consistency.
31-51: LGTM!The modifications to the category group and category filtering logic within the
useMemohook are correct and align with the requirements. The use of thecategoryIdprop to conditionally filter categories provides flexibility in rendering the available categories for transfer. TheuseMemohook is correctly used to memoize the computed values based on the dependencies.packages/desktop-client/src/components/budget/util.ts (1)
38-53: LGTM!The
removeCategoriesFromGroupsfunction is well-implemented and follows best practices:
- It handles edge cases like empty category IDs and groups becoming empty after removal.
- It uses a set for efficient lookup of category IDs.
- It uses modern JavaScript features like the spread operator and optional chaining for concise and readable code.
- The logic is correct and the code is clean and maintainable.
Great job!
packages/loot-core/src/client/state-types/modals.d.ts (2)
228-228: LGTM!The addition of the optional
categoryIdproperty to thetransfermodal object is a good enhancement. It allows associating a specific category with the transfer, enabling better categorization and tracking of financial transfers. The type ofCategoryEntity['id']ensures that thecategoryIdis a valid category ID.
236-236: Looks good!The addition of the optional
categoryIdproperty to thecovermodal object is a nice improvement. It allows associating a specific category with the cover, enabling better categorization and tracking of financial covers. The type ofCategoryEntity['id']ensures that thecategoryIdis a valid category ID.packages/loot-core/src/server/budget/actions.ts (4)
374-387: LGTM!The changes to wrap the budget update and note addition within a
batchMessagescall ensure that both operations are treated as a single transaction, improving data consistency.
419-428: LGTM!The changes to wrap the budget update and note addition within a
batchMessagescall ensure that both operations are treated as a single transaction, improving data consistency.
445-461: LGTM!The changes to wrap the budget updates and note addition within a
batchMessagescall ensure that all operations are treated as a single transaction, improving data consistency.
487-530: Excellent addition of theaddMovementNotesfunction!This function greatly enhances the tracking of budget movements by logging detailed notes for each transfer. It captures the essential information, such as the amount, source and destination categories, and the date of the transaction.
By retrieving the existing notes and appending the new note, it maintains a comprehensive history of budget movements, which will be valuable for auditing and understanding the flow of funds over time.
packages/loot-core/src/server/db/index.ts (3)
285-291: Approve the changes to thegetCategoriesfunction.The updates to the
getCategoriesfunction enhance its flexibility by allowing optional filtering of categories based on their IDs. The use of thetoSqlQueryParametersutility function improves code maintainability and readability by dynamically generating the SQL query parameters.The changes are localized to the function and do not appear to have any negative impact on the rest of the codebase.
293-314: Approve the changes to thegetCategoriesGroupedfunction.The updates to the
getCategoriesGroupedfunction enhance its flexibility by allowing optional filtering of category groups based on their IDs. The use of thetoSqlQueryParametersutility function improves code maintainability and readability by dynamically generating the SQL query parameters.The changes are localized to the function and do not appear to have any negative impact on the rest of the codebase.
698-700: Approve the addition of thetoSqlQueryParametersutility function.The
toSqlQueryParametersfunction is a useful utility that generates a comma-separated string of placeholders (?) for use in SQL queries. It is used in thegetCategoriesandgetCategoriesGroupedfunctions to dynamically generate the SQL query parameters based on the providedidsarray.The function is generic and can be reused in other parts of the codebase where similar functionality is required. It improves code maintainability and readability by abstracting the placeholder generation logic into a separate function.
packages/desktop-client/src/components/Modals.tsx (2)
500-500: LGTM!The addition of the
categoryIdprop to theTransferModalcomponent enhances its functionality by allowing it to work with category-specific data. This change aligns with the AI-generated summary and seems to be a logical enhancement to the modal's capabilities.
513-513: LGTM!The addition of the
categoryIdprop and the removal of thecategoryprop in theCoverModalcomponent streamline the data flow and potentially improve performance. TheCoverModalcan now fetch or interact with category data based on the identifier, rather than relying on the entirecategoryobject. These changes align with the AI-generated summary and seem to be logical enhancements to the modal's implementation.packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (2)
396-396: LGTM!The renaming of the
categoryparameter tocategoryIdin the modal function calls improves code clarity and maintainability. The changes are consistent with the list of alterations provided in the summary and do not alter the overall logic or behavior of the component.Also applies to: 430-430
Line range hint
1-1: No action required.The
BudgetTablefunction itself has not been modified. The changes made in the childExpenseCategorycomponent are minor and do not require any action in this function.
Haha, well, I'm sure a code review would've picked that up - I was more indicating to others as you don't want me pretending I can code review. :) |
* Undo auto transfer notes + auto notes for cover * Release notes * Fix notes * Fix notes undo * Do not show clicked category on transfer or cover menus * Fix typecheck error * typecheck * Fix removeCategoriesFromGroups
* marked files for translation * added release note * fixed linting warnings * 🐛 fix account filters being overridden (#3441) *Reduce package size (#3443) * reduce package size of all packages * release notes * Update beforePackHook.ts * [Maintenance] Cleanup react aria packages and dedupe (#3450) * Cleanup react aria packages and dedupe * Release notes * ♻️ (synced-prefs) moving the prefs from metadata.json to the db (#3423) *
Resolves #3404