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
|
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/reports/Header.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. 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 |
Add "Year to date" and "Last year" to header report