Prevent self-parented report when preexistingReportID equals parentReportID - #99536
Conversation
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Works fine and does not freeze |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
This looks to be specific issue for supportal we will fix it in BE as well but this looks like a safe gate for now
|
@linhvovan29546 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] |
![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: 120d71137c
ℹ️ 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".
| if (preexistingReportID === parentReportID) { | ||
| return; |
There was a problem hiding this comment.
When the API returns this equality, preexistingReportID still means the server rejected the optimistic reportID; HandleUnusedOptimisticID explicitly rewrites queued requests because using that ID would otherwise return 404. This early return leaves the user and the parent action pointing to that local-only report, so any comment sent after the response uses the rejected ID via addActions and fails instead of reaching a server-backed thread. Avoid the self-parent assignment while still reconciling the optimistic report to a valid server report.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is temporary and specific to Supportal, we will unwind this later
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.62-1 🚀
|
Help site review: no changes requiredI reviewed the changes in this PR against Expensify's help site files under Why: This is a defensive, internal-only bug fix. It adds an early return in The fix has no user-facing surface — no new or changed features, UI labels, tabs, settings, buttons, workflows, billing/pricing, or policy behavior. The help site documents user-facing product behavior, and there is nothing here for it to describe. The only changes are the guard clause plus unit tests covering it. No draft PR was created since there is nothing to document. @TMisiukiewicz, since no help site changes are required, there is no linked help site PR to review. If you believe a user-facing behavior did change and should be documented, let me know and I'll draft the PR. |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.62-4 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
The API can return
preexistingReportIDequal to the optimistic report's ownparentReportID. When that happens,replaceOptimisticReportWithActualReportcopiesparentReportIDverbatim onto the preexisting report viaOnyx.set, producing a report wherereportID === parentReportID. Every function that walks report ancestry (getAncestors,isSelfDMOrSelfDMThread,getRootParentReport) then loops forever —getAncestorsfreezes the web app andisSelfDMOrSelfDMThreadthrowsRangeError: Maximum call stack size exceededfromMoneyRequestView.This adds an early return in
replaceOptimisticReportWithActualReportwhenpreexistingReportID === parentReportID, before any Onyx write. Nothing is deleted and nothing is overwritten, so the optimistic report stays intact and the parent action'schildReportIDkeeps pointing at a report that exists. Three unit tests cover the new branch: the parent report keeps its own data, the optimistic report is not cleared, and the user is not navigated away.Fixed Issues
$ #99594
PROPOSAL:
Tests
Offline tests
QA Steps
Ping @mountiny to test, we need to suppotal to the customer account https://expensify.slack.com/archives/C05LX9D6E07/p1787656510726119
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