Fix alternate text prefix mismatch between LHN and search - #98961
Conversation
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.
|
|
@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] |
|
We are adding some logic to Opening Search so there is expected some performance regression from this. For big account on iOS simulator I calculated 7% regression 
|
![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: 08213a14cc
ℹ️ 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}
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3a4dc826a
ℹ️ 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".
| const sortedActionsForReport = sortedActions?.[report.reportID]; | ||
| const lastAction = sortedActionsForReport | ||
| ? sortedActionsForReport.find((action) => isReportActionVisibleAsLastAction(action, canUserPerformWrite, visibleReportActionsData, report.reportID, currentUserAccountID)) | ||
| : getLastVisibleAction(report.reportID, canUserPerformWrite, {}, undefined, visibleReportActionsData); |
There was a problem hiding this comment.
When createOption() receives sortedActions, it uses them to compute result.lastMessageText but does not forward them into getAlternateText(), so this fallback scans only the parent report's actions. For a one-transaction report whose newest visible comment is in the transaction thread, the preview text therefore comes from the child action while the newly added actor prefix comes from the parent's last action; OptionRowLHNData includes the transaction thread when selecting lastAction, so the picker/search option can attribute the message to a different person than the LHN. Forward sortedActions through that call or make this fallback include the transaction thread.
Useful? React with 👍 / 👎.
| reportAttributesDerived, | ||
| }) | ||
| : {actorPrefix: '', customAlternateText: undefined}; | ||
| const formattedLastMessageTextWithPrefix = reportPrefix + actorPrefix + (customAlternateText ?? formattedLastMessageText); |
There was a problem hiding this comment.
For regular messages at the 200-character preview limit, formattedLastMessageText is truncated before actorPrefix is prepended. The LHN instead calls formatReportLastMessageText() on the combined "Name: message", so it truncates the message early enough that the whole actor-prefixed preview remains within the limit. This produces different preview strings—and an over-limit search alternate text—precisely for long messages; format the combined actor and message before adding the report subtitle prefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There is ongoing effort to completely merge LHN and Search visibility of alternateText so I treat this PR as immediate fix for not showing the actor in Search.
I've tested solution for this comment and it may take a lot of time for this edge case to resolve so I proposed that we leave this and focus on the next step. Let me know if you disagree with me - I'm open for discussion
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-08-25.at.17.41.16.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-25.at.17.36.44.mov |
| isPolicyCopyReportAction(lastAction) || | ||
| isMovedTransactionAction(lastAction) || | ||
| (isActionOfType(lastAction, CONST.REPORT.ACTIONS.TYPE.ACTIONABLE_CARD_FRAUD_ALERT) && !!getOriginalMessage(lastAction)?.resolution) || | ||
| POLICY_CHANGE_LOG_ACTION_NAMES.has(actionName) || |
There was a problem hiding this comment.
Hmm, I don't think we handle alternative text for all actions in POLICY_CHANGE_LOG
They fall through to the generic Name: message branch and do get a prefix in the LHN, example: LEAVE_POLICY, DELETE_CATEGORIES,...
So for those, search now drops a prefix the LHN shows https://github.com/Expensify/App/pull/98961/changes#diff-48f7edf19f169a2c5131f019a4701a02e1207efdf4ce3d00b1e81c247330e80fR617 @sosek108
There was a problem hiding this comment.
Good comment, done
| const sortedActionsForReport = sortedActions?.[report.reportID]; | ||
| const lastAction = sortedActionsForReport | ||
| ? sortedActionsForReport.find((action) => isReportActionVisibleAsLastAction(action, canUserPerformWrite, visibleReportActionsData, report.reportID, currentUserAccountID)) | ||
| : getLastVisibleAction(report.reportID, canUserPerformWrite, {}, undefined, visibleReportActionsData); |
There was a problem hiding this comment.
I think we should pass currentUserAccountID, we are should not introduce new caller which missing that param as per #66408
|
I think we should add UTs for new functions @sosek108 |
|
@thelullabyy thanks for feedback, I'm working on these right now |
|
@thelullabyy I've merged fresh main |
|
@thelullabyy do you need anything from me to continue this review? |
|
Reviewing... |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #98958 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c396864f87
ℹ️ 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".
| const sortedActionsForReport = sortedActions?.[report.reportID]; | ||
| const lastAction = sortedActionsForReport | ||
| ? sortedActionsForReport.find((action) => isReportActionVisibleAsLastAction(action, canUserPerformWrite, visibleReportActionsData, report.reportID, currentUserAccountID)) | ||
| : getLastVisibleAction(report.reportID, canUserPerformWrite, {}, undefined, visibleReportActionsData, currentUserAccountID); |
There was a problem hiding this comment.
For a one-transaction report whose newest visible comment is in its transaction thread, this lookup only examines sortedActions[report.reportID] (or the parent's cached actions), while OptionRowLHNData selects across both the parent and transaction-thread reports and getLastMessageTextForReport() can likewise derive the preview text from that child. The resulting search row can therefore show the child comment with the parent's actor prefix. Fresh evidence in this revision is that the newly added helper explicitly restricts actor resolution to the parent report; include the one-transaction thread when selecting lastAction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — with one clarification on scope: when callers pass the derived sortedActions, the one-transaction thread was already covered, because RAM_ONLY_SORTED_REPORT_ACTIONS stores the combined parent + transaction-thread actions (via getCombinedReportActions), so the find() over sortedActions[report.reportID] already selects child comments. The gap was only in the fallback branch for callers that don't pass sortedActions yet (#66381): it used getLastVisibleAction, which scans the parent report only.
The fallback now uses getLastVisibleActionIncludingTransactionThread with deprecatedCachedOneTransactionThreadReportIDs — the exact same lookup getLastMessageTextForReport uses to derive the preview text — so the actor prefix and the message text are selected from the same action. Added a regression test with a one-transaction expense report whose newest visible action is a transaction-thread comment; it fails on the previous revision (parent actor prefix) and passes now (thread comment actor).
| const isLastActionAddComment = report?.lastActionType === CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT; | ||
| const formattedLastMessageText = | ||
| formatReportLastMessageText(Parser.htmlToText(option.lastMessageText ?? '')) || | ||
| formatReportLastMessageText(isLastActionAddComment ? (option.lastMessageText ?? '') : Parser.htmlToText(option.lastMessageText ?? '')) || |
There was a problem hiding this comment.
When a one-transaction report's newest visible message is an ADD_COMMENT in its transaction thread, the parent commonly still has lastActionType === REPORT_PREVIEW. Although option.lastMessageText is derived from the child comment, this parent-level check takes the HTML-stripping branch, so a literal comment such as <b>test</b> becomes test in search while the LHN preserves the typed text. Base this decision on the same resolved last visible action used for the preview rather than report.lastActionType.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this exact scenario was identified and handled earlier. It's even in proposed test steps of this PR.
Code reviewOverview: This PR makes the search-router/picker chat preview ( 🟠 Prefix mismatch still possible for
|
| type ChatPreviewParts = { | ||
| actorPrefix: string; | ||
| customAlternateText?: string; | ||
| }; |
| const sortedActionsForReport = sortedActions?.[report.reportID]; | ||
| const lastAction = sortedActionsForReport | ||
| ? sortedActionsForReport.find((action) => isReportActionVisibleAsLastAction(action, canUserPerformWrite, visibleReportActionsData, report.reportID, currentUserAccountID)) | ||
| : getLastVisibleAction(report.reportID, canUserPerformWrite, {}, undefined, visibleReportActionsData, currentUserAccountID); |
|
Response to MelvinBot's comment: finding was correct. I added On the 🟡 custom-text body divergence: correct and intentional. This PR scopes to prefix parity plus the actions whose LHN text embeds the actor (rename, leave, invite/remove). Full body parity for the remaining custom actions belongs to the ongoing LHN/Search alternate-text merge effort and will be handled there. It will be resolved with next PR that will be posted to review right after this one is merged |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks for addressing those
|
🚧 francoisl 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/francoisl in version: 9.4.61-0 🚀
|
Help site review — no docs changes requiredI reviewed the changes in this PR against the help site articles under Why: This PR is an internal UI-consistency bug fix. It makes the chat preview line shown in the search router / pickers ( The help site documents user-facing features and how to use them; it does not document the micro-formatting of chat message preview lines in the LHN or search. I searched If you believe a specific help article does describe this behavior and should be updated, point me to it and I'll take another look. |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.61-4 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
The alternate text preview shown for a chat in the search router / pickers (built by
OptionsListUtils.getAlternateText) could disagree with the preview shown for the same chat in the LHN (built bySidebarUtils.getOptionData). Two mismatches were fixed:shouldShowLastActorDisplayName). Search was not mirroring this logic. Added an internal helper,getChatPreviewActorPrefix, inOptionsListUtilsthat reproduces the LHN's exact two-regime behavior, including the actor resolution fallback chain (last action's actor →action.person→report.lastActorAccountID).isActionWithCustomAlternateTextinOptionsListUtils, mirroring that same if/else chain, so search no longer adds a prefix to those actions.SearchAutocompleteListhad a display-layer override that ran the alternate text throughParser.htmlToTextunlessreport.lastActionType === ADD_COMMENT. This logic has been moved intogetAlternateTextitself, so literal HTML-like text a user types (e.g.<b>test</b>) now displays as typed consistently, matching the LHN. The now-redundant override inSearchAutocompleteListwas removed, leavingalternateTextwith a single source of truth.Fixed Issues
$ #98958
PROPOSAL:
Tests
You: <message>)<Name>: <message>)<b>test</b>(LHN shows it as typed)You:/<Name>:prefix, HTML-like text shown as typed).Offline tests
N/A
QA Steps
Same as tests
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-08-19.o.11.28.32.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Nagranie.z.ekranu.2026-08-19.o.10.27.04.mov
Nagranie.z.ekranu.2026-08-19.o.10.27.51.mov