Open
Conversation
Adds the 5 shared constants (fetch type, response keys, prefs key, throttle window) that every Inbox V2 task reads from, so literal strings stay in one place and can't silently drift across files. 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds App Inbox v2: new constants, public fetch API + callback, network/fetch infra (scope, throttle, endpoint, call/result types), fetcher/bridge/response/merger, DB pending-actions tables/DAO, delete coordinator, controller wiring and lifecycle/login triggers, swipe-to-refresh UI, and extensive unit + Robolectric tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as Activity
participant ALM as ActivityLifeCycleManager
participant Bridge as InboxV2Bridge
participant Scope as NetworkScope
participant Fetcher as InboxV2Fetcher
participant Throttle as FetchThrottle
participant Endpoint as InboxFetchCall
participant CtApi as CtApi
participant Response as InboxV2Response
participant Controller as CTInboxController
participant DB as DBAdapter
participant Callback as CallbackManager
Activity->>ALM: activityResumed()
ALM->>Bridge: submit(respectThrottle=false, callback=null)
Bridge->>Scope: launch coroutine (coroutineScope)
Scope->>Fetcher: fetch(respectThrottle=false)
Fetcher->>Throttle: shouldThrottle()
Throttle-->>Fetcher: allowed / throttled
alt allowed
Fetcher->>Throttle: recordFetch()
Fetcher->>Endpoint: execute()
Endpoint->>CtApi: sendInboxFetch(body)
CtApi-->>Endpoint: HTTP Response
Endpoint-->>Fetcher: CallResult<JSONObject>
alt Success
Fetcher->>Response: processResponse(JSONObject)
Response->>Controller: processV2Response(parsedDaos)
Controller->>DB: upsert/delete/pending read/remove pending deletes
Controller->>Callback: _notifyInboxMessagesDidUpdate()
else Throttled/Disabled/Failure
Fetcher-->>Bridge: non-success result
end
else throttled
Fetcher-->>Bridge: CallResult.Throttled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java (1)
103-103: Alias the existingisReadkey to avoid contract drift.
KEY_IS_READalready defines the same server key on Line 294. Keeping the Inbox v2 semantic name is fine, but aliasing it avoids two independent"isRead"literals.♻️ Proposed refactor
- String INBOX_V2_ISREAD_KEY = "isRead"; + String INBOX_V2_ISREAD_KEY = KEY_IS_READ;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java` at line 103, Replace the duplicate literal by making INBOX_V2_ISREAD_KEY alias the existing KEY_IS_READ constant instead of reusing the string "isRead"; change the declaration of INBOX_V2_ISREAD_KEY to reference KEY_IS_READ (i.e., INBOX_V2_ISREAD_KEY = KEY_IS_READ) so both names share the same canonical value and avoid contract drift while keeping the Inbox v2 semantic name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java`:
- Line 103: Replace the duplicate literal by making INBOX_V2_ISREAD_KEY alias
the existing KEY_IS_READ constant instead of reusing the string "isRead"; change
the declaration of INBOX_V2_ISREAD_KEY to reference KEY_IS_READ (i.e.,
INBOX_V2_ISREAD_KEY = KEY_IS_READ) so both names share the same canonical value
and avoid contract drift while keeping the Inbox v2 semantic name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9300b69b-f29f-4f24-8bd5-9270c7a3be22
📒 Files selected for processing (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.javaclevertap-core/src/test/java/com/clevertap/android/sdk/ConstantsTest.kt
Code Coverage Debug
Files
|
Routes the V2 inbox to its dedicated endpoint so fetch/delete don't get batched into /a1 alongside regular events. Also teaches getUriForPath to split multi-segment relativeUrl on '/' before appending so paths like inbox/v2/getMessages aren't URL-encoded into a single %2F-laden segment; single-segment callers are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reads the new isRead boolean from V2 responses so cross-device read state survives into the DAO. Missing field defaults to unread, preserving V1 behaviour unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Unifies the five possible outcomes of any single-shot V2 network call (success, throttled, disabled, HTTP error, transport failure) so callers pattern-match once and the compiler enforces exhaustive handling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defines the single-method contract that every V2 endpoint implements, so orchestration, error mapping, and testing all plug into one shape regardless of whether the call is a fetch, delete, or future event push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces one CoroutineScope per CleverTap instance, backed by a SupervisorJob and an injectable dispatcher (default Dispatchers.IO), so V2 network coroutines can be launched with structured lifetimes and isolated failures without blocking the existing CTExecutorFactory threads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Centralizes the [header, event] JSON array layout every direct V2 call needs, so each EndpointCall builds only its own event object and never hand-rolls the surrounding array. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a reusable persistent throttle keyed by account id and pref name, so V2 fetch callers (public fetchInbox and pull-to-refresh) enforce the 5-min window across app restarts without baking feature-specific state into the class. Uses the existing Clock interface and TestClock for determinism. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r (DF6) Wires the V2 inbox fetch end-to-end: builds the wzrk_fetch event, wraps it in EventRequestBody, hits sendInboxFetch on the injected dispatcher, and maps each HTTP outcome to a CallResult (200→Success, 403→Disabled, else →HttpError, empty/parse/IO→NetworkFailure). Sets the per-endpoint error matrix pattern every future EndpointCall will follow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extracts the V2 response merge math into a zero-dependency Kotlin object so the controller method can orchestrate DB and lock while the filter logic stays unit-testable with no mocks. Single predicate definition is shared by the pre-write and post-read passes so they can't drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ler (T1.4) Threads the InboxV2Merger dual-filter through the controller's DB and lock: pre-write filter → upsert → re-read under messagesLock → post-read cleanup → batch delete of stale rows → in-memory swap. Returns whether anything changed so the response handler can decide when to fire the UI callback, matching the V1 updateMessages contract. Pending sets are empty today; T3.3 wires them to the pending_deletes/pending_reads tables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Routes V2 fetch payloads through the same four safety checks V1 uses (analytics-only, key presence, parse try/catch, null-controller init) so the active-fetch path can't bypass guards the passive path enforces. Bridges to CTInboxController.processV2Response and fires inboxMessagesDidUpdate on real changes. Opens the minimum accessors the cross-package response handler needs: CTMessageDAO.initWithJSON becomes public and CTInboxController gains a getUserId() getter. Both classes remain @RestrictTo(LIBRARY) so consumer visibility is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Puts the inbox-specific glue between the generic EndpointCall and the response handler: session-scoped disable flag, throttle gate (honoured for user-initiated triggers, bypassed for app-launch and onUserLogin), recordFetch on the allowed path, and hand-off to InboxV2Response on success so the lock, controller-init, and UI callback stay where T1.4a owns them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2 chain (DF8) Lets Java callers fire-and-forget an Inbox V2 fetch: the bridge launches the suspend fetcher on NetworkScope and delivers success/failure to an optional FetchInboxCallback. The factory now constructs the full InboxV2Response → InboxFetchCall → FetchThrottle → InboxV2Fetcher → InboxV2Bridge chain once per SDK instance and exposes the bridge through CoreState so the trigger wirings (T1.5/T1.6/T1.7) become one-liners. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops a single bridge.submit(false, null) into ActivityLifeCycleManager's cold-launch block so the inbox is actively pulled once per app launch without a throttle. The factory now constructs the V2 chain before the lifecycle manager so the bridge is ready at wire time; activityLifeCycle- Manager and loginController keep their original relative order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt (1)
281-295:⚠️ Potential issue | 🔴 Critical
getUriForPathrefactor: multi-segment path caller found—behavior change confirmed.The old
builder.appendPath(relativeUrl)treated the entire string as a single segment (encoding/as%2F); the new split-and-append logic treats slashes as segment separators. Not all callers pass single-segment paths. ThesendInboxFetchandsendInboxDeletemethods passrelativeUrl = "inbox/v2/getMessages", which will now produce/inbox/v2/getMessagesinstead of/inbox%2Fv2%2FgetMessages. Verify that this behavior change is intentional and that the API endpoint expectations align with the new URL structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt` around lines 281 - 295, getUriForPath now splits relativeUrl into segments which changes "/" handling and breaks callers that expect the entire relativeUrl to be a single encoded segment (e.g., sendInboxFetch and sendInboxDelete that pass "inbox/v2/getMessages"). Restore the original behavior or make the intent explicit: either revert to calling builder.appendPath(relativeUrl) to preserve the old encoded-single-segment behavior in getUriForPath, or add a boolean flag/overload (e.g., preserveAsSingleSegment: Boolean) to getUriForPath and use it so callers like sendInboxFetch/sendInboxDelete can continue passing a single string, while callers that need multi-segment paths can opt-in to splitting. Ensure changes reference getUriForPath and update sendInboxFetch/sendInboxDelete accordingly.
🧹 Nitpick comments (3)
clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTMessageDAO.java (1)
141-161: UseConstants.INBOX_V2_ISREAD_KEYintoJSON()for symmetry withinitWithJSON.
initWithJSONnow reads viaConstants.INBOX_V2_ISREAD_KEY(line 184) whiletoJSON()still writes the literal"isRead"on line 146. The values happen to match, so this is not a bug — but if the constant is ever renamed, serialization and deserialization will silently diverge. Cheap hardening:Proposed fix
- jsonObject.put("isRead", this.read); + jsonObject.put(Constants.INBOX_V2_ISREAD_KEY, this.read);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTMessageDAO.java` around lines 141 - 161, The toJSON() method in CTMessageDAO currently writes the literal "isRead" but initWithJSON reads Constants.INBOX_V2_ISREAD_KEY; update CTMessageDAO.toJSON() to use Constants.INBOX_V2_ISREAD_KEY when putting the read value so serialization and deserialization remain symmetric (modify the jsonObject.put call that writes the read flag in toJSON(), keeping the rest of the method intact).clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Fetcher.kt (1)
64-64:CallResult.Throttledfromendpoint.execute()is unreachable.Per
InboxFetchCall, the endpoint never returnsThrottled— throttling is strictly a client-side gate here. TheCallResult.Throttled -> CallResult.Throttledbranch is present only to satisfy exhaustiveness. That's fine, but worth a one-line comment so a future reader doesn't add logic there assuming the endpoint can produce it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Fetcher.kt` at line 64, The CallResult.Throttled -> CallResult.Throttled branch in InboxV2Fetcher (the when over endpoint.execute() results) is unreachable because InboxFetchCall never returns Throttled; add a one-line comment on that branch explaining that throttling is a client-side gate and the endpoint cannot produce Throttled so the branch exists only for exhaustiveness and should not be given runtime logic.clevertap-core/src/main/java/com/clevertap/android/sdk/response/InboxV2Response.kt (1)
63-74:processV2Responseruns insideinboxControllerLock.The full message processing (DB upsert, re-read, delete-by-ids, in-memory list rebuild — see
CTInboxController.processV2Response) executes whileinboxControllerLockis held. That lock is primarily intended to guard controller initialization; holding it across a DB write/read/delete sequence can serialize unrelated inbox operations and, combined withmessagesLockheld insideprocessV2Response, increases lock-ordering surface. Consider narrowing thesynchronized(inboxControllerLock)block to the init-if-null check only, then grabbing the controller reference and callingprocessV2Response/the update notification outside the lock — matching the typical V1 pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/response/InboxV2Response.kt` around lines 63 - 74, The synchronized(inboxControllerLock) currently wraps controllerManager.initializeInbox(), parseDaos(...), controller.processV2Response(...) and callbackManager._notifyInboxMessagesDidUpdate(), causing long-held locking; change it to only guard controller initialization: inside synchronized(inboxControllerLock) check and call controllerManager.initializeInbox() if null and then obtain a local reference to controllerManager.ctInboxController, then move parseDaos(messages, controller.userId), controller.processV2Response(parsed) and the callbackManager._notifyInboxMessagesDidUpdate() call to execute outside the synchronized block so heavy DB/write/read work (parseDaos and CTInboxController.processV2Response) runs without holding inboxControllerLock while still preserving the init safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Fetcher.kt`:
- Around line 51-65: Move the call to throttle.recordFetch() so it only runs for
outcomes that should actually start the throttle window (e.g. on
CallResult.Success and/or HttpError), not before endpoint.execute();
specifically, call endpoint.execute() first, then when handling its result
invoke throttle.recordFetch() only for the non-transport outcomes you want
throttled (referring to throttle.recordFetch(), endpoint.execute(),
CallResult.Success, CallResult.HttpError), and update InboxV2FetcherTest
expectations (tests referencing respectThrottle and the recordFetch behavior)
and any references to Constants.INBOX_V2_THROTTLE_WINDOW_MS accordingly.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Merger.kt`:
- Around line 36-62: preWriteFilter uses CTMessageDAO.setRead(1) while
postReadCleanup assigns dao.isRead = 1, creating inconsistent behavior if
setRead(...) has side-effects; change postReadCleanup to call the same API as
preWriteFilter (invoke setRead(1) on the CTMessageDAO instance when dao.id is in
pendingReads) so both passes use CTMessageDAO.setRead(int) rather than directly
mutating dao.isRead.
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt`:
- Around line 119-131: sendInboxDelete is using the fetch path
"inbox/v2/getMessages" which will POST fetch semantics; update sendInboxDelete
to use the correct delete endpoint (confirm with backend — e.g.
"inbox/v2/deleteMessage" or the agreed sibling path) by changing the relativeUrl
argument in the createRequest call, keep using getActualDomain/defaultDomain and
defaultHeaders as before, and add a unit test that calls sendInboxDelete (or the
request-building path) and asserts the request's relative path equals the agreed
delete path so this cannot regress.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/FetchThrottle.kt`:
- Line 28: The current shouldThrottle() uses (clock.currentTimeMillis() -
readLast()) which can be negative if a persisted timestamp is in the future;
change shouldThrottle() to guard against future timestamps by first reading now
= clock.currentTimeMillis() and last = readLast(), then compute delta = now -
last and if delta < 0 treat as not throttled (return false) or clamp delta = 0
before comparing to windowMs; update shouldThrottle() and ensure
recordFetch()/readLast() semantics remain unchanged.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/ActivityLifeCycleManagerTest.kt`:
- Around line 165-174: The test currently sets
coreState.coreMetaData.isAppLaunchPushed = true so neither activityResumed call
exercises the app-launch path; change the setup to set isAppLaunchPushed =
false, call activityLifeCycleManager.activityResumed(mockActivity) twice, and
then verify inboxV2Bridge.submit was invoked exactly once with the expected
arguments (verify(exactly = 1) { inboxV2Bridge.submit(false, null) }), relying
on the production code to flip isAppLaunchPushed to true after the first resume;
keep the mocked Activity and relaxed mock behavior unchanged.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiInboxFetchTest.kt`:
- Around line 8-10: The timestamp assertion is flaky because
currentRequestTimestampSeconds is recomputed after building the request; instead
capture a time window around the request by recording a startTime (e.g.,
startRequestMillis = System.currentTimeMillis()) immediately before building or
sending the request and an endTime (endRequestMillis =
System.currentTimeMillis()) immediately after, convert both to seconds (floor
division by 1000), then assert that the emitted request JSON's "ts" value (the
ts field parsed from the request) is >= startSeconds and <= endSeconds; update
the test in CtApiInboxFetchTest.kt to use these start/end bounds rather than
recomputing currentRequestTimestampSeconds.
- Around line 55-59: The test `sendInboxDelete` currently asserts a specific
endpoint path using ctApi.sendInboxDelete and should not hard-fail on an
unconfirmed backend contract; remove or comment out the line asserting the URL
contains "/inbox/v2/getMessages" (the assertContains call in the
`sendInboxDelete` test) or replace it with a non-failing placeholder check
(e.g., only verify a non-null URL or skip the path assertion) and leave the
existing TODO note in place so the path check can be reinstated once the backend
contract is confirmed.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/InboxFetchCallTest.kt`:
- Around line 114-120: Test uses a failure-shaped response body so a real
network hit could still produce NetworkFailure; update the MockHttpClient
invocation in the test function `null header from builder returns NetworkFailure
without hitting network` (the call to `newCall(http, header = null).execute()`)
to use a success-shaped responseBody (e.g. a valid JSON/expected payload instead
of "should-not-be-read") with responseCode 200 so that any accidental network
hit would return success and make the assertion fail.
---
Outside diff comments:
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt`:
- Around line 281-295: getUriForPath now splits relativeUrl into segments which
changes "/" handling and breaks callers that expect the entire relativeUrl to be
a single encoded segment (e.g., sendInboxFetch and sendInboxDelete that pass
"inbox/v2/getMessages"). Restore the original behavior or make the intent
explicit: either revert to calling builder.appendPath(relativeUrl) to preserve
the old encoded-single-segment behavior in getUriForPath, or add a boolean
flag/overload (e.g., preserveAsSingleSegment: Boolean) to getUriForPath and use
it so callers like sendInboxFetch/sendInboxDelete can continue passing a single
string, while callers that need multi-segment paths can opt-in to splitting.
Ensure changes reference getUriForPath and update sendInboxFetch/sendInboxDelete
accordingly.
---
Nitpick comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTMessageDAO.java`:
- Around line 141-161: The toJSON() method in CTMessageDAO currently writes the
literal "isRead" but initWithJSON reads Constants.INBOX_V2_ISREAD_KEY; update
CTMessageDAO.toJSON() to use Constants.INBOX_V2_ISREAD_KEY when putting the read
value so serialization and deserialization remain symmetric (modify the
jsonObject.put call that writes the read flag in toJSON(), keeping the rest of
the method intact).
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Fetcher.kt`:
- Line 64: The CallResult.Throttled -> CallResult.Throttled branch in
InboxV2Fetcher (the when over endpoint.execute() results) is unreachable because
InboxFetchCall never returns Throttled; add a one-line comment on that branch
explaining that throttling is a client-side gate and the endpoint cannot produce
Throttled so the branch exists only for exhaustiveness and should not be given
runtime logic.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/response/InboxV2Response.kt`:
- Around line 63-74: The synchronized(inboxControllerLock) currently wraps
controllerManager.initializeInbox(), parseDaos(...),
controller.processV2Response(...) and
callbackManager._notifyInboxMessagesDidUpdate(), causing long-held locking;
change it to only guard controller initialization: inside
synchronized(inboxControllerLock) check and call
controllerManager.initializeInbox() if null and then obtain a local reference to
controllerManager.ctInboxController, then move parseDaos(messages,
controller.userId), controller.processV2Response(parsed) and the
callbackManager._notifyInboxMessagesDidUpdate() call to execute outside the
synchronized block so heavy DB/write/read work (parseDaos and
CTInboxController.processV2Response) runs without holding inboxControllerLock
while still preserving the init safety.
🪄 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: 280896da-6f5b-48f9-b4d7-2cb30fcc4c36
📒 Files selected for processing (31)
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifeCycleManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.ktclevertap-core/src/main/java/com/clevertap/android/sdk/FetchInboxCallback.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxController.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTMessageDAO.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Bridge.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Fetcher.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Merger.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/CallResult.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/EndpointCall.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/EventRequestBody.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/FetchThrottle.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxFetchCall.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/NetworkScope.ktclevertap-core/src/main/java/com/clevertap/android/sdk/response/InboxV2Response.ktclevertap-core/src/test/java/com/clevertap/android/sdk/ActivityLifeCycleManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/MockCoreState.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/CTInboxControllerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/CTMessageDAOTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/InboxV2BridgeTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/InboxV2FetcherTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/InboxV2MergerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiInboxFetchTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/CallResultTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/EventRequestBodyTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/FetchThrottleTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/InboxFetchCallTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/NetworkScopeTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/response/InboxV2ResponseTest.kt
✅ Files skipped from review due to trivial changes (3)
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/EndpointCall.kt
- clevertap-core/src/test/java/com/clevertap/android/sdk/inbox/CTMessageDAOTest.kt
- clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/CallResultTest.kt
Comment on lines
+51
to
+65
| throttle.recordFetch() | ||
|
|
||
| return when (val raw = endpoint.execute()) { | ||
| is CallResult.Success -> { | ||
| inboxV2Response.processResponse(raw.data) | ||
| CallResult.Success(Unit) | ||
| } | ||
| CallResult.Disabled -> { | ||
| disabledForSession = true | ||
| CallResult.Disabled | ||
| } | ||
| is CallResult.HttpError -> raw | ||
| is CallResult.NetworkFailure -> raw | ||
| CallResult.Throttled -> CallResult.Throttled | ||
| } |
Contributor
There was a problem hiding this comment.
Throttle window is recorded before the network call, so transient failures block retries for the full window.
throttle.recordFetch() runs unconditionally before endpoint.execute(). If the endpoint then returns NetworkFailure (e.g. transport error, timeout, empty body, header-build failure mapped to NetworkFailure in InboxFetchCall) or HttpError, the 5‑minute throttle (Constants.INBOX_V2_THROTTLE_WINDOW_MS) is still in effect and any user-initiated respectThrottle = true retry (pull‑to‑refresh, public fetchInbox()) will be silently swallowed as Throttled until the window elapses — even though no usable fetch ever happened.
Typically you want to record the fetch only on outcomes worth throttling (Success, or maybe HttpError), not on transport failures. Consider moving recordFetch() after a successful endpoint.execute(), or scoping it to non-transport outcomes.
🛠️ Proposed change
- throttle.recordFetch()
-
return when (val raw = endpoint.execute()) {
is CallResult.Success -> {
inboxV2Response.processResponse(raw.data)
+ throttle.recordFetch()
CallResult.Success(Unit)
}
CallResult.Disabled -> {
disabledForSession = true
CallResult.Disabled
}
- is CallResult.HttpError -> raw
+ is CallResult.HttpError -> {
+ throttle.recordFetch()
+ raw
+ }
is CallResult.NetworkFailure -> raw
CallResult.Throttled -> CallResult.Throttled
}Note: if you change this, update InboxV2FetcherTest (respectThrottle=false bypasses throttle and recordFetch runs only when throttle is honoured and endpoint is about to be called) to reflect the new contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Fetcher.kt`
around lines 51 - 65, Move the call to throttle.recordFetch() so it only runs
for outcomes that should actually start the throttle window (e.g. on
CallResult.Success and/or HttpError), not before endpoint.execute();
specifically, call endpoint.execute() first, then when handling its result
invoke throttle.recordFetch() only for the non-transport outcomes you want
throttled (referring to throttle.recordFetch(), endpoint.execute(),
CallResult.Success, CallResult.HttpError), and update InboxV2FetcherTest
expectations (tests referencing respectThrottle and the recordFetch behavior)
and any references to Constants.INBOX_V2_THROTTLE_WINDOW_MS accordingly.
Comment on lines
+36
to
+62
| else -> dao.also { | ||
| if (it.id in pendingReads) it.setRead(1) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Pass 2 — runs after re-reading the full DB. Produces the batch delete | ||
| * list plus the updated in-memory list (with pending-reads applied). | ||
| */ | ||
| fun postReadCleanup( | ||
| full: List<CTMessageDAO>, | ||
| pendingDeletes: Set<String>, | ||
| pendingReads: Set<String>, | ||
| videoSupported: Boolean, | ||
| nowSec: Long | ||
| ): CleanupResult { | ||
| val toDelete = ArrayList<String>() | ||
| val kept = ArrayList<CTMessageDAO>(full.size) | ||
|
|
||
| for (dao in full) { | ||
| when { | ||
| dao.id in pendingDeletes -> toDelete.add(dao.id) | ||
| isExpired(dao, nowSec) -> toDelete.add(dao.id) | ||
| !videoSupported && dao.containsVideoOrAudio() -> toDelete.add(dao.id) | ||
| else -> { | ||
| if (dao.id in pendingReads) dao.isRead = 1 |
Contributor
There was a problem hiding this comment.
Inconsistent API used to mark a message read across the two passes.
preWriteFilter calls dao.setRead(1) (line 37) while postReadCleanup assigns dao.isRead = 1 (line 62). If these two paths are not behaviorally identical on CTMessageDAO (e.g., setRead(int) updates additional state or a derived field, or the setter has side effects like timestamping), the two passes will leave the DAO in different states. Either way, using the same call in both branches removes a subtle source of drift.
🔧 Proposed change
- if (dao.id in pendingReads) dao.isRead = 1
+ if (dao.id in pendingReads) dao.setRead(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxV2Merger.kt`
around lines 36 - 62, preWriteFilter uses CTMessageDAO.setRead(1) while
postReadCleanup assigns dao.isRead = 1, creating inconsistent behavior if
setRead(...) has side-effects; change postReadCleanup to call the same API as
preWriteFilter (invoke setRead(1) on the CTMessageDAO instance when dao.id is in
pendingReads) so both passes use CTMessageDAO.setRead(int) rather than directly
mutating dao.isRead.
Comment on lines
+119
to
+131
| // Working assumption: delete shares V2 fetch's URL shape. Point at the same | ||
| // relative URL for now. CONFIRM with backend before merging T3.2 — if it's | ||
| // actually a sibling path like "inbox/v2/deleteMessage" change this single | ||
| // literal. Nothing else in the pipeline cares about the path. | ||
| fun sendInboxDelete(body: String): Response = | ||
| httpClient.execute( | ||
| createRequest( | ||
| baseUrl = getActualDomain(isViewedEvent = false) ?: defaultDomain, | ||
| relativeUrl = "inbox/v2/getMessages", | ||
| body = body, | ||
| headers = defaultHeaders | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
sendInboxDelete targets the fetch URL — must be resolved before merge.
sendInboxDelete currently POSTs to inbox/v2/getMessages, which is the fetch endpoint. If any caller wires delete through this method before the TODO is addressed, deletion requests will hit (and likely be treated as) fetches, which can silently no-op or corrupt server-side state depending on backend handling. The inline comment already flags this as something to confirm with backend before T3.2 — please resolve it before this lands in a release, and add a test asserting the delete URL path once the correct endpoint is known.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt`
around lines 119 - 131, sendInboxDelete is using the fetch path
"inbox/v2/getMessages" which will POST fetch semantics; update sendInboxDelete
to use the correct delete endpoint (confirm with backend — e.g.
"inbox/v2/deleteMessage" or the agreed sibling path) by changing the relativeUrl
argument in the createRequest call, keep using getActualDomain/defaultDomain and
defaultHeaders as before, and add a unit test that calls sendInboxDelete (or the
request-building path) and asserts the request's relative path equals the agreed
delete path so this cannot regress.
| private val clock: Clock = Clock.SYSTEM | ||
| ) { | ||
| @WorkerThread | ||
| fun shouldThrottle(): Boolean = (clock.currentTimeMillis() - readLast()) < windowMs |
Contributor
There was a problem hiding this comment.
Guard against future persisted timestamps.
If the device clock moves backward after recordFetch(), clock.currentTimeMillis() - readLast() becomes negative and this can throttle fetches until wall time catches up. Treat missing/future timestamps as “not throttled” or clamp the delta.
Proposed fix
- fun shouldThrottle(): Boolean = (clock.currentTimeMillis() - readLast()) < windowMs
+ fun shouldThrottle(): Boolean {
+ val now = clock.currentTimeMillis()
+ val last = readLast()
+ return last > 0L && last <= now && (now - last) < windowMs
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fun shouldThrottle(): Boolean = (clock.currentTimeMillis() - readLast()) < windowMs | |
| fun shouldThrottle(): Boolean { | |
| val now = clock.currentTimeMillis() | |
| val last = readLast() | |
| return last > 0L && last <= now && (now - last) < windowMs | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/FetchThrottle.kt`
at line 28, The current shouldThrottle() uses (clock.currentTimeMillis() -
readLast()) which can be negative if a persisted timestamp is in the future;
change shouldThrottle() to guard against future timestamps by first reading now
= clock.currentTimeMillis() and last = readLast(), then compute delta = now -
last and if delta < 0 treat as not throttled (return false) or clamp delta = 0
before comparing to windowMs; update shouldThrottle() and ensure
recordFetch()/readLast() semantics remain unchanged.
Comment on lines
+165
to
+174
| @Test | ||
| fun `subsequent activityResumed calls within the same launch do not re-submit V2 fetch`() { | ||
| val mockActivity = mockk<Activity>(relaxed = true) | ||
| coreState.coreMetaData.isAppLaunchPushed = true | ||
|
|
||
| activityLifeCycleManager.activityResumed(mockActivity) | ||
| activityLifeCycleManager.activityResumed(mockActivity) | ||
|
|
||
| verify(exactly = 0) { inboxV2Bridge.submit(any(), any()) } | ||
| } |
Contributor
There was a problem hiding this comment.
Test name is slightly misleading — it doesn't exercise "same launch" semantics.
The test sets isAppLaunchPushed = true before the first activityResumed call, so neither call ever enters the app-launch branch. This effectively duplicates test_activityResumed_whenAppLaunchedIsPushed. A stronger test would set isAppLaunchPushed = false initially and rely on the production code flipping it to true after the first resume, then assert exactly one submit(false, null) across both calls.
Suggested adjustment
- coreState.coreMetaData.isAppLaunchPushed = true
-
- activityLifeCycleManager.activityResumed(mockActivity)
- activityLifeCycleManager.activityResumed(mockActivity)
-
- verify(exactly = 0) { inboxV2Bridge.submit(any(), any()) }
+ coreState.coreMetaData.isAppLaunchPushed = false
+
+ activityLifeCycleManager.activityResumed(mockActivity)
+ activityLifeCycleManager.activityResumed(mockActivity)
+
+ verify(exactly = 1) { inboxV2Bridge.submit(false, null) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| fun `subsequent activityResumed calls within the same launch do not re-submit V2 fetch`() { | |
| val mockActivity = mockk<Activity>(relaxed = true) | |
| coreState.coreMetaData.isAppLaunchPushed = true | |
| activityLifeCycleManager.activityResumed(mockActivity) | |
| activityLifeCycleManager.activityResumed(mockActivity) | |
| verify(exactly = 0) { inboxV2Bridge.submit(any(), any()) } | |
| } | |
| `@Test` | |
| fun `subsequent activityResumed calls within the same launch do not re-submit V2 fetch`() { | |
| val mockActivity = mockk<Activity>(relaxed = true) | |
| coreState.coreMetaData.isAppLaunchPushed = false | |
| activityLifeCycleManager.activityResumed(mockActivity) | |
| activityLifeCycleManager.activityResumed(mockActivity) | |
| verify(exactly = 1) { inboxV2Bridge.submit(false, null) } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/ActivityLifeCycleManagerTest.kt`
around lines 165 - 174, The test currently sets
coreState.coreMetaData.isAppLaunchPushed = true so neither activityResumed call
exercises the app-launch path; change the setup to set isAppLaunchPushed =
false, call activityLifeCycleManager.activityResumed(mockActivity) twice, and
then verify inboxV2Bridge.submit was invoked exactly once with the expected
arguments (verify(exactly = 1) { inboxV2Bridge.submit(false, null) }), relying
on the production code to flip isAppLaunchPushed to true after the first resume;
keep the mocked Activity and relaxed mock behavior unchanged.
Comment on lines
+8
to
+10
| import kotlin.test.assertContains | ||
| import kotlin.test.assertEquals | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid a second-boundary flake in the timestamp assertion.
Line 52 recomputes currentRequestTimestampSeconds after building the request, so this can fail when the test crosses a one-second boundary. Capture a time window around the request and assert the emitted ts falls within it.
🧪 Proposed test hardening
import kotlin.test.assertContains
import kotlin.test.assertEquals
+import kotlin.test.assertTrue `@Test`
fun `sendInboxFetch attaches default query params`() {
+ val before = ctApi.currentRequestTimestampSeconds
val url = ctApi.sendInboxFetch(body = "[]").request.url.toString()
+ val after = ctApi.currentRequestTimestampSeconds
assertContains(url, "os=Android")
assertContains(url, "t=${CtApiTestProvider.SDK_VERSION}")
assertContains(url, "z=${CtApiTestProvider.ACCOUNT_ID}")
- assertContains(url, "ts=${ctApi.currentRequestTimestampSeconds}")
+ val ts = requireNotNull(Regex("""[?&]ts=(\d+)""").find(url)) {
+ "Missing ts query param: $url"
+ }.groupValues[1].toLong()
+ assertTrue(ts in before..after)
}Also applies to: 47-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiInboxFetchTest.kt`
around lines 8 - 10, The timestamp assertion is flaky because
currentRequestTimestampSeconds is recomputed after building the request; instead
capture a time window around the request by recording a startTime (e.g.,
startRequestMillis = System.currentTimeMillis()) immediately before building or
sending the request and an endTime (endRequestMillis =
System.currentTimeMillis()) immediately after, convert both to seconds (floor
division by 1000), then assert that the emitted request JSON's "ts" value (the
ts field parsed from the request) is >= startSeconds and <= endSeconds; update
the test in CtApiInboxFetchTest.kt to use these start/end bounds rather than
recomputing currentRequestTimestampSeconds.
Comment on lines
+55
to
+59
| @Test | ||
| fun `sendInboxDelete targets the same inbox v2 path pending backend confirmation`() { | ||
| val url = ctApi.sendInboxDelete(body = "[]").request.url.toString() | ||
| assertContains(url, "/inbox/v2/getMessages") | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file and understand its context
find . -name "CtApiInboxFetchTest.kt" -type fRepository: CleverTap/clevertap-android-sdk
Length of output: 166
🏁 Script executed:
# Find the implementation of sendInboxDelete
rg -l "fun sendInboxDelete" --type kotlinRepository: CleverTap/clevertap-android-sdk
Length of output: 150
🏁 Script executed:
# Search for any pending backend confirmation mentions
rg -i "pending.*backend\|backend.*confirm" --type kotlinRepository: CleverTap/clevertap-android-sdk
Length of output: 57
🏁 Script executed:
# Look at the test file content
cat -n clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiInboxFetchTest.kt | head -100Repository: CleverTap/clevertap-android-sdk
Length of output: 3321
🏁 Script executed:
# Now let's find and read the CtApi.kt implementation to see sendInboxDelete
wc -l clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.ktRepository: CleverTap/clevertap-android-sdk
Length of output: 154
🏁 Script executed:
# Search for the sendInboxDelete implementation
rg -n "fun sendInboxDelete" clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt -A 5Repository: CleverTap/clevertap-android-sdk
Length of output: 369
🏁 Script executed:
# Search for sendInboxFetch implementation to compare
rg -n "fun sendInboxFetch" clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt -A 5Repository: CleverTap/clevertap-android-sdk
Length of output: 368
🏁 Script executed:
# Look for any TODO or comments about pending backend confirmation
rg -n "inbox.*delete\|delete.*inbox" clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt -i -B 2 -A 2Repository: CleverTap/clevertap-android-sdk
Length of output: 57
🏁 Script executed:
# Get more context around the sendInboxDelete implementation
sed -n '120,135p' clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.ktRepository: CleverTap/clevertap-android-sdk
Length of output: 828
🏁 Script executed:
# Get more context around sendInboxFetch implementation
sed -n '105,120p' clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.ktRepository: CleverTap/clevertap-android-sdk
Length of output: 635
🏁 Script executed:
# Search for any comments or documentation about the inbox endpoint
rg -n "inbox.*v2\|getMessages" clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt -B 3Repository: CleverTap/clevertap-android-sdk
Length of output: 57
🏁 Script executed:
# Check git history or comments around the implementation
git log --oneline --all -S "sendInboxDelete" | head -10Repository: CleverTap/clevertap-android-sdk
Length of output: 161
Remove or comment-out the sendInboxDelete endpoint assertion until backend confirms the contract.
The implementation itself contains an explicit TODO: "CONFIRM with backend before merging T3.2 — if it's actually a sibling path like inbox/v2/deleteMessage change this single literal." This acknowledges the endpoint is a working assumption pending confirmation, yet the test codifies it as a hard requirement. Once merged, any backend-driven endpoint change becomes blocked by the test. Either confirm the endpoint contract before merging or disable this assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiInboxFetchTest.kt`
around lines 55 - 59, The test `sendInboxDelete` currently asserts a specific
endpoint path using ctApi.sendInboxDelete and should not hard-fail on an
unconfirmed backend contract; remove or comment out the line asserting the URL
contains "/inbox/v2/getMessages" (the assertContains call in the
`sendInboxDelete` test) or replace it with a non-failing placeholder check
(e.g., only verify a non-null URL or skip the path assertion) and leave the
existing TODO note in place so the path check can be reinstated once the backend
contract is confirmed.
Comment on lines
+114
to
+120
| fun `null header from builder returns NetworkFailure without hitting network`() = runTest { | ||
| val http = MockHttpClient(responseCode = 200, responseBody = "should-not-be-read") | ||
|
|
||
| val result = newCall(http, header = null).execute() | ||
|
|
||
| assertTrue(result is CallResult.NetworkFailure) | ||
| } |
Contributor
There was a problem hiding this comment.
Make the “without hitting network” assertion distinguishable.
With responseBody = "should-not-be-read", this test still passes if the implementation accidentally sends the request, because malformed JSON also returns NetworkFailure. Use a success-shaped response so a network hit would fail the test.
🧪 Proposed test hardening
`@Test`
fun `null header from builder returns NetworkFailure without hitting network`() = runTest {
- val http = MockHttpClient(responseCode = 200, responseBody = "should-not-be-read")
+ val http = MockHttpClient(responseCode = 200, responseBody = """{"inbox_notifs_v2":[]}""")
val result = newCall(http, header = null).execute()
assertTrue(result is CallResult.NetworkFailure)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/InboxFetchCallTest.kt`
around lines 114 - 120, Test uses a failure-shaped response body so a real
network hit could still produce NetworkFailure; update the MockHttpClient
invocation in the test function `null header from builder returns NetworkFailure
without hitting network` (the call to `newCall(http, header = null).execute()`)
to use a success-shaped responseBody (e.g. a valid JSON/expected payload instead
of "should-not-be-read") with responseCode 200 so that any accidental network
hit would return success and make the assertion fail.
Hooks InboxV2Bridge.submit(false, null) into asyncProfileSwitchUser right after resetInbox() so the new identity's inbox loads immediately from the server instead of waiting on the next /a1 push. Throttle is bypassed — every identity switch is a fresh fetch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gives customers with custom inbox UIs a way to force an on-demand refresh from the CleverTap servers. Two overloads (no-arg and callback-taking) route through InboxV2Bridge.submit with respectThrottle=true, so rapid user-initiated calls can't spam the endpoint. Null inbox controller is handled defensively: the callback fires with false, no network call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps CTInboxListViewFragment's list in a SwipeRefreshLayout and routes the refresh gesture through the public fetchInbox API so the throttle applies and existing inboxMessagesDidUpdate path drives the adapter refresh. The outer wrapper delegates canChildScrollUp to whichever RecyclerView is actually on screen — mid-scroll upward drags scroll the list instead of falsely firing a refresh, which the default mTarget.canScrollVertically(-1) check on a LinearLayout target would otherwise do. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java`:
- Around line 2273-2286: The method fetchInbox currently calls
callback.onInboxFetched(false) synchronously when CTInboxController is null,
violating the documented contract that the callback fires on the SDK network
dispatcher thread; change the failure path in fetchInbox so the callback is
invoked asynchronously on the same SDK network dispatcher used for success
callbacks (i.e., use the same dispatcher/post mechanism used elsewhere in
CleverTapAPI for network callbacks) instead of calling it directly on the caller
thread, and ensure FetchInboxCallback javadoc remains consistent with this
behavior if you modify it.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/login/LoginControllerTest.kt`:
- Around line 224-238: The test's verifyOrder must assert the resetInbox()
boundary so submit(false, null) is verified to occur after inbox reset; update
the test for asyncProfileSwitchUser (the one invoking
loginController.asyncProfileSwitchUser) to include inboxV2Bridge.resetInbox() in
the verifyOrder sequence (e.g., verifyOrder {
pushProviders.forcePushDeviceToken(true); inboxV2Bridge.resetInbox();
inboxV2Bridge.submit(false, null);
controllerManager.inAppFCManager.changeUser(any()) }) so that moving submit
before resetInbox would fail the test.
🪄 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: b015dbb8-b9b6-442c-abe1-97161c38763e
📒 Files selected for processing (9)
clevertap-core/build.gradleclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxListViewFragment.javaclevertap-core/src/main/java/com/clevertap/android/sdk/login/LoginController.javaclevertap-core/src/main/res/layout/inbox_list_view.xmlclevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapAPITest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/login/LoginControllerTest.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (2)
- clevertap-core/build.gradle
- gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
Comment on lines
+2273
to
+2286
| * <p><b>Thread note:</b> the callback fires on the SDK's network | ||
| * dispatcher thread, not the main thread. Post back to main yourself if | ||
| * the callback touches UI. | ||
| * | ||
| * @param callback notified with {@code true} on success, {@code false} | ||
| * otherwise. May be {@code null}. | ||
| */ | ||
| @SuppressWarnings("unused") | ||
| public void fetchInbox(@Nullable final FetchInboxCallback callback) { | ||
| if (coreState.getControllerManager().getCTInboxController() == null) { | ||
| getConfigLogger().debug(getAccountId(), | ||
| "Notification Inbox not initialized — call initializeInbox() first"); | ||
| if (callback != null) { | ||
| callback.onInboxFetched(false); |
Contributor
There was a problem hiding this comment.
Keep the callback thread contract consistent.
Line 2273 documents that the callback fires on the SDK network dispatcher thread, but Line 2286 invokes it synchronously on the caller thread when the inbox is not initialized. Either dispatch this failure callback asynchronously through the same documented path, or explicitly document this immediate-failure exception in both this API and FetchInboxCallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java`
around lines 2273 - 2286, The method fetchInbox currently calls
callback.onInboxFetched(false) synchronously when CTInboxController is null,
violating the documented contract that the callback fires on the SDK network
dispatcher thread; change the failure path in fetchInbox so the callback is
invoked asynchronously on the same SDK network dispatcher used for success
callbacks (i.e., use the same dispatcher/post mechanism used elsewhere in
CleverTapAPI for network callbacks) instead of calling it directly on the caller
thread, and ensure FetchInboxCallback javadoc remains consistent with this
behavior if you modify it.
Comment on lines
+224
to
+238
| @Test | ||
| fun `asyncProfileSwitchUser submits V2 fetch after resetInbox and before inAppFCManager changeUser`() { | ||
| val profile = mapOf("Name" to "John Doe") | ||
| mockkStatic(CTExecutorFactory::class) { | ||
| every { CTExecutorFactory.executors(any()) } returns MockCTExecutors(cleverTapInstanceConfig) | ||
|
|
||
| loginController.asyncProfileSwitchUser(profile, "12345", "54321") | ||
| } | ||
|
|
||
| verifyOrder { | ||
| pushProviders.forcePushDeviceToken(true) | ||
| inboxV2Bridge.submit(false, null) | ||
| controllerManager.inAppFCManager.changeUser(any()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make this ordering test assert the actual resetInbox() boundary.
The test name says submit happens after resetInbox, but the verifyOrder only proves it happens after forcePushDeviceToken(true). Add the inbox reset calls so moving submit(false, null) before resetInbox() would fail the test.
Proposed test tightening
verifyOrder {
pushProviders.forcePushDeviceToken(true)
+ controllerManager.setCTInboxController(null)
+ controllerManager.initializeInbox()
inboxV2Bridge.submit(false, null)
controllerManager.inAppFCManager.changeUser(any())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| fun `asyncProfileSwitchUser submits V2 fetch after resetInbox and before inAppFCManager changeUser`() { | |
| val profile = mapOf("Name" to "John Doe") | |
| mockkStatic(CTExecutorFactory::class) { | |
| every { CTExecutorFactory.executors(any()) } returns MockCTExecutors(cleverTapInstanceConfig) | |
| loginController.asyncProfileSwitchUser(profile, "12345", "54321") | |
| } | |
| verifyOrder { | |
| pushProviders.forcePushDeviceToken(true) | |
| inboxV2Bridge.submit(false, null) | |
| controllerManager.inAppFCManager.changeUser(any()) | |
| } | |
| } | |
| `@Test` | |
| fun `asyncProfileSwitchUser submits V2 fetch after resetInbox and before inAppFCManager changeUser`() { | |
| val profile = mapOf("Name" to "John Doe") | |
| mockkStatic(CTExecutorFactory::class) { | |
| every { CTExecutorFactory.executors(any()) } returns MockCTExecutors(cleverTapInstanceConfig) | |
| loginController.asyncProfileSwitchUser(profile, "12345", "54321") | |
| } | |
| verifyOrder { | |
| pushProviders.forcePushDeviceToken(true) | |
| controllerManager.setCTInboxController(null) | |
| controllerManager.initializeInbox() | |
| inboxV2Bridge.submit(false, null) | |
| controllerManager.inAppFCManager.changeUser(any()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/login/LoginControllerTest.kt`
around lines 224 - 238, The test's verifyOrder must assert the resetInbox()
boundary so submit(false, null) is verified to occur after inbox reset; update
the test for asyncProfileSwitchUser (the one invoking
loginController.asyncProfileSwitchUser) to include inboxV2Bridge.resetInbox() in
the verifyOrder sequence (e.g., verifyOrder {
pushProviders.forcePushDeviceToken(true); inboxV2Bridge.resetInbox();
inboxV2Bridge.submit(false, null);
controllerManager.inAppFCManager.changeUser(any()) }) so that moving submit
before resetInbox would fail the test.
…2.1) Inserts data.getMessageId() under evtData._id in pushInboxMessageStateEvent so backend can map Viewed/Clicked events to a specific inbox message and update server-side isRead. Without this field, cross-device read-state sync has no way to land. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (T2.2) Introduces a generic per-key sliding-window EventSuppressor backed by ConcurrentHashMap.put so two suppressors (Viewed 2s, Clicked 5s) can gate pushInboxMessageStateEvent without new locks. Closes the public-API dedup gap: customers building custom inboxes can't inflate analytics by firing Viewed on every scroll-into-view or double-tapping a message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2.3) Adds an early-return guard at the top of pushInboxMessageStateEvent: if the message is already read (V2 delivered isRead=true from another device), suppress the Viewed event to avoid cross-device duplicate view counts. Clicked semantics are unchanged — distinct taps on separate devices remain distinct engagement events. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ence (T3.1) Adds two SQLite tables — inbox_pending_deletes and inbox_pending_reads — keyed by composite (USER_ID, ID) so a local delete/read intent survives app kill and a later V2 fetch can't resurrect a deleted message or overwrite a locally-read message until the server confirms the action. The DB migrates from v6 to v7 additively (no alteration of inboxMessages). The DAO mirrors existing conventions: belowMemThreshold guard, transaction- wrapped batch insert with CONFLICT_IGNORE, parameterized IN batch delete, cursor.use for reads, SQLiteException catches on writes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records a row in inbox_pending_reads whenever the user marks a message read, so a V2 fetch with a stale isRead=false can't un-read the message across app restart. processV2Response now reads both pending sets for real (replacing the T1.4 empty-set placeholder) and, once a server echo confirms a pending read, batch-removes the row. The cleanup runs BEFORE preWriteFilter because the merger's override mutates incoming DAOs in place and would otherwise make every pending id look server-confirmed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…T3.2)
Adds an action-only EndpointCall<Unit> (InboxDeleteCall) and a coordinator
that batches N deletes into one HTTP call, clears inbox_pending_deletes
rows atomically on 2xx, and drains leftover pending rows at inbox-init
time so a delete that failed offline eventually lands. Local intent is
recorded before the server call so an app kill mid-sync can't lose it.
Three literals are captured behind working assumptions, each flippable
with a one-line edit once backend confirms: delete URL path, event name
("Message Deleted"), and the messages array container key.
DBAdapter is fetched via a supplier so the coordinator's first DB load
happens on the NetworkScope's IO dispatcher instead of the factory's
main-thread construction path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java (1)
1019-1028:⚠️ Potential issue | 🟠 MajorPrevent custom data from overriding the message
_id.Line 1020 writes the backend mapping key before merging
customData, so a bundle containing_idoverwrites the real message ID and can corrupt viewed/clicked attribution. Write_idafter custom data, or skip reserved keys while merging.🐛 Proposed fix
JSONObject notif = getWzrkFields(data); - notif.put("_id", msgId); if (customData != null) { for (String x : customData.keySet()) { Object value = customData.get(x); if (value != null) { notif.put(x, value); } } } + + notif.put("_id", msgId); if (clicked) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java` around lines 1019 - 1028, The code currently writes notif.put("_id", msgId) before merging customData, allowing a customData entry named "_id" to overwrite the real message id; update the merge logic in the method that builds the notif JSONObject (uses getWzrkFields, notif, customData, msgId) so that either (a) you merge customData first and then call notif.put("_id", msgId), or (b) when iterating customData.keySet() skip reserved keys like "_id" (and any other backend-reserved keys) to prevent overwriting; ensure the reserved-key check is applied where notif.put(x, value) is called.
🧹 Nitpick comments (3)
clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java (1)
69-70: Use the injected clock for inbox suppressors.These fields currently use
Clock.SYSTEMeven thoughAnalyticsManageralready receivescurrentTimeProvider. Initializing them in the constructor keeps this path consistent with existing dedupe logic and makes suppression expiry testable without real time.♻️ Proposed refactor
- private final EventSuppressor inboxViewedSuppressor = new EventSuppressor(2_000L); - private final EventSuppressor inboxClickedSuppressor = new EventSuppressor(5_000L); + private final EventSuppressor inboxViewedSuppressor; + private final EventSuppressor inboxClickedSuppressor; ... this.currentTimeProvider = currentTimeProvider; + this.inboxViewedSuppressor = new EventSuppressor(2_000L, currentTimeProvider); + this.inboxClickedSuppressor = new EventSuppressor(5_000L, currentTimeProvider); this.executors = executors;Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java` around lines 69 - 70, The inboxViewedSuppressor and inboxClickedSuppressor fields are being created with the system clock instead of the injected clock; change their initialization so they are constructed inside the AnalyticsManager constructor using the currentTimeProvider (or its Clock) rather than at field declaration. Locate the EventSuppressor usages (inboxViewedSuppressor, inboxClickedSuppressor) and move their new EventSuppressor(2_000L) and EventSuppressor(5_000L) calls into the constructor, passing the injected time source from currentTimeProvider so suppression expiry is testable and consistent with existing dedupe logic.clevertap-core/src/main/java/com/clevertap/android/sdk/db/DBAdapter.kt (1)
53-53: LGTM — consistent with existing DAO-delegation pattern.Minor nit only: the new
List<String>?signatures differ from the existingdeleteMessagesForIDs(List<String?>?, ...)/markReadMessagesForIds(List<String?>?, ...)convention (which tolerates null elements and filters). Since the current call sites inCTInboxController/InboxDeleteCoordinatorpass non-null-element lists, this is purely a consistency consideration and can be deferred.Also applies to: 138-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/db/DBAdapter.kt` at line 53, The new DAO methods use nullable-list-of-nonnull-signatures (List<String>?) which is inconsistent with the existing convention (List<String?>?) used by deleteMessagesForIDs and markReadMessagesForIds; update the DAO interface and implementation (e.g., InboxPendingActionsDAO / InboxPendingActionsDAOImpl methods referenced at the new lazy val inboxPendingActionsDAO and the methods around lines 138-206) to accept List<String?>? instead, and inside the implementations filter out null elements (e.g., .filterNotNull()) before processing so call sites in CTInboxController/InboxDeleteCoordinator remain compatible and behavior stays unchanged.clevertap-core/src/main/java/com/clevertap/android/sdk/db/dao/InboxPendingActionsDAO.kt (1)
160-178: Optional: chunkdeleteBatchagainst SQLite's host-param limit.
IN ($placeholders)uses one?per messageId plus one foruserId. SQLite's defaultSQLITE_MAX_VARIABLE_NUMBERis 999 (historically) / 32766 (newer builds), so a very large pending-delete set could throw. In practice pending-delete sets are small, so this is only a defensive note — feel free to defer.♻️ Optional: chunk into safe-sized batches
private fun deleteBatch(table: String, messageIds: List<String>, userId: String): Boolean { if (messageIds.isEmpty()) return true - val placeholders = buildString { - append("?") - repeat(messageIds.size - 1) { append(", ?") } - } - val args = (messageIds + userId).toTypedArray() - return try { - dbHelper.writableDatabase.delete( - table, - "${Column.ID} IN ($placeholders) AND ${Column.USER_ID} = ?", - args - ) - true - } catch (e: SQLiteException) { - logger.verbose("Error batch-deleting from $table", e) - false - } + return try { + messageIds.chunked(500).forEach { chunk -> + val placeholders = List(chunk.size) { "?" }.joinToString(", ") + val args = (chunk + userId).toTypedArray() + dbHelper.writableDatabase.delete( + table, + "${Column.ID} IN ($placeholders) AND ${Column.USER_ID} = ?", + args + ) + } + true + } catch (e: SQLiteException) { + logger.verbose("Error batch-deleting from $table", e) + false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/db/dao/InboxPendingActionsDAO.kt` around lines 160 - 178, The deleteBatch function can exceed SQLite's host parameter limit when messageIds is very large; modify deleteBatch to process messageIds in chunks (e.g., maxParams = 900 or a safe constant), loop over messageIds.chunked(chunkSize) and for each chunk build its own placeholders and args (chunk + userId), call dbHelper.writableDatabase.delete with the same "${Column.ID} IN ($placeholders) AND ${Column.USER_ID} = ?" filter, and aggregate the result so that the method returns false if any chunk delete throws an SQLiteException (log the exception with logger.verbose as currently done) and true only if all chunk deletes succeed. Ensure unique symbols referenced: deleteBatch, Column.ID, Column.USER_ID, dbHelper.writableDatabase.delete, and logger.verbose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCall.kt`:
- Around line 72-75: The code in InboxDeleteCall.kt creates a JSONObject with
obj.put("_id", m.messageId) then copies m.wzrkParams into obj, which allows a
params entry named "_id" to overwrite the intended message id; update the
merging logic in the block that iterates m.wzrkParams so it either skips any key
equal to "_id" or ensures obj.put("_id", m.messageId) is applied after merging,
i.e., when copying params in the lambda for m.wzrkParams (the
params.keys().forEach loop) explicitly ignore the "_id" key (or re-set
obj.put("_id", m.messageId) after the loop) to preserve the CTInboxMessage
messageId.
- Around line 54-60: The current response handling in InboxDeleteCall (the when
block that checks response.code) only treats 200 as success and returns
CallResult.HttpError for other 2xx codes; update the logic in the method that
handles the HTTP response (the when on response.code) to treat any 2xx status as
CallResult.Success(Unit) — e.g., check response.code in 200..299 or use
response.isSuccessful — keep the 403 branch returning CallResult.Disabled and
preserve the else branch for non-2xx/403 errors.
- Around line 8-10: The catch-all exception handlers in InboxDeleteCall are
swallowing coroutine cancellations; add an import for
kotlinx.coroutines.CancellationException and update the two catch blocks inside
the InboxDeleteCall (the Exception handlers around the network call) to rethrow
cancellations before handling other errors (e.g., if (e is
CancellationException) throw e or add a specific catch (CancellationException) {
throw } before the broad catch(Exception)). This ensures cooperative
cancellation is preserved while still converting other exceptions to
NetworkFailure.
---
Outside diff comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java`:
- Around line 1019-1028: The code currently writes notif.put("_id", msgId)
before merging customData, allowing a customData entry named "_id" to overwrite
the real message id; update the merge logic in the method that builds the notif
JSONObject (uses getWzrkFields, notif, customData, msgId) so that either (a) you
merge customData first and then call notif.put("_id", msgId), or (b) when
iterating customData.keySet() skip reserved keys like "_id" (and any other
backend-reserved keys) to prevent overwriting; ensure the reserved-key check is
applied where notif.put(x, value) is called.
---
Nitpick comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java`:
- Around line 69-70: The inboxViewedSuppressor and inboxClickedSuppressor fields
are being created with the system clock instead of the injected clock; change
their initialization so they are constructed inside the AnalyticsManager
constructor using the currentTimeProvider (or its Clock) rather than at field
declaration. Locate the EventSuppressor usages (inboxViewedSuppressor,
inboxClickedSuppressor) and move their new EventSuppressor(2_000L) and
EventSuppressor(5_000L) calls into the constructor, passing the injected time
source from currentTimeProvider so suppression expiry is testable and consistent
with existing dedupe logic.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/db/dao/InboxPendingActionsDAO.kt`:
- Around line 160-178: The deleteBatch function can exceed SQLite's host
parameter limit when messageIds is very large; modify deleteBatch to process
messageIds in chunks (e.g., maxParams = 900 or a safe constant), loop over
messageIds.chunked(chunkSize) and for each chunk build its own placeholders and
args (chunk + userId), call dbHelper.writableDatabase.delete with the same
"${Column.ID} IN ($placeholders) AND ${Column.USER_ID} = ?" filter, and
aggregate the result so that the method returns false if any chunk delete throws
an SQLiteException (log the exception with logger.verbose as currently done) and
true only if all chunk deletes succeed. Ensure unique symbols referenced:
deleteBatch, Column.ID, Column.USER_ID, dbHelper.writableDatabase.delete, and
logger.verbose.
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/db/DBAdapter.kt`:
- Line 53: The new DAO methods use nullable-list-of-nonnull-signatures
(List<String>?) which is inconsistent with the existing convention
(List<String?>?) used by deleteMessagesForIDs and markReadMessagesForIds; update
the DAO interface and implementation (e.g., InboxPendingActionsDAO /
InboxPendingActionsDAOImpl methods referenced at the new lazy val
inboxPendingActionsDAO and the methods around lines 138-206) to accept
List<String?>? instead, and inside the implementations filter out null elements
(e.g., .filterNotNull()) before processing so call sites in
CTInboxController/InboxDeleteCoordinator remain compatible and behavior stays
unchanged.
🪄 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: 7baa8d64-12ed-4f36-843b-2397f2ba5d6c
📒 Files selected for processing (18)
clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.ktclevertap-core/src/main/java/com/clevertap/android/sdk/db/CtDatabase.ktclevertap-core/src/main/java/com/clevertap/android/sdk/db/DBAdapter.ktclevertap-core/src/main/java/com/clevertap/android/sdk/db/dao/InboxPendingActionsDAO.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxController.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/EventSuppressor.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/InboxDeleteCoordinator.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCall.ktclevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/MockCoreState.ktclevertap-core/src/test/java/com/clevertap/android/sdk/db/dao/InboxPendingActionsDAOImplTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/CTInboxControllerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/EventSuppressorTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/inbox/InboxDeleteCoordinatorTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCallTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.kt
- clevertap-core/src/test/java/com/clevertap/android/sdk/inbox/CTInboxControllerTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxController.java
Comment on lines
+8
to
+10
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.Dispatchers | ||
| import kotlinx.coroutines.withContext |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify InboxDeleteCall preserves coroutine cancellation before broad Exception catches.
# Expected after the fix: both broad Exception catch blocks are preceded by CancellationException branches.
python - <<'PY'
from pathlib import Path
for path in Path(".").rglob("InboxDeleteCall.kt"):
print(f"--- {path} ---")
for lineno, line in enumerate(path.read_text().splitlines(), 1):
if "CancellationException" in line or "catch (e: Exception)" in line:
print(f"{lineno}: {line}")
PYRepository: CleverTap/clevertap-android-sdk
Length of output: 244
🏁 Script executed:
cat -n clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCall.ktRepository: CleverTap/clevertap-android-sdk
Length of output: 4075
Rethrow coroutine cancellation instead of converting it to NetworkFailure.
Both exception catch blocks (lines 48 and 63) use broad Exception catches that swallow CancellationException, preventing cooperative cancellation of the calling coroutine scope.
Add CancellationException import and re-throw it explicitly before the broad Exception catches:
Proposed fix
import kotlinx.coroutines.CoroutineDispatcher
+import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
@@
- } catch (e: Exception) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
return@withContext CallResult.NetworkFailure(e)
}
@@
- } catch (e: Exception) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
logger.verbose("InboxV2", "delete failed: $e")
CallResult.NetworkFailure(e)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import kotlinx.coroutines.CoroutineDispatcher | |
| import kotlinx.coroutines.Dispatchers | |
| import kotlinx.coroutines.withContext | |
| import kotlinx.coroutines.CoroutineDispatcher | |
| import kotlinx.coroutines.CancellationException | |
| import kotlinx.coroutines.Dispatchers | |
| import kotlinx.coroutines.withContext |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCall.kt`
around lines 8 - 10, The catch-all exception handlers in InboxDeleteCall are
swallowing coroutine cancellations; add an import for
kotlinx.coroutines.CancellationException and update the two catch blocks inside
the InboxDeleteCall (the Exception handlers around the network call) to rethrow
cancellations before handling other errors (e.g., if (e is
CancellationException) throw e or add a specific catch (CancellationException) {
throw } before the broad catch(Exception)). This ensures cooperative
cancellation is preserved while still converting other exceptions to
NetworkFailure.
Comment on lines
+54
to
+60
| when (response.code) { | ||
| 200 -> CallResult.Success(Unit) | ||
| 403 -> { | ||
| logger.info("InboxV2", "delete 403 — account not enabled") | ||
| CallResult.Disabled | ||
| } | ||
| else -> CallResult.HttpError(response.code, response.readBody()) |
Contributor
There was a problem hiding this comment.
Treat all HTTP 2xx delete responses as confirmed.
The PR behavior says pending deletes clear on 2xx, but this only accepts 200; 202/204 would be retried indefinitely as HttpError.
🐛 Proposed fix
- when (response.code) {
- 200 -> CallResult.Success(Unit)
- 403 -> {
+ when {
+ response.code in 200..299 -> CallResult.Success(Unit)
+ response.code == 403 -> {
logger.info("InboxV2", "delete 403 — account not enabled")
CallResult.Disabled
}
else -> CallResult.HttpError(response.code, response.readBody())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| when (response.code) { | |
| 200 -> CallResult.Success(Unit) | |
| 403 -> { | |
| logger.info("InboxV2", "delete 403 — account not enabled") | |
| CallResult.Disabled | |
| } | |
| else -> CallResult.HttpError(response.code, response.readBody()) | |
| when { | |
| response.code in 200..299 -> CallResult.Success(Unit) | |
| response.code == 403 -> { | |
| logger.info("InboxV2", "delete 403 — account not enabled") | |
| CallResult.Disabled | |
| } | |
| else -> CallResult.HttpError(response.code, response.readBody()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCall.kt`
around lines 54 - 60, The current response handling in InboxDeleteCall (the when
block that checks response.code) only treats 200 as success and returns
CallResult.HttpError for other 2xx codes; update the logic in the method that
handles the HTTP response (the when on response.code) to treat any 2xx status as
CallResult.Success(Unit) — e.g., check response.code in 200..299 or use
response.isSuccessful — keep the 403 branch returning CallResult.Disabled and
preserve the else branch for non-2xx/403 errors.
Comment on lines
+72
to
+75
| val obj = JSONObject().put("_id", m.messageId) | ||
| m.wzrkParams?.let { params -> | ||
| params.keys().forEach { key -> obj.put(key, params.get(key)) } | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent wzrkParams from overwriting the delete _id.
obj.put("_id", m.messageId) is followed by copying every wzrkParams key into the same object. If the payload contains _id, it replaces the target message id; CTInboxMessage.java lines 68-130 show these params are loaded directly from JSON.
🐛 Proposed fix
val obj = JSONObject().put("_id", m.messageId)
m.wzrkParams?.let { params ->
- params.keys().forEach { key -> obj.put(key, params.get(key)) }
+ params.keys().forEach { key ->
+ if (key != "_id") {
+ obj.put(key, params.get(key))
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val obj = JSONObject().put("_id", m.messageId) | |
| m.wzrkParams?.let { params -> | |
| params.keys().forEach { key -> obj.put(key, params.get(key)) } | |
| } | |
| val obj = JSONObject().put("_id", m.messageId) | |
| m.wzrkParams?.let { params -> | |
| params.keys().forEach { key -> | |
| if (key != "_id") { | |
| obj.put(key, params.get(key)) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/InboxDeleteCall.kt`
around lines 72 - 75, The code in InboxDeleteCall.kt creates a JSONObject with
obj.put("_id", m.messageId) then copies m.wzrkParams into obj, which allows a
params entry named "_id" to overwrite the intended message id; update the
merging logic in the block that iterates m.wzrkParams so it either skips any key
equal to "_id" or ensures obj.put("_id", m.messageId) is applied after merging,
i.e., when copying params in the lambda for m.wzrkParams (the
params.keys().forEach loop) explicitly ignore the "_id" key (or re-set
obj.put("_id", m.messageId) after the loop) to preserve the CTInboxMessage
messageId.
Cross-session persistence was redundant. Every process start runs an un-throttled V2 fetch (cold launch and onUserLogin submit with respectThrottle=false), and InboxV2Fetcher calls recordFetch() before the endpoint runs regardless of the caller. The stored timestamp is therefore overwritten before any throttled caller can read it, so a per-instance AtomicLong yields identical observable behavior without the disk I/O, Context, config, and prefKey plumbing. Drops INBOX_V2_LAST_FETCH_TS_KEY, removes Robolectric from FetchThrottleTest, and simplifies construction in CleverTapFactory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/FetchThrottleTest.kt (1)
31-38: Assert the exact throttle-window boundary.Advancing six minutes checks “well after” the five-minute window, but it would miss an off-by-one/comparison regression at exactly
windowMs.🧪 Proposed boundary-tightening change
fun `after windowMs elapsed, shouldThrottle is false`() { val clock = TestClock(60L * 60 * 1000) val t = inboxThrottle(clock) t.recordFetch() - clock.advanceTime(6L * 60 * 1000) + clock.advanceTime(Constants.INBOX_V2_THROTTLE_WINDOW_MS) assertFalse(t.shouldThrottle()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/FetchThrottleTest.kt` around lines 31 - 38, The test advances time by 6 minutes which misses an off-by-one at the precise throttle window boundary; change the test to advance exactly the configured window duration (use the same windowMs value, e.g., advanceTime(5L * 60 * 1000) or reference the window constant) after calling inboxThrottle(...) and t.recordFetch(), then assertFalse(t.shouldThrottle()) to verify the boundary behavior; locate TestClock, inboxThrottle(...), t.recordFetch(), clock.advanceTime(...), and t.shouldThrottle() in the test and update the advanceTime call to the exact window duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/FetchThrottleTest.kt`:
- Around line 11-20: The first-call behavior is wrong when the clock is at 0
because FetchThrottle treats 0 as a valid last-fetch time; change FetchThrottle
to use an explicit "not recorded" sentinel (e.g., NO_LAST_FETCH = -1) for the
last fetch timestamp and update the logic in shouldThrottle() (and any
last-fetch read/write paths) to return false when lastFetch == NO_LAST_FETCH so
a first call (including TestClock(0L)) is never throttled; adjust initialization
to set lastFetch to NO_LAST_FETCH instead of 0 and keep windowMs, clock, and
method names (FetchThrottle, shouldThrottle) unchanged.
---
Nitpick comments:
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/FetchThrottleTest.kt`:
- Around line 31-38: The test advances time by 6 minutes which misses an
off-by-one at the precise throttle window boundary; change the test to advance
exactly the configured window duration (use the same windowMs value, e.g.,
advanceTime(5L * 60 * 1000) or reference the window constant) after calling
inboxThrottle(...) and t.recordFetch(), then assertFalse(t.shouldThrottle()) to
verify the boundary behavior; locate TestClock, inboxThrottle(...),
t.recordFetch(), clock.advanceTime(...), and t.shouldThrottle() in the test and
update the advanceTime call to the exact window duration.
🪄 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: 897e13e2-9983-44c9-bff2-d6d0813aa059
📒 Files selected for processing (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/Constants.javaclevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/FetchThrottle.ktclevertap-core/src/test/java/com/clevertap/android/sdk/ConstantsTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/fetch/FetchThrottleTest.kt
✅ Files skipped from review due to trivial changes (1)
- clevertap-core/src/test/java/com/clevertap/android/sdk/ConstantsTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/fetch/FetchThrottle.kt
V1 and V2 inbox messages live in the same `inboxMessages` table but have divergent backend contracts: V2 events must carry `_id` and V2 deletes must sync to the v2 endpoint, while V1 must do neither. The new `InboxMessageSource` discriminator persists per-message on the DAO and a `source TEXT NOT NULL DEFAULT 'V1'` column (folded into the existing v7 migration — no version bump, branch is unreleased). `InboxResponse` tags parsed DAOs V1, `InboxV2Response` tags V2; `CTInboxController` exposes an `isV2Message(id)` helper so the gate sites in T5.2 / T5.3 can branch without reading source off the public `CTInboxMessage`. `CTInboxMessage` is intentionally untouched and `CTMessageDAO.toJSON()` does not include source, so the V1/V2 tag never reaches the public model via `getData()`. A regression test in `CTMessageDAOTest` asserts the JSON stays clean. No behavioral change yet; T5.2 and T5.3 consume the tag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backend rejects `_id` on Notification Viewed / Clicked events for V1 inbox messages. Gate the `_id` put in `AnalyticsManager.pushInboxMessageStateEvent()` on the T5.1 source tag via a private `isV2InboxMessage(msgId)` helper that consults `CTInboxController.isV2Message`. Null controller, null message id, and unknown ids all fall through to V1 behavior (no `_id` emitted). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backend does not handle V1 deletes, and V1 inbox messages have no server-side state to sync. Four sites in `CTInboxController` now gate on the T5.1 source tag: - `deleteInboxMessage` / `deleteInboxMessagesForIDs` only add to `inbox_pending_deletes` and invoke `InboxDeleteCoordinator.syncDelete` for V2 messages. V1 paths are local-only. - `_markReadForMessageWithId` / `_markReadForMessagesWithIds` only add to `inbox_pending_reads` for V2 messages. V1 markRead stays local. Batch paths (`deleteInboxMessagesForIDs`, `_markReadForMessagesWithIds`) iterate `messages` once under a single `messagesLock` with an id-set membership check — O(N+M) instead of O(N*M) per-id scans. Single-item paths read source directly off the DAO returned by `findMessageById`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 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.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
Tests