Fix report lifecycle when moving expense to new report #90712
Fix report lifecycle when moving expense to new report #90712neerajbachani wants to merge 3 commits into
Conversation
melvin-bot
Bot
requested review from
daledah and
trjExpensify
and removed request for
a team and
trjExpensify
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {translate, localeCompare} = useLocalize(); | ||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const [allReportNextSteps] = useOnyx(ONYXKEYS.COLLECTION.NEXT_STEP); | ||
| const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.COLLECTION.REPORT) subscribes to the entire REPORT collection without a selector, causing this component to re-render whenever any report changes. The allReports value is only used inside the createReport callback to look up a single report by ID (allReports?.[\${ONYXKEYS.COLLECTION.REPORT}${originalReportID}`]`). This component already has many Onyx subscriptions; adding an unselected collection subscription amplifies unnecessary re-renders.
Consider using Onyx.getValue or OnyxUtils.getCachedValue inside the callback to read the report on demand rather than subscribing at the component level, or use a selector that narrows to only the needed report if the ID can be determined at the hook level.
Reviewed at: e24e3ab | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I verified the bot concern is valid: NewReportWorkspaceSelectionPage subscribes to full REPORT collection only to fetch one report in createReport. I’ll remove that subscription and switch to an on demand cached lookup via existing getReportOrDraftReport.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@neerajbachani please fix the failing checks |
…nsify#86637) Pass originalReport into changeTransactionsReport for optimistic new reports. Add tests and avoid full REPORT collection subscription in workspace selection.
neerajbachani
force-pushed
the
test/86637-offline-optimistic-proof
branch
from
e24e3ab to
98b04fa
Compare
|
@daledah All CI checks are passing now, and I’ve added the video demo in the PR media as well. Core fix (actual product change) - lifecycle inheritance when moving an expense to a new report:
Tests only (Codecov / review feedback) - no additional product behavior:
Happy to adjust anything if you’d like a different test split or more coverage in a specific path. |
|
@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: 477377eebf
ℹ️ 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 shouldInheritLifecycleStateFromSource = | ||
| destinationReportID === newReport?.reportID && !!newReport?.pendingFields?.createReport && originalReport?.stateNum !== undefined && originalReport?.statusNum !== undefined; |
There was a problem hiding this comment.
When the search bulk-move flow falls through to the workspace-upgrade path, IOURequestStepUpgrade.tsx still creates an optimistic report at lines 136-148 and calls changeTransactionsReport without originalReport. Because lifecycle inheritance is gated on originalReport here, selected expenses moved from a submitted/approved report via that upgrade path keep the new report's default OPEN lifecycle, while the updated search and workspace-selection paths now inherit correctly. Please derive the single source report in the upgrade path as well (or make this helper infer it from the moved transactions) so all create-and-move entry points get the fix.
Useful? React with 👍 / 👎.
|
@neerajbachani could you please check the bot comment above? And request additional backend updates in the issues section. |
Derive single source report from selected transactions and forward it to changeTransactionsReport so lifecycle inheritance applies in IOURequestStepUpgrade.
|
@daledah checked the bot note , it’s actually valid. Also, for backend:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
@neerajbachani please request in the issue |
|
aight @daledah #86637 (comment) |
2 participants
Explanation of Change
When moving an expense onto a new report (e.g. from a submitted source report), optimistic / merged report data could show Draft instead of preserving the source report’s lifecycle (e.g. Outstanding). This PR passes an explicit
originalReportintochangeTransactionsReportso the client can apply the source report’sstateNum/statusNumwhen the destination is an optimistic newly created report, and keeps lifecycle inheritance logic aligned with that flow. Call sites that know the source report (IOUedit/step report flows, search bulk move, new report workspace selection) passoriginalReport. Aftermainrefactored report selection intouseReportSelectionActions, the sameoriginalReport+policyTagListarguments are wired through that hook so behavior stays correct with currentmain.Fixed Issues
$ #86637
PROPOSAL: #86637 (comment)
Tests
changeTransactionsReport.Offline tests
QA Steps
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
lv_0_20260517235326.online-video-cutter.com.mp4
iOS: Native
iOS: mWeb Safari
lv_0_20260518000701.mp4
MacOS: Chrome / Safari
lv_0_20260518004150.online-video-cutter.com.mp4