Remove Onyx.connect() usage for ONYXKEYS.COLLECTION.POLICY_TAGS in createDistanceRequest function from src/libs/actions/IOU/Split.ts#90353
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@parasharrajat 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] |
| @@ -176,6 +183,7 @@ function useExpenseSubmission(params: UseExpenseSubmissionParams) { | |||
| // Policy-scoped Onyx data | |||
| const policyID = policy?.id; | |||
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.COLLECTION.POLICY_TAGS) subscribes to the entire policy tags collection, causing re-renders whenever any policy's tags change. However, only a single entry is accessed (allPolicyTags?.[key]). Use a targeted useOnyx call with the specific policy key instead, e.g., useOnyx(\${ONYXKEYS.COLLECTION.POLICY_TAGS}${iouReportPolicyID}`). If iouReportPolicyID` is not yet known at hook call time, consider restructuring so the lookup can use a specific key.
Reviewed at: c040501 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const reportTransactions = useReportTransactions(report?.reportID); | ||
| const isMoneyRequestReport = isMoneyRequestReportReportUtils(report); | ||
| const currentChatReport = isMoneyRequestReport ? getReportOrDraftReport(report?.chatReportID) : report; | ||
| const moneyRequestReportID = isMoneyRequestReport ? report?.reportID : ''; |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.COLLECTION.REPORT) subscribes to the entire reports collection without a selector. The reports collection can be very large, and this will re-render the component on every report change. The only usage is allReports?.[key]?.policyID to get a single report's policyID. Use a targeted useOnyx with the specific report key, or if the key depends on runtime values, use a selector that extracts only the needed policyID.
Reviewed at: c040501 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const isMoneyRequestReport = isMoneyRequestReportReportUtils(params.report); | ||
| const currentChatReport = isMoneyRequestReport ? getReportOrDraftReport(params.report?.chatReportID) : params.report; | ||
| const moneyRequestReportID = isMoneyRequestReport ? params.report?.reportID : ''; | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
// eslint-disable-next-line @typescript-eslint/no-deprecated lacks a justification comment explaining why the deprecated API is being used. Add a comment explaining why getMoneyRequestPolicyTags is used despite being deprecated (e.g., // Using deprecated API because no replacement is available yet for this use case).
Reviewed at: c040501 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const isMoneyRequestReport = isMoneyRequestReportReportUtils(report); | ||
| const currentChatReport = isMoneyRequestReport ? getReportOrDraftReport(report?.chatReportID) : report; | ||
| const moneyRequestReportID = isMoneyRequestReport ? report?.reportID : ''; | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
// eslint-disable-next-line @typescript-eslint/no-deprecated lacks a justification comment explaining why the deprecated API is being used. Add a comment explaining why getMoneyRequestPolicyTags is used despite being deprecated.
Reviewed at: c040501 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
No product review needed |
Explanation of Change
This PR is part of a refactor to remove Onyx.connect for the keys: ONYXKEYS.COLLECTION.POLICY_TAGS from the src/libs/actions/IOU/Split.ts file and replace it with useOnyx.
It isolates the
createDistanceRequestfunction from the ONYXKEYS.COLLECTION.POLICY_TAGS Onyx.connect key .Fixed Issues
$ #72721
PROPOSAL:
Tests
Preconditions (all scenarios)
Tags) with valuesTag 1,Tag 2,Tag 3.Tag 2.1,Tag 2.2,Tag 2.3.Scenario 1 — Create distance expense in a Workspace A expense chat
Expected result:
Tag 1,Tag 2,Tag 3).Tag 2.1,Tag 2.2,Tag 2.3) are not shown.Tag 1→ tap Save → back on confirmation tap Submit.Expected result:
Tag 1applied.Scenario 2 — Add a distance expense to an existing expense report (different chat)
open expense report exists with Workspace A policy. Note the
report.
report header → Add expense → Distance.
Expected result:
Tag 1,Tag 2,Tag 3),matching the existing expense report's policy.
Tag 2→ Save → Submit.Expected result:
Tag 2applied.Scenario 3 — Duplicate a distance expense (Duplicate.ts path)
Setup: In Workspace A, create two distance expenses with the same
amount and date so the app treats them as duplicates. Both expenses
must have a tag from Workspace A applied (e.g.,
Tag 1) — this isrequired because
Duplicate.tspasses the resolvedpolicyTagList(the policy's available tag set) to the new distance request, so
without tags on the originals there is nothing observable to verify.
Open the report preview that shows the Duplicates detected banner.
expense.
detail).
the duplicate decision (this triggers
Duplicate.ts→createExpenseByTypewithtransactionType: DISTANCE).Expected result:
Tag 1from theoriginal expense (i.e., the policy tag list resolves to Workspace
A — same as the existing IOU report — so the applied tag survives
validation).
list (
Tag 1,Tag 2,Tag 3).Offline tests
QA Steps
Same as tests
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
Scenario 1
https://github.com/user-attachments/assets/ea3472ab-4bf4-421a-b0c9-ea3e3efd801f
Scenario 2
https://github.com/user-attachments/assets/463f3fd5-a0ad-4230-ad3b-e93b285a7b33
Scenario 3
https://github.com/user-attachments/assets/a53cc1a7-4c15-4f79-9e36-5b4f38d7b68d