Fix LHN preview for transaction threads and deleted requests - #81153
Fix LHN preview for transaction threads and deleted requests#81153BartekObudzinski wants to merge 6 commits into
Conversation
…ort name display for empty reports
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.
|
Screen.Recording.2026-02-02.at.13.08.07.mov |
Screen.Recording.2026-02-02.at.13.08.07.mov |
| const iouReport = getReportOrDraftReport(getIOUReportIDFromReportActionPreview(lastReportAction)); | ||
| const iouReportID = iouReport?.reportID; | ||
| const reportCache = iouReportID ? visibleReportActionsDataParam?.[iouReportID] : undefined; | ||
| const visibleReportActionsForIOUReport = reportCache && Object.keys(reportCache).length > 0 ? visibleReportActionsDataParam : undefined; |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The function canUserPerformWriteAction(report, isReportArchived) is called inside the .find() callback but doesn't use the iterator variable reportAction. Since report and isReportArchived come from the outer scope and are constant across all iterations, this function call executes redundantly for each item in iouReportActions.
Hoist the function call outside the loop to avoid O(n) redundant computation:
const canWrite = canUserPerformWriteAction(report, isReportArchived);
const lastIOUMoneyReportAction =
iouReportID && iouReportActions
? iouReportActions.find(
(reportAction): reportAction is ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.IOU> =>
isReportActionVisible(reportAction, iouReportID, canWrite, visibleReportActionsForIOUReport) &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE &&
isMoneyRequestAction(reportAction),
)
: undefined;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
mkhutornyi
left a comment
There was a problem hiding this comment.
Are these changes safe enough to CP?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75baa5a92c
ℹ️ 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 lastMessageTextFromReport || (report?.lastMessageText ?? ''); | ||
| return lastMessageTextFromReport ?? report?.lastMessageText ?? ''; |
There was a problem hiding this comment.
When a report preview action is pending deletion, this function now sets lastMessageTextFromReport to an empty string earlier in the function. With the new nullish-coalescing return here, the empty string is treated as a final value, so the LHN preview becomes blank instead of falling back to the previous visible message (or report.lastMessageText). That means users can lose the last-message preview whenever a request is deleted, even if the report still has earlier visible chat actions. Consider falling back when lastMessageTextFromReport is empty so prior messages continue to show.
Useful? React with 👍 / 👎.
The function `canUserPerformWriteAction` was being called inside a `.find()` loop in `getLastMessageTextForReport`. Since the arguments `report` and `isReportArchived` are constant for the loop, this calculation was redundant. This change reuses the `canUserPerformWrite` variable which is already calculated at the top of the function, avoiding O(n) redundant computations.
…ng delete When a report preview action is pending deletion, `lastMessageTextFromReport` is set to an empty string. Previously, the code used `??` which treated the empty string as a final value, resulting in a blank preview. This change switches to `||` so that an empty string falls back to `report.lastMessageText` (or the previous visible message), ensuring users don't lose context when a request is deleted.
|
@mkhutornyi Yes, looks safe. We're only adjusting how the last message is calculated for display and hoisting a constant out of a loop, without modifying any underlying data persistence |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
mkhutornyi
left a comment
There was a problem hiding this comment.
Can you please share brief summary of root cause and solution of 2 linked issues?
| } | ||
|
|
||
| return lastMessageTextFromReport || (report?.lastMessageText ?? ''); | ||
| return lastMessageTextFromReport || report?.lastMessageText || ''; |
There was a problem hiding this comment.
What does this change fix?
There was a problem hiding this comment.
This ensures we fall back to the previous visible message instead of showing a blank preview when lastMessageTextFromReport is empty
There was a problem hiding this comment.
my bad we have eslint rule to use nullish opeartor reverting it
|
For the deleted report preview issue (#81102 ), the root cause was the VISIBLE_REPORT_ACTIONS logic incorrectly caching deleted actions, which we solved by updating getLastMessageTextForReport to strictly check deletion status and using || to reliably fall back to the previous message. For the approved report preview issue (#81106 ), the logic failed to check child transaction threads for newer comments, which we fixed by introducing getLastVisibleActionIncludingTransactionThread to prioritize the most recent action across both parent and child reports. More details are in particular ticket. |
…is pending delete" This reverts commit fc79099.
|
Looks good. Doing some more regression test |
| (childLastAction.created && parentLastAction.created && childLastAction.created > parentLastAction.created) || | ||
| (childLastAction.created && !parentLastAction.created)) |
There was a problem hiding this comment.
Please add comment about this change
| } | ||
|
|
||
| return lastMessageTextFromReport || (report?.lastMessageText ?? ''); | ||
| return lastMessageTextFromReport ?? report?.lastMessageText ?? ''; |
There was a problem hiding this comment.
lastMessageTextFromReport can be empty string. Please revert this change.
| // If the report preview action is being deleted, don't show the stale message | ||
| // Instead, show an empty string which will trigger proper recalculation or show default message | ||
| if (lastReportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { | ||
| lastMessageTextFromReport = ''; |
There was a problem hiding this comment.
This is not true. We might have existing messages before report preview.
#81102 is still reproducible. Please check this video:
bug.mov
|
Since original PR was reverted I am going to create official version combined with this changes |
Explanation of Change
Fix LHN preview for transaction threads and deleted requests
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
cases are described in descriptions:
#81102
#81106
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari