Repository navigation
Conversation
…hn-alternate-text # Conflicts: # src/libs/OptionsListUtils/index.ts # src/libs/ReportAlternateTextUtils.ts
…hn-alternate-text # Conflicts: # tests/ui/WorkspaceCompanyCardPageEmptyStateTest.tsx
|
@MelvinBot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f1c093e34
ℹ️ 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".
|
Reviewed the diff. The refactor direction is right — collapsing 1. Search and LHN still resolve
|
|
@MelvinBot review |
|
All three findings from my last pass are fixed at What got resolved
1. The "clean up comment" commit stripped rationale that should stay
The author checklist asks for comments explaining "why" on code that isn't self-explanatory. These four qualify. 2. Preview resolution got slower per row — worth a manual sanity checkMoving to the LHN resolver was the right correctness call, but it changed the cost profile. The deleted That happens once per option inside
Web verification🤖 Web test steps verification results: ✅ pass Subtitle text matched between the LHN and the search router for the one chat present in both lists. Coverage is thin: the test account is fresh and has a single conversation, so this confirms the pipeline runs and produces identical output, not that the edge cases match.
|
mountiny
left a comment
There was a problem hiding this comment.
Nice cleanup — routing Search through getReportAlternateText is the right call, and passing the already computed reportMetadata and participantPersonalDetailListExcludeCurrentUser down from SidebarUtils.getOptionData removes real duplicated work per LHN row.
One request, plus a few non-blocking notes inline.
Please restore tests/unit/ReportAlternateTextParityTest.ts and its snapshot
I ran the deleted suite against this branch: 145 of 145 snapshots pass unchanged. Keeping it costs nothing and CI stays green:
git checkout dc10c9e0a0dac855412530ec08a68ac6798ecc05 -- tests/unit/ReportAlternateTextParityTest.ts tests/unit/__snapshots__/ReportAlternateTextParityTest.ts.snap
It pins the expected string for 115 action types. Only 25 of those appear in SidebarUtilsTest.ts, so 90 action types lose their only pinned expected output. LhnSearchPreviewParityTest asserts searchText === lhnText plus one toContain, so a change that moves both surfaces together is invisible to it.
That matters here specifically because this PR changes getLastActorDisplayNameFromLastVisibleActions to use getLastVisibleActionIncludingTransactionThread, and the LHN goes through that same function — the snapshot is the evidence that no LHN output shifted. The two suites cover different axes, so keep both.
Verified locally on c16419e
| Check | Result |
|---|---|
tsc --noEmit |
clean |
LhnSearchPreviewParityTest + OptionsListUtilsTest + SidebarUtilsTest |
574 passing |
useSearchSelectorTest + useAutocompleteSuggestions + InSelectorTest |
56 passing |
eslint on changed files |
only pre-existing seatbelt patterns, nothing on new lines |
Minor: the PR body still carries the // TODO: These must be filled out line under QA Steps.
| formatPhoneNumber: formatPhoneNumber ?? formatPhoneNumberPhoneUtils, | ||
| dateFnsLocale, | ||
| convertToDisplayString, | ||
| convertToDisplayStringWithoutCurrency: convertToDisplayStringWithoutCurrency ?? fallbackConvertToDisplayStringWithoutCurrency, |
There was a problem hiding this comment.
convertToDisplayStringWithoutCurrency is unreachable, so this fallback is always the implementation.
It is declared on GetAlternateTextConfig (line 251) but not on GetOptionsConfig (types.ts), SearchOptionsConfig, or prepareReportOptionsForDisplay's config. getAlternateText is only reached from createOption, prepareReportOptionsForDisplay, and InSelector, and none of them can supply it.
fallbackConvertToDisplayStringWithoutCurrency (line 283) also duplicates CurrencyListContextProvider/index.tsx lines 58-80 verbatim, comments included. Unlike sortedActions and currentUserAccountID, this param has neither a caller nor a TODO pointing at a refactor issue.
Either thread it down from useCurrencyListActions() alongside convertToDisplayString, or drop the param and the fallback.
There was a problem hiding this comment.
The params thread is done and fallback duplicated code is gone.
It added some files to the diff as the change touched at least 7 callers
| const locale = IntlStore.getCurrentLocale(); | ||
| if (!fallbackCollator || fallbackCollatorLocale !== locale) { | ||
| fallbackCollatorLocale = locale; | ||
| fallbackCollator = new Intl.Collator(locale, {usage: 'sort', sensitivity: 'variant', numeric: true, caseFirst: 'upper'}); |
There was a problem hiding this comment.
This is the fourth copy of these collator options in the codebase:
LocaleContextProvider.tsx:97(COLLATOR_OPTIONS)ReportNameUtils.ts:269SearchQueryUtils.ts:556
Worth exporting one shared helper so they cannot drift apart.
There was a problem hiding this comment.
Extracted Collator to CollatorUtils
| policyCollection: allPolicies, | ||
| personalDetails, | ||
| sortedActions, | ||
| cardList, |
There was a problem hiding this comment.
cardList and workspaceCardList cannot be read on this path.
Line 84 sets isUsedInChatFinder: false, which makes showChatPreviewLine false inside getSearchOptions, and both values are only consumed inside that branch (OptionsListUtils/index.ts:347). The getAlternateText(report, {}, ...) call in InSelector is the same story.
Safe to drop these two lines along with the useCardList() / useWorkspaceCardList() calls on lines 60-61.
| const resolvedCurrentUserAccountID = currentUserAccountID ?? CONST.DEFAULT_NUMBER_ID; | ||
| const oneTransactionThreadReportID = transactionThreadIDs?.[report.reportID]; | ||
| const {lastAction, lastActionReport, movedFromReport, movedToReport} = resolveLastActionContext(report, isReportArchived, visibleReportActionsData, oneTransactionThreadReportID); | ||
| const card = isCardIssuedAction(lastAction) ? getExpensifyCardFromReportAction({reportAction: lastAction, policy, cardList, workspaceCardList}) : undefined; |
There was a problem hiding this comment.
Worth a Reassure run before merge.
The LHN pays this resolution once per visible row (roughly 20), but prepareReportOptionsForDisplay is not memoised the way createFilteredOptionList is, so Search now runs the full getReportAlternateText per option up to maxElements.
tests/perf-test/OptionsListUtils.perf-test.ts and tests/perf-test/SearchRouter.perf-test.tsx both cover this path.
There was a problem hiding this comment.
Good call. Two findings.
The two suites you named don't cover this path. createRandomReport (tests/utils/collections/reports.ts:13) sets no participants and no chatType, so processReport (OptionsListUtils/index.ts:988) drops all 5000 fixture reports before createOption. Both files use that factory, so getLastMessageTextForReport and getReportAlternateText never run in either. Local Reassure also can't resolve deltas this size: main vs main reported "significant" regressions up to +55%.
So I added covering scenarios (tests/perf-test/OptionsListChatPreview.perf-test.ts) — participant-bearing reports with report actions, deterministic fixtures, showChatPreviewLine: true, two result caps. Min of 4 interleaved runs per state:
| Scenario | main | this branch | branch + memo |
|---|---|---|---|
getValidOptions previews, cap 20 |
26.74 ms | 28.56 ms (+6.8%) | 25.09 ms (−6.2%) |
getValidOptions previews, cap 100 |
29.88 ms | 34.82 ms (+16.5%) | 24.94 ms (−16.5%) |
createFilteredOptionList (build) |
26.44 ms | 21.78 ms (−17.6%) | 21.49 ms (−18.7%) |
Your read was right — the preview cost is real and scales with maxElements. The build path is ~18% faster because threading lastActions/transactionThreadIDs through createOption drops the per-report Onyx rescan.
Memo added: prepareReportOptionsForDisplay caches the preview per report, dropping the cache when any input feeding it changes identity (same shape as the createFilteredOptionList cache). One prerequisite — both it and getValidOptions defaulted visibleReportActionsData to a fresh {}, which the share-destination caller doesn't pass, so the guard invalidated every call (0 hits / 1320 misses); pointing both at the existing EMPTY_VISIBLE_REPORT_ACTIONS gives 1220 hits / 100 misses.
Caveat: those means are warm-cache dominated (1 cold + 10 warm), so the first call after a guarded input changes still costs the pre-memo amount.
…ithoutCurrencyForLocale(locale, amountInCents, currency, getCurrencyDecimalsImpl); the existing …EnLocale variant now delegates to it.
| formatPhoneNumber: formatPhoneNumber ?? formatPhoneNumberPhoneUtils, | ||
| dateFnsLocale, | ||
| convertToDisplayString, | ||
| convertToDisplayStringWithoutCurrency: convertToDisplayStringWithoutCurrency ?? fallbackConvertToDisplayStringWithoutCurrency, |
There was a problem hiding this comment.
The params thread is done and fallback duplicated code is gone.
It added some files to the diff as the change touched at least 7 callers
| const locale = IntlStore.getCurrentLocale(); | ||
| if (!fallbackCollator || fallbackCollatorLocale !== locale) { | ||
| fallbackCollatorLocale = locale; | ||
| fallbackCollator = new Intl.Collator(locale, {usage: 'sort', sensitivity: 'variant', numeric: true, caseFirst: 'upper'}); |
There was a problem hiding this comment.
Extracted Collator to CollatorUtils
| policyCollection: allPolicies, | ||
| personalDetails, | ||
| sortedActions, | ||
| cardList, |
| const resolvedCurrentUserAccountID = currentUserAccountID ?? CONST.DEFAULT_NUMBER_ID; | ||
| const oneTransactionThreadReportID = transactionThreadIDs?.[report.reportID]; | ||
| const {lastAction, lastActionReport, movedFromReport, movedToReport} = resolveLastActionContext(report, isReportArchived, visibleReportActionsData, oneTransactionThreadReportID); | ||
| const card = isCardIssuedAction(lastAction) ? getExpensifyCardFromReportAction({reportAction: lastAction, policy, cardList, workspaceCardList}) : undefined; |
There was a problem hiding this comment.
Good call. Two findings.
The two suites you named don't cover this path. createRandomReport (tests/utils/collections/reports.ts:13) sets no participants and no chatType, so processReport (OptionsListUtils/index.ts:988) drops all 5000 fixture reports before createOption. Both files use that factory, so getLastMessageTextForReport and getReportAlternateText never run in either. Local Reassure also can't resolve deltas this size: main vs main reported "significant" regressions up to +55%.
So I added covering scenarios (tests/perf-test/OptionsListChatPreview.perf-test.ts) — participant-bearing reports with report actions, deterministic fixtures, showChatPreviewLine: true, two result caps. Min of 4 interleaved runs per state:
| Scenario | main | this branch | branch + memo |
|---|---|---|---|
getValidOptions previews, cap 20 |
26.74 ms | 28.56 ms (+6.8%) | 25.09 ms (−6.2%) |
getValidOptions previews, cap 100 |
29.88 ms | 34.82 ms (+16.5%) | 24.94 ms (−16.5%) |
createFilteredOptionList (build) |
26.44 ms | 21.78 ms (−17.6%) | 21.49 ms (−18.7%) |
Your read was right — the preview cost is real and scales with maxElements. The build path is ~18% faster because threading lastActions/transactionThreadIDs through createOption drops the per-report Onyx rescan.
Memo added: prepareReportOptionsForDisplay caches the preview per report, dropping the cache when any input feeding it changes identity (same shape as the createFilteredOptionList cache). One prerequisite — both it and getValidOptions defaulted visibleReportActionsData to a fresh {}, which the share-destination caller doesn't pass, so the guard invalidated every call (0 hits / 1320 misses); pointing both at the existing EMPTY_VISIBLE_REPORT_ACTIONS gives 1220 hits / 100 misses.
Caveat: those means are warm-cache dominated (1 cold + 10 warm), so the first call after a guarded input changes still costs the pre-memo amount.
68cf36f to
dc77b33
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Decided to split this one into five smaller: Marking this one as Draft |
|
I think we can close this now as we we are breaking it up to smaller prs |


Explanation of Change
Makes SearchRouter / Search row subtitles (
alternateText) identical to LHN by construction.getReportAlternateText- same as LHNgetChatPreviewParts,isActionWithCustomAlternateText)A new test file
tests/unit/LhnSearchPreviewParityTest.tswill assert strict LHN === Search equality on identical Onyx state.Fixed Issues
$ #98958
PROPOSAL:
Tests
npx jest tests/unit/LhnSearchPreviewParityTest.ts tests/unit/SidebarUtilsTest.ts tests/unit/OptionsListUtilsTest.tsxOffline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Nagranie.z.ekranu.2026-09-1.o.12.49.39.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Nagranie.z.ekranu.2026-09-1.o.12.43.28.mov