Calendar Report #3828
Calendar Report #3828
Conversation
✅ 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
Removed
Bigger
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 |
lelemm
force-pushed
the
calendar_report
branch
from
df17843 to
ad6ac02
Compare
|
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/graphs/CalendarGraph.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 several enhancements to the reporting functionality within the desktop client application. Key modifications include the addition of a new Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
👮 Files not reviewed due to content moderation or server errors (3)
🧰 Additional context used📓 Learnings (2)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (1)packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)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: 14
🧹 Outside diff range and nitpick comments (22)
packages/loot-core/src/types/models/dashboard.d.ts (1)
93-101: Consider moving type definition before its usageWhile the CalendarWidget type definition is well-structured and follows the established patterns, it's currently defined after its usage in the SpecializedWidget union type. Consider moving this type definition to be grouped with other widget types (before line 68) to maintain better code organization and prevent potential TypeScript resolution issues.
- // Current position (after SpecializedWidget) - export type CalendarWidget = AbstractWidget< - 'calendar-card', - { - name?: string; - conditions?: RuleConditionEntity[]; - conditionsOp?: 'and' | 'or'; - timeFrame?: TimeFrame; - } | null - >; + // Suggested position (grouped with other widget types, before SpecializedWidget) + export type CalendarWidget = AbstractWidget< + 'calendar-card', + { + name?: string; + conditions?: RuleConditionEntity[]; + conditionsOp?: 'and' | 'or'; + timeFrame?: TimeFrame; + } | null + >; type SpecializedWidget = | NetWorthWidget | CashFlowWidget | SpendingWidget | MarkdownWidget | CalendarWidget;packages/desktop-client/src/components/reports/reportRanges.ts (2)
164-167: Document the special case behavior.The special case where
offset === 1results instartequalingendshould be documented to prevent confusion.Add a comment explaining this behavior:
export function getLatestRange(offset: number) { const end = monthUtils.currentMonth(); + // When offset is 1, start and end will be the same month let start = end; if (offset !== 1) { start = monthUtils.subMonths(end, offset); } return [start, end, 'sliding-window'] as const; }
164-167: Consider handling negative offsets.The function should validate the
offsetparameter to handle edge cases.Consider adding validation:
export function getLatestRange(offset: number) { + if (offset <= 0) { + throw new Error('Offset must be a positive number'); + } const end = monthUtils.currentMonth(); let start = end; if (offset !== 1) { start = monthUtils.subMonths(end, offset); } return [start, end, 'sliding-window'] as const; }packages/desktop-client/src/components/transactions/TransactionList.jsx (1)
91-92: Document prop types for better maintainability.Consider adding PropTypes or TypeScript type definitions for the new props to improve code maintainability and prevent potential runtime issues.
+ import PropTypes from 'prop-types'; export function TransactionList({ // ... other props showSelection = true, allowSplitTransaction = true, }) { // ... component implementation } + TransactionList.propTypes = { + // ... other prop types + showSelection: PropTypes.bool, + allowSplitTransaction: PropTypes.bool, + };packages/desktop-client/src/components/reports/Overview.tsx (1)
562-570: Consider enhancing robustness and user experienceGood implementation, but consider these improvements:
- Add unit tests for the calendar widget integration
- Implement a loading state for the calendar data to improve user experience
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
885-886: Consider adding PropTypes and default values for the new props.The new props
showSelectionandallowSplitTransactionare propagated correctly through the component hierarchy, but they lack type definitions and default values. This could lead to runtime issues if the props are not provided.Consider adding:
+TransactionTable.propTypes = { + showSelection: PropTypes.bool, + allowSplitTransaction: PropTypes.bool, + // ... other prop types +}; + +TransactionTable.defaultProps = { + showSelection: true, + allowSplitTransaction: true, + // ... other default props +};Also applies to: 1790-1791, 1994-1995, 2615-2616
209-234: Enhance keyboard navigation accessibility for selection cells.While the selection UI is implemented correctly, it could benefit from improved keyboard navigation feedback.
Consider adding:
<SelectCell exposed={true} focused={false} selected={hasSelected} width={20} style={{ borderTopWidth: 0, borderBottomWidth: 0, + outline: 'none', + '&:focus-visible': { + boxShadow: `0 0 0 2px ${theme.focusColor}`, + }, }} onSelect={e => dispatchSelected({ type: 'select-all', isRangeSelect: e.shiftKey, }) } + aria-label="Select all transactions" />packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (4)
130-136: Simplify total value calculations by initializing to zeroCurrently,
totalExpenseValueandtotalIncomeValueare set tonullif there are no values, which requires additional null checks later. Initializing them to zero simplifies the logic and avoids unnecessary null checks.Apply this diff to initialize totals to zero:
const totalExpenseValue = expenseValues.length ? expenseValues.reduce((acc, val) => acc + val, 0) - : null; + : 0; const totalIncomeValue = incomeValues.length ? incomeValues.reduce((acc, val) => acc + val, 0) - : null; + : 0;Then, you can simplify the
getBarLengthfunction by removing the null checks:const getBarLength = (value: number) => { - if (value < 0 && totalExpenseValue !== null && totalExpenseValue !== 0) { + if (value < 0 && totalExpenseValue !== 0) { return (Math.abs(value) / totalExpenseValue) * 100; - } else if ( - value > 0 && - totalIncomeValue !== null && - totalIncomeValue !== 0 - ) { + } else if (value > 0 && totalIncomeValue !== 0) { return (value / totalIncomeValue) * 100; } else { return 0; } };
93-102: Improve clarity by renaming the 'recalculate' functionThe function name
recalculatemight not clearly indicate its purpose. Consider renaming it to better reflect its functionality, such asgenerateCalendarDataorcomputeCalendarMetrics, to enhance code readability and maintainability.
37-67: Combine income and expense queries to reduce database callsCurrently, two separate queries are executed to fetch income and expense data. Consider combining them into a single query to reduce database calls and improve performance.
You can modify the root query to group both positive and negative amounts and then separate them in the processing logic:
const data = await runQuery( makeRootQuery() - .filter({ - $and: { amount: { $lt: 0 } }, - }), + // No additional filter here ); // Process the data into incomeData and expenseData + const incomeData = data.data.filter(item => item.amount > 0); + const expenseData = data.data.filter(item => item.amount < 0);This approach reduces the number of queries from two to one.
106-113: Define a shared type for income and expense data itemsThe types for
incomeDataandexpenseDataare identical. Consider defining a shared type to eliminate duplication and improve code maintainability.Apply this diff to define a shared type:
+ type TransactionDataItem = { + date: string; + amount: number; + }; function recalculate( - incomeData: Array<{ - date: string; - amount: number; - }>, - expenseData: Array<{ - date: string; - amount: number; - }>, + incomeData: TransactionDataItem[], + expenseData: TransactionDataItem[], months: Date[], start: string, firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx'], ) {packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (3)
34-43: Remove unused commented-out code for cleaner codebaseThe commented-out code in lines 34-43 appears to be obsolete or no longer needed. Removing unnecessary code can improve readability and maintainability.
Apply this diff to remove the commented code:
type CalendarGraphProps = { data: { date: Date; incomeValue: number; expenseValue: number; incomeSize: number; expenseSize: number; }[]; start: Date; firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx']; onDayClick: (date: Date) => void; - // onFilter: ( - // conditionsOrSavedFilter: - // | null - // | { - // conditions: RuleConditionEntity[]; - // conditionsOp: 'and' | 'or'; - // id: RuleConditionEntity[]; - // } - // | RuleConditionEntity, - // ) => void; };
49-49: Clean up commented-out parameter in function declarationLine 49 contains a commented-out parameter
//onFilter,. If this parameter is no longer required, consider removing it to tidy up the code.Apply this diff:
export function CalendarGraph({ data, start, firstDayOfWeekIdx, - //onFilter, onDayClick, }: CalendarGraphProps) {
207-211: Eliminate unnecessary state inDayButtoncomponentThe
currentFontSizestate mirrors thefontSizeprop without additional computation. You can simplify the component by usingfontSizedirectly.Apply this diff to remove the redundant state:
function DayButton({ day, onPress, fontSize, resizeRef }: DayButtonProps) { - const [currentFontSize, setCurrentFontSize] = useState(fontSize); - - useEffect(() => { - setCurrentFontSize(fontSize); - }, [fontSize]); return ( <Button ref={resizeRef} /* ... */ > {/* ... */} <span style={{ - fontSize: `${currentFontSize}px`, + fontSize: `${fontSize}px`, fontWeight: 500, position: 'relative', }} > {getDate(day.date)} </span> </Button> ); }packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (1)
487-517: Avoid assignments within expressions for clearer codeUsing assignments within expressions, such as in the
refcallbacks, can make the code harder to read and maintain. Consider separating the assignment from the expression by using dedicated functions for each ref assignment.Apply this diff to refactor the
refassignments:- ref={rel => (monthFormatSizeContainers.current[0] = rel)} + const setMonthFormatSizeRef0 = (rel: HTMLSpanElement | null) => { + monthFormatSizeContainers.current[0] = rel; + }; + // ... + ref={setMonthFormatSizeRef0}Repeat this pattern for the other refs at lines 494, 501, 508, and 515:
// For index 1 - ref={rel => (monthFormatSizeContainers.current[1] = rel)} + const setMonthFormatSizeRef1 = (rel: HTMLSpanElement | null) => { + monthFormatSizeContainers.current[1] = rel; + }; + ref={setMonthFormatSizeRef1} // For index 2 - ref={rel => (monthFormatSizeContainers.current[2] = rel)} + const setMonthFormatSizeRef2 = (rel: HTMLSpanElement | null) => { + monthFormatSizeContainers.current[2] = rel; + }; + ref={setMonthFormatSizeRef2} // For index 3 - ref={rel => (monthFormatSizeContainers.current[3] = rel)} + const setMonthFormatSizeRef3 = (rel: HTMLSpanElement | null) => { + monthFormatSizeContainers.current[3] = rel; + }; + ref={setMonthFormatSizeRef3} // For index 4 - ref={rel => (monthFormatSizeContainers.current[4] = rel)} + const setMonthFormatSizeRef4 = (rel: HTMLSpanElement | null) => { + monthFormatSizeContainers.current[4] = rel; + }; + ref={setMonthFormatSizeRef4}🧰 Tools
🪛 Biome
[error] 487-487: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 494-494: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 501-501: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 508-508: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 515-515: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
278-278: Use Consistent Default Widget NameIn the
onSaveWidgetNamefunction, the default widget name is set to'Net Worth'. Since this component represents a calendar report, consider using'Calendar'as the default name for consistency.Apply this diff to update the default name:
- const name = newName || t('Net Worth'); + const name = newName || t('Calendar');
765-788: Avoid Rendering Empty Strings in Conditional RenderingCurrently, when
totalIncomeortotalExpenseis falsy, an empty string''is rendered. This can lead to unnecessary empty nodes in the DOM.Apply this diff to render
nullinstead:{totalIncome ? ( // Existing rendering logic ) : ( - '' + null )} {totalExpense ? ( // Existing rendering logic ) : ( - '' + null )}
184-210: Ensure Consistency in Error LoggingIn the added error handling, errors are logged using
console.error. For consistency and better debugging, consider using a centralized logging mechanism if available.If your application uses a logging service or utility, replace
console.errorwith that method.packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (4)
Line range hint
32-34: Consolidate duplicate mocks for'../../hooks/useFeatureFlag'There are two
vi.mockstatements for'../../hooks/useFeatureFlag', which might lead to unexpected behavior or conflicts in the tests. It's advisable to combine them into a single mock to maintain clarity and avoid potential issues.Consider merging them as follows:
-vi.mock('../../hooks/useFeatureFlag', () => ({ - default: vi.fn().mockReturnValue(false), -})); -vi.mock('../../hooks/useFeatureFlag', () => ({ - useFeatureFlag: () => false, +vi.mock('../../hooks/useFeatureFlag', () => ({ + default: vi.fn().mockReturnValue(false), + useFeatureFlag: () => false, }));Also applies to: 38-40
Line range hint
279-282: Address the TODO: Fix flakiness and re-enable the testThere's a commented-out section with a TODO note to fix flakiness and re-enable the test for navigating to the top of the transaction list. Resolving this will ensure that all keybindings are thoroughly tested, improving the robustness of the navigation functionality.
Would you like assistance in diagnosing the cause of the flakiness and implementing a fix? I can help by providing suggestions or code changes to resolve the issue.
Line range hint
359-384: Re-enable the skipped test:'dropdown invalid value resets correctly'The test
'dropdown invalid value resets correctly'is currently skipped usingtest.skip. It's important to address any underlying issues causing the test to fail so that input validation for invalid values is thoroughly tested.Can I assist in debugging this test to identify and fix the issues? Re-enabling it will help ensure that invalid inputs are handled correctly.
171-172: Add test cases for'showSelection'and'allowSplitTransaction'propsTo ensure the new props
showSelection={true}andallowSplitTransaction={true}function as intended, consider adding specific test cases that verify their behavior. This will help confirm that selection visibility and split transaction capabilities are working correctly in theTransactionTable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3828.mdis excluded by!**/*.md
📒 Files selected for processing (11)
packages/desktop-client/src/components/reports/Overview.tsx(5 hunks)packages/desktop-client/src/components/reports/ReportRouter.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reportRanges.ts(1 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts(1 hunks)packages/desktop-client/src/components/transactions/TransactionList.jsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(10 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(1 hunks)packages/loot-core/src/types/models/dashboard.d.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
[error] 487-487: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 494-494: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 501-501: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 508-508: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 515-515: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (13)
packages/desktop-client/src/components/reports/ReportRouter.tsx (2)
5-5: LGTM! Import follows established pattern
The Calendar import follows the consistent pattern used for other report components.
23-24: LGTM! Routes follow established pattern
The new Calendar routes maintain consistency with other report routes by:
- Following the same base path + :id parameter pattern
- Using the same component structure
Let's verify the Calendar component implements the expected route parameter handling:
packages/loot-core/src/types/models/dashboard.d.ts (1)
68-69: LGTM: Clean addition to SpecializedWidget union type
The CalendarWidget is properly integrated into the SpecializedWidget union type, following the established pattern.
packages/desktop-client/src/components/reports/reportRanges.ts (1)
164-167: Verify the impact of modified date range logic.
The change introduces a special case where start equals end when offset is 1. While this might be intentional for the new calendar feature, we should verify it doesn't affect existing reports.
Let's check for existing usages:
packages/desktop-client/src/components/transactions/TransactionList.jsx (1)
256-257: LGTM!
The props are correctly passed down to the TransactionTable component, maintaining a clean pass-through implementation.
packages/desktop-client/src/components/reports/Overview.tsx (3)
25-25: LGTM: Clean implementation of user preference sync
The implementation properly handles the first day of week preference with a sensible default value.
Also applies to: 56-57
38-38: LGTM: Consistent import organization
The CalendarCard import follows the established pattern for report components.
562-570: Verify CalendarCard component interface
The integration looks good, but please ensure:
- The CalendarCard component has proper TypeScript interfaces for all props
- The component is documented with JSDoc comments
#!/bin/bash
# Search for CalendarCard component definition and its interface
ast-grep --pattern 'interface $name {
$$$
widgetId: $_
isEditing: $_
meta: $_
firstDayOfWeekIdx: $_
$$$
}'
# Search for JSDoc comments
rg -B 2 "export (type|interface) .*CalendarCard.*|export function CalendarCard"packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (1)
159-162:
Review the calculation logic for 'totalDays'
The calculation for totalDays may not correctly account for the number of days to display, especially when the month doesn't start on the first day of the week. d.differenceInDays(firstDay, beginDay) can return a negative value if beginDay is before firstDay, which is expected, but adding d.getDaysInMonth(firstDay) may not produce the intended result. Consider revising the logic to ensure that totalDays accurately represents the days needed to fill the calendar grid.
Please verify the correctness of the totalDays calculation. You can test this with different months and firstDayOfWeekIdx values to ensure the calendar displays properly.
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (1)
183-183: Verify the logic for assigning resizeRef based on index
Assigning resizeRef to DayButton only when index === 15 may not work consistently with varying data lengths. Ensure that this logic is intentional and functions correctly for all data sets.
Consider applying resizeRef to a consistent element, such as the first day of the month, or refactoring the approach to dynamically select an appropriate DayButton.
packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
356-360: Include Dependencies in useEffect Dependency Array
The useEffect hook uses refContainer.current, but refContainer is not included in the dependency array. Although refs generally don't need to be in dependencies, if refContainer.current can change, consider adding it to ensure the effect runs as expected.
Please verify if refContainer.current can change over time. If so, include it in the dependencies:
useEffect(() => {
if (refContainer.current) {
setTotalHeight(refContainer.current.clientHeight - 115);
}
- }, [query]);
+ }, [query, refContainer.current]);399-415: Ensure Proper Handling of Negative Offset Values
In the useDrag hook, the condition oy < 0 triggers cancel() and opens the transactions. Ensure that negative offset values are correctly handled and that this logic matches the intended user interaction.
Please test the drag behavior to confirm that upward swipes open the transaction list as intended, and that the gestures function correctly on different devices.
121-123: Confirm Default Value for firstDayOfWeekIdx
The default value for firstDayOfWeekIdx is set to '0' if _firstDayOfWeekIdx is undefined. Ensure that this default aligns with the expected first day of the week in your application (e.g., Sunday).
Consider parsing the value to an integer:
- const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0';
+ const firstDayOfWeekIdx = parseInt(_firstDayOfWeekIdx ?? '0', 10);
packages/desktop-client/src/components/transactions/TransactionsTable.jsx
Show resolved
Hide resolved
packages/desktop-client/src/components/transactions/TransactionsTable.jsx
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
lelemm
force-pushed
the
calendar_report
branch
from
b0037de to
ab75214
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (2)
18-36: Consider extracting the callback type definition.The
setDatacallback type is complex and could be reused elsewhere. Consider extracting it into a separate type definition.+type CalendarSetDataType = { + calendarData: { + start: Date; + end: Date; + data: CalendarDataType[]; + totalExpense: number; + totalIncome: number; + }[]; +}; export function calendarSpreadsheet( start: string, end: string, conditions: RuleConditionEntity[] = [], conditionsOp: 'and' | 'or' = 'and', firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx'], ) { return async ( spreadsheet: ReturnType<typeof useSpreadsheet>, - setData: (data: { - calendarData: { - start: Date; - end: Date; - data: CalendarDataType[]; - totalExpense: number; - totalIncome: number; - }[]; - }) => void, + setData: (data: CalendarSetDataType) => void, ) => {
103-114: Simplify month range calculation using date-fns.The
getOneDatePerMonthfunction could be simplified usingeachMonthOfIntervalfrom date-fns.- const getOneDatePerMonth = (start: Date, end: Date) => { - const months = []; - let currentDate = d.startOfMonth(start); - - while (!d.isSameMonth(currentDate, end)) { - months.push(currentDate); - currentDate = d.addMonths(currentDate, 1); - } - months.push(end); - - return months; - }; + const getOneDatePerMonth = (start: Date, end: Date) => { + return d.eachMonthOfInterval({ start, end }); + };packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (4)
34-43: Remove or implement the commented outonFilterpropThe commented out
onFilterprop and its type definition suggest incomplete functionality. Either implement this feature or remove the commented code to maintain clean and maintainable code.
78-81: Extract grid layout constantsConsider extracting the grid layout values into named constants for better maintainability and reusability.
+const CALENDAR_GRID = { + COLUMNS: 7, + GAP: 2, +} as const; + <View style={{ color: theme.pageTextSubdued, display: 'grid', - gridTemplateColumns: 'repeat(7, 1fr)', + gridTemplateColumns: `repeat(${CALENDAR_GRID.COLUMNS}, 1fr)`, gridAutoRows: '1fr', - gap: 2, + gap: CALENDAR_GRID.GAP, }} >
119-176: Extract tooltip content into a separate componentThe tooltip content is complex and could benefit from being extracted into a separate component for better maintainability and reusability.
Consider creating a
DayTooltipcomponent to encapsulate this logic:type DayTooltipProps = { day: { date: Date; incomeValue: number; expenseValue: number; incomeSize: number; expenseSize: number; }; }; function DayTooltip({ day }: DayTooltipProps) { const { t } = useTranslation(); return ( <View> <View style={{ marginBottom: 10 }}> <strong> {t('Day:') + ' '} {format(day.date, 'dd')} </strong> </View> {/* ... rest of the tooltip content ... */} </View> ); }
232-282: Reduce style duplication in bar componentsThe bar styles for income and expense are very similar. Consider extracting common styles into a shared object or utility function.
const getBarStyles = (side: 'left' | 'right', color: string, size: number) => ({ position: 'absolute', [side]: 0, bottom: 0, opacity: 0.9, height: `${Math.ceil(size)}%`, backgroundColor: color, width: '50%', transition: 'height 0.5s ease-out', } as const); // Usage: <View className="bar positive-bar" style={getBarStyles('left', chartTheme.colors.blue, day.incomeSize)} /> <View className="bar" style={getBarStyles('right', chartTheme.colors.red, day.expenseSize)} />packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (3)
44-51: Consider enhancing type safety for meta propThe
metaprop's type could be more strictly defined to prevent potential runtime errors.Consider this improvement:
type CalendarProps = { widgetId: string; isEditing?: boolean; - meta?: CalendarWidget['meta']; + meta?: { + name?: string; + timeFrame?: string; + conditions?: unknown; + conditionsOp?: string; + }; onMetaChange: (newMeta: CalendarWidget['meta']) => void; onRemove: () => void; firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx']; };
97-111: Optimize total calculations with single reduceThe current implementation uses two separate reduce operations to calculate totals. This can be optimized to a single pass.
Consider this optimization:
const { totalIncome, totalExpense } = useMemo(() => { if (!data) { return { totalIncome: 0, totalExpense: 0 }; } - return { - totalIncome: data.calendarData.reduce( - (prev, cur) => prev + cur.totalIncome, - 0, - ), - totalExpense: data.calendarData.reduce( - (prev, cur) => prev + cur.totalExpense, - 0, - ), - }; + return data.calendarData.reduce( + (acc, cur) => ({ + totalIncome: acc.totalIncome + cur.totalIncome, + totalExpense: acc.totalExpense + cur.totalExpense, + }), + { totalIncome: 0, totalExpense: 0 }, + ); }, [data]);
431-473: Enhance accessibility for financial indicatorsWhile basic aria-labels are present, the financial information could be more accessible.
Consider these improvements:
<View style={{ color: chartTheme.colors.blue, flexDirection: 'row', fontSize: '10px', marginRight: 10, }} - aria-label="Income" + aria-label={`Income ${calendar.totalIncome !== 0 ? amountToCurrency(calendar.totalIncome) : 'none'}`} + role="text" > {/* ... */} </View> <View style={{ color: chartTheme.colors.red, flexDirection: 'row', fontSize: '10px', }} - aria-label="Expenses" + aria-label={`Expenses ${calendar.totalExpense !== 0 ? amountToCurrency(calendar.totalExpense) : 'none'}`} + role="text" >packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
93-131: Consider consolidating related state managementThe component manages several related pieces of state (start, end, mode) separately. Consider using useReducer to manage these related states together, which would make the state transitions more predictable and easier to maintain.
- const [start, setStart] = useState(initialStart); - const [end, setEnd] = useState(initialEnd); - const [mode, setMode] = useState(initialMode); + const [dateRange, dispatch] = useReducer(dateRangeReducer, { + start: initialStart, + end: initialEnd, + mode: initialMode + });
702-830: Consider extracting styles for better maintainabilityThe component contains multiple inline styles that could be extracted into a separate styles object or styled-components for better organization and reuse.
+const calendarStyles = { + container: { + minWidth: '300px', + maxWidth: '300px', + padding: 10, + borderRadius: 4, + backgroundColor: theme.tableBackground, + }, + header: { + flexDirection: 'row', + justifyContent: 'space-between', + flexWrap: 'wrap', + marginBottom: 16, + }, + // ... other styles +}; function CalendarWithHeader({ ... }) { return ( <View - style={{ - minWidth: '300px', - maxWidth: '300px', - padding: 10, - borderRadius: 4, - backgroundColor: theme.tableBackground, - }} + style={calendarStyles.container} >
913-932: Simplify field mapping with object literalThe switch statement could be replaced with an object literal for better maintainability and performance.
+const FIELD_MAPPINGS = { + account: 'account.name', + payee: 'payee.name', + category: 'category.name', + payment: 'amount', + deposit: 'amount', +}; function getField(field?: string) { - if (!field) { - return 'date'; - } - - switch (field) { - case 'account': - return 'account.name'; - case 'payee': - return 'payee.name'; - case 'category': - return 'category.name'; - case 'payment': - return 'amount'; - case 'deposit': - return 'amount'; - default: - return field; - } + if (!field) return 'date'; + return FIELD_MAPPINGS[field] || field; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (1)
Learnt from: lelemm
PR: actualbudget/actual#3828
File: packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts:154-157
Timestamp: 2024-11-12T16:44:22.913Z
Learning: In `packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts`, the variable `firstDayOfWeekIdx` comes from a system parameter and is guaranteed to be a number between 0 and 6, so additional validation is not necessary when using it in `startOfWeek`.
🪛 Biome
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
[error] 56-56: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
[error] 490-490: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 497-497: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 504-504: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 511-511: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 518-518: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (6)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (3)
1-17: LGTM! Well-structured imports and type definitions.
The imports are properly organized and the CalendarDataType interface is well-defined with clear type annotations.
79-101: LGTM! Robust error handling for data fetching.
The code properly handles potential errors during data fetching and provides appropriate fallbacks.
187-190: LGTM! Proper handling of firstDayOfWeekIdx.
Based on the learnings from previous reviews, firstDayOfWeekIdx is guaranteed to be a number between 0 and 6, so the current implementation is correct.
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (1)
1-294: Overall implementation looks good!
The calendar graph implementation effectively visualizes financial data with good separation of concerns, proper type definitions, and responsive design. The suggested improvements are mostly about code organization and reducing duplication.
🧰 Tools
🪛 Biome
[error] 56-56: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (1)
1-525: Overall implementation looks good!
The calendar card implementation is well-structured with proper separation of concerns, good use of hooks, and responsive design considerations. The suggested improvements are non-blocking and can be addressed in follow-up iterations.
🧰 Tools
🪛 Biome
[error] 490-490: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 497-497: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 504-504: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 511-511: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 518-518: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
73-86: LGTM! Clean component structure with proper loading handling.
The Calendar component effectively manages the loading state and cleanly delegates the main functionality to CalendarInner.
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (20)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (2)
206-211: Simplify firstDayOfWeekIdx validationThe current validation is verbose and performs multiple
parseIntoperations. Consider simplifying it using a single parse operation.- weekStartsOn: - firstDayOfWeekIdx !== undefined && - !Number.isNaN(parseInt(firstDayOfWeekIdx)) && - parseInt(firstDayOfWeekIdx) >= 0 && - parseInt(firstDayOfWeekIdx) <= 6 - ? (parseInt(firstDayOfWeekIdx) as 0 | 1 | 2 | 3 | 4 | 5 | 6) - : 0, + weekStartsOn: (() => { + const idx = typeof firstDayOfWeekIdx === 'string' ? parseInt(firstDayOfWeekIdx) : firstDayOfWeekIdx; + return idx !== undefined && idx >= 0 && idx <= 6 ? (idx as 0 | 1 | 2 | 3 | 4 | 5 | 6) : 0; + })(),
61-62: Enhance error messages for date parsing failuresThe current error messages could be more specific to help with debugging.
- throw new Error('Invalid start date format'); + throw new Error(`Invalid start date format: "${monthUtils.firstDayOfMonth(start)}". Expected format: yyyy-MM-dd`);- throw new Error('Invalid end date format'); + throw new Error(`Invalid end date format: "${monthUtils.lastDayOfMonth(end)}". Expected format: yyyy-MM-dd`);Also applies to: 73-74
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (6)
34-43: Remove or implement the commented filter functionalityThe commented out
onFilterprop and its type definition suggest an incomplete feature. Either implement the filtering functionality or remove the commented code to maintain code cleanliness.
54-60: Optimize firstDayOfWeekIdx parsingThe
parseIntfunction is called multiple times on the same value. Consider parsing it once and storing the result.- weekStartsOn: - firstDayOfWeekIdx !== undefined && - !Number.isNaN(parseInt(firstDayOfWeekIdx)) && - parseInt(firstDayOfWeekIdx) >= 0 && - parseInt(firstDayOfWeekIdx) <= 6 - ? (parseInt(firstDayOfWeekIdx) as 0 | 1 | 2 | 3 | 4 | 5 | 6) - : 0, + weekStartsOn: (() => { + const parsed = parseInt(firstDayOfWeekIdx); + return firstDayOfWeekIdx !== undefined && + !Number.isNaN(parsed) && + parsed >= 0 && + parsed <= 6 + ? (parsed as 0 | 1 | 2 | 3 | 4 | 5 | 6) + : 0; + })(),
64-71: Extract magic numbers into named constantsThe font size calculation uses magic numbers (14). Consider extracting these into named constants for better maintainability.
+const MAX_FONT_SIZE = 14; + const buttonRef = useResizeObserver(rect => { const newValue = Math.floor(rect.height / 2); - if (newValue > 14) { - setFontSize(14); + if (newValue > MAX_FONT_SIZE) { + setFontSize(MAX_FONT_SIZE); } else { setFontSize(newValue); } });
143-143: Use translation keys for tooltip labelsThe strings "Income:" and "Expenses:" are hardcoded. Since you're using i18n, these should use translation keys for consistency.
- Income: + {t('income.label')} // ... - Expenses: + {t('expenses.label')}Also applies to: 163-163
214-216: Clean up effect dependenciesThe
useEffecthook doesn't clean up the state when the component unmounts. While this might not cause issues in the current implementation, it's a good practice to include cleanup.useEffect(() => { setCurrentFontSize(fontSize); + return () => { + // Clean up any pending state updates + setCurrentFontSize(prevSize => prevSize); + }; }, [fontSize]);
267-267: Extract transition styles into themeThe transition properties are duplicated. Consider extracting them into the theme for consistency and reusability.
// In theme.ts +export const transitions = { + barHeight: 'height 0.5s ease-out', +}; // In this file - transition: 'height 0.5s ease-out', + transition: theme.transitions.barHeight, // ... - transition: 'height 0.5s ease-out', + transition: theme.transitions.barHeight,Also applies to: 281-281
packages/desktop-client/src/style/themes/development.ts (1)
217-217: Group calendar-related constants together.Consider moving this constant to be grouped with other calendar-related constants around line 134 for better maintainability and readability.
export const calendarText = colorPalette.navy50; export const calendarBackground = colorPalette.navy900; export const calendarItemText = colorPalette.navy150; export const calendarItemBackground = colorPalette.navy800; export const calendarSelectedBackground = colorPalette.navy500; +export const calendarCellBackground = colorPalette.navy900;packages/desktop-client/src/style/themes/dark.ts (1)
217-217: Group calendar-related constants together.Consider moving this constant to be grouped with other calendar-related constants around line 144 for better code organization and maintainability.
export const calendarText = colorPalette.navy50; export const calendarBackground = colorPalette.navy900; export const calendarItemText = colorPalette.navy150; export const calendarItemBackground = colorPalette.navy800; export const calendarSelectedBackground = buttonNormalSelectedBackground; +export const calendarCellBackground = colorPalette.navy900;packages/desktop-client/src/style/themes/light.ts (1)
219-219: Consider grouping with existing calendar constants.While the new
calendarCellBackgroundconstant is valid, consider moving it near the other calendar-related constants (around line 144) to maintain better code organization and make it easier to manage calendar theming.export const calendarSelectedBackground = colorPalette.navy500; +export const calendarCellBackground = colorPalette.navy100; export const buttonBareText = buttonNormalText;packages/desktop-client/src/style/themes/midnight.ts (1)
219-219: Group calendar-related constants together.Consider moving
calendarCellBackgroundto be grouped with other calendar-related constants (around line 150) for better code organization and maintainability.export const calendarSelectedBackground = buttonNormalSelectedBackground; +export const calendarCellBackground = colorPalette.navy900; export const buttonBareText = buttonNormalText; - -export const calendarCellBackground = colorPalette.navy900;packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (4)
159-169: Consider using TypeScript discriminated unions for menu itemsThe switch statement uses string literals, which could be made type-safe using a discriminated union type.
+type MenuItem = { name: 'rename'; text: string } | { name: 'remove'; text: string }; -onMenuSelect={item => { +onMenuSelect={(item: MenuItem['name']) => { switch (item) {
207-223: Add aria-label to income and expense sections in tooltipThe tooltip content should have proper aria-labels for better accessibility.
-<View style={{ lineHeight: 1.5 }}> +<View style={{ lineHeight: 1.5 }} aria-label="Financial summary"> <View style={{ display: 'grid', gridTemplateColumns: '70px 1fr', gridAutoRows: '1fr', }} + role="list" >Also applies to: 224-240
398-423: Enhance keyboard navigation for the month buttonThe month button should have proper keyboard focus management and ARIA attributes.
<Button variant="bare" + role="link" + aria-label={`View calendar for ${format(calendar.start, selectedMonthNameFormat)}`} style={{ visibility: monthNameVisible ? 'visible' : 'hidden', overflow: 'visible',
489-532: Refactor month format measurement elementsThe repeated span elements for month format measurements could be generated programmatically to reduce code duplication.
+const MONTH_FORMATS = [ + 'MMMM yyyy', + 'MMM yyyy', + 'MMM yy', + 'MMM', + '' +] as const; <View style={{ fontWeight: 'bold', fontSize: '12px' }}> - <span - ref={node => { - if (node) monthFormatSizeContainers.current[0] = node; - }} - style={{ position: 'fixed', top: -9999, left: -9999 }} - data-format="MMMM yyyy" - > - {format(calendar.start, 'MMMM yyyy')}: - </span> + {MONTH_FORMATS.map((formatString, idx) => ( + <span + key={formatString} + ref={node => { + if (node) monthFormatSizeContainers.current[idx] = node; + }} + style={{ position: 'fixed', top: -9999, left: -9999 }} + data-format={formatString} + > + {formatString ? `${format(calendar.start, formatString)}:` : ''} + </span> + ))}packages/desktop-client/src/components/reports/reports/Calendar.tsx (4)
121-122: Consider using a type-safe default for firstDayOfWeekIdxThe current fallback to '0' could be more explicit and type-safe.
-const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); -const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; +const DEFAULT_FIRST_DAY_OF_WEEK = '0'; +const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); +const firstDayOfWeekIdx = _firstDayOfWeekIdx ?? DEFAULT_FIRST_DAY_OF_WEEK;
724-730: Extract hardcoded dimensions as constantsConsider extracting the hardcoded dimensions for better maintainability.
+const CALENDAR_DIMENSIONS = { + MIN_WIDTH: '300px', + MAX_WIDTH: '300px', + PADDING: 10, + BORDER_RADIUS: 4, +}; + <View style={{ - minWidth: '300px', - maxWidth: '300px', - padding: 10, - borderRadius: 4, + minWidth: CALENDAR_DIMENSIONS.MIN_WIDTH, + maxWidth: CALENDAR_DIMENSIONS.MAX_WIDTH, + padding: CALENDAR_DIMENSIONS.PADDING, + borderRadius: CALENDAR_DIMENSIONS.BORDER_RADIUS, backgroundColor: theme.tableBackground, }} >
889-918: Simplify conditional rendering logicThe conditions for showing income and expense can be simplified to avoid repetition.
- {totalIncome !== 0 && ( + {[ + { label: 'Income:', value: totalIncome, color: chartTheme.colors.blue }, + { label: 'Expenses:', value: totalExpense, color: chartTheme.colors.red } + ].map(({ label, value, color }) => value !== 0 && ( <> <View style={{ textAlign: 'right', marginRight: 4, }} > - Income: + {label} </View> - <View style={{ color: chartTheme.colors.blue }}> - {totalIncome !== 0 ? amountToCurrency(totalIncome) : ''} + <View style={{ color }}> + {amountToCurrency(value)} </View> </> - )} - {totalExpense !== 0 && ( - <> - <View - style={{ - textAlign: 'right', - marginRight: 4, - }} - > - Expenses: - </View> - <View style={{ color: chartTheme.colors.red }}> - {totalExpense !== 0 ? amountToCurrency(totalExpense) : ''} - </View> - </> - )} + ))}
926-945: Enhance type safety of field mappingConsider using TypeScript enums or constants for field names to improve type safety and maintainability.
+enum FieldMapping { + Account = 'account', + Payee = 'payee', + Category = 'category', + Payment = 'payment', + Deposit = 'deposit', +} + +const FIELD_NAME_MAPPING: Record<FieldMapping, string> = { + [FieldMapping.Account]: 'account.name', + [FieldMapping.Payee]: 'payee.name', + [FieldMapping.Category]: 'category.name', + [FieldMapping.Payment]: 'amount', + [FieldMapping.Deposit]: 'amount', +}; + function getField(field?: string) { if (!field) { return 'date'; } - switch (field) { - case 'account': - return 'account.name'; - case 'payee': - return 'payee.name'; - case 'category': - return 'category.name'; - case 'payment': - return 'amount'; - case 'deposit': - return 'amount'; - default: - return field; - } + return FIELD_NAME_MAPPING[field as FieldMapping] || field; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts(1 hunks)packages/desktop-client/src/style/themes/dark.ts(1 hunks)packages/desktop-client/src/style/themes/development.ts(1 hunks)packages/desktop-client/src/style/themes/light.ts(1 hunks)packages/desktop-client/src/style/themes/midnight.ts(1 hunks)
🔇 Additional comments (6)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (1)
1-17: LGTM! Well-organized imports and clear type definitions.
The imports are properly organized and the CalendarDataType interface is well-structured with clear field names.
packages/desktop-client/src/style/themes/dark.ts (1)
217-217: Verify color contrast ratios for accessibility.
The calendarCellBackground uses navy900, which should provide sufficient contrast with text colors. However, let's verify the contrast ratios meet WCAG guidelines.
packages/desktop-client/src/style/themes/light.ts (1)
219-219: Verify color contrast with existing calendar colors.
The new calendarCellBackground uses navy100 which needs to work well with existing calendar colors:
- Text: navy50
- Background: navy900
- Item text: navy150
- Item background: navy800
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (2)
1-51: LGTM! Well-structured imports and type definitions.
The imports are properly organized, and the type definitions are clear and comprehensive.
68-78: Consider adding error handling for invalid date ranges
The useMemo hook calculates params without validating that start is before end. While the calculateTimeRange function likely handles this, explicit validation would improve robustness.
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
73-86: LGTM! Well-structured component with proper loading state handling.
The component follows good practices by:
- Using TypeScript for type safety
- Showing a loading indicator during data fetching
- Having a single responsibility
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
lelemm
force-pushed
the
calendar_report
branch
from
b8e0cdb to
a2cebb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (17)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (3)
18-24: Consider adding input validation for date parameters.The function accepts date strings but doesn't validate their format at the parameter level. While there's error handling during parsing, early validation could prevent unnecessary processing.
Consider adding validation:
export function calendarSpreadsheet( start: string, end: string, conditions: RuleConditionEntity[] = [], conditionsOp: 'and' | 'or' = 'and', firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx'], ) { + const isValidDateFormat = (date: string) => /^\d{4}-\d{2}-\d{2}$/.test(date); + if (!isValidDateFormat(start) || !isValidDateFormat(end)) { + throw new Error('Invalid date format. Expected: YYYY-MM-DD'); + } return async (
219-223: Consider extracting and documenting the calendar grid calculation logic.The logic for calculating
totalDaysand padding to ensure complete weeks is important but could be more self-documenting.Consider extracting this logic:
- let totalDays = - d.differenceInDays(firstDay, beginDay) + d.getDaysInMonth(firstDay); - if (totalDays % 7 !== 0) { - totalDays += 7 - (totalDays % 7); - } + const calculateCalendarGridDays = (firstDay: Date, beginDay: Date) => { + // Calculate days needed to display the full month + const daysBeforeMonth = d.differenceInDays(firstDay, beginDay); + const daysInMonth = d.getDaysInMonth(firstDay); + const totalDays = daysBeforeMonth + daysInMonth; + + // Ensure we have complete weeks by padding if necessary + const remainingDays = totalDays % 7; + return remainingDays === 0 ? totalDays : totalDays + (7 - remainingDays); + }; + + const totalDays = calculateCalendarGridDays(firstDay, beginDay);
244-244: Document currency conversion assumptions.The code divides monetary values by 100, suggesting a conversion from cents to dollars/euros, but this assumption isn't documented.
Consider adding comments:
return { data: daysArray as CalendarDataType[], + // Convert from cents to dollars/euros totalExpense: (totalExpenseValue ?? 0) / 100, totalIncome: (totalIncomeValue ?? 0) / 100, };Also applies to: 246-246, 253-254
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (6)
34-43: Remove or implement the commented filter functionalityThe commented out
onFilterprop suggests incomplete functionality. Either implement the filtering feature or remove the commented code to maintain clean types.- // onFilter: ( - // conditionsOrSavedFilter: - // | null - // | { - // conditions: RuleConditionEntity[]; - // conditionsOp: 'and' | 'or'; - // id: RuleConditionEntity[]; - // } - // | RuleConditionEntity, - // ) => void;
23-34: Add JSDoc comments to type definitionsConsider adding documentation to describe the purpose and usage of each prop in the
CalendarGraphPropstype.+/** + * Props for the CalendarGraph component + * @property data - Array of daily financial data + * @property start - Starting date for the calendar + * @property firstDayOfWeekIdx - User preference for first day of week + * @property onDayClick - Callback when a day is clicked + */ type CalendarGraphProps = { data: { date: Date; incomeValue: number; expenseValue: number; incomeSize: number; expenseSize: number; }[]; start: Date; firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx']; onDayClick: (date: Date) => void; };
110-110: Remove or implement the commented grid calculationThere's a commented out grid calculation that should either be implemented or removed.
- //gridTemplateRows: `repeat(${Math.trunc(data.length) <= data.length / 7 ? Math.trunc(data.length) : Math.trunc(data.length) + 1},1fr)`,
119-178: Extract tooltip content into a separate componentThe tooltip content is complex enough to warrant its own component. This would improve maintainability and reusability.
Consider creating a
DayTooltipcomponent:type DayTooltipProps = { day: { date: Date; incomeValue: number; expenseValue: number; incomeSize: number; expenseSize: number; }; }; function DayTooltip({ day }: DayTooltipProps) { const { t } = useTranslation(); return ( <View> <View style={{ marginBottom: 10 }}> <strong> {t('Day:') + ' '} {format(day.date, 'dd')} </strong> </View> {/* ... rest of the tooltip content ... */} </View> ); }
211-296: Optimize DayButton component performanceThe component could benefit from several performance optimizations:
- Memoize the component to prevent unnecessary re-renders
- Extract static styles
- Use CSS classes instead of inline styles for common properties
+ const dayButtonStyles = { + button: { + borderColor: 'transparent', + backgroundColor: theme.calendarCellBackground, + position: 'relative' as const, + padding: 'unset', + height: '100%', + minWidth: 0, + minHeight: 0, + margin: 0, + }, + // ... other static styles + }; - function DayButton({ day, onPress, fontSize, resizeRef }: DayButtonProps) { + const DayButton = React.memo(function DayButton({ + day, + onPress, + fontSize, + resizeRef + }: DayButtonProps) { // ... component implementation - } + });
234-284: Consider using CSS Grid for bar layoutThe current implementation uses absolute positioning for the bars. Consider using CSS Grid for better maintainability and performance.
+ const barContainerStyle = { + display: 'grid', + gridTemplateColumns: '1fr 1fr', + height: '100%', + position: 'absolute', + width: '100%', + }; - {day.expenseSize !== 0 && ( - <View - style={{ - position: 'absolute', - width: '50%', - height: '100%', - // ... - }} - /> - )} + <View style={barContainerStyle}> + <View + style={{ + height: `${Math.ceil(day.incomeSize)}%`, + backgroundColor: chartTheme.colors.blue, + opacity: 0.9, + alignSelf: 'end', + transition: 'height 0.5s ease-out', + }} + /> + <View + style={{ + height: `${Math.ceil(day.expenseSize)}%`, + backgroundColor: chartTheme.colors.red, + opacity: 0.9, + alignSelf: 'end', + transition: 'height 0.5s ease-out', + }} + /> + </View>packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (4)
200-244: Add i18n translations for tooltip contentThe tooltip content contains hardcoded English strings that should be translated.
Consider using the translation hook:
<View style={{ lineHeight: 1.5 }}> <View style={{ display: 'grid', gridTemplateColumns: '70px 1fr', gridAutoRows: '1fr', }}> {totalIncome !== 0 && ( <> <View style={{ textAlign: 'right', marginRight: 4, }}> - Income: + {t('Income')}: </View> <View style={{ color: chartTheme.colors.blue }}> {totalIncome !== 0 ? amountToCurrency(totalIncome) : ''} </View> </> )} {totalExpense !== 0 && ( <> <View style={{ textAlign: 'right', marginRight: 4, }}> - Expenses: + {t('Expenses')}: </View> <View style={{ color: chartTheme.colors.red }}> {totalExpense !== 0 ? amountToCurrency(totalExpense) : ''} </View> </> )} </View> </View>
160-170: Improve menu selection error handlingThe error handling for unrecognized menu selections could be more graceful.
Consider using a more descriptive error message and logging:
switch (item) { case 'rename': setNameMenuOpen(true); break; case 'remove': onRemove(); break; default: - throw new Error(`Unrecognized selection: ${item}`); + console.error(`Unrecognized menu selection: ${item}`); + // Handle gracefully instead of throwing + break; }
327-333: Memoize month format measurementsThe
measureMonthFormatsfunction could benefit from memoization to prevent unnecessary recalculations.Consider using
useMemo:- const measureMonthFormats = useCallback(() => { + const measureMonthFormats = useMemo(() => () => { const measurements = monthFormatSizeContainers.current.map(container => ({ width: container?.clientWidth ?? 0, format: container?.getAttribute('data-format') ?? '', })); return measurements; }, []);
487-493: Add error boundary for calendar click handlerThe calendar day click handler should be wrapped in a try-catch block to handle potential navigation errors gracefully.
Consider adding error handling:
onDayClick={date => { + try { navigate( isDashboardsFeatureEnabled ? `/reports/calendar/${widgetId}?day=${format(date, 'yyyy-MM-dd')}` : '/reports/calendar', ); + } catch (error) { + console.error('Failed to navigate:', error); + // Handle the error gracefully, perhaps show a notification + } }}packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
93-691: Consider breaking down the CalendarInner component.The component is handling multiple responsibilities including state management, filtering, mobile UI, and transaction list rendering. Consider extracting some of these into separate components for better maintainability.
Potential extractions:
- Filter management logic
- Mobile transaction drawer
- Transaction list wrapper
740-771: Add aria-label to month filter button.The button shows the month and year but lacks an aria-label to indicate its filtering action.
<Button variant="bare" + aria-label={`Filter transactions for ${format(calendar.start, 'MMMM yyyy')}`} style={{ color: theme.pageTextSubdued, fontWeight: 'bold',
926-945: Improve type safety of getField function.Consider using a union type for the field parameter to catch invalid field names at compile time.
+type AllowedFields = 'account' | 'payee' | 'category' | 'payment' | 'deposit' | 'date'; + -function getField(field?: string) { +function getField(field?: AllowedFields) { if (!field) { return 'date'; }packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
1918-1919: Consider using React Context for prop propagationWhile the current implementation correctly propagates the selection and split transaction props through the component hierarchy, consider using React Context to avoid prop drilling, especially since these props are used by multiple nested components.
Example implementation:
+const TransactionContext = React.createContext({ + showSelection: true, + allowSplitTransaction: true +}); + +export function TransactionProvider({ children, showSelection, allowSplitTransaction }) { + return ( + <TransactionContext.Provider value={{ showSelection, allowSplitTransaction }}> + {children} + </TransactionContext.Provider> + ); +}This would simplify prop passing and make the code more maintainable.
Also applies to: 2002-2003, 2028-2028, 2649-2650
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3828.mdis excluded by!**/*.md
📒 Files selected for processing (15)
packages/desktop-client/src/components/reports/Overview.tsx(5 hunks)packages/desktop-client/src/components/reports/ReportRouter.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reportRanges.ts(1 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts(1 hunks)packages/desktop-client/src/components/transactions/TransactionList.jsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(10 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(1 hunks)packages/desktop-client/src/style/themes/dark.ts(1 hunks)packages/desktop-client/src/style/themes/development.ts(1 hunks)packages/desktop-client/src/style/themes/light.ts(1 hunks)packages/desktop-client/src/style/themes/midnight.ts(1 hunks)packages/loot-core/src/types/models/dashboard.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/src/components/reports/reportRanges.ts
- packages/desktop-client/src/components/transactions/TransactionList.jsx
- packages/desktop-client/src/style/themes/dark.ts
- packages/desktop-client/src/style/themes/development.ts
- packages/desktop-client/src/style/themes/light.ts
- packages/desktop-client/src/style/themes/midnight.ts
- packages/loot-core/src/types/models/dashboard.d.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
Learnt from: lelemm
PR: actualbudget/actual#3828
File: packages/desktop-client/src/components/reports/reports/Calendar.tsx:575-631
Timestamp: 2024-11-12T18:18:07.283Z
Learning: In `Calendar.tsx`, transaction-related callbacks such as `onBatchDelete`, `onBatchDuplicate`, `onCreateRule`, and `onScheduleAction` are intentionally left as empty functions because these operations should not be usable on that page.
🔇 Additional comments (9)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (1)
1-17: LGTM! Well-structured imports and type definitions.
The imports are properly organized, and the CalendarDataType interface is well-defined with clear field names.
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (1)
64-68: 🛠️ Refactor suggestion
Add date validation in calculateTimeRange
The time range calculation doesn't validate the input dates. This could lead to unexpected behavior if meta?.timeFrame contains invalid values.
Consider adding date validation:
const [start, end] = calculateTimeRange(meta?.timeFrame, {
+ // Ensure valid dates or fallback to defaults
+ start: monthUtils.isValidDate(monthUtils.dayFromDate(monthUtils.currentMonth()))
+ ? monthUtils.dayFromDate(monthUtils.currentMonth())
+ : new Date(),
+ end: monthUtils.isValidDate(monthUtils.currentDay())
+ ? monthUtils.currentDay()
+ : new Date(),
- start: monthUtils.dayFromDate(monthUtils.currentMonth()),
- end: monthUtils.currentDay(),
mode: 'full',
});packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
73-86: LGTM! Clean and well-structured component.
The Calendar component properly handles loading states and provides good type safety.
417-421: LGTM! Well-structured constants.
Good practice extracting magic numbers into named constants for the drag gesture configuration.
853-924: LGTM! Well-structured responsive component.
The CalendarCardHeader component handles responsive layout well and properly formats financial data.
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (1)
Line range hint 1-1000: Well-structured and comprehensive test suite!
The test suite demonstrates excellent practices:
- Thorough setup with mock data and helper functions
- Comprehensive coverage of transaction operations
- Clear test descriptions and assertions
- Good isolation of test cases
- Proper cleanup after tests
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (3)
Line range hint 182-235: LGTM! Clean implementation of conditional selection in header
The implementation properly handles both selection and non-selection cases while maintaining layout consistency. The SelectCell component is well configured with appropriate styling and event handling.
886-887: LGTM! Well-integrated selection handling in Transaction component
The changes properly integrate selection handling while maintaining existing functionality. The selection cell rendering logic correctly handles all cases including previews and child transactions.
Also applies to: 1193-1203
1524-1524: LGTM! Good enhancement of split transaction control
The allowSplitTransaction prop is well utilized to provide granular control over split transaction functionality in the category autocomplete component.
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (13)
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (6)
34-43: Clean up commented code or document future plansThe commented
onFilterprop suggests an incomplete feature for filtering conditions. Either implement the filtering functionality, remove the commented code, or add a TODO comment explaining future plans.
64-71: Optimize font size calculationThe font size calculation could be memoized to prevent unnecessary updates. Consider using
useMemofor the calculation.- const buttonRef = useResizeObserver(rect => { + const buttonRef = useResizeObserver(rect => { + const calculateFontSize = (height: number) => { + const newValue = Math.floor(height / 2); + return newValue > 14 ? 14 : newValue; + }; + - const newValue = Math.floor(rect.height / 2); - if (newValue > 14) { - setFontSize(14); - } else { - setFontSize(newValue); - } + setFontSize(calculateFontSize(rect.height)); });
79-81: Extract grid layout constantsMagic numbers in the grid layout could make maintenance difficult. Consider extracting these values into named constants.
+const CALENDAR_GRID = { + COLUMNS: 7, + GAP: 2, +}; + <View style={{ color: theme.pageTextSubdued, display: 'grid', - gridTemplateColumns: 'repeat(7, 1fr)', + gridTemplateColumns: `repeat(${CALENDAR_GRID.COLUMNS}, 1fr)`, gridAutoRows: '1fr', - gap: 2, + gap: CALENDAR_GRID.GAP, }} >
120-177: Extract tooltip content into a separate componentThe tooltip content is complex and deeply nested. Consider extracting it into a separate component for better maintainability and reusability.
type DayTooltipProps = { day: { date: Date; incomeValue: number; expenseValue: number; incomeSize: number; expenseSize: number; }; }; function DayTooltip({ day }: DayTooltipProps) { const { t } = useTranslation(); return ( <View> <View style={{ marginBottom: 10 }}> <strong> {t('Day:') + ' '} {format(day.date, 'dd')} </strong> </View> {/* Rest of the tooltip content */} </View> ); }
219-225: Simplify state managementThe
currentFontSizestate and effect could be eliminated since it's just mirroring thefontSizeprop.- const [currentFontSize, setCurrentFontSize] = useState(fontSize); - - useEffect(() => { - setCurrentFontSize(fontSize); - }, [fontSize]); return ( <Button // ... <span style={{ - fontSize: `${currentFontSize}px`, + fontSize: `${fontSize}px`, fontWeight: 500, position: 'relative', }} >
242-292: Extract common styles and calculationsThe bar styles contain duplicated code and inline calculations. Consider extracting common styles and calculations into constants or utility functions.
const BAR_STYLES = { base: { position: 'absolute', width: '50%', height: '100%', opacity: 0.2, }, active: { position: 'absolute', width: '50%', opacity: 0.9, bottom: 0, transition: 'height 0.5s ease-out', }, }; const calculateBarHeight = (size: number) => `${Math.ceil(size)}%`;Then use these in your component:
- <View - style={{ - position: 'absolute', - width: '50%', - height: '100%', - background: chartTheme.colors.red, - opacity: 0.2, - right: 0, - }} - /> + <View + style={{ + ...BAR_STYLES.base, + background: chartTheme.colors.red, + right: 0, + }} + />packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (4)
69-79: Consider adding error boundaries for calendarSpreadsheet calculations.The
useMemohook wraps complex calculations that could potentially throw errors. Adding error boundaries would improve error handling and user experience.const params = useMemo( () => + try { calendarSpreadsheet( start, end, meta?.conditions, meta?.conditionsOp, firstDayOfWeekIdx, ), + } catch (error) { + console.error('Failed to calculate calendar data:', error); + return null; + } [start, end, meta?.conditions, meta?.conditionsOp, firstDayOfWeekIdx], );
198-248: Consider extracting tooltip content into a separate component.The tooltip content is complex enough to warrant its own component, which would improve readability and maintainability.
159-170: Consider using TypeScript discriminated unions for menu items.The current switch statement could benefit from TypeScript's discriminated unions to ensure type safety and exhaustive checking.
type MenuItem = | { name: 'rename'; text: string } | { name: 'remove'; text: string }; const handleMenuSelect = (item: MenuItem['name']) => { switch (item) { case 'rename': setNameMenuOpen(true); break; case 'remove': onRemove(); break; } };
487-493: Add keyboard navigation support for calendar day selection.The calendar day click handler should be accompanied by keyboard navigation support for better accessibility.
onDayClick={date => { navigate( isDashboardsFeatureEnabled ? `/reports/calendar/${widgetId}?day=${format(date, 'yyyy-MM-dd')}` : '/reports/calendar', ); }} +onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + // Handle day selection + } +}} +role="button" +tabIndex={0}packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
121-122: Consider making the default value more explicitThe fallback value for firstDayOfWeekIdx could be more explicitly defined with a named constant.
+const DEFAULT_FIRST_DAY_OF_WEEK = '0'; const [_firstDayOfWeekIdx] = useSyncedPref('firstDayOfWeekIdx'); -const firstDayOfWeekIdx = _firstDayOfWeekIdx || '0'; +const firstDayOfWeekIdx = _firstDayOfWeekIdx || DEFAULT_FIRST_DAY_OF_WEEK;
730-757: Consider extracting button stylesThe button styles could be extracted into a shared style object or component to improve maintainability.
+const calendarHeaderButtonStyle = { + color: theme.pageTextSubdued, + fontWeight: 'bold' as const, + fontSize: '14px', + margin: 0, + padding: 0, + display: 'inline-block', + width: 'max-content', +}; <Button variant="bare" - style={{ - color: theme.pageTextSubdued, - fontWeight: 'bold', - fontSize: '14px', - margin: 0, - padding: 0, - display: 'inline-block', - width: 'max-content', - }} + style={calendarHeaderButtonStyle} onPress={() => {
914-933: Consider using TypeScript enum for field mappingsThe field mapping could be more type-safe using a TypeScript enum or const object.
+const FIELD_MAPPINGS = { + account: 'account.name', + payee: 'payee.name', + category: 'category.name', + payment: 'amount', + deposit: 'amount', +} as const; + +type FieldKey = keyof typeof FIELD_MAPPINGS; + -function getField(field?: string) { +function getField(field?: FieldKey | string) { if (!field) { return 'date'; } - switch (field) { - case 'account': - return 'account.name'; - case 'payee': - return 'payee.name'; - case 'category': - return 'category.name'; - case 'payment': - return 'amount'; - case 'deposit': - return 'amount'; - default: - return field; - } + return FIELD_MAPPINGS[field as FieldKey] || field; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
Learnt from: lelemm
PR: actualbudget/actual#3828
File: packages/desktop-client/src/components/reports/reports/Calendar.tsx:575-631
Timestamp: 2024-11-12T18:18:07.283Z
Learning: In `Calendar.tsx`, transaction-related callbacks such as `onBatchDelete`, `onBatchDuplicate`, `onCreateRule`, and `onScheduleAction` are intentionally left as empty functions because these operations should not be usable on that page.
🔇 Additional comments (4)
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (1)
1-52: LGTM! Well-structured imports and type definitions.
The imports are logically grouped and the type definitions are comprehensive with proper TypeScript features.
packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
73-86: LGTM! Well-structured component with proper loading state handling.
The Calendar component is implemented correctly with:
- Proper TypeScript typing
- Loading state handling
- Clean props passing to CalendarInner
565-621: LGTM! Desktop transaction list implementation
The implementation correctly handles the desktop view with proper props configuration. Empty callbacks are intentionally left empty as these operations should not be usable on this page.
841-912: LGTM! Well-implemented responsive header
The CalendarCardHeader component is well structured with:
- Proper handling of zero values
- Responsive layout adaptation
- Clear TypeScript types
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I just noticed the table is missing the parent transaction Calendar: Ledger: |
any ideas how/why? |
Sorry, I'm not sure - it looks like the parent just isn't in the data |
Co-authored-by: Matt Fiddaman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (4)
51-51: Consider using a more specific type for firstDayOfWeekIdxThe type could be more strictly defined to only allow valid day indices.
- firstDayOfWeekIdx?: SyncedPrefs['firstDayOfWeekIdx']; + firstDayOfWeekIdx?: '0' | '1' | '2' | '3' | '4' | '5' | '6';
219-219: Use Trans component for consistent translationsThe translation strings should use the Trans component consistently.
- Income: + <Trans>Income:</Trans>Also applies to: 240-240
285-292: Extract width calculation logic into a separate functionThe width calculation logic is complex and would be more maintainable if extracted into a separate function.
+ const getCalendarWidth = (orientation: 'row' | 'column', isNarrow: boolean, length: number) => { + if (orientation !== 'row') return 'auto'; + if (isNarrow) return `${length * 100}%`; + if (length > 4) return `${100 + ((length - 4) % 4) * 25}%`; + return 'auto'; + }; <View style={{ // ...other styles - width: - cardOrientation === 'row' - ? isNarrowWidth - ? `${calendarLenSize * 100}%` - : calendarLenSize > 4 - ? `${100 + ((calendarLenSize - 4) % 4) * 25}%` - : 'auto' - : 'auto', + width: getCalendarWidth(cardOrientation, isNarrowWidth, calendarLenSize), }} >
97-111: Consider memoizing totalIncome and totalExpense separatelyThe current implementation recalculates both values even if only one is needed. Consider splitting them into separate memoized values for better performance.
- const { totalIncome, totalExpense } = useMemo(() => { + const totalIncome = useMemo(() => { if (!data) { - return { totalIncome: 0, totalExpense: 0 }; + return 0; } - return { - totalIncome: data.calendarData.reduce( - (prev, cur) => prev + cur.totalIncome, - 0, - ), - totalExpense: data.calendarData.reduce( - (prev, cur) => prev + cur.totalExpense, - 0, - ), - }; + return data.calendarData.reduce( + (prev, cur) => prev + cur.totalIncome, + 0, + ); - }, [data]); + }, [data]); + + const totalExpense = useMemo(() => { + if (!data) { + return 0; + } + return data.calendarData.reduce( + (prev, cur) => prev + cur.totalExpense, + 0, + ); + }, [data]);packages/desktop-client/src/components/reports/reports/Calendar.tsx (2)
429-461: Improve drag gesture implementation.The drag gesture implementation could be more robust with better bounds checking and cleanup.
Apply this diff to improve the implementation:
const bind = useDrag( ({ offset: [, oy], cancel }) => { if (oy < 0) { cancel(); api.start({ y: 0, immediate: true }); return; } - if (oy > totalHeight * 0.05 && mobileTransactionsOpen) { + const dragThreshold = totalHeight * 0.05; + if (oy > dragThreshold && mobileTransactionsOpen) { cancel(); close(); setMobileTransactionsOpen(false); - } else if (!mobileTransactionsOpen) { - if (oy / totalHeight > 0.05) { + } else if (!mobileTransactionsOpen && oy > dragThreshold) { cancel(); open({ canceled: true }); setMobileTransactionsOpen(true); - } else { - api.start({ y: oy, immediate: true }); - } + } else { + api.start({ y: oy, immediate: true }); } }, { from: () => [0, y.get()], filterTaps: true, bounds: { top: -totalHeight + CHEVRON_HEIGHT, bottom: totalHeight - CHEVRON_HEIGHT, }, axis: 'y', rubberband: true, }, ); +useEffect(() => { + return () => { + // Cleanup gesture bindings + bind.clean?.(); + }; +}, [bind]);
874-938: Add ARIA labels for better screen reader support.The summary values should have proper ARIA labels for better accessibility.
Apply this diff to improve screen reader support:
<View + role="region" + aria-label="Calendar summary" style={{ ...styles.smallText, marginLeft: isNarrowWidth ? 0 : 16, marginTop: isNarrowWidth ? 16 : 0, justifyContent: isNarrowWidth ? 'center' : 'flex-end', flexDirection: 'row', height: '100px', minWidth: '210px', }} > {/* ... */} <View + aria-label={`Income: ${amountToCurrency(totalIncome)}`} style={{ color: chartTheme.colors.blue }} > <PrivacyFilter>{amountToCurrency(totalIncome)}</PrivacyFilter> </View> <View + aria-label={`Expenses: ${amountToCurrency(totalExpense)}`} style={{ color: chartTheme.colors.red }} > <PrivacyFilter>{amountToCurrency(totalExpense)}</PrivacyFilter> </View>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
Learnt from: lelemm
PR: actualbudget/actual#3828
File: packages/desktop-client/src/components/reports/reports/Calendar.tsx:575-631
Timestamp: 2024-11-12T18:18:07.283Z
Learning: In `Calendar.tsx`, transaction-related callbacks such as `onBatchDelete`, `onBatchDuplicate`, `onCreateRule`, and `onScheduleAction` are intentionally left as empty functions because these operations should not be usable on that page.
🔇 Additional comments (6)
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (4)
63-67: Add validation for time range calculation
A previous review already identified this issue. The time range calculation uses default values without validation.
163-173: Improve menu error handling
A previous review already identified this issue. The current error handling for unrecognized menu items throws an error.
512-525: Add keyboard navigation support for calendar
A previous review already identified this issue. The calendar graph lacks keyboard navigation support.
432-455: Enhance accessibility for month navigation button
A previous review already identified this issue. The month navigation button lacks proper ARIA attributes.
packages/desktop-client/src/components/reports/reports/Calendar.tsx (2)
940-959: LGTM!
The utility function is well-implemented with proper field mapping and type checking.
729-752: 🛠️ Refactor suggestion
Enhance keyboard accessibility.
The calendar header should be keyboard accessible for better usability.
Apply this diff to improve accessibility:
<View
+ role="button"
+ tabIndex={0}
+ aria-label={`Calendar for ${format(calendar.start, 'MMMM yyyy')}`}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ onApplyFilter({
+ conditions: [...conditions.filter(f => f.field !== 'date')],
+ conditionsOp,
+ id: [],
+ });
+ }
+ }}
style={{
minWidth: '300px',
maxWidth: '300px',
padding: 10,
borderRadius: 4,
backgroundColor: theme.tableBackground,
}}
onClick={() =>
onApplyFilter({
conditions: [...conditions.filter(f => f.field !== 'date')],
conditionsOp,
id: [],
})
}
>Likely invalid or redundant comment.
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (3)
Line range hint
178-231: Improve conditional rendering in TransactionHeaderThe selection cell rendering logic can be simplified to reduce code duplication and improve readability.
Consider refactoring the conditional rendering:
- {showSelection && ( - <SelectCell - exposed={true} - focused={false} - selected={hasSelected} - width={20} - style={{ - borderTopWidth: 0, - borderBottomWidth: 0, - }} - onSelect={e => - dispatchSelected({ - type: 'select-all', - isRangeSelect: e.shiftKey, - }) - } - /> - )} - {!showSelection && ( - <Field - style={{ - width: '20px', - border: 0, - }} - /> - )} + {showSelection ? ( + <SelectCell + exposed={true} + focused={false} + selected={hasSelected} + width={20} + style={{ + borderTopWidth: 0, + borderBottomWidth: 0, + }} + onSelect={e => + dispatchSelected({ + type: 'select-all', + isRangeSelect: e.shiftKey, + }) + } + /> + ) : ( + <Field + style={{ + width: '20px', + border: 0, + }} + /> + )}
Line range hint
1176-1189: Improve selection cell rendering logicThe selection cell rendering logic in the Transaction component can be simplified for better readability.
Consider refactoring:
- ) : (isPreview && isChild) || !showSelection ? ( - <Cell width={20} /> - ) : ( + ) : showSelection && !(isPreview && isChild) ? ( <SelectCell /* Checkmark field for non-child transaction */ exposed buttonProps={{ className: selected || editing ? null : 'hover-visible', }} focused={focusedField === 'select'} onSelect={e => { dispatchSelected({ type: 'select', id: transaction.id, isRangeSelect: e.shiftKey, }); }} onEdit={() => onEdit(id, 'select')} selected={selected} style={{ ...(isChild && { borderLeftWidth: 1 }) }} value={ matched && ( <SvgHyperlink2 style={{ width: 13, height: 13, color: 'inherit' }} /> ) } /> + ) : ( + <Cell width={20} /> )}
Line range hint
1-2631: Add unit tests for new selection and split functionalityThe changes introduce new selection and split transaction features that should be covered by tests.
Would you like me to generate unit tests for:
- Selection visibility toggle
- Split transaction permission checks
- Selection state management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx(10 hunks)
🔇 Additional comments (3)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (3)
883-884: LGTM: New props added to Transaction component
The new props showSelection and allowSplitTransaction are correctly added to control selection visibility and split transaction functionality.
1782-1783: Ensure consistent prop propagation
The new props are correctly propagated through the component hierarchy from TransactionTable to child components.
Also applies to: 1904-1905, 1988-1989, 2014-2014, 2630-2631
1510-1510: Verify split transaction validation
The split transaction option is controlled by allowSplitTransaction, but there's no validation to prevent invalid splits.
Run the following script to check for existing validation:
fixed |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (2)
874-938: Enhance semantic structure for better accessibility.The header component should use more semantic HTML elements for better screen reader support.
Apply this diff to improve the semantic structure:
function CalendarCardHeader({ start, end, totalIncome, totalExpense, isNarrowWidth, }: CalendarCardHeaderProps) { return ( <View + role="complementary" + aria-label={t('Calendar Summary')} style={{ ...styles.smallText, marginLeft: isNarrowWidth ? 0 : 16, marginTop: isNarrowWidth ? 16 : 0, justifyContent: isNarrowWidth ? 'center' : 'flex-end', flexDirection: 'row', height: '100px', minWidth: '210px', }} > <View + role="group" + aria-label={t('Financial Summary')} style={{ width: '200px', borderRadius: 4, backgroundColor: theme.tableBackground, padding: 10, }} >
940-959: Improve type safety with TypeScript enums.The field mapping could be more type-safe using TypeScript enums or constants.
Consider this improvement:
+const enum FieldMapping { + Account = 'account', + Payee = 'payee', + Category = 'category', + Payment = 'payment', + Deposit = 'deposit', +} + +const FIELD_MAPPINGS: Record<FieldMapping, string> = { + [FieldMapping.Account]: 'account.name', + [FieldMapping.Payee]: 'payee.name', + [FieldMapping.Category]: 'category.name', + [FieldMapping.Payment]: 'amount', + [FieldMapping.Deposit]: 'amount', +}; -function getField(field?: string) { +function getField(field?: FieldMapping | string) { if (!field) { return 'date'; } - switch (field) { - case 'account': - return 'account.name'; - case 'payee': - return 'payee.name'; - case 'category': - return 'category.name'; - case 'payment': - return 'amount'; - case 'deposit': - return 'amount'; - default: - return field; - } + return FIELD_MAPPINGS[field as FieldMapping] || field; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
Learnt from: lelemm
PR: actualbudget/actual#3828
File: packages/desktop-client/src/components/reports/reports/Calendar.tsx:575-631
Timestamp: 2024-11-12T18:18:07.283Z
Learning: In `Calendar.tsx`, transaction-related callbacks such as `onBatchDelete`, `onBatchDuplicate`, `onCreateRule`, and `onScheduleAction` are intentionally left as empty functions because these operations should not be usable on that page.
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
…rCard.tsx Co-authored-by: Matt Fiddaman <[email protected]>
A new report type where data shows in a calendar format`:
Actual.-.Google.Chrome.2024-11-12.13-38-51.mp4