Summary report #3792
Summary report #3792
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 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
|
|
Added those:
|
WalkthroughThe pull request introduces several updates across multiple components within the desktop client. A new Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@lelemm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (1.23.1)
packages/desktop-client/src/components/reports/SummaryNumber.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. 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: 12
🧹 Outside diff range and nitpick comments (11)
packages/desktop-client/src/components/reports/ReportRouter.tsx (1)
Line range hint
1-27: Consider adding integration tests and documentation.Since this is part of a larger feature for summary reports, consider:
- Adding integration tests to verify the routing behavior with the Summary component
- Updating documentation to reflect the new report type
- Ensuring the navigation menu or dashboard properly exposes these new routes
packages/desktop-client/src/components/reports/SummaryNumber.tsx (6)
13-17: Fix typo in prop name: 'sufix' should be 'suffix'The property name contains a spelling error that should be corrected for better maintainability and clarity.
type AnimatedNumberProps = { value: number; animate?: boolean; - sufix?: string; + suffix?: string; };
19-27: Update prop name in component parametersFollowing the type definition correction, update the prop name in the component parameters.
export function SummaryNumber({ value, animate = true, - sufix = '', + suffix = '', }: AnimatedNumberProps) {
28-44: Enhance font size adjustment algorithmThe current implementation has potential performance and usability concerns:
- The incremental approach with 0.5px steps could be inefficient for large containers
- No maximum font size limit could lead to excessive text sizes
- Missing error handling for edge cases (e.g., very small containers)
Consider implementing these improvements:
const adjustFontSize = (containerWidth: number, containerHeight: number) => { if (!offScreenRef.current) return; + const MAX_FONT_SIZE = 100; // Add reasonable maximum let testFontSize = 14; const offScreenDiv = offScreenRef.current; offScreenDiv.style.fontSize = `${testFontSize}px`; while ( offScreenDiv.scrollWidth <= containerWidth && - offScreenDiv.scrollHeight <= containerHeight + offScreenDiv.scrollHeight <= containerHeight && + testFontSize <= MAX_FONT_SIZE ) { - testFontSize += 0.5; + testFontSize += Math.max(0.5, testFontSize * 0.1); // Adaptive step size offScreenDiv.style.fontSize = `${testFontSize}px`; } + // Step back once to ensure we're not overflowing + testFontSize = Math.max(14, testFontSize - 0.5); setFontSize(testFontSize); };
46-51: Consider increasing debounce timeoutThe current 10ms debounce timeout might be too aggressive, potentially causing unnecessary recalculations during resize operations.
const handleResize = debounce((rect: DOMRectReadOnly) => { adjustFontSize(rect.width, rect.height); -}, 10); +}, 100); // Increase to reduce unnecessary calculations while maintaining responsiveness
66-68: Update 'sufix' usage in JSX elementsUpdate the misspelled prop name in the JSX elements to match the corrected type definition.
{amountToCurrency(value)} -{sufix} +{suffix}Also applies to: 83-86
70-82: Enhance color accessibilityThe current color scheme might not provide sufficient contrast for all users. Consider adding ARIA attributes and ensuring the color contrast meets WCAG guidelines.
<View ref={mergedRef as Ref<HTMLDivElement>} + role="text" + aria-label={`${value < 0 ? 'Negative' : 'Positive'} amount: ${amountToCurrency(Math.abs(value))}${suffix}`} style={{ alignItems: 'center', height: '100%', width: '100%', fontSize: `${fontSize}px`, justifyContent: 'center', transition: animate ? 'font-size 0.3s ease' : '', - color: value < 0 ? chartTheme.colors.red : chartTheme.colors.blue, + color: value < 0 ? chartTheme.colors.red : chartTheme.colors.blue, + fontWeight: 'bold', // Improve readability }} >packages/loot-core/src/types/models/dashboard.d.ts (1)
104-109: Consider adding JSDoc comments for better documentation.The SummaryContent type is well-structured and aligns with the PR objectives for different calculation modes. However, adding JSDoc comments would improve documentation, especially for the calculation types and divisor-related fields.
Consider adding documentation like this:
+/** + * Defines the structure for summary calculations + * @property type - The calculation mode: + * - 'sum': Total sum of filtered values + * - 'avgPerMonth': Monthly average of filtered values + * - 'avgPerTransact': Per-transaction average + * - 'percentage': Ratio between two filtered sets of values + * @property divisorConditions - Filter conditions for the denominator in percentage calculations + * @property divisorConditionsOp - Operator for combining divisor conditions + * @property divisorIncludeDateRange - Whether to apply the date range to divisor calculations + */ export type SummaryContent = { type: 'sum' | 'avgPerMonth' | 'avgPerTransact' | 'percentage'; divisorConditions?: RuleConditionEntity[]; divisorConditionsOp?: 'and' | 'or'; divisorIncludeDateRange?: boolean; };packages/desktop-client/src/components/reports/Overview.tsx (1)
558-565: Consider performance optimization strategies.Given that the summary card performs calculations on filtered data, consider these architectural improvements:
- Implement caching for calculated values to prevent unnecessary recalculations.
- Add performance monitoring to track the impact of calculations on dashboard performance.
- Consider implementing pagination or lazy loading if dealing with large datasets.
packages/desktop-client/src/components/reports/reports/Summary.tsx (2)
271-271: Correct the typo in the 'sufix' propThe prop
sufixis misspelled and should besuffix. This correction ensures that the suffix displays correctly in theSummaryNumbercomponent.Suggested change:
- sufix={content.type === 'percentage' ? '%' : ''} + suffix={content.type === 'percentage' ? '%' : ''}
355-365: Associate the checkbox with a label for better accessibilityCurrently, the checkbox is not associated with a label, which can affect accessibility. Wrap the checkbox and its text in a
<label>element to ensure proper association.Suggested change:
-<Checkbox - id="enabled-field" - checked={content.divisorIncludeDateRange ?? true} - onChange={() => - setContent(prev => ({ - ...prev, - divisorIncludeDateRange: !content.divisorIncludeDateRange, - })) - } -/>{' '} -Include summary date range +<label style={{ display: 'flex', alignItems: 'center' }}> + <Checkbox + id="enabled-field" + checked={content.divisorIncludeDateRange ?? true} + onChange={() => + setContent(prev => ({ + ...prev, + divisorIncludeDateRange: !content.divisorIncludeDateRange, + })) + } + /> + <span style={{ marginLeft: 8 }}>Include summary date range</span> +</label>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3792.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/reports/Overview.tsx(3 hunks)packages/desktop-client/src/components/reports/ReportRouter.tsx(2 hunks)packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts(1 hunks)packages/loot-core/src/types/models/dashboard.d.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts
[error] 87-87: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
[error] 106-106: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (7)
packages/desktop-client/src/components/reports/ReportRouter.tsx (2)
9-9: LGTM! Import statement follows conventions.
The Summary component import follows the established pattern and maintains alphabetical ordering with other report imports.
23-24: LGTM! Routes follow established pattern. Verify route parameter handling.
The new routes follow the consistent pattern used for other report types. However, let's verify that the Summary component properly handles both routes, especially the :id parameter.
✅ Verification successful
Let me try a different approach to verify the route parameter handling.
Based on the output, I can now generate the final response since we found that the Summary component does handle the route parameter correctly using useParams().
✅ Route parameter handling verified and implemented correctly
The Summary component properly handles both routes by using useParams() to access the optional ID parameter, consistent with React Router patterns. The implementation shows:
- Uses
params.id ?? ''pattern to handle both routes - Integrates with
useWidgethook for data fetching - Follows loading state management pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Summary component handles both route patterns correctly
# Test 1: Check if Summary component handles the ID parameter
echo "Checking Summary component for route parameter handling..."
ast-grep --pattern 'function Summary() {
$$$
const { id } = useParams();
$$$
}' packages/desktop-client/src/components/reports/reports/Summary.tsx
# Test 2: Verify consistent route parameter handling across all report components
echo "Checking route parameter handling consistency across report components..."
ast-grep --pattern 'const { id } = useParams()' packages/desktop-client/src/components/reports/reports/
Length of output: 544
Script:
#!/bin/bash
# Let's examine the Summary component implementation and other report components
echo "Checking Summary component implementation..."
rg -A 10 "^export.*function Summary" packages/desktop-client/src/components/reports/reports/Summary.tsx
echo "\nChecking other report components for comparison..."
rg -A 10 "^export.*function.*\(" packages/desktop-client/src/components/reports/reports/ --type tsx
Length of output: 675
packages/loot-core/src/types/models/dashboard.d.ts (2)
68-69: LGTM: SummaryWidget correctly added to union type.
The addition of SummaryWidget to the SpecializedWidget union type is properly implemented and maintains consistent formatting.
93-102: LGTM: SummaryWidget type definition is well-structured.
The type definition follows the established pattern of other widget types and includes all necessary fields for summary configuration. Let's verify that all referenced types exist and are properly imported.
✅ Verification successful
The verification shows that both referenced types exist and are properly defined in the codebase:
TimeFrameis defined in the same file (dashboard.d.ts) with all required fieldsRuleConditionEntityis defined inrule.d.tsas a union type based onBaseConditionEntity
Let me verify the AbstractWidget type as well to complete the verification:
All type dependencies for SummaryWidget are properly defined and accessible
The verification confirms that all referenced types are well-defined and properly located:
AbstractWidget<T, Meta>is defined in the same file with correct generic parametersTimeFrameis defined in the same file with all required fieldsRuleConditionEntityis defined inrule.d.tsand properly exported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all referenced types exist
echo "Checking for TimeFrame type definition..."
rg "type\s+TimeFrame\s*=" -A 5
echo "Checking for RuleConditionEntity type definition..."
rg "type\s+RuleConditionEntity\s*=" -A 5 || rg "interface\s+RuleConditionEntity\s*" -A 5
Length of output: 1092
Script:
#!/bin/bash
# Verify AbstractWidget type definition
rg "type\s+AbstractWidget\s*[<=]" -A 5 || rg "interface\s+AbstractWidget\s*" -A 5
Length of output: 465
packages/desktop-client/src/components/reports/Overview.tsx (2)
43-43: LGTM!
The import statement follows the established pattern for report card components.
558-565: Consider enhancing type safety and adjusting default dimensions.
The SummaryCard integration looks good, but consider these improvements:
- Define specific types for the meta prop to ensure type safety.
- Review if the default widget dimensions (width: 4, height: 2) in
onAddWidgetare suitable for summary content.
#!/bin/bash
# Description: Check for type definitions related to SummaryCard
# Expected: Find type definitions for SummaryCard props and meta
# Search for SummaryCard type definitions
rg -t typescript "type.*Summary(Card|Widget|Content)" .packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (1)
153-213: Confirm divisor conditions are correctly handled
Ensure that the divisorConditions and divisorConditionsOp in summaryContent are correctly defined and not empty to prevent unexpected results in percentage calculations. Missing or incorrect configurations could lead to division by zero or inaccurate percentages.
Run the following script to check for transactions matching the divisor conditions:
If the divisor conditions are empty or incorrect, consider adding validation or default values.
✅ Verification successful
Let me gather more information about the validation and handling of these conditions.
Based on the analysis of the code, I can now provide a final response:
Divisor conditions are properly handled with fallback safeguards
The implementation includes several safety measures:
- Optional chaining is used (
summaryContent?.divisorConditions?.filter) - Division by zero is prevented by returning
{ total: 0 }whendivisorValue === 0 - Non-custom conditions are filtered out before processing
- The conditions operation (
or/and) has a proper fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that divisor conditions yield valid transactions.
# Test: Search for transactions matching the divisor conditions. Expect: At least one matching transaction.
rg -A 5 $'divisorConditions'
Length of output: 4647
Script:
#!/bin/bash
# Check how divisorConditions are validated and handled
ast-grep --pattern 'summaryContent?.divisorConditions'
# Check the implementation of make-filters-from-conditions
rg -A 10 "make-filters-from-conditions"
# Check for any default values or validation
ast-grep --pattern $'interface SummaryContent {
$$$
divisorConditions$_
$$$
}'
Length of output: 16856
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (1)
73-77: Optimize 'select' clause by removing 'date' when not groupingIn the
makeRootQueryfunction, you include'date'in theselectclause even when the query is not grouped by'date'. This may result in unnecessary data being fetched. Consider conditionally including'date'in theselectonly when grouping by date.Apply this diff to conditionally include
'date'in the select clause:.select([ - 'date', + ...(summaryContent.type === 'avgPerMonth' ? ['date'] : []), { amount: { $sum: '$amount' } }, { count: { $count: '*' } }, ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/reports/SummaryNumber.tsx
- packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
🔇 Additional comments (2)
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (1)
195-195:
Ensure error handling for 'runQuery' in 'calculatePercentage'
In calculatePercentage, the call to await runQuery(query); may result in an error if the query fails. To prevent unhandled exceptions, consider wrapping this call in a try-catch block.
Apply this diff to add error handling:
let divisorData;
try {
divisorData = (await runQuery(query)) as { data: { amount: number }[] };
+ } catch (error) {
+ // Handle the error appropriately
+ console.error('Error executing divisor query:', error);
+ return { total: 0 };
+ }Likely invalid or redundant comment.
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
155-158: Handle undefined 'widget' without throwing an error
Throwing an error when widget is undefined can lead to unhandled promise rejections in an async function. Instead, handle this case gracefully by notifying the user and preventing further execution.
Also applies to: 177-179
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
|
I've had a quick play since I saw it discussed in discord. I'm not seeing any transaction list pop up. I think mode should show up above the big figure so all the options are together. Some more info needed around the mode to be clearer what it's displaying or how it's calculated. I'm not sure what to make of the percentage calculation (live budget used, the payee is <$5/week), I think it's doing the reverse calculation of what I'm thinking? That is it's showing my income as a percentage of this payee, not this payee as a percentage of my income? Or is it a percentage of something else? Also not sure why it'd be red.:
|
it was removed to open space for the divisor filter
if it was on the footer so the number gets centered on the screen. If a lot of filters is made on the top, they keep pushing the number down and smaller.
it can't be called NET, because its just a sum of the filters allied on the top of the page. If the users select > 0, it just incomes for example
you applied the payee filter on the bottom. So basically what this mean is: this way it will be sum of every transaction for payee / sum of all income |
lelemm
force-pushed
the
summary_card
branch
2 times, most recently
from
3104c93
to
1f6a772
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
19-27: Add JSDoc documentation for the component.While the implementation is solid, adding documentation would improve maintainability and help other developers understand the component's purpose, props usage, and behavior.
Add documentation like this:
+/** + * Displays a formatted number that automatically scales to fit its container. + * @param {number} value - The number to display + * @param {boolean} [animate=true] - Whether to animate font size changes + * @param {string} [suffix=''] - Optional suffix to append to the number + * @returns A responsive number display that fills its container + */ export function SummaryNumber({
28-44: Optimize font size adjustment algorithm.The current implementation could be more efficient:
- The while loop might run many times with 0.5px increments
- There's no upper limit on the font size
- Binary search could be more efficient than linear search
Consider this optimization:
const adjustFontSize = (containerWidth: number, containerHeight: number) => { if (!offScreenRef.current) return; - let testFontSize = 14; + let minSize = 14; + let maxSize = 100; // Reasonable upper limit const offScreenDiv = offScreenRef.current; - offScreenDiv.style.fontSize = `${testFontSize}px`; - while ( - offScreenDiv.scrollWidth <= containerWidth && - offScreenDiv.scrollHeight <= containerHeight - ) { - testFontSize += 0.5; - offScreenDiv.style.fontSize = `${testFontSize}px`; - } + while (maxSize - minSize > 1) { + const mid = Math.floor((minSize + maxSize) / 2); + offScreenDiv.style.fontSize = `${mid}px`; + + if (offScreenDiv.scrollWidth <= containerWidth && + offScreenDiv.scrollHeight <= containerHeight) { + minSize = mid; + } else { + maxSize = mid; + } + } - setFontSize(testFontSize); + setFontSize(minSize); };packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (3)
13-23: Consider splitting the function into smaller, more focused componentsThe function handles multiple types of summaries with different logic flows. Consider extracting each summary type calculation into its own function for better maintainability and testing.
Example structure:
export function summarySpreadsheet( start: string, end: string, conditions: RuleConditionEntity[] = [], conditionsOp: 'and' | 'or' = 'and', summaryContent: SummaryContent, ) { const calculators = { sum: calculateSum, avgPerTransact: calculateAvgPerTransaction, avgPerMonth: calculateAvgPerMonth, percentage: calculatePercentage }; return async (spreadsheet, setData) => { const calculator = calculators[summaryContent.type]; if (!calculator) { throw new Error(`Unsupported summary type: ${summaryContent.type}`); } return calculator(spreadsheet, setData, { start, end, conditions, conditionsOp, summaryContent }); }; }
139-166: Optimize monthly calculations performanceThe current implementation iterates through the data multiple times. Consider using a single pass approach:
function calculatePerMonth( data: Array<{ date: string; amount: number; count: number; }>, months: Date[], ) { - const monthsSum = months.map(m => { - const currentMonthData = data.filter(day => - d.isSameMonth(d.parse(day.date, 'yyyy-MM-dd', new Date()), m), - ); - - return currentMonthData.reduce( - (prev, actual) => ({ - amount: prev.amount + actual.amount, - }), - { amount: 0 }, - ); - }); + const monthsSum = new Map(); + months.forEach(m => monthsSum.set(d.format(m, 'yyyy-MM'), { amount: 0 })); + + data.forEach(day => { + const monthKey = d.format(d.parse(day.date, 'yyyy-MM-dd', new Date()), 'yyyy-MM'); + const monthData = monthsSum.get(monthKey); + if (monthData) { + monthData.amount += day.amount; + } + }); const totalAmount = monthsSum.reduce((sum, month) => sum + month.amount, 0); const averageAmountPerMonth = totalAmount / months.length; return { total: averageAmountPerMonth / 100, }; }
168-228: Add documentation for percentage calculation logicThe percentage calculation logic is complex and would benefit from documentation explaining:
- The purpose of the 10000 multiplier
- The rounding logic
- The meaning of divisorIncludeDateRange flag
Add JSDoc comment:
/** * Calculates the percentage of transactions amount relative to a divisor amount. * @param data - Array of transaction amounts to sum * @param summaryContent - Configuration for the divisor calculation * @param startDay - Start date for filtering transactions * @param endDay - End date for filtering transactions * @returns Object containing the calculated percentage with 2 decimal places * * The percentage is calculated as: (sum of transactions / divisor amount) * 100 * The result is rounded to 2 decimal places using the multiplier method */packages/desktop-client/src/components/reports/reports/SummaryCard.tsx (3)
70-71: Add loading and error handling for report dataCurrently, the component does not handle loading and error states when fetching report data with
useReport. Consider adding proper loading indicators and error messages to improve user experience.
107-107: Simplify padding styles for claritySetting
padding: 20and then overridingpaddingBottom: 0can be simplified. Consider usingpaddingHorizontal: 20andpaddingTop: 20for clarity.Apply this diff to simplify the padding styles:
- <View style={{ flexDirection: 'row', padding: 20, paddingBottom: 0 }}> + <View style={{ flexDirection: 'row', paddingHorizontal: 20, paddingTop: 20 }}>
108-108: Avoid using negative margins in stylesUsing negative margins (
marginBottom: -5on line 108 andmarginTop: -20on line 128) might lead to unexpected layout behavior and reduce maintainability. Consider adjusting the layout or using alternative styling approaches to achieve the desired effect without negative margins.Also applies to: 126-131
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
318-321: Use constants or enums for 'content.type' valuesTo enhance maintainability and reduce the risk of typos, consider defining constants or an enum for the possible values of
content.typeinstead of using hardcoded strings throughout the code.Example:
enum SummaryType { Sum = 'sum', AvgPerMonth = 'avgPerMonth', AvgPerTransaction = 'avgPerTransact', Percentage = 'percentage', }Update the usages accordingly:
- ['sum', 'Sum'], - ['avgPerMonth', 'Average per month'], - ['avgPerTransact', 'Average per transaction'], - ['percentage', 'Percentage'], + [SummaryType.Sum, 'Sum'], + [SummaryType.AvgPerMonth, 'Average per month'], + [SummaryType.AvgPerTransaction, 'Average per transaction'], + [SummaryType.Percentage, 'Percentage'],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3792.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/reports/Overview.tsx(3 hunks)packages/desktop-client/src/components/reports/ReportRouter.tsx(2 hunks)packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.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 (2)
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/reports/ReportRouter.tsx
🔇 Additional comments (10)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
1-17: LGTM! Well-structured imports and type definitions.
The imports are well-organized and the type definition for AnimatedNumberProps is clear and properly typed.
1-90: Verify handling of edge cases for summary calculations.
Given that this component will display various types of calculations (averages, ratios, etc.), please verify:
- Handling of zero values (especially for division operations)
- Very large or small numbers that might affect font scaling
- Different currency formats
packages/loot-core/src/types/models/dashboard.d.ts (1)
68-69: LGTM!
The addition of SummaryWidget to the SpecializedWidget union type is correct and maintains consistent formatting.
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (2)
1-11: LGTM! Well-organized imports with clear separation of utilities and types
The imports are properly structured and all dependencies appear to be necessary for the implementation.
64-93: Add validation for query filters
The query construction assumes that filters will always be valid. Consider adding validation to ensure that:
- At least one filter is present when required
- Filter conditions are properly formatted
✅ Verification successful
Filter Validation is Adequate
No additional validation is required for query filters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential filter validation issues
ast-grep --pattern 'filter({
$$$
})' | grep -v '\$and\|\$or' || echo "No unvalidated filters found"
Length of output: 131
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx (2)
43-54: Good error handling when parsing meta.content
Great job implementing a try...catch block to handle potential exceptions when parsing meta.content. This enhances the robustness of the component and prevents runtime errors due to malformed JSON.
135-135: Confirmed correction of suffix prop
The prop suffix is now correctly spelled, ensuring the SummaryNumber component displays the suffix properly.
packages/desktop-client/src/components/reports/reports/Summary.tsx (3)
168-171: Handle undefined 'widget' without throwing an error
Throwing an error when widget is undefined in an async function could lead to unhandled promise rejections. Consider handling this case gracefully by notifying the user and preventing further execution.
190-192: Avoid throwing errors in async functions when 'widget' is undefined
Similar to the previous comment, throwing an error here can cause unhandled promise rejections. Notify the user instead and halt the execution of the function.
379-386: Toggle logic for 'divisorIncludeDateRange' is correctly implemented
The toggle functionality for divisorIncludeDateRange correctly accounts for undefined values by defaulting to true. The current implementation ensures proper toggling behavior.
packages/desktop-client/src/components/reports/SummaryNumber.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/types/models/dashboard.d.ts (1)
104-115: Add JSDoc comments to improve type documentation.While the type definitions are well-structured, they would benefit from JSDoc comments explaining:
- The purpose of each summary type
- The calculation method for each mode
- The meaning and usage of divisor-related fields
Example:
+/** + * Base content type for summary widgets that perform simple calculations + * @property type - The calculation mode: + * - 'sum': Total of all matching transactions + * - 'avgPerMonth': Monthly average of matching transactions + * - 'avgPerTransact': Average amount per transaction + */ export type BaseSummaryContent = { type: 'sum' | 'avgPerMonth' | 'avgPerTransact'; };packages/desktop-client/src/components/reports/reports/Summary.tsx (2)
71-114: Consider abstracting filter logic to reduce duplicationThe useFilters hook is used twice with similar patterns for both main and divisor conditions. This could be abstracted into a custom hook.
Consider creating a custom hook:
function useDualFilters(mainConditions, mainOp, divisorConditions, divisorOp) { const main = useFilters(mainConditions ?? [], mainOp ?? 'and'); const divisor = useFilters(divisorConditions ?? [], divisorOp ?? 'and'); return { main, divisor }; }🧰 Tools
🪛 GitHub Check: typecheck
[failure] 112-112:
Property 'divisorConditions' does not exist on type 'SummaryContent'.
[failure] 113-113:
Property 'divisorConditionsOp' does not exist on type 'SummaryContent'.🪛 GitHub Check: lint
[warning] 79-79:
Replace·widget?.meta?.conditionsOp·??·'and'with⏎····widget?.meta?.conditionsOp·??·'and',⏎··
378-391: Simplify checkbox toggle logicThe current implementation of the checkbox toggle is more complex than necessary.
Simplify the toggle logic:
<Checkbox id="enabled-field" checked={content.divisorIncludeDateRange ?? true} - onChange={() => { - const currentValue = - content.divisorIncludeDateRange ?? true; - setContent(prev => ({ - ...prev, - divisorIncludeDateRange: !currentValue, - })); - }} + onChange={() => + setContent(prev => ({ + ...prev, + divisorIncludeDateRange: !(prev.divisorIncludeDateRange ?? true), + })) + } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)packages/loot-core/src/types/models/dashboard.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/reports/SummaryNumber.tsx
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/reports/reports/Summary.tsx
[failure] 112-112:
Property 'divisorConditions' does not exist on type 'SummaryContent'.
[failure] 113-113:
Property 'divisorConditionsOp' does not exist on type 'SummaryContent'.
[failure] 328-328:
Argument of type '(prev: SummaryContent) => { type: "percentage" | "sum" | "avgPerMonth" | "avgPerTransact"; } | { type: "percentage" | "sum" | "avgPerMonth" | "avgPerTransact"; divisorConditions: RuleConditionEntity[]; divisorConditionsOp: "and" | "or"; divisorIncludeDateRange?: boolean; }' is not assignable to parameter of type 'SetStateAction'.
🪛 GitHub Check: lint
packages/desktop-client/src/components/reports/reports/Summary.tsx
[warning] 79-79:
Replace ·widget?.meta?.conditionsOp·??·'and' with ⏎····widget?.meta?.conditionsOp·??·'and',⏎··
🔇 Additional comments (4)
packages/loot-core/src/types/models/dashboard.d.ts (3)
68-69: LGTM: Union type update is correct.
The addition of SummaryWidget to the SpecializedWidget union type is consistent with the existing pattern.
93-102: Content field type needs updating.
The existing review comment about updating the content field type from string to SummaryContent is still valid. This change is essential for proper type safety.
Line range hint 3-9: Verify TimeFrame compatibility with summary features.
Let's verify that the existing TimeFrame type supports all the required date range scenarios for summary calculations, especially for the percentage mode where we have divisorIncludeDateRange.
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
1-56: LGTM! Well-structured component initialization
The component structure demonstrates good separation of concerns with proper loading state management and type definitions.
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/reports/Summary.tsx (1)
327-345: Improve accessibility for the mode selectorThe mode selector lacks proper accessibility attributes.
- <span style={{ marginRight: 4 }}>Mode</span> + <label + htmlFor="mode-select" + style={{ marginRight: 4 }} + > + Mode + </label> <FieldSelect + id="mode-select" + aria-label="Select summary mode" style={{ marginRight: 16 }} fields={[ ['sum', 'Sum'],🧰 Tools
🪛 GitHub Check: typecheck
[failure] 340-340:
Argument of type '(prev: SummaryContent) => { type: "percentage" | "sum" | "avgPerMonth" | "avgPerTransact"; } | { type: "percentage" | "sum" | "avgPerMonth" | "avgPerTransact"; divisorConditions: RuleConditionEntity[]; divisorConditionsOp: "and" | "or"; divisorIncludeDateRange?: boolean; }' is not assignable to parameter of type 'SetStateAction'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/reports/reports/Summary.tsx
[failure] 112-112:
Property 'divisorConditions' does not exist on type 'SummaryContent'.
[failure] 113-113:
Property 'divisorConditionsOp' does not exist on type 'SummaryContent'.
[failure] 340-340:
Argument of type '(prev: SummaryContent) => { type: "percentage" | "sum" | "avgPerMonth" | "avgPerTransact"; } | { type: "percentage" | "sum" | "avgPerMonth" | "avgPerTransact"; divisorConditions: RuleConditionEntity[]; divisorConditionsOp: "and" | "or"; divisorIncludeDateRange?: boolean; }' is not assignable to parameter of type 'SetStateAction'.
🪛 GitHub Check: lint
packages/desktop-client/src/components/reports/reports/Summary.tsx
[warning] 79-79:
Replace ·widget?.meta?.conditionsOp·??·'and' with ⏎····widget?.meta?.conditionsOp·??·'and',⏎··
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx (4)
92-104: Improve type safety of menu handling with TypeScript discriminated unions.The menu item handling could benefit from stronger typing to prevent future maintenance issues.
Consider defining a discriminated union type for menu items:
type MenuItem = | { name: 'rename'; text: string } | { name: 'remove'; text: string }; // Then use it in onMenuSelect onMenuSelect={(item: MenuItem['name']) => {
77-81: Extract route construction logic.The navigation URL construction logic could be extracted to improve maintainability.
Consider creating a utility function:
const getSummaryRoute = (isDashboardsEnabled: boolean, widgetId: string) => isDashboardsEnabled ? `/reports/summary/${widgetId}` : '/reports/summary'; // Then use it in the component to={getSummaryRoute(isDashboardsFeatureEnabled, widgetId)}
110-110: Consider making the default name configurable.The default name "Summary" is hardcoded in the translation key, which might make it difficult to change across the application.
Consider defining it as a constant:
const DEFAULT_SUMMARY_NAME = 'Summary'; // Then use it in the component name={meta?.name || t(DEFAULT_SUMMARY_NAME)}
43-56: Document supported summary content types.The PR objectives mention multiple modes for data presentation (average per transaction, average per month, ratio calculations), but these modes are not documented in the code.
Consider adding JSDoc comments to document the supported content types:
/** * Supported content types for summary calculation: * - 'sum': Simple sum of filtered values * - 'average_per_transaction': Average value per transaction * - 'average_per_month': Average value per month * - 'percentage': Ratio calculation between two filters */ const content = useMemo(...)packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (4)
13-23: Consider breaking down the function for better maintainabilityThe
summarySpreadsheetfunction handles multiple responsibilities (filtering, querying, calculations). Consider extracting the summary type handlers into separate functions for better maintainability and testing.Example refactor:
type SummaryHandler = (data: QueryResult, params: HandlerParams) => { total: number }; const handlers: Record<SummaryContent['type'], SummaryHandler> = { sum: handleSum, avgPerTransact: handleAvgPerTransaction, avgPerMonth: handleAvgPerMonth, percentage: handlePercentage, };
21-23: Improve type safety for spreadsheet parameterThe type
ReturnType<typeof useSpreadsheet>could be made more explicit by defining a proper interface for the spreadsheet object.interface Spreadsheet { // Define the actual shape of the spreadsheet object } function summarySpreadsheet( // ...other params ) { return async ( spreadsheet: Spreadsheet, setData: (data: { total: number }) => void, ) => {
168-234: Enhance error handling and query constructionThe function has good zero divisor handling but could benefit from:
- More robust error handling for the query execution
- Memoization of the filter generation for performance
async function calculatePercentage( data: Array<{ amount: number }>, summaryContent: SummaryContent, startDay: Date, endDay: Date, ) { + try { if (summaryContent.type !== 'percentage') { return { total: 0 }; } const conditionsOpKey = summaryContent.divisorConditionsOp === 'or' ? '$or' : '$and'; - const { filters } = await send('make-filters-from-conditions', { + const { filters } = await memoizedMakeFilters({ conditions: summaryContent?.divisorConditions?.filter( cond => !cond.customName, ), }); // ... rest of the function + } catch (error) { + console.error('Error calculating percentage:', error); + return { total: 0 }; + } }
1-234: Consider architectural improvements for better scalabilityWhile the implementation is solid, consider these architectural improvements:
- Implement a caching mechanism for frequently accessed calculations
- Add a proper logging system instead of console.error
- Consider implementing an event system to notify UI of calculation progress for long-running operations
Would you like assistance in implementing any of these architectural improvements?
packages/desktop-client/src/components/reports/reports/Summary.tsx (4)
39-51: Consider implementing an error boundary.The component would benefit from error boundary implementation to gracefully handle runtime errors that might occur during widget data fetching or rendering.
+class SummaryErrorBoundary extends React.Component { + state = { hasError: false }; + + static getDerivedStateFromError() { + return { hasError: true }; + } + + render() { + if (this.state.hasError) { + return ( + <View style={{ padding: 20 }}> + <Trans>Failed to load summary. Please try refreshing the page.</Trans> + </View> + ); + } + return this.props.children; + } +} export function Summary() { const params = useParams(); const { data: widget, isLoading } = useWidget<SummaryWidget>( params.id ?? '', 'summary-card', ); if (isLoading) { return <LoadingIndicator />; } - return <SummaryInner widget={widget} />; + return ( + <SummaryErrorBoundary> + <SummaryInner widget={widget} /> + </SummaryErrorBoundary> + ); }
57-70: Consider extracting date range initialization logic.The date range initialization logic could be moved to a custom hook for better reusability and testing.
+function useDateRange(timeFrame?: TimeFrame) { + const [start, setStart] = useState(() => { + const [initialStart] = calculateTimeRange(timeFrame, { + start: monthUtils.dayFromDate(monthUtils.currentMonth()), + end: monthUtils.currentDay(), + mode: 'full', + }); + return initialStart; + }); + + const [end, setEnd] = useState(() => { + const [, initialEnd] = calculateTimeRange(timeFrame, { + start: monthUtils.dayFromDate(monthUtils.currentMonth()), + end: monthUtils.currentDay(), + mode: 'full', + }); + return initialEnd; + }); + + const [mode, setMode] = useState(() => { + const [, , initialMode] = calculateTimeRange(timeFrame, { + start: monthUtils.dayFromDate(monthUtils.currentMonth()), + end: monthUtils.currentDay(), + mode: 'full', + }); + return initialMode; + }); + + return { start, setStart, end, setEnd, mode, setMode }; +}
138-165: Consider memoizing the month calculation logic.The month calculation logic in the useEffect could be memoized to prevent unnecessary recalculations.
+const calculateMonthRange = (earliestDate: string | null) => { + const currentMonth = monthUtils.currentMonth(); + let earliestMonth = earliestDate + ? monthUtils.monthFromDate(parseISO(fromDateRepr(earliestDate))) + : currentMonth; + + const yearAgo = monthUtils.subMonths(currentMonth, 12); + if (earliestMonth > yearAgo) { + earliestMonth = yearAgo; + } + + return monthUtils + .rangeInclusive(earliestMonth, currentMonth) + .map(month => ({ + name: month, + pretty: monthUtils.format(month, 'MMMM, yyyy'), + })) + .reverse(); +}; useEffect(() => { async function run() { const trans = await send('get-earliest-transaction'); - const currentMonth = monthUtils.currentMonth(); - let earliestMonth = trans - ? monthUtils.monthFromDate(parseISO(fromDateRepr(trans.date))) - : currentMonth; - - const yearAgo = monthUtils.subMonths(monthUtils.currentMonth(), 12); - if (earliestMonth > yearAgo) { - earliestMonth = yearAgo; - } - - const allMonths = monthUtils - .rangeInclusive(earliestMonth, monthUtils.currentMonth()) - .map(month => ({ - name: month, - pretty: monthUtils.format(month, 'MMMM, yyyy'), - })) - .reverse(); + const allMonths = calculateMonthRange(trans?.date ?? null); setAllMonths(allMonths); } run(); }, []);
354-413: Extract divisor filter component for better maintainability.The divisor filter logic is complex and should be extracted into a separate component.
+type DivisorFilterProps = { + content: SummaryContent; + setContent: (content: SummaryContent) => void; + divisorConditions: Array<RuleConditionEntity>; + divisorConditionsOp: string; + divisorOnApplyFilter: (filter: any) => void; + divisorOnUpdateFilter: (filter: any) => void; + divisorOnDeleteFilter: (filter: any) => void; + divisorOnConditionsOpChange: (op: string) => void; + isNarrowWidth: boolean; +}; + +function DivisorFilter({ + content, + setContent, + divisorConditions, + divisorConditionsOp, + divisorOnApplyFilter, + divisorOnUpdateFilter, + divisorOnDeleteFilter, + divisorOnConditionsOpChange, + isNarrowWidth, +}: DivisorFilterProps) { + return ( + <View style={{ margin: 16, marginTop: 0 }}> + {/* ... existing divisor filter JSX ... */} + </View> + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3792.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/reports/Overview.tsx(3 hunks)packages/desktop-client/src/components/reports/ReportRouter.tsx(2 hunks)packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.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 (4)
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/src/components/reports/SummaryNumber.tsx
- packages/loot-core/src/types/models/dashboard.d.ts
🔇 Additional comments (3)
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx (2)
1-26: LGTM! Well-structured imports and type definitions.
The imports are organized logically, and the SummaryProps type is well-defined with appropriate TypeScript types.
58-70: Verify implementation of all calculation modes.
Let's verify that the summarySpreadsheet function properly implements all calculation modes mentioned in the PR objectives.
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts (1)
1-12: LGTM! Well-organized imports with proper type declarations.
The imports are well-structured, properly typed, and follow a clear organization pattern.
packages/desktop-client/src/components/reports/spreadsheets/summary-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Summary.tsx
Outdated
Show resolved
Hide resolved
I did some changes. It looks like it fits better. Take a look |
Not sure when I introduced this error, but fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/reports/reports/Summary.tsx (3)
85-106: Improve type safety of content state initialization.The content state initialization could be simplified and made more type-safe by extracting the default state and using a type guard.
+const defaultContent: SummaryContent = { + type: 'sum', + divisorAllTimeDateRange: false, + divisorConditions: [], + divisorConditionsOp: 'and', +}; + +function isValidContent(content: unknown): content is SummaryContent { + return ( + typeof content === 'object' && + content !== null && + 'type' in content && + typeof content.type === 'string' + ); +} + const [content, setContent] = useState<SummaryContent>( widget?.meta?.content ? (() => { try { - return JSON.parse(widget.meta.content); + const parsed = JSON.parse(widget.meta.content); + return isValidContent(parsed) ? parsed : defaultContent; } catch (error) { console.error('Failed to parse widget meta content:', error); - return { - type: 'sum', - divisorAllTimeDateRange: false, - divisorConditions: [], - divisorConditionsOp: 'and', - }; + return defaultContent; } })() - : { - type: 'sum', - divisorAllTimeDateRange: false, - divisorConditions: [], - divisorConditionsOp: 'and', - }, + : defaultContent, );
425-429: Enhance accessibility for color-based information.Using color alone to distinguish between positive and negative values could be problematic for colorblind users.
color: (data?.total ?? 0) < 0 ? chartTheme.colors.red : chartTheme.colors.blue, +role: "status", +aria-label: `${(data?.total ?? 0) < 0 ? t('Negative') : t('Positive')} ${t('amount')}`,
543-557: Add overflow handling for long filter conditions.The filter conditions container has a fixed width which could lead to content overflow with long filter names.
-<View style={{ marginLeft: 16, maxWidth: '220px', marginRight: 16 }}> +<View style={{ + marginLeft: 16, + maxWidth: '220px', + marginRight: 16, + overflow: 'hidden', +}}> {(filterObject.conditions?.length ?? 0) === 0 ? ( <Text style={{ fontSize: '25px', color: theme.pageTextPositive }}> {t('all transactions')} </Text> ) : ( <AppliedFilters + style={{ overflow: 'auto' }} conditions={filterObject.conditions} onUpdate={filterObject.onUpdate} onDelete={filterObject.onDelete} conditionsOp={filterObject.conditionsOp} onConditionsOpChange={filterObject.onConditionsOpChange} /> )} </View>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/reports/reports/Summary.tsx (2)
46-58: 🛠️ Refactor suggestion
Add error handling for widget fetch failures.
The component should handle cases where the widget fetch fails, not just loading states.
export function Summary() {
const params = useParams();
- const { data: widget, isLoading } = useWidget<SummaryWidget>(
+ const { data: widget, isLoading, error } = useWidget<SummaryWidget>(
params.id ?? '',
'summary-card',
);
if (isLoading) {
return <LoadingIndicator />;
}
+ if (error) {
+ return (
+ <View style={{ padding: 20 }}>
+ <Text>{t('Failed to load summary widget')}</Text>
+ </View>
+ );
+ }
return <SummaryInner widget={widget} />;
}Likely invalid or redundant comment.
150-177: 🛠️ Refactor suggestion
Add error handling and cleanup for data fetching effect.
The effect for fetching the earliest transaction should handle errors gracefully and clean up properly.
useEffect(() => {
+ let mounted = true;
async function run() {
- const trans = await send('get-earliest-transaction');
- const currentMonth = monthUtils.currentMonth();
- let earliestMonth = trans
- ? monthUtils.monthFromDate(parseISO(fromDateRepr(trans.date)))
- : currentMonth;
+ try {
+ const trans = await send('get-earliest-transaction');
+ if (!mounted) return;
+
+ const currentMonth = monthUtils.currentMonth();
+ let earliestMonth = trans
+ ? monthUtils.monthFromDate(parseISO(fromDateRepr(trans.date)))
+ : currentMonth;
- // Make sure the month selects are at least populates with a
- // year's worth of months. We can undo this when we have fancier
- // date selects.
- const yearAgo = monthUtils.subMonths(monthUtils.currentMonth(), 12);
- if (earliestMonth > yearAgo) {
- earliestMonth = yearAgo;
- }
+ // Ensure at least a year's worth of months
+ const yearAgo = monthUtils.subMonths(currentMonth, 12);
+ earliestMonth = earliestMonth > yearAgo ? yearAgo : earliestMonth;
- const allMonths = monthUtils
- .rangeInclusive(earliestMonth, monthUtils.currentMonth())
- .map(month => ({
- name: month,
- pretty: monthUtils.format(month, 'MMMM, yyyy'),
- }))
- .reverse();
+ const allMonths = monthUtils
+ .rangeInclusive(earliestMonth, currentMonth)
+ .map(month => ({
+ name: month,
+ pretty: monthUtils.format(month, 'MMMM, yyyy'),
+ }))
+ .reverse();
- setAllMonths(allMonths);
+ if (mounted) {
+ setAllMonths(allMonths);
+ }
+ } catch (error) {
+ console.error('Failed to fetch earliest transaction:', error);
+ if (mounted) {
+ // Set default range of last 12 months
+ const currentMonth = monthUtils.currentMonth();
+ const yearAgo = monthUtils.subMonths(currentMonth, 12);
+ const allMonths = monthUtils
+ .rangeInclusive(yearAgo, currentMonth)
+ .map(month => ({
+ name: month,
+ pretty: monthUtils.format(month, 'MMMM, yyyy'),
+ }))
+ .reverse();
+ setAllMonths(allMonths);
+ }
+ }
}
run();
+ return () => {
+ mounted = false;
+ };
}, []);Likely invalid or redundant comment.
It fits better and works on different screen sizes but it's overflowing, which causes it to have unequal spacing on top and bottom (because the bottom has overflown the card).
This is minor, I'm happy to approve this but will wait for others to feedback since others have added to convo. |
|
yeah, I removed the height limiting. now its being controlled just by the width limit. I can revert it to height limiting too, but it will make it get smaller as you saw |
I'm easy either way. I'll approve once the VRT's are fixed and will wait for 1 more approval before merging. |
|
VRT done after master merge @MikesGlitch |
|
The text still needs some help. |
|
I will bring back the height limiting ok? |
|
This is a great report to add! |
packages/desktop-client/src/components/reports/SummaryNumber.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/SummaryNumber.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
packages/desktop-client/src/icons/v2/Sum.tsx (2)
4-12: Enhance SVG attributes for accessibility and rendering.Add ARIA attributes and additional SVG properties for better accessibility and rendering control.
<svg {...props} xmlns="http://www.w3.org/2000/svg" viewBox="0 0 28.3 28.3" + width="1em" + height="1em" + role="img" + aria-labelledby={title ? `sum-title-${props.id || ''}` : undefined} style={{ color: 'inherit', ...props.style, }} > + {title && <title id={`sum-title-${props.id || ''}`}>{title}</title>}
13-18: Optimize path data for better maintainability.The path data contains unnecessary whitespace and tabs. Consider formatting it for better readability.
<path - d="M23.2,10.1c1.1,0,2-0.9,2-2V2.2c0-1.1-0.9-2-2-2h-18c-1.1,0-2,0.9-2,2c0,0.4,0.1,0.9,0.4,1.2l8.1,10.8L3.6,25 - c-0.7,0.9-0.5,2.1,0.4,2.8c0.3,0.3,0.8,0.4,1.2,0.4h18c1.1,0,2-0.9,2-2v-5.8c0-1.1-0.9-2-2-2s-2,0.9-2,2v3.8h-12l6.6-8.8 - c0.5-0.7,0.5-1.7,0-2.4L9.2,4.2h12v3.9C21.2,9.2,22.1,10.1,23.2,10.1z" + d="M23.2 10.1c1.1 0 2-0.9 2-2V2.2c0-1.1-0.9-2-2-2h-18c-1.1 0-2 0.9-2 2c0 0.4 0.1 0.9 0.4 1.2l8.1 10.8L3.6 25c-0.7 0.9-0.5 2.1 0.4 2.8c0.3 0.3 0.8 0.4 1.2 0.4h18c1.1 0 2-0.9 2-2v-5.8c0-1.1-0.9-2-2-2s-2 0.9-2 2v3.8h-12l6.6-8.8c0.5-0.7 0.5-1.7 0-2.4L9.2 4.2h12v3.9C21.2 9.2 22.1 10.1 23.2 10.1z" fill="currentColor" />packages/desktop-client/src/icons/v2/CloseParenthesis.tsx (1)
1-3: Consider optimizing the React importSince this component only needs the type import and doesn't use any React features directly, you can optimize the import.
-import * as React from 'react'; +import type { SVGProps } from 'react'; -import type { SVGProps } from 'react';packages/desktop-client/src/icons/v2/OpenParenthesis.tsx (1)
15-18: Format the path data for better maintainabilityThe path data contains unnecessary tabs and newlines which make it harder to maintain. Consider formatting it as a single line or using a consistent indentation pattern.
- d="M15.1,54.5L15.1,54.5c-1.3-0.2-2.4-0.7-3.4-1.5c-0.8-0.8-1.5-1.8-2-2.9c-0.9-2.3-3.1-8.4-3.1-21.8S8.8,8.9,9.8,6.6 - c0.5-1.1,1.2-2.1,2-2.9c1-0.8,2.1-1.3,3.3-1.5h0.1c0.4-0.1,0.6-0.4,0.6-0.8c-0.1-0.3-0.3-0.6-0.6-0.6c-1.6-0.2-3.2,0.1-4.7,0.8 - C9,2.4,7.8,3.5,6.8,4.8c-1.7,2.6-2.9,5.4-3.6,8.4l-0.1,0.3c-2.5,9.7-2.5,19.8,0,29.5l0.1,0.3c0.7,3,1.9,5.8,3.6,8.4 - c0.9,1.3,2.2,2.4,3.6,3.2c1.5,0.7,3.1,0.9,4.7,0.8c0.4,0,0.7-0.4,0.6-0.7C15.7,54.8,15.5,54.5,15.1,54.5L15.1,54.5z" + d="M15.1,54.5L15.1,54.5c-1.3-0.2-2.4-0.7-3.4-1.5c-0.8-0.8-1.5-1.8-2-2.9c-0.9-2.3-3.1-8.4-3.1-21.8S8.8,8.9,9.8,6.6c0.5-1.1,1.2-2.1,2-2.9c1-0.8,2.1-1.3,3.3-1.5h0.1c0.4-0.1,0.6-0.4,0.6-0.8c-0.1-0.3-0.3-0.6-0.6-0.6c-1.6-0.2-3.2,0.1-4.7,0.8C9,2.4,7.8,3.5,6.8,4.8c-1.7,2.6-2.9,5.4-3.6,8.4l-0.1,0.3c-2.5,9.7-2.5,19.8,0,29.5l0.1,0.3c0.7,3,1.9,5.8,3.6,8.4c0.9,1.3,2.2,2.4,3.6,3.2c1.5,0.7,3.1,0.9,4.7,0.8c0.4,0,0.7-0.4,0.6-0.7C15.7,54.8,15.5,54.5,15.1,54.5L15.1,54.5z"packages/desktop-client/src/components/reports/reports/SummaryCard.tsx (2)
36-40: Consider making themodeparameter configurableThe
modeparameter is hardcoded as 'full' in the time range calculation. Given that this is a summary card with different display modes (as per PR objectives), consider making this parameter configurable through themetaprops to support different time range calculations for different summary types.const [start, end] = calculateTimeRange(meta?.timeFrame ?? 'all', { start: monthUtils.dayFromDate(monthUtils.currentMonth()), end: monthUtils.currentDay(), - mode: 'full', + mode: meta?.mode ?? 'full', });
71-71: Add cleanup for nameMenuOpen stateConsider adding a cleanup effect to reset the nameMenuOpen state when the component unmounts or when isEditing becomes false, to prevent any potential memory leaks or stale UI states.
const [nameMenuOpen, setNameMenuOpen] = useState(false); + +useEffect(() => { + if (!isEditing) { + setNameMenuOpen(false); + } + return () => setNameMenuOpen(false); +}, [isEditing]);packages/desktop-client/src/components/reports/reports/Summary.tsx (3)
85-106: Extract default content state to a constant.The default values for the content state are duplicated in both the error handler and initial state. Consider extracting these to a constant to maintain DRY principles and ensure consistency.
+const DEFAULT_CONTENT: SummaryContent = { + type: 'sum', + divisorAllTimeDateRange: false, + divisorConditions: [], + divisorConditionsOp: 'and', +}; const [content, setContent] = useState<SummaryContent>( widget?.meta?.content ? (() => { try { return JSON.parse(widget.meta.content); } catch (error) { console.error('Failed to parse widget meta content:', error); - return { - type: 'sum', - divisorAllTimeDateRange: false, - divisorConditions: [], - divisorConditionsOp: 'and', - }; + return DEFAULT_CONTENT; } })() - : { - type: 'sum', - divisorAllTimeDateRange: false, - divisorConditions: [], - divisorConditionsOp: 'and', - }, + : DEFAULT_CONTENT, );
442-449: Consider using a union type for operation types.The type property could be more strictly typed using a union type constant to prevent magic strings and improve maintainability.
+const OPERATION_TYPES = { + SUM: 'sum', + AVG_PER_MONTH: 'avgPerMonth', + AVG_PER_TRANSACT: 'avgPerTransact', + PERCENTAGE: 'percentage', +} as const; + +type OperationType = typeof OPERATION_TYPES[keyof typeof OPERATION_TYPES]; + type OperatorProps = { - type: 'sum' | 'avgPerMonth' | 'avgPerTransact' | 'percentage'; + type: OperationType; dividendFilterObject: FilterObject; divisorFilterObject: FilterObject; fromRange: string; toRange: string; showDivisorDateRange: boolean; };
523-568: Extract inline styles to constants.The component contains multiple inline styles that could be extracted to constants for better maintainability and reusability.
+const styles = { + container: { + height: '100%', + flexDirection: 'row' as const, + alignItems: 'center' as const, + position: 'relative' as const, + display: 'grid', + gridTemplateColumns: '70px 15px 1fr 15px', + }, + sumContainer: { + position: 'relative' as const, + height: '50px', + marginRight: 50, + }, + dateText: { + position: 'absolute' as const, + right: -30, + }, + filterContainer: { + marginLeft: 16, + maxWidth: '220px', + marginRight: 16, + }, +}; return ( <View style={{ - ...containerStyle, - height: '100%', - flexDirection: 'row', - alignItems: 'center', - position: 'relative', - display: 'grid', - gridTemplateColumns: '70px 15px 1fr 15px', + ...styles.container, + ...containerStyle, }} > - <View style={{ position: 'relative', height: '50px', marginRight: 50 }}> + <View style={styles.sumContainer}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/desktop-client/src/icons/v2/CloseParenthesis.svgis excluded by!**/*.svgpackages/desktop-client/src/icons/v2/OpenParenthesis.svgis excluded by!**/*.svgpackages/desktop-client/src/icons/v2/Sum.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
packages/desktop-client/src/components/reports/Overview.tsx(3 hunks)packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/Summary.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/SummaryCard.tsx(1 hunks)packages/desktop-client/src/icons/v2/CloseParenthesis.tsx(1 hunks)packages/desktop-client/src/icons/v2/OpenParenthesis.tsx(1 hunks)packages/desktop-client/src/icons/v2/Sum.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/reports/SummaryNumber.tsx
🔇 Additional comments (9)
packages/desktop-client/src/icons/v2/Sum.tsx (1)
1-20: Verify icon usage in summary report components.
Let's verify how this icon is being used in the summary report components to ensure consistent styling and accessibility.
✅ Verification successful
Icon implementation aligns with design patterns and usage
The SvgSum icon follows the established patterns in the codebase:
- Uses consistent styling with
color: 'inherit'and style prop spreading - Implements proper SVG attributes and viewBox
- Used appropriately in the Summary component with explicit width/height (50px)
- Follows the same implementation pattern as other v2 icons in the codebase
The icon is correctly integrated and matches the design system's implementation standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SvgSum usage in summary-related components
rg -l "SvgSum" packages/desktop-client/src/components/reports/
# Check if the icon size matches other icons in the codebase
rg "width=\"1em\".*height=\"1em\"" packages/desktop-client/src/icons/v2/
Length of output: 200
Script:
#!/bin/bash
# Let's check how the SvgSum is used in the Summary component
rg -A 5 "SvgSum" packages/desktop-client/src/components/reports/reports/Summary.tsx
# Let's check the styling pattern of other icons in v2 directory
fd . packages/desktop-client/src/icons/v2/ -e tsx -x head -n 20
Length of output: 39508
packages/desktop-client/src/icons/v2/CloseParenthesis.tsx (2)
14-20: LGTM! Path implementation follows SVG best practices
The path implementation is correct and follows best practices:
- Uses
currentColorfor fill, allowing for theme compatibility - Path data is valid and properly formatted
1-22: Verify the icon's usage in the summary feature
Since this icon is part of the summary report feature update, let's verify its integration.
✅ Verification successful
Icon is properly integrated in the Summary report feature
The CloseParenthesis icon is correctly used in the Summary component alongside its matching OpenParenthesis icon. They are used to create a visual parenthetical enclosure for transaction filters in the report's mathematical expression display. The implementation is consistent with the design pattern, where both icons:
- Share the same viewBox and preserveAspectRatio properties
- Use currentColor for proper theme integration
- Are styled with matching height properties
- Work together to create a cohesive visual container
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of CloseParenthesis in the summary feature components
rg -A 3 "CloseParenthesis" --type tsx 'packages/desktop-client/src'
# Check if this is part of a pair (OpenParenthesis)
fd "OpenParenthesis.tsx" packages/desktop-client/src/icons
Length of output: 209
Script:
#!/bin/bash
# Search for usage of CloseParenthesis with proper file extension
rg -A 3 "CloseParenthesis" packages/desktop-client/src
# Check the content of OpenParenthesis for consistency
cat packages/desktop-client/src/icons/v2/OpenParenthesis.tsx
# Look for any components that might use parentheses icons together
rg -l "OpenParenthesis|CloseParenthesis" packages/desktop-client/src
Length of output: 2553
Script:
#!/bin/bash
# Check the Summary component to understand how these parenthesis icons are used together
cat packages/desktop-client/src/components/reports/reports/Summary.tsx
Length of output: 16368
packages/desktop-client/src/icons/v2/OpenParenthesis.tsx (2)
1-22: LGTM! Well-structured SVG component
The implementation follows React best practices:
- Proper TypeScript types for SVG props
- Correct props spreading and style merging
- Use of currentColor for theme support
- Proper SVG namespace and viewBox configuration
8-8: Consider the implications of preserveAspectRatio='none'
While preserveAspectRatio='none' allows the SVG to stretch freely, it might cause the parenthesis to look distorted when scaled non-uniformly. Consider if preserveAspectRatio='xMidYMid meet' would be more appropriate to maintain the shape's proportions.
packages/desktop-client/src/components/reports/reports/SummaryCard.tsx (1)
57-67: Verify implementation of all summary modes
Based on the PR objectives, this component should support three modes:
- Average per transaction
- Average per month
- Ratio calculation (expenses/income)
However, the current implementation only seems to handle basic sum and percentage calculations. Consider extending the summarySpreadsheet parameters to support all required modes.
packages/desktop-client/src/components/reports/reports/Summary.tsx (3)
1-65: LGTM! Imports and type definitions are well-organized.
The imports and type definitions are properly structured and all appear to be necessary for the component's functionality.
46-58: 🛠️ Refactor suggestion
Add error state handling for widget data fetching.
The component handles loading state but doesn't handle potential errors from the useWidget hook. This could lead to silent failures.
export function Summary() {
const params = useParams();
- const { data: widget, isLoading } = useWidget<SummaryWidget>(
+ const { data: widget, isLoading, error } = useWidget<SummaryWidget>(
params.id ?? '',
'summary-card',
);
if (isLoading) {
return <LoadingIndicator />;
}
+ if (error) {
+ return (
+ <View style={{ padding: 20 }}>
+ <Text>{t('Failed to load summary widget')}</Text>
+ </View>
+ );
+ }
return <SummaryInner widget={widget} />;
}Likely invalid or redundant comment.
150-177: 🛠️ Refactor suggestion
Add error handling for earliest transaction fetch.
The get-earliest-transaction API call lacks error handling. If the API call fails, it could lead to undefined behavior.
useEffect(() => {
async function run() {
+ try {
const trans = await send('get-earliest-transaction');
const currentMonth = monthUtils.currentMonth();
let earliestMonth = trans
? monthUtils.monthFromDate(parseISO(fromDateRepr(trans.date)))
: currentMonth;
// Make sure the month selects are at least populates with a
// year's worth of months. We can undo this when we have fancier
// date selects.
const yearAgo = monthUtils.subMonths(monthUtils.currentMonth(), 12);
if (earliestMonth > yearAgo) {
earliestMonth = yearAgo;
}
const allMonths = monthUtils
.rangeInclusive(earliestMonth, monthUtils.currentMonth())
.map(month => ({
name: month,
pretty: monthUtils.format(month, 'MMMM, yyyy'),
}))
.reverse();
setAllMonths(allMonths);
+ } catch (error) {
+ console.error('Failed to fetch earliest transaction:', error);
+ // Set default range of last 12 months
+ const yearAgo = monthUtils.subMonths(monthUtils.currentMonth(), 12);
+ const allMonths = monthUtils
+ .rangeInclusive(yearAgo, monthUtils.currentMonth())
+ .map(month => ({
+ name: month,
+ pretty: monthUtils.format(month, 'MMMM, yyyy'),
+ }))
+ .reverse();
+ setAllMonths(allMonths);
+ }
}
run();
}, []);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (1)
3-3: Consider using lodash's debounce implementation.The 'debounce' package has fewer maintenance updates compared to lodash. Since lodash is already a common dependency in React projects, consider using its implementation for better long-term maintenance.
-import { debounce } from 'debounce'; +import { debounce } from 'lodash'; const handleResize = debounce(() => { - adjustFontSizeBinary(14, 200); + adjustFontSizeBinary(MIN_FONT_SIZE, MAX_FONT_SIZE); }, 250);Also applies to: 87-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/SummaryNumber.tsx(1 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
94-148: LGTM! Well-implemented accessibility and privacy features.
The render logic demonstrates good practices:
- Proper ARIA attributes for accessibility
- Privacy filtering for sensitive data
- Semantic HTML structure
35-35: 🛠️ Refactor suggestion
Prevent layout shift by using initialFontSize.
Setting the initial fontSize state to 0 could cause a layout shift when the component first renders. Initialize it with the initialFontSize prop instead.
-const [fontSize, setFontSize] = useState<number>(0);
+const [fontSize, setFontSize] = useState<number>(initialFontSize);Likely invalid or redundant comment.
A way to show only a sum of the filter applied to the card.
Actual.-.Google.Chrome.2024-11-04.19-04-56.mp4
Possible new modes: