[SDK - 5236] [Android] Parse color fallback #895
Conversation
…y flow -> it causes app crash
✅ 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. |
WalkthroughAdds safe color-parsing utilities and integrates them into CTInAppNotification deserialization and JSON parsing to validate or fallback color string values. Changes
Sequence Diagram(s)sequenceDiagram
participant CT as CTInAppNotification
participant Utils as ColorUtils.Color / extensions
participant Parser as android.graphics.Color
CT->>Utils: toValidColorOrFallback(colorString, fallback)
alt colorString null/blank
Utils-->>CT: return fallback
else
Utils->>Parser: toColorInt(colorString)
alt parse success
Parser-->>Utils: colorInt
Utils-->>CT: return original colorString
else parse error
Parser-->>Utils: throws
Utils-->>CT: return fallback
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppBaseFullNativeFragment.kt(4 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppBasePartialNativeFragment.kt(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeCoverFragment.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeCoverImageFragment.kt(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeFooterFragment.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHalfInterstitialFragment.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHalfInterstitialImageFragment.kt(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHeaderFragment.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeInterstitialFragment.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeInterstitialImageFragment.kt(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/utils/ColorUtils.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-09T13:27:32.141Z
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#846
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHalfInterstitialImageFragment.kt:20-143
Timestamp: 2025-07-09T13:27:32.141Z
Learning: In the CleverTap Android SDK, CTLalit considers the CTInAppNativeHalfInterstitialImageFragment class as legacy code and prefers not to refactor it, even when the code has complex methods that could benefit from restructuring.
Applied to files:
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeFooterFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeCoverFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppBasePartialNativeFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHalfInterstitialFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeCoverImageFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeInterstitialImageFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeInterstitialFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHeaderFragment.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHalfInterstitialImageFragment.kt
📚 Learning: 2025-06-18T09:47:50.616Z
Learnt from: vasct
PR: CleverTap/clevertap-android-sdk#831
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeCoverImageFragment.kt:55-59
Timestamp: 2025-06-18T09:47:50.616Z
Learning: In CleverTap Android SDK in-app notification fragments, the `isHideCloseButton` flag has counterintuitive logic where `!isHideCloseButton` sets visibility to GONE and `isHideCloseButton` sets visibility to VISIBLE. This is intentional behavior preserved from the original Java implementation and should not be "fixed" during Kotlin conversion.
Applied to files:
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppBasePartialNativeFragment.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (7)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeInterstitialFragment.kt (2)
30-30: LGTM! Safe color parsing import added.The import of
toColorIntOrDefaultcorrectly replaces the previoustoColorIntimport, enabling defensive color parsing throughout this fragment.
87-87: Validate BLACK fallback UX
ThetoColorIntOrDefault()extension defaults toColor.BLACKon failure. Verify that using black as the fallback for backgrounds, titles, and messages doesn’t produce unreadable combinations (e.g., black text on a black background).clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppBaseFullNativeFragment.kt (1)
16-16: LGTM! Button color parsing now safely defaults.The import and usage of
toColorIntOrDefault()for button text color, background color, and border color provide appropriate fallback behavior, preventing crashes from invalid color values in button configurations.Also applies to: 29-29, 47-47, 66-66
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeCoverImageFragment.kt (1)
13-13: LGTM! Background color parsing now safely defaults.The import and usage of
toColorIntOrDefault()for the cover image background color appropriately prevents crashes from invalid color values.Also applies to: 31-31
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppBasePartialNativeFragment.kt (1)
19-19: LGTM! Button color parsing now safely defaults.The import and usage of
toColorIntOrDefault()for button text and background colors provide appropriate fallback behavior for partial fragment buttons (header/footer), preventing crashes from invalid color values.Also applies to: 115-116
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeHeaderFragment.kt (1)
16-16: LGTM! Header color parsing now safely defaults.The import and usage of
toColorIntOrDefault()for background, title, and message colors appropriately prevent crashes from invalid color values in header fragments.Also applies to: 32-32, 60-60, 64-64
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/fragment/CTInAppNativeFooterFragment.kt (1)
16-16: LGTM! Footer color parsing now safely defaults.The import and usage of
toColorIntOrDefault()for background, title, and message colors appropriately prevent crashes from invalid color values in footer fragments. The implementation is consistent with the pattern applied across all other in-app fragments in this PR.Also applies to: 31-31, 59-59, 63-63
clevertap-core/src/main/java/com/clevertap/android/sdk/utils/ColorUtils.kt
Outdated
Show resolved
Hide resolved
|
Code Coverage Debug
|
CTLalit
changed the title
| fl.findViewById<RelativeLayout>(R.id.interstitial_image_relative_layout) | ||
|
|
||
| relativeLayout.setBackgroundColor(inAppNotification.backgroundColor.toColorInt()) | ||
| relativeLayout.setBackgroundColor(inAppNotification.backgroundColor.toColorIntOrDefault()) |
There was a problem hiding this comment.
The default colour will be black here @SuriyaKannimuthu ?
Ideally it should be white
Will it be simpler to validate all the colours in CTInAppNotification.kt while parsing from the JSONObject
There was a problem hiding this comment.
Hi @Anush-Shand
I assigned black as the default based on a default assumption (since background is usually white). It’s configurable, so we can change it to white if needed. - Will change to white.
We can handle color validation directly in CTInAppNotification.kt while parsing the JSON — I initially thought of doing it that way since it’s simpler.
However, creating a utility to handle color parsing helps avoid manual null and format checks.
With the utility in place, all invalid or malformed color values are safely handled in one place. in future we can use the util method ".toColorIntOrDefault()"
White color default
| * @return The parsed color integer or [defaultColor] if parsing fails. | ||
| */ | ||
| @ColorInt | ||
| fun String?.toColorIntOrDefault(@ColorInt defaultColor: Int = Color.WHITE): Int { |
There was a problem hiding this comment.
Making default colour as white for everywhere won't work since the text might have default colour as black
Also we have defined default colours in CTInappNotification.kt
Again defining separate default colours in the extension function will be duplicate. In the future if I have to change the default colour I will have to change it in 2 places. A developer might miss this and hence could be erroneous
There was a problem hiding this comment.
Okay @Anush-Shand, Doing as per the comments and making changes considering the default colors in CTInappNotification.kt.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt (2)
196-199: Color validation in parcel deserialization looks good.The pattern
(parcel.readString() ?: existingColor).toValidColorOrFallback(existingColor)correctly handles null values and validates color strings, falling back to the existing default colors (BLACK for text, WHITE for background).Minor: Kotlin convention is to omit semicolons at the end of statements.
- titleColor = (parcel.readString() ?: titleColor).toValidColorOrFallback(titleColor); - backgroundColor = (parcel.readString() ?: backgroundColor).toValidColorOrFallback(backgroundColor); + titleColor = (parcel.readString() ?: titleColor).toValidColorOrFallback(titleColor) + backgroundColor = (parcel.readString() ?: backgroundColor).toValidColorOrFallback(backgroundColor)- messageColor = (parcel.readString() ?: messageColor).toValidColorOrFallback(messageColor); + messageColor = (parcel.readString() ?: messageColor).toValidColorOrFallback(messageColor)
356-372: Color validation in JSON deserialization looks good.The validation logic correctly ensures that invalid color strings from JSON fall back to the existing defaults, preventing crashes from malformed data.
Minor: Remove trailing semicolons per Kotlin conventions.
- backgroundColor = jsonObject.optString(Constants.KEY_BG, backgroundColor).toValidColorOrFallback(backgroundColor); + backgroundColor = jsonObject.optString(Constants.KEY_BG, backgroundColor).toValidColorOrFallback(backgroundColor)- titleColor = titleObject.optString(Constants.KEY_COLOR, titleColor).toValidColorOrFallback(titleColor); + titleColor = titleObject.optString(Constants.KEY_COLOR, titleColor).toValidColorOrFallback(titleColor)- messageColor = msgObject.optString(Constants.KEY_COLOR, messageColor).toValidColorOrFallback(messageColor); + messageColor = msgObject.optString(Constants.KEY_COLOR, messageColor).toValidColorOrFallback(messageColor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt(4 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/utils/ColorUtils.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T12:08:40.196Z
Learnt from: Anush-Shand
PR: CleverTap/clevertap-android-sdk#872
File: clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/styles/Style.kt:39-45
Timestamp: 2025-09-24T12:08:40.196Z
Learning: The Style.kt setNotificationBuilderBasics method in the CleverTap Android SDK push templates is legacy code. When flagging potential safety issues like NPE from Html.fromHtml(pt_title) with null values or Color.parseColor crashes with blank strings, the team prefers to defer such improvements rather than modify legacy code during feature-focused PRs.
Applied to files:
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt
📚 Learning: 2025-04-28T08:41:36.134Z
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#797
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.java:184-185
Timestamp: 2025-04-28T08:41:36.134Z
Learning: In the CleverTap Android SDK, parcels in the CTInAppNotification class exist only in memory and are not persisted, so backward compatibility concerns between different SDK versions are not applicable for these parcels.
Applied to files:
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt
📚 Learning: 2025-07-09T13:28:25.364Z
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#846
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt:422-432
Timestamp: 2025-07-09T13:28:25.364Z
Learning: CTLalit considers the CTInAppNotification class in the CleverTap Android SDK as legacy code and prefers not to refactor it, even when the code has potential improvements like media validation logic that could be more efficient.
Applied to files:
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (3)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/CTInAppNotification.kt (1)
14-14: LGTM!The import of
toValidColorOrFallbackis appropriate for the safe color validation added in this PR.clevertap-core/src/main/java/com/clevertap/android/sdk/utils/ColorUtils.kt (2)
24-39: LGTM!The
toColorIntOrDefaultextension function provides robust null-safe color parsing with appropriate exception handling for invalid formats.
58-69: LGTM!The
toValidColorOrFallbackextension function correctly validates color strings and returns the fallback when validation fails. This is the key function used byCTInAppNotificationto prevent crashes from malformed color data.
clevertap-core/src/main/java/com/clevertap/android/sdk/utils/ColorUtils.kt
Show resolved
Hide resolved
3 participants
Resolved app crashes caused by invalid or malformed color strings by introducing safe color parsing in ColorUtils. Now defaults to a fallback color instead of throwing exceptions.
Summary by CodeRabbit
Bug Fixes
New Features