refactor shouldHideNewMarker to use isOffline from useOnyx#85732
Conversation
|
@DylanDylann 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
heyjennahay
and removed request for
a team
![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: 155eb5ca14
ℹ️ 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".
| unreadMarkerTime, | ||
| scrollingVerticalOffset: scrollOffsetRef.current, | ||
| prevUnreadMarkerReportActionID: prevUnreadMarkerReportActionID.current, | ||
| isOffline, |
There was a problem hiding this comment.
This memoized unread-marker scan now depends on isOffline (it is passed into shouldDisplayNewMarkerOnReportAction), but isOffline is missing from the useMemo dependency list. When connectivity changes and the other dependencies remain stable (for example earliestReceivedOfflineMessageIndex stays undefined), React will reuse a stale marker result, so the "New" marker can stay visible/hidden with the wrong online/offline state until another update forces recomputation.
Useful? React with 👍 / 👎.
| unreadMarkerTime, | ||
| scrollingVerticalOffset: scrollingVerticalBottomOffset.current, | ||
| prevUnreadMarkerReportActionID: prevUnreadMarkerReportActionID.current, | ||
| isOffline, |
There was a problem hiding this comment.
The unread-marker useMemo in this list also newly reads isOffline via shouldDisplayNewMarkerOnReportAction, but its dependency array was not updated. If the network flips between offline/online without changing the other memo deps, this hook can return stale output and keep the marker in the wrong place/state until unrelated state changes trigger a recompute.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Could you verify if this comment is valid? It looks like the new compiler handles useMemo and useCallback automatically, so they may no longer be needed.
There was a problem hiding this comment.
I think in cases where the React compiler can compile this file, then we need to remove the useMemo() calls. Either that, or I am pretty sure the dependency array needs updated. The compiled option without useMemo() should be preferred.
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.
|
|
The comment from Why the comment is technically right: Why it doesn't matter in practice: Bottom line: The |
|
@dukenv0307 Could you include videos on the PR description? |
|
@DylanDylann Done |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-03-20.at.10.53.31.mov |
|
cc @tgolen |
| unreadMarkerTime, | ||
| scrollingVerticalOffset: scrollingVerticalBottomOffset.current, | ||
| prevUnreadMarkerReportActionID: prevUnreadMarkerReportActionID.current, | ||
| isOffline, |
There was a problem hiding this comment.
I think in cases where the React compiler can compile this file, then we need to remove the useMemo() calls. Either that, or I am pretty sure the dependency array needs updated. The compiled option without useMemo() should be preferred.
|
@DylanDylann can you please review again? |
|
@dukenv0307 Please also address this issue in our previous PRs (missing update dependency array). For heavy computations, I suggest using useCallback and updating the dependency array only. Removing useCallback or useMemo could lead to performance issues. I just checked and It looks like we already have a refactor issue to safely remove them. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @tgolen 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! 🧪🧪
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.43-3 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #66407
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Screen.Recording.2026-03-19.at.22.20.42.mov