Deprecate SearchTransaction > moneyRequestReportActionID - #75648
Conversation
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const parentReportActionSelector = useCallback( | ||
| (reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${oneTransactionItem?.moneyRequestReportActionID}`], | ||
| [oneTransactionItem?.moneyRequestReportActionID], | ||
| (reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${oneTransactionItem?.reportAction?.reportActionID}`], |
There was a problem hiding this comment.
Potential runtime error: When oneTransactionItem?.reportAction is undefined, the expression oneTransactionItem?.reportAction?.reportActionID evaluates to undefined. Inside the template literal \${undefined}`, this becomes the **string** "undefined", causing the selector to look up reportActions["undefined"]instead of returningundefined`.
Suggested fix: Add a null check before the template literal:
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => {
const reportActionID = oneTransactionItem?.reportAction?.reportActionID;
return reportActionID ? reportActions?.[reportActionID] : undefined;
},Or use a more concise approach:
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> =>
oneTransactionItem?.reportAction?.reportActionID
? reportActions?.[oneTransactionItem.reportAction.reportActionID]
: undefined,
There was a problem hiding this comment.
This is similar to what we currently have. That being said, I like the suggestion because it's more explicit. @DylanDylann let's implement it this way
There was a problem hiding this comment.
@DylanDylann we can use the suggested approach to avoid reportActions["undefined"]. Also @luacmartins agrees to it.
| const parentReportActionSelector = useCallback( | ||
| (reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${transactionItem?.moneyRequestReportActionID}`], | ||
| [transactionItem?.moneyRequestReportActionID], | ||
| (reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${transactionItem?.reportAction?.reportActionID}`], |
There was a problem hiding this comment.
Potential runtime error: When transactionItem?.reportAction is undefined, the expression transactionItem?.reportAction?.reportActionID evaluates to undefined. Inside the template literal \${undefined}`, this becomes the **string** "undefined", causing the selector to look up reportActions["undefined"]instead of returningundefined`.
Suggested fix: Add a null check before the template literal:
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => {
const reportActionID = transactionItem?.reportAction?.reportActionID;
return reportActionID ? reportActions?.[reportActionID] : undefined;
},Or use a more concise approach:
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> =>
transactionItem?.reportAction?.reportActionID
? reportActions?.[transactionItem.reportAction.reportActionID]
: undefined,
| setOptimisticDataForTransactionThreadPreview(item, previewData, IOUTransactionID); | ||
|
|
||
| const transactionThreadReport = createTransactionThreadReport(item.report, {reportActionID: item.moneyRequestReportActionID} as OnyxTypes.ReportAction); | ||
| const transactionThreadReport = createTransactionThreadReport(item.report, {reportActionID: item.reportAction?.reportActionID} as OnyxTypes.ReportAction); |
There was a problem hiding this comment.
Potential runtime error: When item.reportAction is undefined, the expression item.reportAction?.reportActionID evaluates to undefined. This will be passed to createTransactionThreadReport as {reportActionID: undefined}, which may cause unexpected behavior.
Suggested fix: Add a null check or default value:
const transactionThreadReport = createTransactionThreadReport(
item.report,
{reportActionID: item.reportAction?.reportActionID ?? ''} as OnyxTypes.ReportAction
);Or validate that reportAction exists before calling this function, depending on the expected behavior of createTransactionThreadReport.
| */ | ||
| function setOptimisticDataForTransactionThreadPreview(item: TransactionListItemType, transactionPreviewData: TransactionPreviewData, IOUTransactionID?: string) { | ||
| const {moneyRequestReportActionID, reportID, report, amount, currency, transactionID, created, policyID} = item; | ||
| const {reportID, report, amount, currency, transactionID, created, policyID} = item; |
There was a problem hiding this comment.
Type safety issue: The destructuring attempts to extract moneyRequestReportActionID from item, but this field has been removed from the type definition. This line will always result in moneyRequestReportActionID being undefined.
Suggested fix: Remove the destructuring of moneyRequestReportActionID since it's now accessed via item.reportAction?.reportActionID on line 1109:
const {reportID, report, amount, currency, transactionID, created, policyID} = item;
const moneyRequestReportActionID = item.reportAction?.reportActionID;This makes the code clearer and ensures the correct value is used.
|
@Pujan92 Could you review this PR? |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Yes, I will do it tomorrow. |
|
@DylanDylann conflicts |
|
Resolved conflict. @Pujan92 Could you take a look? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@Pujan92 Any other feedback? |
|
Bump @Pujan92 |
Pujan92
left a comment
Pujan92
left a comment
There was a problem hiding this comment.
LGTM!
Screen.Recording.2025-12-07.at.17.06.00.mov
|
✋ 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/luacmartins in version: 9.2.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #73965
PROPOSAL:
Tests
Offline tests
QA Steps
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
Screen.Recording.2025-12-03.at.16.42.13.mov
Android: mWeb Chrome
Screen.Recording.2025-12-03.at.16.40.03.mov
iOS: Native
Screen.Recording.2025-12-03.at.16.40.55.mov
iOS: mWeb Safari
Screen.Recording.2025-12-03.at.16.39.03.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-03.at.16.37.37.mov