Conversation
|
@thelullabyy 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: 58c4309455
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4efa3efe7
ℹ️ 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".
|
@thelullabyy ping me if you need anything from me |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| const canUserPerformWrite = canUserPerformWriteAction(report, isReportArchived); | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| const resolvedOneTransactionThreadReportID = oneTransactionThreadReportID ?? deprecatedCachedOneTransactionThreadReportIDs[report.reportID]; | ||
| const lastAction = getLastVisibleActionIncludingTransactionThread(report.reportID, canUserPerformWrite, undefined, visibleReportActionsData, resolvedOneTransactionThreadReportID); |
There was a problem hiding this comment.
This passes undefined as reportActionsParam, could we add sortedActions param to resolveLastActionContext?
There was a problem hiding this comment.
Tried it. sortedActions can't be passed as reportActionsParam — it's Record<reportID, ReportAction[]> while the param is OnyxCollection<ReportActions> keyed reportActions_<reportID>. The way main actually used it was a .find() fast path in getChatPreviewParts, and that's the divergence this PR fixes: it scans the parent report's sorted array only, so it never sees the one-transaction thread, while LHN (OptionRowLHNData.tsx:105) has always gone through getLastVisibleActionIncludingTransactionThread with oneTransactionThreadReportID.
I implemented the fast path locally to check — LhnSearchPreviewParityTest.ts drops to 146/147, failing "one-transaction expense report whose newest action is the IOU create action" (Search loses the Alice: actor prefix LHN renders).
|
@MelvinBot Please do deep review this PR |
🤖 Deep reviewThe refactor direction is right and the mechanical work is solid — the deleted But the PR's core claim is not actually established, and the new cache it adds to pay for the extra work is defeated in three separate places. Details below, most important first. 1. Expense-report previews still diverge between LHN and Search —
|
| Gap | Where |
|---|---|
No non-English locale test at all. Both parity files do IntlStore.load(CONST.LOCALES.EN). The new fallbackLocaleCompare / fallbackConvertToDisplayStringWithoutCurrency resolve via IntlStore.getCurrentLocale() — under EN they are byte-identical to the hook versions, so a fallback resolving the wrong locale passes every test in this PR. |
OptionsListUtils/index.ts:275-285 |
The parity harness passes convertToDisplayStringWithoutCurrency to LHN but not to getSearchOptions, so the two surfaces aren't configured identically and the fallback is silently taken. |
LhnSearchPreviewParityTest.ts:165,215 |
Untested branches: attachment-only messages, deleted messages / deleted parent action, type: TASK reports, whispers (covered in OptionsListUtilsTest but never LHN-vs-Search). Money amounts are built in fixtures but never asserted. |
— |
Perf test: ACTIONS_PER_REPORT = 8 so the per-report scan from 4(a) isn't measured; transaction-thread IDs 300–599 point at reports that are never created, so the most expensive path walks empty collections; forcePolicyNamePreview: true + MAX_ELEMENTS = 20 mean ~20–100 previews resolve, not 300, contrary to the docblock. |
OptionsListChatPreview.perf-test.ts:28,36,133,164 |
Every parity case reuses reportID: '100', and Onyx.clear() does not purge the deprecated* module caches (the callback short-circuits on falsy actions). Doesn't trip today, but OptionsListUtilsTest.tsx already guards this exact hazard with unique per-test IDs. |
LhnSearchPreviewParityTest.ts:42,385 |
294 lines of the diff (ReportAlternateTextParityTest.ts +31/-31 and its snapshot +147/-147) are pure it() renames — every snapshot value is unchanged. Worth landing separately; as-is it's review noise in a behavior-change PR, and it means no snapshot proves the Search-side string changed. |
— |
Verified clean (so these don't get re-reviewed)
Things that looked risky but check out
- Deleted-branch parity. Every predicate in
isActionWithCustomAlternateText(renamed, task, invite/removed, card-issued, OldDot, policy-copy, moved-transaction, fraud alert, the change-log sets, all 14CUSTOM_ALTERNATE_TEXT_ACTION_NAMES) has an explicit branch ingetReportAlternateText. Nothing dropped. - No dangling references to
getChatPreviewParts/isActionWithCustomAlternateText; the two symbols removed fromReportActionsUtilsexports have no remaining external importers. - No new circular imports. New edges are to
CollatorUtils(type-only) andCurrencyUtils(leaf). SidebarUtils.ts+2/-0 is a genuine no-op for the LHN — both hoisted values are byte-identical to the inline versions, and the empty-array-is-truthy case still works.lastMessageTextFromReportProp || …correctly preserves the''-forces-recompute contract thatOptionRowLHNDatarelies on for pending-whisper transactions.??would have broken it.getLastActorDisplayNamePrefix()extraction is safe — both replaced blocks were identical and the closure reads only never-reassigned values.fallbackLocaleCompareis exactly equivalent to the hook (getCollator(currentLocale).compare, sameIntlStoreseed, per-locale cached, no init required) — and it can't affect which action is picked, onlygetDisplayNamesWithTooltips.fallbackConvertToDisplayStringWithoutCurrencyuses the same locale source and the samegetCurrencyDecimals/CURRENCY_LISTdefaults as the context version.- Dropping
currentUserAccountIDfrom the visibility check is behaviour-preserving (isWhisperActionTargetedToOthersfalls back to the same global), and every Search caller threads a real one anyway — "You" is not lost. resolveLastActionContextmatchesOptionRowLHNData's own derivation line for line, apart from a draft-report fallback that can't regress.- All new
useMemo/useCallbackdeps members are stable —localeCompare/formatPhoneNumber/translateare React-Compiler-verified stable,convertToDisplayStringWithoutCurrencyis auseCallback, and no inline object/function literal was added to any deps array. Deps arrays are complete. - No new Onyx collection subscription — the call sites already had
useSortedReportActionsData();useCardList/useWorkspaceCardListare app-wide context reads, called unconditionally before any early return. - Typing in the router does not recompute per keystroke —
autocompleteQueryValueis debounced. forcePolicyNamePreviewhoisting is equivalent (the two report types are mutually exclusive).- Loss of the
iou.expense/common.grouptype fallbacks for message-less reports, and archived rooms losinggetReportSubtitlePrefix, are both intentional LHN parity. - No new non-null assertions, no removed optional chaining, no hardcoded English, no
.only/.skip.
Minor / optional: currentUserLogin is sourced from currentUserEmail in two call sites and personalDetails.login in two others — the LHN uses login, so unify on that. ShareTab.tsx and InSelector.tsx pass convertToDisplayStringWithoutCurrency but both set isUsedInChatFinder: false, so it is unreachable there (the translate added in InSelector is live and is a real fix). Three of five consumers now hold a second useOnyx subscription to VISIBLE_REPORT_ACTIONS. createOption/createOptionFromReport have no way to supply cardList/localeCompare/etc — harmless today since no caller sets showChatPreviewLine, but the first one that does gets wrong strings rather than a type error.
Suggested minimum before merge: items 1, 2, 4(b), 4(c) and 4(d). 1 and 2 go to whether this PR achieves what it says; 4(b)–(d) are one-line fixes that decide whether the new cache does anything at all.
Verification run
npm test -- tests/unit/LhnSearchPreviewParityTest.ts tests/unit/ReportAlternateTextParityTest.ts \
tests/unit/OptionsListUtilsTest.tsx tests/unit/SidebarUtilsTest.ts --ci
→ Test Suites: 4 passed | Tests: 734 passed | Snapshots: 147 passed | 40s
npm run typecheck → passed (all 5 tsconfigs)
npm run lint (11 files) → clean, exit 0
gh pr checks → 32/33 pass; only `Check independent approval` fails (needs a human approver)
Reviewed at d32a7e0. Raw npx eslint reports 20 errors on the changed files, but all 20 are grandfathered by the repo's seatbelt baseline — pre-existing, not introduced here.
Reply with @MelvinBot <your feedback> to dig into any of these further.
Confirmed and reproduced via unit tests. A one-transaction expense report renders differently for our both cases:
This is because different argument is passed in both cases. For Search we pass
The question is if that exact path would be visible by user at all? I tried to reproduce this but it seems that one-transaction reports are not visible neither in LHN nor Search 
|
The line The second part |
This one is 100% valid. I added additional test case to verify missing part |
Proposed fixes applied. Not done: replacing the O(n)-no-early-exit scan with a precomputed lastVisibleActions map on the sortedReportActions derived value. That's a derived-value change well outside this PR — and the scan is the same one the LHN pays, so it isn't a parity regression. If needed it should be done in external PR/issue. |
Fixed except for last row as this is not coverage gap. |
|
@thelullabyy I've responded to each point of Melvin's review |
|
@MelvinBot review |
|
🤖 Code review — the refactor is sound and all CI checks pass. Two places where "identical by construction" doesn't hold yet, both from the same root cause, plus a perf-evidence gap. 1. Search short-circuits the shared recompute with a weaker precomputed string
a. DEW "queued to submit/approve" is unreachable in Search. b. Archived-reason previews can name the wrong person. Neither is a regression — 2. LHN still reads the deprecated one-transaction-thread cache while Search reads the derived value
3. The perf numbers describe the warm cache, so they can't see the extra scanDetails
The new Separately, the cache comment at Smaller notesDetails
TestsDetails
Uncovered branches that users see constantly: Two existing assertions were weakened rather than updated:
Also untested: the new ~60-line I reviewed statically only — no app run, so none of the above is confirmed against a live large account. Reply with |
… derived OTT ID, remove dead card plumbing
…-parity-melvin-r2 Melvin r2: drop search preview precompute, derived OTT ID, remove card plumbing
This branch has not been deployed
3 participants
Explanation of Change
Makes Search row subtitles (
alternateText) identical to LHN by construction.This is the fifth and final piece of #99062, which was split into five smaller PRs. The four prerequisites are already merged.
What is left in this PR:
getReportAlternateText, the exact same entry point the LHN uses, instead of re-deriving it along a separate code path.resolveLastActionContext, so LHN and Search read the same sorted/visible report actions and the same one-transaction-thread report ID.getChatPreviewParts,isActionWithCustomAlternateText), which existed only to approximate LHN output from the Search side.localeCompare/convertToDisplayStringWithoutCurrencyfall back toCollatorUtilsandconvertToDisplayStringWithoutCurrencyForLocale, so the shared path stays reachable from everywhere.Performance tests (against current
main):Fixed Issues
$ #98958
PROPOSAL:
Tests
npx jest tests/unit/LhnSearchPreviewParityTest.ts tests/unit/ReportAlternateTextParityTest.ts tests/unit/OptionsListUtilsTest.tsx tests/unit/SidebarUtilsTest.tsOffline tests
N/A — preview text is derived entirely from local Onyx state, so there is no network-dependent branch.
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-16.o.10.46.07.mov