Add YTD and last year to Reports headers #4019
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 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
|
|
The end date selector doesn't get a date added in when using the new buttons. That will need fixed. |
Done |
rodriguestiago0
changed the title
WalkthroughThe pull request introduces enhancements to date range management across multiple report components in the desktop client. The changes primarily focus on adding two new time frame modes: 'lastYear' and 'yearToDate' to the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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: 3
🧹 Nitpick comments (3)
packages/desktop-client/src/components/reports/Header.tsx (1)
34-35: Props type declarations need documentation.The newly added props lack TypeScript documentation explaining their purpose and expected format.
Add JSDoc comments to document the props:
type HeaderProps = { + /** ISO date string of the earliest transaction in the system */ earliestTransaction: string; + /** Index of the first day of the week (0-6, where 0 is Sunday) */ firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx'];packages/desktop-client/src/components/reports/reports/NetWorth.tsx (1)
166-168: Remove unused state setter and improve variable naming.
- The state setter
_is unused and can be removed- The variable name
_firstDayOfWeekIdxis unclearApply these improvements:
-const [earliestTransaction, _] = useState(''); +const [earliestTransaction] = useState(''); -const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); -const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; +const [syncedFirstDayOfWeek] = useSyncedPref('firstDayOfWeekIdx'); +const firstDayOfWeekIdx = syncedFirstDayOfWeek || '0';packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
Line range hint
151-153: Consider creating a custom hook for transaction date managementThe pattern of managing
earliestTransactionandfirstDayOfWeekIdxis repeated across components. Consider extracting this logic into a custom hook for better code reuse.Example implementation:
// hooks/useTransactionDateManagement.ts export function useTransactionDateManagement() { const [earliestTransaction, setEarliestTransaction] = useState(''); const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; useEffect(() => { async function fetchEarliestTransaction() { try { const trans = await send('get-earliest-transaction'); if (trans) { setEarliestTransaction(trans.date); } } catch (error) { console.error('Error fetching earliest transaction:', error); } } fetchEarliestTransaction(); }, []); return { earliestTransaction, firstDayOfWeekIdx }; }Also applies to: 463-464
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4019.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/reports/Header.tsx(6 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/CashFlow.tsx(3 hunks)packages/desktop-client/src/components/reports/reports/NetWorth.tsx(3 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
Learnt from: lelemm
PR: actualbudget/actual#3792
File: packages/desktop-client/src/components/reports/reports/Summary.tsx:134-161
Timestamp: 2024-11-12T19:52:52.889Z
Learning: In `packages/desktop-client/src/components/reports/reports/Summary.tsx`, API calls like `get-earliest-transaction` are used without explicit error handling to maintain consistency with other components.
🔇 Additional comments (5)
packages/desktop-client/src/components/reports/Header.tsx (1)
173-206: Verify error handling in the new date range buttons.
The implementation assumes getLiveRange always returns valid dates, but there's no error handling if it fails.
Also, consider adding error boundaries to handle potential failures gracefully.
Add error handling:
<Button
variant="bare"
onPress={() =>
- onChangeDates(
- ...convertToMonth(
- ...getLiveRange(
- 'Year to date',
- earliestTransaction,
- true,
- firstDayOfWeekIdx,
- ),
- ),
- )
+ try {
+ const range = getLiveRange(
+ 'Year to date',
+ earliestTransaction,
+ true,
+ firstDayOfWeekIdx,
+ );
+ onChangeDates(...convertToMonth(...range));
+ } catch (error) {
+ console.error('Failed to get date range:', error);
+ // Handle error appropriately
+ }
}
>packages/desktop-client/src/components/reports/reports/CashFlow.tsx (2)
191-193: Same issues as in NetWorth component.
The implementation has the same issues regarding unused state setter and unclear variable naming.
Please apply the same improvements suggested for the NetWorth component:
- Remove unused state setter
- Improve variable naming clarity
232-233: 🛠️ Refactor suggestion
Consider initializing earliestTransaction.
The earliestTransaction is passed to Header while being initialized as an empty string, which might cause issues with the new date range buttons.
Consider initializing it with a proper value:
-const [earliestTransaction, _] = useState('');
+const [earliestTransaction] = useState(() => {
+ // Initialize with current date or fetch from API
+ return new Date().toISOString().split('T')[0];
+});packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
281-282: LGTM: Props correctly passed to Header component
The new props earliestTransaction and firstDayOfWeekIdx are properly passed to the Header component.
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
497-498: LGTM: Props correctly passed to Header component
The new props earliestTransaction and firstDayOfWeekIdx are properly passed to the Header component, maintaining consistency with other components.
|
/update-vrt |
|
/update-vrt |
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-cash-flow-graph-and-checks-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-cash-flow-graph-and-checks-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-cash-flow-graph-and-checks-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-graph-and-checks-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-graph-and-checks-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-graph-and-checks-visuals-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
packages/desktop-client/src/components/reports/Header.tsx(6 hunks)packages/desktop-client/src/components/reports/reports/NetWorth.tsx(3 hunks)
🔇 Additional comments (6)
packages/desktop-client/src/components/reports/Header.tsx (4)
9-9: LGTM! Import statements are correctly added.The new imports for
SyncedPrefstype andgetLiveRangefunction are necessary for implementing the YTD and last year functionality.Also applies to: 19-19
34-35: LGTM! Props are well-typed and properly defined.The new props
earliestTransactionand optionalfirstDayOfWeekIdxare correctly typed and follow TypeScript best practices.
72-78: Consider adding input validation toconvertToMonth.The function assumes valid date strings but doesn't handle invalid inputs.
173-206: LGTM! New buttons are well-implemented.The "Year to date" and "Last year" buttons are properly implemented with:
- Consistent styling using the "bare" variant
- Proper internationalization using the translation function
- Correct prop passing for date calculations
However, let's verify the date calculations are working as expected.
Run this script to verify the getLiveRange implementation:
✅ Verification successful
✓ Date calculations are correctly implemented
The getLiveRange implementation properly handles both "Year to date" and "Last year" calculations:
- YTD: Correctly spans from the start of current year to today
- Last year: Properly covers the full previous year (Jan 1st to Dec 31st)
- Both respect the earliest transaction date constraint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify getLiveRange implementation for YTD and last year calculations # Search for getLiveRange implementation and its tests ast-grep --pattern 'function getLiveRange($_) { $$$ }' # Search for related test cases rg -A 5 'test.*getLiveRange|describe.*getLiveRange'Length of output: 112
Script:
#!/bin/bash # Try different patterns to find getLiveRange implementation ast-grep --pattern 'const getLiveRange = $_' ast-grep --pattern 'export const getLiveRange = $_' ast-grep --pattern 'export function getLiveRange($_)' # Search for files that might contain the implementation fd -t f -e ts -e tsx getLiveRange # Broader text-based search rg -l "getLiveRange" # Search for date-related utility files that might contain the implementation fd -t f "date" -e ts -e tsxLength of output: 7233
Script:
#!/bin/bash # Get the implementation cat packages/desktop-client/src/components/reports/getLiveRange.ts # Check usage patterns in other files rg -A 3 "getLiveRange\(" packages/desktop-client/src/components/reports/reports/CustomReport.tsx packages/desktop-client/src/components/reports/GetCardData.tsx packages/desktop-client/src/components/reports/ReportSidebar.tsxLength of output: 2983
packages/desktop-client/src/components/reports/reports/NetWorth.tsx (2)
18-18: LGTM! Import statement is correctly added.The import for
useSyncedPrefhook follows project conventions.
205-206: LGTM! Props are correctly passed to Header component.The new props
earliestTransactionandfirstDayOfWeekIdxare properly passed with their respective state values.
|
Can you add a dummy commit to get the checks to run? |
|
When I save a "last year" range in the reports, the range doesn't get saved right. The months in the start/end selection boxes are correct, but get shifted when going back to the reports dashboard and when viewing the report again after.
|
rodriguestiago0
force-pushed
the
add_ytd_and_last_year
branch
from
fd90221 to
d50f048
Compare
rodriguestiago0
force-pushed
the
add_ytd_and_last_year
branch
from
d50f048 to
239e180
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/desktop-client/src/components/reports/reportRanges.ts (1)
199-209: Ensure consistent date validation across all modes.The new 'lastYear' and 'yearToDate' modes bypass the validation logic present in
validateRange,validateStart, andvalidateEndfunctions. This could lead to inconsistent date handling.Consider:
- Adding the new modes to
boundedRangefunction's interval handling- Ensuring the new modes work correctly with the validation functions
- Adding unit tests for the new modes with edge cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-client/src/components/reports/Header.tsx(6 hunks)packages/desktop-client/src/components/reports/reportRanges.ts(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/reports/reports/Summary.tsx
- packages/desktop-client/src/components/reports/Header.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
Done |
| @@ -4,7 +4,7 @@ import { type RuleConditionEntity } from './rule'; | |||
| export type TimeFrame = { | |||
| start: string; | |||
| end: string; | |||
| mode: 'sliding-window' | 'static' | 'full'; | |||
| mode: 'sliding-window' | 'static' | 'full' | 'lastYear' | 'yearToDate'; | |||
There was a problem hiding this comment.
Is this necessary? The existing mode types are for how the window functions. Plus the existing window options aren't in this list, so it seems like you shouldn't need to change this.
There was a problem hiding this comment.
The problem is. Sliding window will be always against current month.
With ytd and last year is not against current month.
In my opinion we could migrate it to other structure to be more productive but we can’t because they are already being used.
we should get the interval size, interval type and anchor.
3 month
Would be 3, month, current month
or
Last year would be
12, month, begin of current year
Two years
2, years, current month
|
There's a small width range where it doesn't wrap properly on my screen, all time is very slightly clipped into the save button 
|
|
/update-vrt |
| @@ -129,7 +143,7 @@ export function Header({ | |||
| </SpaceBetween> | |||
| </SpaceBetween> | |||
|
|
|||
| <SpaceBetween> | |||
| <SpaceBetween gap={3}> | |||
There was a problem hiding this comment.
Could we up this? I think it looks a bit squished.
Current:
Here's 10px
There was a problem hiding this comment.
small screens would have a problem.
There was a problem hiding this comment.
is there any way to force it to wrap when the save button starts to overlap with the filter?
There was a problem hiding this comment.
No idea. my knowledge of CSS is less than js
There was a problem hiding this comment.
No worries, we'll go with it for now then. If anyone works out a way it can be a separate PR
Add "Year to date" and "Last year" to header report