Fix android build #105
Fix android build #105
Conversation
WalkthroughThe pull request introduces significant updates to the Android application configuration. The Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Outside diff range and nitpick comments (2)
android/app/build.gradle (2)
Line range hint
69-77: Security: Avoid hardcoding signing credentialsThe fallback signing configuration contains hardcoded credentials and references a development keystore. This poses security risks if the keystore is committed to the repository.
Recommendations:
- Move development signing credentials to a secure location (e.g., local.properties)
- Add development.jks to .gitignore
- Document the setup process for development signing
Example implementation:
} else { release { - keyAlias 'ProjectUnity' - keyPassword 'ProjectUnity' - storeFile file('development.jks') - storePassword 'ProjectUnity' + keyAlias localProperties.getProperty('keyAlias') + keyPassword localProperties.getProperty('keyPassword') + storeFile file(localProperties.getProperty('storeFile')) + storePassword localProperties.getProperty('storePassword') } }
Line range hint
89-97: Incorrect debug build configuration may impact developmentThe debug build configuration has several issues that could complicate development:
minifyEnabled truein debug builds will make debugging more difficult- Using release signing config for debug builds is unusual and unnecessary
- The TODO comment is outdated as signing config is already implemented
Apply these changes to fix the debug configuration:
debug { - // TODO: Add your own signing config for the release build. - // Signing with the debug keys for now, so `flutter run --release` works. - minifyEnabled true + minifyEnabled false debuggable true - - signingConfig signingConfigs.release + // Use default debug signing config }
android/app/build.gradle
Outdated
| minSdkVersion 34 | ||
| targetSdkVersion 34 |
There was a problem hiding this comment.
Critical: Overly restrictive minSdkVersion will prevent installation on most Android devices
Setting minSdkVersion to 34 (Android 14) will prevent the app from being installed on devices running Android 13 and below, which represents the majority of Android devices in use. This is likely not intended and could severely limit your app's reach.
Consider lowering the minSdkVersion while keeping targetSdkVersion at 34:
- minSdkVersion 34
+ minSdkVersion 21 // Supports Android 5.0 and above
targetSdkVersion 34This change will:
- Support ~98% of active Android devices
- Maintain compatibility with latest Android features through targetSdkVersion 34
- Follow Android best practices for backward compatibility
📝 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.
| minSdkVersion 34 | |
| targetSdkVersion 34 | |
| minSdkVersion 21 // Supports Android 5.0 and above | |
| targetSdkVersion 34 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
android/app/build.gradle (2)
Line range hint
29-29: Review build configuration settingsSeveral configuration issues need attention:
- The commented out
flutter.compileSdkVersionreference should be removed or used consistently- Debug builds should not have
minifyEnabled trueas it makes debugging more difficult- Debug builds should use debug signing config instead of release
Apply these changes:
- compileSdkVersion 34//flutter.compileSdkVersion + compileSdkVersion flutter.compileSdkVersion debug { - minifyEnabled true + minifyEnabled false debuggable true - signingConfig signingConfigs.release + signingConfig signingConfigs.debug }Also applies to: 89-93
Line range hint
61-69: Address security concerns in signing configuration
- Development keystore credentials should not be hardcoded in source control
- The TODO comment about signing config can be removed as it's implemented
Consider these changes:
- Move development signing config to a separate, gitignored properties file
- Remove the outdated TODO comment
- keyAlias 'ProjectUnity' - keyPassword 'ProjectUnity' - storeFile file('development.jks') - storePassword 'ProjectUnity' + keyAlias localProperties.getProperty('debug.keyAlias') + keyPassword localProperties.getProperty('debug.keyPassword') + storeFile file(localProperties.getProperty('debug.storeFile')) + storePassword localProperties.getProperty('debug.storePassword') buildTypes { release { - // TODO: Add your own signing config for the release build. - // Signing with the debug keys for now, so `flutter run --release` works.Also applies to: 82-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/android_build.yml(0 hunks)android/app/build.gradle(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/android_build.yml
🔇 Additional comments (1)
android/app/build.gradle (1)
51-52: LGTM: SDK version configuration is appropriate
The SDK version configuration is well-balanced:
- minSdkVersion 23 provides good device coverage while maintaining modern API support
- targetSdkVersion 34 ensures compatibility with the latest Android features
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
lib/ui/sign_in/bloc/sign_in_view_bloc.dart (1)
42-44: Consider logging the exception for better error trackingIn the
_googleSignInmethod, consider logging the exception to help with debugging and monitoring. Currently, exceptions caught are not being logged or recorded. You can use Firebase Crashlytics or another logging mechanism to record the error details, similar to how it's done in the_appleSignInmethod.Example:
} catch (e, stack) { FirebaseCrashlytics.instance .recordError(e, stack, reason: 'Google Sign In Error'); emit(state.copyWith( googleSignInLoading: false, error: firebaseAuthError)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
android/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/round_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/round_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/round_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/round_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/round_launcher.pngis excluded by!**/*.png
📒 Files selected for processing (16)
android/.gitignore(1 hunks)android/app/build.gradle(1 hunks)android/app/src/debug/AndroidManifest.xml(1 hunks)android/app/src/main/AndroidManifest.xml(2 hunks)android/app/src/main/kotlin/com/canopas/canopas_unity/MainActivity.kt(0 hunks)android/app/src/main/kotlin/com/canopas/projectunity/MainActivity.kt(0 hunks)android/app/src/main/kotlin/com/canopas/unity/MainActivity.kt(0 hunks)android/app/src/main/kotlin/com/example/canopas_unity/MainActivity.kt(1 hunks)android/app/src/main/res/values-night/styles.xml(1 hunks)android/app/src/main/res/values/styles.xml(1 hunks)android/app/src/profile/AndroidManifest.xml(1 hunks)android/build.gradle(1 hunks)android/gradle.properties(1 hunks)android/gradle/wrapper/gradle-wrapper.properties(1 hunks)android/settings.gradle(1 hunks)lib/ui/sign_in/bloc/sign_in_view_bloc.dart(1 hunks)
💤 Files with no reviewable changes (3)
- android/app/src/main/kotlin/com/canopas/unity/MainActivity.kt
- android/app/src/main/kotlin/com/canopas/canopas_unity/MainActivity.kt
- android/app/src/main/kotlin/com/canopas/projectunity/MainActivity.kt
✅ Files skipped from review due to trivial changes (7)
- android/app/src/main/res/values-night/styles.xml
- android/app/src/main/res/values/styles.xml
- android/.gitignore
- android/build.gradle
- android/app/src/debug/AndroidManifest.xml
- android/app/src/main/kotlin/com/example/canopas_unity/MainActivity.kt
- android/app/src/profile/AndroidManifest.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- android/gradle/wrapper/gradle-wrapper.properties
🔇 Additional comments (6)
android/app/build.gradle (1)
27-27: Critical: Overly restrictive minSdk will limit device compatibility
Setting minSdk = 34 will prevent the app from being installed on devices running Android 13 and below, which represents the majority of Android devices currently in use. Consider lowering the minSdk to support more devices while keeping targetSdk at 34 to utilize the latest features.
android/gradle.properties (1)
1-1: Verify build environments have sufficient memory for increased JVM arguments
The org.gradle.jvmargs have been updated to allocate more memory (-Xmx4G) and metaspace (-XX:MaxMetaspaceSize=2G), which can improve build performance. Ensure that all development and CI environments have enough physical memory to accommodate these settings without causing system instability or build failures due to insufficient memory.
android/settings.gradle (1)
21-22: Confirm removal of Google Services and Crashlytics plugins
It appears that the plugins for com.google.gms.google-services and com.google.firebase.crashlytics have been removed. If your app relies on Google services or Firebase Crashlytics for functionality like authentication, analytics, or crash reporting, this may impact those features. Please confirm that this removal is intentional and that alternative solutions are in place if needed.
android/app/src/main/AndroidManifest.xml (3)
1-1: Confirm removal of the package attribute in the manifest
The package attribute has been removed from the <manifest> tag. Since you're using the namespace property in build.gradle, this is acceptable in newer Android Gradle Plugin versions. Ensure that all references to the application package are correctly updated and that this change doesn't affect the app's behavior.
10-10: Verify the use of empty android:taskAffinity
Setting android:taskAffinity="" means this activity will not have a task affinity and can affect how tasks and activities are managed in the back stack. This can impact navigation and the behavior of your app when interacting with other apps. Ensure this is intended and tested across different scenarios.
34-44: LGTM: Addition of <queries> section for package visibility
Adding the <queries> element with intent filters for android.intent.action.PROCESS_TEXT is appropriate for Android 11 (API level 30) and above. This ensures that the app can query for activities that can process text, maintaining functionality in compliance with the package visibility changes introduced in Android 11.
| } catch (e) { | ||
| emit(state.copyWith( | ||
| googleSignInLoading: false, error: firesbaseAuthError)); |
There was a problem hiding this comment.
Typo in variable name firesbaseAuthError
There's a typo in the variable firesbaseAuthError. It should be firebaseAuthError. This typo will cause a runtime error due to the undefined variable.
Apply this diff to fix the typo:
} catch (e) {
emit(state.copyWith(
googleSignInLoading: false, error: firesbaseAuthError));
+ // Correct variable name
+ googleSignInLoading: false, error: firebaseAuthError));
}Committable suggestion skipped: line range outside the PR's diff.
Purpose
Summary of Changes
Test steps
Conformity
Visual Evidence (Video, Images or Gif)
Summary by CodeRabbit
New Features
MainActivity.Bug Fixes
Chores