Add Copy last 6/12 months to budget menu #4096
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 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 |
psybers
changed the title
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces enhanced budget management functionality across multiple components and files. The changes primarily focus on adding new options for setting 6-month and 12-month budget averages. The modifications span the desktop client's user interface components, core client actions, server-side budget actions, and type definitions. Specifically, new cases are added to the 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: 2
🧹 Nitpick comments (4)
packages/loot-core/src/server/budget/types/handlers.d.ts (1)
16-19: Consider consolidating methods with'budget/set-n-month-avg'.Since there is already a
'budget/set-n-month-avg'method, these dedicated six- and twelve-month methods might lead to duplication. It is perfectly valid to keep these as convenience methods if they are heavily used, but you could alternatively route their logic through'budget/set-n-month-avg'to streamline maintenance and testing.packages/loot-core/src/server/budget/app.ts (1)
23-24: Methods look good, but consider possible redundancy.Both methods are straightforward wrappers around
setNMonthAvg. As noted elsewhere, unifying them under'budget/set-n-month-avg'with different arguments may reduce potential duplication, but this is purely a design preference.packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetMonthMenu.tsx (1)
68-75: Optional textual or grouping improvement.The text for "Set budgets to 6 month average" and "Set budgets to 12 month average" is clear. For stylistic consistency, consider using "6-month" and "12-month" with a hyphen, as already done with "3 month." This is a minor UI/UX improvement, but not mandatory.
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetMonthMenu.tsx (1)
73-80: Minor consistency suggestion.The labels "Set budgets to 6 month average" and "Set budgets to 12 month average" are clear. You could align the grammar with the existing "Set budgets to 3 month average," perhaps by consistently adding a hyphen ("6-month" or "12-month"), but this is fully optional and not a functional issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4096.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetMonthMenu.tsx(2 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetMonthMenu.tsx(2 hunks)packages/loot-core/src/client/actions/queries.ts(1 hunks)packages/loot-core/src/server/budget/actions.ts(1 hunks)packages/loot-core/src/server/budget/app.ts(1 hunks)packages/loot-core/src/server/budget/types/handlers.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetMonthMenu.tsx (1)
44-49: No issues found.The new menu options for 6 and 12 months integrate cleanly into the existing switch block. The logic to call
onSetMonthsAverageis consistent with the approach for other menu items.packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetMonthMenu.tsx (1)
46-51: Implementation mirrors the tracking menu, which is good for consistency.The switch case additions for 6- and 12-month ranges are correct, and calling
onSetMonthsAverageis consistent with the existing code. This ensures consistent behavior between different budget menu components.packages/loot-core/src/client/actions/queries.ts (2)
32-34: The new'set-6-avg'case aligns well with existing budget actions.
No issues found with this implementation, and it remains consistent with the established pattern of callingawait send(...)for budget updates.
35-37: The new'set-12-avg'case follows the same pattern successfully.
All logic appears coherent, with consistent usage ofawait send(...).
| export async function set12MonthAvg({ | ||
| month, | ||
| }: { | ||
| month: string; | ||
| }): Promise<void> { | ||
| const categories = await db.all( | ||
| 'SELECT * FROM v_categories WHERE tombstone = 0', | ||
| ); | ||
|
|
||
| await batchMessages(async () => { | ||
| for (const cat of categories) { | ||
| if (cat.is_income === 1 && !isReflectBudget()) { | ||
| continue; | ||
| } | ||
| setNMonthAvg({ month, N: 12, category: cat.id }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await the async calls within the loop to prevent premature return.
Inside the for loop, setNMonthAvg is an async function, and currently it's not awaited. This means the parent function may resolve before all asynchronous operations have completed, potentially causing unexpected delays or race conditions.
Below is an essential fix to ensure each async operation completes:
- setNMonthAvg({ month, N: 12, category: cat.id });
+ await setNMonthAvg({ month, N: 12, category: cat.id });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function set12MonthAvg({ | |
| month, | |
| }: { | |
| month: string; | |
| }): Promise<void> { | |
| const categories = await db.all( | |
| 'SELECT * FROM v_categories WHERE tombstone = 0', | |
| ); | |
| await batchMessages(async () => { | |
| for (const cat of categories) { | |
| if (cat.is_income === 1 && !isReflectBudget()) { | |
| continue; | |
| } | |
| setNMonthAvg({ month, N: 12, category: cat.id }); | |
| } | |
| }); | |
| } | |
| export async function set12MonthAvg({ | |
| month, | |
| }: { | |
| month: string; | |
| }): Promise<void> { | |
| const categories = await db.all( | |
| 'SELECT * FROM v_categories WHERE tombstone = 0', | |
| ); | |
| await batchMessages(async () => { | |
| for (const cat of categories) { | |
| if (cat.is_income === 1 && !isReflectBudget()) { | |
| continue; | |
| } | |
| await setNMonthAvg({ month, N: 12, category: cat.id }); | |
| } | |
| }); | |
| } |
| export async function set6MonthAvg({ | ||
| month, | ||
| }: { | ||
| month: string; | ||
| }): Promise<void> { | ||
| const categories = await db.all( | ||
| 'SELECT * FROM v_categories WHERE tombstone = 0', | ||
| ); | ||
|
|
||
| await batchMessages(async () => { | ||
| for (const cat of categories) { | ||
| if (cat.is_income === 1 && !isReflectBudget()) { | ||
| continue; | ||
| } | ||
| setNMonthAvg({ month, N: 6, category: cat.id }); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await the async calls for setting 6-month averages.
Similar to the 12-month average function, ensure that the call to setNMonthAvg is awaited within the loop:
- setNMonthAvg({ month, N: 6, category: cat.id });
+ await setNMonthAvg({ month, N: 6, category: cat.id });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function set6MonthAvg({ | |
| month, | |
| }: { | |
| month: string; | |
| }): Promise<void> { | |
| const categories = await db.all( | |
| 'SELECT * FROM v_categories WHERE tombstone = 0', | |
| ); | |
| await batchMessages(async () => { | |
| for (const cat of categories) { | |
| if (cat.is_income === 1 && !isReflectBudget()) { | |
| continue; | |
| } | |
| setNMonthAvg({ month, N: 6, category: cat.id }); | |
| } | |
| }); | |
| } | |
| export async function set6MonthAvg({ | |
| month, | |
| }: { | |
| month: string; | |
| }): Promise<void> { | |
| const categories = await db.all( | |
| 'SELECT * FROM v_categories WHERE tombstone = 0', | |
| ); | |
| await batchMessages(async () => { | |
| for (const cat of categories) { | |
| if (cat.is_income === 1 && !isReflectBudget()) { | |
| continue; | |
| } | |
| await setNMonthAvg({ month, N: 6, category: cat.id }); | |
| } | |
| }); | |
| } |
|
Should we do this or should we point people to the average template? With templates enabled that menu will be pretty long. |
|
Not everyone (such as me) uses goal templates? |
|
Based on the discussion in Discord, I had these added actions (and the existing ones) make use of the |
|
I also added translations to one of the files since they were missing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx (1)
171-176: Ensure consistent placeholder usage in translations.
You are mixing placeholder-based translations (e.g.,{{displayMonth}}) with template literals (e.g.,`${displayMonth} budgets...`). While both approaches work, consistency helps translators handle the strings more reliably.Below is an example refactor for lines 191-200 that aligns with the placeholder style used in lines 171-186:
- message: - numberOfMonths === 12 - ? t( - `${displayMonth} budgets have all been set to yearly average.`, - ) - : t( - `${displayMonth} budgets have all been set to ${numberOfMonths} month average.`, - ), + message: numberOfMonths === 12 + ? t('{{displayMonth}} budgets have all been set to yearly average.', { + displayMonth, + }) + : t('{{displayMonth}} budgets have all been set to {{months}} month average.', { + displayMonth, + months: numberOfMonths, + }),Similarly, consider using placeholders in other lines to keep the code uniform and translator-friendly.
Also applies to: 181-186, 191-200, 209-214, 219-224, 229-234
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx (2)
54-54: Watch for typographic quotes
The line'MMMM ‘yy'contains a curly single quote “‘”. Confirm that this character is intentional and correctly interpreted by themonthUtils.formatfunction. If accidental, replace it with a standard apostrophe'.-const displayMonth = monthUtils.format(month, 'MMMM ‘yy'); +const displayMonth = monthUtils.format(month, 'MMMM \'yy');
187-196: Ternary-based message
Using a ternary for 12 vs. other months is valid. If further averaging options are added in the future, consider a more general approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx(7 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (12)
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx (6)
2-2: Great introduction of translation support.
UsinguseTranslationfromreact-i18nextis a solid approach to ensure internationalization in theBudgetSummarycomponent.
8-8: Undo handling is well-integrated.
Importing the customuseUndohook is a neat solution for providing user feedback with undo notifications.
37-37: Confirmation of undo notification usage.
ExtractingshowUndoNotificationfromuseUndo()centralizes the undo logic and keeps the component concise. Good practice.
54-54: Well-placed translation extraction.
Usingtfromreact-i18nextright before rendering ensures straightforward localization.
98-102: Localized aria-label expressions.
Good job on localizing the expand/collapse aria-label. Improves accessibility for non-English speakers.
152-152: Localized menu label.
Replacing the hardcoded "Menu" string witht('Menu')is consistent with the rest of the component's localization approach.packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetSummary.tsx (6)
9-9: Import usage looks good
The import of theuseUndohook appears correct and aligns with its usage below. No issues spotted here.
167-172: Undo notification message
The displayed text is clear. No further issues found.
177-182: Undo notification for zero budgets
This notification mirrors the pattern in the previous block, ensuring consistent user feedback. Looks good.
205-210: Template application notification
The message properly referencesdisplayMonth. Implementation is consistent with other notifications.
215-220: Overwrite template notification
No issues found with logic or messaging.
40-40: Properly extracting showUndoNotification
CallinguseUndo()and extractingshowUndoNotificationis appropriate. Make sure the component remains mounted when triggering notifications to avoid potential React state warnings.✅ Verification successful
Notification System Implementation is Unmount-Safe
TheshowUndoNotificationimplementation uses Redux for state management and operates independently of component lifecycle, making it safe to use even if the component unmounts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that showUndoNotification is unmounted-safe usage. rg -A 5 "showUndoNotification"Length of output: 38025
packages/desktop-client/src/components/budget/envelope/budgetsummary/BudgetSummary.tsx
Show resolved
Hide resolved
Currently, the individual categories have option to copy last 3, 6, or 12 months. But the overall budget only has last 3. This makes them similar.