Improve ShouldReportBeInOptionList function#81551
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| if (report?.reportID === currentReportId) { | ||
| return CONST.REPORT_IN_LHN_REASONS.IS_FOCUSED; | ||
| } |
There was a problem hiding this comment.
| if (report?.reportID === currentReportId) { | |
| return CONST.REPORT_IN_LHN_REASONS.IS_FOCUSED; | |
| } | |
| // Include the currently viewed report. If we excluded the currently viewed report, then there | |
| // would be no way to highlight it in the options list and it would be confusing to users because they lose | |
| // a sense of context. | |
| if (report.reportID === currentReportId) { | |
| return CONST.REPORT_IN_LHN_REASONS.IS_FOCUSED; | |
| } |
can we restore the comment you moved?
src/libs/ReportUtils.ts
Outdated
| let parentReportActionComputed = false; | ||
| let parentReportActionCache: OnyxEntry<ReportAction>; | ||
| const getParentReportAction = (): OnyxEntry<ReportAction> => { | ||
| if (!parentReportActionComputed) { | ||
| parentReportActionComputed = true; | ||
| parentReportActionCache = isThreadReport ? allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID] : undefined; | ||
| } | ||
| return parentReportActionCache; | ||
| }; |
There was a problem hiding this comment.
I see you are doing some kind of caching here, but I don't think how this would make significant difference as we are basically just performing a lookup in allReportActions, right?
There was a problem hiding this comment.
yes you right so I reverted this one
src/libs/ReportUtils.ts
Outdated
| if (!report.lastReadTime) { | ||
| return !isEmptyReport(report, isReportArchived); | ||
| } | ||
|
|
||
| if (isEmptyReport(report, isReportArchived)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
In both ifs we check for isEmptyReport(report, isReportArchived), it looks like it can be even more simplified
| if (!report.lastReadTime) { | |
| return !isEmptyReport(report, isReportArchived); | |
| } | |
| if (isEmptyReport(report, isReportArchived)) { | |
| return false; | |
| } | |
| if (isEmptyReport(report, isReportArchived)) { | |
| return false; | |
| } | |
| if (!report.lastReadTime) { | |
| return true; | |
| } |
src/libs/ReportUtils.ts
Outdated
| const isThreadReport = isThread(report); | ||
| let parentReportActionComputed = false; | ||
| let parentReportActionCache: OnyxEntry<ReportAction>; | ||
| const getParentReportAction = (): OnyxEntry<ReportAction> => { |
There was a problem hiding this comment.
i do not see that we use such kind on patter define function inside function.
I think it makes sense to move it out from here, pass need params and just use
const parentReportAction = getParentReportAction(parentReportActionComputed, isThreadReport, allReportActions)
There was a problem hiding this comment.
I revert this function in my commit
src/libs/ReportUtils.ts
Outdated
| let parentReportActionCache: OnyxEntry<ReportAction>; | ||
| const getParentReportAction = (): OnyxEntry<ReportAction> => { | ||
| if (!parentReportActionComputed) { | ||
| parentReportActionComputed = true; |
There was a problem hiding this comment.
I think it is better to change it to true after the actual operation
At this case if smth will go wrong - we do not update the flag
There was a problem hiding this comment.
Im not using it in my new commit
JKobrynski
left a comment
JKobrynski
left a comment
There was a problem hiding this comment.
Left one comment, other than that LGTM
src/libs/ReportUtils.ts
Outdated
| const reportActionKeys = Object.keys(currentReportActions); | ||
|
|
||
| const isThreadReport = isThread(report); | ||
| const parentReportAction = isThreadReport ? allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID] : undefined; |
There was a problem hiding this comment.
| const parentReportAction = isThreadReport ? allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID] : undefined; | |
| const parentReportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]; | |
| const parentReportAction = isThreadReport | |
| ? parentReportActions?.[report.parentReportActionID] | |
| : undefined; |
I would consider something like this, I think it makes it more readable
| const triggerUnreadUpdate = debounce(() => { | ||
| const currentReportID = navigationRef?.isReady?.() ? Navigation.getTopmostReportId() : undefined; | ||
| const draftComment = allDraftComments?.[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${currentReportID}`]; | ||
| Timing.start(CONST.TIMING.GET_UNREAD_REPORTS_FOR_UNREAD_INDICATOR); |
There was a problem hiding this comment.
FYI the Timing module is getting deprecated here #81691
There was a problem hiding this comment.
let's either not track this at all, or record regular Sentry spans
There was a problem hiding this comment.
Ill modify my code. . btw there will be other local alternative or mostly with sentry ?
| } | ||
|
|
||
| if (isSelfDM(report)) { | ||
| if (isSelfDMReport) { |
There was a problem hiding this comment.
what does Opus say about these changes, is there still some room for more improvement from a static analysis of hot paths?
There was a problem hiding this comment.
Opus you mean the ai model right ? I used it to get the initial recommendations and while there was also more options It didn't make difference in performance and was more complex and risky than this minor changes.
melvin-bot
bot
requested review from
thesahindia
and removed request for
a team
|
@thesahindia 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] |
melvin-bot
bot
requested review from
a team,
abdulrahuman5196,
joekaufmanexpensify and
mountiny
and removed request for
a team
|
@abdulrahuman5196 @mountiny One of you needs to 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: 8805b63ab1
ℹ️ 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".
abdulrahuman5196
left a comment
abdulrahuman5196
left a comment
There was a problem hiding this comment.
@elirangoshen
The code looks good, I have posted a question, kindly have a look into it.
We have merge conflicts in the PR.
In the Author Checklist, we don't have the required Screenshots/Videos for the platforms, could you check on these and add.
| // Include the currently viewed report. If we excluded the currently viewed report, then there | ||
| // would be no way to highlight it in the options list and it would be confusing to users because they lose | ||
| // a sense of context. | ||
| if (report?.reportID === currentReportId) { |
There was a problem hiding this comment.
I see we moved this from the below to the top of functions. Could you kindly provide information of why we are doing this? And how would this improve the performance.
There was a problem hiding this comment.
I tried to move it here to make early exit without executing more expensive operations after
I added to android native do you want me to add to other platforms as well ? |
|
@elirangoshen the code looks good.
@elirangoshen Ideally we should first resolve the conflicts and then take the required screenshots/videos for all the platforms in author's checklist |
|
Sure I will look into it asap |
Done, its ready now |
|
@abdulrahuman5196 can you retest please |
|
checking now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-14.at.12.23.29.AM.movAndroid: mWeb ChromeScreen.Recording.2026-02-14.at.12.28.23.AM.moviOS: HybridAppScreen.Recording.2026-02-14.at.12.14.49.AM.moviOS: mWeb SafariScreen.Recording.2026-02-14.at.12.18.40.AM.movMacOS: Chrome / SafariScreen.Recording.2026-02-13.at.11.49.46.PM.mov |
abdulrahuman5196
left a comment
abdulrahuman5196
left a comment
There was a problem hiding this comment.
Changes looks good and works well. Reviewers checklist is also complete.
All yours. @mountiny
🎀 👀 🎀
C+ Reviewed
|
🚧 @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.20-0 🚀
|
|
This PR failing because of a regression issue 1.mp4 |
|
Hey @elirangoshen, quick question, is it expected for archived workspaces not to appear on LHN? Step 9 seems a bit unclear. Thanks |
I will double check it asap |
whats the expected behaviour for the achieved workspaces, should it appear in LHN? |
|
Archived workspace chats should appear in the LHN in the Most recent mode |
|
Maybe it could be marked as hidden, but you can still search for it |
|
about regression I will deliver a fix asap , about the achieved chat, Im not aware of an issue related that but if you will find anything Id check it out |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.20-6 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.20-6 🚀
|
13 participants
Explanation of Change
Fixed Issues
$#81900
PROPOSAL: ShouldReportBeInOptionList improve
This pr improve
ShouldReportBeInOptionListperformance by reordering the actions to create more cheap early exists and use caching for repeatable expensive operationsTests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Open the app in Default mode (not Focus/GSD mode).
Verify that all expected chats appear: DMs, group chats, workspace rooms, policy expense chats, task reports, expense requests.
Verify the currently viewed report is always highlighted/visible in the LHN.
Navigate between different reports and confirm the LHN selection follows correctly.
Focus Mode
Switch to Focus mode
Verify that only unread chats appear (and muted chats do NOT).
Send a message in a chat from another account, then check that it appears as unread in Focus mode.
Read the chat, and confirm it disappears from Focus mode.
Verify that a brand new report that has never been read (no lastReadTime) appears as unread in Focus mode. (This is the isUnread change.)
Currently Focused Report
In Focus mode, navigate to a report that is read (not unread).
Confirm it still appears in the LHN because it's the currently focused report.
Navigate away from it, and confirm it disappears from the LHN (since it's read and no longer focused).
Chat Threads
Open a chat thread that has replies — it should appear in the LHN.
Find an empty chat thread (no replies, just the parent message) — it should not appear in the LHN.
Delete the parent message of a thread with no replies — the thread should not appear.
Open a thread where the parent message is pending deletion — it should not appear.
Pinned Reports
Pin a report — confirm it appears in LHN in both Default and Focus modes.
Unpin it — confirm normal visibility rules apply again.
Reports with Drafts
Start typing a message in a chat (create a draft) but don't send it.
Confirm the report appears in the LHN (both modes) with the draft indicator.
Self DM
Verify the Self DM (chat with yourself) appears correctly in the LHN.
System Chat
If you have a system chat (notifications account), verify it appears correctly.
Archived Reports
Archive a report.
In Default mode, confirm it still appears in the LHN.
In Focus mode, confirm it only appears if unread.
Empty Chats
Create a new DM with someone but don't send any message.
In Default mode with excludeEmptyChats, confirm the empty chat is hidden.
Send a message — confirm it now appears.
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