Skip to content

feat(rokt): resolve embedded placeholders by placeholderName - #410

Open
thomson-t wants to merge 7 commits into
mainfrom
thomson-t/fix-findnodehandle
Open

thomson-t wants to merge 7 commits into
mainfrom
thomson-t/fix-findnodehandle

Conversation

@thomson-t

@thomson-t thomson-t commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Why

Apps that show a Rokt offer inside their own screen have to hold a reference to the placeholder view and convert it to an internal React Native number before asking for the offer. They also have to time the request: if they ask before the view has finished appearing, the offer silently never shows, so apps add delays or wait for layout callbacks that do not actually guarantee the view exists. The conversion also relies on React Native interfaces that are being retired. Once this lands, an app asks for the offer by the placeholder's name, which it already gives the view, whenever it likes, including in the same step that draws the screen. Existing apps keep working without changes.

Programme

Standalone change, no programme. It comes from an engineer's request to move off findNodeHandle for embedded placements. The plan record is internal and cannot be linked here.

What changes

Before: selectPlacements(id, attributes, { Location1: findNodeHandle(ref.current) }), with a ref on every RoktLayoutView, called only after the native view exists.

After: selectPlacements(id, attributes, ['Location1']), where 'Location1' is the view's existing placeholderName. It can be called at any time, for example from the useEffect that renders the view. The old map form is still accepted, and positive tags resolve exactly as before. A null in the old map, which findNodeHandle returns before the view mounts, is now resolved by name instead of being skipped.

Each native layout view registers itself under its placeholderName when mounted and unregisters when removed or recycled. When selectPlacements names a placeholder that has not mounted yet, the native module waits for it to register, for at most 2 seconds, then calls the Rokt SDK with the views it has. A newer call with the same identifier replaces a pending one, and close() cancels pending waits. Legacy tags are never waited for.

This PR also fixes an Android Lint error from the first commit: HashMap.forEach needs API 24 and minSdk is 21.

The README, the sample app and the Expo test app are updated to the new form. The changelog is generated by the release draft workflow, so it is not edited here.

Start reading at toNativePlaceholders in js/rokt/rokt.ts, then selectPlacements and resolvePlaceholders: in ios/RNMParticle/RNMPRokt.mm and whenPlaceholdersMounted in MPRoktModuleImpl.kt, then the two RoktPlaceholderRegistry files. Left alone on purpose: removing the tag path and the codegen spec shape, both planned for the next major version; an onLayout or ready prop on RoktLayoutView, which the wait makes unnecessary.

Linked work

Related: the direct Rokt React Native SDK (ROKT/rokt-sdk-react-native) uses the same tag-based placeholder map. A matching change there is not opened.

Rollout

Path: merging publishes nothing. The change reaches apps in the next release of this package, which the release draft workflow prepares, and takes effect when an app upgrades and rebuilds. It is live in that release, and existing call sites need no change.
Feature flags: none.
Turning it off: a published version cannot be recalled. Reverting this pull request and releasing again restores the old behaviour in the next version. Apps that adopted the name form would then need to go back to the map form.
What we watch: there is no production dashboard for a client library. After the release, we watch this repository's issues for embedded placements that stop appearing, or appear late, on either platform. The log line "Cannot resolve placeholder" names any placeholder that never mounted.

Risks

  • Apps that still pass React tags could regress; prevented because a positive tag is resolved by the same lookup as before, is never waited for, and the name lookup runs only when the tag finds nothing; a regression would show as embedded placements missing after upgrade.
  • iOS New Architecture drops null values from the map before native code sees them (found while testing this change); prevented by sending 0 instead of null, with a JavaScript test asserting no null reaches native; we would see an empty placeholder dictionary and a "Cannot resolve placeholder" log.
  • A misspelled or never-rendered placeholder name delays the Rokt request by 2 seconds before it proceeds without that view; contained by the fixed timeout, after which the call behaves as it did before; we would see offers arriving 2 seconds late and the "Cannot resolve placeholder" log (on iOS debug builds this is a red box, as unresolved placeholders already were).
  • The Rokt SDK running inside a React Native mount pass, where it would change the view hierarchy mid-mount; prevented because a satisfied wait posts the call to the next main-thread turn, with tests on both platforms asserting it does not run inside registration; we would see layout glitches or a crash when an offer loads.
  • Two calls, or a call and close(), racing a pending wait; contained because a newer call with the same identifier replaces the pending one and close() drops all waits, each covered by tests; we would see an offer shown twice or after it was closed.
  • Two screens in a navigation stack both mount the same placeholder name; contained because the newest view that is on screen wins, and the older one stays resolvable after the top screen is removed, with tests on each platform; we would see the offer land in the wrong screen.
  • A recycled iOS view keeps a stale registration; prevented because prepareForRecycle unregisters and the registry holds weak references; we would see an offer drawn into a view that now belongs to another placeholder.
  • On the iOS legacy architecture, a removed view that is still retained (the Rokt SDK holds placeholder views strongly while a request is in flight) stays registered; prevented because a category adds RCTInvalidating to RoktEmbeddedView, and RCTUIManager calls -invalidate on every view it removes, which unregisters it (a subclass is not possible, because the class is closed to Objective-C subclassing); we would see a new call skip the wait and embed into the removed screen.
  • The registry is shared across every React Native host in the app; not addressed, because apps that mount the same placeholder name in two hosts at once are rare and the fix means keying by surface; we would see the offer in the other host's view.
  • The call shape now differs from the direct Rokt React Native SDK; not addressed here; developers using both would see two forms.

Risk class: low.

Who

Written by: an automated coding agent (Claude Code), at an engineer's request, following a re-evaluated migration plan; the plan record is internal.
Code reviewed before opening: the requesting engineer reviewed the first commit, found that iOS New Architecture dropped null map values, and fixed it by sending 0; an independent review agent reviewed each commit before it was made; it blocked the third commit until the old-architecture scope was shown, and passed all three. After opening, 3b26a18 addresses the Copilot finding on dropped legacy iOS views and hardens close() ordering in response to Cursor Bugbot.
Design reviewed before opening: the requesting engineer chose the name-array form over implicit attachment of every mounted view, and chose an SDK-side wait with a 2-second timeout over an onLayout or ready prop, after a survey of how other React Native SDKs bind views to requests.
Decision this implements: the requesting engineer's approval of the migration plan on 2026-09-24 and of the wait design on 2026-09-25; the records are internal and cannot be linked.
Checked: on 2026-09-25, yarn test (23 tests and lint); Android ./gradlew lint ktlintCheck test (registry tests 10/10, module tests 8/8); iOS RNMPRoktPlaceholderTests 15/15 on an iOS 26.5 simulator. The sample app (React Native 0.84, New Architecture, production test account) was run with a throwaway screen, not committed, on an iOS 26.5 simulator and an Android emulator. On both, four cases behaved as designed: the view mounting 1 second after the call (the request started only after the mount and the offer rendered); the call in the same useEffect that renders the view (rendered); a view that never mounts (waited about 2 seconds, then proceeded without it); and the legacy findNodeHandle tag (rendered).
Not checked: Old Architecture on either platform at runtime (Android's old-architecture module is compiled and unit-tested; the iOS legacy view's -invalidate unregister is unit-tested by calling it directly, and RCTUIManager calling it was confirmed from React Native 0.76, 0.79 and 0.81 source); running the Expo test app (its source was updated to the new call form but not run); physical devices.

Size

Hand-written: 1000 lines added and 138 removed in 23 files, over three commits (379 of the added lines are tests).
Generated: none.
Why one pull request: the JavaScript name form does nothing without both native sides, and the wait is what makes the name form safe to call from useEffect; any split would ship an API that silently resolves nothing on one platform, or one that still needs partners to time the call.

Notes for reviewers

Why wait, rather than an onLayout prop. The native Rokt kits need concrete embedded views when selectPlacements runs (MPKitRokt confirmEmbeddedViews:, RoktKit placeHolders → WeakReference<Widget>). React Native Fabric does not guarantee the view exists when JavaScript thinks it does: on Android, commits from the JS thread queue mount items that are applied on the next Choreographer frame (FabricUIManager.scheduleMountItem → DispatchUIFrameCallback), which a runOnUiThread from a module call can beat; on iOS the mount is dispatched to the main queue and a module call from an effect in the same JS task can be queued ahead of it; and Fabric emits onLayout in the commit before scheduling the mount (ShadowTree.cpp). The only reliable signal is the native view registering itself, so the wait keys off that.

Why the sentinel is 0, not null. React Native's convertJSIObjectToNSDictionary (ReactCommon/.../RCTTurboModule.mm) inserts a converted value only if (v), so a JavaScript null is dropped and { Location1: null } reached selectPlacements as an empty dictionary. React tags are always positive, so 0 is unambiguous. The codegen spec type narrows from number | null to number; the generated native types are unchanged.

Resolution rule, both platforms. Value > 0 → resolve as a React tag, never waited for. Otherwise, if no view is registered under the key, wait up to 2 s for one. Then resolve by tag, falling back to RoktPlaceholderRegistry by key; still nothing → log and skip.

Registry. Stores name → ordered weak references, main/UI thread only. register removes the view from every name first. lookup returns the newest view in a window, else the newest alive. Waits are keyed by placement identifier; completion is posted, never run inline. The Android registry is typed View because the Rokt kit is compile-only; its scheduler is injectable for unit tests.

Commits. 688ad2c name-based resolution; 0e33d5e the mount wait and the Android Lint fix; 3b26a18 review follow-ups (legacy iOS unregister, close() ordering).

🤖 Generated with Claude Code

selectPlacements now accepts the placeholderName of each mounted
RoktLayoutView, e.g. ['Location1'], so apps no longer need a ref and
findNodeHandle to embed a placement. The legacy map of name to react
tag still works: positive tags resolve exactly as before. A null value
(findNodeHandle called before mount), which was previously skipped, now
resolves by name.

Each native layout view registers itself by placeholderName in a new
RoktPlaceholderRegistry on iOS and Android. The JS wrapper sends the
name form as { name: 0 }: a positive value is still resolved as a react
tag, and zero, or a tag that no longer resolves, is looked up by name.
The sentinel is numeric because the iOS TurboModule conversion drops
null-valued map entries.

Also delivers placeholderName to native where it was never read: the
Android old-architecture view manager lacked @ReactProp, and neither
iOS view consumed the prop.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@thomson-t
thomson-t marked this pull request as ready for review September 25, 2026 17:08
@thomson-t
thomson-t requested a review from a team as a code owner September 25, 2026 17:08
Copilot AI lite review requested due to automatic review settings September 25, 2026 17:08
@cursor

cursor Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Changes are low risk and backward-compatible, adding non-breaking name resolution and mount timeouts for Rokt embedded views while preserving legacy React tag lookup paths.

Overview
Adds name-based placeholder resolution for embedded Rokt placements, allowing MParticle.Rokt.selectPlacements to accept an array of placeholderName strings (e.g. ['Location1']) instead of requiring findNodeHandle React tags.

Native modules on iOS and Android now track mounted RoktLayoutView instances via a RoktPlaceholderRegistry. If selectPlacements is called before the corresponding view finishes mounting (such as inside a useEffect), native resolution waits up to two seconds for the view to register before proceeding. Legacy React tag maps remain backward-compatible, and the sample and Expo test apps have been updated to use the new name-based API.

Reviewed by Cursor Bugbot for commit 7c42124. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Three moderate unresolved findings affect lifecycle cleanup and cross-host placeholder selection.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds name-based embedded Rokt placeholder resolution while preserving legacy React tag maps.

Changes:

  • Converts placeholder-name arrays to native sentinel maps.
  • Adds iOS and Android placeholder registries with lifecycle handling.
  • Updates native modules, documentation, samples, and tests.
File Summary
sample/​ios/​MParticleSampleTests/​RNMPRoktPlaceholderTests.m Adds iOS registry tests.
sample/​index.js Uses placeholder-name arrays.
README.md Documents the new API.
js/​rokt/​rokt.ts Converts placeholder inputs.
js/​index.tsx Exports placeholder types.
js/​codegenSpecs/​rokt/​NativeMPRokt.ts Updates native typing.
js/​__tests__/​rokt-placeholders.test.ts Tests conversion behavior.
ios/​RNMParticle/​RoktPlaceholderRegistry.m Implements iOS registry; moderate cross-host lookup concern (1 vote).
ios/​RNMParticle/​RoktPlaceholderRegistry.h Declares the iOS registry.
ios/​RNMParticle/​RoktNativeLayoutComponentView.mm Registers Fabric views.
ios/​RNMParticle/​RoktLayoutManager.m Registers legacy views; moderate missing unregister cleanup (2 votes).
ios/​RNMParticle/​RNMPRokt.mm Resolves tags and names.
ios/​RNMParticle.xcodeproj/​project.pbxproj Includes registry sources.
ExpoTestApp/​App.tsx Updates Expo usage.
android/​src/​test/​java/​com/​mparticle/​react/​rokt/​RoktPlaceholderRegistryTest.kt Tests Android registry behavior.
android/​src/​oldarch/​java/​com/​mparticle/​react/​rokt/​RoktLayoutViewManager.kt Adds legacy lifecycle handling.
android/​src/​oldarch/​java/​com/​mparticle/​react/​rokt/​MPRoktModule.kt Adds legacy resolution fallback.
android/​src/​newarch/​java/​com/​mparticle/​react/​rokt/​RoktLayoutViewManager.kt Adds Fabric lifecycle handling.
android/​src/​newarch/​java/​com/​mparticle/​react/​rokt/​MPRoktModule.kt Adds new-architecture resolution fallback.
android/​src/​main/​java/​com/​mparticle/​react/​rokt/​RoktPlaceholderRegistry.kt Implements Android registry; moderate cross-host lookup concern (1 vote).
android/​src/​main/​java/​com/​mparticle/​react/​rokt/​RoktLayoutViewManagerImpl.kt Manages Android registration lifecycle.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +20 to +23
RCT_CUSTOM_VIEW_PROPERTY(placeholderName, NSString, RoktEmbeddedView)
{
[RoktPlaceholderRegistry registerView:view name:json ? [RCTConvert NSString:json] : nil];
}

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.

Agreed that a dropped legacy view should not stay resolvable. Since the wait added in 0e33d5e, it would also make selectPlacements skip the wait and embed into the removed view.

onDropViewInstance: is Android-only; iOS RCTViewManager has no drop hook. Subclassing RoktEmbeddedView is also out: it is a Swift public (not open) class, and its generated header marks it objc_subclassing_restricted.

Fixed in 3b26a18 with a category that adds RCTInvalidating to RoktEmbeddedView and unregisters in -invalidate. The legacy RCTUIManager calls that on every view it purges. I checked _purgeChildren:fromRegistry: at v0.76.0, v0.79.0 and v0.81.0. This manager is only mounted when Fabric is off; Fabric views already unregister in prepareForRecycle.

On who retains the view: in the Rokt iOS SDK, the placements dictionary is strong only while an execute is in flight, and the UX layer holds the view weakly. So the stale window is narrow, but it is real.

Covered by testLegacyEmbeddedViewUnregistersWhenInvalidated. Not run on a legacy-architecture app at runtime.

selectPlacements can now be called before its RoktLayoutView has
mounted, for example from the same useEffect that renders it. The
native Rokt kits need the embedded view at call time, but Fabric
creates views on a later frame on Android and in a later main-queue
block on iOS, and emits onLayout before mounting, so neither
useEffect nor onLayout guarantees the view exists.

For each placeholder named for name-based resolution that has no
mounted view yet, the native module waits on RoktPlaceholderRegistry
until the view registers, or for at most 2 seconds, then calls the
SDK with the views it has. The call is posted after registration so
the SDK never runs inside a mount pass. A newer call with the same
identifier replaces a pending one, and close() cancels pending waits.
Legacy react tags are never waited for.

Also replaces HashMap.forEach in the old-architecture Android module,
which needs API 24 (minSdk is 21) and failed Android Lint.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@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 using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0e33d5e. Configure here.

Comment thread ios/RNMParticle/RNMPRokt.mm
On the legacy architecture, RoktLayoutViewManager registered each
RoktEmbeddedView by placeholderName but only lost the entry when the
view deallocated. A dropped view that was still retained, for example
by an in-flight Rokt request, stayed resolvable by name, so
selectPlacements could skip the mount wait and embed into it.

iOS RCTViewManager has no drop callback and RoktEmbeddedView cannot be
subclassed, so a category adds RCTInvalidating: RCTUIManager calls
-invalidate on every view it removes, and the view unregisters there.

Also runs close and resolve in the same main-queue block as
cancelAllWaits on iOS, matching Android. The module's method queue is
already the main queue, so the order is unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread android/src/main/java/com/mparticle/react/rokt/RoktPlaceholderRegistry.kt Outdated
// methodQueue is the main queue, so this runs inline; kept as one block so pending waits are
// always cancelled before close even if that changes. Matches Android's MPRoktModuleImpl.close.
RCTExecuteOnMainQueue(^{
[RoktPlaceholderRegistry cancelAllWaits];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cancelling a pending wait discards the select block, so the Rokt SDK is never called and no event is emitted for that view name. The JS promise has already resolved, so the app can't tell "still loading" from "will never arrive" — it just waits on a callback that can't come.

The README now recommends calling selectPlacements from useEffect, which makes the triggering pattern more likely, not less: a screen that mounts, calls, and unmounts inside the 2s window with close() in its cleanup. Async APIs that can finish without invoking the caller's handler are a recurring source of partner reports.

Could we either run the completion on cancel and let the SDK report the failure, or emit a placement-failure event for the view name?

This also occurs in android/src/main/java/com/mparticle/react/rokt/MPRoktModuleImpl.kt line 70

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.

Agreed, a pending call should never end silently. It will now emit PlacementFailure event to notify the partner app.

[RoktPlaceholderWaits() removeObjectForKey:key];
// Asynchronously: registration happens mid-mount, and the Rokt SDK mutates the placeholder
// view hierarchy, so it must not run inside the mount transaction.
dispatch_async(dispatch_get_main_queue(), wait.completion);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If wait is nil this passes a NULL block to dispatch_async, which traps. I checked all three callers and none can reach it today, but the Android twin guards with waits.remove(key) ?: return at RoktPlaceholderRegistry.kt:221. Worth making symmetric since it's a cheap guard on a path that's easy to add a fourth caller to.

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.

Added the wait == nil early return in 2d162b8, matching Android’s waits.remove(key) ?: return

Comment thread ios/RNMParticle/RNMPRokt.mm Outdated
return;
}
_rokt_log(@"[mParticle-Rokt] waiting up to %.0fs for placeholder(s) to mount: %@", kRoktPlaceholderMountTimeout, pending);
[RoktPlaceholderRegistry waitForNames:pending key:identifer timeout:kRoktPlaceholderMountTimeout completion:select];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keying the wait on the page identifier means two surfaces sharing one identifier — two tabs, or the same screen pushed twice — will have the newer call silently drop the older one's request, via the same path as the close() comment above. Embedded integrations commonly reuse a single page identifier across a stack, so this seems more reachable than the description implies. Keying on identifier plus the name set would avoid it, though fixing the dropped-callback issue would also make it survivable.

same at MPRoktModuleImpl.kt line 94

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.

The silent drop was the real problem, and it’s fixed by the same change as the close() thread: a replaced wait now emits PlacementFailure for its identifier.
We shouldn't handle same page pushed twice scenario. It is considered as an integration issue and failing the call is the right approach IMO.

// conformance is added as a category; the trade-off is that another -invalidate category on this
// class would collide silently. Without it a dropped view that something still retains stays
// registered and resolvable by name.
@interface RoktEmbeddedView (RNMPPlaceholderRegistration) <RCTInvalidating>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the collision risk you noted maybe try wrapping the category in #ifndef RCT_NEW_ARCH_ENABLED It costs nothing and limits it to the builds that need it.

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.

Done in 2d162b8.

thomson-t and others added 4 commits September 25, 2026 16:23
A selectPlacements call waiting for its placeholder to mount was
dropped silently when close() cancelled it or a newer call with the
same identifier replaced it: the Rokt SDK was never called, no event
was emitted, and the JS promise had already resolved, so the app
could wait forever on a callback that never came.

Registry waits now take a discard callback that runs on the main/UI
thread when a wait is replaced or cancelled, and both modules use it
to emit PlacementFailure for the call's identifier, the same event the
Rokt SDK emits when it rejects an overlapping call.

Also, from review:
- guard the iOS wait completion against a missing wait, as Android does
- compile the legacy iOS RCTInvalidating category only into builds
  without RCT_NEW_ARCH_ENABLED, the only ones that mount that view
- drop a tooling prefix from two registry comments

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sample app:
- replace the Objective-C AppDelegate and main.m with AppDelegate.swift
  and a SceneDelegate, adding the UIApplicationSceneManifest
- read the mParticle key and secret from the MPARTICLE_KEY and
  MPARTICLE_SECRET scheme environment variables instead of source, and
  document that in the sample README
- find the root view controller through connected scenes in the render
  test, which now looks for the sample's own title
- bump @react-native-community/cli-platform-ios to ^20.2.0

Expo test app:
- move to Expo 57 and React Native 0.86, with expo-splash-screen
- set the iOS deployment target to 16.4 with scene support

API keys stay as placeholders.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The sample's Objective-C AppDelegate.mm was replaced by
AppDelegate.swift, so the React Native 0.77+ dependency provider note
now links to the Swift file and shows its setup lines.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
RCTAppDependencyProvider needs `import ReactAppDependencyProvider`,
which the Objective-C snippet carried and the Swift one had dropped.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

3 participants