Task/sdk 5612/live updates notification factory [WIP]#964
Open
Anush-Shand wants to merge 6 commits into
Open
Conversation
…le to configure a notification
Anush-Shand
changed the title
✅ 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 a public notification factory API ( Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant CleverTapAPI as CleverTapAPI
participant PushProviders as PushProviders
participant Factory as ICleverTapNotificationFactory
participant NotifMgr as NotificationManager
App->>CleverTapAPI: setNotificationFactory(customFactory)
CleverTapAPI->>CleverTapAPI: sNotificationFactory = customFactory
Note over App: Push received (extras include isForFactory=true)
App->>PushProviders: handlePush(extras)
PushProviders->>CleverTapAPI: getNotificationFactory()
CleverTapAPI-->>PushProviders: customFactory
PushProviders->>Factory: onCreateNotification(context, extras)
Factory->>Factory: build Notification / choose ID
Factory-->>PushProviders: NotificationResult(notification, id)
PushProviders->>NotifMgr: notify(id, notification)
PushProviders->>PushProviders: postNotificationRendered(context, extras)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 3
🤖 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 162-163: sNotificationFactory is not safely published across
threads; make its updates visible to background readers by changing the static
field to use a volatile reference or an AtomicReference and update
setNotificationFactory() to assign via that mechanism; ensure any other static
notification factory fields (the occurrences around the 3334-3340 region) are
treated the same so PushProviders._createNotification() and other background
threads always see the most recent factory.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/ICleverTapNotificationFactory.kt`:
- Around line 31-32: Update the KDoc on
ICleverTapNotificationFactory.onCreateNotification to remove the
guaranteed-background-thread statement and instead state that the callback may
be invoked on the caller's thread (it can be called synchronously from
CleverTapAPI.createNotification and renderPushNotificationOnCallerThread via
_createNotification), warn callers not to perform blocking I/O on the main
thread, and recommend offloading heavy work to a background thread or returning
a lightweight result.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java`:
- Around line 190-197: The code gates use of the registered notification factory
behind an undocumented payload flag extras.getString("isForFactory", ""), which
renders setNotificationFactory()/CleverTapAPI.getNotificationFactory()
ineffective; update the branch in PushProviders to either (a) remove the payload
gate so that when CleverTapAPI.getNotificationFactory() returns a non-null
ICleverTapNotificationFactory the code always calls
triggerNotificationFromFactory(context, extras, customFactory) and returns, or
(b) formalize the gate by replacing the ad-hoc "isForFactory" key with a
documented configuration (e.g., a public constant or SDK config flag) and use
that constant to decide routing; ensure references to extras, isForFactory,
ICleverTapNotificationFactory, CleverTapAPI.getNotificationFactory(),
triggerNotificationFromFactory, and setNotificationFactory are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bcae4853-7088-4aea-96aa-7a5d6fb3f628
📒 Files selected for processing (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.javaclevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/ICleverTapNotificationFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.javasample/src/main/java/com/clevertap/demo/CustomNotificationFactory.ktsample/src/main/java/com/clevertap/demo/MyApplication.kt
Comment on lines
+190
to
+197
| // todo - Update key based on implementation from BE | ||
| boolean isForCustomFactory = extras.getString("isForFactory", "").equalsIgnoreCase("true"); | ||
|
|
||
| ICleverTapNotificationFactory customFactory = CleverTapAPI.getNotificationFactory(); | ||
| if (customFactory != null && isForCustomFactory) { | ||
| triggerNotificationFromFactory(context, extras, customFactory); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Factory gating key usage:"
rg -n -C2 '"isForFactory"'
echo
echo "Factory registration/access points:"
rg -n -C2 'setNotificationFactory|getNotificationFactory'Repository: CleverTap/clevertap-android-sdk
Length of output: 4214
The factory path is currently hidden behind an undocumented payload key.
Registering a factory via setNotificationFactory() is not enough to activate it; this branch only executes when extras["isForFactory"] == "true". Since this key is never defined or populated anywhere in the codebase and the inline TODO indicates the contract is still pending backend implementation, the new public API is effectively inert. Clarify whether all eligible core pushes should route through the registered factory unconditionally, or document and stabilize this gate as a formal configuration before shipping.
🤖 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/pushnotification/PushProviders.java`
around lines 190 - 197, The code gates use of the registered notification
factory behind an undocumented payload flag extras.getString("isForFactory",
""), which renders
setNotificationFactory()/CleverTapAPI.getNotificationFactory() ineffective;
update the branch in PushProviders to either (a) remove the payload gate so that
when CleverTapAPI.getNotificationFactory() returns a non-null
ICleverTapNotificationFactory the code always calls
triggerNotificationFromFactory(context, extras, customFactory) and returns, or
(b) formalize the gate by replacing the ad-hoc "isForFactory" key with a
documented configuration (e.g., a public constant or SDK config flag) and use
that constant to decide routing; ensure references to extras, isForFactory,
ICleverTapNotificationFactory, CleverTapAPI.getNotificationFactory(),
triggerNotificationFromFactory, and setNotificationFactory are updated
accordingly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (1)
3325-3342: Call out that this factory is process-global.Because the backing field and accessors are
static, this registration applies across allCleverTapAPIinstances in the app process, not per account/instance. A short note in the Javadoc would make that contract clearer for multi-instance integrations.✏️ Suggested doc tweak
- * Sets a factory that gives the client complete control over creating push notifications. + * Sets a process-wide factory that gives the client complete control over creating push notifications. * When set, the SDK will invoke {`@link` ICleverTapNotificationFactory#onCreateNotification} * instead of its built-in renderers for core push notifications. * * <p>The client is responsible for creating the notification channel, building the * {`@link` android.app.Notification}, and choosing the notification ID. The SDK still * handles push deduplication, analytics, and displaying the notification.</p> + * + * <p>This registration is global and applies to all CleverTap SDK instances in the + * current app process.</p>🤖 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 3325 - 3342, The Javadoc should explicitly state that the notification factory registration is process-global because the backing field sNotificationFactory and the accessors setNotificationFactory/getNotificationFactory are static; update the method docs for setNotificationFactory (and mirror in getNotificationFactory) to add a concise note that the provided ICleverTapNotificationFactory will apply across all CleverTapAPI instances in the app process (not per account/instance) so multi-instance integrations are aware of the global scope.
🤖 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/CleverTapAPI.java`:
- Around line 3325-3342: The Javadoc should explicitly state that the
notification factory registration is process-global because the backing field
sNotificationFactory and the accessors
setNotificationFactory/getNotificationFactory are static; update the method docs
for setNotificationFactory (and mirror in getNotificationFactory) to add a
concise note that the provided ICleverTapNotificationFactory will apply across
all CleverTapAPI instances in the app process (not per account/instance) so
multi-instance integrations are aware of the global scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c5cd770-1028-4873-bbbf-9f85e90ff9dd
📒 Files selected for processing (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java
Code Coverage Debug
Files
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/ICleverTapNotificationFactory.kt (1)
28-35:⚠️ Potential issue | 🟠 MajorDocument the callback's threading contract explicitly.
This KDoc still leaves the execution context ambiguous. For a public callback that may tempt implementors to fetch assets or do other blocking work, the docs should state whether it can run on the caller/main thread and warn against blocking it there.
📝 Minimal KDoc fix
/** * Called when a CleverTap push notification needs to be rendered. + * This callback runs on the same thread that triggers notification rendering. + * Avoid blocking work on the main thread. If you need I/O, dispatch it + * off-thread before returning a [NotificationResult]. * * `@param` context The application context. * `@param` extras The notification payload bundle containing all CleverTap keys. * `@return` A [NotificationResult] containing the built notification and notification ID,#!/bin/bash # Verify how ICleverTapNotificationFactory.onCreateNotification is reached # and whether invocation is synchronous on caller-owned threads. rg -n -C4 'onCreateNotification\s*\(' --glob '*.kt' --glob '*.java' printf '\n--- notification rendering entry points ---\n' rg -n -C4 'createNotification\s*\(|renderPushNotificationOnCallerThread\s*\(|_createNotification\s*\(' --glob '*.kt' --glob '*.java'🤖 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/pushnotification/ICleverTapNotificationFactory.kt` around lines 28 - 35, Update the KDoc for ICleverTapNotificationFactory.onCreateNotification to explicitly state its threading contract: indicate whether the callback is invoked on the main/UI thread or on a background thread, warn implementors not to perform blocking I/O or long-running work on that thread, and advise using background threads/async APIs (or provided helper methods) for fetching assets or heavy processing; reference the entry points that call it (e.g., createNotification, renderPushNotificationOnCallerThread, _createNotification) so reviewers can verify invocation context and adjust implementation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/ICleverTapNotificationFactory.kt`:
- Around line 28-35: Update the KDoc for
ICleverTapNotificationFactory.onCreateNotification to explicitly state its
threading contract: indicate whether the callback is invoked on the main/UI
thread or on a background thread, warn implementors not to perform blocking I/O
or long-running work on that thread, and advise using background threads/async
APIs (or provided helper methods) for fetching assets or heavy processing;
reference the entry points that call it (e.g., createNotification,
renderPushNotificationOnCallerThread, _createNotification) so reviewers can
verify invocation context and adjust implementation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 01142dcb-1946-4979-9c15-dff80e4f83c8
📒 Files selected for processing (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/ICleverTapNotificationFactory.kt
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/pushnotification/PushProviders.java`:
- Around line 193-199: The current branch that checks isForCustomFactory returns
unconditionally when no factory exists, causing notifications to be dropped;
change the flow in the block that reads
isForCustomFactory/ICleverTapNotificationFactory by only returning if a non-null
customFactory was found and triggerNotificationFromFactory(context, extras,
customFactory) was invoked, otherwise allow execution to continue to the default
renderer path (i.e., remove the unconditional return and only return after
successful factory handling).
- Around line 193-198: The branch that returns after calling
triggerNotificationFromFactory skips SDK silent-push handling; update
PushProviders so that when isForCustomFactory is true it still performs the SDK
silent-push logic (call storeUninstallTimestamp() and
updatePingFrequencyIfNeeded(...) or reuse the existing silent-notification
handling path) before returning or, alternately, detect silent payloads first
and let the SDK handle them regardless of the custom factory flag; locate the
isForCustomFactory check and triggerNotificationFromFactory(...) call to add the
silent-push handling (storeUninstallTimestamp, updatePingFrequencyIfNeeded) or
reroute to the existing silent-notification branch prior to returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a14b9584-5970-4bf1-be0e-174ef16cc96a
📒 Files selected for processing (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java
Comment on lines
+193
to
+199
| if (isForCustomFactory) { | ||
| ICleverTapNotificationFactory customFactory = CleverTapAPI.getNotificationFactory(); | ||
| if (customFactory != null) { | ||
| triggerNotificationFromFactory(context, extras, customFactory); | ||
| } | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Fall back to the default renderer when no factory is registered.
If extras["isForFactory"] is "true" and CleverTapAPI.getNotificationFactory() returns null, the unconditional return drops the notification entirely. Only return after triggerNotificationFromFactory(...) actually runs; otherwise continue into the existing renderer.
💡 Minimal fix
if (isForCustomFactory) {
ICleverTapNotificationFactory customFactory = CleverTapAPI.getNotificationFactory();
if (customFactory != null) {
triggerNotificationFromFactory(context, extras, customFactory);
+ return;
}
- return;
}🤖 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/pushnotification/PushProviders.java`
around lines 193 - 199, The current branch that checks isForCustomFactory
returns unconditionally when no factory exists, causing notifications to be
dropped; change the flow in the block that reads
isForCustomFactory/ICleverTapNotificationFactory by only returning if a non-null
customFactory was found and triggerNotificationFromFactory(context, extras,
customFactory) was invoked, otherwise allow execution to continue to the default
renderer path (i.e., remove the unconditional return and only return after
successful factory handling).
Comment on lines
+193
to
+198
| if (isForCustomFactory) { | ||
| ICleverTapNotificationFactory customFactory = CleverTapAPI.getNotificationFactory(); | ||
| if (customFactory != null) { | ||
| triggerNotificationFromFactory(context, extras, customFactory); | ||
| } | ||
| return; |
Contributor
There was a problem hiding this comment.
Keep silent pushes on the SDK-owned path.
This branch runs before the existing empty-message //silent notification handling, so a payload with isForFactory=true and no message now skips storeUninstallTimestamp() / updatePingFrequencyIfNeeded(...) and gets handed to the client factory instead. That changes silent-push behavior and contradicts the interface contract that the SDK still handles silent push detection.
🤖 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/pushnotification/PushProviders.java`
around lines 193 - 198, The branch that returns after calling
triggerNotificationFromFactory skips SDK silent-push handling; update
PushProviders so that when isForCustomFactory is true it still performs the SDK
silent-push logic (call storeUninstallTimestamp() and
updatePingFrequencyIfNeeded(...) or reuse the existing silent-notification
handling path) before returning or, alternately, detect silent payloads first
and let the SDK handle them regardless of the custom factory flag; locate the
isForCustomFactory check and triggerNotificationFromFactory(...) call to add the
silent-push handling (storeUninstallTimestamp, updatePingFrequencyIfNeeded) or
reroute to the existing silent-notification branch prior to returning.
deeksha-rgb
approved these changes
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation