Optimize LHN sidebar sorting with pre-computed sort keys#87847
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
TMisiukiewicz
changed the title
melvin-bot
Bot
requested review from
abzokhattab and
flaviadefaria
and removed request for
a team
|
@abzokhattab 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] |
![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: db5abdd5b1
ℹ️ 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".
) Replace per-comparison localeCompare calls in sortCategorizedReports with a precomputed sort-key map. The map is built once per sort pass from all display names across the five LHN categories, then garbage-collected. Sort keys use NFKD normalization + lowercasing + zero-padded numerics for fast string < comparison. localeCompare is only invoked as a tiebreaker when two sort keys are identical. Advantages over module-level Map cache approach (PR Expensify#87847): - No unbounded cache / memory leak — the map is local to each sort pass - No new field on MiniReport — same type, smaller change surface - Natural numeric ordering (Report 2 before Report 10) without requiring Intl.Collator with numeric:true on every comparison
|
@codex review |
|
@MelvinBot review the PR |
PR ReviewGood performance optimization — reducing 230k 1. Unbounded
|
| Concern | Severity | Action needed |
|---|---|---|
| Unbounded cache | Medium | Add sortKeyCache.clear() at start of categorize function |
| Locale ordering lost | Low-Medium | Acknowledge caseFirst loss; consider if acceptable |
| No i18n test coverage | Low | Add accented character test case |
Next Steps: Reply with @MelvinBot <your feedback> to discuss any of these points.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fd027117a
ℹ️ 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".
Updated ReviewThe latest commit (
Remaining minor note: The Overall this looks good — clean implementation with solid perf gains. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-17.at.01.56.00.movAndroid: mWeb ChromeScreen.Recording.2026-04-17.at.01.50.40.moviOS: HybridAppScreen.Recording.2026-04-17.at.01.56.00.moviOS: mWeb SafariScreen.Recording.2026-04-17.at.01.50.40.movMacOS: Chrome / SafariScreen.Recording.2026-04-17.at.01.48.21.mov |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 753cc055fa
ℹ️ 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".
| return cached; | ||
| } | ||
|
|
||
| const key = displayName.toLowerCase().replaceAll(DIGIT_SEQUENCE, (match) => match.padStart(NUMERIC_PAD_WIDTH, '0')); |
There was a problem hiding this comment.
The fixed-width padding in buildSortKey() regresses numeric ordering for names containing digit runs longer than 15 characters. With NUMERIC_PAD_WIDTH = 15, comparing "Report 999999999999999" and "Report 1000000000000000" produces sort keys where the 16-digit value sorts before the 15-digit one lexicographically, so the sidebar order becomes incorrect for long numeric identifiers. The previous Intl.Collator(..., {numeric: true}) path handled arbitrary-length numeric segments, so this is a functional regression whenever users include long numbers in report names.
Useful? React with 👍 / 👎.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @mountiny 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.61-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are needed for this PR. The changes are a performance optimization to the internal LHN sidebar sorting algorithm (pre-computed sort keys replacing per-comparison |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.3.61-4 🚀
|
5 participants
Explanation of Change
The LHN sidebar sort previously called
Intl.Collator.compare()on every comparison in every sort pass — roughly 230k calls per sort on a large account. Each call is expensive becauseIntl.Collatormust apply locale-sensitive Unicode collation rules at call time.This PR replaces that approach with pre-computed sort keys: each report name is lowercased and has its numeric segments zero-padded, then cached at module level. Sorting then compares plain strings (fast), and only falls back to
Intl.Collatorwhen two sort keys are identical (ties — roughly 1,050 out of 230k comparisons in practice).The tradeoff: locale-specific diacritic ordering (Swedish ä/ö, German ß, Spanish ñ) now uses Unicode codepoint order rather than locale-aware order. Numeric sorting is fully preserved.
Performance results:
Fixed Issues
$ #87939
PROPOSAL:
Tests
Offline tests
N/A
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
Android: Native
Android: mWeb Chrome
iOS: Native
ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov