fix: Re-apply all props when Fabric re-creates a HybridView's native View - #1479
fix: Re-apply all props when Fabric re-creates a HybridView's native View#1479giaBaoJS wants to merge 1 commit into
Conversation
…View `CachedProp::isDirty` tracks whether a prop changed in the ShadowTree - not whether it has ever been applied to a specific native View. Fabric can create a new native View for a ShadowNode that did not change (for example when a subtree is hidden and shown again, which is what react-native-screens does when a screen is detached and re-attached). All props are clean at that point, so no setter runs and the fresh View keeps its Swift/Kotlin defaults. A newly created - or recycled - View now applies every prop it has once, which is what React Native core Views do implicitly by diffing against `oldProps` (`defaultProps` for a fresh View). Props that JS never set are skipped via the new `CachedProp::hasValue()`, so their default-constructed value is never pushed into the View. Steady-state updates are unaffected: after the first update the flag is set and only dirty props are applied, so the JNI/Swift roundtrip optimization from mrousavy#1195 still holds. Fixes mrousavy#1380
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Thanks for this fix! Independent confirmation of this root cause: we hit the exact same bug through I had opened #1484 for the same issue before finding this PR — closing it as a duplicate in favor of this one, which is better: the 🤖 Generated with Claude Code |
Fixes #1380
The bug
CachedProp::isDirtyanswers "did this prop change in the ShadowTree?", but the generatedupdatePropsuses it to answer "does this prop need to be applied to this native View?". Those are different questions the moment Fabric creates a new native View for a ShadowNode that did not change — every prop is clean, no setter runs, and the fresh view keeps its Swift/Kotlin defaults.React Native core views are immune because
updateProps(newProps, oldProps)diffs by value, andoldPropsisdefaultPropsfor a freshly mounted view, so everything is re-applied on mount.The
isDirty = falsewrite (added in 67d45a4 / #1195 to avoid repeated JNI/Swift roundtrips) is what turns the flag into one-shot state on props that are shared across mounts.Reproducing it without
react-native-screensThe issue reproduces through
react-native-screens, but the underlying trigger is plain React: a hidden<Activity>(or<Suspense>— that is whatreact-freeze, and thereforereact-native-screens, uses) drops the native views of its subtree, and showing it again creates new ones for the same, unchanged ShadowNodes.Verified on the iOS simulator on
mainby loggingHybridTestView'sinitandisBlue.didSet:hybridRefis affected by the same flag, so JS is also left holding the destroyed HybridView.The fix
A newly created — or recycled — View applies every prop it has, once. That is exactly what an RN core view does on mount; after the first update the behaviour is unchanged and only dirty props are applied.
SwiftHybridViewManager.ts): a_didUpdatePropsivar, reset inprepareForRecycleso a pooled view re-used for a different ShadowNode does not keep the previous node's values.KotlinHybridViewManager.ts):updateViewPropsis a free JNI function, so the flag lives on the view itself as a tag (needs_full_props_update_tag, next to the existingassociated_hybrid_view_tag), set increateViewInstance/prepareToRecycleViewand passed into the JNI call.One correction to the fix proposed in the issue
Forcing all props unconditionally is not safe. A prop JS never passed still has a default-constructed
CachedProp— an emptystd::function, anullptrshared_ptr<HybridObject>, and so on — and it isisDirty == falseforever, so today its setter is never called. Pushing that into Swift/Kotlin on first update would be a new crash source.So this PR adds
CachedProp::hasValue()(jsiValue != nullptr, i.e. "JS assigned this prop at some point") and the generated condition is:if ((forceUpdate && newViewProps.isBlue.hasValue()) || newViewProps.isBlue.isDirty) { … }This also makes the behaviour exactly equivalent to RN core: a prop JS never set equals
defaultProps, so core would not apply it either.Performance
This partially walks back #1195, so to be explicit about the cost:
RCTMountingManagercallsupdatePropsexactly once perInsertafter aCreate(RCTMountingManager.mm,ShadowViewMutation::Insert→updateProps:oldProps:nullptr), and the flag is set on that first call. On Android the same holds for the firstupdateStateaftercreateViewInstance.forceUpdateisfalse,isDirtydecides, andisDirty = falsestill short-circuits unchanged props on later clones. The JNI/Swift roundtrips perf: SetisDirtytofalseto avoid JNI roundtrips #1195 removed stay removed.Remove+Insert(reorder) reuses the same view instance, whose flag is already set.hasValue()), so the forced pass is bounded by the props actually present on the element.Net: on mount, a Nitro view now does what an RN core view has always done. Nothing changes for the update path that #1195 optimized.
Test
example/__tests__/views.harness.tsx— a runtime test in the Harness suite.getTests.tshas no React renderer, and this bug is only observable by mounting a view, so it lives in__tests__/alongsidenitro.harness.ts;example/__tests__/**is already in both harness workflows' path filters. It reuses the existingTestViewspec and adds no new spec, struct, or dependency.The test renders
<TestView isBlue={true} />inside an<Activity>, hides it, shows it again, and asserts that the re-created View reportsisBlue === true— i.e. that the box is still blue, which is the user-visible symptom in the issue.Counterfactual, iOS simulator (iPhone 17, iOS 26.3), same test file both times:
Full suites:
523 passed, 523 total(2 suites).9 failed, 1 skipped, 513 passed. The 9 failures are allcreateHardwareBuffer/ArrayBuffercases and are identical onmainwithout this change (verified by rebuilding and re-running the baseline APK) — emulator limitations, unrelated to this PR. The 1 skip is the new test, see below.What I could not verify
The new test is iOS-only. On Android with RN 0.85.3 I could not get Fabric to re-create a HybridView's native View: with the same
<Activity>toggle,createViewInstanceis called exactly once (confirmed by loggingHybridTestView'sinitand reading logcat) — the view survives being hidden. The issue reporter also only validated on iOS.The Android half of the fix is therefore the symmetric change to identical code, verified to build and to leave the existing Android suite unchanged, but not verified to fix an observable Android symptom. It is a no-op on Android today:
forceUpdateis only evertrueon the firstupdateState, where every prop JS set is already dirty. If you would rather keep the Android side out until there is a reproducing case, I am happy to drop it.Notes
bun specswas re-run; the generated diff is limited to the view manager files and is mechanical. This PR adds no spec, so the generated tree changes only because of the codegen change.bun typecheckandbun lint-all(JS/TS, clang-format, swift-format, ktlint) pass with no reformatting.| undefinedon array elements and Record values #1478 (my other open PR, which also regenerates nitrogen output): merges cleanly with no conflicts, andbun specson the merged tree produces no further diff — the two touch disjoint generated files (views/*vsHybridTestObject*).