[PR 2 of 13] Remove global Onyx reference from getOriginalReportID step 1#81707
[PR 2 of 13] Remove global Onyx reference from getOriginalReportID step 1#81707
Conversation
tgolen
changed the title
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
|
@situchan do you want to review this one as well? |
melvin-bot
bot
requested review from
carlosmiceli
and removed request for
a team
|
@carlosmiceli 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] |
yes thanks |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| function clearReportActionErrors(reportID: string, reportAction: ReportAction, keys?: string[]) { | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); |
There was a problem hiding this comment.
❌ [PERF-2] Return early before expensive work (docs)
The function calls getOriginalReportID() before checking if reportAction?.reportActionID exists. getOriginalReportID() performs expensive operations (Onyx reads, function calls) that will be wasted if reportActionID is missing.
Fix: Move the simple validation check before the expensive work:
function clearReportActionErrors(reportID: string, reportAction: ReportAction, keys?: string[]) {
if (!reportAction?.reportActionID) {
return;
}
const originalReportID = getOriginalReportID(reportID, reportAction, undefined);
// ... rest of function
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
There is almost never a case where reportAction.reportActionID wouldn't exist, so this is not a valuable optimization.
| ) { | ||
| const reportID = report?.reportID; | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); |
There was a problem hiding this comment.
❌ [PERF-2] Return early before expensive work (docs)
The function calls getOriginalReportID() (line 2095) before checking if reportAction.reportActionID exists (line 2098). getOriginalReportID() performs expensive operations (Onyx reads, function calls) that will be wasted if the validation fails.
Fix: Extract simple checks first, then perform expensive work:
const reportID = report?.reportID;
const reportActionID = reportAction.reportActionID;
if (\!reportActionID || \!reportID) {
return;
}
const originalReportID = getOriginalReportID(reportID, reportAction, undefined);
if (\!originalReportID) {
return;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
There is almost never a case where reportAction.reportActionID wouldn't exist, so this is not a valuable optimization.
![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: 17ba793732
ℹ️ 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".
| const renderItem = useCallback( | ||
| ({item: reportAction, index}: ListRenderItemInfo<OnyxTypes.ReportAction>) => { | ||
| const originalReportID = getOriginalReportID(report.reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(report.reportID, reportAction, reportActionsFromOnyx); | ||
| const reportDraftMessages = draftMessage?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`]; |
There was a problem hiding this comment.
The renderItem callback now depends on reportActionsFromOnyx to compute originalReportID, but it is not listed in the useCallback dependency array. When the report actions collection updates (e.g., Onyx loads/evicts actions after initial render), the callback will keep using a stale collection and can compute the wrong originalReportID, which then indexes drafts/emoji reactions for the wrong report. Add reportActionsFromOnyx to the dependency list (or derive originalReportID outside the callback) so it stays in sync with Onyx updates.
Useful? React with 👍 / 👎.
| if (isTrackExpenseReport && !isDeletedParentAction) { | ||
| const actionReportID = getOriginalReportID(report.reportID, parentReportAction); | ||
| const actionReportID = getOriginalReportID(report.reportID, parentReportAction, reportActionsForOriginalReportID); | ||
| const whisperAction = getTrackExpenseActionableWhisper(iouTransactionID, moneyRequestReport?.reportID); |
There was a problem hiding this comment.
The menuItems useMemo now uses reportActionsForOriginalReportID when calling getOriginalReportID, but the dependency list omits reportActionsForOriginalReportID. If the report actions collection changes after initial render, the memoized actionReportID used by the Track Expense actions can become stale, leading to drafts/navigations targeting the wrong report. Include reportActionsForOriginalReportID in the dependency array so the menu recomputes when actions load or change.
Useful? React with 👍 / 👎.
|
Affected files to test
(Passing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a351f64b3f
ℹ️ 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".
| onPress: (closePopover, {reportAction, reportActions, reportID, translate, currentUserPersonalDetails}) => { | ||
| if (!reportID) { | ||
| return; | ||
| } | ||
|
|
||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, reportActions); | ||
| if (closePopover) { |
There was a problem hiding this comment.
Here getOriginalReportID is now fed reportActions from the context menu payload, but that payload is sourced from useOnyx keyed by originalReportID (see BaseReportActionContextMenu where it loads reportActions with originalReportID). When originalReportID !== reportID (e.g., actions displayed in a different report than where they were created), the passed reportActions contains the action so getOriginalReportID returns reportID unconditionally. This causes actions like “Explain” (and also “Copy link” below) to point to the wrong report. The fix is to pass report actions for the reportID being evaluated or avoid overriding the global lookup when the payload’s actions don’t match reportID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes, that's a good call. I'll update it to get the report actions from reportID
There was a problem hiding this comment.
I actually updated this to remove getOriginalReportID() completely when I saw that originalReportID already existed in the base context.
| return; | ||
| } | ||
|
|
||
| const originalReportID = getOriginalReportID(reportID, reportAction); |
There was a problem hiding this comment.
In both of these cases here, I was able to remove the call to getOriginalReportID() because the originalReportID already existed in the base context, it could be passed down directly.
| return !hasCreditBankAccount(bankAccountList) ? 'bankAccount' : undefined; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This was 100% dead code, so easy to remove.
|
Here's the case where
|
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb1.movweb2.mov |
Nice, looks like that's fixed in my recent changes. |
2 participants
Explanation of Change
This is the first step of several to refactor
getOriginalReportID()(18 references). Here is the full refactoring plan:reportActionsParamparameter and fallback to the global Onyx value forallReportActionsundefinedfor the parameterreportActionsavailable to pass that as the parameterreportActionsas the parameterreportActionsParamtoreportActionsallReportActionsand the Onyx connectionFixed Issues
Part of #66419
Tests
Offline tests
Same as tests
QA Steps
Same as tests
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