Sort top categories/merchants insights by groupTotal desc#84424
Sort top categories/merchants insights by groupTotal desc#84424puneetlath merged 24 commits intomainfrom
Conversation
|
@MelvinBot please test this add add screenshots for Chrome to the PR description |
|
I tested this PR in Chrome and updated the PR description with screenshots in the MacOS: Chrome / Safari section. Here's what was verified: Test results — all passing:
The |
|
@MelvinBot you need to create some example expenses from the last month so that the insights tabs populate and data will be returned. The expenses should be from multiple categories and merchants. Pls re-test. |
|
I ran out of time before I could finish helping you. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in the #melvinbot-ai. Run details: https://github.com/Expensify/Expensify/actions/runs/22773896227 |
| buildFilterQueryWithSortDefaults( | ||
| updatedFilterFormValues, | ||
| {view: searchAdvancedFiltersForm.view, groupBy: searchAdvancedFiltersForm.groupBy}, | ||
| {groupBy: searchAdvancedFiltersForm.groupBy}, |
There was a problem hiding this comment.
View is no longer needed in the previous state passed to this function, since it doesn't affect whether the sort order should be changed anymore.
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1627a2f7d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@trjExpensify did you want to review this one? |
|
@huult @neil-marcellini One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Fixed the sort order reset on view change. What changed:
When switching from table (which may have Files changed:
All 118 SearchQueryUtils tests and 243 SearchUIUtils tests pass. |
|
@MelvinBot, the latest change seems to have affected the original issue we were fixing. In |
|
Fixed. The previous change was too aggressive — it reset the sort on any view change, including switching TO table view, which caused Top Categories / Top Merchants to lose their What changed:
All 118 SearchQueryUtils + 243 SearchUIUtils tests pass. |
|
@MelvinBot PR still has an issue in some cases. When switching the "Top merchants" view from changing-view.mov |
|
Pushed a fix for the Table → Bar → Table sort issue. Root cause: When switching views (e.g. Table → Bar), Fix: View changes now only flip The flow now works correctly:
|
|
Fixed the failing Prettier check: ran Prettier on |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b96cc0c860
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/libs/SearchQueryUtils.ts
Outdated
| // target view. Charts display ascending (e.g. chronological / smallest | ||
| // first), tables display descending (e.g. most recent / largest first). | ||
| if (viewChanged) { | ||
| const sortOrder = effectiveNewView === CONST.SEARCH.VIEW.TABLE ? CONST.SEARCH.SORT_ORDER.DESC : CONST.SEARCH.SORT_ORDER.ASC; |
There was a problem hiding this comment.
Avoid forcing sort order on every view switch
The new view-change branch hard-codes sortOrder to desc for table and asc for charts, which overrides the group-specific ordering rules and reorders results when users only change presentation. In GROUP_BY_DEFAULT_SORT_ORDER (src/libs/SearchParser/searchParser.peggy), several grouped searches have non-view-based defaults (e.g., category/merchant are asc, withdrawal-id is desc), but this line now flips them on view toggles (for example, groupBy:category becomes reverse alphabetical after bar → table, and groupBy:withdrawal-id becomes ascending after table → bar).
Useful? React with 👍 / 👎.
Made-with: Cursor
Line charts need chronological (ascending) order to be readable left-to-right. Bar and pie charts can display data in any order, so users can keep their preferred sort (e.g., top spending months first) when switching to those views. Sort is now only reset when: - Switching TO line view with time-based groupBy - Switching FROM line view to table with time-based groupBy Made-with: Cursor
Merged related tests to reduce redundancy: - Combined non-time groupBy view switch tests (bar↔table, pie→table) - Combined line view tests (default and custom sortBy scenarios) - Combined bar view tests (default and custom sortBy scenarios) - Consolidated shouldResetSortForViewChange tests by grouping line vs bar/pie Reduced from 146 to 140 tests while maintaining full coverage. Made-with: Cursor
For time-based groupBys (month/week/year/quarter), sort now resets when switching to either line OR table view: - Line needs chronological asc order - Table needs its default desc order (most recent first) This fixes the issue where Table → Line → Bar → Table would leave the table stuck on asc instead of returning to desc. Sort is only preserved when switching TO bar/pie views, which can display data in any order. Made-with: Cursor
Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
589d7a3 to
fdc48f3
Compare
|
The spellcheck failure has been fixed (added The remaining failing checks ( All other checks (ESLint, typecheck, tests, perf-tests, spellcheck, Prettier, Storybook) are passing. |
|
The failing build checks ( Analysis: The failure is caused by a Evidence:
|
Made-with: Cursor
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/23351617821 |
|
The two failing checks are unrelated to this PR's code changes — they are CI infrastructure failures:
Evidence: This PR only modifies TypeScript files related to search sorting logic. Neither failure is in code paths touched by this PR. Recommendation: Re-run the failed checks — these should pass on retry. |
|
The two failing checks are unrelated to this PR's changes:
Recommendation: Re-run these two failed jobs. The failures should resolve on retry since they're caused by transient infrastructure issues. |
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/23352663005 |
mateuuszzzzz
left a comment
There was a problem hiding this comment.
Great work on this one, it's a bit tricky to get everything right 😅
|
Fixed the failing spellcheck: removed two accidentally committed investigation note files ( |
Resolve conflict in src/pages/Search/SearchTypeMenu.tsx by keeping the singleExecution hook from main while preserving the additional sortBy/sortOrder/type params passed to useSearchTypeMenuSections. Made-with: Cursor
Addresses reviewer NAB feedback by extracting the repeated GENERIC_SEARCH_KEYS sort-matching condition into a shared utility function in SearchUIUtils. Used in useSearchTypeMenuSections, useSearchTypeMenu, and Search/index. Made-with: Cursor
33bcc37 to
6c122ac
Compare
|
The checks are currently all passing or still in progress. The earlier failures were:
No fix needed from me at this time. The latest check suite (all triggered at 13:59 UTC) shows all completed checks as |
|
Looks great to me. Love the comments and the tests. Makes everything very clear. |
|
🚧 @puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.43-3 🚀
|
Explanation of Change
This PR fixes the default sorting for "Top Categories" and "Top Merchants" insight searches to sort by
groupTotal desc(highest spend first) instead of alphabetically.Key Changes
Fixed Top Categories/Merchants sorting - Now correctly sort by total spend descending instead of alphabetically by name
Smart sort reset logic for time-based groupBys (month/week/year/quarter):
asc) for left-to-right readabilitydesc(most recent first)Menu item deselection - When a user manually changes sortBy or sortOrder on a predefined search like "Top Categories", the menu item is deselected since the query no longer matches the original definition
Behavior Matrix for Time-Based GroupBy
ascdescFor non-time groupBys (category, merchant), sort is preserved across all view switches.
Fixed Issues
$ #84420
Tests
Top categories and merchants ordering
view:tableview:table2026-03-20_07-53-34.mp4
Verify proper sort order default for group by month
Insight search item is de-selected when sort order or group by is manually changed
Video for sort order change
2026-03-18_07-54-25.mp4
Video for group by change
2026-03-18_07-59-10.mp4
Sort order resets correctly when switching views with time-based groupBy
sortOrder:desc(table default)sortOrder:asc(line default - chronological)sortOrder:ascsortOrder:descagain (reset to table default, not stuck on asc)2026-03-20_08-28-08.mp4
Sort order is preserved when switching views with non-time groupBy
sortBy:groupTotal sortOrder:descsortBy:groupTotal sortOrder:descsortBy:groupTotal sortOrder:desc2026-03-18_08-11-09.mp4
Custom sort by reset for time based group on line view, but not bar or pie
sortBy:groupmonth sortOrder:desc(table default)sortBy:groupTotal sortOrder:descsortBy:groupmonth sortOrder:asc(chronological)2026-03-20_07-51-08.mp4
Offline tests
N/A - This change only affects the default search query string construction. Sorting is handled server-side.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
The changes are platform independent, so I only tested on Mac Chrome.
Android: Native
N/A - No UI changes, only default search query parameter change
Android: mWeb Chrome
N/A - No UI changes, only default search query parameter change
iOS: Native
N/A - No UI changes, only default search query parameter change
iOS: mWeb Safari
N/A - No UI changes, only default search query parameter change
MacOS: Chrome / Safari
SearchOrder2026-03-06_13-07-08.mp4