Multi-level categories with colons are shown with hierarchical indent…#83200
Multi-level categories with colons are shown with hierarchical indent…#83200yuwenmemon merged 4 commits intoExpensify:mainfrom
Conversation
|
@ZhenjaHorbach 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] |
Review: Critical correctness concerns1. PARENT_CHILD_SEPARATOR change — Likely breaks hierarchy buildingCategory names from the backend use
2. Changing isOneLine from true to false in 4 placesShowing hierarchical indentation in search results may be confusing since filtered results lose parent context. 3. Existing tests will fail
RecommendationIf the issue is category names with literal colons being incorrectly treated as hierarchical, the current |
![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: b5b4d64500
ℹ️ 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/CategoryOptionListUtils.ts
Outdated
| })); | ||
|
|
||
| const data = getCategoryOptionTree(searchCategories, true); | ||
| const data = getCategoryOptionTree(searchCategories, false); |
There was a problem hiding this comment.
Switching search results to getCategoryOptionTree(..., false) makes tree-building order-dependent: when a child category is already selected, categoriesForSearch prepends that child before enabledCategories, so getCategoryOptionTree creates a synthetic disabled parent first and then skips the real enabled parent due optionCollection.has(searchText). In large policies (where search is used), this can make a valid parent category unselectable while searching (e.g. selected Food: Meat, search Food).
Useful? React with 👍 / 👎.
| }, | ||
| INDENTS: ' ', | ||
| PARENT_CHILD_SEPARATOR: ': ', | ||
| PARENT_CHILD_SEPARATOR: ':', |
There was a problem hiding this comment.
Changing PARENT_CHILD_SEPARATOR to ':' causes existing category names in the common "Parent: Child" format to split into segments like ' Child'; since getCategoryOptionTree does not trim segments before rendering, child rows/tooltip text gain a leading space and appear over-indented at each level. This is a visible regression for existing policies that already store names with ": ".
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@ijmalik
And actually about this
Did you check this for already-created categories?
There was a problem hiding this comment.
@ZhenjaHorbach Yes, I've already tested this and it's working correctly. My second commit (Multi-level categories - trim leaf) specifically addresses this by trimming the segments before rendering, so existing categories with the ": " format display properly without over-indentation.
|
@ijmalik |
ijmalik
force-pushed
the
82068
branch
2 times, most recently
from
85a856e to
4a12a98
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a12a98a72
ℹ️ 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/CategoryOptionListUtils.ts
Outdated
| const cutRecentlyUsedCategories = filteredRecentlyUsedCategories.slice(0, maxRecentReportsToShow); | ||
|
|
||
| const data = getCategoryOptionTree(cutRecentlyUsedCategories, true); | ||
| const data = getCategoryOptionTree(cutRecentlyUsedCategories, false); |
There was a problem hiding this comment.
Passing false here enables tree-building for Recent items, but the input order is recency-based, not parent-first. When a child (e.g. Food: Meat) appears before its enabled parent (Food), getCategoryOptionTree() creates a synthetic disabled parent from the child and then drops the real parent on the later pass due optionCollection.has(searchText), leaving a valid recent category unselectable. This regression is user-visible in large workspaces where the Recent section is shown and both parent and child categories were used recently.
Useful? React with 👍 / 👎.
src/libs/CategoryOptionListUtils.ts
Outdated
|
|
||
| if (selectedOptions.length > 0) { | ||
| const data = getCategoryOptionTree(selectedOptionsWithDisabledState, true); | ||
| const data = getCategoryOptionTree(selectedOptionsWithDisabledState, false); |
There was a problem hiding this comment.
Do we still need isOneLine param?
If we use false everywhere
There was a problem hiding this comment.
After analyzing the isOneLine usage across the codebase, it appears that it's only used internally within CategoryOptionListUtils and its unit tests. Since we now want hierarchical indentation for all category lists (as per the issue), this parameter seems redundant.
I'll update the PR to:
Remove the isOneLine parameter
Simplify internal calls (no need to pass false)
Update the tests accordingly
This keeps the code cleaner while maintaining the desired behavior. Let me know if you'd like me to proceed with this change.
There was a problem hiding this comment.
@ZhenjaHorbach
Yes, I’ll remove the isOneLine parameter and clean up the related code as discussed. I’ll also update the tests accordingly. I’ll push the changes shortly – Let me know if you have any other concerns before I proceed.
There was a problem hiding this comment.
Only this remark regarding the code
There was a problem hiding this comment.
Thanks @ZhenjaHorbach! I've just pushed the changes removing the isOneLine parameter and cleaned up the related code and tests. The PR is now ready for your final review.
Please let me know if you see anything else that needs adjustment.
|
@ZhenjaHorbach Could you please verify how edge cases like extra spaces before or after the colon are handled in Classic/OldDot and compare them with our PR? We want to ensure our handling remains consistent with existing behavior. In NewDot, the Category field in expense reports displays extra spaces exactly as entered. OldDot also preserves extra spaces after selection. So this part looks consistent. See the attached videos for reference: . Additionally, we noticed a separate issue: categories with multiple consecutive spaces
Current behaviour on staging showing truncated category and our PR is also doing same This seems like a separate UI display issue (likely unrelated to our changes). Should we handle it in this PR or create a follow‑up issue? Thanks for your guidance! |
I suppose it's expected @Expensify/design |
|
I think it's fine if we just use consistent behavior with OldDot for the spaces. I would imagine most people won't set up their categories that way? |
|
Agree - I think we should just mirror Classic's behavior for these. |
|
Thanks @ZhenjaHorbach , @shawnborton, and @dannymcclain for the feedback. This PR keeps the behavior consistent with OldDot as discussed. The truncation of categories with multiple spaces appears to be a separate UI issue. Is there anything else you'd like me to adjust in this PR? Otherwise, I believe it's ready for final review. |
|
Sounds good, let's get it into final review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-03-13.17.21.13.movAndroid: mWeb Chrome2026-03-13.17.18.00.moviOS: HybridApp2026-03-13.17.21.13.moviOS: mWeb Safari2026-03-13.17.18.00.movMacOS: Chrome / Safari2026-03-13.17.10.26.mov |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
LGTM! |
|
🚧 @yuwenmemon has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.3.38-0 🚀
|
|
Deploy Blocker #85357 was identified to be related to this PR. |
|
Deploy Blocker #85359 was identified to be related to this PR. |
|
Deploy Blocker #85361 was identified to be related to this PR. |
|
Deploy Blocker #85364 was identified to be related to this PR. |
|
Revert PR: #85433 |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.38-4 🚀
|
8 participants
…ation
Explanation of Change
Fixed Issues
$ #82068
PROPOSAL:
$ #82068 (comment)
Tests
Offline tests
Same as Tests
QA Steps
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari