Fix iOS Track distance screen blink on swipe back#97020
Draft
MobileMage wants to merge 3 commits into
Draft
Conversation
usePreventRemove was hard-coded to true, so iOS set preventNativeDismiss even with nothing to discard: the edge-swipe was cancelled and snapped back before JS popped the screen. Drive the guard from a shouldPreventRemove state that only reflects real unsaved changes, recomputed in a callback/effect (never reading refs during render, so React Compiler still compiles the hook). Ref-based input screens call recheckUnsavedChanges after each keystroke so the guard re-arms. Fixes Expensify#94904.
7 tasks
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
The preventRemove state could read one input event behind (or never arm) for screens whose typed value lives in a child input ref: - Defer the recompute past the commit so it reads the child's settled state instead of the previous render's value (single digit or paste then back would have dismissed with no prompt). - Re-evaluate after every commit instead of only when the memoized dirtiness callback changes identity, which covers screens like the odometer step whose refs move together with state the compiler does not track through the callback. - Wire recheckUnsavedChanges into the distance step's manual tab, which never re-renders on typing. - Notify the parent from the amount sign flip, which bypassed the input's change handler entirely. - Replace the web hook's counter reducer with a stable no-op so amount, hours, and manual-distance screens stop re-rendering per keystroke. - Pin the recheck arming behavior in the native hook tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
On iOS, edge-swiping back on an IOU step screen (Track distance, amount, hours, manual distance) flashes the screen before it dismisses. The shared discard-changes hook armed react-navigation's removal guard with a hard-coded
usePreventRemove(true, ...), which iOS maps topreventNativeDismiss = true. On a clean screen with nothing to discard, the native swipe still gets cancelled and snaps back before JS re-dispatches the pop, and that snap-back is the blink.This PR ties the guard to whether the screen has unsaved changes. The native hook now drives
usePreventRemovefrom ashouldPreventRemovestate, recomputed from the screen's dirtiness in a callback and effect rather than by reading refs during render, so the hook still compiles under React Compiler. The recompute runs after every commit and is deferred one tick past it, so ref-backed inputs (which update their child's state in the same event) are read after they settle. A clean screen dismisses in one native animation with no blink; a dirty screen still blocks the swipe and shows the discard modal, and the in-callback check still lets an intentional save pass through.Screens whose dirtiness transitions come with a re-render are covered by the every-commit recompute. Screens that go dirty without re-rendering at all call the hook's new
recheckUnsavedChangesfrom their change handler:IOURequestStepAmount(viaMoneyRequestAmountFormforwardingonAmountChange, including the +/- sign flip, which bypassed the input's change handler),IOURequestStepHours,IOURequestStepDistanceManual, and the distance step's Manual tab inIOURequestStepDistance. The web hook keeps its behavior (it reads dirtiness inuseBeforeRemoveat navigation time) and returns a stable no-op recheck for the shared type. Android's back handling reads dirtiness on its own rather than throughpreventRemove, and header-back taps flow through the same re-armed guard.Reviewer note: the accepted proposal's literal
usePreventRemove(hasUnsavedChanges())reads a ref during render, which fails the repo's React Compiler compliance gate (main compiles this hook today, so that would be a regression). This PR implements the same behavior through ashouldPreventRemovestate recomputed in a callback/effect instead.Fixed Issues
$ #94904
PROPOSAL: #94904 (comment)
Tests
Offline tests
No offline-specific behavior. The change is client-side navigation only.
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Videos will be added in a few hours.
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari