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 a series of modifications across multiple report components in the desktop client, focusing on enhancing date-related functionalities. The changes primarily involve adding two new props to the The modifications are consistently applied across several report components, including Calendar, CashFlow, NetWorth, and Summary. In each component, a new state variable 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: 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.
| function convertToMonth( | ||
| start: string, | ||
| end: string, | ||
| mode: TimeFrame['mode'], | ||
| ): [string, string, TimeFrame['mode']] { | ||
| return [monthUtils.getMonth(start), monthUtils.getMonth(end), mode]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation to convertToMonth.
The function assumes valid date strings but doesn't handle invalid inputs.
Add input validation:
function convertToMonth(
start: string,
end: string,
mode: TimeFrame['mode'],
): [string, string, TimeFrame['mode']] {
+ if (!start || !end) {
+ throw new Error('Invalid date strings provided');
+ }
return [monthUtils.getMonth(start), monthUtils.getMonth(end), mode];
}📝 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.
| function convertToMonth( | |
| start: string, | |
| end: string, | |
| mode: TimeFrame['mode'], | |
| ): [string, string, TimeFrame['mode']] { | |
| return [monthUtils.getMonth(start), monthUtils.getMonth(end), mode]; | |
| } | |
| function convertToMonth( | |
| start: string, | |
| end: string, | |
| mode: TimeFrame['mode'], | |
| ): [string, string, TimeFrame['mode']] { | |
| if (!start || !end) { | |
| throw new Error('Invalid date strings provided'); | |
| } | |
| return [monthUtils.getMonth(start), monthUtils.getMonth(end), mode]; | |
| } |
| const [earliestTransaction, _] = useState(''); | ||
| const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); | ||
| const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize earliestTransaction state with data from useEffect
The earliestTransaction state is initialized but never updated, despite having an effect that fetches the earliest transaction. Consider updating the state within the effect.
- const [earliestTransaction, _] = useState('');
+ const [earliestTransaction, setEarliestTransaction] = useState('');
useEffect(() => {
async function run() {
const trans = await send('get-earliest-transaction');
+ if (trans) {
+ setEarliestTransaction(trans.date);
+ }
// ... rest of the effect
}
run();
}, []);Committable suggestion skipped: line range outside the PR's diff.
| const [earliestTransaction, _] = useState(''); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize earliestTransaction state with data from useEffect
The earliestTransaction state is initialized but never updated. The component already has an effect that fetches the earliest transaction, which should be used to update this state.
- const [earliestTransaction, _] = useState('');
+ const [earliestTransaction, setEarliestTransaction] = useState('');
useEffect(() => {
async function run() {
try {
const trans = await send('get-earliest-transaction');
+ if (trans) {
+ setEarliestTransaction(trans.date);
+ }
// ... rest of the effect
} catch (error) {
console.error('Error fetching earliest transaction:', error);
}
}
run();
}, []);Committable suggestion skipped: line range outside the PR's diff.
|
/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.
| const [earliestTransaction, _] = useState(''); | ||
| const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); | ||
| const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize earliestTransaction with a meaningful default.
The empty string default for earliestTransaction might cause issues with date calculations. Consider:
- Using a default like the current date
- Adding error handling for empty values in date calculations
-const [earliestTransaction, _] = useState('');
+const [earliestTransaction, _] = useState(() => new Date().toISOString());📝 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.
| const [earliestTransaction, _] = useState(''); | |
| const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); | |
| const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; | |
| const [earliestTransaction, _] = useState(() => new Date().toISOString()); | |
| const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); | |
| const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; |
Add "Year to date" and "Last year" to header report