Connected with BE authentication #148
Conversation
WalkthroughThis pull request updates multiple components of the mobile application. It adds a new internet permission in the Android manifest and modifies the Gradle distribution URL. Several new data classes, enums, and cubits have been introduced to support reactive state management using flutter_bloc while removing the previous GetIt-based approach. Additional changes include the introduction of a logging interface, language management for internationalization, and refactored OpenID authentication logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginCubit
participant ApiImpl
participant OpenIDAuth
User->>LoginCubit: Initiate login
LoginCubit->>ApiImpl: checkLogin(Client, scopes)
ApiImpl->>OpenIDAuth: authenticate(Client, scopes)
OpenIDAuth-->>ApiImpl: Return Credential
ApiImpl-->>LoginCubit: Credential received
LoginCubit-->>User: Login successful
sequenceDiagram
participant App
participant MainCubit
participant ThemeModeManager
participant LanguageManager
App->>MainCubit: Initialize app
MainCubit->>ThemeModeManager: Load theme mode
MainCubit->>LanguageManager: Load locale
MainCubit-->>App: Return configured state
Suggested labels
Suggested reviewers
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: 11
🔭 Outside diff range comments (1)
sep490-mobile/lib/utils/openid_io.dart (1)
65-91:⚠️ Potential issueVerify the security implications of simplified authentication.
The new implementation has several potential concerns:
- Removed token validation and user info checks
- Changed port number from 4000 to 4200
- No error handling for failed authentication
Add proper error handling and validation:
Future<Credential> authenticate(Client client, {List<String> scopes = const []}) async { // ... existing code ... // starts the authentication - var c = await authenticator.authorize(); + try { + var c = await authenticator.authorize(); + if (c == null) { + throw Exception('Authentication failed: No credentials received'); + } + + // Validate the credential + var userInfo = await c.getUserInfo(); + if (userInfo == null) { + throw Exception('Authentication failed: No user info received'); + } + + return c; + } catch (e) { + throw Exception('Authentication failed: ${e.toString()}'); + }
♻️ Duplicate comments (1)
sep490-mobile/lib/pages/login/login_cubit.dart (1)
22-23:⚠️ Potential issueFix potential null pointer dereference.
The code assumes
result.responseis not null. Add null checks to handle potential null values safely.
🧹 Nitpick comments (14)
sep490-mobile/lib/repositories/i18n_repository.dart (4)
3-4: Add documentation comments for better code maintainability.Consider adding documentation comments to explain:
- The purpose of the LanguageManager class
- The format and possible values for language codes
- Whether this class follows any specific i18n standards
+/// Manages the application's language preferences using SharedPreferences. +/// Language codes should follow ISO 639-1 format (e.g., 'en', 'vi'). class LanguageManager { static const _languageKey = 'selectedLanguage';
6-9: Add input validation and optimize SharedPreferences usage.Consider these improvements:
- Validate language code format
- Cache SharedPreferences instance
class LanguageManager { static const _languageKey = 'selectedLanguage'; + SharedPreferences? _prefs; + + Future<SharedPreferences> get prefs async { + return _prefs ??= await SharedPreferences.getInstance(); + } Future<void> saveLanguage(String languageCode) async { + if (!RegExp(r'^[a-z]{2}$').hasMatch(languageCode)) { + throw ArgumentError('Invalid language code format. Expected ISO 639-1 format (e.g., "en")'); + } - final prefs = await SharedPreferences.getInstance(); + final prefs = await this.prefs; await prefs.setString(_languageKey, languageCode); }
11-15: Refactor for consistency and configurability.Consider these improvements:
- Use the cached SharedPreferences instance
- Make default language configurable
- Remove extra blank line
+ static const defaultLanguage = 'en'; + Future<String> loadLanguage() async { - final prefs = await SharedPreferences.getInstance(); + final prefs = await this.prefs; - return prefs.getString(_languageKey) ?? 'en'; + return prefs.getString(_languageKey) ?? defaultLanguage; - } + }
3-16: Consider implementing dependency injection for better testability.The current implementation directly instantiates SharedPreferences, making it difficult to mock for testing. Consider:
- Using dependency injection
- Creating an abstract storage interface
+/// Abstract interface for language storage +abstract class ILanguageStorage { + Future<void> saveLanguage(String code); + Future<String?> loadLanguage(); +} + +/// Concrete implementation using SharedPreferences +class SharedPreferencesLanguageStorage implements ILanguageStorage { + final SharedPreferences prefs; + + SharedPreferencesLanguageStorage(this.prefs); + + @override + Future<void> saveLanguage(String code) => + prefs.setString(LanguageManager._languageKey, code); + + @override + Future<String?> loadLanguage() => + Future.value(prefs.getString(LanguageManager._languageKey)); +} class LanguageManager { static const _languageKey = 'selectedLanguage'; + final ILanguageStorage _storage; + + LanguageManager(this._storage);sep490-mobile/lib/main_cubit.dart (1)
36-68: Consider advanced error handling.
The code logs an error if translations fail to load but continues execution. If localization is critical, consider additional recovery steps or fallback locale handling.sep490-mobile/lib/pages/login/login.dart (2)
45-54: Reliance on global credential.
initStateuses the globalcredentialfrommain.dart. Consider passing it through theLoginCubitor a repository to reduce global state usage.
91-95: TODO listener in BlocConsumer.
Add real code to thelistenerblock to handle login success/failure states.Do you want me to open a new issue or propose some example code for the listener?
sep490-mobile/lib/main.dart (3)
75-80: Keycloak config is minimal.
Scopes are defined, but no error handling if the Keycloak server is unreachable. Consider adding try/catch or fallback logic.
82-84: Global variables can cause coupling.
credentialandclientare global. Consider encapsulating them in a service class or repository for maintainability and testability.
152-180: Robust BLoC-based MaterialApp.
The dynamic theme, reactive locale, and fallback loading screen are well-designed. Keep an eye on error handling for routing or i18n.sep490-mobile/lib/dtos/responses/login_response.dart (1)
22-24: Optimize string concatenation in toString.Use string interpolation instead of concatenation for better readability and performance.
- return 'LoginResponse{' + ' credential: $credential,' + '}'; + return 'LoginResponse{ credential: $credential }';sep490-mobile/lib/pages/login/login_state.dart (1)
30-35: Optimize string concatenation in toString.Use string interpolation instead of concatenation for better readability and performance.
- return 'LoginState{' + - ' loadStatus: $loadStatus,' + - ' loginResponse: $loginResponse,' + - '}'; + return 'LoginState{ loadStatus: $loadStatus, loginResponse: $loginResponse }';sep490-mobile/lib/main_state.dart (1)
42-49: Optimize string concatenation in toString.Use string interpolation instead of concatenation for better readability and performance.
- return 'MainState{' + - ' isLightTheme: $isLightTheme,' + - ' selected: $selected,' + - ' locale: $locale,' + - ' isLoading: $isLoading,' + - '}'; + return 'MainState{ isLightTheme: $isLightTheme, selected: $selected, locale: $locale, isLoading: $isLoading }';sep490-mobile/lib/utils/openid_io.dart (1)
9-27: Consider externalizing configuration.The commented-out configuration suggests hardcoded values were removed. While this is good, ensure these values are properly managed through configuration files or environment variables.
Consider implementing a configuration management system that:
- Supports different environments (dev, staging, prod)
- Securely manages sensitive values
- Allows easy configuration updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
sep490-mobile/android/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngsep490-mobile/android/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngsep490-mobile/android/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngsep490-mobile/android/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngsep490-mobile/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngsep490-mobile/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
sep490-mobile/android/app/src/main/AndroidManifest.xml(1 hunks)sep490-mobile/android/gradle/wrapper/gradle-wrapper.properties(1 hunks)sep490-mobile/lib/dtos/responses/login_response.dart(1 hunks)sep490-mobile/lib/enums/drawer_item.dart(1 hunks)sep490-mobile/lib/enums/load_status.dart(1 hunks)sep490-mobile/lib/main.dart(2 hunks)sep490-mobile/lib/main_cubit.dart(1 hunks)sep490-mobile/lib/main_state.dart(1 hunks)sep490-mobile/lib/pages/login/login.dart(1 hunks)sep490-mobile/lib/pages/login/login_cubit.dart(1 hunks)sep490-mobile/lib/pages/login/login_state.dart(1 hunks)sep490-mobile/lib/repositories/i18n_repository.dart(1 hunks)sep490-mobile/lib/repositories/log/log.dart(1 hunks)sep490-mobile/lib/repositories/log/log_impl.dart(1 hunks)sep490-mobile/lib/services/api.dart(1 hunks)sep490-mobile/lib/services/api_impl.dart(1 hunks)sep490-mobile/lib/services/base_service.dart(0 hunks)sep490-mobile/lib/utils/l18n/app_localizations.dart(0 hunks)sep490-mobile/lib/utils/openid_browser.dart(0 hunks)sep490-mobile/lib/utils/openid_io.dart(3 hunks)sep490-mobile/pubspec.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- sep490-mobile/lib/utils/openid_browser.dart
- sep490-mobile/lib/utils/l18n/app_localizations.dart
- sep490-mobile/lib/services/base_service.dart
✅ Files skipped from review due to trivial changes (2)
- sep490-mobile/lib/enums/drawer_item.dart
- sep490-mobile/lib/enums/load_status.dart
🔇 Additional comments (18)
sep490-mobile/lib/repositories/i18n_repository.dart (1)
1-1:❓ Verification inconclusive
Verify shared_preferences package version for compatibility.
Ensure the shared_preferences package version is compatible with the Flutter version in use.
🏁 Script executed:
#!/bin/bash # Check the package version in pubspec.yaml and compare with latest version cat pubspec.yaml | grep -A 5 "shared_preferences:" # Get latest version from pub.dev curl -s https://pub.dev/api/packages/shared_preferences | jq -r '.latest.version'Length of output: 183
Attention: Verify the shared_preferences dependency configuration in the correct pubspec file.
We noticed that the initial version check attempted to read
pubspec.yamlfrom the repository root, which didn’t exist. Please ensure that you verify the shared_preferences version in the appropriate pubspec file (likely located atsep490-mobile/pubspec.yaml). Since the latest version on pub.dev is 2.5.2, confirm that the dependency version there is either set to2.5.2or a compatible version range that supports it.
- Verify the correct pubspec file location (e.g.,
sep490-mobile/pubspec.yaml) using a command like:fd -HI pubspec.yaml- Then check the shared_preferences section to confirm the version, for example:
cat sep490-mobile/pubspec.yaml | grep -A 5 "shared_preferences:"sep490-mobile/lib/main_cubit.dart (7)
13-22: Initialize in constructor carefully.
Invoking_initialize()in the constructor prevents callers from handling initialization errors. Consider providing a separate initialization method or exposing potential load failures.
27-33: Parallel initialization is good.
UsingFuture.waitto run_initializeTheme()and_initializeLocalization()concurrently is efficient. Ensure that if one fails, you handle exception logic appropriately (e.g., partial initialization).
70-73: Theme loading logic is succinct.
The_initializeTheme()method properly loads the stored theme mode before setting it. Good job.
75-79: Persist errors gracefully.
Saving the theme mode is synchronous here, but if_themeModeManager.saveThemeModecan fail, add try/catch to handle potential persistence issues.
81-83: DrawerItem selection looks fine.
ThesetSelectedmethod updates the state with a new value. Straightforward and clear.
84-89: Possible async mismatch.
_loadLocale()readsfinal get = _languageManager.loadLanguage()but does not await it. Verify ifloadLanguage()returns aFuture; if so, it should be awaited before converting to string.
91-95: Locale updates are well-handled.
The code sets the locale in state, translates, and then persists the language. Good approach.sep490-mobile/lib/pages/login/login.dart (3)
25-27: BlocProvider usage looks correct.
ProvidingLoginCubitwith anApifrom context is a clean approach. Good job decoupling the login logic.
57-71: Clean BLoC approach for MainCubit.
ReadingMainCubitand usingBlocBuilderensures the UI re-renders on theme or language changes. No issues found.
81-88: Toggle UI is well-implemented.
The code flips between dark and light themes usingsetTheme(!stateMain.isLightTheme). Straightforward logic.sep490-mobile/lib/main.dart (4)
85-96: Verify the discovery steps.
Issuer.discover(uri)is robust, but ensure unexpected network errors are handled or retried for better user experience.
97-117: Initialization flow is comprehensive.
The main function does an async setup for the entire app. Great job ensuring theme and logs are set up first.
119-130: Repository widget is a neat pattern.
Wrapping providers in separate widgets helps keep code organized. No issues found.
132-143: Chain of multi-provider to MainCubit.
Clean layering approach:Repository→Provider→MainCubit. Maintains a clear separation of concerns.sep490-mobile/android/gradle/wrapper/gradle-wrapper.properties (1)
5-5:❓ Verification inconclusive
Verify Gradle version compatibility.
Downgrading from Gradle 8.10 to 7.6.3 could impact build stability or compatibility with newer Android features.
Please confirm:
- Why the downgrade was necessary
- Whether this version is compatible with your Android plugin version
- If any features you're using require Gradle 8.x
🌐 Web query:
What is the latest stable version of Gradle and its compatibility with Flutter?💡 Result:
The latest stable version of Gradle as of February 2025 is 8.12.1 (released January 24, 2025)[4][10]. However, Flutter projects targeting Android have specific compatibility requirements:
Compatibility with Flutter
Recommended Gradle versions:
- For projects using Java/JDK ≥17 (required by Android Studio Flamingo+), use Gradle 7.3–7.6.1 to avoid build errors like
Unsupported class file major version[2][4].- Newer projects using Flutter ≥3.16 (with declarative Plugin DSL) may support newer Gradle versions[5], but official documentation still prioritizes stability with the 7.x series for most use cases[2][6].
Key dependencies:
Component Requirement for Flutter Compatibility Java SDK ≥17 (bundled with Android Studio) Android Gradle Plugin (AGP) ≤8.x (matches AGP-Gradle compatibility)[7] Kotlin Aligns with AGP version (e.g., AGP ≤8.x → Kotlin ≤2.x)[4] Migration Guidance
- To fix Java version conflicts:
cd android && ./gradlew wrapper --gradle-version=7.6.1- Verify JDK setup with
flutter doctor -vand update viaflutter config --jdk-dir=[PATH_TO_JDK17]if needed[2].Gradle’s roadmap indicates future alignment with JDK ≥17 in Gradle ≥9.x (Q2 2025)[1], but current Flutter workflows prioritize proven stability over bleeding-edge toolchain updates[2][4].
Citations:
- 1: https://gradle.org/whats-new/gradle-9/
- 2: https://docs.flutter.dev/release/breaking-changes/android-java-gradle-migration-guide
- 3: https://www.aubergine.co/insights/upgrade-the-flutter-version
- 4: https://endoflife.date/gradle
- 5: https://docs.flutter.dev/release/breaking-changes/flutter-gradle-plugin-apply
- 6: https://docs.flutter.dev/release/release-notes/release-notes-3.24.0
- 7: https://developer.android.com/build/releases/gradle-plugin
- 8: https://docs.gradle.org/current/userguide/compatibility.html
- 9: Gradle 8.0 is not useable with Flutter 3.7. flutter/flutter#124838
- 10: https://gradle.org/releases/
Action: Confirm Gradle Compatibility for Your Flutter Build
The gradle-wrapper.properties file now specifies:
distributionUrl=https\://services.gradle.org/distributions/gradle-7.6.3-all.zipWhile the latest stable Gradle version as of February 2025 is 8.12.1, Flutter projects (especially those using Java/JDK ≥17) typically recommend a stable 7.x series—commonly within the 7.3–7.6.1 range—to avoid issues like
Unsupported class file major version. Given that this update represents a downgrade from Gradle 8.10 to 7.6.3 and a switch from the 'bin' to 'all' distribution, please confirm the following:
- Rationale: Document why downgrading to 7.6.3 was necessary.
- Compatibility: Verify that Gradle 7.6.3 is fully compatible with your current Android Gradle plugin and overall build configuration.
- Feature Impact: Ensure that none of your project's features depend on Gradle 8.x capabilities.
If this downgrade has been tested and meets your project's requirements, then no further changes are needed.
sep490-mobile/android/app/src/main/AndroidManifest.xml (1)
2-2: LGTM: Internet permission is required for authentication.The INTERNET permission is necessary for OpenID authentication to work.
sep490-mobile/pubspec.yaml (1)
47-47:❓ Verification inconclusive
Verify the migration from GetIt to flutter_bloc.
The switch from GetIt to flutter_bloc represents a significant architectural change in state management.
Please ensure:
- All GetIt service registrations have been properly migrated to BLoC providers
- State management logic is properly implemented in the new BLoC classes
- No remnants of GetIt usage remain in the codebase
Run this script to verify the migration:
🏁 Script executed:
#!/bin/bash # Check for any remaining GetIt usage echo "Checking for remaining GetIt usage..." rg "get_it|GetIt" -l # Check for proper BLoC implementation echo "Checking BLoC implementation..." ast-grep --pattern 'class $_ extends Cubit<$_>'Length of output: 341
Migration from GetIt to flutter_bloc – Verification and Next Steps
- Verified that no residual GetIt or get_it references appear in the scanned files.
- The BLoC implementation scan yielded an error due to a non-code (JSON asset) file. This does not impact Dart code but suggests that the scan should be limited to Dart files.
- Action Items:
- Manually verify (or adjust the scan to only Dart files) that all service registrations and state management logic have been migrated to flutter_bloc patterns (e.g., using Cubit or Bloc classes).
- Ensure that any search tools or scripts exclude non-Dart assets to avoid similar errors in the future.
huynhlephcvinh
requested review from
thongdanghoang,
GiaBaorr and
nganntqe170236
Summary by CodeRabbit
New Features
Improvements