Freeze non top screens to prevent extra-rerenders 2#85443
Freeze non top screens to prevent extra-rerenders 2#85443VickyStash wants to merge 7 commits intoExpensify:mainfrom
Conversation
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
FYI, there is a bug with newly created account for gmail user - I can't pass through the onboarding modal. This happens on main too, so it's not related to current pr: main-bug-onboarding.mp4 |
|
@MelvinBot please post summary of new changes which fix regressions from v1 PR. |
| useLayoutEffect(() => { | ||
| // When unfreezing, always apply immediately so the screen is visible right away. | ||
| if (!isScreenBlurred) { | ||
| // eslint-disable-next-line react-hooks/set-state-in-effect |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react-hooks/set-state-in-effect suppression lacks an explicit justification for why the lint rule is safe to bypass here. The preceding comment describes the intended behavior but does not explain why calling setFrozen synchronously inside useLayoutEffect will not cause re-render loops.
Add a comment directly explaining why the rule is safe to disable, for example:
// eslint-disable-next-line react-hooks/set-state-in-effect -- Synchronous unfreeze is intentional; the early return prevents infinite loops since isScreenBlurred is the only dependency and won't change as a result of this setState.
setFrozen(false);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| // No blockers or overlays — freeze immediately. | ||
| // eslint-disable-next-line react-hooks/set-state-in-effect |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react-hooks/set-state-in-effect suppression lacks an explicit justification for why the lint rule is safe to bypass here. The preceding comment describes the intent but does not explain why calling setFrozen inside useLayoutEffect will not cause re-render loops.
Add a comment directly explaining why the rule is safe to disable, for example:
// eslint-disable-next-line react-hooks/set-state-in-effect -- Synchronous freeze is safe; isScreenBlurred is the only dependency and setFrozen(true) won't trigger further isScreenBlurred changes.
setFrozen(isScreenBlurred);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Summary of New Changes Fixing v1 RegressionsThis PR re-applies the original v1 PR (#82764) and adds three fix commits on top to address the five regressions that caused the revert. Here's what each fix does: 1. Fix for #85184 — Web: App freezes when clicking "Go to Expensify Cards" buttonCommit: The original Fix: Added an 2. Fix for #85170, #85185, #85250 — iOS: Gray-out on swipe-back, Copilot unresponsive, Downgrade freezeCommit: All three iOS issues shared the same root cause: on Fabric, the Fix: Introduced a 3. Fix for #85149 — Web: ENTER on Merchant opens Schedule Call after visiting #adminsCommit: When a screen was frozen, its keyboard shortcuts were supposed to unsubscribe via cleanup effects. But the freeze happened before cleanup could run, leaving stale shortcuts from a previously visited screen (e.g., #admins' "Schedule Call" shortcut) still active. Pressing Enter would trigger that stale shortcut instead of the expected action. Fix: Introduced a freeze defer mechanism:
TL;DR: The three fix commits address: (1) modal-awareness in freeze logic, (2) a longer delay for iOS native animations before freezing, and (3) a defer mechanism so keyboard shortcut cleanup runs before screens are frozen. |
Code Review: Freeze non-top screens v2Overall this is a well-structured approach to preventing unnecessary re-renders. The 1.
|
| # | Issue | Severity | Type |
|---|---|---|---|
| 1 | contextValue not memoized — causes re-renders of all keyboard shortcut consumers in split screens |
Medium | Performance |
| 2 | rAF timing in submitAndClose may fire before native DelayedFreeze unfreezes target screen |
Medium | Correctness |
| 3 | wrappedDescriptors not memoized |
Low | Performance |
| 4 | DelayedFreeze delays unfreezing by 400ms — should unfreeze immediately |
Medium | Design |
| 5 | Minor: magic number, missing dep, missing comment | Low | Code quality |
Items 1 and 4 seem most actionable. The rest are worth discussing but may be acceptable tradeoffs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5450de216
ℹ️ 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".
| + const id = setTimeout(() => { | ||
| + rafID = requestAnimationFrame(() => setFreezeState(freeze)); | ||
| + }, FREEZE_DELAY_MS); |
There was a problem hiding this comment.
Unfreeze screens immediately when focus returns
This effect now waits FREEZE_DELAY_MS for all freeze transitions, not just when entering the frozen state. When a route becomes focused again (freeze flips true -> false), setFreezeState(false) is delayed by ~400ms, so the newly focused screen can remain suspended and miss immediate UI updates/taps after back/pop navigation. The delay should only apply when freeze is true, while unfreezing should happen immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In DelayedFreeze if freeze is false it always applied right away, https://github.com/software-mansion/react-native-screens/blob/63b3baab65a1fd36da04ae426f98ad460217e1e0/src/components/helpers/DelayedFreeze.tsx#L24
return <Freeze freeze={freeze ? freezeState : false}>{children}</Freeze>;
|
Melvin's comments don't look accurate to me, as it thinks that unfreezing is delayed too, but it's not, as I've mentioned here #85443 (comment). |
|
Currently blocked hybrid iOS testing https://expensify.slack.com/archives/C05LX9D6E07/p1773753991898119 |
|
No product review needed |
@situchan it's not blocked anymore |
|
Hey @situchan, any ETA on this one? |
|
Please merge main. I'll review today |
|
@situchan kind bump here, since I'll be OOO March 25-27 next week |
|
I've been testing this from yesterday. Will update shortly |
|
iOS: app crashes immediately after creating new account crash.mov |
|
@MelvinBot can you investigate #85443 (comment)? |
🔍 Investigation: iOS crash after creating new accountConfidence: Medium-High (code evidence is strong, but no native crash stack trace to fully confirm) Root CauseThe crash is caused by the combination of three changes in this PR interacting poorly with the new-account onboarding navigation flow:
Why this is a new regression (not seen in v1 PR #82764)All v1 issues (#85184, #85185, #85149, #85170, #85250) were freezes/unresponsiveness, not actual crashes. The new account creation flow was never a known failure mode. The v2 fixes (400ms delay, modal checks, freeze defer) changed the timing characteristics enough to turn what was previously a freeze-class issue into a native crash during the specific onboarding navigation sequence. Notably, Evidence Chain
Suggestions
Open Questions
|
|
Yeah, I'm able to reproduce that + overall the behaviour on ios became super weird (strange navigation transitions and etc). I'm trying to see what has happend, maybe some overlapping updates were merged. |
|
Okay, it's seems to be broken after React Native v0.83 upgrade merge. I'm figuring out why |
|
Okay, it looks like the But there is a new component supported in React 19.2 - Activity - it hides components and unmounts effects. I had a quick test, and it looks pretty good. So maybe it can be a good alternative to the current approach. |
Explanation of Change
Freezes non-top screens in the navigation stack to prevent unnecessary re-renders, using a ScreenFreezeWrapper for the split navigator on web and a react-navigation patch for mobile.
Previous PR #82764 was reverted #85198 with the next blockers:
$ #85184
$ #85185
$ #85149
$ #85170
$ #85250
Fixed Issues
$ #33725
PROPOSAL: https://expensify.slack.com/archives/C05LX9D6E07/p1771428903641059
Tests
#85184 — Web: App freezes when clicking "Go to Expensify Cards" button
Preconditions: Have a workspace with Expensify cards enabled. At least one Expensify card is assigned with the limit type set to Smart limit.
#85185 — iOS: App becomes unresponsive when changing Copilot access
Preconditions: Account has a copilot set.
#85149 — Web: ENTER on Merchant opens Schedule Call after visiting #admins
#85170 — iOS: Workspace settings gray out briefly after swiping back
#85250 — iOS: App freezes on downgrade workspace page
Offline tests
Same, as in the Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in the Tests section
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
android.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
ios-85250.mov
ios-85170.mov
ios-85185.mp4
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4
web-85149.mp4
web-85184.mp4