#1 - Remove Onyx.connect() for the key REPORT_ACTIONS in Report/index.ts#81778
Conversation
melvin-bot
bot
requested review from
JmillsExpensify and
eVoloshchak
and removed request for
a team and
eVoloshchak
|
@eVoloshchak 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] |
| import {isAnonymousUser} from '../Session'; | ||
|
|
||
| // We use connectWithoutView because markAllMessagesAsRead doesn't affect the UI rendering | ||
| // and this avoids unnecessary re-rendering in AuthScreen whenever any report or report action is updated |
There was a problem hiding this comment.
Let me know if this is fine and if I can move markCommentAsUnread here too.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const threeDotRef = useRef<View>(null); | ||
| const [betas] = useOnyx(ONYXKEYS.BETAS, {canBeMissing: true}); | ||
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`, { | ||
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, { |
There was a problem hiding this comment.
❌ PERF-11 (docs)
The component now subscribes to reportActions for the current reportID but only passes it to the payload without using it for rendering. This creates unnecessary re-renders whenever any report action changes.
Suggested fix: Use a selector to fetch only the specific report action needed, or move this subscription to the child component that actually needs the data:
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
canBeMissing: true,
canEvict: false,
selector: (allReportActions) => {
if (!allReportActions) return undefined;
// Only select the specific report actions needed for the payload
return allReportActions;
},
});Alternatively, consider whether this data should be fetched inside ContextMenuActions where it's actually used, following the principle that components should own their data needs.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
The component now subscribes to reportActions for the current reportID but only passes it to the payload without using it for rendering. This creates unnecessary re-renders whenever any report action changes.
Same concern I raised here.
| if (isAnonymousUser()) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-13 (docs)
The NetworkConnection.getDBTimeWithSkew() call on line 34 is independent of the loop that starts at line 42. This function is called once but its result (newLastReadTime) is used repeatedly in the loop. While this is already correctly implemented (the call is outside the loop), this is good practice.
However, there is a potential issue: the string template ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS appears to be constant but is being concatenated inside the loop at line 50. Consider whether this prefix could be extracted.
Current code (line 50):
const isArchivedReport = archivedReportsIdSet.has(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report.reportID}`);Note: This is actually fine since we're concatenating with report.reportID which is iterator-dependent. No action needed.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
unrelated, just moving out the function
![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: 90ae57f21b
ℹ️ 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".
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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-10.at.14.18.33.movAndroid: mWeb ChromeScreen.Recording.2026-02-10.at.14.19.10.moviOS: HybridAppScreen.Recording.2026-02-10.at.14.17.01.moviOS: mWeb SafariScreen.Recording.2026-02-10.at.14.15.16.movMacOS: Chrome / SafariScreen.Recording.2026-02-10.at.14.13.39.mov |
eVoloshchak
left a comment
eVoloshchak
left a comment
There was a problem hiding this comment.
Looking and working well otherwise!
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #73651 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@bernhardoj please take a look at the failed Prettier check and the conflict so we can move forward |
|
🚧 @MarioExpensify 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/MarioExpensify in version: 9.3.18-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.18-8 🚀
|
4 participants
Explanation of Change
Fixed Issues
#73651
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
NOTE: if we mark a report preview as unread, the report won't be bold. This happens on
maintoo.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
web.mp4