Add sorting option to custom reports #4141
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
/update-vrt |
|
/update-vrt |
|
/update-vrt |
matt-fidd
force-pushed
the
custom-report-sorting
branch
from
2e7ab25 to
10938cf
Compare
matt-fidd
changed the title
WalkthroughThe pull request introduces a comprehensive sorting functionality across various components of the desktop client and core reporting system. The changes span multiple files in the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Learnings (1)packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (9)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/desktop-client/src/components/reports/spreadsheets/grouped-spreadsheet.ts (1)
145-154: Optimize sorting implementation for better performance.The current implementation has unnecessary spread operations and multiple iterations:
- Spreading arrays is unnecessary when using
sort()as it already returns the sorted array- The
mapoperation can be combined with the initialsortConsider this more efficient implementation:
- const sortedGroupedDataFiltered = [...groupedDataFiltered] - .sort(sortData({ balanceTypeOp, sortByOp })) - .map(g => { - g.categories = [...(g.categories ?? [])].sort( - sortData({ balanceTypeOp, sortByOp }), - ); - return g; - }); + const sortedGroupedDataFiltered = groupedDataFiltered + .sort(sortData({ balanceTypeOp, sortByOp })) + .map(g => ({ + ...g, + categories: (g.categories ?? []).sort( + sortData({ balanceTypeOp, sortByOp }), + ), + })); setData(sortedGroupedDataFiltered);packages/loot-core/src/server/reports/app.ts (1)
Line range hint
20-36: Add validation for sort_by field.Consider adding validation in the
validatemethod to ensuresort_byonly accepts valid values ('asc' or 'desc').validate( report: Omit<CustomReportEntity, 'tombstone'>, { update }: { update?: boolean } = {}, ) { requiredFields('Report', report, ['conditionsOp'], update); if (!update || 'conditionsOp' in report) { if (!['and', 'or'].includes(report.conditionsOp)) { throw new ValidationError( 'Invalid filter conditionsOp: ' + report.conditionsOp, ); } } + if (!update || 'sortBy' in report) { + if (report.sortBy && !['asc', 'desc'].includes(report.sortBy)) { + throw new ValidationError( + 'Invalid sortBy value: ' + report.sortBy, + ); + } + } return report; }packages/desktop-client/src/components/reports/ReportOptions.ts (1)
26-26: Consider using const values for sort direction.Instead of using string literals, consider defining const values for sort directions to prevent typos and improve maintainability.
+const SORT_DIRECTIONS = { + ASCENDING: 'Ascending', + DESCENDING: 'Descending', +} as const; export const defaultReport: CustomReportEntity = { // ...other properties - sortBy: 'Ascending', + sortBy: SORT_DIRECTIONS.ASCENDING, // ...other properties };packages/desktop-client/src/components/reports/graphs/BarGraph.tsx (1)
2-2: Remove unused CSSProperties import.The
CSSPropertiestype is imported but not used in the changed code.-import React, { useState, type CSSProperties } from 'react'; +import React, { useState } from 'react';packages/desktop-client/src/components/reports/ReportSidebar.tsx (1)
164-172: Extract complex disable logic to a separate function.The
disableSortlogic is complex and would be more maintainable as a separate function.+const shouldDisableSort = ( + graphType: string, + groupBy: string, + mode: string, + disabledList: any +) => { + return ( + graphType !== 'TableGraph' && + (groupBy === 'Interval' || + (disabledList?.mode + ?.find(m => m.description === mode) + ?.graphs.find(g => g.description === graphType) + ?.disableSort ?? + false)) + ); +}; - const disableSort = - customReportItems.graphType !== 'TableGraph' && - (customReportItems.groupBy === 'Interval' || - (disabledList?.mode - ?.find(m => m.description === customReportItems.mode) - ?.graphs.find(g => g.description === customReportItems.graphType) - ?.disableSort ?? - false)); + const disableSort = shouldDisableSort( + customReportItems.graphType, + customReportItems.groupBy, + customReportItems.mode, + disabledList + );packages/loot-core/migrations/1736640000000__custom_report_sorting.sql (1)
1-5: LGTM! Consider adding a CHECK constraint.The migration follows best practices by using a transaction. For additional data integrity, consider adding a CHECK constraint to ensure only valid sort directions ('asc' or 'desc') can be stored:
BEGIN TRANSACTION; -ALTER TABLE custom_reports ADD COLUMN sort_by TEXT DEFAULT 'asc'; +ALTER TABLE custom_reports ADD COLUMN sort_by TEXT DEFAULT 'asc' + CHECK (sort_by IN ('asc', 'desc')); COMMIT;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/4141.mdis excluded by!**/*.md
📒 Files selected for processing (14)
packages/desktop-client/src/components/reports/ReportOptions.ts(3 hunks)packages/desktop-client/src/components/reports/ReportSidebar.tsx(5 hunks)packages/desktop-client/src/components/reports/disabledList.ts(4 hunks)packages/desktop-client/src/components/reports/graphs/BarGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/DonutGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(10 hunks)packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts(5 hunks)packages/desktop-client/src/components/reports/spreadsheets/grouped-spreadsheet.ts(3 hunks)packages/desktop-client/src/components/reports/spreadsheets/sortData.ts(1 hunks)packages/loot-core/migrations/1736640000000__custom_report_sorting.sql(1 hunks)packages/loot-core/src/client/data-hooks/reports.ts(1 hunks)packages/loot-core/src/server/aql/schema/index.ts(1 hunks)packages/loot-core/src/server/reports/app.ts(3 hunks)packages/loot-core/src/types/models/reports.d.ts(3 hunks)
🔇 Additional comments (17)
packages/loot-core/src/client/data-hooks/reports.ts (1)
21-21: LGTM!The addition of the
sortByproperty follows the established pattern for mapping database fields to entity properties.packages/desktop-client/src/components/reports/disabledList.ts (3)
53-53: LGTM! Type definition properly updated.The
disableSortoptional boolean property is correctly added to thegraphOptionstype.
79-79: LGTM! Sorting correctly disabled for AreaGraph.Sorting is appropriately disabled for the AreaGraph in total mode, which aligns with the PR objectives.
99-99: LGTM! Sorting consistently disabled for time mode graphs.Sorting is appropriately disabled for all time mode graphs (TableGraph, StackedBarGraph, LineGraph), maintaining consistency with the PR objectives.
Also applies to: 107-107, 117-117
packages/loot-core/src/server/reports/app.ts (1)
44-44: LGTM! Report model properly updated.The
sort_byfield is correctly mapped in both directions between the database and JS representations.Also applies to: 68-68
packages/desktop-client/src/components/reports/graphs/DonutGraph.tsx (1)
Line range hint
250-271: LGTM! Sorting logic properly moved to spreadsheet layer.The removal of local sorting in the DonutGraph component is appropriate as sorting is now handled at the spreadsheet level, making the code cleaner and more maintainable.
packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts (2)
17-17: LGTM! Type definitions properly updated.The
sortByOpTypeimport andsortByOpproperty are correctly added with proper typing.Also applies to: 52-52
73-73: LGTM! Sorting implementation is clean and efficient.The sorting implementation:
- Sets a sensible default sort order ('asc')
- Uses spread operator to avoid mutating the original array
- Properly integrates with the existing data flow
Also applies to: 288-291
packages/desktop-client/src/components/reports/ReportOptions.ts (2)
53-56: LGTM! Well-structured sort options with translations.The sort options are well-defined with proper translations and type-safe format values.
207-210: LGTM! Clean mapping implementation.The mapping between display values and internal format is implemented correctly using Map.
packages/desktop-client/src/components/reports/graphs/BarGraph.tsx (1)
222-222: LGTM! Clean data flow.The direct use of
data[splitData]simplifies the data flow by removing local sorting, which is now handled centrally.packages/desktop-client/src/components/reports/ReportSidebar.tsx (2)
146-150: LGTM! Clean handler implementation.The
onChangeSortByhandler follows the established pattern and correctly updates both session state and parent component.
287-308: LGTM! Well-structured conditional rendering.The sort options UI is correctly implemented with proper conditional rendering based on the
disableSortflag.packages/desktop-client/src/components/reports/reports/CustomReport.tsx (4)
15-15: LGTM! Type and state management look good.The sortByOpType import and sortBy state are properly integrated with the existing state management pattern.
Also applies to: 234-235
365-365: LGTM! Data processing logic is properly integrated.The sorting functionality is consistently implemented across both group and graph data processing, with proper memoization dependencies.
Also applies to: 388-388, 403-403, 423-423, 445-445
465-465: LGTM! Component integration is consistent.The sortBy functionality is well-integrated into the component hierarchy, following existing patterns for state management and prop passing.
Also applies to: 604-604, 735-735
Line range hint
1-924: Verify test coverage for the new sorting functionality.While the implementation is solid, ensure that test cases are added to verify:
- Default sorting behavior
- Ascending and descending sort operations
- Persistence of sort preferences
- Integration with different report types (bar charts, pie charts, data tables)
Would you like me to help generate test cases for the new sorting functionality?
packages/desktop-client/src/components/reports/spreadsheets/sortData.ts
Outdated
Show resolved
Hide resolved
|
This is looking pretty good. I have a few thoughts.
|
|
Agree with youngcw's first point, instantly felt weird. It's the size of the bar vs the raw amount, I'd go with the former as it's a visual setting for bar charts. Except possibly for Net Deposit which seems to make sense as-is (ascending shows small to large bar with my budget. For migration I'd suggest populating the sort box in the UI, my reports loaded with nothing in it so it was tiny.
|
…d budget sort order
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/components/reports/spreadsheets/sortData.ts (2)
7-10: Consider using a complete mapping for better type safety.The
Partial<Record>allows for undefined mappings. Since we know all possible values, we can make this more type-safe.-const reverseSort: Partial<Record<sortByOpType, sortByOpType>> = { +const reverseSort: Record<'asc' | 'desc', sortByOpType> = { asc: 'desc', desc: 'asc', };
12-15: Optimize balance type lookup performance.For better performance with larger datasets, consider using a Set for O(1) lookup instead of array includes O(n).
-const balanceTypesToReverse = ['totalDebts', 'netDebts']; +const balanceTypesToReverse = new Set(['totalDebts', 'netDebts']); const shouldReverse = (balanceTypeOp: balanceTypeOpType) => - balanceTypesToReverse.includes(balanceTypeOp); + balanceTypesToReverse.has(balanceTypeOp);packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts (1)
288-291: Avoid unnecessary array spread for sorting.Creating a new array with spread operator before sorting is unnecessary as
sort()already mutates and returns the same array.- const sortedCalcDataFiltered = [...calcDataFiltered].sort( + const sortedCalcDataFiltered = calcDataFiltered.sort( sortData({ balanceTypeOp, sortByOp }), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/desktop-client/src/components/reports/ReportOptions.ts(3 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(10 hunks)packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts(5 hunks)packages/desktop-client/src/components/reports/spreadsheets/sortData.ts(1 hunks)packages/loot-core/migrations/1736640000000__custom_report_sorting.sql(1 hunks)packages/loot-core/src/server/aql/schema/index.ts(1 hunks)packages/loot-core/src/types/models/reports.d.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loot-core/src/server/aql/schema/index.ts
- packages/loot-core/migrations/1736640000000__custom_report_sorting.sql
🧰 Additional context used
📓 Learnings (4)
packages/loot-core/src/types/models/reports.d.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/loot-core/src/types/models/reports.d.ts:14-14
Timestamp: 2025-01-12T20:27:52.636Z
Learning: In the Actual Budget codebase's reports feature, `sortBy` in `CustomReportEntity` is typed as string to store user-friendly values ('Ascending'/'Descending'), while `sortByOpType` ('asc'/'desc') is used internally for operations.
packages/desktop-client/src/components/reports/spreadsheets/sortData.ts (2)
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/loot-core/src/types/models/reports.d.ts:14-14
Timestamp: 2025-01-12T20:27:52.636Z
Learning: In the Actual Budget codebase's reports feature, `sortBy` in `CustomReportEntity` is typed as string to store user-friendly values ('Ascending'/'Descending'), while `sortByOpType` ('asc'/'desc') is used internally for operations.
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/desktop-client/src/components/reports/spreadsheets/sortData.ts:15-26
Timestamp: 2025-01-12T20:22:02.704Z
Learning: In the custom reports feature of Actual, the `balanceTypeOp` and `sortByOp` parameters in the `sortData` function are always set and should not be marked as optional.
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/loot-core/src/types/models/reports.d.ts:14-14
Timestamp: 2025-01-12T20:27:52.636Z
Learning: In the Actual Budget codebase's reports feature, `sortBy` in `CustomReportEntity` is typed as string to store user-friendly values ('Ascending'/'Descending'), while `sortByOpType` ('asc'/'desc') is used internally for operations.
packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/desktop-client/src/components/reports/spreadsheets/sortData.ts:15-26
Timestamp: 2025-01-12T20:22:02.704Z
Learning: In the custom reports feature of Actual, the `balanceTypeOp` and `sortByOp` parameters in the `sortData` function are always set and should not be marked as optional.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (12)
packages/desktop-client/src/components/reports/spreadsheets/sortData.ts (2)
1-5: Clean imports using type imports!Good use of type imports for better tree-shaking and runtime performance.
31-44: 🛠️ Refactor suggestionAdd numeric type safety and stable sort implementation.
The current implementation has two issues:
- No handling of potential NaN values in numeric comparisons
- Sort is not stable, which can lead to inconsistent ordering when values are equal
return (a, b) => { let comparison = 0; if (sortByOp === 'asc') { - comparison = a[balanceTypeOp] - b[balanceTypeOp]; + const aVal = a[balanceTypeOp] || 0; + const bVal = b[balanceTypeOp] || 0; + comparison = aVal - bVal; } else if (sortByOp === 'desc') { - comparison = b[balanceTypeOp] - a[balanceTypeOp]; + const aVal = a[balanceTypeOp] || 0; + const bVal = b[balanceTypeOp] || 0; + comparison = bVal - aVal; } else if (sortByOp === 'name') { comparison = (a.name ?? '').localeCompare(b.name ?? ''); } else if (sortByOp === 'budget') { comparison = 0; } - return comparison; + // Implement stable sort by using name as secondary key + return comparison || (a.name ?? '').localeCompare(b.name ?? ''); };Likely invalid or redundant comment.
packages/loot-core/src/types/models/reports.d.ts (3)
14-14: Correct use of string type for user-friendly values!The
sortByproperty is correctly typed as string to store user-friendly values ('Ascending'/'Descending') as mentioned in the learnings.
34-34: Well-defined sortByOpType with all possible values!The
sortByOpTypecorrectly includes all possible sort operations: 'asc', 'desc', 'name', and 'budget'.
128-128: Consistent use of sortByOpType for internal operations!The
sort_byproperty inCustomReportDatacorrectly usessortByOpTypefor internal operations, maintaining the separation between user-friendly and internal values.packages/desktop-client/src/components/reports/ReportOptions.ts (3)
53-58: LGTM! Sort options align with requirements.The sort options include both basic (Ascending/Descending) and specialized (Name/Budget) sorting as requested in the PR feedback. The options are properly internationalized.
209-212: LGTM! Clean implementation of sort options mapping.The implementation correctly maps user-friendly descriptions to internal format values, following the established pattern in the codebase.
26-26: 🛠️ Refactor suggestionConsider context-aware default sorting.
Based on user feedback, the default descending sort might feel counterintuitive for certain reports. Consider making the default sort order context-aware:
- For reports with negative amounts, automatically flip the sort order
- For reports showing Net Deposit, use ascending order by default
- sortBy: 'Descending', + sortBy: context => { + if (context.hasNegativeAmounts) return 'Ascending'; + if (context.mode === 'Net Deposit') return 'Ascending'; + return 'Descending'; + },Likely invalid or redundant comment.
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (4)
15-15: LGTM! Proper type import.The import of
sortByOpTypealigns with the retrieved learning about separating user-friendly values from internal operation types.
234-235: LGTM! Clean state management for sorting.The implementation properly:
- Manages sort state using React's useState hook
- Maps user-friendly values to internal operation types
- Provides appropriate fallback values
Also applies to: 365-365
465-465: LGTM! Consistent state updates.The sort state is properly:
- Included in the report configuration
- Updated when loading report data
- Passed to the sidebar component
Also applies to: 604-604, 735-735
388-388: Verify sort behavior for time-based and interval-split graphs.The PR objectives mention that sorting should be disabled for time mode graphs and total mode graphs split by interval. However,
sortByOpis being passed unconditionally to bothgetGroupDataandgetGraphData.Also applies to: 403-403, 423-423, 445-445
matt-fidd
force-pushed
the
custom-report-sorting
branch
from
1f66f33 to
0d0ae3c
Compare
|
Amazing I would vote for budget order to be the default but what do others think? Descending would be my second choice though. |
|
Good ideas both, I have to admit that I got the sorting all muddled up even when writing...
I prefer descending on the pies and bars, but budget makes more sense in the table. I can make new defaults based on the type? |
I think thats fine as long as its not overly difficult to code up. |
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)
545-550: Consider adding error handling for invalid sort values.The default sort handling assumes the presence of a valid sort value. Consider adding a fallback for cases where
defaultsGraphListreturns an unexpected value.- const defaultSort = defaultsGraphList(mode, chooseGraph, 'defaultSort'); - if (defaultSort) { - setSessionReport('sortBy', defaultSort); - setSortBy(defaultSort); - } + const defaultSort = defaultsGraphList(mode, chooseGraph, 'defaultSort'); + const validSort = defaultSort || 'Descending'; // Fallback to a safe default + setSessionReport('sortBy', validSort); + setSortBy(validSort);packages/desktop-client/src/components/reports/disabledList.ts (1)
104-105: Remove redundant defaultSort from time graphs.Since sorting is disabled for time-based graphs (
disableSort: true), having adefaultSortvalue is redundant and might cause confusion in maintenance. Consider removing thedefaultSortproperty from these configurations.{ description: 'TableGraph', // ... other properties ... disableSort: true, - defaultSort: 'Descending', },Also applies to: 113-114, 124-125
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/reports/disabledList.ts(6 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(11 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/loot-core/src/types/models/reports.d.ts:14-14
Timestamp: 2025-01-12T20:27:52.636Z
Learning: In the Actual Budget codebase's reports feature, `sortBy` in `CustomReportEntity` is typed as string to store user-friendly values ('Ascending'/'Descending'), while `sortByOpType` ('asc'/'desc') is used internally for operations.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: Wait for Netlify build to finish
🔇 Additional comments (8)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (5)
15-15: LGTM! Type import looks good.The
sortByOpTypeimport aligns with the retrieved learning about sort operations.
234-235: LGTM! State initialization is correct.The sort state is properly initialized from the loaded report configuration.
365-365: LGTM! Sort operation mapping is well implemented.The mapping from user-friendly values to operation types is handled correctly using the
ReportOptions.sortByMap.
741-741: LGTM! Sort setter prop is correctly passed.The
setSortByfunction is properly passed to theReportSidebarcomponent.
388-388: Verify sort behavior with time mode and total mode graphs.The sort operation is being passed to both grouped and custom spreadsheet creation. According to the PR objectives, sorting should be disabled for time mode graphs and total mode graphs that are split by interval.
Run this script to verify the implementation:
Also applies to: 403-403, 423-423, 445-445
packages/desktop-client/src/components/reports/disabledList.ts (3)
51-54: LGTM! Type definition changes align with requirements.The addition of
defaultSortanddisableSortproperties provides a clean way to configure sorting behavior per graph type.
185-185: LGTM! Function signature updated correctly.The addition of 'defaultSort' to the type union allows retrieving default sort values through the existing API.
51-54: Verify sort options initialization.As per Teprifer's feedback, reports initially loaded with no content. Ensure that:
- The migration properly populates the sort_by column
- The UI correctly initializes with these default sort values
✅ Verification successful
Sort initialization is properly implemented
The sort functionality is correctly implemented with:
- Default sort values defined for all graph types
- Proper persistence in both session storage and database
- Consistent initialization in the UI components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for migration files and sort initialization logic echo "Checking migrations:" fd -e sql . packages/loot-core/src/server/db/migrations/ echo "Checking sort initialization:" rg -A 5 "sort.*initialize|initialize.*sort" packages/desktop-client/src/components/reports/Length of output: 404
Script:
#!/bin/bash # Find migrations directory echo "Locating migrations directory:" fd migrations echo "Checking sort-related code in reports:" rg -A 5 "defaultSort|sort_by" packages/desktop-client/src/components/reports/ echo "Checking sort initialization in the entire codebase:" ast-grep --pattern 'defaultSort = $_'Length of output: 9363
Script:
#!/bin/bash # Check setSessionReport implementation echo "Checking setSessionReport implementation:" rg -A 5 "setSessionReport|saveReport" packages/desktop-client/src/components/reports/ # Check if there are any related tests echo "Checking related tests:" rg -A 5 "test.*CustomReport|describe.*CustomReport" packages/desktop-client/src/**/*test*Length of output: 21069
matt-fidd
force-pushed
the
custom-report-sorting
branch
from
987eed5 to
5dd53db
Compare
Done! |
|
do you think you could revert-rerun the vrt on those data table snapshots? There didn't pick up the default order change and I don't want that causing issues down the line. |
|
/update-vrt |
|
weird. I wonder if the table sorting is defaulting to descending with a direct navigation like the VRT is doing. |
yeah, strange. I don't think it's a massive deal because it's rare that anyone would browse directly but I'll have a look tomorrow to see if I can sort it. |
If its consistent then its not a big deal. The interface seems to work right for a real user. |
matt-fidd
force-pushed
the
custom-report-sorting
branch
from
5ca41ea to
5d9a1f5
Compare
That's fine, I'll leave as-is ready for review then. I've retriggered the actions. |
Adds support for sorting data in barcharts, pie charts and data tables
Disabled on time mode graphs and total mode graphs split by interval
Sort modes:
Sort is reversed to make ascending/descending more natural on all negative values