Remove iOSCoreAnimationBorderRendering RN patch causing HybridApp iOS AppHang (APP-8K6)#95447
Conversation
… AppHang (APP-8K6) The patch wrapped every Fabric mount in RCTMountingManager.mm in an explicit [CATransaction begin]/setDisableActions/[CATransaction commit] block. When no implicit CATransaction was already open, its commit flushed the layer tree to the render server synchronously on the main thread (CA::Layer::collect_animations_), which on the large HybridApp combined UIKit+Fabric tree could exceed the 2s AppHang threshold (Sentry APP-8K6). The border/background-stretching issue this patch worked around (Expensify#40243, from react/react-native#42604) was fixed upstream in RN 0.73.6 (d9794916b, RN Expensify#42922) and is already present in RN 0.83.1, so the workaround is no longer needed. Removes the patch and its details.md entry.
|
|
|
@bernhardoj Please 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] |
|
Also, merge with main please |
…apphang-mounting-manager
|
@bernhardoj Done! |
|
429: Too Many Requests |
1 similar comment
|
429: Too Many Requests |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.mp4iOS: mWeb SafariMacOS: Chrome / Safari |
No library update, so no need to update the Mobile-Expensify repo. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.33-0 🚀
|
Explanation of Change
This fixes the HybridApp iOS fatal App Hang reported in Sentry
APP-8K6, where the main thread was blocked for 2000ms+ insideRCTMountingManager performTransactionwith Core Animation deep inCA::Layer::collect_animations_.The hang is caused by an App-owned React Native patch,
react-native+0.83.1+008+iOSCoreAnimationBorderRendering.patch. That patch wraps every Fabric mount transaction inRCTMountingManager.mminside an explicit[CATransaction begin]/setDisableActions/[CATransaction commit]block:performTransactionis dispatched onto the main queue. When no implicitCATransactionis already open on that run-loop turn, the patch'sbeginbecomes the outermost transaction, so itscommitflushes the layer tree to the render server synchronously on the main thread instead of coalescing into Core Animation's normal end-of-run-loop flush. In HybridApp that layer tree is the combined OldDot UIKit + NewDot Fabric hierarchy, so the synchronouscollect_animations_walk can exceed the 2s AppHang threshold — which is exactly theRCTMountingManager → performTransaction → collect_animations_stack Sentry captures.The patch was originally added in #40243 (taken from facebook/react-native#42604) to work around a border/background stretching issue during navigation transitions. As the C+ reviewer confirmed on the issue, that underlying issue was fixed upstream in React Native 0.73.6 by
d9794916b(RN #42922 — "remove animation from borderLayer to stop unwanted animations"), which removes the implicit animation directly on the border layer inRCTViewComponentView.mm. This app is now on React Native 0.83.1, so that upstream fix is already present and the broad mount-time workaround is no longer needed.This PR therefore removes the patch entirely (and its entry in
patches/react-native/details.md), which eliminates the outermost synchronousCATransactioncommit during Fabric mounts while relying on the upstream border-rendering fix already shipped in RN 0.83.1. The removal is validated by the #40243 border/background regression check in the Tests section below.No automated test fixture is added: the patch is removed outright, so there is no narrowed patch left in the repo to assert against, and per the C+ reviewer's plan the fix is confirmed by monitoring Sentry
APP-8K6after release rather than by a unit test.Fixed Issues
$ #95194
PROPOSAL: #95194 (comment)
Tests
npm install(this re-runsscripts/applyPatches.sh, so the removed patch is no longer applied tonode_modules/react-native). Verify it completes with no patch-package errors orWarning:lines — this confirms the remaining patches, in particular035+fix-pressability-new-archwhich also editsRCTMountingManager.mm, still apply cleanly after008is removed.ls patches/react-native/ | grep 008returns nothing, andnode_modules/react-native/React/Fabric/Mounting/RCTMountingManager.mmno longer contains a[CATransaction begin]wrapper around thepullTransactioncall inRCTPerformMountInstructions. (Patch035'smeasureAsyncOnUI:addition near@endin the same file should still be present — that is expected.)Offline tests
This change only removes a native iOS Core Animation transaction wrapper around Fabric mounts and does not modify any network, data, or offline logic. There is no offline-specific behavior affected by this PR.
QA Steps
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
Android: mWeb Chrome
iOS: Native
ezyZip.72.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari