fix(SDK-5848): restore DU callback contract, fix cache race, fix changelog typo

[CTLalit]

Summary

Addresses review comments on #1017 before it ships.

1. Restore legacy callback contract (critical behaviour fix)

onDisplayUnitsLoaded was being fired with null in three error paths introduced by this PR — null adUnit_notifs array, null cache, and all-invalid parsed units. The legacy pre-8.x SDK never fired the callback in these cases, and iOS already guards on displayUnitJSON.count > 0 before doing anything at all.

Changes in DisplayUnitResponse.java:

• Restores the messages.length() == 0 early-return (no cache touch, no callback) — preserves the old no-op contract for empty arrays and matches iOS parity
• Removes the three notifyDisplayUnitsLoaded(null) call-sites in error paths
• Callback only fires when parseDisplayUnitsFromJson returns a non-empty list

2. Fix lazy cache-install race (CodeRabbit major comment)

The synchronized(displayUnitControllerLock) block in DisplayUnitResponse was not atomic with ControllerManager.setDisplayUnitCache() — a host thread calling setDisplayUnitCache(customCache) concurrently could be clobbered by the server-response thread installing a fresh CTDisplayUnitController.

Changes in ControllerManager.java:

• Adds getOrCreateDisplayUnitCache() — synchronized on this, so the check-and-create is atomic
• Makes setDisplayUnitCache() synchronized to pair with it
• DisplayUnitResponse now calls getOrCreateDisplayUnitCache() and drops its own displayUnitControllerLock

3. Fix changelog method name typo (CodeRabbit minor comment)

getDisplayUnitForID() → getDisplayUnitForId(String) (lowercase d) in the 8.3.0 entry in both docs/CTCORECHANGELOG.md and templates/CTCORECHANGELOG.md.

Also adds a Behavioral Notes section documenting the callback contract explicitly so integrators have a clear contract to rely on.

Test plan

• Verify onDisplayUnitsLoaded is not called when server returns adUnit_notifs: []
• Verify onDisplayUnitsLoaded is not called when server returns adUnit_notifs: null / key absent
• Verify onDisplayUnitsLoaded is called with the correct list when server returns valid units
• Verify a host calling setDisplayUnitCache(customCache) before the first server response sees its cache updated (not replaced) when the response arrives
• Existing AnalyticsManagerTest / CTDisplayUnitControllerTest pass

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@CTLalit, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 24 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2ed5f004-9a75-4b61-819a-b65edb5ff4d9

📥 Commits

Reviewing files that changed from the base of the PR and between 1ba33bb and b2897a7.

📒 Files selected for processing (2)

• docs/CTCORECHANGELOG.md
• templates/CTCORECHANGELOG.md

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/SDK-5848-display-unit-callback-docs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[francispereira]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[clevertap-vision]

Code Review

Summary

This is effectively a documentation-only PR. Although the description outlines three Java changes (callback contract, cache-install race, changelog typo), the actual diff against the develop base contains only docs/CTCORECHANGELOG.md and templates/CTCORECHANGELOG.md — the Java changes were already landed in commit 1ba33bbb and are present in the base branch. The changelog edits correctly fix the getDisplayUnitForID() → getDisplayUnitForId(String) typo (verified against CleverTapAPI.java:1794) and add an accurate Behavioral Notes section describing the restored callback contract.

What I verified

• ✅ getDisplayUnitForId(String) is the real public signature (CleverTapAPI.java:1794); the lowercase-d fix is correct.
• ✅ The new callback-contract note matches DisplayUnitResponse.parseDisplayUnits() — a null/empty adUnit_notifs array returns early (no cache touch, no callback), and the callback fires only when at least one valid unit parses.
• ⚠️ The PR unexpectedly also rewrites two unrelated Medium links in the v5.x changelog entries, replacing the hyphen before the article hash with a slash. This breaks the URLs (Medium slugs end in -<hash>, not /<hash>).

📊 Visual Overview

flowchart TD
    A[PR #1018 diff vs develop] --> B[docs/CTCORECHANGELOG.md]
    A --> C[templates/CTCORECHANGELOG.md]
    B --> D["Fix: getDisplayUnitForID -> getDisplayUnitForId(String) ✅"]
    B --> E["Add: Behavioral Notes / callback contract ✅"]
    B --> F["Rewrite 2 Medium URLs: hyphen -> slash ❌ breaks links"]
    C --> D
    C --> E
    C --> F

Loading

Verdict

REQUEST CHANGES

The intended changes (method-name fix and Behavioral Notes) are accurate and well-written. However, the diff also introduces an unintended regression: two pre-existing Medium documentation links (in unrelated v5.x sections, present in both files = 4 lines total) had their -<hash> suffix changed to /<hash>, which turns working links into 404s. These edits are outside the PR's stated scope and should be reverted before merge. Everything else looks good.

---

To retrigger this review, comment /vision-review-deep on the PR.

Reviewed by Vision AI

[clevertap-vision]

🐛 Bug — broken link (unrelated to PR scope): This line changes the hyphen before the article hash to a slash. Medium article URLs use the form …/<title-slug>-<12charhash>, so …/customizing-workmanager-fundamentals/fdaa17c46dd2 404s, whereas the original …-fdaa17c46dd2 resolves. This edit is in an unrelated v5.x changelog entry and appears accidental — please revert it.

Suggested change

	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals/fdaa17c46dd2)
	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals-fdaa17c46dd2)

[clevertap-vision]

🐛 Bug — broken link: Same issue as line 333 — the /fdaa17c46dd2 slash breaks the Medium URL (should be -fdaa17c46dd2). Unrelated to this PR's scope; please revert.

Suggested change

	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals/fdaa17c46dd2)
	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals-fdaa17c46dd2)

[clevertap-vision]

🐛 Bug — broken link: Same broken Medium URL as in docs/CTCORECHANGELOG.md:333 — slash should be a hyphen before the hash. Please revert this out-of-scope edit.

Suggested change

	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals/fdaa17c46dd2)
	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals-fdaa17c46dd2)

[clevertap-vision]

🐛 Bug — broken link: Same as line 333 / docs counterpart — /fdaa17c46dd2 breaks the Medium link (should be -fdaa17c46dd2). Please revert.

Suggested change

	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals/fdaa17c46dd2)
	blog [here](https://medium.com/androiddevelopers/customizing-workmanager-fundamentals-fdaa17c46dd2)

[clevertap-vision]

💡 Suggestion (precision): The note says "Null or empty adUnit_notifs responses are a no-op — the cache is not cleared." That is accurate for null/empty arrays. However, in the merged code (DisplayUnitResponse.parseDisplayUnits), a non-empty adUnit_notifs array whose entries are all invalid still calls cache.updateDisplayUnits([]), which effectively clears the cache while the callback is skipped. Readers may generalize "no-op" to that case too. Consider scoping the wording to null/empty arrays explicitly, or documenting the all-invalid case. Not blocking.