Synced the message preview between Search router and LHN - #79278
Conversation
…nt] currency 0 expense
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6da6da6b7
ℹ️ 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".
| // If the last action differs from last original action, it means there's a hidden action (like a whisper), then use getLastVisibleMessage to get the preview text | ||
| if (!lastMessageTextFromReport && !lastReportAction && !!lastOriginalReportAction) { | ||
| if (!lastMessageTextFromReport && !lastReportAction && !!lastOriginalReportAction && isWhisperAction(lastOriginalReportAction)) { | ||
| return lastVisibleMessage?.lastMessageText ?? ''; |
There was a problem hiding this comment.
Preserve hidden-action filtering for non-whispers
This new isWhisperAction guard means we only fall back to getLastVisibleMessage when the hidden last action is a whisper; for any other hidden action, we now fall through to report.lastMessageText. That can surface actions that shouldReportActionBeVisible intentionally hides for permission reasons (e.g., ACTIONABLE_CARD_FRAUD_ALERT is hidden when canUserPerformWriteAction is false in ReportActionsUtils.ts 1070–1077), so a user without write permission could see the alert text in LHN/search previews despite the visibility gate. The previous logic returned the last visible message (empty in this case), so this change effectively bypasses the visibility filter for non-whisper hidden actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@suneox @suhailpthaj What about this comment? What about other cases of hidden report actions that are not whisper - canUserPerformWriteAction is false
trjExpensify
left a comment
There was a problem hiding this comment.
I think this PR is making some unintended changes, and just having Expense as the subtext of a report has the potential to introduce confusion between expenses and reports - as it seemingly appears to label the report as an "Expense".
Looking at the OP of the parent issue, the bug was that the amount in the search router preview doesn't update reliably to incl. the value of the expense in the title after it completes receipt scanning. We should focus on that.
|
@trjExpensify This PR introduces return in But this PR loosely guarded the whisper filtering which leads to letting the non whisper code enter in the snippet and return the lastVisibleMessage. Before this PR, the application was running with We are just guarding it strongly or preventing the code going inside the added conditions (in the PR) which was made to whispers not expenses. The backend fix is already being handled by @getusha cc: @mountiny |
|
Yeah, I'm going to need Vit's input. I don't see why changing
|
@trjExpensify I’d like some clarification on the suggestion to show the message preview as Expense when there is no chat or activity on the expense. The goal is to keep the message preview consistent with the current behavior before opening the expense details. Message previewCleanShot.2026-01-14.at.00.07.08.mp4Please correct me if I'm wrong — should we instead keep the message preview consistent as |
|
Yeah, so maybe to recap for @mountiny where I'm at with the status quo for expense reports (and by extension IOUReports where applicable):
Just to restate the question to ensure I'm interpreting you correctly. The question is:
If that's the question, then historically it has been the standard "No activity yet". I'm unaware of where a conscious product change/decision was made to change that. Feel free to point me in that direction of somewhere, if so! I do see on staging this looks broken in a few ways:
2026-01-14_02-06-10.mp4A couple of other observations downstream..
2026-01-14_02-16-53.mp4 |
|
I agree that the preview of the last message should match between LHN and Search router. Regarding the:
I think what makes it hard to follow are the three levels we can have: the workspace chat > Expense report > Transaction thread. But I agree these 3 messages should be there as the preview of the last action on the EXPENSE REPORT, not on the transaction thread or workspace chat |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Yeah, I'm only talking about the report here, not the others. 👍 |
|
@trjExpensify What we are gonna do now with this PR? Should we move ahead with this PR and create an another follow up issue in which we will achieve the behaviour among LHN and search router which you have mentioned? One thing I wanna say, |
I think we should do it in this PR, because this PR is changing it to show |
|
So seems like for empty report, we want to make sure both LHN and search will show
Then:
In this case, the previous should not be Expense, but And then for:
The preview should show @suhailpthaj That is the expected behaviour, can you please update the test steps and update the code to match that? cc @suneox Thank you! |
|
On it. |
|
@mountiny Can you please have a look at backend PR created by @getusha It is required to achieve this expected behaviour.
@trjExpensify The following expected behaviour I covered:
Now it is matching between LHN and Search router for Expense reports instead of always showing I have couple of doubts which I'll ask later. I'll also give the reasoning of why these issues were occuring, reported by you. |
|
@getusha PR still needs some work https://github.com/Expensify/Auth/pull/19268 |
|
@trjExpensify @mountiny Backend PR has been merged. As per the expected behaviour mentioned by @trjExpensify This is how it will look: Scan-expense-issue-recording.mp4Let me know if that sound good to go, then I can push these commits and PR will move forward. Note: This video is recorded in development server as I have these local changes. cc: @suneox |
|
Looks ok to me, @trjExpensify can you also confirm? |
|
@suneox can you please review? |
Yes, I’ll take a look at this PR today |
|
@suhailpthaj After merging with the latest main, the changes in this PR still don’t match the expected behavior when submitting an expense directly without going through a report group. (at 1:20, 2:10) CleanShot.2026-02-05.at.23.44.28_.mp4 |
|
Thanks @suneox I'll take a look at this on tomorrow. |
|
@suneox Fixed the issue, you pointed out. Please have a look now. |
|
@suneox bump for a review |
Yes, I'll take a look at this one soon |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppCleanShot.2026-02-09.at.19.41.57_.mp4Android: mWeb ChromeCleanShot.2026-02-09.at.19.43.45_.mp4iOS: HybridAppCleanShot.2026-02-09.at.19.33.39_.mp4iOS: mWeb SafariCleanShot.2026-02-09.at.19.24.50_.mp4MacOS: Chrome / SafariCleanShot.2026-02-09.at.19.18.47_.mp4Fix CleanShot.2026-02-09.at.19.22.44_.mp4CleanShot.2026-02-09.at.19.48.13_.mp4 |
| // If the last action differs from last original action, it means there's a hidden action (like a whisper), then use getLastVisibleMessage to get the preview text | ||
| if (!lastMessageTextFromReport && !lastReportAction && !!lastOriginalReportAction) { | ||
| if (!lastMessageTextFromReport && !lastReportAction && !!lastOriginalReportAction && isWhisperAction(lastOriginalReportAction)) { | ||
| return lastVisibleMessage?.lastMessageText ?? ''; |
There was a problem hiding this comment.
@suneox @suhailpthaj What about this comment? What about other cases of hidden report actions that are not whisper - canUserPerformWriteAction is false
| if (reportID && !lastMessageTextFromReport && reportUtilsIsMoneyRequestReport(report)) { | ||
| const transactions = getReportTransactions(reportID); | ||
| const scanningTransactions = transactions.filter((transaction) => isScanning(transaction)); | ||
|
|
||
| if (scanningTransactions.length > 0) { | ||
| lastMessageTextFromReport = translate('iou.receiptScanning', {count: scanningTransactions.length}); | ||
| } else if (report?.transactionCount && report?.transactionCount > 0 && report?.currency) { | ||
| lastMessageTextFromReport = lastVisibleMessage?.lastMessageText; | ||
| } else if (report?.transactionCount === 0) { | ||
| lastMessageTextFromReport = translate('report.noActivityYet'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you make sure to cover all of this with unit tests? also you can add unit tests for the ACTIONABLE_CARD_FRAUD_ALERT that has canUserPerformWriteAction as false
|
@mountiny Added the unit tests, reverted the changes in whisper logic. Should we still add the test case "
|
|
Yeah please if that test does not exist please add it |
|
@mountiny Added the test case for ACTIONABLE_CARD_FRAUD_ALERT. |
|
🚧 @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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.18-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.18-8 🚀
|
| if (scanningTransactions.length > 0) { | ||
| lastMessageTextFromReport = translate('iou.receiptScanning', {count: scanningTransactions.length}); | ||
| } else if (report?.transactionCount && report?.transactionCount > 0 && report?.currency) { | ||
| lastMessageTextFromReport = lastVisibleMessage?.lastMessageText; |
There was a problem hiding this comment.
Directly using lastVisibleMessage?.lastMessageText as the LHN preview for expense reports here caused LHN - Expense amount is shown in bracket in LHN preview.
This text comes from the backend, which formats negative amounts with accounting-style parentheses (e.g., ($25.00)).
We customized this text in frontend from stable expense data like amount, currency, and comment in #86904 instead of relying on a raw message string that can be rehydrated differently.
Explanation of Change
Showing the same message preview in LHN and Search router, i.e:
Fixed Issues
$ #78217
PROPOSAL: #78217 (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-native.mp4
Android: mWeb Chrome
Android-mweb.mp4
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-mWeb-safari.mp4
MacOS: Chrome / Safari
Scan-expense-issue-recording.mp4