[CP Staging] Fix focus trap tab registration loop - #96027
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // This callback is used to register the focus trap container element of each available tab screen | ||
| const setTabFocusTrapContainerElement = (tabName: string, containerElement: HTMLElement | null) => { | ||
| const setTabFocusTrapContainerElement = useCallback((tabName: string, containerElement: HTMLElement | null) => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase (babel-plugin-react-compiler runs first in the build pipeline and this file is in scope — no "use no memo" opt-out is present). Wrapping setTabFocusTrapContainerElement in useCallback is redundant: the compiler already auto-memoizes this closure based on its captured values, and the manual wrapper adds a dependency array to maintain and can interfere with the compiler's own caching model.
Remove the useCallback and keep it a plain function:
const setTabFocusTrapContainerElement = (tabName: string, containerElement: HTMLElement | null) => {
setFocusTrapContainerElementMapping((prevMapping) => {
// ...
});
};(Note: if check-compiler.sh src/libs/Navigation/OnyxTabNavigator.tsx reports "Failed to compile" for this file, this rule does not apply and manual memoization is acceptable.)
Reviewed at: dbdaf63 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const isDiscardModalOpenRef = useRef(false); | ||
|
|
||
| const registerTabGuard: RegisterTabSwitchGuard = (guard) => { | ||
| const registerTabGuard = useCallback<RegisterTabSwitchGuard>((guard) => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler auto-memoizes closures, so wrapping registerTabGuard in useCallback<RegisterTabSwitchGuard> is redundant and adds noise the compiler already handles. Manual memoization here provides no value and must be maintained manually.
Remove the useCallback and keep the plain typed function:
const registerTabGuard: RegisterTabSwitchGuard = (guard) => {
guardsRef.current.set(guard.tabName, guard);
return () => {
// ...
};
};(Note: if check-compiler.sh src/libs/Navigation/OnyxTabNavigator.tsx reports "Failed to compile" for this file, this rule does not apply.)
Reviewed at: dbdaf63 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| isDiscardModalOpenRef.current = false; | ||
| if (result.action !== ModalActions.CONFIRM) { | ||
| guard.onCancel?.(); | ||
| const handleTabPress = useCallback( |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
Wrapping handleTabPress in useCallback with [showConfirmModal, translate] is redundant under React Compiler, which already tracks the captured variables and memoizes this handler automatically. The manual dependency array must be kept in sync by hand and can interfere with the compiler's optimizations.
Remove the useCallback wrapper and revert to a plain function declaration:
const handleTabPress = (navigation: NavigationProp<ParamListBase>, event: EventArg<'tabPress', true, undefined>) => {
// ...
};(Note: if check-compiler.sh src/libs/Navigation/OnyxTabNavigator.tsx reports "Failed to compile" for this file, this rule does not apply.)
Reviewed at: dbdaf63 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const handleContainerElementChanged = (element: HTMLElement | null) => { | ||
| setTabContainerElement(route.name, element); | ||
| }; | ||
| const handleContainerElementChanged = useCallback( |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
Wrapping handleContainerElementChanged in useCallback with [setTabContainerElement, route.name] is redundant — React Compiler auto-memoizes this closure from its captured values. The manual wrapper and dependency array add maintenance burden without benefit.
Remove the useCallback and keep it a plain function:
const handleContainerElementChanged = (element: HTMLElement | null) => {
setTabContainerElement(route.name, element);
};(Note: if check-compiler.sh src/libs/Navigation/OnyxTabNavigator.tsx reports "Failed to compile" for this file, this rule does not apply.)
Reviewed at: dbdaf63 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@marufsharifi any idea why it(OXC React Compiler) is only causing issue for this component and not anywhere else? |
|
With the help of Claude code, I see 42 files bailed out Divergent (babel/native memoizes, OXC/web bails): 42 files By reason: Files: |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
The trigger: a type-parameter reference in a type position (x as T) used inside a nested callback that React Compiler needs to memoize/outline. Evidence chain: Move const v = a as T inside a nested callback → bails ❌ (case F). The non-generic version of the same structure compiles (case I). So it's specifically the type-param cast inside the outlined function. App/src/libs/Navigation/OnyxTabNavigator.tsx Lines 264 to 267 in 6d2f215 What's happening under the hood: React Compiler outlines/memoizes that nested state callback. While lowering it to HIR, it encounters newSelectedTab as TTabName — a TSTypeReference to TTabName. To resolve the reference it looks up TTabName's declaration, finds a TSTypeParameter node, and OXC's hoisting pass has no handler for that declaration kind → it throws the Todo: Unsupported declaration type for hoisting and bails the entire component. babel-plugin-react-compiler strips/normalizes TS types earlier in its pipeline, so it never hits this — which is exactly why native memoizes and web doesn't. Hoist the cast out of the callback (cast newSelectedTab to TTabName once, above the screenListeners arrow), or @marufsharifi As claude is suggesting to hoist the case out of the callback with below changes, can you plz try this as looks good to me? // Hoist the `TTabName` casts out of the `screenListeners` callback below. OXC's React Compiler (used by the
// web build) cannot hoist a type-parameter reference that appears inside that outlined callback, so it bails
// on memoizing the whole component — which leaves the context callbacks unstable and reintroduces an infinite
// tab focus-trap update loop on web. Re-typing the consumers to accept a plain string here keeps the callback
// body free of any `as TTabName`, so the component compiles and gets auto-memoized.
const persistSelectedTab = Tab.setSelectedTab as (tabId: string, tabName: string) => void;
const notifyTabSelected = onTabSelected as (newTabName: string | undefined) => void;Replace the existing with these vars if (newSelectedTab) {
persistSelectedTab(id, newSelectedTab);
}
notifyTabSelected(newSelectedTab); |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-07-14.at.18.58.30.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-07-14.at.18.59.35.movMacOS: Chrome / SafariScreen.Recording.2026-07-14.at.18.52.33.mov |
|
The failing tests appear unrelated to the changes in our PR. |
mountiny
changed the title
|
the Jest tests are unrelated to this |
[CP Staging] Fix focus trap tab registration loop (cherry picked from commit 485f997) (cherry-picked to staging by mountiny)
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
The failing tests were unrelated to these changes and addressed in another pr already |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.34-2 🚀
|
|
No help site changes are required for this PR, so I did not create a draft docs PR. Why: This is an internal, non-user-facing fix. The only change is to There is no new or renamed feature, tab, setting, button, label, or workflow — nothing that the customer-facing articles under If I've misread the scope and you believe a specific article needs updating, let me know which one and I'll draft it. |
|
Deploy Blocker #96077 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.35-1 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR. It only modifies There is no user-facing change here:
Because nothing in @marufsharifi, if you believe a specific help article is affected that I may have missed, let me know which article/behavior and I'll take another look. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.4.35-6 🚀
|
7 participants
Explanation of Change
Fixes a web crash when opening the create expense flow.
The tab focus-trap registration in
OnyxTabNavigatorwas updating state every time the ref callback fired, even when the registered container element had not changed. After the web build pipeline change to OXC React Compiler, this unstable callback/ref path could repeatedly re-render until React threwMaximum update depth exceeded.This PR makes the registration callbacks stable and prevents redundant focus-trap mapping updates when the container element is unchanged or already absent.
Fixed Issues
$ #96035
PROPOSAL:
Tests
Offline tests
Same as Tests.
QA Steps
Same as Tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-07-14.at.3.02.55.PM.mov
Android: mWeb Chrome
Recording_20260714_145039.mp4
iOS: Native
Screen.Recording.2026-07-14.at.2.46.12.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-07-14.at.2.47.39.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-14.at.2.21.43.PM.mov