Task/sdk 4887/sample custom template + app_inbox #833
Conversation
…task/SDK-4887/sample-custom-template
|
Warning Rate limit exceeded@Anush-Shand has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
## Walkthrough
This update introduces Jetpack Compose UI integration and custom in-app messaging templates to the Android sample project. It adds new Compose-based activities, screens, dialogs, and ViewModels, updates the Gradle configuration to support Compose, and registers custom templates with CleverTap. The codebase now supports Compose-driven inbox and custom dialog interactions.
## Changes
| File(s) | Change Summary |
|--------------------------------------------------------------------------------------------|---------------|
| sample/build.gradle | Refactored to use the `plugins` block, enabled Compose build features, and added Compose and Glide dependencies. |
| sample/src/main/AndroidManifest.xml | Added `.ui.inbox.CustomInboxComposeActivity` activity declaration with Compose theme. |
| sample/src/main/java/com/clevertap/demo/HomeScreenActivity.kt | Integrated Compose overlay dialog, added ViewModels for custom templates and URL confirmation, and updated intent handling. |
| sample/src/main/java/com/clevertap/demo/MyApplication.kt | Registered custom in-app templates with CleverTap and performed minor code cleanups. |
| sample/src/main/java/com/clevertap/demo/ViewModelFactory.kt | Extended factory to support new Compose-related ViewModels. |
| sample/src/main/java/com/clevertap/demo/WebViewActivity.kt | Removed unused import, updated method signatures, and cleaned up annotations. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt | **New:** Defines custom templates, presenters, and argument handling for in-app messages and functions. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt | **New:** Implements a reusable Compose overlay dialog component and supporting UI elements. |
| sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxComposeActivity.kt | **New:** Adds a Compose-based inbox activity with ViewModel and Compose UI setup. |
| sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxScreen.kt | **New:** Implements a Compose screen for displaying and interacting with inbox messages. |
| sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModel.kt | **New:** ViewModel for managing inbox state and actions, implementing CleverTap inbox listener. |
| sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModelContract.kt | **New:** Interface defining the contract for inbox ViewModel behavior and properties. |
| sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenFragment.kt | Added logic to launch the new Compose inbox activity from a command. |
| sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenModel.kt | Added new features to the list data: Compose inbox and custom templates. |
| sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt | Added command handling for syncing templates and testing custom template dialog; added test dialog logic. |
| sample/src/main/res/layout/home_screen_activity.xml | Updated layout to include a ComposeView overlay for Compose dialogs. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt | **New:** ViewModel for managing custom interstitial dialog state and behavior in Compose. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModelProvider.kt | **New:** Singleton provider for custom template ViewModel, enabling access outside standard lifecycle. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt | **New:** Compose dialog for confirming URL actions with user interaction. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModel.kt | **New:** ViewModel for managing the state of the URL confirmation dialog. |
| sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModelProvider.kt | **New:** Singleton provider for OpenUrlConfirmViewModel, for use outside standard lifecycle. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant HomeScreenActivity
participant CustomTemplateViewModel
participant OpenUrlConfirmViewModel
participant ComposeView
participant OverlayDialog
participant OpenUrlConfirmDialog
User->>HomeScreenActivity: Triggers custom template action
HomeScreenActivity->>CustomTemplateViewModel: showDialog(...)
CustomTemplateViewModel->>ComposeView: Update dialog state
ComposeView->>OverlayDialog: Render dialog UI
User->>OverlayDialog: Clicks primary/secondary button
OverlayDialog->>CustomTemplateViewModel: Invoke callback/hideDialog()
CustomTemplateViewModel->>ComposeView: Update dialog state
User->>HomeScreenActivity: Triggers open URL confirm
HomeScreenActivity->>OpenUrlConfirmViewModel: showDialog(url, ...)
OpenUrlConfirmViewModel->>ComposeView: Update dialog state
ComposeView->>OpenUrlConfirmDialog: Render confirm dialog
User->>OpenUrlConfirmDialog: Clicks Yes/No
OpenUrlConfirmDialog->>OpenUrlConfirmViewModel: onConfirm/onCancel
OpenUrlConfirmViewModel->>ComposeView: Update dialog statesequenceDiagram
participant User
participant HomeScreenFragment
participant CustomInboxComposeActivity
participant CustomInboxViewModel
participant InboxScreen
User->>HomeScreenFragment: Selects "Open Compose Inbox Screen"
HomeScreenFragment->>CustomInboxComposeActivity: Start activity
CustomInboxComposeActivity->>CustomInboxViewModel: Initialize ViewModel
CustomInboxViewModel->>InboxScreen: Provide inbox data
InboxScreen->>User: Display inbox messages and actions
User->>InboxScreen: Click/View/Mark Read/Delete message
InboxScreen->>CustomInboxViewModel: Handle action
CustomInboxViewModel->>InboxScreen: Update state
Possibly related PRs
Suggested reviewers
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
|
Code Coverage Debug
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
sample/src/main/java/com/clevertap/demo/WebViewActivity.kt (1)
41-51: Improve type safety when accessing Bundle extrasThe current implementation has potential runtime issues when accessing Bundle values. The code assumes that
payload["pt_id"]returns a String andpayload["notificationId"]is an Int, but Bundle values are of typeAny?which could lead to ClassCastException or unexpected behavior.Consider applying this diff to improve type safety:
- val payload = intent.extras - if (payload?.containsKey("pt_id") == true && payload["pt_id"] =="pt_rating") - { - val nm = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager - nm.cancel(payload["notificationId"] as Int) - } - if (payload?.containsKey("pt_id") == true && payload["pt_id"] =="pt_product_display") - { - val nm = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager - nm.cancel(payload["notificationId"] as Int) - } + val payload = intent.extras + val ptId = payload?.getString("pt_id") + val notificationId = payload?.getInt("notificationId", -1) + + if (ptId != null && notificationId != -1 && (ptId == "pt_rating" || ptId == "pt_product_display")) { + val nm = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager + nm.cancel(notificationId) + }This approach:
- Uses type-safe Bundle methods (
getString,getInt)- Consolidates the duplicate notification cancellation logic
- Avoids potential ClassCastException risks
🧹 Nitpick comments (9)
sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxScreen.kt (1)
43-44: Consider using LazyColumn with items() extension for better performance.The current implementation using
items(count = ...)works but could be improved for better performance and type safety.-LazyColumn(Modifier.fillMaxWidth()) { - items(count = viewModel.inboxMessages.size) { index -> - val message = viewModel.inboxMessages[index] +LazyColumn(Modifier.fillMaxWidth()) { + items(viewModel.inboxMessages) { message ->This approach is more efficient and provides better item identity for animations.
sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxComposeActivity.kt (1)
22-29: Consider using the shared ViewModelFactory for consistency.While the inline factory works, using the shared
ViewModelFactorywould provide consistency across the app and better maintainability.-private val customInboxViewModel: CustomInboxViewModel by viewModels { - object : ViewModelProvider.Factory { - override fun <T : ViewModel> create(modelClass: Class<T>): T { - @Suppress("UNCHECKED_CAST") - return CustomInboxViewModel() as T - } - } -} +// Import the shared factory +private val viewModelFactory by lazy { ViewModelFactory(null) } +private val customInboxViewModel: CustomInboxViewModel by viewModels { viewModelFactory }However, this requires updating the shared
ViewModelFactoryto supportCustomInboxViewModelcreation.sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt (1)
779-807: Improve import organization and add parameter validation.The test method implementation is solid with good error handling and logging. Consider these improvements:
Move the import to the top of the file:
+import com.clevertap.demo.ui.customtemplates.CustomTemplateManagerAnd update the method:
private fun testCustomTemplateDialog() { - // Import the custom template manager and test directly try { - val customTemplateManager = com.clevertap.demo.ui.customtemplates.CustomTemplateManager - val viewModel = customTemplateManager.getViewModel() + val viewModel = CustomTemplateManager.getViewModel()sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt (2)
20-24: Consider externalizing long default message string.The default message string is quite long and embedded in the code. Consider moving it to a string resource for better maintainability and localization support.
stringArgument( "Message", - "This is a custom interstitial message that can be quite long and will be displayed in a scrollable view if needed. You can add multiple lines of text here and it will automatically scroll when the content exceeds the available space." + context.getString(R.string.default_custom_interstitial_message) ) // Message string with defaultThen add to
strings.xml:<string name="default_custom_interstitial_message">This is a custom interstitial message that can be quite long and will be displayed in a scrollable view if needed. You can add multiple lines of text here and it will automatically scroll when the content exceeds the available space.</string>
13-16: Add documentation for the public API function.The
createCustomTemplatesfunction is a public API that should be documented to explain its purpose and parameters.+/** + * Creates a set of custom templates for CleverTap in-app messaging. + * + * @param customInterPresenter Presenter for custom interstitial template + * @param copyToClipboardPresenter Presenter for clipboard copy functionality + * @return TemplatesSet containing all defined custom templates + */ fun createCustomTemplates( customInterPresenter: CustomInterstitialPresenter, copyToClipboardPresenter: CopyToClipboardPresenter ) = templatesSet(sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt (4)
48-48: Document the usage of experimental Glide Compose API.The
ExperimentalGlideComposeApiis being used without documentation about its stability implications.+// Note: Using ExperimentalGlideComposeApi - this API may change in future Glide versions +// Consider monitoring Glide releases for API stability updates @OptIn(ExperimentalGlideComposeApi::class)
83-83: Consider using relative sizing instead of fixed height fraction.Using
fillMaxHeight(0.7f)might not work well on all screen sizes, especially smaller screens or landscape orientations.- .fillMaxHeight(0.7f) + .fillMaxHeight() + .heightIn(max = 600.dp) // Maximum height with proper constraintsThis ensures the dialog doesn't exceed a reasonable maximum height while being more flexible for different screen sizes.
175-178: Improve parameter naming and add default parameter documentation.The
OverlayScreencomposable parameters could be better named and documented for clarity.+/** + * Screen-level composable that displays an overlay dialog with semi-transparent background. + * + * @param dialogConfig Configuration for the dialog content and callbacks + * @param showDialog Whether to show the dialog overlay + */ @Composable fun OverlayScreen( dialogConfig: OverlayDialogConfig? = null, - showDialog: Boolean = false, + showDialog: Boolean = false ) {
184-188: Extract overlay background color to theme or constants.The semi-transparent black overlay color should be configurable or use theme colors.
+private val OverlayBackgroundColor = Color.Black.copy(alpha = 0.5f) Box( modifier = Modifier .fillMaxSize() - .background(Color.Black.copy(alpha = 0.5f)) + .background(OverlayBackgroundColor) )Alternatively, use theme colors:
- .background(Color.Black.copy(alpha = 0.5f)) + .background(MaterialTheme.colorScheme.onSurface.copy(alpha = 0.5f))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
sample/build.gradle(4 hunks)sample/src/main/AndroidManifest.xml(1 hunks)sample/src/main/java/com/clevertap/demo/HomeScreenActivity.kt(6 hunks)sample/src/main/java/com/clevertap/demo/MyApplication.kt(5 hunks)sample/src/main/java/com/clevertap/demo/ViewModelFactory.kt(2 hunks)sample/src/main/java/com/clevertap/demo/WebViewActivity.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateViewModel.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateViewModelProvider.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxComposeActivity.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxScreen.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModel.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModelContract.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenFragment.kt(2 hunks)sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenModel.kt(2 hunks)sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt(2 hunks)sample/src/main/res/layout/home_screen_activity.xml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (35)
sample/src/main/java/com/clevertap/demo/WebViewActivity.kt (1)
39-39: LGTM: Intent parameter made non-nullableThe change from nullable to non-nullable
Intentparameter is correct since the Android framework guarantees thatonNewIntentwill never receive a null Intent.sample/build.gradle (4)
3-9: LGTM: Modern Gradle plugin configurationThe migration from
apply plugin:statements to thepluginsblock follows current Gradle best practices. Usingalias(libs.plugins.compose.compiler)for the Compose compiler plugin is also the recommended approach.
81-81: LGTM: Compose build features enabledEnabling Compose build features is required for Jetpack Compose functionality.
138-138: LGTM: Glide Compose integrationAdding Glide Compose integration enables seamless image loading in Compose components. The beta version is appropriate for sample/demo applications.
152-158: LGTM: Comprehensive Compose dependency setupThe Compose dependencies are well-structured:
- Uses Compose BOM for consistent versioning
- Includes essential Compose UI components
- Adds necessary lifecycle integration for ViewModels
- Follows recommended Compose dependency patterns
sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModelContract.kt (1)
5-16: LGTM: Well-designed view model contractThe interface provides a clean contract with:
- Essential read-only properties for inbox state
- Standard message interaction operations (click, view, mark read, delete)
- Proper use of CleverTap's CTInboxMessage type
- Clear, descriptive function names following Kotlin conventions
sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenFragment.kt (2)
38-38: LGTM: Import additionClean import addition for the new Compose inbox activity.
108-108: LGTM: Consistent command handlingThe new command "2-16" follows the existing pattern and properly launches the CustomInboxComposeActivity using an explicit intent. The command string corresponds to the menu structure.
sample/src/main/AndroidManifest.xml (1)
168-171: LGTM: Proper activity declarationThe activity declaration is correctly configured:
android:exported="false"is appropriate for internal activities- NoActionBar theme is suitable for Compose activities
- No unnecessary intent filters, maintaining security
sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenModel.kt (2)
37-38: LGTM: Inbox menu extensionThe addition of "Open Compose Inbox Screen" to the INBOX category properly extends the existing menu structure and aligns with the command handling in HomeScreenFragment.
115-115: LGTM: New custom templates categoryThe new "CUSTOM TEMPLATES" category with sync and test dialog options aligns with the PR's objective to introduce custom template functionality.
sample/src/main/res/layout/home_screen_activity.xml (1)
2-17: LGTM! Proper ComposeView integration for overlay functionality.The layout structure correctly implements the overlay pattern by:
- Maintaining the existing container FrameLayout for backward compatibility
- Adding ComposeView as a sibling for overlay rendering
- Using appropriate dimensions for full-screen coverage
This enables seamless integration of Compose UI elements on top of existing Views.
sample/src/main/java/com/clevertap/demo/ViewModelFactory.kt (2)
7-7: LGTM! Proper import addition.The import is correctly added for the new CustomTemplateViewModel class.
18-19: LGTM! Consistent ViewModel factory pattern.The CustomTemplateViewModel creation follows the established factory pattern:
- Proper type checking with
isAssignableFrom- Parameterless constructor call consistent with the ViewModel design
- Maintains consistency with existing factory structure
sample/src/main/java/com/clevertap/demo/MyApplication.kt (5)
6-6: LGTM! Proper import for ClipboardManager.The ClipboardManager import is correctly added to support the clipboard functionality in custom templates.
29-32: LGTM! Proper imports for custom template components.The imports are correctly structured for:
- Custom template presenters
- Template creation function
223-223: LGTM! Minor formatting improvement.Removed unnecessary semicolon improves code consistency.
257-257: LGTM! Simplified system service calls.The direct cast to
NotificationManageris more concise and maintains the same functionality.Also applies to: 261-261
74-81: ```shell
#!/bin/bashSearch for createCustomTemplates declaration and usage in Kotlin files
rg -n "fun createCustomTemplates" -C3 -- *.kt
rg -n "createCustomTemplates" -C3 -- *.ktLocate AndroidManifest.xml and display its permission entries
fd AndroidManifest.xml --exec sed -n '1,200p' {} ; | grep -i "permission"
</details> <details> <summary>sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxScreen.kt (2)</summary> `15-24`: **LGTM! Well-structured Composable function.** The function signature and layout structure follow Compose best practices: - Proper parameter ordering with modifier last - Clear responsibility separation - Appropriate use of Column for vertical layout --- `25-42`: **LGTM! Proper loading state handling.** The conditional UI rendering correctly handles the loading state with: - Clear loading message - Visual progress indicator - Proper spacing and layout </details> <details> <summary>sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxComposeActivity.kt (3)</summary> `31-31`: **LGTM! Proper experimental API usage.** The `@OptIn(ExperimentalMaterial3Api::class)` annotation is correctly used for the TopAppBar API. --- `35-58`: **LGTM! Well-structured Compose UI setup.** The Compose UI implementation follows best practices: - Proper theme wrapping with MaterialTheme - Correct Surface usage for background - Well-structured Scaffold with TopAppBar - Proper padding propagation to content --- `41-55`: To confirm that all interface members are implemented, let’s list every `override` in the ViewModel: ```shell #!/bin/bash # List all overrides in CustomInboxViewModel rg "override" sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModel.ktsample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateViewModelProvider.kt (2)
10-38: Well-implemented singleton pattern with proper thread safety.The double-checked locking pattern and synchronized methods ensure thread-safe access to the ViewModel instance.
14-16: ```shell
#!/bin/bashDisplay the contents of MyApplication.kt to verify ctInstance initialization
sed -n '1,200p' sample/src/main/java/com/clevertap/demo/MyApplication.kt
</details> <details> <summary>sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt (2)</summary> `32-33`: **LGTM - Event consolidation.** The change from multiple events to a single "Product viewed" event is clean and the commented line shows intentional removal. --- `759-768`: **Well-structured new template commands.** Both commands follow the existing pattern and provide clear functionality - syncing templates and testing dialog behavior. </details> <details> <summary>sample/src/main/java/com/clevertap/demo/HomeScreenActivity.kt (3)</summary> `52-55`: **Good ViewModel integration pattern.** The custom factory approach is consistent with the existing codebase and properly passes the CleverTap instance. --- `81-95`: **Excellent Compose integration with proper lifecycle management.** The `ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed` ensures proper cleanup, and the state observation pattern is well-implemented. --- `226-226`: **Verify Intent nullability change impact.** The parameter change from `Intent?` to `Intent` could be a breaking change if any callers pass null. ```shell #!/bin/bash # Check for any onNewIntent implementations or calls that might pass null rg -A 3 -B 3 "onNewIntent.*null" --type kotlin rg -A 3 "\.onNewIntent\(" --type kotlin ast-grep --pattern 'onNewIntent($_)'sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModel.kt (2)
16-32: Excellent Compose state management and initialization.The use of
mutableStateOfandmutableStateListOfwith proper encapsulation is well-implemented. The initialization pattern correctly sets up the listener and triggers inbox initialization.
38-49: Good coroutine usage for UI updates.Using
Dispatchers.Mainensures UI state updates occur on the correct thread. The lifecycle event handling is properly implemented.sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateViewModel.kt (2)
15-28: Well-designed state management with proper encapsulation.The data class approach for dialog state and the use of
mutableStateOfwith private setters follows Compose best practices.
37-86: Excellent auto-close implementation with proper job management.The coroutine-based auto-close functionality is well-implemented with proper job cancellation and user interaction handling.
sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxScreen.kt
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxScreen.kt
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/inbox/CustomInboxViewModel.kt
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateViewModel.kt
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt
Outdated
Show resolved
Hide resolved
Anush-Shand
requested review from
nzagorchev,
piyush-kukadiya,
CTLalit and
vasct
Anush-Shand
changed the base branch from
develop
to
task/SDK-4886/custom_inbox_sample_app
Anush-Shand
changed the base branch from
task/SDK-4886/custom_inbox_sample_app
to
develop
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt (3)
63-63: Add null safety checks for ViewModel access.The ViewModel retrieval should include null safety checks and error handling to prevent potential crashes.
134-138: Add error handling for clipboard operations.The clipboard operation lacks error handling and could cause crashes if the operation fails.
88-90: Incomplete onClose implementation - missing ViewModel interaction.The
onClosemethod only callscontext.setDismissed()but doesn't hide the dialog through the ViewModel, which could leave the dialog visible after the template is closed.override fun onClose(context: CustomTemplateContext.TemplateContext) { + try { + val viewModel = CustomTemplateManager.getViewModel() + viewModel.hideDialog() + } catch (e: Exception) { + // Log error but continue with dismissal + } context.setDismissed() }
🧹 Nitpick comments (4)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt (1)
36-36: Use theme colors instead of hardcoded color values.The dialog uses hardcoded colors that should be replaced with theme colors for better consistency and dark mode support.
- containerColor = Color.White + containerColor = MaterialTheme.colorScheme.surfaceConsider replacing other hardcoded colors with appropriate theme colors like
MaterialTheme.colorScheme.primary,MaterialTheme.colorScheme.onSurface, etc.Also applies to: 74-74, 80-80, 102-102, 121-121
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModel.kt (1)
29-29: Remove unnecessary empty line.) { - dialogState = OpenUrlConfirmDialogState(sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (2)
19-19: Remove empty default constructor.The empty constructor serves no purpose and can be removed as flagged by static analysis.
-class CustomTemplateViewModel() : ViewModel() { +class CustomTemplateViewModel : ViewModel() {
32-42: Consider refactoring method with too many parameters.The
showDialogmethod has 9 parameters, which makes it difficult to use and maintain. Consider using a data class or builder pattern.data class DialogConfig( val title: String, val message: String, val imageUrl: String? = null, val primaryButtonText: String = "Continue", val secondaryButtonText: String = "Close", val autoCloseAfterSeconds: Int = 0, val onPrimaryAction: () -> Unit = {}, val onSecondaryAction: () -> Unit = {}, val onAutoClose: () -> Unit = {} ) fun showDialog(config: DialogConfig) { // Implementation using config properties }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
sample/src/main/java/com/clevertap/demo/HomeScreenActivity.kt(6 hunks)sample/src/main/java/com/clevertap/demo/MyApplication.kt(7 hunks)sample/src/main/java/com/clevertap/demo/ViewModelFactory.kt(2 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModelProvider.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModel.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModelProvider.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- sample/src/main/java/com/clevertap/demo/MyApplication.kt
- sample/src/main/java/com/clevertap/demo/ViewModelFactory.kt
- sample/src/main/java/com/clevertap/demo/HomeScreenActivity.kt
🧰 Additional context used
🪛 detekt (1.23.8)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt
[warning] 19-19: An empty default constructor can be removed.
(detekt.empty-blocks.EmptyDefaultConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (5)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModelProvider.kt (1)
14-16: I’ll inspect the declaration and initialization ofctInstancein MyApplication to confirm if it can ever remain null:#!/bin/bash # Show ctInstance declaration and nearby context rg "ctInstance" -n -C5 sample/src/main/java/com/clevertap/demo/MyApplication.kt # Show where onCreate sets up ctInstance rg "override fun onCreate" -n -C5 sample/src/main/java/com/clevertap/demo/MyApplication.kt # Locate buildCtInstance implementation rg "fun buildCtInstance" -n -C5 sample/src/main/java/com/clevertap/demo/MyApplication.ktsample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModelProvider.kt (1)
14-16: Add null safety check for MyApplication.ctInstance.Same issue as in
OpenUrlConfirmViewModelProvider- the ViewModelFactory initialization could fail ifMyApplication.ctInstanceis null.Consider adding the same null check pattern for consistency:
private val viewModelFactory by lazy { - ViewModelFactory(MyApplication.ctInstance) + ViewModelFactory(MyApplication.ctInstance ?: throw IllegalStateException("Application instance not initialized")) }sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModel.kt (1)
15-67: Well-structured ViewModel implementation.The ViewModel follows Android architecture patterns correctly with proper state management using Compose's
mutableStateOf, appropriate cleanup inonCleared(), and good separation of concerns.sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt (2)
1-14: LGTM! Clean imports and package structure.The imports are well-organized and include all necessary dependencies for the custom template functionality.
15-51: Well-structured template definitions with comprehensive arguments.The template and function definitions are properly configured with appropriate argument types, default values, and presenter assignments. The visual/non-visual distinction is correctly implemented.
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt
Show resolved
Hide resolved
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomTemplateDefinitions.kt
Show resolved
Hide resolved
…6/custom_inbox_sample_app
…ple_app task(SDK-4886) - UI and integration for custom inbox in the sample app
Anush-Shand
changed the title
…late' into task/SDK-4887/sample-custom-template
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (1)
92-98: Remove manual viewModelScope cancellation.The built-in
viewModelScopeis automatically cancelled when the ViewModel is cleared. Manual cancellation is unnecessary and potentially problematic.override fun onCleared() { super.onCleared() autoCloseJob?.cancel() - viewModelScope.cancel() forceHideDialog() }
🧹 Nitpick comments (2)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (2)
18-18: Remove the empty default constructor.The empty default constructor is unnecessary and can be removed as indicated by static analysis.
-class CustomTemplateViewModel() : ViewModel() { +class CustomTemplateViewModel : ViewModel() {
86-90: Consider consolidating duplicate logic.
forceHideDialogappears to duplicate the logic ofhideDialog. Consider whether both methods are necessary or if they can be consolidated.If both methods serve the same purpose, you could consolidate them:
- fun forceHideDialog() { - autoCloseJob?.cancel() - dialogState = CustomTemplateDialogState(isVisible = false, config = null) - }Or if
forceHideDialoghas specific semantics, consider adding documentation to clarify the difference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt
🧰 Additional context used
🪛 detekt (1.23.8)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt
[warning] 18-18: An empty default constructor can be removed.
(detekt.empty-blocks.EmptyDefaultConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (5)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (5)
1-12: LGTM! Clean imports and package structure.The imports are well-organized and appropriate for the ViewModel functionality with Compose integration.
13-16: LGTM! Well-structured data class.The data class properly encapsulates dialog state with appropriate default values.
20-24: LGTM! Proper state management setup.Good use of
mutableStateOffor Compose state management with private setter to encapsulate state mutations.
29-74: LGTM! Well-implemented dialog configuration.The
showDialogmethod properly handles dialog configuration, auto-close functionality, and coroutine job management. Good practice of canceling existing jobs before starting new ones.
76-84: LGTM! Proper cleanup in hideDialog.Good job canceling the auto-close job and resetting state when hiding the dialog.
There was a problem hiding this comment.
checkFirstTimePreferences method should be in Viewmodel
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (3)
17-17: Remove the empty default constructor.The empty default constructor is unnecessary and can be removed as flagged by the static analysis tool.
-class CustomTemplateViewModel() : ViewModel() { +class CustomTemplateViewModel : ViewModel() {
28-73: Consider refactoring the showDialog method to improve maintainability.The method has 8 parameters, which makes it complex to use and maintain. Consider using a data class or builder pattern to encapsulate the dialog configuration parameters.
Example refactor using a configuration data class:
data class DialogConfig( val title: String, val message: String, val imageUrl: String? = null, val primaryButtonText: String = "Continue", val secondaryButtonText: String = "Close", val autoCloseAfterSeconds: Int = 0, val onPrimaryAction: () -> Unit = {}, val onSecondaryAction: () -> Unit = {}, val onAutoClose: () -> Unit = {} ) fun showDialog(config: DialogConfig) { // Implementation using config properties }
75-88: Consider consolidating hideDialog and forceHideDialog methods.Both methods perform nearly identical operations - they cancel the auto-close job and reset the dialog state. The duplication could be eliminated by having one method call the other or by extracting the common logic.
Looking at the relevant code from
OpenUrlConfirmViewModel.kt, this pattern appears consistent across the codebase, so this refactor should be applied consistently if pursued.fun hideDialog() { - // Cancel auto-close job when manually hiding dialog - autoCloseJob?.cancel() - dialogState = CustomTemplateDialogState( - isVisible = false, - config = null - ) + forceHideDialog() } fun forceHideDialog() { + // Cancel auto-close job when hiding dialog autoCloseJob?.cancel() dialogState = CustomTemplateDialogState(isVisible = false, config = null) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt(1 hunks)sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmDialog.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- sample/src/main/java/com/clevertap/demo/ui/customtemplates/OverlayDialog.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (1)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/OpenUrlConfirmViewModel.kt (2)
hideDialog(47-54)forceHideDialog(59-61)
🪛 detekt (1.23.8)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt
[warning] 17-17: An empty default constructor can be removed.
(detekt.empty-blocks.EmptyDefaultConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (2)
sample/src/main/java/com/clevertap/demo/ui/customtemplates/CustomInterstitialViewModel.kt (2)
19-23: LGTM: Proper state management and coroutine handling.The use of
mutableStateOffor Compose state management and the private setter for encapsulation is well implemented. The auto-close job management follows Android best practices.
91-95: LGTM: Proper ViewModel lifecycle cleanup.The
onCleared()method correctly cancels the coroutine job and cleans up the dialog state, preventing memory leaks and ensuring proper resource management.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores