Add support for compileSdkVersion as per client project with default value of 34 #167
Conversation
WalkthroughUpdated Android Gradle config to use a dynamic Changes
Sequence Diagram(s)sequenceDiagram
participant Flutter as Flutter App
participant Dart as KommunicateFlutterPlugin
participant Channel as MethodChannel
participant Android as KmMethodHandler
participant SDK as Kommunicate SDK
note over Dart,Channel: User calls KommunicateFlutterPlugin.init(appId)
Dart->>Channel: invokeMethod("init", appId)
Channel->>Android: onMethodCall("init", appId)
Android->>SDK: Kommunicate.init(context, appId)
SDK-->>Android: success / throws
alt success
Android-->>Channel: result.success(null)
else error
Android-->>Channel: result.error(code, message, details)
end
Channel-->>Dart: future completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (2)
android/build.gradle (2)
32-32: Target SDK in a library module is typically ignored; consider removing or decoupling.Setting
targetSdkVersionin an Android library rarely affects the published AAR and can mislead consumers (it may only impact test APKs). Consider removing it or leaving it fixed/documented.Possible change:
- targetSdkVersion sdk + // targetSdkVersion is generally ignored for libraries; omit to avoid confusion. + // If you keep it, consider a fixed value or a separate override property.
35-35: Use the modern lint DSL; lintOptions is deprecated.AGP 7.x supports the
lint {}block and AGP 8 removeslintOptions. Migrate now for forward compatibility.Apply this diff:
- lintOptions { disable 'InvalidPackage' } + lint { + disable += ['InvalidPackage'] + }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/kommunicate_flutter.dart (1)
16-18: Tighten API: return Future and validateappIdbefore invoking native.Small clarity/UX win and avoids a roundtrip with empty input.
- static Future<dynamic> init(String appId) async { - return await _channel.invokeMethod('init', appId); - } + static Future<void> init(String appId) async { + if (appId.isEmpty) { + throw ArgumentError.value(appId, 'appId', 'must not be empty'); + } + await _channel.invokeMethod<void>('init', appId); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/io/kommunicate/kommunicate_flutter_plugin/KmMethodHandler.java(1 hunks)lib/kommunicate_flutter.dart(1 hunks)
| } else if (call.method.equals("init")) { | ||
| try { | ||
| final String initAppID = (String) call.arguments(); | ||
| Kommunicate.init(context, initAppID); | ||
| } catch (Exception e) { | ||
| result.error(ERROR, e.getMessage(), null); | ||
| } |
There was a problem hiding this comment.
Complete the MethodChannel result and validate input for "init".
The branch doesn’t call result.success(...), which can leave the Dart Future pending. Also, guard against empty appId and prefer e.toString() to avoid null messages.
} else if (call.method.equals("init")) {
- try {
- final String initAppID = (String) call.arguments();
- Kommunicate.init(context, initAppID);
- } catch (Exception e) {
- result.error(ERROR, e.getMessage(), null);
- }
+ try {
+ final String initAppID = (String) call.arguments();
+ if (TextUtils.isEmpty(initAppID)) {
+ result.error(ERROR, "appId is missing", null);
+ return;
+ }
+ Kommunicate.init(context, initAppID);
+ result.success(null); // complete Future<void>
+ } catch (Exception e) {
+ result.error(ERROR, e.toString(), null);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (call.method.equals("init")) { | |
| try { | |
| final String initAppID = (String) call.arguments(); | |
| Kommunicate.init(context, initAppID); | |
| } catch (Exception e) { | |
| result.error(ERROR, e.getMessage(), null); | |
| } | |
| } else if (call.method.equals("init")) { | |
| try { | |
| final String initAppID = (String) call.arguments(); | |
| if (TextUtils.isEmpty(initAppID)) { | |
| result.error(ERROR, "appId is missing", null); | |
| return; | |
| } | |
| Kommunicate.init(context, initAppID); | |
| result.success(null); // complete Future<void> | |
| } catch (Exception e) { | |
| result.error(ERROR, e.toString(), null); | |
| } |
🤖 Prompt for AI Agents
In
android/src/main/java/io/kommunicate/kommunicate_flutter_plugin/KmMethodHandler.java
around lines 69 to 75, the "init" branch neither returns a success to the
MethodChannel nor validates the input and uses e.getMessage() which can be null;
update the handler to validate that the received appId is a non-empty string and
if invalid call result.error with a clear code/message, wrap Kommunicate.init in
try/catch and on success call result.success(null) (or a confirmation value),
and in the catch use result.error(ERROR, e.toString(), null) to ensure the Dart
Future completes and the full exception text is logged.
Summary by CodeRabbit
New Features
Chores