feat: Add default ImageView for App Inbox when media orientation is missing#972
Open
darshanclevertap wants to merge 5 commits intotask/release/8.1.0from
Open
feat: Add default ImageView for App Inbox when media orientation is missing#972darshanclevertap wants to merge 5 commits intotask/release/8.1.0from
darshanclevertap wants to merge 5 commits intotask/release/8.1.0from
Conversation
✅ 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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 hidden fallback/default ImageView elements to carousel, icon, and simple inbox layouts and wires them into adapter and view-holder logic to handle non-"l"/"p" orientations; also adjusts media/video sizing calculations to use the new default image dimensions or aspect-ratio fallbacks. Additionally adds a custom-event dialog and related ViewModel changes in the sample app. Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as CTCarouselViewPagerAdapter
participant Layout as defaultImageView/Layout
participant ViewHolder as InboxViewHolder
participant ImageLoader as Glide
Adapter->>Layout: inflate / findViewById(`defaultImageView`)
Adapter->>ViewHolder: addImageAndSetClick(`defaultImageView`, media)
ViewHolder->>ImageLoader: load(media.url) { scaleType: FIT_CENTER }
ImageLoader-->>ViewHolder: image/thumbnail or fallback
ViewHolder-->>Layout: setImageDrawable / setVisibility(VISIBLE)
ViewHolder-->>Adapter: attach view to container
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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTCarouselViewPagerAdapter.java (1)
81-81: Minor: Redundant scale type assignment.The
scaleTypeis already defined asfitCenterin the XML layout. The programmatic assignment here is redundant but harmless. You can remove this line if desired, or keep it for explicit clarity.♻️ Optional cleanup
} else { ImageView imageView = view.findViewById(R.id.defaultImageView); - imageView.setScaleType(ImageView.ScaleType.FIT_CENTER); addImageAndSetClick(imageView, view, position, container); }🤖 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/CTCarouselViewPagerAdapter.java` at line 81, The line imageView.setScaleType(ImageView.ScaleType.FIT_CENTER) in CTCarouselViewPagerAdapter is redundant because the ImageView's scaleType is already set to fitCenter in the XML; remove that programmatic assignment to clean up the code (or, if you prefer explicitness, leave it)—locate the call to imageView.setScaleType in the CTCarouselViewPagerAdapter class and delete it if opting for the cleanup.
🤖 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/CTInboxBaseMessageViewHolder.java`:
- Around line 97-102: In CTInboxBaseMessageViewHolder.addMediaPlayer(), guard
against defaultImage being null before calling
getMeasuredWidth()/getMeasuredHeight(): wrap the dereferences of
this.defaultImage in a null check and only use its measured dimensions when
non-null, otherwise fall back to the existing resources-based width/height
computation; apply the same null-guard to both the landscape and portrait
branches so CTCarouselImageViewHolder/CTCarouselMessageViewHolder callers won’t
trigger a NullPointerException if they haven’t bound defaultImage.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTSimpleMessageViewHolder.java`:
- Around line 459-465: The fallback branch currently sets height to
RelativeLayout.LayoutParams.WRAP_CONTENT, which only sizes to the spinner
instead of the media area; change the else branch in CTSimpleMessageViewHolder
(the block using inboxMessage.getOrientation()) to set height = width (or
another fixed media-height calculation consistent with the portrait case) so the
progress frame overlays the full media area, and make the identical change in
the mirrored block in CTIconMessageViewHolder (replace WRAP_CONTENT with the
same calculated height variable).
- Around line 164-165: When binding a new message in CTSimpleMessageViewHolder,
clear the recycled ImageView's accessibility label so a previous item's media
description isn't announced for the current fallback image: when you call
defaultImage.setVisibility(View.GONE) / defaultImage.setBackgroundColor(...)
also call defaultImage.setContentDescription(null) (or set to an appropriate new
description) to reset the content description; apply the same change in the
other bind branch referenced around the second occurrence (the lines
corresponding to the 359-360 block) so reused holders never retain stale content
descriptions.
---
Nitpick comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTCarouselViewPagerAdapter.java`:
- Line 81: The line imageView.setScaleType(ImageView.ScaleType.FIT_CENTER) in
CTCarouselViewPagerAdapter is redundant because the ImageView's scaleType is
already set to fitCenter in the XML; remove that programmatic assignment to
clean up the code (or, if you prefer explicitness, leave it)—locate the call to
imageView.setScaleType in the CTCarouselViewPagerAdapter class and delete it if
opting for the cleanup.
🪄 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: e5ed75f0-19f2-44df-ac20-d1831d3cf3c3
📒 Files selected for processing (10)
clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTCarouselViewPagerAdapter.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTIconMessageViewHolder.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxBaseMessageViewHolder.javaclevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTSimpleMessageViewHolder.javaclevertap-core/src/main/res/layout-land/inbox_carousel_image_layout.xmlclevertap-core/src/main/res/layout-land/inbox_icon_message_layout.xmlclevertap-core/src/main/res/layout-land/inbox_simple_message_layout.xmlclevertap-core/src/main/res/layout/inbox_carousel_image_layout.xmlclevertap-core/src/main/res/layout/inbox_icon_message_layout.xmlclevertap-core/src/main/res/layout/inbox_simple_message_layout.xml
Comment on lines
+164
to
+165
| this.defaultImage.setVisibility(View.GONE); | ||
| this.defaultImage.setBackgroundColor(Color.parseColor(inboxMessage.getBgColor())); |
Contributor
There was a problem hiding this comment.
Reset defaultImage's content description during reuse.
A recycled holder can otherwise announce the previous item's custom media description when the next fallback-orientation row doesn't provide one.
♿ Suggested fix
this.defaultImage.setVisibility(View.GONE);
this.defaultImage.setBackgroundColor(Color.parseColor(inboxMessage.getBgColor()));
+ this.defaultImage.setContentDescription(
+ context.getString(R.string.ct_inbox_media_content_description));Also applies to: 359-360
🤖 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/CTSimpleMessageViewHolder.java`
around lines 164 - 165, When binding a new message in CTSimpleMessageViewHolder,
clear the recycled ImageView's accessibility label so a previous item's media
description isn't announced for the current fallback image: when you call
defaultImage.setVisibility(View.GONE) / defaultImage.setBackgroundColor(...)
also call defaultImage.setContentDescription(null) (or set to an appropriate new
description) to reset the content description; apply the same change in the
other bind branch referenced around the second occurrence (the lines
corresponding to the 359-360 block) so reused holders never retain stale content
descriptions.
Comment on lines
+459
to
+465
| if (inboxMessage.getOrientation().equalsIgnoreCase("l")) { | ||
| height = Math.round(width * 0.5625f); | ||
| } else if (inboxMessage.getOrientation().equalsIgnoreCase("p")) { | ||
| height = width; | ||
| } else { | ||
| height = RelativeLayout.LayoutParams.WRAP_CONTENT; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't size the fallback progress frame with WRAP_CONTENT.
That only wraps the spinner, so buffering audio/video in the missing-orientation path won't overlay the media area. The mirrored block in CTIconMessageViewHolder needs the same fix.
🛠️ Suggested fix
} else {
width = resources.getDisplayMetrics().widthPixels;
if (inboxMessage.getOrientation().equalsIgnoreCase("l")) {
height = Math.round(width * 0.5625f);
} else if (inboxMessage.getOrientation().equalsIgnoreCase("p")) {
height = width;
} else {
- height = RelativeLayout.LayoutParams.WRAP_CONTENT;
+ height = this.defaultImage.getMeasuredHeight();
+ if (height == 0) {
+ height = Math.round(width * 0.5625f);
+ }
}
}🤖 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/CTSimpleMessageViewHolder.java`
around lines 459 - 465, The fallback branch currently sets height to
RelativeLayout.LayoutParams.WRAP_CONTENT, which only sizes to the spinner
instead of the media area; change the else branch in CTSimpleMessageViewHolder
(the block using inboxMessage.getOrientation()) to set height = width (or
another fixed media-height calculation consistent with the portrait case) so the
progress frame overlays the full media area, and make the identical change in
the mirrored block in CTIconMessageViewHolder (replace WRAP_CONTENT with the
same calculated height variable).
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Anush-Shand
approved these changes
Apr 1, 2026
Contributor
Anush-Shand
left a comment
Anush-Shand
left a comment
There was a problem hiding this comment.
Approved with small comments
| } | ||
| } | ||
| break; | ||
| default: |
Contributor
There was a problem hiding this comment.
Duplicate code with CTIconMessageViewHolder.java
| android:layout_gravity="center" | ||
| android:adjustViewBounds="true" | ||
| android:scaleType="fitCenter" | ||
| android:src="@drawable/ct_audio" |
Contributor
There was a problem hiding this comment.
Is this default src intentional?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt (1)
29-29: Expose immutableLiveDatainstead of publicMutableLiveData.The Fragment directly mutates this property (line 118 of HomeScreenFragment.kt:
viewModel.showCustomEventDialog.value = false), breaking unidirectional data flow and allowing the UI layer to control ViewModel state. The ViewModel should be the sole owner and mutator of its own state.Suggested refactor
+import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData @@ - val showCustomEventDialog: MutableLiveData<Boolean> by lazy { MutableLiveData<Boolean>() } + private val _showCustomEventDialog = MutableLiveData<Boolean>() + val showCustomEventDialog: LiveData<Boolean> get() = _showCustomEventDialog @@ - 9 -> showCustomEventDialog.value = true + 9 -> _showCustomEventDialog.value = trueAdd a ViewModel method to handle external reset requests instead of exposing mutable state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt` at line 29, Replace the public MutableLiveData with a private MutableLiveData and expose it as immutable LiveData from HomeScreenViewModel (keep the existing showCustomEventDialog symbol but make it privateMutableShowCustomEventDialog and add a public val showCustomEventDialog: LiveData<Boolean>); stop letting fragments set .value directly and instead add a ViewModel method (e.g., hideCustomEventDialog() or resetShowCustomEventDialog()) that updates the private MutableLiveData, and update callers (HomeScreenFragment) to call that method rather than writing to the MutableLiveData.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenFragment.kt`:
- Around line 145-153: The dialog currently uses
AlertDialog.Builder.setPositiveButton which auto-dismisses regardless of your
early return; change to create the AlertDialog via
AlertDialog.Builder(...).create() and after calling show(), attach an
OnShowListener and override the positive button click with
dialog.getButton(AlertDialog.BUTTON_POSITIVE).setOnClickListener to validate
eventNameInput.text.toString().trim() and only call dialog.dismiss() when
non-empty (otherwise show the Toast), updating the code around the existing
AlertDialog.Builder / setPositiveButton usage to instead build, show, and wire
the custom click handler.
---
Nitpick comments:
In `@sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt`:
- Line 29: Replace the public MutableLiveData with a private MutableLiveData and
expose it as immutable LiveData from HomeScreenViewModel (keep the existing
showCustomEventDialog symbol but make it privateMutableShowCustomEventDialog and
add a public val showCustomEventDialog: LiveData<Boolean>); stop letting
fragments set .value directly and instead add a ViewModel method (e.g.,
hideCustomEventDialog() or resetShowCustomEventDialog()) that updates the
private MutableLiveData, and update callers (HomeScreenFragment) to call that
method rather than writing to the MutableLiveData.
🪄 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: 5a95f88b-eb2d-4325-bc46-671ff1f3f2db
📒 Files selected for processing (3)
sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenFragment.ktsample/src/main/java/com/clevertap/demo/ui/main/HomeScreenModel.ktsample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenModel.kt
Comment on lines
+145
to
+153
| AlertDialog.Builder(ctx) | ||
| .setTitle("Push Custom Event") | ||
| .setView(layout) | ||
| .setPositiveButton("Push") { _, _ -> | ||
| val eventName = eventNameInput.text.toString().trim() | ||
| if (eventName.isEmpty()) { | ||
| Toast.makeText(ctx, "Event name cannot be empty", Toast.LENGTH_SHORT).show() | ||
| return@setPositiveButton | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In androidx.appcompat.app.AlertDialog, does setPositiveButton(...) always dismiss the dialog after click, even if the callback returns early?
💡 Result:
Yes. For androidx.appcompat.app.AlertDialog, buttons added via AlertDialog.Builder.setPositiveButton(...) will dismiss the dialog after a click regardless of what your callback does (there’s no “return true/false” to control it—the listener returns void). The AppCompat implementation invokes your OnClickListener and then posts a separate message that calls dismiss() unconditionally. [1], [2]
If you need to keep the dialog open (e.g., validation fails), create/show the dialog first and then replace the positive button’s click handler with your own View.OnClickListener (and call dialog.dismiss() only when you want). [1], [2]
Sources:
[1] Android Developers – Dialogs guide (auto-dismiss behavior) (developer.android.com)
[2] AppCompat AlertController source (always posts dismiss after handler) (android.googlesource.com)
Citations:
- 1: https://developer.android.com/guide/topics/ui/dialogs?utm_source=openai
- 2: https://android.googlesource.com/platform/frameworks/support/%2B/19eaf14/v7/appcompat/src/android/support/v7/app/AlertController.java
Prevent auto-dismiss when event name is invalid.
On line 150, empty input shows a toast but the dialog still dismisses, forcing users to reopen it to correct the input. The setPositiveButton callback uses return@setPositiveButton to exit early, but AlertDialog.Builder still posts a separate dismiss message regardless of the callback's behavior.
Suggested fix
- AlertDialog.Builder(ctx)
- .setTitle("Push Custom Event")
- .setView(layout)
- .setPositiveButton("Push") { _, _ ->
- val eventName = eventNameInput.text.toString().trim()
- if (eventName.isEmpty()) {
- Toast.makeText(ctx, "Event name cannot be empty", Toast.LENGTH_SHORT).show()
- return@setPositiveButton
- }
- val propertiesText = propertiesInput.text.toString().trim()
- val properties = parseProperties(propertiesText)
- viewModel.pushCustomEvent(eventName, properties)
- Toast.makeText(ctx, "Event '$eventName' pushed", Toast.LENGTH_SHORT).show()
- }
- .setNegativeButton("Cancel", null)
- .show()
+ val dialog = AlertDialog.Builder(ctx)
+ .setTitle("Push Custom Event")
+ .setView(layout)
+ .setPositiveButton("Push", null)
+ .setNegativeButton("Cancel", null)
+ .create()
+
+ dialog.setOnShowListener {
+ dialog.getButton(android.content.DialogInterface.BUTTON_POSITIVE).setOnClickListener {
+ val eventName = eventNameInput.text.toString().trim()
+ if (eventName.isEmpty()) {
+ Toast.makeText(ctx, "Event name cannot be empty", Toast.LENGTH_SHORT).show()
+ return@setOnClickListener
+ }
+ val properties = parseProperties(propertiesInput.text.toString().trim())
+ viewModel.pushCustomEvent(eventName, properties)
+ Toast.makeText(ctx, "Event '$eventName' pushed", Toast.LENGTH_SHORT).show()
+ dialog.dismiss()
+ }
+ }
+ dialog.show()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenFragment.kt` around
lines 145 - 153, The dialog currently uses AlertDialog.Builder.setPositiveButton
which auto-dismisses regardless of your early return; change to create the
AlertDialog via AlertDialog.Builder(...).create() and after calling show(),
attach an OnShowListener and override the positive button click with
dialog.getButton(AlertDialog.BUTTON_POSITIVE).setOnClickListener to validate
eventNameInput.text.toString().trim() and only call dialog.dismiss() when
non-empty (otherwise show the Toast), updating the code around the existing
AlertDialog.Builder / setPositiveButton usage to instead build, show, and wire
the custom click handler.
…issing
When the backend does not provide an orientation key ("l" or "p") in the
App Inbox payload, the SDK now falls back to a plain ImageView with
match_parent width and wrap_content height, rendering the image at its
natural aspect ratio using FIT_CENTER scale type. This supports the new
Personalized Media feature where customers paste arbitrary image URLs
without dimension validation.
https://claude.ai/code/session_01C7qvo8dKQfZ8tkbnWdyKFW
- Null-guard defaultImage in addMediaPlayer() to prevent NPE when called from carousel ViewHolders that don't bind defaultImage - Reset contentDescription on recycled defaultImage to prevent stale accessibility labels from previous messages - Replace WRAP_CONTENT with calculated 16:9 height for progress bar frame so the loading overlay properly covers the media area https://claude.ai/code/session_01C7qvo8dKQfZ8tkbnWdyKFW
Variables could be used before assignment in the else branch when defaultImage is null, causing "variable might not have been initialized" compiler errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…operties Adds a "Custom Event" entry in the Events section that opens an AlertDialog where users can enter an event name and optional key:value properties, then pushes the event via CleverTapAPI.pushEvent(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Make showCustomEventDialog a private MutableLiveData with a public LiveData getter, following standard MVVM encapsulation. Add resetShowCustomEventDialog() method so the fragment no longer writes directly to the MutableLiveData. https://claude.ai/code/session_01C7qvo8dKQfZ8tkbnWdyKFW
CTLalit
force-pushed
the
claude/add-media-orientation-default-ZPW6n
branch
from
621bbf1 to
080e7e7
Compare
piyush-kukadiya
approved these changes
Apr 7, 2026
Contributor
piyush-kukadiya
left a comment
piyush-kukadiya
left a comment
There was a problem hiding this comment.
code duplication for media handling in switch case between CTIconMessageViewHolder and CTSimpleMessageViewHolder
piyush-kukadiya
approved these changes
Apr 7, 2026
| android:layout_gravity="center" | ||
| android:adjustViewBounds="true" | ||
| android:scaleType="fitCenter" | ||
| android:src="@drawable/ct_audio" |
CTLalit
approved these changes
Apr 16, 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
6 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.
When the backend does not provide an orientation key ("l" or "p") in the App Inbox payload, the SDK now falls back to a plain ImageView with match_parent width and wrap_content height, rendering the image at its natural aspect ratio using FIT_CENTER scale type. This supports the new Personalized Media feature, where customers paste arbitrary image URLs without dimension validation.
Summary by CodeRabbit
Bug Fixes
New Features