report sorting on dashboard #4276
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
UnderKoen
changed the title
WalkthroughThe changes introduce a new type, Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4276.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/reports/ReportOptions.ts(3 hunks)packages/desktop-client/src/components/reports/ReportSidebar.tsx(3 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/GetCardData.tsx(2 hunks)packages/loot-core/src/types/models/reports.d.ts(1 hunks)
🧰 Additional context used
📓 Learnings (3)
packages/desktop-client/src/components/reports/ReportOptions.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/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/ReportSidebar.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 (6)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (7)
packages/loot-core/src/types/models/reports.d.ts (2)
14-14: LGTM! Type change aligns with internal operations.The change to use
sortByOpTypealigns with the codebase's internal operations, where 'asc'/'desc' values are used instead of user-friendly values like 'Ascending'/'Descending'.
14-14: Verify the impact of makingsortByoptional.The change to make
sortByoptional (with?) is a breaking change that could affect existing code that assumes this property is always present.Run the following script to verify the usage of
sortByin the codebase:packages/desktop-client/src/components/reports/reports/GetCardData.tsx (1)
132-132: LGTM! Consistent usage of sorting options.The
sortByOpproperty is correctly passed to both spreadsheet creation functions, ensuring consistent sorting behavior across the application.Also applies to: 153-153
packages/desktop-client/src/components/reports/ReportOptions.ts (2)
54-57: LGTM! Enhanced type safety for sort options.The explicit type annotations for
sortByOptionsimprove type safety and make the code more maintainable.
27-27: LGTM! Consistent value format.The
sortBydefault value now uses the lowercase format ('desc') which aligns with thesortByOpTypevalues.packages/desktop-client/src/components/reports/ReportSidebar.tsx (2)
149-150: LGTM! Safe handling of optional sort values.The function now accepts an optional parameter and provides a sensible default value of 'asc', ensuring backward compatibility.
304-309: LGTM! Type-safe select options.The
Selectcomponent props are correctly updated to handle the new type system:
onChangeaccepts optionalsortByOpTypeoptionsuseformatfor valuesdisabledKeysis properly cast tosortByOpType[]
packages/desktop-client/src/components/reports/ReportSidebar.tsx
Outdated
Show resolved
Hide resolved
|
/update-vrt |
|
The VRT changes don't look quite right |
|
/update-vrt |
UnderKoen
force-pushed
the
UnderKoen/sorting-report-dashboard
branch
from
7cf781e to
83ab62c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/desktop-client/src/components/reports/disabledList.ts (2)
67-67: Consider removing redundantdefaultSortvalues.Some graph types (like 'AreaGraph') have
disableSort: truebut still specify adefaultSortvalue. Consider removing thedefaultSortproperty for graph types where sorting is disabled to avoid confusion.{ description: 'AreaGraph', disabledSplit: ['Category', 'Group', 'Payee', 'Account'], defaultSplit: 'Interval', disabledType: [], defaultType: 'Payment', disableLegend: true, disableSort: true, - defaultSort: 'desc', },Also applies to: 75-75, 85-85, 93-93
107-107: Remove redundantdefaultSortvalues from time graphs.All graph types in
timeGraphOptionshavedisableSort: truebut still specify adefaultSortvalue. Since sorting is disabled for these graphs, thedefaultSortproperty is redundant and should be removed.{ description: 'TableGraph', disabledSplit: ['Interval'], defaultSplit: 'Category', disabledType: ['Net Payment', 'Net Deposit'], defaultType: 'Payment', disableLegend: true, disableLabel: true, disableSort: true, - defaultSort: 'desc', }, { description: 'StackedBarGraph', disabledSplit: ['Interval'], defaultSplit: 'Category', disabledType: [], defaultType: 'Payment', disableSort: true, - defaultSort: 'desc', }, { description: 'LineGraph', disabledSplit: ['Interval'], defaultSplit: 'Category', disabledType: [], defaultType: 'Payment', disableLegend: false, disableLabel: true, disableSort: true, - defaultSort: 'desc', },Also applies to: 116-116, 127-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-client/src/components/reports/ReportSidebar.tsx(3 hunks)packages/desktop-client/src/components/reports/disabledList.ts(7 hunks)packages/loot-core/migrations/1736640000000__custom_report_sorting.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loot-core/migrations/1736640000000__custom_report_sorting.sql
- packages/desktop-client/src/components/reports/ReportSidebar.tsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/reports/disabledList.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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (2)
packages/desktop-client/src/components/reports/disabledList.ts (2)
3-3: LGTM!The type-only import is correctly implemented using the
typekeyword.
53-53: LGTM!The type update improves type safety by constraining
defaultSortto valid sort operations usingsortByOpType.
packages/loot-core/migrations/1736640000000__custom_report_sorting.sql
Outdated
Show resolved
Hide resolved
…ting.sql Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk>
|
Actually, one more thing. This doesn't work for existing edge users because of the migration. It's probably best to add another to fix the database instead of changing the first one. |
|
Just going to try VRT again now to see if this fixes the original things we were seeing with the sorting in the original PR /update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loot-core/migrations/1738491452000__sorting_rename.sql (1)
3-7: Standardization ofsort_byColumn Values.The ALTER TABLE statement updates the default value for
sort_byto'desc', and the subsequent UPDATE statements correctly convert legacy string representations to standardized lower-case values ('Descending' → 'desc', 'Ascending' → 'asc', 'Budget' → 'budget', 'Name' → 'name'). This aligns well with our internal type conventions and promotes consistency across the dataset.Minor Note: Consider verifying that any unexpected or uncategorized values in the
sort_bycolumn are handled appropriately, if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/migrations/1738491452000__sorting_rename.sql(1 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
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 (6)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (ubuntu-latest)
- GitHub Check: compare
🔇 Additional comments (2)
packages/loot-core/migrations/1738491452000__sorting_rename.sql (2)
1-1: Transactional Integrity Initiation.The
BEGIN TRANSACTION;statement ensures that all subsequent modifications are executed atomically. This sets a solid foundation for safe migrations.
9-9: Transaction Completion.The
COMMIT;statement ensures that all the changes within the transaction are finalized. This is critical for maintaining data integrity post-migration.
packages/loot-core/migrations/1738491452000__sorting_rename.sql
Outdated
Show resolved
Hide resolved
Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk>
The sorting of reports on the dashboard