refactor: migrate Rokt contracts and facade ownership to rokt-kit#700
refactor: migrate Rokt contracts and facade ownership to rokt-kit#700denischilik wants to merge 23 commits into
Conversation
Stop instantiating and storing Rokt inside android-core MParticle, and create it from rokt-kit extensions instead to start decoupling Rokt object ownership from the core SDK.
Relocate the Rokt facade class and its unit tests from android-core to the rokt kit so Rokt-specific API ownership continues shifting out of core while preserving current behavior and test coverage.
Use a single helper in the Rokt facade to resolve the kit API for all operations, making the upcoming decoupling from the legacy roktKitApi chain incremental and safer.
Remove the mParticle RoktEvent wrapper and return native Rokt SDK events directly to simplify the event pipeline and reduce duplicate mapping logic.
Remove the mParticle unload reason wrapper and pass Rokt SDK unload reasons through directly to simplify callback handling and eliminate redundant mapping.
Replace MpRoktEventCallback with the native RoktCallback across the rokt kit and tests to remove callback wrappers and simplify callback delegation.
Replace the local PlacementOptions wrapper with the native Rokt SDK PlacementOptions across the rokt kit and remove the now-redundant conversion layer.
Replace mParticle RoktConfig and CacheConfig wrappers with native Rokt SDK config types and remove the now-unnecessary config conversion layer and tests.
Relocate Rokt, RoktEmbeddedView, RoktLayoutDimensionCallBack, and RoktTest into com.mparticle.kits to keep kit-owned types co-located and simplify package boundaries.
Avoid runtime method lookup for isEnabled by injecting an explicit enablement callback and wiring it to core callbacks with a safe opt-out fallback, preserving behavior under obfuscation.
PR SummaryMedium Risk Overview The Rokt flow now uses native Enablement checks are reworked to rely on a new Reviewed by Cursor Bugbot for commit 4aec17d. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Add isEnabled to KitManager and use it from the Rokt facade so enablement is resolved through a stable kit-layer API without reflection or config manager coupling.
Clean up stale imports in KitManagerImplTest so android-kit-base ktlint test source checks pass in CI.
denischilik
requested review from
BrandonStalnaker,
nickolas-dimitrakas and
thomson-t
Replace replaceFirstChar in buildSrc for older Kotlin compatibility and address rokt ktlint violations by suppressing Java-style accessor naming and wrapping long warning messages.
| * | ||
| * @param wrapperSdkVersion the version of the mParticle SDK | ||
| */ | ||
| void setWrapperSdkVersion(@NonNull WrapperSdkVersion wrapperSdkVersion); |
There was a problem hiding this comment.
If this is used by just Rokt SDK, do we need to keep it?
| */ | ||
| object MParticleRokt { | ||
| @Suppress("FunctionName") | ||
| @JvmStatic |
There was a problem hiding this comment.
Do we need JvmStatic here? Only consumer is Kotlin, isn't it?
|
|
||
| class Rokt internal constructor(private val mKitManager: KitManager) { | ||
| @JvmOverloads | ||
| fun selectPlacements( |
There was a problem hiding this comment.
Since this is a public api, it would be nice to have some javadoc documentation.
Keep only the no-arg public Rokt accessor with explicit start precondition, restore original public method documentation text, and make prepareAttributesAsync internal per review feedback.
| if (mConfigManager.isEnabled) { | ||
| mKitManager.roktKitApi?.close() | ||
| if (isEnabled()) { | ||
| resolveRoktKit()?.second?.close() |
There was a problem hiding this comment.
Removed try-catch error handling around Rokt SDK calls
Medium Severity
The old RoktKitApiImpl wrapped every delegate call (selectPlacements, events, purchaseFinalized, close, setSessionId, getSessionId, prepareAttributesAsync) in try-catch blocks that logged warnings on failure. The new Rokt facade and RoktKitRequestHelper removed all of these safety wrappers. Unhandled exceptions from the Rokt SDK will now propagate directly to the calling app code, potentially causing crashes where the app previously remained stable.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f0870f7. Configure here.
Update MParticleRokt package to com.mparticle.kits so declaration matches the file location and resolves package/path review feedback.
Keep a synchronized singleton Rokt instance in MParticleRokt to avoid repeated allocations while preserving the explicit start precondition.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ 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 4aec17d. Configure here.
| rokt = it | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Singleton caches stale Rokt after MParticle restart
High Severity
The MParticleRokt singleton caches the Rokt instance forever in a static field that is never cleared. Previously, Rokt was a field on the MParticle instance and was naturally recreated on restart/workspace switch. Now, after MParticle.setInstance(null) or switchWorkspace, the cached Rokt still holds a reference to the old, defunct KitManager. Subsequent calls to MParticleRokt.Rokt() return this stale instance because rokt?.let { return it } short-circuits before checking whether MParticle was re-initialized.
Reviewed by Cursor Bugbot for commit 4aec17d. Configure here.
2 participants
Background
This change completes the Rokt ownership migration to the
rokt-kitmodule so Rokt-specific contracts no longer leak throughandroid-coreandandroid-kit-base.It also removes fragile reflection in the Rokt enablement check to improve runtime reliability.
What Has Changed
kits/rokt/roktcom.rokt.roktsdktypes across the Rokt kit flowRoktKitApibridging from core kit manager contractsisEnabledresolution to use a callback provider instead of reflection and updated unit tests accordinglyScreenshots/Video
N/A
Checklist
Additional Notes
This PR is intentionally scoped to internal ownership/type migration for Rokt integration and related reliability fixes.