chore: custom sdk updates #19
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change introduces a callback-based event and error reporting system throughout the Android SDK, adding a new Changes
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
android/src/main/java/com/formbricks/android/helper/FormbricksConfig.kt (1)
53-56: New builder method for error handling configurationThe
setAutoDismissErrorsmethod follows the existing builder pattern consistently. Consider adding a brief comment to explain what this setting controls, similar to other methods in the class.+/** + * Controls whether survey fragments automatically dismiss on error conditions + */ fun setAutoDismissErrors(autoDismissErrors: Boolean): Builder { this.autoDismissErrors = autoDismissErrors return this }android/proguard-rules.pro (1)
53-55: Duplicated StringConcatFactory rule with clarifying comment.This rule is already defined at line 45, but the comment indicates it was auto-generated by the Android Gradle plugin, which explains the duplication.
Consider consolidating these duplicate rules to avoid confusion, although keeping it as is won't cause any functional issues.
android/src/main/java/com/formbricks/android/Formbricks.kt (1)
217-217: Inconsistent success callback implementation.Success callback is only implemented for logout operations. Consider adding success callbacks for other operations like
setUserId,setAttribute, etc., to maintain API consistency.For example, add success callbacks after successful operations:
fun setUserId(userId: String, allowOverrideUserId: Boolean) { // existing checks... UserManager.set(userId) + callback?.onSuccess(SuccessType.USER_ID_SET_SUCCESS) }This would require adding corresponding enum values to the
SuccessTypeenum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
android/build.gradle.kts(4 hunks)android/consumer-rules.pro(1 hunks)android/proguard-rules.pro(2 hunks)android/src/main/java/com/formbricks/android/Formbricks.kt(11 hunks)android/src/main/java/com/formbricks/android/helper/FormbricksConfig.kt(2 hunks)android/src/main/java/com/formbricks/android/manager/SurveyManager.kt(10 hunks)android/src/main/java/com/formbricks/android/manager/UserManager.kt(3 hunks)android/src/main/java/com/formbricks/android/network/queue/UpdateQueue.kt(2 hunks)android/src/main/java/com/formbricks/android/webview/FormbricksFragment.kt(7 hunks)android/src/main/java/com/formbricks/android/webview/FormbricksViewModel.kt(3 hunks)android/src/main/java/com/formbricks/android/webview/WebAppInterface.kt(2 hunks)gradle/libs.versions.toml(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)
🔇 Additional comments (42)
android/src/main/java/com/formbricks/android/helper/FormbricksConfig.kt (3)
18-19: New parameter added for error handling controlThe addition of
autoDismissErrorswith a default value oftruemaintains backward compatibility while adding configurable behavior for error handling.
26-26: Private variable for new configuration propertyThis matches the pattern used for other configuration properties in the builder.
65-66: Configuration property passed to constructorThe builder correctly passes the new
autoDismissErrorsparameter to theFormbricksConfigconstructor.gradle/wrapper/gradle-wrapper.properties (1)
1-1: Significant Gradle version downgradeDowngrading from Gradle 8.11.1 to 7.5 is a substantial change that may affect build performance and available features. While this appears to be intentional to align with other dependency downgrades in the project, please verify this is necessary.
Can you confirm if this downgrade is required for compatibility with other build dependencies? Newer Gradle versions typically offer better performance and more features.
Also applies to: 4-4
android/src/main/java/com/formbricks/android/network/queue/UpdateQueue.kt (2)
3-3: Added import for callback supportThe new import supports the error reporting enhancement.
72-72: Enhanced error reporting with callbackGood improvement to not only log errors but also report them through the callback system, which aligns with the broader error handling enhancements in this PR.
android/consumer-rules.pro (1)
5-11: ProGuard rules for callback interface and required librariesThe added ProGuard rules correctly preserve the new
FormbricksCallbackinterface and necessary dependencies from code shrinking and obfuscation.The warning suppression for
StringConcatFactoryis a standard practice for handling warnings related to Java 9+ features.android/src/main/java/com/formbricks/android/webview/WebAppInterface.kt (2)
4-4: Good addition of Formbricks import for callback access.The import is necessary to access the global callback mechanism that's being integrated throughout the SDK.
39-40: Consistent error reporting through callback mechanism.Good implementation of the callback-based error reporting system across all exception handling blocks. This provides a centralized way to notify consumers of the SDK about errors, in addition to the existing logging mechanism.
Also applies to: 42-43, 45-46, 48-49
android/proguard-rules.pro (2)
38-38: Properly kept FormbricksCallback interface from obfuscation.Good addition of the ProGuard rule to preserve the FormbricksCallback interface, which is necessary for the newly introduced callback system to work correctly.
48-52: Additional ProGuard rules for external dependencies.These rules ensure that necessary classes from external dependencies are preserved during code shrinking and obfuscation.
android/src/main/java/com/formbricks/android/manager/UserManager.kt (4)
11-11: Added SuccessType import for callback integration.The import is necessary for the success callback integration being added to the user manager.
150-150: User sync success callback notification.Good addition of the success callback after user synchronization, providing feedback to SDK consumers about successful operations.
153-153: Error reporting through callback in exception handling.Appropriate integration of the error callback in the exception handler, allowing SDK consumers to be notified of user sync failures.
167-168: Error reporting during logout operation.Good addition of the error callback when attempting to logout without a user ID set, providing clear feedback to SDK consumers.
android/src/main/java/com/formbricks/android/webview/FormbricksViewModel.kt (5)
15-15: Added Gson import for JSON serialization.The import is necessary for serializing the new hiddenFields parameter into JSON.
115-115: Extended loadHtml method to support hiddenFields.Good addition of an optional hiddenFields parameter with a default value of null, ensuring backward compatibility.
117-121: Updated getJson invocation with hiddenFields parameter.Properly passing the hiddenFields parameter to the getJson method.
126-130: Updated getJson method signature with hiddenFields parameter.The method signature is appropriately updated to accept the new optional parameter.
148-148: Added hiddenFields to JSON payload.Good implementation of conditional adding of hiddenFields to the JSON payload using Gson for serialization. The null check ensures backward compatibility.
android/src/main/java/com/formbricks/android/manager/SurveyManager.kt (4)
76-76: Good error handling improvement.Adding callback error reporting during environment data deserialization provides better visibility for SDK consumers into what's happening behind the scenes.
134-134: Proper callback integration for environment refresh.Adding success and error callbacks for the environment refresh operation provides useful feedback to SDK consumers about the operation's outcome.
Also applies to: 138-138
149-149: Feature enhancement: Hidden fields support.The addition of an optional
hiddenFieldsparameter to thetrackmethod and passing it toshowSurveyallows for more contextual data to be sent with surveys. This is a valuable enhancement for capturing additional information without modifying the survey structure.Also applies to: 191-191
163-164: Comprehensive error reporting across survey lifecycle.Adding callback error reporting for various failure scenarios (survey not found, language unavailability, display issues, missing IDs) provides consistent error handling throughout the SDK. This significantly improves the debugging experience for SDK consumers.
Also applies to: 174-175, 198-200, 214-215, 228-230, 296-298
gradle/libs.versions.toml (1)
2-4:Details
❓ Verification inconclusive
Verify compatibility with downgraded dependency versions.
Multiple dependencies have been significantly downgraded (e.g., AGP from 8.x to 7.2.2, Kotlin from 2.1.0 to 1.7.20). While this may improve compatibility with older Android environments, it could introduce compatibility issues or lose access to newer features.
Also applies to: 6-26
🏁 Script executed:
#!/bin/bash # Check if any of the dependency versions are too old or have known vulnerabilities # Check AGP compatibility with Gradle version cat gradle/wrapper/gradle-wrapper.properties # Check for any deprecation warnings in build logs grep -i "deprecat" android/build/outputs/logs/build-info.xml 2>/dev/null || echo "No build logs available" # Look for any commented migrations or TODOs related to version changes grep -r "TODO.*upgrad" --include="*.kt" --include="*.gradle*" .Length of output: 460
Verify compatibility with downgraded dependencies
We’ve rolled back:
- AGP from 8.x → 7.2.2
- Kotlin from 2.1.0 → 1.7.20
- core-ktx to 1.8.0
Preliminary checks show your Gradle wrapper is on 7.5 (acceptable for AGP 7.2.2), but we couldn’t locate any deprecation warnings or TODOs in the logs. Please:
• Confirm AGP 7.2.2 officially supports Gradle 7.5 (see Android Gradle Plugin release notes).
• Run a full clean build (./gradlew clean assembleDebug) and address any missing-feature or deprecation errors.
• Verify that Kotlin 1.7.20 covers all language/API usages in your codebase.Key files for review:
- gradle/libs.versions.toml (lines 2–4)
- gradle/wrapper/gradle-wrapper.properties
android/build.gradle.kts (5)
5-7: Style change: Using plugin ID syntax instead of Kotlin DSL syntax.The switch from Kotlin DSL plugin application to ID-based plugin application is consistent with the overall downgrading pattern. Ensure this doesn't affect build functionality.
28-28: SDK version alignment with Kotlin version.Downgrading the compileSdk from 35 to 34 and adding a Kotlin BOM enforcement aligns with the dependency downgrades in the versions.toml file. This ensures consistent versioning across the build.
Also applies to: 110-110
52-71: Enhanced packaging configuration using sets.The updated packagingOptions block with explicit sets for excludes and pickFirsts is more maintainable and clearer. Good improvement to handle potential packaging conflicts.
114-115: Maven publishing configuration update.The changes to the Maven publishing configuration (removing the CENTRAL_PORTAL parameter and using property setters) match the downgraded maven-publish plugin version. This ensures proper artifact publishing with the older plugin version.
Also applies to: 122-142
43-43:Details
❓ Verification inconclusive
Disabling minification for release builds.
Turning off minification for release builds might increase the SDK size, but can help avoid potential ProGuard-related issues with the callback implementation. Consider if this trade-off is acceptable.
🏁 Script executed:
#!/bin/bash # Check the impact of disabling minification on the AAR size # Find previous release AAR size if available find . -name "*.aar" -type f -exec ls -lh {} \; # See if there are any ProGuard-related files that might explain this change grep -r "Formbricks.*keep" --include="*.pro" .Length of output: 101
Verify impact of disabling minification (android/build.gradle.kts:43)
Disabling
isMinifyEnabledfor release builds can prevent ProGuard-related callback issues but will increase your AAR size. We weren’t able to locate any existing AAR artifacts or ProGuard keep rules for Formbricks in the repo—please double-check the following before merging:
- Compare the size of a pre-minified release AAR against the new build to quantify the increase.
- Confirm whether any custom ProGuard rules (e.g.,
-keepfor callback implementations) are required once minification is off.- Decide if the larger SDK footprint is acceptable for your distribution targets.
android/src/main/java/com/formbricks/android/webview/FormbricksFragment.kt (5)
42-42: Feature enhancement: Hidden fields support in FormbricksFragment.Adding
hiddenFieldsparameter to the constructor andshowmethod allows for passing contextual data through to the survey, which aligns with the changes in SurveyManager. This is a valuable enhancement.Also applies to: 248-253
51-51: Comprehensive lifecycle event reporting.Adding callback reporting for survey lifecycle events (started, closed, page visible) provides valuable feedback to SDK consumers about the survey's state, improving the integration experience.
Also applies to: 57-58, 194-195
150-154: Improved error handling for missing surveyId.Adding an explicit check for null or empty surveyId with appropriate error callback and early return prevents potential issues later in the code. This is a good defensive programming practice.
163-166: Configurable error handling behavior.The addition of
Formbricks.autoDismissErrorsflag allows SDK consumers to control whether surveys should be automatically dismissed on errors. This provides flexibility in error handling strategies.
210-210: Proper propagation of hidden fields to the view model.Forwarding the hiddenFields parameter to the view model's loadHtml method ensures that the data flows correctly through the entire survey display pipeline.
android/src/main/java/com/formbricks/android/Formbricks.kt (7)
18-26: Well-structured callback interface design.The new
FormbricksCallbackinterface is well-designed with clear method signatures for survey lifecycle events, error reporting, and success notifications. This allows client applications to better integrate with the SDK and respond appropriately to various events.
40-40: Good addition of callback property.Making the callback property publicly accessible allows client applications to set and update the callback implementation as needed. The nullability ensures flexibility when no callback handling is required.
105-110: Good implementation of user ID override protection.The check for existing user ID and the
allowOverrideUserIdparameter prevents accidental user ID overrides, which is a good security practice to maintain user data integrity.
182-182: Beneficial addition of hiddenFields parameter.The addition of the optional
hiddenFieldsparameter totrackandshowSurveymethods allows for passing additional data through the survey flow, which enhances flexibility and customization options.Also applies to: 197-197, 236-236, 245-245
65-65: Consistent error reporting through callbacks.The addition of error callbacks throughout the code provides a consistent mechanism for error reporting across the SDK, making errors more visible to client applications and allowing them to handle errors appropriately.
Also applies to: 100-100, 107-107, 127-127, 146-146, 165-165, 185-185, 192-192, 212-212, 239-239
36-36:Details
✅ Verification successful
Verify usage of autoDismissErrors property.
The
autoDismissErrorsproperty is added and initialized from the configuration, but its actual usage is not apparent in this file. Please ensure this property is correctly used in the error handling flow.Also applies to: 76-76
🏁 Script executed:
#!/bin/bash # Search for usage of autoDismissErrors property in the codebase rg "autoDismissErrors" --type kotlinLength of output: 1026
autoDismissErrors property usage confirmed
The
autoDismissErrorsflag is correctly applied in the error handling flow:
- In Formbricks.kt:
autoDismissErrors = config.autoDismissErrors- In FormbricksFragment.kt:
if (Formbricks.autoDismissErrors) { // dismiss errors }No further changes needed.
13-13:Details
✅ Verification successful
Ensure SuccessType enum is properly defined.
The code imports and uses
SuccessTypeenum. Make sure this enum has all the necessary values for different success scenarios in the SDK.
🏁 Script executed:
#!/bin/bash # Check the definition of SuccessType enum rg -A 10 "enum class SuccessType"Length of output: 492
🏁 Script executed:
#!/bin/bash # List all public methods in Formbricks.kt and show where SuccessType is used echo "== Methods in Formbricks.kt ==" rg -n "fun " android/src/main/java/com/formbricks/android/Formbricks.kt echo echo "== SuccessType usages in Formbricks.kt ==" rg -n "SuccessType" -C2 android/src/main/java/com/formbricks/android/Formbricks.ktLength of output: 1737
SuccessType enum is complete for current SDK use cases
The
SuccessTypeenum inandroid/src/main/java/com/formbricks/android/model/enums/SuccessType.ktdefines exactly three values:
- SET_USER_SUCCESS
- GET_ENVIRONMENT_SUCCESS
- LOGOUT_SUCCESS
These match the only three places where
onSuccess(successType: SuccessType)is invoked inFormbricks.kt(user setup, environment fetch, logout). No additional enum entries are needed at this time.
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
android/src/main/java/com/formbricks/android/Formbricks.kt (1)
97-97:⚠️ Potential issueBreaking change in method signature without backward compatibility.
The
setUserIdmethod now requires a new parameterallowOverrideUserId, which could break existing client code. Consider providing an overloaded method with the original signature for backward compatibility.// Add an overloaded method for backward compatibility + /** + * Sets the user id for the current user with the given [String]. + * The SDK must be initialized before calling this method. + * + * ``` + * Formbricks.setUserId("my_user_id") + * ``` + * + */ + fun setUserId(userId: String) { + setUserId(userId, false) + }
🧹 Nitpick comments (5)
android/src/main/java/com/formbricks/android/Formbricks.kt (5)
40-40: Consider adding a setter method for the callback property.While the callback property is public and can be set directly, it would be more consistent with the SDK's design to provide a dedicated setter method with appropriate documentation, similar to other configuration methods in this class.
+ /** + * Sets the callback for receiving survey lifecycle events, errors, and success notifications. + * + * ``` + * Formbricks.setCallback(object : FormbricksCallback { + * override fun onSurveyStarted() { /* ... */ } + * override fun onSurveyFinished() { /* ... */ } + * override fun onSurveyClosed() { /* ... */ } + * override fun onPageCommitVisible() { /* ... */ } + * override fun onError(error: Exception) { /* ... */ } + * override fun onSuccess(successType: SuccessType) { /* ... */ } + * }) + * ``` + */ + fun setCallback(callback: FormbricksCallback?) { + this.callback = callback + }
36-36: Add documentation for the autoDismissErrors property.The
autoDismissErrorsproperty is introduced and initialized from the config, but there's no documentation explaining its purpose or behavior. Consider adding KDoc to explain how this property affects error handling in the SDK.Also applies to: 76-76
182-182: Add documentation for the hiddenFields parameter in track method.The
trackmethod now accepts an optionalhiddenFieldsparameter that's passed toSurveyManager.track, but there's no documentation explaining what hidden fields are or how they're used. Consider updating the KDoc to clarify the purpose and usage of this parameter./** * Tracks an action with the given [String]. The SDK will process the action and it will present the survey if any of them can be triggered. * The SDK must be initialized before calling this method. * +* @param action The action to track +* @param hiddenFields Optional map of key-value pairs that will be passed to the survey but not visible to the end user * * ``` * Formbricks.track("button_clicked") +* // With hidden fields +* Formbricks.track("purchase_completed", mapOf("order_id" to "12345", "amount" to 99.99)) * ``` * */Also applies to: 197-197
236-236: Add documentation for the hiddenFields parameter in showSurvey method.Similar to the
trackmethod, theshowSurveymethod now accepts an optionalhiddenFieldsparameter, but there's no documentation explaining its purpose. Although this is an internal method, it would be helpful to add documentation for maintainers./// Assembles the survey fragment and presents it +/// @param id The ID of the survey to show +/// @param hiddenFields Optional map of key-value pairs that will be passed to the survey but not visible to the end user internal fun showSurvey(id: String, hiddenFields: Map<String, Any>? = null) {Also applies to: 245-245
217-217: Consider implementing consistent success callbacks across all methods.The
logoutmethod now callscallback?.onSuccess(SuccessType.LOGOUT_SUCCESS)to notify successful logout, which is a good improvement. Consider implementing similar success notifications for other methods likesetUserId,setAttribute,setAttributes, etc., to provide a consistent notification pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/com/formbricks/android/Formbricks.kt(11 hunks)
🔇 Additional comments (2)
android/src/main/java/com/formbricks/android/Formbricks.kt (2)
18-26: Good addition of FormbricksCallback interface.The introduction of this callback interface provides a structured way for client applications to handle survey lifecycle events, errors, and success notifications. The interface methods are well-named and clearly indicate their purpose.
65-65: Good error reporting through callbacks.The consistent addition of error callbacks throughout the SDK methods provides a uniform way for client applications to handle errors. This is a significant improvement to the error handling mechanism.
Also applies to: 100-100, 107-107, 127-127, 146-146, 165-165, 185-185, 192-192, 212-212, 239-239
|
@longvantruong The merge target for this PR is the |
longvantruong
force-pushed
the
longtv/mobile-sdk-custom
branch
from
4f8ce2e to
67b1528
Compare
3 participants
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores