Feature: Fix NFC sometimes giving scanned status while not having anything scanned (KIDS-1592) #1210
Conversation
WalkthroughThe pull request introduces asynchronous modifications to various widget callback functions across multiple files. Notably, the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
lib/features/family/features/scan_nfc/cubit/scan_nfc_cubit.dart
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
lib/features/family/shared/widgets/buttons/givt_back_button_flat.dart (1)
33-34: Consider adding error handling for async operations
While the async flow is correct, it might be worth adding try-catch blocks around the onPressedExt?.call() to handle potential errors gracefully, especially since this is used in the NFC scanning flow where hardware operations could fail.
Here's a suggested improvement:
- await onPressedExt?.call();
+ try {
+ await onPressedExt?.call();
+ } catch (e) {
+ if (context.mounted) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text('An error occurred while going back')),
+ );
+ }
+ }lib/features/family/features/recommendation/widgets/charity_finder_app_bar.dart (1)
24-24: Consider using BlocProvider.of for better error handling.
The async implementation looks good. However, consider using BlocProvider.of<FlowsCubit>(context) instead of context.read<FlowsCubit>() as it provides better error messages when the bloc is not found.
- onPressedExt ?? () async => context.read<FlowsCubit>().resetFlow(),
+ onPressedExt ?? () async => BlocProvider.of<FlowsCubit>(context).resetFlow(),lib/features/family/features/scan_nfc/cubit/scan_nfc_cubit.dart (3)
36-36: Consider adding error handling for stopSession call.
While adding the error message is good, the stopSession call could throw an exception that should be handled.
- NfcManager.instance.stopSession(errorMessage: "stopScanningSession");
+ try {
+ await NfcManager.instance.stopSession(errorMessage: "stopScanningSession");
+ } catch (e) {
+ log('Failed to stop NFC session: $e');
+ }135-135: Consider using a more descriptive alert message.
The current alert message is just a space character. Consider using a more meaningful message like "Scan completed" to improve user feedback.
- await NfcManager.instance.stopSession(alertMessage: ' ', errorMessage: 'onDiscovered');
+ await NfcManager.instance.stopSession(alertMessage: 'Scan completed', errorMessage: 'onDiscovered');178-178: Good error handling improvement!
The addition of error message and stack trace logging will help with debugging incorrect scan status issues.
Consider adding the error message to analytics event for better tracking:
- AnalyticsHelper.logEvent(eventName: AmplitudeEvents.coinScannedError);
+ AnalyticsHelper.logEvent(
+ eventName: AmplitudeEvents.coinScannedError,
+ eventProperties: {'error': e.toString()},
+ );lib/features/registration/pages/us_signup_page.dart (2)
Line range hint 256-321: Security: Consider strengthening password validation.
The current password validation requires:
- Minimum length of 7 characters
- At least one number
- At least one uppercase letter
Consider enhancing security by:
- Increasing minimum length to 8 characters (industry standard)
- Requiring special characters
- Preventing common passwords
Line range hint 187-198: Security: Add rate limiting for registration attempts.
The _register function handles sensitive operations. Consider implementing rate limiting to prevent brute force attacks and protect user data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- lib/features/family/features/qr_scanner/widgets/camera_screen_frame.dart (1 hunks)
- lib/features/family/features/recommendation/widgets/charity_finder_app_bar.dart (1 hunks)
- lib/features/family/features/reflect/presentation/pages/family_selection_screen.dart (1 hunks)
- lib/features/family/features/scan_nfc/cubit/scan_nfc_cubit.dart (4 hunks)
- lib/features/family/features/scan_nfc/nfc_scan_screen.dart (1 hunks)
- lib/features/family/shared/widgets/buttons/givt_back_button_flat.dart (3 hunks)
- lib/features/registration/pages/us_signup_page.dart (1 hunks)
🔇 Additional comments (12)
lib/features/family/shared/widgets/buttons/givt_back_button_flat.dart (3)
1-2: LGTM: Required import added for async support
The addition of dart:async import is necessary for using unawaited.
18-18: LGTM: Type signature updated for async support
The change from void Function()? to Future<void> Function()? properly reflects that the callback can now perform async operations.
28-37: Verify the order of operations in back button handler
The implementation looks correct with proper async handling, but let's verify a few aspects:
- Analytics and sound effects are properly fire-and-forget using
unawaited - Custom back action (
onPressedExt) is awaited before navigation - Context mounted check prevents navigation after async gap
Let's verify the usage patterns across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
Back button implementation follows safe async patterns
The verification confirms:
- All usages of
GivtBackButtonFlatfollow safe async patterns - The original implementation with
unawaitedfor analytics/sound and context mounted check is consistent with best practices - Most usages either have no custom action or properly handle async operations before navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent async patterns in back button usage
# Verify that:
# 1. All usages properly await async operations
# 2. No direct context.pop() calls after async gaps without mounted check
# Search for GivtBackButtonFlat usage patterns
rg -A 5 "GivtBackButtonFlat\("
# Search for potential unsafe pop() calls after async operations
ast-grep --pattern 'async {
$$$
context.pop();
}'
Length of output: 14928
lib/features/family/features/recommendation/widgets/charity_finder_app_bar.dart (1)
17-17: Verify all callers of CharityFinderAppBar handle the async callback.
The change to make onPressedExt return Future<void> is appropriate for async operations. However, we should verify that all existing callers have been updated accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response:
The async callback change is safe to proceed with
The search results show that all current usages of CharityFinderAppBar don't pass any callback to onPressedExt (they all use the default constructor with either no parameters or just showWallet). Since the parameter is optional and no existing code is actually passing a callback, the change from sync to async is non-breaking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all instantiations of CharityFinderAppBar to verify callback implementations
ast-grep --pattern 'CharityFinderAppBar(
$$$
onPressedExt: $_,
$$$
)'
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search pattern to find usages of CharityFinderAppBar
rg "CharityFinderAppBar" -A 5 -B 5
Length of output: 7197
lib/features/family/features/qr_scanner/widgets/camera_screen_frame.dart (1)
32-34: Verify FlowsCubit's reset implementation.
The async callback change looks good and aligns with the app-wide standardization of async button handlers. However, we should verify that FlowsCubit.resetFlow() properly handles async context.
lib/features/family/features/scan_nfc/cubit/scan_nfc_cubit.dart (1)
Line range hint 100-104: Good improvement in error handling logic!
The systemIsBusy check prevents unnecessary session cancellation, which helps avoid the incorrect scan status issue mentioned in the PR objectives.
lib/features/family/features/reflect/presentation/pages/family_selection_screen.dart (2)
48-50: LGTM - Part of broader async refactoring.
The addition of the async keyword aligns with similar changes across other files, maintaining consistency in the codebase.
48-50: Consider if async is necessary here.
The callback only contains a synchronous Navigator.pop() call. Consider if the async keyword is really needed in this case, or if it's just added for consistency with other similar changes.
✅ Verification successful
Based on the analysis of the GivtBackButtonFlat widget implementation and its usages across the codebase, I can make a definitive conclusion:
The async keyword is necessary here.
The onPressedExt callback is defined as Future<void> Function()? in the widget, and several usages show that it's used to perform asynchronous operations before navigation, such as:
- Logging out users
- Resetting flows
- Canceling scanning operations
- Performing cleanup tasks
Additionally, the widget's own implementation uses await on both:
- The
onPressedExt?.call() - Checking
context.mountedbefore navigation
Therefore, the async keyword is required to properly handle these asynchronous operations and ensure they complete before navigation occurs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other GivtBackButtonFlat usages perform async operations
ast-grep --pattern 'GivtBackButtonFlat(
$$$
onPressedExt: $_($$$) async {
$$$
}
$$$
)'
Length of output: 100
Script:
#!/bin/bash
# Search for GivtBackButtonFlat usages with onPressedExt
rg "GivtBackButtonFlat.*onPressedExt" -A 5
Length of output: 44
Script:
#!/bin/bash
# Search for any usage of Navigator.pop with async
ast-grep --pattern '($$$) async {
$$$
Navigator.of($_).pop();
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Search for the GivtBackButtonFlat class definition to understand onPressedExt type
rg "class GivtBackButtonFlat"
Length of output: 152
Script:
#!/bin/bash
# Let's check the GivtBackButtonFlat widget implementation
cat lib/features/family/shared/widgets/buttons/givt_back_button_flat.dart
Length of output: 1174
Script:
#!/bin/bash
# Let's try a different pattern to find GivtBackButtonFlat usages
rg -l "GivtBackButtonFlat" | xargs cat
Length of output: 127584
lib/features/family/features/scan_nfc/nfc_scan_screen.dart (1)
127-129: 🛠️ Refactor suggestion
Consider improving the back button's error handling and operation order.
While making the callback async and awaiting cancelScanning() is good, there are a few improvements to consider:
- The flow reset should happen after successful scan cancellation to maintain UI consistency
- Error handling should be added for potential failures
Consider this implementation:
onPressedExt: () async {
- context.read<FlowsCubit>().resetFlow();
- await context.read<ScanNfcCubit>().cancelScanning();
+ try {
+ await context.read<ScanNfcCubit>().cancelScanning();
+ context.read<FlowsCubit>().resetFlow();
+ } catch (e) {
+ // Handle cancellation failure
+ debugPrint('Failed to cancel NFC scanning: $e');
+ }
},Let's verify the UI state handling in the ScanNfcCubit:
lib/features/registration/pages/us_signup_page.dart (3)
99-102: LGTM: Async callback change is correct.
The conversion of onPressedExt to an async callback is consistent with the broader changes being made across the codebase to support asynchronous operations.
Line range hint 322-334: Security: Sanitize phone number input.
While there's validation for US phone numbers, ensure proper sanitization of the input to prevent potential injection attacks, especially since this data is likely stored in a database.
Let's check for input sanitization:
#!/bin/bash
# Search for phone number sanitization
rg -B 2 -A 2 "formatPhoneNrUs|phoneNumber" --type dart99-102: Verify error handling during logout.
While the async conversion is correct, we should ensure proper error handling during logout to prevent the UI from becoming unresponsive.
Let's check the implementation of the logout helper:
Also applies to: 256-321
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Error handling during logout is properly implemented
The logout implementation in AuthCubit has appropriate error handling:
- Sets loading state before logout
- Logs the action via LoggingInfo
- Handles the logout operation through repository
- Updates state to unauthenticated with preserved email
- Clears analytics user properties
- Any repository errors are caught and logged
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the logout helper implementation
rg -A 5 "class LogoutHelper|void logout" --type dart
Length of output: 22484
Script:
#!/bin/bash
# Search for the logout implementation
rg -A 10 "void logout.*context" --type dart
Length of output: 45
Script:
#!/bin/bash
# Search for any auth-related logout methods
rg -A 10 "logout.*\{" --type dart
Length of output: 3754
Script:
#!/bin/bash
# Search for AuthCubit implementation which might handle logout
ast-grep --pattern 'class AuthCubit extends Cubit<$_> {
$$$
}'
Length of output: 37725
lib/features/family/features/qr_scanner/widgets/camera_screen_frame.dart
Show resolved
Hide resolved
lib/features/family/features/scan_nfc/cubit/scan_nfc_cubit.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
lib/features/family/features/scan_nfc/cubit/scan_nfc_cubit.dart (3)
Line range hint 100-104: Add missing await for cancelScanning.
The error handling logic is good, but cancelScanning() needs to be awaited since it's now an async method.
Apply this fix:
- cancelScanning();
+ await cancelScanning();135-136: Document the purpose of the space in alertMessage.
The space character in alertMessage: ' ' seems like a workaround. Please add a comment explaining why this is necessary for iOS.
Apply this change:
- await NfcManager.instance.stopSession(alertMessage: ' ',);
+ // iOS requires non-empty alertMessage to properly dismiss the NFC dialog
+ await NfcManager.instance.stopSession(alertMessage: ' ',);Line range hint 28-175: Consider architectural improvements for better maintainability.
To better handle the NFC scanning issues and improve maintainability:
- Extract platform-specific logic into separate classes/methods
- Create a dedicated service for simulator/device detection
- Consider using sealed classes or enums for more precise NFC scan states to prevent incorrect status
Example structure:
// Platform-specific implementations
abstract class NfcPlatformHandler {
Future<void> stopSession();
Future<void> startSession();
}
class IosNfcHandler extends NfcPlatformHandler { ... }
class AndroidNfcHandler extends NfcPlatformHandler { ... }
// Device detection
class DeviceEnvironment {
Future<bool> isSimulator();
Future<bool> isNfcAvailable();
}
// Enhanced states
enum NfcScanResult {
ready,
scanning,
scanned(String mediumId),
error(String message),
unavailable
}
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation