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 20 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 core/kit-base bridge API is deleted and invocation is rerouted. Enablement plumbing is made explicit. Reviewed by Cursor Bugbot for commit df42198. 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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 73c65a8. Configure here.
| @@ -0,0 +1,25 @@ | |||
| package com.mparticle.rokt | |||
There was a problem hiding this comment.
File package doesn't match directory structure
High Severity
MParticleRoktExtensions.kt declares package com.mparticle.rokt but is physically located in the com/mparticle/kits/ directory. There is no com/mparticle/rokt/ directory under the Kotlin source root in this module. This mismatch between the declared package and the file's directory path will likely cause build failures or the file being invisible to the compiler, since Android/Gradle projects conventionally require the source file path to match the package declaration.
Reviewed by Cursor Bugbot for commit 73c65a8. Configure here.
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?
| * Java-friendly accessors for the legacy Rokt API object. | ||
| */ | ||
| object MParticleRokt { | ||
| @Suppress("FunctionName") |
There was a problem hiding this comment.
This is probably confusing. Can we keep the function name start with small letters?
| */ | ||
| object MParticleRokt { | ||
| @Suppress("FunctionName") | ||
| @JvmStatic |
There was a problem hiding this comment.
Do we need JvmStatic here? Only consumer is Kotlin, isn't it?
| object MParticleRokt { | ||
| @Suppress("FunctionName") | ||
| @JvmStatic | ||
| fun Rokt(mParticle: MParticle?): Rokt? = mParticle?.let { createRokt(it) } |
There was a problem hiding this comment.
Why do we need this nullable?
|
|
||
| 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.
| null | ||
| } | ||
|
|
||
| fun prepareAttributesAsync(attributes: Map<String, String>) { |
There was a problem hiding this comment.
We don't want this to be public.
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.