Fix iOS Track distance screen blink on swipe back - #97020
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.
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.
|
@nyomanjyotisa 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] |
|
@nyomanjyotisa @heyjennahay ready for review. The issue only reproduces on iOS, so I recorded iOS native and mWeb Safari. Happy to add Android and macOS if you want them. |
![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: 938fec7d22
ℹ️ 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".
| setTimeout(() => { | ||
| setShouldPreventRemove(isFocused && !isSavingRef.current && getHasUnsavedChanges()); | ||
| }, 0); |
There was a problem hiding this comment.
When a clean native screen becomes dirty, this leaves shouldPreventRemove false until the zero-delay timer fires and the follow-up render propagates to usePreventRemove. During that window an iOS edge-swipe/header pop can start with preventNativeDismiss still disabled and dismiss the dirty amount/distance/hours screen without showing the discard modal; the new tests don't catch this because they call the mocked beforeRemove callback even when the flag is false. The guard needs to be armed synchronously (or otherwise conservatively) before any deferred ref read can refine it.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // `usePreventRemove` reads this during render, so the signal must be state, never a ref (React Compiler) | ||
| const [shouldPreventRemove, setShouldPreventRemove] = useState(false); | ||
| // Deferred past the commit so ref-backed inputs are read after their child state settles, not one event behind | ||
| const recheckUnsavedChanges = useCallback(() => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase and automatically memoizes closures based on their captured variables. If this hook compiles with React Compiler (which the PR states is the case), the manual useCallback wrapping recheckUnsavedChanges is redundant — it adds a dependency array to maintain and interferes with the compiler's own caching.
Remove the useCallback and let the compiler memoize the closure:
const recheckUnsavedChanges = () => {
setTimeout(() => {
setShouldPreventRemove(isFocused && !isSavingRef.current && getHasUnsavedChanges());
}, 0);
};(If check-compiler.sh reports "Failed to compile" for this file, disregard — the rule does not apply when the file cannot be compiled.)
Reviewed at: 938fec7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }, 0); | ||
| }, [isFocused, getHasUnsavedChanges]); | ||
| // Runs every commit since dirtiness often lives in refs; screens that never re-render on input call `recheckUnsavedChanges` themselves | ||
| useEffect(recheckUnsavedChanges); |
There was a problem hiding this comment.
❌ PERF-12 (docs)
useEffect(recheckUnsavedChanges) runs on every commit and each run schedules a setTimeout(..., 0) that is never cleared. If the component unmounts (e.g. the screen is dismissed) with a timer still pending, the callback fires after unmount and calls setShouldPreventRemove on an unmounted component — an uncleaned resource / potential leak and a React state-update-after-unmount warning. A setTimeout(…, 0) scheduled on the final pre-unmount commit is not guaranteed to run before unmount.
Track the timer id and clear it in the effect's cleanup:
const recheckUnsavedChanges = useCallback(() => {
// return the id so the caller can clear it
}, [isFocused, getHasUnsavedChanges]);
useEffect(() => {
const id = setTimeout(() => {
setShouldPreventRemove(isFocused && !isSavingRef.current && getHasUnsavedChanges());
}, 0);
return () => clearTimeout(id);
});Note that the manual recheckUnsavedChanges() calls from the input change handlers also leak their timers and should be cleaned up (e.g. via a shared ref that is cleared on unmount).
Reviewed at: 938fec7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
An explicit recheck now sets shouldPreventRemove immediately and lets the deferred read relax it, so a swipe starting before the timer fires cannot dismiss a dirty screen unguarded. The every-commit effect stays a plain read to keep preventNativeDismiss off clean screens.
nyomanjyotisa
left a comment
nyomanjyotisa
left a comment
There was a problem hiding this comment.
Swipe-back correctly shows the discard modal for dirty Odometer and Hours inputs. After reverting to the baseline, however, an immediate swipe is swallowed and only works after the deferred recheck.
Could you please follow the selected direction by deriving dirtiness from current values and baselines instead of a ref recheck, while retaining the callback safety check and covering clean, dirty, reverted, and prefilled cases?
Please also add an Hours test step, recordings for Manual distance, Amount +/-, and Hours, plus the missing Android native, Android mWeb, and macOS recordings.
Each step screen now mirrors its input value in state and compares it with the committed baseline, so the hook can read dirtiness during render. That removes the ref recheck, the deferred timer, and the window where reverting to the baseline left the guard armed and swallowed an immediate swipe. The in-callback check still applies the save suppression, and the amount value is reported signed the same way the form composes it.
|
Please let me know once everything in my previous review has been addressed. |
|
@MobileMage bump |
|
@nyomanjyotisa I've made the changes and updated the videos |
nyomanjyotisa
left a comment
nyomanjyotisa
left a comment
There was a problem hiding this comment.
Please also update the Explanation of Change to match the latest implementation.
| @@ -159,11 +159,12 @@ function IOURequestStepDistanceManual({ | |||
| const distanceInMeters = getDistanceInMeters(transaction, transaction?.comment?.customUnit?.distanceUnit ? transaction.comment.customUnit.distanceUnit : unit); | |||
| const distance = typeof transaction?.comment?.customUnit?.quantity === 'number' ? roundToTwoDecimalPlaces(DistanceRequestUtils.convertDistanceUnit(distanceInMeters, unit)) : undefined; | |||
|
|
|||
| const committedDistance = distance?.toString() ?? ''; | |||
| // Mirrors the input so dirtiness compares the current value against the baseline instead of reading a ref | |||
| const [typedDistance, setTypedDistance] = useState(committedDistance); | |||
There was a problem hiding this comment.
This stays stale if a prefilled transaction loads after mount, so an untouched screen shows the discard prompt. Please sync it and add a late-hydration test.
The mount-time mirror read committedDistance before Onyx had the transaction, so it held '' while the input already showed the loaded distance. An untouched edit screen then read as dirty and prompted on back. The sync effect now seeds the mirror alongside the imperative input update and keys on committedDistance, matching IOURequestStepHours. The new test mounts with only the report in Onyx, merges the transaction after mount, and asserts the guard stays disarmed.
|
Both are fixed and pushed in 94c2d41.
I also rewrote the Explanation of Change. It described |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
|
||
| // Callers derive dirtiness from current values and baselines, so this is safe to read during render. | ||
| // The save suppression stays out of it because `isSavingRef` is a ref: the callback below applies that. | ||
| const shouldPreventRemove = isFocused && getHasUnsavedChanges(); |
There was a problem hiding this comment.
The Odometer guard still misses dirty input on iOS native. Entering 7 and edge-swiping back dismisses the screen without the discard modal. The Odometer callback reads refs, so React Compiler keeps this render-time result stale. Please derive dirtiness from startReading and endReading, add native clean → dirty → reverted coverage, and update the iOS recording.
|
Hi, I encountered this on the QAB "Create expense" flow too (amount step, 1:1 and to WS) - same blink on swipe-back. Bisected it down to |
|
@JakubKorytko Yes, I tested it and confirmed that this PR fixes the QAB Create expense flow as well. |
|
@MobileMage bump |
|
@nyomanjyotisa Fixed and pushed in 99ddc4d.
iOS recording next. |
|
Waiting for the iOS recording. Please also resolve the conflicts, @MobileMage. |
# Conflicts: # src/pages/iou/request/step/IOURequestStepDistance.tsx
|
@nyomanjyotisa Merged main, conflicts resolved, CI is green, and the iOS recording is updated.
Clean -> dirty -> reverted coverage is in |
nyomanjyotisa
left a comment
nyomanjyotisa
left a comment
There was a problem hiding this comment.
Please also update the Explanation of Change to include the Odometer readingsBaseline state mirror.
| @@ -0,0 +1,241 @@ | |||
| /* eslint-disable @typescript-eslint/no-unsafe-assignment */ | |||
There was a problem hiding this comment.
Could you add a brief justification for these file-wide ESLint disables? The same applies to IOURequestStepDistanceOdometerDiscardGuardTest.tsx.
There was a problem hiding this comment.
I just took them out entirely, most of it came from one bare require('react') making everything downstream any, so typing that plus the native hook cleared nearly all of it.
There's one assertion left for the route params the screen reads at runtime but the type calls never.
3 participants
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 arms the guard only when the screen has unsaved changes.
useDiscardChangesConfirmation/index.native.tscomputesisFocused && getHasUnsavedChanges()during render and passes that tousePreventRemove. A clean screen dismisses in one native animation with no blink, and a dirty screen still cancels the swipe and shows the discard modal. ThebeforeRemovecallback re-checks dirtiness and adds the save suppression, which is what lets an intentional save through:isSavingRefis a ref, so it cannot be read during render.Reading dirtiness during render only works if every caller derives it from values that change with a re-render. Each step that used to read its input through an imperative ref at navigation time now mirrors that input into state and diffs it against the committed value on the transaction:
IOURequestStepAmountmirrorstypedAmount, fed byMoneyRequestAmountForm's newonAmountChange. It staysundefineduntil the form reports a change, so a prefilled amount starts clean. The +/- sign flip reports as well, since it bypasses the input's own change handler.IOURequestStepHoursmirrorstypedCountand re-seeds it fromcommittedCountin the same effect that pushes the value into the input.IOURequestStepDistanceManualmirrorstypedDistancethe same way. The re-seed carries weight here because the transaction can hydrate after the screen mounts, which would otherwise leave an empty mirror against a loaded distance and prompt on a screen the user never touched.IOURequestStepDistancemirrors the Manual tab's value inmanualDistanceValue,undefineduntil that tab reports one, so a map expense the user never switched to Manual is not compared against an empty field.The web hook keeps its behavior: it reads dirtiness inside
useBeforeRemoveat navigation time. A hardware back goes through this hook's ownBackHandlerlistener, which callshasUnsavedChanges()directly rather than throughpreventRemove, and header-back taps flow through the same re-armed guard.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
Android: Native
97020-android-native-small.mp4
Android: mWeb Chrome
97020-android-mweb-small.mp4
iOS: Native
Cap.2026-08-15.at.16.38.59.mp4
iOS: mWeb Safari
97020-ios-mweb.mp4
MacOS: Chrome / Safari
97020-chrome-web-small.mp4