Conversation
Updating develop.
…DisplayUnitCache API (#1012) * SDK-5763: [Android Core SDK] Add DisplayUnitCache interface + setter on CleverTapAPI (#999) * feat(SDK-5763): add DisplayUnitCache interface + setter on CleverTapAPI Extracts the public surface of CTDisplayUnitController into a new DisplayUnitCache interface and exposes CleverTapAPI.setDisplayUnitCache() so external SDKs (e.g. Native Display) can install their own cache implementation. Default behaviour preserved when the setter is not called — the SDK lazily installs CTDisplayUnitController on first adUnit_notifs response. - displayunits/DisplayUnitCache.java: new public interface - displayunits/CTDisplayUnitController.java: implements DisplayUnitCache - ControllerManager.java: holds DisplayUnitCache reference; new getDisplayUnitCache()/setDisplayUnitCache(); deprecated old getters - CleverTapAPI.java: setDisplayUnitCache() public; lookup methods route via interface - AnalyticsManager, DisplayUnitResponse, LoginController: route via getDisplayUnitCache() - AnalyticsManagerTest: mock points updated to displayUnitCache * SDK-5763: add pushDisplayUnitElementClickedEventForID(unitID, elementID, additionalProperties) Adds a dedicated Core SDK method for tracking element-level clicks within a Native Display unit, separate from the existing unit-level pushDisplayUnitClickedEventForID. The new method: - Carries the campaign's wzrk_* fields from the cached unit JSON (same enrichment as the unit-level method). - Adds wzrk_element_id = elementID to evtData from the dedicated parameter. - Merges additionalProperties into evtData after wzrk_* enrichment. Keys starting with wzrk_ are stripped defensively — the prefix is reserved for server-controlled attribution. The existing pushDisplayUnitClickedEventForID(unitId) stays untouched for unit-level clicks (graceful degradation for older ND SDK clients). Why a new method (vs an overload of the existing one): - Method name encodes the semantic — readers/dashboard query authors don't need tribal knowledge to interpret "click + additionalProperties". - ND SDK PR #32 wires its reflective probe to this new signature; old Core SDK callers continue to call the legacy single-arg method unchanged. No silent attribution semantics drift. Tests in AnalyticsManagerTest cover: - Happy path: elementID lands as wzrk_element_id; additionalProperties merge as first-class evtData fields. - wzrk_* keys in additionalProperties are filtered out. - coreMetaData.setWzrkParams (which feeds wzrk_ref batch headers) receives only wzrk_* keys — caller-supplied non-wzrk extras don't ride along on subsequent batched events. - displayUnitCache null short-circuit preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * SDK-5763: type updateDisplayUnits with parsed models, hoist parsing to response Refactor the DisplayUnitCache interface so updateDisplayUnits takes List<CleverTapDisplayUnit> instead of JSONArray and is void. The cache contract is now pure-model — JSON parsing lives in DisplayUnitResponse, not in cache implementations. - DisplayUnitCache: parameter is List<CleverTapDisplayUnit>; return removed (caller already has the list). - CTDisplayUnitController: resets and populates the items map from the supplied list, skipping null/empty-id entries. - DisplayUnitResponse: new parseDisplayUnitsFromJson(JSONArray) filters malformed entries (error field) and per-item JSONException, then hands the model list to the cache and notifies the listener. - CTDisplayUnitControllerTest: rewritten against the list-based API. Host-installed DisplayUnitCache implementations no longer need to know about the SDK's JSON shape; they only deal with model objects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * SDK-5763: address review — TOCTOU + cross-thread visibility for display-unit cache - ControllerManager.displayUnitCache is now volatile so writes from setDisplayUnitCache() (any thread, including a public-API caller outside the synchronized init block in DisplayUnitResponse) are safely visible to subsequent reads. - CleverTapAPI.getAllDisplayUnits() and getDisplayUnitForId() now capture the cache reference into a local variable once, then null-check and dereference that local. Closes the TOCTOU window where a concurrent setDisplayUnitCache(null) could let the second getDisplayUnitCache() return null after the guard had passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * SDK-5763: import DisplayUnitCache and use the short name throughout CleverTapAPI Adds the DisplayUnitCache import in CleverTapAPI.java, switches the setDisplayUnitCache(...) parameter to the short name, and replaces the fully-qualified DisplayUnitCache references inside getAllDisplayUnits() and getDisplayUnitForId() (introduced when the import wasn't yet present) with the short name as well. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: fixes test case. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit e3090b2) * SDK-5763: stop filtering wzrk_ keys from additionalProperties; layer cached wzrk_* on top Flip the merge order in pushDisplayUnitElementClickedEventForID so that server-controlled attribution still wins over same-named caller keys (the original security intent) WITHOUT silently dropping novel caller wzrk_* keys that aren't in the cached unit. New order of evtData assembly (later layers win on key collision): 1. additionalProperties, merged verbatim (no wzrk_ filter). 2. wzrk_element_id from the dedicated argument. 3. Cached unit wzrk_* fields layered on top. Result: - A caller-supplied wzrk_id never overwrites the cached wzrk_id (same guarantee as before). - Novel caller wzrk_* keys (e.g. an integration that needs to mark a campaign-relevant field the server didn't ship) now pass through instead of being silently dropped + logged. Tests: - AnalyticsManagerTest: replaced `strips wzrk_ keys from additionalProperties` with `cached wzrk_ wins over caller wzrk_ but novel wzrk_ keys pass through`, covering both halves of the new contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: remove dedicated elementID param — wzrk_element_id flows via additionalProperties * fix: remove dedicated elementID param — wzrk_element_id flows via additionalProperties * fix: remove dedicated elementID param — wzrk_element_id flows via additionalProperties * fix: remove dedicated elementID param — wzrk_element_id flows via additionalProperties * fix: remove dedicated elementID param — wzrk_element_id flows via additionalProperties * SDK-5848: v8.3.0 release prep — bump version and update changelog Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * SDK-5848: Fix PR review findings — TOCTOU cache reads, empty-array response, import conflicts, javadoc - AnalyticsManager: capture getDisplayUnitCache() into local var in all three push methods (clicked/element-clicked/viewed) to close volatile double-read race - AnalyticsManager: add verbose logs on no-cache and no-unit early returns in pushDisplayUnitElementClickedEventForID - DisplayUnitResponse: guard only on null (not empty array) so an empty server payload clears stale cache and fires notifyDisplayUnitsLoaded(null) - AnalyticsManagerTest: remove duplicate kotlin.test.assertEquals/assertFalse imports that conflict with org.junit.Assert equivalents - CleverTapAPI: fix @SInCE 7.x.0 → 8.3.0 on setDisplayUnitCache javadoc - templates/CTCORECHANGELOG.md: fix getDisplayUnitForId casing → getDisplayUnitForID; run copyTemplates to propagate to docs/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: add missing DisplayUnitCache import in AnalyticsManager * fix: address remaining vision review findings - LoginController.resetDisplayUnits: capture getDisplayUnitCache() into local to close volatile double-read (same fix applied in AnalyticsManager) - ControllerManager: fix @deprecated since v7.x.0 → 8.3.0 on both shim methods - CTDisplayUnitController: fix misleading log — reset() already ran, so "cache not updated" is wrong; corrected to "cache cleared" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: remove Behaviour Changes from changelog; add SampleDisplayUnitCache to sample app - templates/CTCORECHANGELOG.md: drop Behaviour Changes section from v8.3.0 entry (wzrk_* layering is an implementation detail of the element-click API, not a standalone behaviour change worth documenting separately); run copyTemplates to propagate to docs/ - MyApplication.kt: install SampleDisplayUnitCache via setDisplayUnitCache() to demonstrate the new public API; adds a thread-safe HashMap-backed implementation as an example for integrators Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ 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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds a pluggable DisplayUnitCache, refactors CTDisplayUnitController to implement it, routes display-unit lookups through ControllerManager/CleverTapAPI (with setDisplayUnitCache), implements pushDisplayUnitElementClickedEventForID that merges caller props and enriches with cached wzrk_* fields, and updates parsing, reset flows, tests, sample, and docs for v8.3.0. ChangesDisplay Unit Cache Abstraction and Event Enrichment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
deeksha-rgb
approved these changes
Jun 4, 2026
![clevertap-vision[bot]](https://avatars.githubusercontent.com/in/648995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java`:
- Around line 92-105: The current lazy-install in DisplayUnitResponse can race
with host code that calls controllerManager.setDisplayUnitCache(...); move the
create-if-null logic into ControllerManager as a single atomic helper (e.g., add
a synchronized getOrCreateDisplayUnitCache() that returns the existing cache or
installs a new CTDisplayUnitController), then in DisplayUnitResponse call that
helper and use the returned DisplayUnitCache instance for
parseDisplayUnitsFromJson -> cache.updateDisplayUnits(...) and for the
callbackManager.notifyDisplayUnitsLoaded(...) decision so you never
create/modify a cache outside ControllerManager.
In `@docs/CTCORECHANGELOG.md`:
- Line 6: Update the changelog to use the exact public API method name to avoid
confusion: replace the incorrect `getDisplayUnitForID()` with the correct
`getDisplayUnitForId(String)` in the sentence describing the Display Unit Cache
API (references: DisplayUnitCache, setDisplayUnitCache(DisplayUnitCache),
CleverTapAPI, CTDisplayUnitController).
In `@templates/CTCORECHANGELOG.md`:
- Line 6: Update the template to use the correct API name and signature: replace
the incorrect getDisplayUnitForID() with getDisplayUnitForId(String) in the
CTCORECHANGELOG.md entry that documents DisplayUnitCache, ensuring the line that
mentions DisplayUnitCache, setDisplayUnitCache(DisplayUnitCache), and
CleverTapAPI uses the exact method signature getDisplayUnitForId(String) so
future changelogs reflect the real API (CTDisplayUnitController,
getAllDisplayUnits(), setDisplayUnitCache()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae99a9e0-30d7-4ab4-8102-eb60693adc80
📒 Files selected for processing (19)
CHANGELOG.mdREADME.mdclevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/BaseAnalyticsManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.javaclevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitController.javaclevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/DisplayUnitCache.javaclevertap-core/src/main/java/com/clevertap/android/sdk/login/LoginController.javaclevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.javaclevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/MockAnalyticsManager.ktclevertap-core/src/test/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitControllerTest.ktdocs/CTCORECHANGELOG.mddocs/CTGEOFENCE.mddocs/CTPUSHTEMPLATES.mdgradle/libs.versions.tomlsample/src/main/java/com/clevertap/demo/MyApplication.kttemplates/CTCORECHANGELOG.md
|
|
||
| #### New Features | ||
| * **Native Display Element Click:** New `pushDisplayUnitElementClickedEventForID(String unitID, HashMap<String, Object> additionalProperties)` on `CleverTapAPI` records a `Notification Clicked` event for a specific element within a Display Unit. Caller-supplied `additionalProperties` (including `wzrk_element_id` from the action's `metadata`) are merged first, then enriched with cached `wzrk_*` attribution fields from the unit — giving finer-grained click analytics for Native Display experiences. | ||
| * **Display Unit Cache API:** New public interface `DisplayUnitCache` and `setDisplayUnitCache(DisplayUnitCache)` on `CleverTapAPI` let external SDKs (e.g. the Native Display SDK) inject a custom display-unit store. `getAllDisplayUnits()` and `getDisplayUnitForID()` now route through this cache. The default implementation (`CTDisplayUnitController`) remains active when no override is installed. |
Contributor
There was a problem hiding this comment.
Use the exact public API method name in release notes.
Line 6 says getDisplayUnitForID(), but the public API is getDisplayUnitForId(String) (lowercase d in Id). This can cause integrators to copy a non-compiling method name from the changelog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/CTCORECHANGELOG.md` at line 6, Update the changelog to use the exact
public API method name to avoid confusion: replace the incorrect
`getDisplayUnitForID()` with the correct `getDisplayUnitForId(String)` in the
sentence describing the Display Unit Cache API (references: DisplayUnitCache,
setDisplayUnitCache(DisplayUnitCache), CleverTapAPI, CTDisplayUnitController).
|
|
||
| #### New Features | ||
| * **Native Display Element Click:** New `pushDisplayUnitElementClickedEventForID(String unitID, HashMap<String, Object> additionalProperties)` on `CleverTapAPI` records a `Notification Clicked` event for a specific element within a Display Unit. Caller-supplied `additionalProperties` (including `wzrk_element_id` from the action's `metadata`) are merged first, then enriched with cached `wzrk_*` attribution fields from the unit — giving finer-grained click analytics for Native Display experiences. | ||
| * **Display Unit Cache API:** New public interface `DisplayUnitCache` and `setDisplayUnitCache(DisplayUnitCache)` on `CleverTapAPI` let external SDKs (e.g. the Native Display SDK) inject a custom display-unit store. `getAllDisplayUnits()` and `getDisplayUnitForID()` now route through this cache. The default implementation (`CTDisplayUnitController`) remains active when no override is installed. |
Contributor
There was a problem hiding this comment.
Align template API name with the actual method signature.
Line 6 uses getDisplayUnitForID(), while the API is getDisplayUnitForId(String). Keeping the template incorrect will reintroduce this typo in future generated changelogs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/CTCORECHANGELOG.md` at line 6, Update the template to use the
correct API name and signature: replace the incorrect getDisplayUnitForID() with
getDisplayUnitForId(String) in the CTCORECHANGELOG.md entry that documents
DisplayUnitCache, ensuring the line that mentions DisplayUnitCache,
setDisplayUnitCache(DisplayUnitCache), and CleverTapAPI uses the exact method
signature getDisplayUnitForId(String) so future changelogs reflect the real API
(CTDisplayUnitController, getAllDisplayUnits(), setDisplayUnitCache()).
Code Coverage Debug
Files
|
…stall race - Callback fires only when server delivers >=1 valid DU (matches pre-8.x behaviour and iOS parity). Null / empty adUnit_notifs arrays are a no-op: cache not touched, notifyDisplayUnitsLoaded never called. - Moved default-cache lazy-install into ControllerManager getOrCreateDisplayUnitCache() so check-and-create is atomic with host calls to setDisplayUnitCache(). Removes stale displayUnitControllerLock in DisplayUnitResponse. - setDisplayUnitCache() is now synchronized to pair with the new helper.
There was a problem hiding this comment.
Code Review — PR #1017: Release 8.3.0
Follow-up Review: Since my previous review (2026-06-04 10:35Z), commit
1ba33bb("fix(display-units): restore legacy callback contract and fix cache-install race", 11:16Z) was pushed. This review verifies that commit against the issues raised earlier and re-scans the new code. I am not re-posting previously-flagged items that have since been addressed, answered, or are trivial nits already on open threads.
Summary
This release adds a public DisplayUnitCache interface + setDisplayUnitCache(...) (so host SDKs can inject their own display-unit store), introduces pushDisplayUnitElementClickedEventForID(...) for element-level Native Display click analytics, and migrates all lookup/attribution call-sites off the now-deprecated getCTDisplayUnitController() onto getDisplayUnitCache(). JSON→model parsing is lifted out of the controller into DisplayUnitResponse. The follow-up commit resolves the two behavior-change concerns from the prior review (empty/null adUnit_notifs is now a true no-op, and the load callback no longer fires with null) and closes the lazy-cache-install race by moving check-and-create into a single synchronized ControllerManager.getOrCreateDisplayUnitCache().
📊 Visual Overview
flowchart TD
SR[Server adUnit_notifs] --> PDU[DisplayUnitResponse.parseDisplayUnits]
PDU -->|null or empty| NOP[No-op: cache untouched, no callback]
PDU -->|>=1 item| GOC[ControllerManager.getOrCreateDisplayUnitCache<br/>synchronized]
Host[Host SDK] -->|setDisplayUnitCache synchronized| CM[ControllerManager.displayUnitCache<br/>volatile]
GOC --> CM
CM --> IF{{DisplayUnitCache}}
IF -.default.-> CTC[CTDisplayUnitController]
IF -.custom.-> Custom[Host impl]
PDU -->|parsed list non-empty| CB[notifyDisplayUnitsLoaded]
AM[AnalyticsManager click / view / element-click] --> CM
Follow-up Verification
| Prior finding | Status |
|---|---|
Empty adUnit_notifs array wiped the cache |
✅ Fixed — parseDisplayUnits now early-returns on `messages == null |
Load callback fired with null on null/empty response |
✅ Fixed — callback fires only when !displayUnits.isEmpty(), restoring the pre-8.x / iOS contract. |
Lazy cache-install race vs. host setDisplayUnitCache (raised by CodeRabbit) |
✅ Fixed — moved into synchronized getOrCreateDisplayUnitCache(); setDisplayUnitCache is now synchronized too, and displayUnitCache is volatile. |
Element click fires with empty evtData (AnalyticsManager:286) |
✅ Accepted — author clarified "server ensures the fields are present." Not re-flagging. |
CTDisplayUnitController.updateDisplayUnits signature change is a public binary break (CTDisplayUnitController:68) |
⏳ Still open, no code change. Informational only — acceptable if this was never intended as host-facing surface. Not duplicating the existing thread. |
Missing trailing newline in CTDisplayUnitController.java (line 80) |
⏳ Still present. Trivial; existing thread stands. |
Independent re-scan of the new code: imports in AnalyticsManager (HashMap/Map/Iterator/JSONException) are all present, pushDisplayUnitElementClickedEventForID correctly layers caller additionalProperties then server wzrk_* on top (server wins on collision) and filters setWzrkParams back down to the wzrk_ namespace — a thoughtful contract-preserving touch. The malformed-only-response case (non-empty array, all entries invalid → empty list → reset()) matches legacy behavior (legacy also reset()-then-rebuilt), so no regression. Tests were migrated to the new List-based API and add useful coverage (replace-previous-cache, empty-list-clears).
Minor / Non-blocking
- Doc API name (
docs/CTCORECHANGELOG.md/templates/CTCORECHANGELOG.md, line 6): the entry referencesgetDisplayUnitForID(), but the publicCleverTapAPImethod integrators call isgetDisplayUnitForId(String)(lowercased).getDisplayUnitForIDis the cache interface method name, so it's not strictly wrong, but for a changelog aimed at integrators theCleverTapAPIspelling is clearer. CodeRabbit already has open threads on these exact lines — flagging here only for completeness, not as a new duplicate. - Sample (
MyApplication.kt):setDisplayUnitCache(SampleDisplayUnitCache())is now installed unconditionally in the demo app, which means the sample's display units no longer flow through the default controller. Fine as a feature demo; just noting it changes default sample behavior.
Verdict
APPROVE
The follow-up commit cleanly resolves both behavior-change concerns and the cache-install race from the prior round. The migration is consistent (no stale internal callers of the deprecated accessor), thread-safety is sound (volatile field + synchronized create/replace), and tests cover the new contract. Remaining items are trivial nits already tracked on open threads. No blocking issues.
To retrigger this review, comment /vision-review-deep on the PR.
Reviewed by Vision AI
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://wizrocket.atlassian.net/browse/SDK-5848
Summary by CodeRabbit
New Features
Samples
Chores