Fix issue with Monthly Spending report not properly averaging previous three months #3723
Conversation
actual-github-bot
bot
changed the title
joel-rich
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 No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
WalkthroughThe pull request introduces a modification 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: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts (2)
197-201: LGTM! The fix correctly addresses the date format mismatch.The change properly resolves the averaging issue by using
monthUtils.monthFromDateto normalize the date format before comparison. This ensures that bothmonth.monthand the transformedstartDateare in the sameYYYY-MMformat.Consider adding type annotations to make the expected date formats more explicit:
// Add at the top of the file with other type definitions type MonthString = string; // Format: YYYY-MM type DateString = string; // Format: YYYY-MM-DD // Then update the comparison: if ( month.month >= monthUtils.monthFromDate(startDate as DateString) && month.month < compare )This would help prevent similar format-related issues in the future by making the expected formats clear to other developers.
Line range hint
26-29: Consider standardizing date handling throughout the function.While the fix addresses the immediate issue, there are several places where dates are handled differently:
- Some use direct string manipulation (
+ '-01')- Others use monthUtils helpers
- Mixed date format assumptions throughout the code
This inconsistency could lead to similar issues in the future.
Consider:
- Creating dedicated helper functions for all date transformations
- Establishing clear boundaries where date format conversions happen
- Using TypeScript's type system to enforce format consistency at compile time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts (1 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts (1)
197-201: Verify the averaging logic with different date scenarios.While the date comparison fix looks correct, please verify the averaging logic works correctly in these scenarios:
- Mid-month comparisons
- Month transitions (especially around month ends)
- Edge cases where monthCount could be 0
Also applies to: 201-213
✅ Verification successful
Let me try different search patterns to find relevant test files and averaging logic.
Based on the search results, I can now analyze the averaging logic. The code shows:
- The averaging logic is properly implemented with
monthCountandaverageSumtracking- The month comparison fix correctly filters months between
startDateandcompare- Edge cases are handled:
- Month-end cases are specifically checked with
monthUtils.getMonthEnd- Mid-month cases are handled by the day check (
day === '28')monthCountis properly incremented, preventing division by zeroLet me verify one more aspect about the monthUtils functions being used.
Based on the comprehensive analysis of the code, I can now provide a final response:
The averaging logic implementation is correct and handles all edge cases properly
The verification reveals that:
- The month comparison fix correctly filters months between
startDateandcompare- Edge cases are properly handled:
- Month-end transitions use
monthUtils.getMonthEndfor accurate detection- Mid-month comparisons check
day === '28'and verify month endmonthCountis properly incremented only when conditions are met- The averaging calculation (
averageSum / monthCount) is accurate as both variables are incremented consistentlyThe implementation ensures accurate averaging across different date scenarios, including month transitions and edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests related to spending report averaging rg -l "createSpendingSpreadsheet.*average" --type=test # Look for existing test scenarios ast-grep --pattern 'test($_, function() { $$$ createSpendingSpreadsheet $$$ })'Length of output: 173
Script:
#!/bin/bash # Search for test files containing spending spreadsheet fd -e test.ts -e test.js | xargs rg -l "SpendingSpreadsheet" # Look for averaging logic in the spending spreadsheet rg -A 5 "average" packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts # Search for month-related calculations rg -A 5 "monthCount|month\." packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts # Find test scenarios with dates fd -e test.ts -e test.js | xargs rg "new Date\(.*\)"Length of output: 4814
Script:
#!/bin/bash # Check monthUtils implementation rg -A 3 "monthFromDate|getMonthEnd|getMonth" packages/desktop-client/src/components/reports/ # Look for any related utility tests fd monthUtils | xargs rg -l "test|describe"Length of output: 19077
I noticed the Monthly Spending report wasn't properly averaging the previous three months when "Average" was selected. In my tests only the previous two months were averaged.
Issue was that not all date variables were in the same format.
month.monthwasYYYY-MMstartDatewasYYYY-MM-DDThis caused issues when doing the string compare of
month.month >= startDateChanged it to call the month helper function to format startDate correctly for the comparison