Migrate useSingleExecution off InteractionManager#95391
Conversation
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.
|
collectioneur
changed the title
|
posted a discussion on an alternate solution: https://expensify.slack.com/archives/C01GTK53T8Q/p1783451929635129 |
|
@Pujan92 @ikevin127 One of you needs to 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: 781df22a2e
ℹ️ 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".
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
| timeoutRef.current = setTimeout(() => { | ||
| transitionHandleRef.current = TransitionTracker.runAfterTransitions({ | ||
| callback: () => { |
There was a problem hiding this comment.
| timeoutRef.current = setTimeout(() => { | |
| transitionHandleRef.current = TransitionTracker.runAfterTransitions({ | |
| callback: () => { | |
| transitionHandleRef.current = TransitionTracker.runAfterTransitions({ | |
| waitForUpcomingTransition: true, | |
| upcomingTransitionTimeoutMs: CONST.TIMING.SINGLE_EXECUTION_TRANSITION_WAIT_TIME, // ~150 for non-nav taps | |
| callback: () => { |
shall we use waitForUpcomingTransition and new prop with a lower timeout value?
There was a problem hiding this comment.
I looked into that, but unfortunately, it won't quite work. On slower devices, the transition can sometimes take more than 150ms to register, which would bring back the exact regression that useSingleExecution was originally meant to fix.
Instead, I went with a slightly different approach: I added a new prop to GenericPressable that disables useSingleExecution altogether, and I've applied it to all buttons where users might need to tap rapidly. I feel this is a cleaner solution because it explicitly separates rapid-tap buttons from regular ones, while still safely waiting for transitions when needed. It seems like a reasonable trade-off.
As for using the new prop, that's a great suggestion, thanks! I've added the updated TransitionTracker code from this PR and applied the changes (removed setTimeout and added a timeout prop instead).
The PR is ready for another look 🙌
…when it has UX regression because of debounce
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Let's step back ... it seems like we are headed towards a more robust solution in this thread, without any delay/throttling issues, or needing to manually track the buttons for which a double-tap might trigger a double-navigation.
Meanwhile, the current solution using the (albeit deprecated) InteractionManager doesn't have these pitfalls either.
So I suggest that we close this pull request, and instead pursue the better solution described here, keeping the deprecated InteractionManager-based solution in the meantime.
|
@roryabraham Yeah, I agree with you, it’s not a clean migration. I’ve tried to add Since we are currently on version I think it might be a good idea to spin up a quick test app first, just to make sure this approach works for us. If it looks good, we can go ahead with the update. I have some availability right now, so I’d be happy to take this on and start the update if we want to go that route 🙌 |
|
sounds good |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0dd7fb8e4
ℹ️ 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".
| navigationRef.addListener('__unsafe_action__', (event) => { | ||
| if (event.data.noop || ACTION_TYPES_WITHOUT_TRANSITION.has(event.data.action.type)) { | ||
| return; |
There was a problem hiding this comment.
Plain tab bar presses in this app dispatch NAVIGATE (for example NavigationTabBar calls Navigation.navigate(...), and linkTo preserves NAVIGATE for isSwitchingTabsWithinTabNavigator). This branch treats those tab switches as transition candidates, so after the focused leaf route changes the hook waits for an upcoming TransitionTracker transition. For tabs backed by nested navigators, the new bottom-tab screen layout ignores the tab-level transition and the nested stack does not emit a stack transition for a root tab switch, so ordinary tab switches fall back to MAX_TRANSITION_START_WAIT_MS with a timeout log and a disabled pressed tab. Please special-case tab-switch NAVIGATEs or track those tab transitions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The tab-level navigator and the nested stack navigator are two different navigators. In my tests, the nested navigator always emits transitionStart and transitionEnd later than the tab navigator. That's why I disabled all tabs except for Home
|
@codex review |
war-in
left a comment
war-in
left a comment
There was a problem hiding this comment.
Changes lgtm, left a few comments
| COMPOSER_FOCUS_DELAY: 150, | ||
| MAX_TRANSITION_DURATION_MS: 1000, | ||
| MAX_TRANSITION_START_WAIT_MS: 1000, | ||
| NAVIGATION_PREDICTION_WINDOW_MS: 150, |
There was a problem hiding this comment.
I'd verify if 150ms is enough even on low-end android devices
There was a problem hiding this comment.
I’ve investigated runAfterPredictedTransition on a lower-end device (Motorola Moto G15), and the results are pretty interesting.
The registered 150ms safety timeout is blocked and does not run while the js thread is busy with react’s state update and commit. After react commits, that overdue timeout races the state listener: react navigation emits the new state from useEffect (after commit), so once the thread is free again the timeout can fire either before or after the listener sees the update.
I fixed this by adding an extra check when the 150ms timeout runs: before clearing the prediction, it checks whether the navigation ref already has the new focused route. That check is reliable for normal navigations, because the ref store is updated during react navigation setState, before commit, so the new route is already visible even if the state listener has not run yet.
| // never move the focused route. JUMP_TO can, but only tab navigators use it and they do not emit | ||
| // transitionStart/transitionEnd, so waiting on TransitionTracker would never settle. |
There was a problem hiding this comment.
I wonder if we should try to support JUMP_TO in TransitionTracker 🤔 From what the comment says, it's not an issue at the moment, but we could be future-proof and handle it right away
Of course, we should do it in a separate issue
Let me know if that's worth investigating @roryabraham @collectioneur
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0dd7fb8e4
ℹ️ 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".
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
nice direction overall! most of my comments are around scoping tab-level tracking, a couple edge cases in the prediction helper, and test coverage gaps
| backBehavior="fullHistory" | ||
| tabBar={renderTabBar} | ||
| screenOptions={screenOptions} | ||
| screenLayout={bottomTabScreenLayoutWrapper} |
There was a problem hiding this comment.
the PR description mentions wiring this with a focus guard so nested tab routes do not double-register, but screenLayout is applied on Tab.Navigator for every tab. Nested tabs already get stack screenLayout, and tab switches use animation: 'none', so it is unclear what extra transitions this adds outside Home. If Home is the only tab that mounts a screen directly, consider scoping bottomTabScreenLayoutWrapper to the Home Tab.Screen instead of the whole navigator
| let innerHandle: CancelHandle | null = null; | ||
|
|
||
| // Yield one macrotask so a navigation dispatch in the same press handler can register first. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
cancel() sets a flag and cancels the inner TransitionTracker handle, but the outer setTimeout(..., 0) is never cleared. It is harmless today because the callback no-ops when cancelled is true, but storing the timeout id and calling clearTimeout in cancel() would make the CancelHandle fully abort work (and avoids running whenPredictionSettled after unmount)
| InteractionManager.runAfterInteractions(() => { | ||
| // Re-enables the button once the predicted (or actual) transition triggered by this press | ||
| // ends - or immediately, if the press wasn't predicted to cause one. | ||
| transitionHandleRef.current = runAfterPredictedTransition(() => { |
There was a problem hiding this comment.
unmount cleanup cancels the transition handle, but if action() returned a Promise, execution.finally inside this callback can still call setIsExecuting(false) afterward. That is the same class of issue as the old uncancelled debounce timer. A small mounted/ref guard around the setIsExecuting(false) paths would prevent a post-unmount state update when the async action settles late
| return; | ||
| } | ||
|
|
||
| const didFocusedRouteChange = focusedRouteKey !== lastFocusedRouteKey; |
There was a problem hiding this comment.
when lastFocusedRouteKey is still undefined, the first hydrated key always looks like a focus change (undefined !== 'route-a'). That is probably fine once the app has emitted a baseline state event, but on a cold start where the first state event arrives after a press, this can confirm a focus move even if the action did not actually move focus yet.
Worth a test (or an explicit "baseline captured" flag) so we do not regress the double-tap fix on slow initial hydration
| expect(callback).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('waits for an upcoming transition when an action is followed by a focus change', async () => { |
There was a problem hiding this comment.
this covers the happy path where a focus change is followed by a real transitionStart. There is still no test for a focus change with no upcoming transition (for example a tab switch that changes the focused leaf route but never emits transitionStart). That path falls through to waitForUpcomingTransition: true and then the 1s TransitionTracker timeout. A test here would document the expected behavior and catch accidental 1s button lockouts
| // `runAfterPredictedTransition`/`TransitionTracker`), which don't happen in unit tests and would | ||
| // otherwise leave buttons stuck disabled. Mocked globally so no test needs to import the navigation | ||
| // listener machinery just because it renders a Pressable-based component (Button, MenuItem, etc.). | ||
| jest.mock('@hooks/useSingleExecution', () => ({ |
There was a problem hiding this comment.
globally mocking useSingleExecution makes sense for unit tests, but it means nothing in Jest exercises the native hook wiring end to end. The new runAfterPredictedTransition tests are thorough; a small native-hook test (even with mocked navigation listeners) would give extra confidence that isExecuting clears on both sync and async actions
| }; | ||
| } | ||
|
|
||
| const TransitionTracker = { |
There was a problem hiding this comment.
new onTransitionStart API drives prediction clearing in runAfterPredictedTransition, but TransitionTrackerTest does not cover listener notification or error isolation through this path (only the renamed invokeSafely messages changed). A small test that startTransition invokes subscribers would protect this hook from regressions
7 participants
Explanation of Change
Migrates native
useSingleExecutionoffInteractionManager.runAfterInteractionsonto a new heuristic helper,runAfterPredictedTransition, and fixes twoTransitionTracker/ScreenLayoutbugs that made that wait unreliable.Bug fixes in
transitionStart/transitionEndtrackingTab transitions were unreliable on Android for every tab, and on iOS for the Home tab specifically. Besides Home, the other bottom-tab routes host nested split navigators whose screens register via stack
screenLayout, but that registration relies ontransitionStart/transitionEnd, whichreact-native-screensonly fires when tied to an actual view animation. Since tab switches use animation: 'none', no such animation ever runs on Android, so the nested-stack signal never fires there either, it just happened to look fine on iOS, whereviewWillAppearfires on any visibility change regardless of animation. Home was simply the most visible case on any platform, since it has no nested navigator and thus noscreenLayoutwiring at all.Fixed by wiring
bottomTabScreenLayoutWrapperdirectly intoTabNavigator's ownscreenLayout, so tracking now comes from bottom-tabs' owntransitionStart/transitionEnd, which fires consistently on both platforms regardless of the animation option.Unmount can leave a stuck open transition. A screen may emit
transitionStartand then unmount beforetransitionEnd; cleanup unsubscribes the listeners, sotransitionEndnever arrives and queued callbacks wait up toMAX_TRANSITION_DURATION_MS(1s). Fixed by callingTransitionTracker.endTransitionon unmount whenever a handle is still open.runAfterPredictedTransition(prediction mechanism)Naive
TransitionTracker.runAfterTransitions()right after a press usually sees zero active transitions (registration gap) and unlocks too early. Always usingwaitForUpcomingTransition: truewould make non-navigating presses wait up to 1s. The new helper predicts whether the current press will cause a visual transition by watchingnavigationRef__unsafe_action__+state: if a navigable action is followed by a focused-route change withinNAVIGATION_PREDICTION_WINDOW_MS(150ms), it waits for that upcoming transition to finish; otherwise it settles immediately.useSingleExecutionthen re-enables the button after the predicted (or already-active) transition ends - or right away when no transition is predicted.Fixed Issues
$ #71913
$ #83071
Tests
Tests (on iOS and Android):
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Screen.Recording.2026-07-07.at.12.41.22.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-07-07.at.12.46.48.mov
iOS: mWeb Safari
MacOS: Chrome / Safari