feat: [Insights] [Release 1] Top Categories - Add group-by:category#80333
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e977f624
ℹ️ 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".
|
@TaduJR how's this going? Will we have it ready for review today? |
Yes, @puneetlath wrapping up, adding more unit tests, and reviewing. Will make it ready in 1/1.5 hr |
…egoryListItemHeader
|
Tagging @Expensify/design team since there is usage of icons and other UI additions |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17395a0eb4
ℹ️ 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".
…abilize callback reference
…ts-Release-1-Top-Categories-Add-group-by-category
| const areTransactionsEmpty = useRef(true); | ||
|
|
||
| // Use a ref to access searchContextData in callbacks without causing callback reference changes | ||
| const searchContextDataRef = useRef(searchContextData); |
There was a problem hiding this comment.
Note: iOS Selection Mode Bug Fix (Pre-existing)
Bug: Selection mode on iOS would immediately turn off after long-press on group-by views.
Root Cause: clearSelectedTransactions callback was unstable - it had searchContextData in its dependency array, causing reference changes on every selection. This triggered cleanup in effects that depend on this callback (here), which calls turnOffMobileSelectionMode(). On iOS, navigation transitions keep old component instances mounted longer, so stale instances would incorrectly turn off selection mode.
Fix: Used a ref to read searchContextData inside the callback, keeping the dependency array stable (ref declaration, usage).
cc @JS00001 @luacmartins I find it odd that we need to modify this filter, given the maturity of the Search page and the impact this would bring (it's easy to now have edge cases if we only allow strict equality - we'd avoid returning results that may matter). Could you please give your opinion here? |
|
Another bug, total expenses show 9 but the list has only 7. Might be some BE issue, I'm seeing a similar discrepancy when grouped by "From" as well. 
|
|
Hmm this is a good point. If we're using this for grouping, we would need to not match with LIKE when we expand the filter. We're also going to have to consider this for merchant... I dont think we should be removing LIKE, especially for merchant, so we way need to think of a better way to implement this to work, while keeping the current functionality |
|
I agree, I think we should fix the BE for equality since we only want to group by distinct categories. But I think if that's the only blocker for this, we can merge this. And have that BE fix done today. |
…ts-Release-1-Top-Categories-Add-group-by-category
…ries-Add-group-by-category
…ts-Release-1-Top-Categories-Add-group-by-category
…ries-Add-group-by-category
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
It all looks good to me. I have a couple suggestions to DRY this up for the future.
| [CONST.SEARCH.TABLE_COLUMNS.AVATAR]: null, | ||
| [CONST.SEARCH.TABLE_COLUMNS.GROUP_BANK_ACCOUNT]: 'bankName' as const, | ||
| [CONST.SEARCH.TABLE_COLUMNS.GROUP_WITHDRAWN]: 'debitPosted' as const, | ||
| [CONST.SEARCH.TABLE_COLUMNS.WITHDRAWN]: 'debitPosted' as const, |
There was a problem hiding this comment.
NAB: Why are we adding these?
| if (isTransactionCategoryGroupListItemType(item)) { | ||
| const categoryValue = item.category === '' ? CONST.SEARCH.CATEGORY_EMPTY_VALUE : item.category; | ||
| const newFlatFilters = queryJSON.flatFilters.filter((filter) => filter.key !== CONST.SEARCH.SYNTAX_FILTER_KEYS.CATEGORY); | ||
| newFlatFilters.push({key: CONST.SEARCH.SYNTAX_FILTER_KEYS.CATEGORY, filters: [{operator: CONST.SEARCH.SYNTAX_OPERATORS.EQUAL_TO, value: categoryValue}]}); | ||
| const newQueryJSON: SearchQueryJSON = {...queryJSON, groupBy: undefined, flatFilters: newFlatFilters}; | ||
| const newQuery = buildSearchQueryString(newQueryJSON); | ||
| const newQueryJSONWithHash = buildSearchQueryJSON(newQuery); | ||
| if (!newQueryJSONWithHash) { | ||
| return; | ||
| } | ||
| handleSearch({queryJSON: newQueryJSONWithHash, searchKey, offset: 0, shouldCalculateTotals: false, isLoading: false}); | ||
| return; | ||
| } |
There was a problem hiding this comment.
NAB: This is getting repetitve. Maybe we can do this in a follow up?
Use transactionsQueryJSON from the item
You already compute transactionsQueryJSON in getCategorySections (and the other group section functions in SearchUIUtils.ts). Since each group item already has its drilldown query pre-computed, you could simplify even further:
// All group types have transactionsQueryJSON pre-computed
if (isTransactionMemberGroupListItemType(item) || isTransactionCardGroupListItemType(item) ||
isTransactionWithdrawalIDGroupListItemType(item) || isTransactionCategoryGroupListItemType(item)) {
if (item.transactionsQueryJSON) {
handleSearch({queryJSON: item.transactionsQueryJSON, searchKey, offset: 0, shouldCalculateTotals: false, isLoading: false});
return;
}
}This would be the cleanest solution since it reuses the query already built in SearchUIUtils.ts, avoiding duplicate logic entirely. You'd just need to verify all group item types have transactionsQueryJSON populated (which they should based on the getSections functions we looked at earlier).
There was a problem hiding this comment.
NAB:follow-up:
All of these ListItemHeader components are very similar. I would like to see us refactor these to use a BaseListItemHeader component.
|
✋ 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 staging by https://github.com/neil-marcellini in version: 9.3.10-0 🚀
|
|
This PR failing because of the issue |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.10-6 🚀
|
9 participants
Explanation of Change
This PR implements the
groupBy:categoryfunctionality for the Search feature, allowing users to group expenses by category in the Insights view.Fixed Issues
$ #80034
PROPOSAL: #80034 (comment)
Tests
Prerequisites:
Test 1: Access Group by Category via Advanced Filters
Test 2: Verify Category Grouping Display (Large Screen)
groupBy:categoryfilterTest 3: Verify Category Grouping Display (Small Screen)
groupBy:categoryfilterTest 4: Verify Empty/No Category Handling
groupBy:categoryTest 5: Verify Category Expand/Collapse
groupBy:categoryappliedTest 6: Verify Sorting
groupBy:categoryTest 7: Verify Multi-select Checkbox
groupBy:categoryOffline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: Native
Android-Native.mp4
Android: mWeb Chrome
Android-mWeb.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
iOS-Safari.mp4
MacOS: Chrome / Safari
Macbook-Chrome.mp4