Remove Onyx.connect() key ONYXKEYS.COLLECTION.REPORT_ACTIONS in src/libs/OptionsListUtils.ts - part 5#92330
Remove Onyx.connect() key ONYXKEYS.COLLECTION.REPORT_ACTIONS in src/libs/OptionsListUtils.ts - part 5#92330truph01 wants to merge 23 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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] |
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
no code blockers, building test
|
🚧 @grgia 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! 🧪🧪
|
|
Let's wait until #92308 is merged |
|
@codex review |
![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: 4b618c6785
ℹ️ 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".
| participantReportDraft, | ||
| undefined, | ||
| undefined, | ||
| sortedActions, |
There was a problem hiding this comment.
When the confirmation screen is already mounted and useSortedActions() later publishes updated report actions (for example after a report preview/expense action changes while the user is on this step), this memoized participant list keeps using the old sortedActions value because it is not in the dependency array below. That leaves the participant option/preview text stale until some unrelated dependency changes or the screen remounts, which defeats the new data path being passed into getReportOption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Add sortedActions to deps
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2f965fa00
ℹ️ 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".
| useShareFileSizeValidation(currentReceiptSource, setErrorTitle, setErrorMessage, !errorTitle); | ||
|
|
||
| const selectedParticipants = unknownUserDetails ? [unknownUserDetails] : getMoneyRequestParticipantsFromReport(report, currentUserPersonalDetails.accountID); | ||
| const participantReportIDs = selectedParticipants.map((participant) => participant.reportID).filter(Boolean); |
There was a problem hiding this comment.
In the share-submit flow this array is rebuilt on every render, but useRelevantSortedActions memoizes its Onyx selector on the array identity and explicitly requires callers to pass a stable reference. When this page re-renders while editing the receipt/comment or receiving Onyx updates on a high-traffic account, it recreates the selector/subscription to RAM_ONLY_SORTED_REPORT_ACTIONS, so this path still pays the expensive derived-action selection work the migration is trying to avoid. Please wrap the selected participants and/or mapped IDs in useMemo, as the IOU confirmation path does.
Useful? React with 👍 / 👎.
|
@DylanDylann All good now! |
|
@truph01 Could you verify again whether we actually need to pass sortedActions in these places? |
|
@MelvinBot Please review this PR and let me know if we need to pass sortActions in these places and please explain why do we need to pass them or not |
@DylanDylann We still need to pass |
|
Ex: At SubmitDetailsPage, passing sortedActions into getReportOption has no observable effect:
So passing sortedActions vs undefined yields an identical UI |
|
@MelvinBot What do you think about #92330 (comment)? |
@DylanDylann I agree that we can stop passing
But if |
|
We can update the type to |
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
@truph01 do we need sortedActions at the two getReportOption sites? Could we use the useRelevantSortedActions?
Seems SubmitDetailsPage and IOURequestStepConfirmation gain a useRelevantSortedActions subscription whose result is discarded, adding re-renders for nothing
|
@truph01 Let's hold off on this PR for a bit. I want to double check everything to make sure our approach is the best option and doesn't cause any performance regressions. |
|
@truph01 Using selector looks ok, please continue updating this comment? #92330 (comment) |
| for (const action of reportActions) { | ||
| if (!isReportPreviewAction(action)) { | ||
| continue; | ||
| } | ||
| const iouReportID = getIOUReportIDFromReportActionPreview(action); |
There was a problem hiding this comment.
but this scan includes every REPORT_PREVIEW in the chat's history, so on a workspace chat the confirmation page subscribes to every linked expense report and re-renders whenever any of them changes.
Since sortedActions is newest-first, can we break after the first preview with pendingAction !== DELETE?
There was a problem hiding this comment.
@grgia Just checking pendingAction !== DELETE wouldn't be enough here — getLatestVisibleMoneyRequestAction (used downstream in getReportOption) also filters on isMoneyRequestAction and isReportActionVisible
|
@truph01 would you please add WIP to the title while you update |
grgia
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7c0055623
ℹ️ 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".
| useShareFileSizeValidation(currentReceiptSource, setErrorTitle, setErrorMessage, !errorTitle); | ||
|
|
||
| const selectedParticipants = unknownUserDetails ? [unknownUserDetails] : getMoneyRequestParticipantsFromReport(report, currentUserPersonalDetails.accountID); | ||
| const participantReportIDs = selectedParticipants.map((participant) => participant.reportID).filter(Boolean); |
There was a problem hiding this comment.
participantReportIDs is rebuilt on every render, despite useRelevantSortedActions requiring a referentially stable array. Consequently, any unrelated state or Onyx update in this receipt-submission screen recreates the selector passed to useOnyx and renews its subscription to the global sorted-actions derived value, defeating the targeted-subscription optimization and causing avoidable work while users edit the submission.
Useful? React with 👍 / 👎.
@DylanDylann @grgia I don't think this is a good pattern — it couples callers to the function's internal implementation. For example: function calculate(a: number, b: number) {
if (a === 1) return a;
if (b === 2) return b;
// ... 10 more branches using b
if (b === 99) return b;
}Knowing that Anyway, we already reduced the size of the |
truph01
changed the title
5 participants
Explanation of Change
This is the 5th PR in the migration of
Onyx.connect()forONYXKEYS.COLLECTION.REPORT_ACTIONSinOptionsListUtils. It refactors:Fixed Issues
$ #66381
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include ""[No QA].""
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