Mobile native client via Flutter #426
Conversation
Basic functionality development includes adding and deleting transactions,viewing balances,
WalkthroughAdds a complete Flutter mobile client: Android and iOS platform projects, Flutter app code (models, services, providers, screens, widgets), CI workflow for builds/tests, documentation, and project configuration including signing setup and assets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Flutter App
participant AuthProv as AuthProvider
participant AuthSvc as AuthService
participant Backend as Backend API
participant Storage as SecureStorage
User->>App: Submit credentials
App->>AuthProv: login(email,password)
AuthProv->>AuthSvc: login(...)
AuthSvc->>Backend: POST /login
Backend-->>AuthSvc: { access_token, refresh_token, user } or { mfa_required }
alt MFA required
AuthSvc-->>AuthProv: mfa_required
AuthProv-->>App: show OTP input
User->>App: Submit OTP
App->>AuthProv: login(..., otp)
AuthProv->>AuthSvc: login(..., otp)
AuthSvc->>Backend: POST /login (with otp)
Backend-->>AuthSvc: tokens + user
end
AuthSvc->>Storage: save tokens & user
AuthSvc-->>AuthProv: success + user
AuthProv->>App: notifyListeners (authenticated)
App->>App: Navigate to Dashboard
sequenceDiagram
participant Dashboard as DashboardScreen
participant AuthProv as AuthProvider
participant Storage as SecureStorage
participant AccountsProv as AccountsProvider
participant AccountsSvc as AccountsService
participant Backend as Backend API
Dashboard->>AuthProv: request valid access token
AuthProv->>Storage: read tokens
Storage-->>AuthProv: tokens (maybe expired)
alt token expired
AuthProv->>AuthSvc: refreshToken
AuthSvc->>Backend: POST /refresh
Backend-->>AuthSvc: new access_token
AuthSvc->>Storage: save tokens
end
AuthProv-->>Dashboard: accessToken
Dashboard->>AccountsProv: fetchAccounts(accessToken)
AccountsProv->>AccountsSvc: getAccounts(accessToken, page)
AccountsSvc->>Backend: GET /accounts...
Backend-->>AccountsSvc: accounts + pagination
AccountsSvc-->>AccountsProv: accounts
AccountsProv->>Dashboard: notifyListeners (accounts ready)
Dashboard->>Dashboard: render UI summaries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 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: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (13)
mobile/.gitignore-11-11 (1)
11-11: Remove pubspec.lock from .gitignore.For Flutter applications,
pubspec.lockshould be committed to version control to ensure reproducible builds and consistent dependency resolution across all environments and team members.Apply this diff:
-.metadata -pubspec.lock - +.metadataCommittable suggestion skipped: line range outside the PR's diff.
mobile/lib/services/accounts_service.dart-16-22 (1)
16-22: Add a timeout to prevent indefinite hangs.The HTTP request lacks a timeout configuration, which could cause the app to hang indefinitely if the server is unresponsive.
Apply this diff:
final response = await http.get( url, headers: { 'Authorization': 'Bearer $accessToken', 'Accept': 'application/json', }, -); +).timeout( + const Duration(seconds: 30), + onTimeout: () => http.Response('{"error": "Request timeout"}', 408), +);mobile/lib/services/accounts_service.dart-42-48 (1)
42-48: Wrap jsonDecode in try-catch to handle invalid JSON responses.The server might return HTML error pages or plain text for certain error conditions, which would cause
jsonDecodeto throw an exception.Apply this diff:
} else { - final responseData = jsonDecode(response.body); - return { - 'success': false, - 'error': responseData['error'] ?? 'Failed to fetch accounts', - }; + try { + final responseData = jsonDecode(response.body); + return { + 'success': false, + 'error': responseData['error'] ?? 'Failed to fetch accounts', + }; + } catch (e) { + return { + 'success': false, + 'error': 'Failed to fetch accounts: ${response.statusCode}', + }; + } }mobile/lib/services/accounts_service.dart-24-35 (1)
24-35: Add defensive null checks and type validation.The code assumes
responseData['accounts']exists and is a List, and thatresponseData['pagination']exists. If the API response structure changes or is malformed, this will throw an exception.Apply this diff:
if (response.statusCode == 200) { - final responseData = jsonDecode(response.body); - - final accountsList = (responseData['accounts'] as List) - .map((json) => Account.fromJson(json)) - .toList(); - - return { - 'success': true, - 'accounts': accountsList, - 'pagination': responseData['pagination'], - }; + try { + final responseData = jsonDecode(response.body); + + if (responseData['accounts'] is! List) { + return { + 'success': false, + 'error': 'Invalid response format', + }; + } + + final accountsList = (responseData['accounts'] as List) + .map((json) => Account.fromJson(json)) + .toList(); + + return { + 'success': true, + 'accounts': accountsList, + 'pagination': responseData['pagination'], + }; + } catch (e) { + return { + 'success': false, + 'error': 'Failed to parse response: $e', + }; + }mobile/lib/models/user.dart-14-21 (1)
14-21: Handle missingidmore safely inUser.fromJson
idis non‑nullable, butfromJsoncurrently does:id: json['id'].toString(),If
json['id']isnullor absent, this becomes the literal string"null", which is almost certainly not a valid user id and can hide backend/data issues.Consider tightening this by either:
- enforcing presence and failing fast:
final rawId = json['id']; if (rawId == null) { throw ArgumentError('User JSON is missing "id"'); } return User( id: rawId.toString(), // ... );or
- making
idnullable (final String? id;) if missing ids are genuinely expected.This keeps your user identifiers consistent and easier to reason about.
mobile/lib/models/account.dart-18-27 (1)
18-27: Add null-safety handling infromJsonto prevent runtime crashes.Direct casts like
json['name'] as Stringwill throwTypeErrorif the server returnsnullfor any of these fields. Consider defensive parsing:factory Account.fromJson(Map<String, dynamic> json) { return Account( id: json['id'].toString(), - name: json['name'] as String, - balance: json['balance'] as String, - currency: json['currency'] as String, + name: json['name'] as String? ?? '', + balance: json['balance'] as String? ?? '0', + currency: json['currency'] as String? ?? 'USD', classification: json['classification'] as String?, - accountType: json['account_type'] as String, + accountType: json['account_type'] as String? ?? 'other_asset', ); }mobile/lib/models/auth_tokens.dart-16-24 (1)
16-24: Add null checks infromJsonto prevent crashes from malformed API responses.Direct casts like
json['access_token'] as Stringwill throw if the key is missing or null. The_parseToInthelper also throwsFormatExceptionwhich propagates uncaught.factory AuthTokens.fromJson(Map<String, dynamic> json) { + final accessToken = json['access_token']; + final refreshToken = json['refresh_token']; + if (accessToken == null || refreshToken == null) { + throw FormatException('Missing required token fields'); + } return AuthTokens( - accessToken: json['access_token'] as String, - refreshToken: json['refresh_token'] as String, - tokenType: json['token_type'] as String, + accessToken: accessToken as String, + refreshToken: refreshToken as String, + tokenType: json['token_type'] as String? ?? 'Bearer', expiresIn: _parseToInt(json['expires_in']), createdAt: _parseToInt(json['created_at']), ); }mobile/lib/providers/transactions_provider.dart-61-79 (1)
61-79: Add try-catch for network exceptions.Same issue applies to bulk delete operation.
mobile/lib/services/auth_service.dart-98-132 (1)
98-132: Same error handling and duplicate parsing issues as login method.The
signupmethod has the same issues identified inlogin: missing try-catch for network/JSON exceptions and duplicateUser.fromJsoncalls on lines 116 and 124. Apply the same fix pattern for consistency.mobile/lib/providers/transactions_provider.dart-41-59 (1)
41-59: Add try-catch for network exceptions.Same issue: wrap the service call in try-catch to handle network failures gracefully.
mobile/lib/services/auth_service.dart-141-168 (1)
141-168: Wrap in try-catch for network and JSON parsing errors.Same issue applies here:
jsonDecodeon line 153 can throw if the response is not valid JSON.mobile/lib/providers/transactions_provider.dart-16-39 (1)
16-39: Missing try-catch for network exceptions.Unlike
AccountsProvider.fetchAccounts, this method lacks a try-catch block. If the service call throws (network error, timeout), the exception will propagate uncaught.Future<void> fetchTransactions({ required String accessToken, String? accountId, }) async { _isLoading = true; _error = null; notifyListeners(); + try { final result = await _transactionsService.getTransactions( accessToken: accessToken, accountId: accountId, ); _isLoading = false; if (result['success']) { _transactions = result['transactions']; _error = null; } else { _error = result['error']; } + } catch (e) { + _isLoading = false; + _error = 'Connection error. Please check your internet connection.'; + } notifyListeners(); }mobile/lib/services/auth_service.dart-31-71 (1)
31-71: Missing error handling for JSON parsing and network failures.The
jsonDecodecall on line 40 andAuthTokens.fromJsonon line 44 can throw exceptions if the server returns invalid JSON or an unexpected response structure. Additionally, duplicateUser.fromJsoncalls occur on lines 49 and 57. Consider wrapping in try-catch and reusing the parsed user.+ try { final response = await http.post( url, headers: { 'Content-Type': 'application/json', 'Accept': 'application/json', }, body: jsonEncode(body), ); final responseData = jsonDecode(response.body); if (response.statusCode == 200) { // Store tokens final tokens = AuthTokens.fromJson(responseData); await _saveTokens(tokens); // Store user data + User? user; if (responseData['user'] != null) { - final user = User.fromJson(responseData['user']); + user = User.fromJson(responseData['user']); await _saveUser(user); } return { 'success': true, 'tokens': tokens, - 'user': responseData['user'] != null - ? User.fromJson(responseData['user']) - : null, + 'user': user, }; } else if (response.statusCode == 401 && responseData['mfa_required'] == true) { return { 'success': false, 'mfa_required': true, 'error': responseData['error'], }; } else { return { 'success': false, 'error': responseData['error'] ?? responseData['errors']?.join(', ') ?? 'Login failed', }; } + } catch (e) { + return { + 'success': false, + 'error': 'Connection error: ${e.toString()}', + }; + }
🟡 Minor comments (6)
mobile/docs/SIGNING_SETUP.md-9-9 (1)
9-9: Clarify the keystore-base64.txt file location.The documentation references "项目根目录的
keystore-base64.txt文件" (keystore-base64.txt in the project root). Since the mobile project is in the/mobilefolder per the PR description, clarify whether this file is at the repository root or within the/mobiledirectory.mobile/docs/SIGNING_SETUP.md-58-58 (1)
58-58: Correct the path reference to include the mobile folder.The path
android/app/upload-keystore.jksshould bemobile/android/app/upload-keystore.jksto reflect the actual project structure.Apply this diff:
-- **文件位置**: `android/app/upload-keystore.jks` +- **文件位置**: `mobile/android/app/upload-keystore.jks`mobile/docs/SIGNING_SETUP.md-63-63 (1)
63-63: Correct the path reference to include the mobile folder.The path
android/key.propertiesshould bemobile/android/key.propertiesfor consistency.Apply this diff:
-- 这些信息只存储在本地的 `android/key.properties` 文件中（已添加到 .gitignore） +- 这些信息只存储在本地的 `mobile/android/key.properties` 文件中（已添加到 .gitignore）mobile/docs/SIGNING_SETUP.md-41-41 (1)
41-41: Correct the path reference to include the mobile folder.The path
android/key.propertiesshould bemobile/android/key.propertiesto reflect the actual project structure where the mobile app is in the/mobilefolder.Apply this diff:
-本地构建已经配置好，`android/key.properties` 文件包含签名信息。 +本地构建已经配置好，`mobile/android/key.properties` 文件包含签名信息。mobile/lib/screens/transactions_list_screen.dart-281-302 (1)
281-302: Async deletion inonDismissedmay cause inconsistent UI state.The
onDismissedcallback removes the item from the UI before the API call completes. IfdeleteTransactionfails, the item has already been visually removed with no rollback mechanism.Consider moving the deletion logic to
confirmDismissand only allowing dismissal on success, or implementing an undo/rollback pattern.- onDismissed: (direction) async { - if (transaction.id != null) { - final scaffoldMessenger = ScaffoldMessenger.of(context); - final authProvider = Provider.of<AuthProvider>(context, listen: false); - final accessToken = await authProvider.getValidAccessToken(); - if (accessToken != null) { - final success = await transactionsProvider.deleteTransaction( - accessToken: accessToken, - transactionId: transaction.id!, - ); - - if (mounted && success) { - scaffoldMessenger.showSnackBar( - const SnackBar( - content: Text('Transaction deleted'), - backgroundColor: Colors.green, - ), - ); - } - } - } - }, + onDismissed: (direction) { + // Item already removed, show undo option + final removedTransaction = transaction; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: const Text('Transaction deleted'), + action: SnackBarAction( + label: 'Undo', + onPressed: () => _loadTransactions(), + ), + ), + ); + // Perform deletion in background + _performDeletion(removedTransaction.id!); + },Committable suggestion skipped: line range outside the PR's diff.
mobile/lib/services/transactions_service.dart-31-42 (1)
31-42: Guard JSON decoding when response bodies may not be valid JSON
jsonDecode(response.body)is called unconditionally increateTransactionanddeleteTransaction. If the backend returns an empty body or non‑JSON error payload for some status codes, this will throw and be reported as a generic “Network error: FormatException”, losing the more accurate server context. Consider defensive decoding (e.g., try/catch aroundjsonDecodeor checkingresponse.body.isNotEmpty) and treating JSON parse failures as a separate error case.- try { - final response = await http.post( + try { + final response = await http.post( url, headers: { 'Content-Type': 'application/json', 'Accept': 'application/json', 'Authorization': 'Bearer $accessToken', }, body: jsonEncode(body), ); - - final responseData = jsonDecode(response.body); + Map<String, dynamic>? responseData; + if (response.body.isNotEmpty) { + try { + final decoded = jsonDecode(response.body); + if (decoded is Map<String, dynamic>) { + responseData = decoded; + } + } catch (_) { + // leave responseData as null; fall back to generic messages + } + } @@ - } else { - return { - 'success': false, - 'error': responseData['error'] ?? 'Failed to create transaction', - }; - } + } else { + return { + 'success': false, + 'error': responseData?['error'] ?? 'Failed to create transaction', + }; + }Apply similar guarded decoding in
deleteTransactionbefore accessingresponseData['error'].Also applies to: 55-59, 144-159
🧹 Nitpick comments (30)
mobile/assets/images/.gitkeep (1)
1-2: Consider moving documentation outside of.gitkeep.
.gitkeepfiles are conventionally empty. While the documentation here is helpful, it's unconventional to include content. Consider creating a separateREADME.mdin this directory or documenting the asset structure elsewhere (e.g., in the mobile app's main README).Alternatively, if keeping this minimal documentation is intentional, you may leave it as-is—it does provide value for developers exploring the project.
If you'd like to move the documentation, create
mobile/assets/images/README.mdwith the same content and remove the text from.gitkeep:- # Placeholder for assets - This directory contains image assets for the Sure Mobile app.Then ensure
.gitkeepremains empty.mobile/test/widget_test.dart (1)
4-8: Consider adding meaningful widget tests.While the placeholder smoke test satisfies the CI requirements, consider adding tests for key components like
AccountCardwidget rendering, theme application, or navigation flows as the app matures.mobile/android/app/build.gradle (1)
7-33: Make release signing behavior clearer whenkey.propertiesis absentRight now
signingConfigs.releaseis always defined, even ifkey.propertiesdoes not exist, which can lead to slightly confusing Gradle errors when trying to assemble areleasebuild.Consider either:
- only creating
signingConfigs.releasewhenkeystorePropertiesFile.exists(), or- adding a clear failure (e.g.,
throw new GradleException("key.properties missing for release signing")) when a release build is requested and the file is not present.This keeps the “debug just works, release needs signing vars” story explicit for contributors and CI.
mobile/lib/models/transaction.dart (1)
22-33: Confirm that silent defaults for missing fields (especiallynature) are intentional
Transaction.fromJsonfalls back to empty strings and, fornature, to'expense':nature: json['nature']?.toString() ?? 'expense',If the backend ever omits or mis‑spells
nature, this will quietly turn such rows into expenses, which can skew reports and make debugging harder.You might want to instead:
- treat a missing/invalid
natureas an error, or- make
naturenullable and handle the “unknown” case explicitly in UI/logic.Similar questions apply to other fields defaulting to
''or'0'. If this is by design and matches the API contract, then it’s fine — but it’s worth double‑checking.mobile/lib/services/api_config.dart (1)
8-36: Align in‑memory base URL changes with persisted config (and consider platform defaults)
ApiConfig.initialize()loadsbackend_urlfromSharedPreferences, butsetBaseUrlonly updates the in‑memory_baseUrland doesn’t write it back. Unless some other code is persisting the same key, users’ backend URL changes will be lost on app restart.One option is to add a helper that both updates and persists:
static Future<void> persistBaseUrl(String url) async { _baseUrl = url; final prefs = await SharedPreferences.getInstance(); await prefs.setString('backend_url', url); }and use that from your settings/backend‑config screen.
Also note that the default
'http://10.0.2.2:3000'works for Android emulators but not for the iOS simulator; you may want to:
- choose a platform‑specific default, or
- prominently document that iOS users must configure the backend URL on first run.
It might also help to log (or otherwise surface) failures in
initialize()instead of silently swallowing them, to make backend URL issues easier to debug.mobile/docs/TECHNICAL_GUIDE.md (3)
9-9: Use markdown link syntax for bare URLs.Bare URLs should be wrapped in proper markdown link syntax for better rendering and accessibility across different markdown parsers.
Apply this diff:
-This application is a client app for the Sure Finance Management System and requires connection to the Sure backend server (Rails API) to function properly. Backend project: https://github.com/we-promise/sure +This application is a client app for the Sure Finance Management System and requires connection to the Sure backend server (Rails API) to function properly. Backend project: [Sure](https://github.com/we-promise/sure)
87-107: Add language specifier to flow diagram code blocks.The ASCII flow diagrams should have a language specifier (e.g.,
textorplaintext) for markdown linting compliance. This applies to code blocks at lines 60, 87, 113, 148, and 178.Example fix for the startup flow:
-``` +```text App Launch ↓ Initialize ApiConfig (load saved backend URL)
453-457: Convert bare URLs to markdown links.The related links section should use proper markdown link syntax for consistency with documentation standards.
## Related Links -- **Backend Project**: https://github.com/we-promise/sure -- **Flutter Official Documentation**: https://docs.flutter.dev -- **Dart Language Documentation**: https://dart.dev/guides +- **Backend Project**: [Sure](https://github.com/we-promise/sure) +- **Flutter Official Documentation**: [Flutter Docs](https://docs.flutter.dev) +- **Dart Language Documentation**: [Dart Guides](https://dart.dev/guides)mobile/lib/screens/transactions_list_screen.dart (2)
30-53: Confusing and fragile amount sign calculation logic.The logic for determining positive/negative amounts by counting minus signs and adjusting based on
isAssetis unintuitive and error-prone. This approach assumes the backend may send amounts with multiple negative signs (e.g.,"--100"), which is unusual.Consider clarifying the expected input format or simplifying to parse the amount as a number directly.
Map<String, dynamic> _getAmountDisplayInfo(String amount, bool isAsset) { - // 計算負號個數 - int negativeCount = '-'.allMatches(amount).length; - - // Asset 帳戶需要在負號個數上 +1 進行微調 - if (isAsset) { - negativeCount += 1; - } - - // 移除所有負號以獲取純數字 - String cleanAmount = amount.replaceAll('-', ''); - - // 偶數個負號 = 正數，奇數個負號 = 負數 - bool isPositive = negativeCount % 2 == 0; + // Parse the amount as a double for proper sign handling + final parsedAmount = double.tryParse(amount) ?? 0.0; + String cleanAmount = parsedAmount.abs().toStringAsFixed(2); + + // For asset accounts: positive amount = income (green), negative = expense (red) + // For liability accounts: positive amount = debt increase (red), negative = payment (green) + bool isPositive = isAsset ? parsedAmount >= 0 : parsedAmount < 0; return { 'isPositive': isPositive, 'displayAmount': cleanAmount, 'color': isPositive ? Colors.green : Colors.red, 'icon': isPositive ? Icons.arrow_upward : Icons.arrow_downward, 'prefix': isPositive ? '' : '-', }; }
324-391: Duplicate computation of amount display info.
_getAmountDisplayInfois called twice with the same arguments for each transaction item (once for the icon container and once for the amount text). Consider computing it once and reusing the result.child: Padding( padding: const EdgeInsets.all(16), - child: Row( + child: Builder( + builder: (context) { + final displayInfo = _getAmountDisplayInfo( + transaction.amount, + widget.account.isAsset, + ); + return Row( children: [ if (_isSelectionMode) // ... checkbox code ... - Builder( - builder: (context) { - final displayInfo = _getAmountDisplayInfo( - transaction.amount, - widget.account.isAsset, - ); - return Container( + Container( // ... use displayInfo ... - ); - }, - ), + ), // ... rest of row using same displayInfo ... + ); + }, ),mobile/lib/screens/login_screen.dart (2)
145-154: Email validation is minimal.The current validation only checks for the presence of
@, which would accept invalid emails like@ortest@. Consider using a more robust email validation pattern.validator: (value) { if (value == null || value.isEmpty) { return 'Please enter your email'; } - if (!value.contains('@')) { + // Basic email pattern check + final emailRegex = RegExp(r'^[^@\s]+@[^@\s]+\.[^@\s]+$'); + if (!emailRegex.hasMatch(value)) { return 'Please enter a valid email'; } return null; },
42-46: Local MFA state may desync with provider.The local
_showOtpFieldstate is initialized tofalsebut depends onauthProvider.mfaRequired. If the widget is remounted while the provider retainsmfaRequired = true, the OTP field won't show until another failed login attempt.Consider initializing
_showOtpFieldfrom the provider ininitStateor deriving it directly from the provider in the build method.+ @override + void initState() { + super.initState(); + // Sync with provider's MFA state on mount + WidgetsBinding.instance.addPostFrameCallback((_) { + final authProvider = Provider.of<AuthProvider>(context, listen: false); + if (authProvider.mfaRequired && !_showOtpField) { + setState(() => _showOtpField = true); + } + }); + }mobile/lib/screens/backend_config_screen.dart (2)
78-81: Raw exception messages may expose internal details.Displaying
e.toString()directly to users could leak internal implementation details or confusing technical messages. Consider providing user-friendly error messages.} catch (e) { setState(() { - _errorMessage = 'Connection failed: ${e.toString()}'; + _errorMessage = 'Connection failed. Please check the URL and your network connection.'; }); } finally {
111-114: Same raw exception exposure in save error handling.Apply the same user-friendly error message pattern here.
} catch (e) { setState(() { - _errorMessage = 'Failed to save URL: ${e.toString()}'; + _errorMessage = 'Failed to save configuration. Please try again.'; }); } finally {mobile/lib/screens/dashboard_screen.dart (3)
66-87: USD and TWD use the same currency symbol.Both USD and TWD return
$as the symbol. While TWD does use the dollar sign, the New Taiwan Dollar symbol is typicallyNT$to distinguish it from other dollar currencies.case 'USD': return '\$'; case 'TWD': - return '\$'; + return 'NT\$'; case 'BTC':
50-64: Consider using a record type instead ofList<String>.Returning
List<String>for a fixed two-element tuple loses type information. Dart 3 records provide better type safety.- List<String> _formatCurrencyItem(String currency, double amount) { + ({String currency, String formatted}) _formatCurrencyItem(String currency, double amount) { final symbol = _getCurrencySymbol(currency); // ... formatting logic ... - return [currency, '$symbol$finalAmount']; + return (currency: currency, formatted: '$symbol$finalAmount'); }Then update the
_SummaryCardcallback signature accordingly.
40-43: String-based error type comparison is fragile.Comparing
errorMessage == 'unauthorized'relies on an exact string match. Consider using an enum or constant to represent error types for better maintainability.mobile/lib/models/account.dart (1)
32-40: Consider logging malformed balance values instead of silently returning 0.0.The try-catch silently swallows parsing failures, which could mask data quality issues from the backend. While the fallback is safe, consider adding debug logging to help identify problematic data.
double get balanceAsDouble { try { final cleanedBalance = balance.replaceAll(RegExp(r'[^\d.-]'), ''); return double.parse(cleanedBalance); } catch (e) { + // Consider: debugPrint('Failed to parse balance: $balance'); return 0.0; } }mobile/lib/providers/auth_provider.dart (1)
68-82: Add defensive checks for map key access to prevent runtime errors.The result map access assumes keys like
'success','tokens','mfa_required'always exist. If AuthService changes or returns unexpected data, this could fail.- if (result['success'] == true) { - _tokens = result['tokens'] as AuthTokens?; - _user = result['user'] as User?; + if (result['success'] as bool? ?? false) { + _tokens = result['tokens'] as AuthTokens?; + _user = result['user'] as User?; _isLoading = false; notifyListeners(); return true; } else { - if (result['mfa_required'] == true) { + if (result['mfa_required'] as bool? ?? false) { _mfaRequired = true; } - _errorMessage = result['error'] as String?; + _errorMessage = result['error'] as String? ?? 'Login failed';mobile/lib/main.dart (1)
31-92: Consider extracting shared theme properties to reduce duplication.The light and dark themes share identical
appBarTheme,cardTheme,inputDecorationTheme, andelevatedButtonTheme. Extract common properties into a helper function or constants:// Example: Extract common theme data ThemeData _buildTheme(Brightness brightness) { final colorScheme = ColorScheme.fromSeed( seedColor: const Color(0xFF6366F1), brightness: brightness, ); return ThemeData( colorScheme: colorScheme, useMaterial3: true, appBarTheme: const AppBarTheme(centerTitle: true, elevation: 0), cardTheme: CardThemeData( elevation: 2, shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(12)), ), // ... other shared properties ); }mobile/lib/models/auth_tokens.dart (2)
47-52: Consider adding a small buffer for clock skew inisExpired.Token validation without buffer could cause failures if client/server clocks differ slightly. A 30-60 second buffer is common practice:
bool get isExpired { + const clockSkewBuffer = 60; // seconds final expirationTime = DateTime.fromMillisecondsSinceEpoch( - (createdAt + expiresIn) * 1000, + (createdAt + expiresIn - clockSkewBuffer) * 1000, ); return DateTime.now().isAfter(expirationTime); }
27-35:_parseToInthelper correctly handles int/String but consider handling doubles.If the backend ever sends
expires_inas a double (e.g.,3600.0), this will throw. Adding double handling would improve robustness:static int _parseToInt(dynamic value) { if (value is int) { return value; } else if (value is String) { return int.parse(value); + } else if (value is double) { + return value.toInt(); } else { throw FormatException('Cannot parse $value to int'); } }mobile/lib/services/auth_service.dart (1)
205-215: Consider usinguser.toJson()for consistency.Unlike
_saveTokenswhich usestokens.toJson(),_saveUsermanually constructs the JSON object. This can lead to inconsistencies if theUsermodel changes. IfUserhas atoJson()method, use it for consistency.Future<void> _saveUser(User user) async { await _storage.write( key: _userKey, - value: jsonEncode({ - 'id': user.id, - 'email': user.email, - 'first_name': user.firstName, - 'last_name': user.lastName, - }), + value: jsonEncode(user.toJson()), ); }mobile/lib/services/device_service.dart (2)
18-25: Hardcodedapp_versionshould be dynamic.The
app_versionis hardcoded to'1.0.0'. Consider using thepackage_info_pluspackage to retrieve the actual app version dynamically.
27-32: Weak device ID generation - consider using UUID.The current implementation derives both parts of the ID from the same timestamp, providing no additional entropy.
timestamp.hashCodeis deterministic, so two devices generating IDs at the same millisecond would get the same ID.+import 'package:uuid/uuid.dart'; + String _generateDeviceId() { - // Generate a unique device ID - final timestamp = DateTime.now().millisecondsSinceEpoch; - final random = timestamp.toString().hashCode.abs(); - return 'sure_mobile_${timestamp}_$random'; + return 'sure_mobile_${const Uuid().v4()}'; }Alternatively, if you prefer not to add a dependency, use
Random.secure():import 'dart:math'; String _generateDeviceId() { final random = Random.secure(); final bytes = List<int>.generate(16, (_) => random.nextInt(256)); return 'sure_mobile_${bytes.map((b) => b.toRadixString(16).padLeft(2, '0')).join()}'; }mobile/lib/screens/transaction_form_screen.dart (4)
31-39: Make date handling more robust and state‑drivenRight now the selected date is stored only as a formatted string and transformed for the API via
split('/'), and the date picker always opens onDateTime.now()instead of the currently selected date. Consider tracking aDateTime _selectedDatein state and deriving both_dateController.textand the APIyyyy-MM-ddstring from it. This avoids potentialRangeErrorif the text ever deviates from the expected format and improves UX by reopening the picker on the last selection.- final _dateController = TextEditingController(); + final _dateController = TextEditingController(); + DateTime _selectedDate = DateTime.now(); @override void initState() { super.initState(); - final now = DateTime.now(); - final formattedDate = DateFormat('yyyy/MM/dd').format(now); - _dateController.text = formattedDate; + _dateController.text = DateFormat('yyyy/MM/dd').format(_selectedDate); _nameController.text = 'SureApp'; } Future<void> _selectDate() async { - final DateTime? picked = await showDatePicker( + final DateTime? picked = await showDatePicker( context: context, - initialDate: DateTime.now(), + initialDate: _selectedDate, firstDate: DateTime(2000), lastDate: DateTime(2100), ); if (picked != null) { setState(() { - _dateController.text = DateFormat('yyyy/MM/dd').format(picked); + _selectedDate = picked; + _dateController.text = DateFormat('yyyy/MM/dd').format(picked); }); } } - // Convert date format from yyyy/MM/dd to yyyy-MM-dd - final dateParts = _dateController.text.split('/'); - final apiDate = '${dateParts[0]}-${dateParts[1]}-${dateParts[2]}'; + final apiDate = DateFormat('yyyy-MM-dd').format(_selectedDate);Also applies to: 66-79, 107-110
49-64: Consider enforcing numeric input format for amountValidation is solid, but without an
inputFormatter, users can still enter non‑numeric characters (or locale‑specific separators like,) that will faildouble.tryParse. Consider adding aFilteringTextInputFormatter.allowfor digits and.or using a locale‑aware money input widget to reduce validation errors at the source.Also applies to: 295-306
81-105: Align unauthorized handling and session‑expired UXYou handle a missing/expired token (
accessToken == null) with a clear “Session expired” snackbar pluslogout(), but a 401 from the create API surfaces the raw'unauthorized'string and separately callslogout(). For consistency and clearer UX, consider mapping the'unauthorized'error path to the same user‑friendly message and flow as the null‑token case (and optionally closing the sheet after logout).- } else { - final error = result['error'] ?? 'Failed to create transaction'; - ScaffoldMessenger.of(context).showSnackBar( - SnackBar( - content: Text(error), - backgroundColor: Colors.red, - ), - ); - - if (error == 'unauthorized') { - await authProvider.logout(); - } - } + } else { + final error = result['error'] ?? 'Failed to create transaction'; + + if (error == 'unauthorized') { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('Session expired. Please login again.'), + backgroundColor: Colors.red, + ), + ); + await authProvider.logout(); + Navigator.of(context).pop(); // optionally close the sheet + } else { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text(error), + backgroundColor: Colors.red, + ), + ); + } + }Also applies to: 122-143
25-26: Consider injectingTransactionsServicefor testabilityCreating
TransactionsServicedirectly in state ties the widget to a concrete implementation and makes it harder to mock in tests. If you expect to unit‑test this screen or swap implementations, consider providingTransactionsServiceviaProvider/GetItor passing it in through the widget constructor.mobile/lib/services/transactions_service.dart (1)
168-200: Multi‑delete aggregation is straightforward; consider exposing partial success infoRunning
deleteTransactioncalls in parallel and aggregating the results withallSuccesskeeps the implementation simple. If you need more granular feedback later, you might extend the error branch to also report how many succeeded (and/or which IDs failed), but the current shape is adequate for a first pass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
mobile/android/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngmobile/android/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngmobile/android/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngmobile/android/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngmobile/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngmobile/assets/icon/app_icon.pngis excluded by!**/*.png
📒 Files selected for processing (52)
.github/workflows/flutter-build.yml(1 hunks)mobile/.gitignore(1 hunks)mobile/README.md(1 hunks)mobile/analysis_options.yaml(1 hunks)mobile/android/app/build.gradle(1 hunks)mobile/android/app/src/main/AndroidManifest.xml(1 hunks)mobile/android/app/src/main/kotlin/com/sure/mobile/MainActivity.kt(1 hunks)mobile/android/app/src/main/res/drawable/launch_background.xml(1 hunks)mobile/android/app/src/main/res/values/styles.xml(1 hunks)mobile/android/build.gradle(1 hunks)mobile/android/gradle.properties(1 hunks)mobile/android/gradle/wrapper/gradle-wrapper.properties(1 hunks)mobile/android/settings.gradle(1 hunks)mobile/assets/images/.gitkeep(1 hunks)mobile/docs/SIGNING_SETUP.md(1 hunks)mobile/docs/TECHNICAL_GUIDE.md(1 hunks)mobile/docs/iOS_BUILD.md(1 hunks)mobile/ios/Flutter/AppFrameworkInfo.plist(1 hunks)mobile/ios/Flutter/Debug.xcconfig(1 hunks)mobile/ios/Flutter/Release.xcconfig(1 hunks)mobile/ios/Podfile(1 hunks)mobile/ios/Runner.xcodeproj/project.pbxproj(1 hunks)mobile/ios/Runner/AppDelegate.swift(1 hunks)mobile/ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json(1 hunks)mobile/ios/Runner/Assets.xcassets/LaunchImage.imageset/Contents.json(1 hunks)mobile/ios/Runner/Base.lproj/LaunchScreen.storyboard(1 hunks)mobile/ios/Runner/Base.lproj/Main.storyboard(1 hunks)mobile/ios/Runner/GeneratedPluginRegistrant.h(1 hunks)mobile/ios/Runner/GeneratedPluginRegistrant.m(1 hunks)mobile/ios/Runner/Info.plist(1 hunks)mobile/ios/Runner/Runner-Bridging-Header.h(1 hunks)mobile/lib/main.dart(1 hunks)mobile/lib/models/account.dart(1 hunks)mobile/lib/models/auth_tokens.dart(1 hunks)mobile/lib/models/transaction.dart(1 hunks)mobile/lib/models/user.dart(1 hunks)mobile/lib/providers/accounts_provider.dart(1 hunks)mobile/lib/providers/auth_provider.dart(1 hunks)mobile/lib/providers/transactions_provider.dart(1 hunks)mobile/lib/screens/backend_config_screen.dart(1 hunks)mobile/lib/screens/dashboard_screen.dart(1 hunks)mobile/lib/screens/login_screen.dart(1 hunks)mobile/lib/screens/transaction_form_screen.dart(1 hunks)mobile/lib/screens/transactions_list_screen.dart(1 hunks)mobile/lib/services/accounts_service.dart(1 hunks)mobile/lib/services/api_config.dart(1 hunks)mobile/lib/services/auth_service.dart(1 hunks)mobile/lib/services/device_service.dart(1 hunks)mobile/lib/services/transactions_service.dart(1 hunks)mobile/lib/widgets/account_card.dart(1 hunks)mobile/pubspec.yaml(1 hunks)mobile/test/widget_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account model is a Rails delegated type with subtypes: Depository, Investment, Crypto, Property, Vehicle, OtherAsset (assets) and CreditCard, Loan, OtherLiability (liabilities)
Applied to files:
mobile/lib/models/account.dart
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Domain model structure: User → has many Accounts → has many Transactions; Account types include checking, savings, credit cards, investments, crypto, loans, properties
Applied to files:
mobile/lib/models/account.dart
🪛 Clang (14.0.6)
mobile/ios/Runner/Runner-Bridging-Header.h
[error] 1-1: 'Flutter/Flutter.h' file not found
(clang-diagnostic-error)
mobile/ios/Runner/GeneratedPluginRegistrant.h
[error] 1-1: 'Flutter/Flutter.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
mobile/README.md
[uncategorized] ~146-~146: The official name of this software platform is spelled with a capital “H”.
Context: ...des automated CI/CD via GitHub Actions (.github/workflows/flutter-build.yml): - **Tri...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
mobile/docs/TECHNICAL_GUIDE.md
9-9: Bare URL used
(MD034, no-bare-urls)
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
455-455: Bare URL used
(MD034, no-bare-urls)
456-456: Bare URL used
(MD034, no-bare-urls)
457-457: Bare URL used
(MD034, no-bare-urls)
mobile/README.md
34-34: Bare URL used
(MD034, no-bare-urls)
85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
This commit addresses the critical issues identified in the mobile-support PR: 1. **GitHub Actions Workflow Path Issues (Critical)** - Add mobile/ prefix to all path filters in flutter-build.yml - Add working-directory to all Flutter commands - Fix Android keystore and iOS CocoaPods paths - Fix artifact upload paths 2. **Error Handling Improvements** - Add try-catch blocks to all HTTP requests in services - Wrap all JSON parsing operations in error handling - Add proper error messages for network failures 3. **HTTP Request Timeout Configuration** - Add 30-second timeout to all HTTP requests - Prevents hanging on network failures 4. **Defensive Null Checks in Providers** - Add containsKey() checks before accessing result maps - Add proper type casting with null safety - Add fallback error messages These changes ensure the workflow triggers correctly on mobile/ directory changes and improves overall code robustness.
…jzYuZUBLFExbCNsz Fix mobile support issues in PR we-promise#426
This commit addresses the critical issues identified in the mobile-support PR: 1. **GitHub Actions Workflow Path Issues (Critical)** - Add mobile/ prefix to all path filters in flutter-build.yml - Add working-directory to all Flutter commands - Fix Android keystore and iOS CocoaPods paths - Fix artifact upload paths 2. **Error Handling Improvements** - Add try-catch blocks to all HTTP requests in services - Wrap all JSON parsing operations in error handling - Add proper error messages for network failures 3. **HTTP Request Timeout Configuration** - Add 30-second timeout to all HTTP requests - Prevents hanging on network failures 4. **Defensive Null Checks in Providers** - Add containsKey() checks before accessing result maps - Add proper type casting with null safety - Add fallback error messages These changes ensure the workflow triggers correctly on mobile/ directory changes and improves overall code robustness.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
mobile/lib/services/auth_service.dart (2)
141-145: Overly broad exception handling leaks internal details.Same issue as in the
loginmethod—all exceptions are caught and internal error details are exposed.Apply the same fix as suggested for the
loginmethod to categorize exceptions properly.
184-188: Overly broad exception handling leaks internal details.Same exception handling issue as in
loginandsignup.Apply the same categorized exception handling fix.
🧹 Nitpick comments (10)
mobile/lib/providers/transactions_provider.dart (1)
6-6: Consider dependency injection for TransactionsService.The service instance is hardcoded, making testing difficult and reducing flexibility. Consider accepting the service via constructor parameter to enable dependency injection and improve testability.
Apply this diff to inject the service:
-class TransactionsProvider with ChangeNotifier { - final TransactionsService _transactionsService = TransactionsService(); +class TransactionsProvider with ChangeNotifier { + final TransactionsService _transactionsService; + + TransactionsProvider({TransactionsService? transactionsService}) + : _transactionsService = transactionsService ?? TransactionsService();mobile/lib/services/transactions_service.dart (2)
91-99: Document or standardize the API response format.The code handles both array and object-with-transactions-key responses. This flexibility suggests the API contract is unclear or inconsistent. If possible, standardize the backend to return a consistent shape.
If standardization isn't feasible, add a comment explaining why both formats are supported (e.g., versioning, multiple endpoints).
173-178: Consider fail-fast behavior for batch deletions.
Future.waitexecutes all deletions in parallel. If the first deletion fails due to an expired token (401), the remaining deletions will also fail but are still attempted. Consider checking the first failure and short-circuiting for auth errors.Alternatively, document that this behavior is intentional (e.g., for resilience or partial success tracking).
mobile/lib/services/auth_service.dart (1)
226-236: Preferuser.toJson()for consistency and maintainability.Unlike
_saveTokens(which usestokens.toJson()),_saveUsermanually constructs the JSON map. This inconsistency increases maintenance overhead—if theUsermodel adds fields, this method may not persist them.If
Userhas atoJson()method, use it:Future<void> _saveUser(User user) async { await _storage.write( key: _userKey, - value: jsonEncode({ - 'id': user.id, - 'email': user.email, - 'first_name': user.firstName, - 'last_name': user.lastName, - }), + value: jsonEncode(user.toJson()), ); }If not, consider adding
toJson()to theUsermodel for consistency.mobile/lib/providers/accounts_provider.dart (6)
5-17: Consider injectingAccountsServiceand returning an unmodifiableaccountsviewTwo small robustness tweaks here:
- Make
_accountsServiceinjectable (e.g., via a constructor parameter) to ease testing and future mocking.- Expose
accountsas an unmodifiable view so callers can’t mutate the backing list without going through the provider (and withoutnotifyListeners()).For example:
-import '../models/account.dart'; -import '../services/accounts_service.dart'; +import '../models/account.dart'; +import '../services/accounts_service.dart'; +import 'dart:collection'; @@ -class AccountsProvider with ChangeNotifier { - final AccountsService _accountsService = AccountsService(); +class AccountsProvider with ChangeNotifier { + final AccountsService _accountsService; + + AccountsProvider({AccountsService? accountsService}) + : _accountsService = accountsService ?? AccountsService(); @@ - List<Account> get accounts => _accounts; + UnmodifiableListView<Account> get accounts => + UnmodifiableListView(_accounts);This keeps state changes centralized in the provider and makes it harder to misuse from the UI layer.
18-28: Derived asset/liability lists look good; watch for repeated sorting cost only if lists grow largeThe filtering +
_sortAccountscall on a copied list is clean and side-effect free on_accounts. The only trade-off is repeated sorting every time the getter is accessed; that’s fine for small lists, but if you end up with many accounts and these getters are hit frequently (e.g., in rebuild-heavy widgets), you might later want to:
- Cache sorted results until
_accountschanges, or- Pre-sort
_accountsand filter without re-sorting.No change strictly needed now; just something to keep in mind if performance ever becomes a concern.
30-44: Currency total logic is correct; consider a small helper to DRY up asset/liability aggregationBoth totals getters implement the same aggregation pattern with only the predicate differing. You could centralize that into a private helper for readability and to avoid divergence if the logic evolves:
- Map<String, double> get assetTotalsByCurrency { - final totals = <String, double>{}; - for (var account in _accounts.where((a) => a.isAsset)) { - totals[account.currency] = (totals[account.currency] ?? 0.0) + account.balanceAsDouble; - } - return totals; - } + Map<String, double> _totalsByCurrency(bool Function(Account) filter) { + final totals = <String, double>{}; + for (final account in _accounts.where(filter)) { + totals[account.currency] = + (totals[account.currency] ?? 0.0) + account.balanceAsDouble; + } + return totals; + } + + Map<String, double> get assetTotalsByCurrency => + _totalsByCurrency((a) => a.isAsset); @@ - Map<String, double> get liabilityTotalsByCurrency { - final totals = <String, double>{}; - for (var account in _accounts.where((a) => a.isLiability)) { - totals[account.currency] = (totals[account.currency] ?? 0.0) + account.balanceAsDouble; - } - return totals; - } + Map<String, double> get liabilityTotalsByCurrency => + _totalsByCurrency((a) => a.isLiability);Pure readability/maintenance win; functionality stays the same.
46-63: Sorting comparator is clear; confirm that the chosen priority matches product expectationsThe multi-key sort (type → currency → balance desc → name) is explicit and well-commented. Since this impacts what users see first, it’s worth double-checking:
- That
accountTypeandcurrencyvalues are stable and won’t change shape (e.g., backend renaming or localization), and- That this ordering matches how accounts are sorted in other clients (web/PWA) so users get a consistent experience.
If there’s a domain enum for type or classification, consider sorting on that instead of raw strings to make the intent more robust.
65-99:fetchAccountsflow is solid; suggest typed result andfinallyto centralize loading-state resetBehavior-wise this looks correct: loading flag is set before the call, accounts/pagination are updated only on success, errors are surfaced via
_errorMessage, and the method returns a simple success flag.Two improvements to consider:
- Avoid stringly-typed result maps from
AccountsServiceUsing
result['success'],result['accounts'],result['pagination'],result['error']is fragile and forces casts likecast<Account>(). If feasible, changeAccountsService.getAccountsto return a typed object or record, e.g.:class AccountsFetchResult { final bool success; final List<Account> accounts; final Map<String, dynamic>? pagination; final String? error; // ... }Then this method can become strongly typed and safer to refactor.
- Use
finallyto guarantee_isLoadingreset and avoid duplicationYou currently repeat
_isLoading = false; notifyListeners();in all branches. Afinallyblock centralizes that and protects you if new return paths are added later:Future<bool> fetchAccounts({ @@ - try { + try { final result = await _accountsService.getAccounts( @@ - if (result['success'] == true && result.containsKey('accounts')) { - _accounts = (result['accounts'] as List<dynamic>?)?.cast<Account>() ?? []; - _pagination = result['pagination'] as Map<String, dynamic>?; - _isLoading = false; - notifyListeners(); - return true; - } else { - _errorMessage = result['error'] as String? ?? 'Failed to fetch accounts'; - _isLoading = false; - notifyListeners(); - return false; - } - } catch (e) { - _errorMessage = 'Connection error. Please check your internet connection.'; - _isLoading = false; - notifyListeners(); - return false; - } + if (result['success'] == true && result.containsKey('accounts')) { + _accounts = + (result['accounts'] as List<dynamic>?)?.cast<Account>() ?? []; + _pagination = result['pagination'] as Map<String, dynamic>?; + return true; + } else { + _errorMessage = + result['error'] as String? ?? 'Failed to fetch accounts'; + return false; + } + } catch (e) { + _errorMessage = + 'Connection error. Please check your internet connection.'; + return false; + } finally { + _isLoading = false; + notifyListeners(); + }One UX point to double-check: on failure you keep the previous
_accountsand_pagination, which can be nice for offline/temporary-error behavior. Confirm that this is intentional versus wanting to show an explicitly “empty” state on error.
101-111: Clear helpers are fine; consider resetting_isLoadingand reusingclearError()
clearAccountsandclearErrordo what they say. Two minor polish ideas:
clearAccountsmight also reset_isLoadingtofalsein case it’s ever called during a loading state (e.g., on logout while a fetch is in-flight), so the UI can’t get stuck showing a spinner.- You can reuse
clearError()fromclearAccounts()to avoid duplicating the error-reset logic.For example:
void clearAccounts() { _accounts = []; _pagination = null; - _errorMessage = null; - notifyListeners(); + _isLoading = false; + clearError(); }And keep
clearError()as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/flutter-build.yml(1 hunks)mobile/lib/providers/accounts_provider.dart(1 hunks)mobile/lib/providers/transactions_provider.dart(1 hunks)mobile/lib/services/accounts_service.dart(1 hunks)mobile/lib/services/auth_service.dart(1 hunks)mobile/lib/services/transactions_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mobile/lib/services/accounts_service.dart
- .github/workflows/flutter-build.yml
🔇 Additional comments (6)
mobile/lib/providers/transactions_provider.dart (1)
32-32: Verify the cast operation is safe.The code casts
result['transactions']toList<dynamic>and then calls.cast<Transaction>(). This assumes the service already returns a list ofTransactionobjects. If the service returns raw JSON maps instead, the cast will fail at runtime. Verify thatTransactionsService.getTransactionsproperly deserializes the response intoTransactionobjects before returning.mobile/lib/services/auth_service.dart (5)
1-11: LGTM! Clean setup.The imports, storage initialization, and constant definitions follow Flutter best practices.
192-217: LGTM! Robust read operations with appropriate error handling.The
logout,getStoredTokens, andgetStoredUsermethods handle storage operations and JSON parsing errors gracefully by returning null on failures.
219-224: LGTM! Consistent serialization approach.Uses
tokens.toJson()for serialization, which is the standard Flutter pattern.
20-20: Verify HTTPS is enforced in ApiConfig.baseUrl.Credentials (email/password) are transmitted in the request body. Ensure that
ApiConfig.baseUrlstrictly uses HTTPS to prevent credential exposure over insecure channels. Check the ApiConfig definition to confirm the baseUrl is hardcoded with HTTPS protocol and cannot be overridden with HTTP.
226-236: Verify User model has toJson() method.Check whether the
Usermodel provides atoJson()method to enable consistent serialization instead of manually constructing the JSON object in_saveUser().
Will register Google Play credentials and set this up. 👍 Thanks for kicking things off! |
|
Login fails when account has Two-Factor Authentication enabled Issue: Steps to reproduce:
Error message: |
|
@jjmata I can help publishing on iOS if you guys need |
Excellent! What I really want to jump on is making the chatbot available under the Flutter codebase. Want to look into that? ;-) |
- Add UnmodifiableListView to transactions getter to prevent external mutation - Call notifyListeners() immediately after setting _isLoading = false - Move jsonDecode to run only after successful statusCode verification - Replace string concatenation with Uri.replace() for proper URL encoding - Add try/catch for jsonDecode on non-2xx responses to handle non-JSON errors
- Replace broad catch-all exception handlers with targeted exception handling - Add specific catches for SocketException, TimeoutException, HttpException, FormatException, and TypeError - Return safe, user-friendly error messages instead of exposing internal details - Log full exception details and stack traces using debugPrint for debugging - Fix duplicate User.fromJson calls in login and signup methods by parsing once and reusing the instance - Improve code efficiency and security by preventing information leakage
…mobile-support
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
mobile/lib/providers/transactions_provider.dart (3)
7-7: Consider injectingTransactionsServicefor testability and flexibility.Directly instantiating
TransactionsServicehere couples the provider to a concrete implementation and makes testing harder. Consider constructor injection so you can pass a mock or alternate implementation in tests or in different environments.-class TransactionsProvider with ChangeNotifier { - final TransactionsService _transactionsService = TransactionsService(); +class TransactionsProvider with ChangeNotifier { + final TransactionsService _transactionsService; + + TransactionsProvider({TransactionsService? transactionsService}) + : _transactionsService = transactionsService ?? TransactionsService();
17-41: StreamlinefetchTransactionsstate updates and confirm error handling.You currently call
notifyListeners()three times: once for starting load, once when setting_isLoading = false, and once after updating_transactions/_error. This causes extra rebuilds and briefly exposes a “not loading but still old data” state. You can update all post‑request state in one block and notify once.Also, if
TransactionsService.getTransactionscan throw (e.g., network error),_isLoadingmay remaintrueand listeners won’t see an error. Please confirm the service always returns a result map and never throws, or wrap the call intry/catchand set_isLoading/_erroraccordingly.Future<void> fetchTransactions({ required String accessToken, String? accountId, }) async { _isLoading = true; _error = null; notifyListeners(); - final result = await _transactionsService.getTransactions( - accessToken: accessToken, - accountId: accountId, - ); - - _isLoading = false; - notifyListeners(); - - if (result['success'] == true && result.containsKey('transactions')) { - _transactions = (result['transactions'] as List<dynamic>?)?.cast<Transaction>() ?? []; - _error = null; - } else { - _error = result['error'] as String? ?? 'Failed to fetch transactions'; - } - - notifyListeners(); + try { + final result = await _transactionsService.getTransactions( + accessToken: accessToken, + accountId: accountId, + ); + + if (result['success'] == true && result.containsKey('transactions')) { + _transactions = + (result['transactions'] as List<dynamic>?)?.cast<Transaction>() ?? []; + _error = null; + } else { + _error = result['error'] as String? ?? 'Failed to fetch transactions'; + } + } catch (_) { + _error = 'Failed to fetch transactions'; + } finally { + _isLoading = false; + notifyListeners(); + } }Please double‑check
TransactionsService.getTransactionsand adjust thetry/catchif it already normalizes all failures into theresultmap.
63-80: Optimize membership check when deleting multiple transactions.Using
transactionIds.contains(t.id)insideremoveWheremakes the operation O(n × m) fornlocal transactions andmIDs. ConvertingtransactionIdsto aSetfirst gives O(1) membership checks and improves performance for larger lists.Future<bool> deleteMultipleTransactions({ required String accessToken, required List<String> transactionIds, }) async { + final idsSet = transactionIds.toSet(); + final result = await _transactionsService.deleteMultipleTransactions( accessToken: accessToken, - transactionIds: transactionIds, + transactionIds: transactionIds, ); if (result['success'] == true) { - _transactions.removeWhere((t) => transactionIds.contains(t.id)); + _transactions.removeWhere((t) => idsSet.contains(t.id)); notifyListeners(); return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mobile/lib/providers/transactions_provider.dart(1 hunks)mobile/lib/services/auth_service.dart(1 hunks)mobile/lib/services/transactions_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mobile/lib/services/transactions_service.dart
- mobile/lib/services/auth_service.dart
🔇 Additional comments (1)
mobile/lib/providers/transactions_provider.dart (1)
9-15: Good use of encapsulation for transactions and loading/error state.Exposing an unmodifiable view for
transactionsand read‑only getters forisLoadinganderrorfits well with the provider pattern and avoids accidental external mutation.
Mobile support
Fixed the crash that occurred when logging in with 2FA-enabled accounts and improved the user experience by not showing error messages when MFA is required (it's a normal flow, not an error). Changes: - Added mounted check before setState() in login screen - Modified AuthProvider to not set error message when MFA is required - Ensures smooth transition from password entry to OTP entry - Prevents "setState() called after dispose()" error The flow now works correctly: 1. User enters email/password → clicks Sign In 2. Backend responds with mfa_required 3. OTP input field appears with friendly blue prompt (no red error) 4. User enters 6-digit code → clicks Sign In again 5. Login succeeds
Added comprehensive debug logging to understand why OTP field is not showing when MFA is required: - Log backend response status and body - Log login result in AuthProvider - Log MFA required state - Log when OTP field should be shown This will help identify if the issue is: 1. Backend not returning mfa_required flag 2. Response parsing issue 3. State management issue 4. UI rendering issue
PROBLEM: The LoginScreen was being recreated when AuthProvider called notifyListeners(), causing all internal state (_showOtpField) to be lost. This resulted in the OTP input field never appearing, making 2FA login impossible. ROOT CAUSE: The AppWrapper uses a Consumer<AuthProvider> that rebuilds the entire widget tree when auth state changes. When login() sets isLoading=false and calls notifyListeners(), a brand new LoginScreen instance is created, resetting all internal state. SOLUTION: - Moved _showMfaInput state from LoginScreen to AuthProvider - AuthProvider now manages when to show the MFA input field - LoginScreen uses Consumer to read this state reactively - State survives widget rebuilds FLOW: 1. User enters email/password → clicks Sign In 2. Backend responds with mfa_required: true 3. AuthProvider sets _showMfaInput = true 4. Consumer rebuilds, showing OTP field (state preserved) 5. User enters code → clicks Sign In 6. Backend validates → returns tokens → login succeeds Backend is confirmed working via tests (auth_controller_test.rb).
…ucmZ4ShNS9G7vBrGF Fix 2FA login crash caused by setState after dispose
Problem: When 2FA is required during mobile login, the LoginScreen was being destroyed and recreated, causing text controllers to reset and forcing users to re-enter their credentials. Root cause: AppWrapper was checking authProvider.isLoading and showing a full-screen loading indicator during login attempts. This caused LoginScreen to be unmounted when isLoading=true, destroying the State and text controllers. When the backend returned mfa_required, isLoading=false triggered recreation of LoginScreen with empty fields. Solution: - Add isInitializing state to AuthProvider to distinguish initial auth check from active login attempts - Update AppWrapper to only show loading spinner during isInitializing, not during login flow - LoginScreen now persists across login attempts, preserving entered credentials Flow after fix: 1. User enters email/password 2. LoginScreen stays mounted (shows loading in button only) 3. Backend returns mfa_required 4. MFA field appears, email/password fields retain values 5. User enters OTP and submits (email/password automatically included) Files changed: - mobile/lib/providers/auth_provider.dart: Add isInitializing state - mobile/lib/main.dart: Use isInitializing instead of isLoading in AppWrapper
When users enter an incorrect OTP code during 2FA login, the app now: - Displays an error message indicating the code was invalid - Keeps the MFA input field visible for retry - Automatically clears the OTP field for easy re-entry Changes: - mobile/lib/providers/auth_provider.dart: * Distinguish between first MFA request vs invalid OTP error * Show error message when OTP code was submitted but invalid * Keep MFA input visible when in MFA flow with errors - mobile/lib/screens/login_screen.dart: * Clear OTP field after failed login attempt * Improve UX by allowing easy retry without re-entering credentials User flow after fix: 1. User enters email/password 2. MFA required - OTP field appears 3. User enters wrong OTP 4. Error message shows "Two-factor authentication required" 5. OTP field clears, ready for new code 6. User can immediately retry without re-entering email/password
When user enters an invalid OTP code, show clearer error message "Invalid authentication code. Please try again." instead of the confusing "Two-factor authentication required" from backend. This makes it clear that the OTP was wrong, not that they need to start the 2FA process.
…qyLpzjV7z46V3JMMau Fix mobile 2FA login requiring double password entry
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test_2fa_flow.md (1)
20-27: Specify language for code block.The code block on line 20 lacks a language identifier for syntax highlighting. While the static content is clear, adding a language specifier improves readability.
Apply this diff:
-3. **Check the console output for these debug messages:** -``` +3. **Check the console output for these debug messages:** +```text Login response status: 401 Login response body: {"error":"Two-factor authentication required","mfa_required":true} Login result: {success: false, mfa_required: true, error: Two-factor authentication required}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mobile/lib/main.dart(1 hunks)mobile/lib/providers/auth_provider.dart(1 hunks)mobile/lib/screens/login_screen.dart(1 hunks)mobile/lib/services/auth_service.dart(1 hunks)test_2fa_flow.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mobile/lib/providers/auth_provider.dart
- mobile/lib/main.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:56:44.818Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/self_improve.mdc:0-0
Timestamp: 2025-11-24T16:56:44.818Z
Learning: Check for consistent error handling patterns across the codebase
Applied to files:
mobile/lib/services/auth_service.dart
🪛 markdownlint-cli2 (0.18.1)
test_2fa_flow.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
mobile/lib/services/auth_service.dart (2)
16-115: Excellent error handling and optimizations.The login method has been significantly improved based on previous feedback:
- Lines 54-65: User parsing is now optimized (parse once, reuse)
- Lines 78-114: Comprehensive exception handling with specific catch blocks for SocketException, TimeoutException, HttpException, FormatException, and TypeError
- All exceptions are logged with stack traces using
debugPrint- User-facing error messages are safe and don't leak implementation details
The implementation now follows best practices for error handling. Based on learnings, the consistent error handling pattern across authentication methods is excellent.
323-333: Review comment is incorrect and should be dismissed.The
Usermodel does not have atoJson()method. WhilefromJson()exists for deserialization, there is no correspondingtoJson()method in the User class. The manual serialization in_saveUseris the appropriate approach given the current model structure. The suggestion to useuser.toJson()cannot be applied.If consistency with other models (e.g.,
AuthTokens,Transaction) is desired, consider adding atoJson()method to theUsermodel instead.Likely an incorrect or invalid review comment.
mobile/lib/screens/login_screen.dart (1)
203-203: Verify Flutter version supportswithValuesAPI.The
withValues(alpha: 0.3)syntax is a Flutter 3.27+ API that replaces the olderwithOpacitymethod. Ensure the project's Flutter version inpubspec.yamland CI/CD configuration supports this API.Run the following script to check the Flutter version constraints:
#!/bin/bash # Check Flutter SDK version constraints in pubspec.yaml echo "=== Checking Flutter SDK version in pubspec.yaml ===" cat mobile/pubspec.yaml | grep -A 2 "environment:" echo "" echo "=== Checking if withValues is documented (Flutter 3.27+) ===" echo "Please verify that the project uses Flutter 3.27 or later."
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lazy Bone <[email protected]>
Thank you for your message. I have fixed it, and 2FA verification should now work correctly. |
Excellent! Let me engage with it over the weekend and merge as soon as my developer accounts are approved/have signing keys for both builds. Google Play didn't like my docs so I have no Google Play account yet. 😦 |
|
One thing that would be nice is if we make the mobile build optional, since most people won't need it/there is no need to rebuild when there are no changes in |
I just checked, and an adjustment was made during the migration to |
5 participants
#411 After the discussion, I did some conflict resolution and merging work, moving the project to the /mobile folder.
@jjmata Currently, it needs to be configured with the environment variables required for Android signing.
#241 lightweight alternative to the current PWA implementation.
I'm not sure if any information is missing.
If there is anything missing, please point it out to me. Thank you.
Summary by CodeRabbit
New Features
CI/CD
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.