-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Freeze non top screens to prevent extra-rerenders #82764
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
Changes from all commits
43fa954
273e9c1
b7f25fe
13e4629
74563bc
11821ab
fbf4646
7f9b9e7
ab31e39
176b708
9df11aa
76d2a9b
ad30958
971bd23
cfe0d14
181b060
a72359c
16ecb00
b0e8908
16bb598
b16e8f9
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 |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # `react-native-screens` patches | ||
|
|
||
| ### [react-native-screens+4.15.4+001+delay-freeze-until-paint.patch](react-native-screens+4.15.4+001+delay-freeze-until-paint.patch) | ||
|
|
||
| - Reason: Adds `requestAnimationFrame` to `DelayedFreeze` so the screen freeze is deferred until after the current frame is painted. Without this, screens can be frozen while transitional UI states are still visible, causing a visual flicker. | ||
| - Upstream PR/issue: N/A | ||
| - E/App issue: https://github.com/Expensify/App/issues/33725 | ||
| - PR introducing patch: https://github.com/Expensify/App/pull/82764 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| diff --git a/node_modules/react-native-screens/src/components/helpers/DelayedFreeze.tsx b/node_modules/react-native-screens/src/components/helpers/DelayedFreeze.tsx | ||
| index 443da63..72517fd 100644 | ||
| --- a/node_modules/react-native-screens/src/components/helpers/DelayedFreeze.tsx | ||
| +++ b/node_modules/react-native-screens/src/components/helpers/DelayedFreeze.tsx | ||
| @@ -12,12 +12,17 @@ function DelayedFreeze({ freeze, children }: FreezeWrapperProps) { | ||
| // flag used for determining whether freeze should be enabled | ||
| const [freezeState, setFreezeState] = React.useState(false); | ||
|
|
||
| + // We use requestAnimationFrame to ensure the current frame is fully painted | ||
| + // before scheduling the freeze. This prevents freezing the screen while | ||
| + // transitional UI states (e.g. opacity from useSingleExecution) are still visible. | ||
| React.useEffect(() => { | ||
| + let rafID: number; | ||
| const id = setTimeout(() => { | ||
| - setFreezeState(freeze); | ||
| + rafID = requestAnimationFrame(() => setFreezeState(freeze)); | ||
| }, 0); | ||
| return () => { | ||
| clearTimeout(id); | ||
| + cancelAnimationFrame(rafID); | ||
| }; | ||
| }, [freeze]); | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| diff --git a/node_modules/@react-navigation/native-stack/lib/module/views/NativeStackView.native.js b/node_modules/@react-navigation/native-stack/lib/module/views/NativeStackView.native.js | ||
| index 35dfc05..1758a24 100644 | ||
| --- a/node_modules/@react-navigation/native-stack/lib/module/views/NativeStackView.native.js | ||
| +++ b/node_modules/@react-navigation/native-stack/lib/module/views/NativeStackView.native.js | ||
| @@ -376,9 +376,9 @@ export function NativeStackView({ | ||
| const isModal = modalRouteKeys.includes(route.key); | ||
| const isPreloaded = preloadedDescriptors[route.key] !== undefined && descriptors[route.key] === undefined; | ||
|
|
||
| - // On Fabric, when screen is frozen, animated and reanimated values are not updated | ||
| - // due to component being unmounted. To avoid this, we don't freeze the previous screen there | ||
| - const shouldFreeze = isFabric() ? !isPreloaded && !isFocused && !isBelowFocused : !isPreloaded && !isFocused; | ||
| + // Freezing the screen below the focused one is safe on Fabric because | ||
| + // DelayedFreeze defers it to the next macrotask, and transition animations run on the UI thread. | ||
| + const shouldFreeze = !isPreloaded && !isFocused; | ||
| return /*#__PURE__*/_jsx(SceneView, { | ||
| index: index, | ||
| focused: isFocused, | ||
| diff --git a/node_modules/@react-navigation/native-stack/src/views/NativeStackView.native.tsx b/node_modules/@react-navigation/native-stack/src/views/NativeStackView.native.tsx | ||
| index ca15b1d..3191475 100644 | ||
| --- a/node_modules/@react-navigation/native-stack/src/views/NativeStackView.native.tsx | ||
| +++ b/node_modules/@react-navigation/native-stack/src/views/NativeStackView.native.tsx | ||
| @@ -542,11 +542,9 @@ export function NativeStackView({ | ||
| preloadedDescriptors[route.key] !== undefined && | ||
| descriptors[route.key] === undefined; | ||
|
|
||
| - // On Fabric, when screen is frozen, animated and reanimated values are not updated | ||
| - // due to component being unmounted. To avoid this, we don't freeze the previous screen there | ||
| - const shouldFreeze = isFabric() | ||
| - ? !isPreloaded && !isFocused && !isBelowFocused | ||
| - : !isPreloaded && !isFocused; | ||
| + // Freezing the screen below the focused one is safe on Fabric because | ||
| + // DelayedFreeze defers it to the next macrotask, and transition animations run on the UI thread. | ||
| + const shouldFreeze = !isPreloaded && !isFocused; | ||
|
|
||
| return ( | ||
| <SceneView | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import React, {useLayoutEffect, useState} from 'react'; | ||
| import {Freeze} from 'react-freeze'; | ||
| import TooltipSense from '@components/Tooltip/TooltipSense'; | ||
|
|
||
| type ScreenFreezeWrapperProps = { | ||
| /** Whether the screen is not currently visible to the user */ | ||
| isScreenBlurred: boolean; | ||
|
|
||
| /** The screen content to freeze when blurred */ | ||
| children: React.ReactNode; | ||
|
VickyStash marked this conversation as resolved.
|
||
| }; | ||
|
|
||
| function ScreenFreezeWrapper({isScreenBlurred, children}: ScreenFreezeWrapperProps) { | ||
|
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. ❌ CONSISTENCY-3 (docs)The
Consider extracting the shared freeze logic into a single reusable component. For example, refactor the existing Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
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. Seems unnecessary I think. Slightly different cases.
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. Yeah, I would leave it as is for now. |
||
| const [frozen, setFrozen] = useState(false); | ||
|
|
||
| // Decouple the Suspense render task so it won't be interrupted by React's concurrent mode | ||
| // and stuck in an infinite loop | ||
| useLayoutEffect(() => { | ||
| if (!TooltipSense.isActive() || !isScreenBlurred) { | ||
| // eslint-disable-next-line react-hooks/set-state-in-effect | ||
| setFrozen(isScreenBlurred); | ||
| } | ||
|
|
||
| // When a tooltip is active, defer freezing by one frame so it can dismiss first. | ||
| // Otherwise the frozen tree can't hide the tooltip portal rendered on <body>. | ||
| const id = requestAnimationFrame(() => setFrozen(isScreenBlurred)); | ||
| return () => { | ||
| cancelAnimationFrame(id); | ||
| }; | ||
| }, [isScreenBlurred]); | ||
|
|
||
| return <Freeze freeze={frozen}>{children}</Freeze>; | ||
| } | ||
|
|
||
| export default ScreenFreezeWrapper; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,7 +142,7 @@ function WorkspaceInviteMessageComponent({ | |
| } | ||
|
|
||
| if ((backTo as string)?.endsWith('members')) { | ||
| Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.dismissModal()); | ||
| Navigation.dismissModal(); | ||
|
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 am not able to reproduce iOS - App doesn't return to members page after inviting a user to a workspace even without this change. Screen.Recording.2026-03-07.at.4.32.01.AM.mov
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. @situchan Please, try to follow these steps:
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 tried but still not reproducible. Btw not blocker |
||
| return; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.