App Inbox: add default media fallback when orientation is missing#530
Open
darshanclevertap wants to merge 2 commits intoSDK-5626-v7.6.0from
Open
App Inbox: add default media fallback when orientation is missing#530darshanclevertap wants to merge 2 commits intoSDK-5626-v7.6.0from
darshanclevertap wants to merge 2 commits intoSDK-5626-v7.6.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:
WalkthroughThis PR introduces a new default media layout feature for inbox message cells by adding enum variants, a new secondary media view property, helper methods to determine layout eligibility, and conditional logic that routes image rendering between standard and default views based on orientation detection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Contributor
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CleverTapSDK/Inbox/cells/CTCarouselImageMessageCell.m (1)
14-21:⚠️ Potential issue | 🟠 MajorPreserve the original content index when skipping empty media.
Lines 20-21 now drop empty-string media URLs, but Line 37 still tags the rendered card with the compacted index.
handleItemViewTapGesture:forwards that tag asindex, so any visible card after a skipped entry will resolve the wrongmessage.content[...]item on tap.💡 Suggested fix
- int index = 0; + NSInteger contentIndex = 0; int imageNumber = 1; for (CleverTapInboxMessageContent *content in (self.message.content)) { NSString *imageUrl = content.mediaUrl; NSString *actionUrl = content.actionUrl; NSString *imageDescription = content.mediaDescription ? content.mediaDescription : [NSString stringWithFormat:@"Message Image %d", imageNumber]; imageNumber = imageNumber + 1; if (imageUrl == nil || imageUrl.length == 0) { + contentIndex++; continue; } CTCarouselImageView *itemView; if (itemView == nil){ CGRect frame = self.carouselView.bounds; @@ UITapGestureRecognizer *itemViewTapGesture = [[UITapGestureRecognizer alloc] initWithTarget:self action:`@selector`(handleItemViewTapGesture:)]; itemView.userInteractionEnabled = YES; - itemView.tag = index; + itemView.tag = contentIndex; [itemView addGestureRecognizer:itemViewTapGesture]; [self.itemViews addObject:itemView]; - index++; + contentIndex++; }Also applies to: 35-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CleverTapSDK/Inbox/cells/CTCarouselImageMessageCell.m` around lines 14 - 21, The loop that builds carousel image views currently skips entries with empty mediaUrl but compacts the index used as the view tag, causing handleItemViewTapGesture: to resolve the wrong CleverTapInboxMessageContent from self.message.content; fix by preserving the original content index (e.g., use an index variable like originalIndex or capture the for-loop index) and assign that original index to the view.tag even when you continue for empty media, making sure any counters used for layout (visibleImageCount) remain separate from the contentIndex used for tags and lookup in handleItemViewTapGesture:.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CleverTapSDK/Inbox/cells/CTCarouselMessageCell.m`:
- Around line 41-44: populateLandscapeViews and populateItemViews compress the
loop index when skipping media-less contents, but handleItemViewTapGesture
expects each view.tag to equal the original index in self.message.content; this
causes mis-mapped taps/analytics. Fix by preserving the original content index
when assigning view.tag (e.g., use an originalIndex variable or the loop index
over self.message.content) and only skip adding views for empty mediaUrls while
still setting tag = originalIndex; apply this change in both
populateLandscapeViews and populateItemViews (and the other mentioned blocks
around lines 68-73, 91-93, 110-115) so tags always map to the source content
array index.
In `@CleverTapSDK/Inbox/cells/CTInboxBaseMessageCell.m`:
- Around line 632-639: resetDefaultMediaView currently clears only
defaultMediaHeightConstraint but the shared imageViewHeightConstraint (the
outlet promoted at lines referencing imageViewHeightConstraint) keeps a fallback
constant so reused text-only cells retain stale blank space; update
resetDefaultMediaView to either reset/restore the original constant on
imageViewHeightConstraint (store the original constant when promoting it, then
set it back and reactivate/deactivate appropriately) or separate sizing by
keeping defaultMediaHeightConstraint exclusively for default media and never
mutating imageViewHeightConstraint elsewhere (ensure methods that set the
fallback height target defaultMediaHeightConstraint). Also apply the same fix
pattern to the other similar methods in the 642-683 range that manipulate media
sizing so all paths restore or isolate the shared constraint.
---
Outside diff comments:
In `@CleverTapSDK/Inbox/cells/CTCarouselImageMessageCell.m`:
- Around line 14-21: The loop that builds carousel image views currently skips
entries with empty mediaUrl but compacts the index used as the view tag, causing
handleItemViewTapGesture: to resolve the wrong CleverTapInboxMessageContent from
self.message.content; fix by preserving the original content index (e.g., use an
index variable like originalIndex or capture the for-loop index) and assign that
original index to the view.tag even when you continue for empty media, making
sure any counters used for layout (visibleImageCount) remain separate from the
contentIndex used for tags and lookup in handleItemViewTapGesture:.
🪄 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: 29b6ad35-89e6-4124-a9a5-ad427e600e31
📒 Files selected for processing (9)
CleverTapSDK/Inbox/cells/CTCarouselImageMessageCell.mCleverTapSDK/Inbox/cells/CTCarouselMessageCell.mCleverTapSDK/Inbox/cells/CTInboxBaseMessageCell.hCleverTapSDK/Inbox/cells/CTInboxBaseMessageCell.mCleverTapSDK/Inbox/cells/CTInboxIconMessageCell.mCleverTapSDK/Inbox/cells/CTInboxSimpleMessageCell.mCleverTapSDK/Inbox/controllers/CleverTapInboxViewController.mCleverTapSDK/Inbox/views/CTCarouselImageView.hCleverTapSDK/Inbox/views/CTCarouselImageView.m
Comment on lines
41
to
+44
| for (CleverTapInboxMessageContent *content in (self.message.content)) { | ||
| if (content.mediaUrl.length == 0) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep tag mapped to the source content index in both carousel builders.
The new skip paths for empty media URLs compact the local index, but handleItemViewTapGesture: later treats tag as the original message.content index. After a skipped item, taps and analytics will be sent for the wrong carousel entry in both populateLandscapeViews and populateItemViews.
💡 Suggested fix
- NSUInteger index = 0;
+ NSUInteger contentIndex = 0;
int imageNumber = 1;
for (CleverTapInboxMessageContent *content in (self.message.content)) {
if (content.mediaUrl.length == 0) {
+ contentIndex++;
continue;
}
CTCarouselImageView *carouselItemView;
if (carouselItemView == nil){
carouselItemView = [[[CTUIUtils bundle] loadNibNamed: NSStringFromClass([CTCarouselImageView class]) owner:nil options:nil] lastObject];
@@
UITapGestureRecognizer *carouselViewTapGesture = [[UITapGestureRecognizer alloc] initWithTarget:self action:`@selector`(handleItemViewTapGesture:)];
carouselItemView.userInteractionEnabled = YES;
- carouselItemView.tag = index;
+ carouselItemView.tag = contentIndex;
[carouselItemView addGestureRecognizer:carouselViewTapGesture];
[self.itemViews addObject:carouselItemView];
- index++;
+ contentIndex++;
}Apply the same pattern in populateItemViews.
Also applies to: 68-73, 91-93, 110-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CleverTapSDK/Inbox/cells/CTCarouselMessageCell.m` around lines 41 - 44,
populateLandscapeViews and populateItemViews compress the loop index when
skipping media-less contents, but handleItemViewTapGesture expects each view.tag
to equal the original index in self.message.content; this causes mis-mapped
taps/analytics. Fix by preserving the original content index when assigning
view.tag (e.g., use an originalIndex variable or the loop index over
self.message.content) and only skip adding views for empty mediaUrls while still
setting tag = originalIndex; apply this change in both populateLandscapeViews
and populateItemViews (and the other mentioned blocks around lines 68-73, 91-93,
110-115) so tags always map to the source content array index.
Comment on lines
+632
to
+639
| - (void)resetDefaultMediaView { | ||
| [self.defaultCellImageView sd_cancelCurrentImageLoad]; | ||
| self.defaultCellImageView.image = nil; | ||
| self.defaultCellImageView.hidden = YES; | ||
| if (self.defaultMediaHeightConstraint) { | ||
| self.defaultMediaHeightConstraint.constant = 0; | ||
| self.defaultMediaHeightConstraint.active = NO; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the fallback height off the shared imageViewHeightConstraint.
Lines 654-655 promote the existing outlet constraint, and Line 674 overwrites its constant, but resetDefaultMediaView only clears defaultMediaHeightConstraint. After reuse, a text-only or standard-layout cell can inherit the previous fallback height and render with stale blank space. Please keep the default-media sizing on its own constraint, or restore the original constant during reset.
Also applies to: 642-683
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CleverTapSDK/Inbox/cells/CTInboxBaseMessageCell.m` around lines 632 - 639,
resetDefaultMediaView currently clears only defaultMediaHeightConstraint but the
shared imageViewHeightConstraint (the outlet promoted at lines referencing
imageViewHeightConstraint) keeps a fallback constant so reused text-only cells
retain stale blank space; update resetDefaultMediaView to either reset/restore
the original constant on imageViewHeightConstraint (store the original constant
when promoting it, then set it back and reactivate/deactivate appropriately) or
separate sizing by keeping defaultMediaHeightConstraint exclusively for default
media and never mutating imageViewHeightConstraint elsewhere (ensure methods
that set the fallback height target defaultMediaHeightConstraint). Also apply
the same fix pattern to the other similar methods in the 642-683 range that
manipulate media sizing so all paths restore or isolate the shared constraint.
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
Bug Fixes
New Features