Migrate useBeforeRemove to usePreventRemove in DiscardChangesConfirmation and fix regressions#84605
Migrate useBeforeRemove to usePreventRemove in DiscardChangesConfirmation and fix regressions#84605mkzie2 wants to merge 4 commits intoExpensify:mainfrom
useBeforeRemove to usePreventRemove in DiscardChangesConfirmation and fix regressions#84605Conversation
…firmation` and fix regressions
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
sobitneupane
and removed request for
a team
|
@sobitneupane 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] |
melvin-bot
Bot
requested review from
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.gh.lumenfield.work/in/15368?s=60&v=4){kind=small}
| onCancel?: () => void; | ||
| onVisibilityChange?: (visible: boolean) => void; | ||
| isEnabled?: boolean; | ||
| hasUnsavedChanges: boolean; |
There was a problem hiding this comment.
❌ CONSISTENCY-4 (docs)
isEnabled (line 2) remains in UseDiscardChangesConfirmationOptions but is no longer destructured or referenced by either the native or web hook implementation after the changes in this PR. It is dead code in the type.
Remove isEnabled from the type definition.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| @@ -0,0 +1,10 @@ | |||
| type DiscardChangesConfirmationProps = { | |||
| onCancel?: () => void; | |||
There was a problem hiding this comment.
❌ CONSISTENCY-4 (docs)
onCancel, shouldNavigateAfterSave, and navigateBack are declared in DiscardChangesConfirmationProps but are not used by DiscardChangesConfirmation in index.native.tsx (which only destructures hasUnsavedChanges and onVisibilityChange). There is also no web counterpart that could consume them, and no file imports this component.
Remove the unused fields or add the consuming code first.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.gh.lumenfield.work/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: 93a0c067c7
ℹ️ 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".
| onModalHide: () => { | ||
| if (isConfirmed.current) { | ||
| setNavigationActionToMicrotaskQueue(goBack); | ||
| } else { | ||
| setShouldNavigateBack(false); | ||
| onCancel?.(); | ||
| } |
There was a problem hiding this comment.
onModalHide branches on isConfirmed.current, but this ref is never updated to true anywhere in the hook, so the cancel path runs on every close. In practice this means onCancel is triggered even after a confirm, and on a real cancel it can run twice (once in onModalHide, once in the .then cancel branch), which causes incorrect side effects for callers that use onCancel (e.g., refocusing inputs while navigating away).
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| }, [currentTransaction?.comment?.odometerStart, currentTransaction?.comment?.odometerEnd, isEditing]); | ||
| }, [currentTransaction?.comment?.odometerStart, currentTransaction?.comment?.odometerEnd, isEditing, hasLocalState]); |
There was a problem hiding this comment.
Including hasLocalState in this effect dependency list makes the initializer rerun when the user clears both fields while editing an existing odometer request. Because the effect still has the hasTransactionData && !hasLocalState && hasInitializedRefs.current branch, it immediately repopulates the original transaction values, so users cannot clear both readings to re-enter values from scratch.
Useful? React with 👍 / 👎.
1 participant
Explanation of Change
This PR replaces the usage of
useBeforeRemovewithusePreventRemoveinDiscardChangesConfirmationto prevent issue where the logics is not properly executed with IOS swipe back gestureFixed Issues
$ #76326
PROPOSAL: #76326 (comment)
Tests
Original Bug Fixed
Open the Hybrid app.
Create a new account.
Start a chat with any user.
Create an expense.
In the description section, type any comments and swipe right to go back without saving your changes.
Tap on create expense button.
It should avoid going back without displaying the modal to discard the changes made to the description
THIS SHOULD NOT HAPPEN: The modal for discarding changes made is not displayed, which causes the expense to load indefinitely when it is created.
Verify that no errors appear in the JS console
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-23.at.00.24.47.mov
Android: mWeb Chrome
Screen.Recording.2025-12-23.at.00.25.09.mov
iOS: Native
Screen.Recording.2025-12-23.at.00.23.46.mov
iOS: mWeb Safari
Screen.Recording.2025-12-23.at.00.25.31.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-23.at.00.22.11.mov