Repository navigation
Soft-fail Fabric addViewAt into a non-ViewGroup parent and backport upstream differ fix (APP-K0G) - #102901
Conversation
…pstream differ fix (APP-K0G) Co-authored-by: Eugene Voloshchak <eVoloshchak@users.noreply.github.com>
Co-authored-by: Eugene Voloshchak <eVoloshchak@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
@codex review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20261006-114534-1791279914321.mp4 |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Not sure if there are concrete steps to reproduce this (couldn't find them in the error trace), so just did some smoke testing to verify nothing is broken |
|
I added smoke-test steps to the The steps cover the screens that re-render the most: fast scrolling in the Inbox, switching chats quickly, message actions, the create-expense flow, and Reports filters. Run them on Android and iOS native. |
eVoloshchak
left a comment
There was a problem hiding this comment.
LGTM
@MelvinBot, check off every checkbox in the PR author checklist and mark this ready for review
|
I checked off all 43 boxes in the PR Author Checklist. I couldn't mark the PR ready for review. That action isn't allowed in my environment. Please click Ready for review yourself. |
|
@MelvinBot, this is ready for review, mark it as ready |
|
Done. This PR is now marked as ready for review. |
|
@eVoloshchak 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
@MelvinBot is this the same patch that's required for this issue? #103369 cc @collectioneur
|
No, it's not the same patch. [Sentry: APP-H7A] Fabric removeViewAt crashes removing view from non-ViewGroup parent needs a separate patch, but it should follow the same approach.
You can add the |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.5-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.5-2 🚀
Bundle Size Analysis (Sentry): |




Explanation of Change
Sentry
APP-K0Gis a fatal Android Fabric crash:IllegalStateException: Unable to add a view into a view that is not a ViewGroup, thrown fromSurfaceMountingManager.addViewAt. It still fires on9.4.94-0, after #101057 shipped in9.4.93-3. That PR removed one source of out-of-order mounts but not the others.This PR adds two React Native patches:
+045– soft-failaddViewAtwhen the parent is not aViewGroup. RN 0.86.0 throws when the parent tag has aViewStatewhose view is null or not aViewGroup. The usual cause is a placeholderViewStatewith no view, whichupdateEventEmittercreates for a tag Java no longer has.IntBufferBatchMountItemis not retryable, so the throw crashes the app. The patch logs aReactNoCrashSoftException(including the parent view class) and skips the insert. This matches upstream's change for the same condition inremoveViewAt(3f553d7) and howaddViewAtalready handles a missing parentViewState. Trade-off: the child view can be missing until the next render rebuilds that parent, instead of the app crashing.+046– backport upstream differ fix 361bc24. In a nested flatten/unflatten, the differ could create a view that was still mounted, or delete one that only moved. It also stored a pointer to a loop-local copy inunvisitedRecursiveChildPairs. Wrong mutations like these leave the native tree out of sync with the shadow tree, which is one source of this crash family. The only change from upstream: 0.86.0'sDiffMaphas nocontains, so the patch usesfind(...) != end().Both patches are documented in
patches/react-native/details.md.Note: App does not register a
ReactSoftExceptionLoggerlistener, so the new soft exception goes to logcat, not Sentry. A drop inAPP-K0Gevents on the release that includes this PR is the signal that it worked.AI Tests:
./scripts/validatePatches.sh– passednpm run spell-changed– passed+001–+044patches (git applyagainst the patchednode_modules/react-native@0.86.0)+046C++ change also affects iOS, so CI's native build jobs are the compile check for both patches.Fixed Issues
$ #102741
PROPOSAL: #102741 (comment)
Tests
There are no reliable steps to reproduce
APP-K0G. The crash comes from a native mount race, not from a specific screen. These are smoke tests for the native mounting code that the patches change. Run them on Android and iOS native.+045is Android-only.+046is shared C++ and affects both platforms. Web and mWeb don't use this code, so for those just check that the app loads and works as usual.Offline tests
N/A. The change only affects how native views are mounted. It has no network behavior.
QA Steps
There are no reliable steps to reproduce
APP-K0G. Smoke test on Android HybridApp and iOS HybridApp: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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari