SDK-6066 [Android][ND] Phase 11: Kotlin-first ND classes + factory named args - #1069
SDK-6066 [Android][ND] Phase 11: Kotlin-first ND classes + factory named args#1069CTLalit wants to merge 1 commit into
Conversation
…med args Kotlin-first cleanup of the ND fcap code (done on top of the stack, incremental). - Convert NdFCManager.java -> NdFCManager.kt (idiomatic; @JvmStatic isFcapManaged and shownTodayCount as a val for the Java/Kotlin callers). - Convert NdFcapGate.java -> NdFcapGate.kt (internal object). - Convert DisplayUnitResponse.java -> DisplayUnitResponse.kt (internal; nullable ND deps + content-only secondary ctor for the send-test path). - ControllerManager: import NativeDisplayController instead of FQ name. - CleverTapFactory: named arguments on every Kotlin constructor call. NOTE: Java constructors are left positional on purpose — this module does not enable javaParameters, so Kotlin named args don't compile against Java ctors. - New code uses imports (no fully-qualified class names). - Tests: add EvalRulesTest, HeaderVoteListsTest, NativeDisplayControllerTest; extend EventQueueManagerTest to assert the ND controller fan-out. Existing NdFCManagerTest / NdFcapGateTest / DisplayUnitResponseTest pass unchanged (behavior-preserving conversions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Code Review
Summary
Phase 11 converts NdFCManager, NdFcapGate and DisplayUnitResponse from Java to Kotlin, adds named arguments to the Kotlin constructions in CleverTapFactory, and adds EvalRulesTest / HeaderVoteListsTest / NativeDisplayControllerTest. I diffed each converted file line-by-line against its deleted Java original: control flow, cap ordering, catch scopes, pref key derivation and the parseDisplayUnits cache-write-once path are all faithfully preserved, all named arguments resolve against real Kotlin parameter names, and every remaining Java caller (AnalyticsManager, CleverTapAPI, LoginController, ControllerManager) still links because Kotlin internal is public in bytecode.
📊 Visual Overview
flowchart LR
subgraph Converted["Java → Kotlin (this PR)"]
A["NdFCManager.kt<br/>@JvmStatic isFcapManaged<br/>shownTodayCount val<br/>init → initDailyState"]
B["NdFcapGate.kt<br/>internal object<br/>returns List"]
C["DisplayUnitResponse.kt<br/>internal + content-only ctor"]
end
F["CleverTapFactory.kt<br/>named args"] --> A
F --> C
C --> B --> A
JavaCallers["AnalyticsManager.java<br/>CleverTapAPI.java"] -.-> A
JavaCallers -.-> C
T["New tests:<br/>EvalRules · HeaderVoteLists<br/>NativeDisplayController"] -.-> Converted
Verdict
APPROVE
The conversions are behavior-preserving apart from one benign, self-healing capture-semantics change in NdFCManager's async init; the remaining notes are readability/doc hygiene.
Automatic reviews run once per commit — the same commit isn't re-reviewed. Push a new commit to trigger a fresh review.
Reviewed by Vision AI
|
|
||
| init { | ||
| executors.postAsyncSafelyTask<Unit>().execute("initNdFCManager") { | ||
| initDailyState(deviceId) |
There was a problem hiding this comment.
💡 SUGGESTION — Bug: This is the one place the conversion is not behavior-preserving. In the deleted Java the async task captured the constructor parameter deviceId (an effectively-final local), so the deferred init(...) always ran for the device id the manager was built with. Here deviceId inside the lambda resolves to the mutable property this.deviceId and is read when the task actually executes — if changeUser(newId) (LoginController.java:177) lands before the queued task runs, initDailyState runs twice for the new id and the original user's daily-counter reset is skipped for that day.
Impact is low (counters are keyed per device id and the reset self-heals on the next changeUser back to that user), but it's an unintended delta in a PR advertised as behavior-preserving. Pin the value:
| initDailyState(deviceId) | |
| init { | |
| val initialDeviceId = deviceId | |
| executors.postAsyncSafelyTask<Unit>().execute("initNdFCManager") { | |
| initDailyState(initialDeviceId) | |
| } | |
| } |
| */ | ||
| internal object NdFcapGate { | ||
|
|
||
| private const val UNCAPPED = -1 |
There was a problem hiding this comment.
💡 SUGGESTION — Code quality: UNCAPPED is the right name for the tlc/tdc defaults on lines 40-41, but it's also used as the optInt default for efc and excludeGlobalFCaps (lines 35-36), which are presence/boolean flags — -1 there means "absent", not "uncapped". It also silently duplicates NdFCManager.UNCAPPED, which this PR made private, so the two can now drift. Either add a separate ABSENT = -1 for the flag lookups, or make NdFCManager.UNCAPPED internal and reuse it for the two cap defaults.
| * The ND dependencies are nullable: the send-test / push-preview path (see the content-only secondary | ||
| * constructor) carries a single display unit and no ND meta, so meta ingestion is skipped there. | ||
| */ | ||
| internal class DisplayUnitResponse( |
There was a problem hiding this comment.
💡 SUGGESTION — Code quality: The epic's living design doc still points at the pre-conversion filenames — docs/NativeDisplayFrequencyCaps.md:52 and :420 reference response/DisplayUnitResponse.java, and :143 references NdFCManager.getNdCounts/init (now initDailyState). Worth updating in this PR since it's the file-map readers use to navigate the ND channel.
Part of epic SDK-6055 · ticket SDK-6066. Stacked on #1068. Kotlin-first cleanup of the ND fcap code (done on top of the stack, incremental).
Kotlin conversions (idiomatic, behavior-preserving)
NdFCManager.java→NdFCManager.kt—@JvmStatic isFcapManaged(JavaAnalyticsManagercalls it),shownTodayCountas aval,when/runCatching/string-templates; renamed the privateinit(...)→initDailyState(...)to avoid clashing with the Kotlininit {}block.NdFcapGate.java→NdFcapGate.kt(internal object).DisplayUnitResponse.java→DisplayUnitResponse.kt(internal; nullable ND deps + content-only secondary constructor for the send-test/preview path).Idiom fixes
ControllerManager:importforNativeDisplayControllerinstead of the fully-qualified name.CleverTapFactory: named arguments on every Kotlin constructor call.Named args — Java-constructor limitation
This module does not enable
javaParameters, and Kotlin named arguments don't compile against Java constructors without it (every pre-existing named-arg call in the factory already targets a Kotlin class). So Java-defined constructors are intentionally left positional; named args were applied to all Kotlin constructions. EnablingjavaParametersmodule-wide to cover the rest is a separate call.Tests
EvalRulesTest,HeaderVoteListsTest,NativeDisplayControllerTest.EventQueueManagerTestto assert the ND-controller fan-out.NdFCManagerTest/NdFcapGateTest/DisplayUnitResponseTest(proving the conversions preserve behavior) +EvaluationManagerTest,QueueHeaderBuilderTest,InAppControllerTest.Coverage matrix — every touched class
🤖 Generated with Claude Code