[No QA] Fix: useConfirmModal back navigation#78141
[No QA] Fix: useConfirmModal back navigation#78141roryabraham merged 1 commit intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@ZhenjaHorbach 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] |
| return context.showModal({ | ||
| component: ConfirmModalWrapper, | ||
| props: options, | ||
| props: { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Creating a new object literal on every showConfirmModal call causes unnecessary re-renders. The spread operator ...options combined with the wrapping object creates a new object reference each time, preventing React from optimizing renders.
Suggested fix:
const showConfirmModal = useCallback((options: ConfirmModalOptions) => {
return context.showModal({
component: ConfirmModalWrapper,
props: {
shouldHandleNavigationBack: true,
...options,
},
});
}, [context]);Or if shouldHandleNavigationBack should be overridable:
const showConfirmModal = useCallback((options: ConfirmModalOptions) => {
return context.showModal({
component: ConfirmModalWrapper,
props: {
...options,
shouldHandleNavigationBack: options.shouldHandleNavigationBack ?? true,
},
});
}, [context]);There was a problem hiding this comment.
this hook is correctly compiled by React Compiler, so this should be safe to ignore
|
Didnt upload video yet as im struggling with running local web on the device as "setupNewDotWebForEmulators" cant find any simulators, will upload once I get this work |
|
No product considerations, removing my review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2025-12-22.10.35.44.movAndroid: mWeb Chrome2025-12-22.10.35.44.moviOS: HybridApp2025-12-22.10.28.05.moviOS: mWeb Safari2025-12-22.10.21.10.movMacOS: Chrome / Safari2025-12-22.10.18.02.mov |
|
Okay That after pressing the system back button 2025-12-22.10.35.44.mov2025-12-22.10.33.07.movBut at the moment, this issue doesn't seem to be a blocker for me And since @waterim is OOO now And when @waterim returns CC: @roryabraham |
|
@ZhenjaHorbach Im not OOO yet, will work for 2 days, but this fix requires a bit more time which I dont have unfortunately. Right now I need to close all the critical things before going OOO!
Sounds good to me, lets do this in January |
|
@roryabraham can you please check it when you have some time? |
|
@waterim |
|
done |
|
✋ 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/roryabraham in version: 9.2.94-0 🚀
|
…m/fix-useConfirmModal Fix: useConfirmModal back navigation
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.94-4 🚀
|
Details
This PR fixes useConfirmModal "back" navigation
Fixed Issues
$ #68799
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Details