Skip to content

feat(session-replay-react-native): add Fabric SRMaskView foundation#1843

Draft
chungdaniel wants to merge 5 commits into
mainfrom
feat/sdkrn-32-fabric-foundation
Draft

feat(session-replay-react-native): add Fabric SRMaskView foundation#1843
chungdaniel wants to merge 5 commits into
mainfrom
feat/sdkrn-32-fabric-foundation

Conversation

@chungdaniel

@chungdaniel chungdaniel commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds the dual-arch Fabric foundation for layout-transparent masking in @amplitude/session-replay-react-native: codegen SRMaskView, a C++ display: contents ShadowNode, the recorder-agnostic SRMaskingRegistry seam, and instrumented canary tests.
  • No public API changes — SRMaskView is internal plumbing only; the Paper path (AMPMaskComponentView) is untouched.
  • Part of SDKRN-29 / SDKRN-32. Blocks SDKRN-33 (<AmpMask> public API).

Implementation notes

  • Zero-box layout via adopt(): display:contents + traits_.unset(ForceFlattenView) is enforced in SRMaskViewComponentDescriptor::adopt(), which runs after updateYogaProps() on every create/clone, rather than a fragile constructor initialize(). This survives prop-driven Yoga style resets.
  • iOS registration: +load registers the view with RCTComponentViewFactory, and +componentDescriptorProvider binds SRMaskViewComponentDescriptor; codegenConfig.ios.componentProvider maps SRMaskView. (Does not rely on auto-gen for npm-installed packages.)
  • Android codegen: patchComponentDescriptorsHeader() replaces the generated descriptor typedef with our custom descriptor, and patchCodegenCMakeLists() compiles cpp/SRMaskViewShadowNode.cpp into the codegen target so the descriptor links into libappmodules.so.
  • Masking lifecycle: masking re-applies on prop changes; clearForView fires on unmount (unmountChildComponentView on iOS, removeView/removeViewAt on Android).

Differences from the original plan (POC)

The ticket body documents the POC plan; the shipped code diverges in a few places. All are hardening with the same external behavior:

Area Plan (ticket) Shipped Why
display:contents enforcement explicit ShadowNode constructors calling initialize() (DIV-1) enforced in SRMaskViewComponentDescriptor::adopt() constructor-only init is clobbered by prop-driven updateYogaProps(); adopt() runs after every create/clone (react-native-screens pattern) and is authoritative
Android codegen patch patchShadowNodesHeader() regex on generated ShadowNodes.h (DIV-3) patchComponentDescriptorsHeader() (swap the descriptor typedef) + patchCodegenCMakeLists() (compile cpp/SRMaskViewShadowNode.cpp into the codegen target) the descriptor typedef is the real binding, not the shadow-node typedef; the cpp must link into libappmodules.so
iOS registration +loadRCTThirdPartyComponentsProvider +loadRCTComponentViewFactory registerComponentViewClass: + +componentDescriptorProvider + codegenConfig.ios.componentProvider factory registration + componentProvider is the reliable binding for npm packages; the generated registerComponentDescriptorsFromCodegen path is effectively dead code
Naming SRMaskViewCustomComponentDescriptor SRMaskViewComponentDescriptor (descriptor) / SRMaskViewContentsShadowNode (node — named distinctly from the codegen typedef to avoid an ODR clash)

Test plan

  • pnpm --filter @amplitude/session-replay-react-native typecheck && build && test (98 tests)
  • Android + iOS new-architecture example builds + launch (no duplicate-symbol/link errors)
  • Manual layout repro (both platforms): native SRMaskView (row 5, no JS display:contents override) matches the no-wrapper baseline (row 1) across sections A/B/C, while plain <View>/AmpMaskView collapse flex:1 and shift absolutely-positioned children
  • Android SRMaskViewTest + iOS SRMaskViewTests assert mask(child) fires for every direct child (run locally)
  • CI (lint, build, test matrix)

CI caveat: the Android instrumented (SRMaskViewTest) and iOS (SRMaskViewTests) suites are not run by any workflow today — ci.yml runs Jest only, and rn-smoke.yml covers analytics-react-native, not this package. They were verified locally this session. There is an open question on SRMaskViewTest.kt about whether to wire a native test job into CI or treat these as local canaries.

Follow-ups

  • SDKRN-33: public <AmpMask> / <AmpUnmask> built on this foundation. Note: today unmask shares AMPMaskComponentView with mask, so it has the same layout-boundary problem whenever the child is layout-dependent — this foundation is what lets the new components avoid that.
  • SDKRN-33 also registers the production SRMaskingPrimitive (SRMaskingRegistry.setPrimitive(...)) — until then the Fabric masking path is inert (no-op) by design.
  • SDKRN-14: plugin package Fabric backport

Add dual-arch Fabric infrastructure for layout-transparent masking:
SRMaskView codegen component, C++ display:contents ShadowNode,
SRMaskingRegistry seam, and Android instrumented canary test.

No public API changes. Blocks SDKRN-33 (AmpMask).
@linear-code

linear-code Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

SDKRN-32

SDKRN-29

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

size-limit report 📦

Path Size
packages/analytics-browser/lib/scripts/amplitude-min.js.gz 58.43 KB (0%)
packages/session-replay-browser/lib/scripts/session-replay-browser-min.js.gz 133.19 KB (0%)
packages/unified/lib/scripts/amplitude-min.umd.js.gz 210.94 KB (0%)
@amplitude/element-selector (gzipped esm) 2.67 KB (0%)

@chungdaniel

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Mask props skip existing children
    • Prop updates now reapply the current masking state to already-mounted children on both Android and iOS, unmasking children when the wrapper is disabled or unmasking.

Create PR

Or push these changes by commenting:

@cursor push 83e5f8f458
Preview (83e5f8f458)
diff --git a/packages/session-replay-react-native/android/src/main/java/com/amplitude/sessionreplayreactnative/fabric/SRMaskView.kt b/packages/session-replay-react-native/android/src/main/java/com/amplitude/sessionreplayreactnative/fabric/SRMaskView.kt
--- a/packages/session-replay-react-native/android/src/main/java/com/amplitude/sessionreplayreactnative/fabric/SRMaskView.kt
+++ b/packages/session-replay-react-native/android/src/main/java/com/amplitude/sessionreplayreactnative/fabric/SRMaskView.kt
@@ -14,9 +14,16 @@
     private set
 
   fun setMaskingProps(enabled: Boolean, unmask: Boolean, maskLevel: String) {
+    val maskingPropsChanged =
+      this.enabled != enabled || this.unmask != unmask || this.maskLevel != maskLevel
+
     this.enabled = enabled
     this.unmask = unmask
     this.maskLevel = maskLevel
+
+    if (maskingPropsChanged) {
+      applyMaskingToChildren()
+    }
   }
 
   override fun addView(child: View, index: Int) {
@@ -29,12 +36,14 @@
     super.removeView(view)
   }
 
-  private fun applyMaskingToChild(child: View) {
-    if (!enabled) {
-      return
+  private fun applyMaskingToChildren() {
+    for (i in 0 until childCount) {
+      applyMaskingToChild(getChildAt(i))
     }
+  }
 
-    if (unmask) {
+  private fun applyMaskingToChild(child: View) {
+    if (!enabled || unmask) {
       SRMaskingRegistry.unmask(child)
       return
     }

diff --git a/packages/session-replay-react-native/ios/fabric/SRMaskView.mm b/packages/session-replay-react-native/ios/fabric/SRMaskView.mm
--- a/packages/session-replay-react-native/ios/fabric/SRMaskView.mm
+++ b/packages/session-replay-react-native/ios/fabric/SRMaskView.mm
@@ -45,14 +45,21 @@
 
 - (void)updateProps:(const Props::Shared &)props oldProps:(const Props::Shared &)oldProps
 {
-  const auto &oldViewProps = *std::static_pointer_cast<const SRMaskViewProps>(_props);
   const auto &newViewProps = *std::static_pointer_cast<const SRMaskViewProps>(props);
+  NSString *newMaskLevel = [NSString stringWithUTF8String:newViewProps.maskLevel.c_str()];
+  BOOL maskingPropsChanged =
+      _enabled != newViewProps.enabled || _unmask != newViewProps.unmask ||
+      ![_maskLevel isEqualToString:newMaskLevel];
 
   _enabled = newViewProps.enabled;
   _unmask = newViewProps.unmask;
-  _maskLevel = [NSString stringWithUTF8String:newViewProps.maskLevel.c_str()];
+  _maskLevel = newMaskLevel;
 
   [super updateProps:props oldProps:oldProps];
+
+  if (maskingPropsChanged) {
+    [self applyMaskingToChildren];
+  }
 }
 
 - (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
@@ -67,13 +74,16 @@
   [super unmountChildComponentView:childComponentView index:index];
 }
 
-- (void)applyMaskingToChild:(UIView *)childView
+- (void)applyMaskingToChildren
 {
-  if (!_enabled) {
-    return;
+  for (UIView *childView in self.subviews) {
+    [self applyMaskingToChild:childView];
   }
+}
 
-  if (_unmask) {
+- (void)applyMaskingToChild:(UIView *)childView
+{
+  if (!_enabled || _unmask) {
     [SRMaskingRegistry unmaskView:childView];
     return;
   }

You can send follow-ups to the cloud agent here.

… lifecycle

- iOS: custom SRMaskViewCustomComponentDescriptor bound to
  SRMaskViewContentsShadowNode (display:contents); drop duplicate
  SRMaskViewComponentName definition
- Android: remove redundant CMake native lib; keep gradle header patch
- Re-apply masking on prop changes; clearForView on unmount;
  handle removeViewAt on Android
- README: native display:contents, no JS workaround required
Move Yoga display:contents enforcement into
ComponentDescriptor::adopt() so it survives prop-driven
updateYogaProps() on every clone. Register the custom
descriptor on Android via jni + header patch; add iOS +load
registration for npm consumers.
Use Gradle-patched OBJECT codegen CMake with absolute
cpp paths instead of a standalone jni SHARED target.
@chungdaniel

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit bb50600. Configure here.

}

@Test
fun maskChildFiresForEveryDirectChild() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question for reviewers: is this test worth keeping as-is?

It verifies the SRMaskingRegistry.mask(child) dispatch (and the iOS SRMaskViewTests equivalent) via a test-double — the seam SDKRN-33's production adapter will depend on. Two caveats worth weighing:

  1. It does not run in CI (no native/instrumented test job exists today; ci.yml runs Jest only, rn-smoke.yml covers analytics-react-native). So it can't actually guard against regressions — it's a local-only canary right now.
  2. It mocks the riskiest part by calling addView / mountChildComponentView manually. It does not prove that Fabric mounts real display:contents children under SRMaskView at runtime — which is the thing that would actually break masking end-to-end.

Options: (a) keep and add a native test job to CI, (b) keep as a documented local smoke test, or (c) drop until SDKRN-33 adds a true end-to-end test (real RN subtree → recorder receives mask signal). Preferences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant