-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Migrate useSingleExecution off InteractionManager #95391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
09fa9b8
6ed1b54
c54c312
6c22f72
781df22
ecf9ca1
b873881
1730f13
f34e16e
8e0f630
98621de
d9ae7a4
6cea635
7cf7aa8
179c97c
6891682
3746a60
a8dc987
88e52e8
0b7dcf5
e0dd7fb
8cd95fd
f9ab817
925dd4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -260,6 +260,7 @@ const CONST = { | |
| COMPOSER_FOCUS_DELAY: 150, | ||
| MAX_TRANSITION_DURATION_MS: 1000, | ||
| MAX_TRANSITION_START_WAIT_MS: 1000, | ||
| NAVIGATION_PREDICTION_WINDOW_MS: 150, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd verify if 150ms is enough even on low-end android devices
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ve investigated 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 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 |
||
| EXPENSE_REPORT_DELETE_DELAY_MS: 300, | ||
| ELEMENT_NAME: { | ||
| INPUT: 'INPUT', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import {useCallback, useRef, useState} from 'react'; | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import {InteractionManager} from 'react-native'; | ||
| import runAfterPredictedTransition from '@libs/Navigation/runAfterPredictedTransition'; | ||
| import type {CancelHandle} from '@libs/Navigation/TransitionTracker'; | ||
|
|
||
| import {useCallback, useEffect, useRef, useState} from 'react'; | ||
|
|
||
| type Action<T extends unknown[]> = (...params: T) => void | Promise<void>; | ||
|
|
||
|
|
@@ -10,9 +11,17 @@ type Action<T extends unknown[]> = (...params: T) => void | Promise<void>; | |
| export default function useSingleExecution() { | ||
| const [isExecuting, setIsExecuting] = useState(false); | ||
| const isExecutingRef = useRef<boolean | undefined>(undefined); | ||
| const transitionHandleRef = useRef<CancelHandle | null>(null); | ||
|
|
||
| isExecutingRef.current = isExecuting; | ||
|
|
||
| useEffect( | ||
| () => () => { | ||
| transitionHandleRef.current?.cancel(); | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
| const singleExecution = useCallback( | ||
| <T extends unknown[]>(action: Action<T>) => | ||
| (...params: T) => { | ||
|
|
@@ -24,7 +33,9 @@ export default function useSingleExecution() { | |
| isExecutingRef.current = true; | ||
|
|
||
| const execution = action(...params); | ||
| 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(() => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unmount cleanup cancels the transition handle, but if |
||
| if (!(execution instanceof Promise)) { | ||
| setIsExecuting(false); | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout'; | |
| import useTheme from '@hooks/useTheme'; | ||
|
|
||
| import {getPreservedNavigatorState, setPreservedNavigatorState} from '@libs/Navigation/AppNavigator/createSplitNavigator/usePreserveNavigatorState'; | ||
| import {bottomTabScreenLayoutWrapper} from '@libs/Navigation/PlatformStackNavigation/ScreenLayout'; | ||
| import type {TabNavigatorParamList} from '@libs/Navigation/types'; | ||
|
|
||
| import HomePage from '@pages/home/HomePage'; | ||
|
|
@@ -95,6 +96,7 @@ function TabNavigator() { | |
| backBehavior="fullHistory" | ||
| tabBar={renderTabBar} | ||
| screenOptions={screenOptions} | ||
| screenLayout={bottomTabScreenLayoutWrapper} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the PR description mentions wiring this with a focus guard so nested tab routes do not double-register, but |
||
| UNSTABLE_router={tabRouterOverride} | ||
| > | ||
| <Tab.Screen | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,23 +24,25 @@ type RunAfterTransitionsOptions = { | |
|
|
||
| const activeTransitions = new Map<TransitionHandle, ReturnType<typeof setTimeout>>(); | ||
|
|
||
| const transitionStartListeners = new Set<() => void>(); | ||
|
|
||
| let pendingCallbacks: Array<() => void | Promise<void>> = []; | ||
|
|
||
| let nextTransitionStartResolve: (() => void) | null = null; | ||
| let promiseForNextTransitionStart = new Promise<void>((resolve) => { | ||
| nextTransitionStartResolve = resolve; | ||
| }); | ||
|
|
||
| function runCallback(callback: () => void | Promise<void>): void { | ||
| function invokeSafely(fn: () => void | Promise<void>): void { | ||
| try { | ||
| const result = callback(); | ||
| const result = fn(); | ||
| if (result instanceof Promise) { | ||
| result.catch((error) => { | ||
| Log.warn('[TransitionTracker] A pending async callback threw an error', {error}); | ||
| Log.warn(`[TransitionTracker] An async callback/listener threw an error`, {error}); | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| Log.warn('[TransitionTracker] A pending callback threw an error', {error}); | ||
| Log.warn(`[TransitionTracker] A callback/listener threw an error`, {error}); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -52,7 +54,7 @@ function flushCallbacks(): void { | |
| const callbacks = pendingCallbacks; | ||
| pendingCallbacks = []; | ||
| for (const callback of callbacks) { | ||
| runCallback(callback); | ||
| invokeSafely(callback); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -91,6 +93,10 @@ function startTransition(): TransitionHandle { | |
|
|
||
| activeTransitions.set(handle, timeout); | ||
|
|
||
| for (const listener of transitionStartListeners) { | ||
| invokeSafely(listener); | ||
| } | ||
|
|
||
| return handle; | ||
| } | ||
|
|
||
|
|
@@ -168,7 +174,7 @@ function runAfterTransitions({ | |
| } | ||
|
|
||
| if (activeTransitions.size === 0 || runImmediately) { | ||
| runCallback(callback); | ||
| invokeSafely(callback); | ||
| return {cancel: () => {}}; | ||
| } | ||
|
|
||
|
|
@@ -184,10 +190,22 @@ function runAfterTransitions({ | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Subscribes to be notified synchronously whenever a new transition starts (via {@link startTransition}). | ||
| * Returns an unsubscribe function. | ||
| */ | ||
| function onTransitionStart(listener: () => void): () => void { | ||
| transitionStartListeners.add(listener); | ||
| return () => { | ||
| transitionStartListeners.delete(listener); | ||
| }; | ||
| } | ||
|
|
||
| const TransitionTracker = { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new |
||
| startTransition, | ||
| endTransition, | ||
| runAfterTransitions, | ||
| onTransitionStart, | ||
| }; | ||
|
|
||
| export default TransitionTracker; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globally mocking
useSingleExecutionmakes sense for unit tests, but it means nothing in Jest exercises the native hook wiring end to end. The newrunAfterPredictedTransitiontests are thorough; a small native-hook test (even with mocked navigation listeners) would give extra confidence thatisExecutingclears on both sync and async actions