Mobile: Prepare demo app POC with hardcoded data #960
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe changes in this pull request enhance the application's handling of demo functionalities and organization data. Key updates include the introduction of the Changes
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 (
|
MDikkii
changed the title
MDikkii
force-pushed
the
mdikkii/demo-poc
branch
from
73c8b74 to
88e4263
Compare
ssandino
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (17)
recipients_app/lib/data/repositories/payment_repository.dart (1)
9-10: Remove unnecessary 'late' keywords for initialized variablesThe variables
remoteDataSourceanddemoDataSourceare initialized at declaration, so thelatekeyword is unnecessary.Apply this diff to remove the
latekeywords:-late PaymentDataSource remoteDataSource = PaymentRemoteDataSource(firestore: firestore); -late PaymentDataSource demoDataSource = PaymentDemoDataSource(); +PaymentDataSource remoteDataSource = PaymentRemoteDataSource(firestore: firestore); +PaymentDataSource demoDataSource = PaymentDemoDataSource();recipients_app/lib/data/repositories/user_repository.dart (2)
12-13: Remove unnecessary 'late' keywords for initialized variablesThe variables
remoteDataSourceanddemoDataSourceare initialized at declaration, so thelatekeyword is unnecessary.Apply this diff to remove the
latekeywords:-late UserDataSource remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth); -late UserDataSource demoDataSource = UserDemoDataSource(); +UserDataSource remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth); +UserDataSource demoDataSource = UserDemoDataSource();
42-45: Set 'onCancel' handler outside of the listenerAssigning
authStateController.onCancelinside theisDemoEnabledStreamlistener reassigns it every time the demo mode changes. Move it outside to set it once.Apply this diff to move the
onCancelassignment:+authStateController.onCancel = () { + authStateSubscription?.cancel(); +}; demoManager.isDemoEnabledStream.listen((isDemoMode) { authStateSubscription?.cancel(); authStateSubscription = _activeDataSource.authStateChanges().listen((authState) { authStateController.add(authState); }); - authStateController.onCancel = () { - authStateSubscription?.cancel(); - }; });recipients_app/lib/data/datasource/organization_data_source.dart (1)
4-8: Add documentation to the interface and method.Add dartdoc comments to describe:
- The purpose of this data source
- The expected behavior of
fetchOrganization- The meaning of a null return value
+/// Defines the contract for accessing organization data. abstract class OrganizationDataSource { + /// Fetches an organization by its Firestore reference. + /// Returns null if the organization doesn't exist. Future<Organization?> fetchOrganization( DocumentReference organizationRef, ); }recipients_app/lib/data/datasource/demo/organization_demo_data_source.dart (1)
8-10: Consider using more realistic demo dataThe hardcoded organization data could be more representative of real-world scenarios.
- return const Organization(name: "Demo organization", contactName: "Demo manager", contactNumber: "+232 123456789"); + return const Organization( + name: "Sierra Leone Social Income", + contactName: "John Smith", + contactNumber: "+232 76 123456" + );recipients_app/lib/data/datasource/remote/organization_remote_data_source.dart (1)
5-5: Remove unused constantThe
surveyCollectionconstant is not used in this file.-const String surveyCollection = "surveys";recipients_app/lib/data/repositories/survey_repository.dart (1)
10-11: Consider eager initialization of data sourcesLate initialization of data sources could lead to runtime errors if accessed before initialization. Consider moving initialization to the constructor.
- late SurveyDataSource remoteDataSource = SurveyRemoteDataSource(firestore: firestore); - late SurveyDataSource demoDataSource = SurveyDemoDataSource(); + final SurveyDataSource remoteDataSource; + final SurveyDataSource demoDataSource; + + SurveyRepository({ + required this.firestore, + required this.demoManager, + }) : remoteDataSource = SurveyRemoteDataSource(firestore: firestore), + demoDataSource = SurveyDemoDataSource();recipients_app/lib/data/datasource/demo/survey_demo_data_source.dart (1)
13-16: Document demo survey ID formatAdd documentation explaining the demo survey ID format and how it relates to production IDs. This helps maintain consistency when adding new demo surveys.
+ /// Demo survey IDs follow the format: <survey_type> + /// Production format: UUIDAlso applies to: 18-21, 23-28, 30-35
recipients_app/lib/data/datasource/demo/no_op_document_reference.dart (3)
3-4: Add class documentationAdd documentation explaining the purpose and usage of this no-op implementation.
+ /// A no-op implementation of DocumentReference for demo/testing scenarios. + /// All operations throw UnimplementedError.
8-11: Improve error messagesReplace generic UnimplementedError with descriptive messages indicating this is a demo implementation.
- throw UnimplementedError(); + throw UnimplementedError('Operation not supported in demo mode');Also applies to: 14-17, 24-27, 42-45, 55-58
20-21: Fix TODO comment formattingRemove TODO comments from getter implementations as they're redundant with the thrown error.
- // TODO: implement firestore FirebaseFirestore get firestore => throw UnimplementedError();Also applies to: 30-31, 34-35, 38-39
recipients_app/lib/data/datasource/demo/user_demo_data_source.dart (2)
14-15: Consider parameterizing phone numbersHardcoded phone numbers should be moved to a configuration file for easier maintenance and testing.
42-51: Implement auth flow demoThe TODO comments indicate missing authentication flow implementation. This could affect testing of auth-dependent features.
Would you like help implementing a basic demo auth flow?
Also applies to: 58-61
recipients_app/lib/data/datasource/demo/payment_demo_data_source.dart (2)
18-19: Consider deterministic demo dataRandom payment counts could make testing unpredictable. Consider using fixed counts or making them configurable.
31-33: Parameterize payment constantsCurrency and amount values should be configurable rather than hardcoded.
Consider moving these to a configuration object:
+class PaymentDemoConfig { + final String currency; + final double amount; + + const PaymentDemoConfig({ + this.currency = "SLE", + this.amount = 700, + }); +}Also applies to: 48-50
recipients_app/lib/data/datasource/demo/demo_user.dart (1)
3-149: Consider implementing essential methodsMost methods throw UnimplementedError which could cause crashes. Consider implementing essential methods with demo values.
Examples:
- bool get isAnonymous => throw UnimplementedError(); + bool get isAnonymous => false; - String? get phoneNumber => throw UnimplementedError(); + String? get phoneNumber => "+1234567890"; - List<UserInfo> get providerData => throw UnimplementedError(); + List<UserInfo> get providerData => [];recipients_app/lib/my_app.dart (1)
Line range hint
45-69: Consider consolidating repository creationMultiple repositories are created with similar dependencies.
Consider using a factory or builder pattern to reduce repetition:
class RepositoryFactory { static T create<T>(BuildContext context, { required FirebaseFirestore firestore, required DemoManager demoManager, }) { switch (T) { case UserRepository: return UserRepository( firestore: firestore, demoManager: demoManager, ) as T; case PaymentRepository: return PaymentRepository( firestore: firestore, demoManager: demoManager, ) as T; // ... other repositories } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
recipients_app/lib/core/cubits/auth/auth_cubit.dart(2 hunks)recipients_app/lib/core/cubits/auth/auth_state.dart(1 hunks)recipients_app/lib/data/datasource/demo/demo_user.dart(1 hunks)recipients_app/lib/data/datasource/demo/no_op_document_reference.dart(1 hunks)recipients_app/lib/data/datasource/demo/organization_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/demo/payment_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/demo/survey_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/demo/user_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/organization_data_source.dart(1 hunks)recipients_app/lib/data/datasource/payment_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/organization_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/payment_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/survey_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/user_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/survey_data_source.dart(1 hunks)recipients_app/lib/data/datasource/user_data_source.dart(1 hunks)recipients_app/lib/data/models/recipient.dart(0 hunks)recipients_app/lib/data/repositories/organization_repository.dart(1 hunks)recipients_app/lib/data/repositories/payment_repository.dart(1 hunks)recipients_app/lib/data/repositories/survey_repository.dart(1 hunks)recipients_app/lib/data/repositories/user_repository.dart(1 hunks)recipients_app/lib/demo_manager.dart(1 hunks)recipients_app/lib/l10n/app_en.arb(1 hunks)recipients_app/lib/l10n/app_kri.arb(1 hunks)recipients_app/lib/main.dart(3 hunks)recipients_app/lib/my_app.dart(4 hunks)recipients_app/lib/view/widgets/welcome/phone_input_page.dart(4 hunks)
💤 Files with no reviewable changes (1)
- recipients_app/lib/data/models/recipient.dart
🔇 Additional comments (8)
recipients_app/lib/l10n/app_kri.arb (1)
179-180: Verify translation accuracy for "Demo"
The English word "Demo" is used in both English and Krio versions. Consider if this needs localization to Krio or if it's an intentionally preserved English term.
✅ Verification successful
"Demo" appears to be an acceptable English term in Krio context
Looking at the codebase:
- "Demo" is used consistently in both English and Krio versions
- Several other English terms are preserved in the Krio translations where they represent technical or widely adopted concepts (e.g., "Surveys", "Overview", "In Progress")
- This follows a common localization pattern where certain technical or universally recognized terms are intentionally kept in English
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if "Demo" is consistently used across other Krio translations
rg -i "demo" recipients_app/lib/l10n/app_kri.arb
Length of output: 70
Script:
#!/bin/bash
# Check how "Demo" is translated in other language files
fd -e arb -E 'app_kri.arb' -E 'app_en.arb' . recipients_app/lib/l10n/ --exec rg -i "demo" {}
Length of output: 90
Script:
#!/bin/bash
# Check English source file for context
rg -i "demo" recipients_app/lib/l10n/app_en.arb
Length of output: 69
Script:
#!/bin/bash
# Check for other English words in Krio translations to understand pattern
rg -A 1 -B 1 '"[^"]+": "[A-Za-z]+"' recipients_app/lib/l10n/app_kri.arb
Length of output: 2035
recipients_app/lib/l10n/app_en.arb (1)
279-280: LGTM: Consistent formatting and clear messaging
The new entries maintain consistent JSON formatting and use clear, concise text.
recipients_app/lib/data/datasource/survey_data_source.dart (1)
1-7: Interface definition is correct
The abstract class SurveyDataSource properly defines the required method fetchSurveys.
recipients_app/lib/data/repositories/survey_repository.dart (1)
21-26: LGTM: Clean implementation of data source switching
The implementation of _activeDataSource getter and its usage in fetchSurveys follows good practices for abstracting data source selection.
recipients_app/lib/core/cubits/auth/auth_state.dart (1)
29-43: LGTM: Complete state implementation
The state implementation properly includes the new organization field in both props and copyWith method, maintaining immutability and equality comparison.
recipients_app/lib/data/repositories/organization_repository.dart (1)
20-25: LGTM: Clean data source implementation
The implementation of data source switching and organization fetching is clean and follows the established pattern.
recipients_app/lib/main.dart (1)
2-2: LGTM: Clean integration of DemoManager
The DemoManager is properly initialized and integrated into the application setup flow.
Also applies to: 35-35, 57-57
recipients_app/lib/view/widgets/welcome/phone_input_page.dart (1)
115-116: LGTM: Good use of theme system
Using Theme.of(context) ensures consistent styling across the app.
recipients_app/lib/data/datasource/remote/payment_remote_data_source.dart
Show resolved
Hide resolved
recipients_app/lib/data/datasource/remote/user_remote_data_source.dart
Outdated
Show resolved
Hide resolved
recipients_app/lib/data/datasource/demo/no_op_document_reference.dart
Outdated
Show resolved
Hide resolved
MDikkii
force-pushed
the
mdikkii/demo-poc
branch
from
88e4263 to
99ebeeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
recipients_app/lib/data/repositories/organization_repository.dart (1)
9-10: 🛠️ Refactor suggestionConsider eager initialization in constructor
Late initialization of data sources could lead to initialization errors.
- late OrganizationDataSource remoteDataSource = OrganizationRemoteDataSource(firestore: firestore); - late OrganizationDataSource demoDataSource = OrganizationDemoDataSource(); + final OrganizationDataSource remoteDataSource; + final OrganizationDataSource demoDataSource; + + OrganizationRepository({ + required this.firestore, + required this.demoManager, + }) : + remoteDataSource = OrganizationRemoteDataSource(firestore: firestore), + demoDataSource = OrganizationDemoDataSource();recipients_app/lib/core/cubits/auth/auth_cubit.dart (1)
26-30: 🛠️ Refactor suggestionExtract duplicated organization fetch logic
Organization fetching logic is duplicated between
authStateChangeslistener andinitmethod.Extract into a private method as suggested in previous review.
Also applies to: 67-68
recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (3)
29-36:⚠️ Potential issueAdd phone number validation.
The phone number parsing could throw exceptions with invalid input.
Apply this diff:
final phoneNumber = firebaseUser.phoneNumber ?? ""; +if (phoneNumber.length <= 1) { + return null; +}
43-43:⚠️ Potential issueFix List access.
The firstOrNull method usage was previously flagged as incorrect.
Apply this diff:
-final userSnapshot = matchingUsers.docs.firstOrNull; +final userSnapshot = matchingUsers.docs.isNotEmpty ? matchingUsers.docs.first : null;
74-76:⚠️ Potential issueUse correct parameter name in timeout callback.
The parameter should be named 'verificationId' for clarity.
Apply this diff:
-codeAutoRetrievalTimeout: (e) { +codeAutoRetrievalTimeout: (verificationId) { log("auto-retrieval timeout"); },
🧹 Nitpick comments (6)
recipients_app/lib/data/repositories/payment_repository.dart (3)
9-10: Consider initializing data sources in constructorLate initialization could lead to runtime errors if accessed before initialization. Consider moving these initializations to the constructor.
class PaymentRepository { - late PaymentDataSource remoteDataSource = PaymentRemoteDataSource(firestore: firestore); - late PaymentDataSource demoDataSource = PaymentDemoDataSource(); + final PaymentDataSource remoteDataSource; + final PaymentDataSource demoDataSource;
20-20: Consider adding error handling for data source selectionWhile the selection logic is clean, consider adding error handling for cases where data sources might not be properly initialized.
- PaymentDataSource get _activeDataSource => demoManager.isDemoEnabled ? demoDataSource : remoteDataSource; + PaymentDataSource get _activeDataSource { + try { + return demoManager.isDemoEnabled ? demoDataSource : remoteDataSource; + } catch (e) { + throw StateError('Data source not properly initialized: $e'); + } + }
22-48: Add error handling and logging to repository methodsWhile the delegation to data sources is clean, consider adding error handling and logging for better debugging and error recovery.
Example for
fetchPayments:Future<List<SocialIncomePayment>> fetchPayments({ required String recipientId, }) async { + try { return _activeDataSource.fetchPayments(recipientId: recipientId); + } catch (e) { + log.error('Failed to fetch payments for recipient $recipientId', e); + throw PaymentRepositoryException('Failed to fetch payments', e); + } }recipients_app/lib/data/repositories/user_repository.dart (1)
12-13: Consider moving data source initialization to constructorLate initialization of data sources could lead to runtime errors if accessed before initialization. Consider moving the initialization to the constructor for better safety.
class UserRepository { - late UserDataSource remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth); - late UserDataSource demoDataSource = UserDemoDataSource(); + final UserDataSource remoteDataSource; + final UserDataSource demoDataSource; UserRepository({ required this.firestore, required this.firebaseAuth, - required this.demoManager, + required this.demoManager, + }) : remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth), + demoDataSource = UserDemoDataSource();recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (2)
45-49: Remove or properly document commented code.Either remove the commented code or convert it to a proper documentation comment explaining why this approach was considered and rejected.
70-70: Consider making timeout duration configurable.The 60-second timeout is hardcoded. Consider making it a configurable parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
recipients_app/lib/core/cubits/auth/auth_cubit.dart(2 hunks)recipients_app/lib/core/cubits/auth/auth_state.dart(1 hunks)recipients_app/lib/data/datasource/demo/demo_user.dart(1 hunks)recipients_app/lib/data/datasource/demo/no_op_document_reference.dart(1 hunks)recipients_app/lib/data/datasource/demo/organization_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/demo/payment_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/demo/survey_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/demo/user_demo_data_source.dart(1 hunks)recipients_app/lib/data/datasource/organization_data_source.dart(1 hunks)recipients_app/lib/data/datasource/payment_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/organization_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/payment_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/survey_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/remote/user_remote_data_source.dart(1 hunks)recipients_app/lib/data/datasource/survey_data_source.dart(1 hunks)recipients_app/lib/data/datasource/user_data_source.dart(1 hunks)recipients_app/lib/data/models/recipient.dart(0 hunks)recipients_app/lib/data/repositories/organization_repository.dart(1 hunks)recipients_app/lib/data/repositories/payment_repository.dart(1 hunks)recipients_app/lib/data/repositories/survey_repository.dart(1 hunks)recipients_app/lib/data/repositories/user_repository.dart(1 hunks)recipients_app/lib/demo_manager.dart(1 hunks)recipients_app/lib/l10n/app_en.arb(1 hunks)recipients_app/lib/l10n/app_kri.arb(1 hunks)recipients_app/lib/main.dart(3 hunks)recipients_app/lib/my_app.dart(4 hunks)recipients_app/lib/view/pages/terms_and_conditions_page.dart(2 hunks)recipients_app/lib/view/widgets/welcome/phone_input_page.dart(4 hunks)
💤 Files with no reviewable changes (1)
- recipients_app/lib/data/models/recipient.dart
🚧 Files skipped from review as they are similar to previous changes (21)
- recipients_app/lib/data/datasource/organization_data_source.dart
- recipients_app/lib/data/datasource/survey_data_source.dart
- recipients_app/lib/main.dart
- recipients_app/lib/l10n/app_kri.arb
- recipients_app/lib/l10n/app_en.arb
- recipients_app/lib/my_app.dart
- recipients_app/lib/data/datasource/demo/organization_demo_data_source.dart
- recipients_app/lib/data/datasource/demo/survey_demo_data_source.dart
- recipients_app/lib/core/cubits/auth/auth_state.dart
- recipients_app/lib/data/datasource/demo/no_op_document_reference.dart
- recipients_app/lib/view/widgets/welcome/phone_input_page.dart
- recipients_app/lib/data/datasource/remote/survey_remote_data_source.dart
- recipients_app/lib/demo_manager.dart
- recipients_app/lib/data/datasource/demo/demo_user.dart
- recipients_app/lib/data/datasource/payment_data_source.dart
- recipients_app/lib/data/datasource/remote/organization_remote_data_source.dart
- recipients_app/lib/data/repositories/survey_repository.dart
- recipients_app/lib/data/datasource/remote/payment_remote_data_source.dart
- recipients_app/lib/data/datasource/user_data_source.dart
- recipients_app/lib/data/datasource/demo/payment_demo_data_source.dart
- recipients_app/lib/data/datasource/demo/user_demo_data_source.dart
🔇 Additional comments (12)
recipients_app/lib/data/repositories/organization_repository.dart (2)
20-20: Clean data source selection logicThe active data source selection is clear and maintainable.
24-25: Good delegation to data sourceClean delegation to the active data source implementation.
recipients_app/lib/core/cubits/auth/auth_cubit.dart (1)
36-38: Proper state emission with organizationState emission correctly includes the organization data.
recipients_app/lib/data/repositories/payment_repository.dart (2)
1-5: LGTM! Clean import organizationThe imports are well-structured and properly segregated between data sources, models, and external dependencies.
12-18: LGTM! Well-structured constructorProperties are clearly defined with appropriate required parameters.
recipients_app/lib/view/pages/terms_and_conditions_page.dart (2)
3-3: Import looks good
No issues found with the newly added import statement for DemoManager.
96-96: Conditional label assignment is correct
The ternary condition cleanly switches between the demo and normal account creation texts. This is straightforward and should serve the intended use case effectively.recipients_app/lib/data/repositories/user_repository.dart (2)
19-25: Well-structured data source switching implementationThe getter provides clean encapsulation of the data source switching logic.
50-78: Clean implementation of repository methodsThe methods properly delegate to the active data source and maintain clean separation of concerns. The
signOutmethod correctly handles demo mode cleanup.recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (3)
1-16: Clean implementation of class structure and dependencies.The class follows good practices with proper dependency injection and clear interface implementation.
18-22: Well-implemented Firebase Auth integration.Clean implementation of auth state management methods.
80-84: Clean implementation of auth operations.Direct and appropriate use of Firebase Auth methods.
|
Visit the preview URL for this PR (updated for commit 7a6840d): https://si-admin-staging--pr960-mdikkii-demo-poc-224l3evc.web.app (expires Sun, 19 Jan 2025 10:35:35 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
| @@ -23,12 +23,14 @@ class AuthCubit extends Cubit<AuthState> { | |||
| if (user != null) { | |||
| try { | |||
| final recipient = await userRepository.fetchRecipient(user); | |||
| final Organization? organization = await _fetchOrganization(recipient); | |||
There was a problem hiding this comment.
You can always just write
final organization = await ....;
instead of
final Organization? organization = await ....;
Type will be assigned automatically. Just FYI :D
| import "package:cloud_firestore/cloud_firestore.dart"; | ||
|
|
||
| class OrganizationRepository { | ||
| late OrganizationDataSource remoteDataSource = OrganizationRemoteDataSource(firestore: firestore); |
There was a problem hiding this comment.
Try to avoid late when possible. Instead its better to pass the remoteDataSource and localDataSource via dependency injection:
class OrganizationRepository {
final OrganizationDataSource remoteDataSource;
final OrganizationDataSource demoDataSource;
...
const OrganizationRepository({
required this.remoteDataSource,
required this.demoDataSource,
...
});
}
There was a problem hiding this comment.
@novas1r1
But would not save the current implementation memory e.g. in the case if a user only uses the demo mode? Then the instance for "remoteDataSource" is never created and does not use memory.
So, could you explain a bit more why it is better to avoid late? Would be interesting :)
| class PaymentRepository { | ||
| late PaymentDataSource remoteDataSource = PaymentRemoteDataSource(firestore: firestore); |
There was a problem hiding this comment.
Same as in organization repository
| class UserRepository { | ||
| late UserDataSource remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth); |
There was a problem hiding this comment.
Same as in organization repository
| @@ -18,13 +19,15 @@ class _PhoneInputPageState extends State<PhoneInputPage> { | |||
| late final RoundedLoadingButtonController btnController; | |||
There was a problem hiding this comment.
In the widgets (I also just learned) its better to assign them directly:
final btnController = RoundedLoadingButtonController();
final phoneNumberController = TextEditingController();
...
There was a problem hiding this comment.
@novas1r1 Do you have a link where your learning is described? :)
| @@ -45,6 +48,18 @@ class _PhoneInputPageState extends State<PhoneInputPage> { | |||
| child: Column( | |||
| mainAxisAlignment: MainAxisAlignment.center, | |||
| children: <Widget>[ | |||
| Align( | |||
There was a problem hiding this comment.
Instead of
final localizations = AppLocalizations.of(context)!;
localizations.demoCta
You can also do:
context.l10n.demoCta
There was a problem hiding this comment.
@novas1r1
If I try to use "context.l10n", then "I10n" is undefined. 🤷🏻♀️
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (1)
43-43:⚠️ Potential issueUse correct method to retrieve first document.
The method
firstOrNullis not defined forList<DocumentSnapshot>.-final userSnapshot = matchingUsers.docs.firstOrNull; +final userSnapshot = matchingUsers.docs.isNotEmpty ? matchingUsers.docs.first : null;
🧹 Nitpick comments (2)
recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (2)
45-48: Document the user ID mismatch architectural decision.The commented code indicates an important architectural decision about user ID mapping between Firebase Auth and Firestore.
Consider adding this information to the architecture documentation and remove the commented code.
74-76: Enhance logging for timeout scenario.Add more context to the timeout log to aid debugging.
-log("auto-retrieval timeout"); +log("Phone verification auto-retrieval timeout for verification ID: $verificationId");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
recipients_app/lib/core/cubits/dashboard_card_manager/dashboard_card_manager_cubit.dart(0 hunks)recipients_app/lib/data/datasource/remote/user_remote_data_source.dart(1 hunks)
💤 Files with no reviewable changes (1)
- recipients_app/lib/core/cubits/dashboard_card_manager/dashboard_card_manager_cubit.dart
🧰 Additional context used
📓 Learnings (1)
recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (1)
Learnt from: MDikkii
PR: socialincome-san/public#960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (4)
recipients_app/lib/data/datasource/remote/user_remote_data_source.dart (4)
9-23: LGTM! Clean class structure with proper dependency injection.The class follows good practices by implementing an interface and using constructor injection for dependencies.
80-84: LGTM! Clean authentication operations.The methods properly delegate to Firebase Auth without unnecessary complexity.
87-91: LGTM! Proper audit field update.The implementation correctly sets the audit field and, as confirmed by previous feedback,
userIdis non-nullable so no additional checks are needed.
29-36:⚠️ Potential issueAdd phone number validation before parsing.
The current implementation could throw exceptions if the phone number is invalid.
final phoneNumber = firebaseUser.phoneNumber ?? ""; +if (phoneNumber.length <= 1) { + return null; +}Likely invalid or redundant comment.
… new states after calling close -- 15729368" This fixes the error "onError -- DashboardCardManagerCubit, Bad state: Cannot emit new states after calling close -- 201270230" when you start the app and do the following: - Click on the "Demo" button in the right upper corner => You are now on the Account page - On Account page click on "Enter demo" button => You are now on the Dashboard - Click on "Edit" in the right upper corner => You are now on the profile page - Scroll down to the "Sign out" button and click it => You are now on the Login page - Now click again on the "Demo" button in the right upper corner => In the debug console you get "onError --DashboardCardManagerCubit, Bad state: Cannot emit new states after calling close -- 15729368"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
recipients_app/lib/data/repositories/user_repository.dart (2)
12-13: Consider initializing data sources in constructorLate initialization of data sources could be moved to the constructor for better initialization control.
- late UserDataSource remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth); - late UserDataSource demoDataSource = UserDemoDataSource(); UserRepository({ required this.firestore, required this.firebaseAuth, required this.demoManager, - }); + }) : remoteDataSource = UserRemoteDataSource(firestore: firestore, firebaseAuth: firebaseAuth), + demoDataSource = UserDemoDataSource();
70-74: Consider handling sign out errorsThe sign out implementation should handle potential errors to ensure demo mode is reset appropriately.
Future<void> signOut() { - return _activeDataSource.signOut().whenComplete(() { + return _activeDataSource.signOut().then((_) { demoManager.isDemoEnabled = false; + }).catchError((error) { + demoManager.isDemoEnabled = false; + throw error; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
recipients_app/lib/core/cubits/dashboard_card_manager/dashboard_card_manager_cubit.dart(2 hunks)recipients_app/lib/data/repositories/user_repository.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipients_app/lib/core/cubits/dashboard_card_manager/dashboard_card_manager_cubit.dart
🧰 Additional context used
📓 Learnings (1)
recipients_app/lib/data/repositories/user_repository.dart (1)
Learnt from: MDikkii
PR: socialincome-san/public#960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.
🔇 Additional comments (2)
recipients_app/lib/data/repositories/user_repository.dart (2)
25-25: Clean implementation of data source selectionThe ternary operator provides a clear and concise way to select the active data source.
27-48: Auth state management implementation looks goodThe implementation correctly:
- Manages auth state subscriptions
- Handles demo mode changes
- Cleans up subscriptions appropriately
Keeping the StreamController open is intentional as confirmed in previous discussions.
…ize" to query the size attribute because this results in better performance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
recipients_app/lib/view/widgets/dialogs/social_income_contact_dialog.dart (2)
Line range hint
46-65: Consider consistent error handling across all contact methods.Only WhatsApp has error handling. Phone and email launches should handle failures similarly.
Line range hint
44-58: Consider moving contact information to data sources.Since this PR introduces demo/remote data sources, the hardcoded contact details could be managed through these sources for better flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
recipients_app/lib/view/widgets/dialogs/social_income_contact_dialog.dart(1 hunks)recipients_app/lib/view/widgets/welcome/phone_input_page.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipients_app/lib/view/widgets/welcome/phone_input_page.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Security checks (typescript)
- GitHub Check: Prettify
🔇 Additional comments (1)
recipients_app/lib/view/widgets/dialogs/social_income_contact_dialog.dart (1)
34-34: Good update to modern MediaQuery API.The change from
MediaQuery.of()toMediaQuery.sizeOf()follows Flutter's best practices.
Added
dataSourceconcept and provided bothdemoandremotedata sources where needed.Added
demoManagerto make it possible to flip the demo flag in the repositories and use proper data source in both demo and real app scenarios.Summary by CodeRabbit
New Features
DemoManager.Enhancements
Bug Fixes
Documentation