Contextual research surveys #2864
Conversation
…zone management - Implemented LocationPrivacyScreen to manage location tracking status and controls. - Added functionality to start, pause, and stop location tracking. - Created UI components for displaying tracking status, privacy zones, and location data management. - Integrated dialogs for adding privacy zones and deleting data ranges. - Enhanced user experience with feedback messages for actions performed.
…or Location Privacy settings
… toggle functionality
- Added a new utility class `DateFormatters` for consistent date and time formatting. - Updated `_LocationDetailDialogState` to use `DateFormatters` for formatting timestamps and time ago. - Cleaned up the `AddPrivacyZoneDialog` by removing redundant date formatting methods and replacing them with calls to `DateFormatters`. - Improved code readability by organizing imports and formatting code consistently.
… improved location handling
…ed on location, air quality, and time conditions feat: Create SurveyCard widget to display survey details with progress indication feat: Add SurveyProgressIndicator for tracking question progress during surveys feat: Develop SurveyQuestionWidget to handle various question types and user responses
- Add air quality alert notifications with category-specific icons and colors - Integrate survey triggers with location and air quality monitoring - Implement responsive UI design across all device sizes - Update survey pages with proper AirQo design system compliance - Remove test buttons from production app for clean deployment - Add automatic survey triggering after air quality threshold alerts - Fix button color consistency and navigation positioning issues Key features: - Air quality alerts appear before survey triggers (5 second display) - Category-specific SVG icons from existing assets - Hex color support matching main app AQI cards - Responsive design for mobile, tablet, and desktop - Cooldown periods to prevent notification spam - Integration with existing dashboard and location services
📝 WalkthroughWalkthroughInitializes EnhancedLocationServiceManager at startup, wires NavigationService.navigatorKey, adds encrypted SecureLocationStorage, implements a full survey system (models, repo, Bloc, trigger service, UI), integrates survey triggers with air-quality and location updates, and adds privacy/data-sharing and location-management UIs and dialogs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor System
participant App as main.dart
participant ELSM as EnhancedLocationServiceManager
participant SecureStore as SecureLocationStorage
participant Cache as CacheManager
participant NavS as NavigationService
participant Dash as DashboardPage
participant STS as SurveyTriggerService
participant Repo as SurveyRepository
System->>App: start()
App->>ELSM: await initialize()
ELSM->>SecureStore: open encrypted box / load persisted data
SecureStore-->>ELSM: privacy zones, history, settings
ELSM-->>App: initialized
App->>Cache: CacheManager.initialize()
App->>NavS: set navigatorKey
App->>Dash: launch UI
Dash->>STS: initialize() / setActiveSurveys
Dash->>Repo: getSurveys()
Repo-->>Dash: surveys
Dash->>ELSM: checkLocationPermission()
alt permission granted
Dash->>ELSM: startLocationTracking()
ELSM-->>STS: location updates (stream)
else denied
Dash-->>Dash: limited triggers (no precise location)
end
sequenceDiagram
autonumber
participant AQ as DashboardRepository
participant STS as SurveyTriggerService
participant NM as NotificationManager
participant NavS as NavigationService
participant UI as User
AQ-->>STS: updateAirQuality(payload)
STS->>STS: evaluate air-quality triggers
alt threshold met
STS->>NM: showAirQualityAlert(...)
STS-->>NavS: showSurveyNotification(survey) / emit surveyTriggeredStream
NM-->>UI: display overlay/dialog/banner
UI->>NavS: tap -> navigateToSurvey(survey)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mobile-v3/lib/src/app/dashboard/widgets/dashboard_app_bar.dart (2)
99-106: Guard against empty users list before indexingAccessing users[0] will throw when the list is empty.
- final user = userState.model.users[0]; + final users = userState.model.users; + if (users.isEmpty) { + return _buildGuestAvatar(context); + } + final user = users.first;
39-45: Fix theme toggle to respect current brightness- return GestureDetector( - onTap: () { - themeBloc.add(ToggleTheme(true)); - }, + return GestureDetector( + onTap: () { + themeBloc.add(ToggleTheme(!isDarkMode)); + },src/mobile-v3/lib/src/app/shared/services/notification_manager.dart (1)
44-45: Guard Overlay access and use root overlayPrevents NPEs in contexts without an Overlay and ensures overlays appear above app bars.
- Overlay.of(context).insert(_currentNotification!); + final overlay = Overlay.maybeOf(context, rootOverlay: true); + if (overlay == null) return; + overlay.insert(_currentNotification!); @@ - Overlay.of(context).insert(entry); + final overlay = Overlay.maybeOf(context, rootOverlay: true); + if (overlay == null) return; + overlay.insert(entry); @@ - Overlay.of(context).insert(entry); + final overlay = Overlay.maybeOf(context, rootOverlay: true); + if (overlay == null) return; + overlay.insert(entry);Also applies to: 342-343, 524-525
🧹 Nitpick comments (45)
src/mobile-v3/lib/src/meta/utils/date_formatters.dart (1)
8-21: Handle future timestamps in time-ago and keep wording correctSupport “In X …” for future times; keep “Just now” for sub-minute deltas.
- static String formatTimeAgo(DateTime dateTime) { - final now = DateTime.now(); - final difference = now.difference(dateTime); - - if (difference.inDays > 0) { - return '${difference.inDays} day${difference.inDays == 1 ? '' : 's'} ago'; - } else if (difference.inHours > 0) { - return '${difference.inHours} hour${difference.inHours == 1 ? '' : 's'} ago'; - } else if (difference.inMinutes > 0) { - return '${difference.inMinutes} minute${difference.inMinutes == 1 ? '' : 's'} ago'; - } else { - return 'Just now'; - } - } + static String formatTimeAgo(DateTime dateTime) { + final now = DateTime.now(); + final isFuture = dateTime.isAfter(now); + final diff = isFuture ? dateTime.difference(now) : now.difference(dateTime); + + if (diff.inDays > 0) { + final v = diff.inDays; + return isFuture ? 'In $v day${v == 1 ? '' : 's'}' : '$v day${v == 1 ? '' : 's'} ago'; + } else if (diff.inHours > 0) { + final v = diff.inHours; + return isFuture ? 'In $v hour${v == 1 ? '' : 's'}' : '$v hour${v == 1 ? '' : 's'} ago'; + } else if (diff.inMinutes > 0) { + final v = diff.inMinutes; + return isFuture ? 'In $v minute${v == 1 ? '' : 's'}' : '$v minute${v == 1 ? '' : 's'} ago'; + } else { + return 'Just now'; + } + }If product copy prefers “X mins ago” style or localization, consider using intl and passing a Locale.
src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
130-139: Clarify/align enter/exit semanticsisTriggered() ignores triggerOnEnter/triggerOnExit. Either rename to isInZone() (and handle enter/exit in the service with prior state), or extend the API to accept previousPosition and evaluate transitions.
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (2)
177-183: Simplify and make range deletion inclusive and clearerEquivalent, more readable predicate.
- _locationHistory.removeWhere((point) => - point.timestamp.isAfter(start.subtract(Duration(milliseconds: 1))) && - point.timestamp.isBefore(end.add(Duration(milliseconds: 1)))); + _locationHistory.removeWhere( + (p) => !p.timestamp.isBefore(start) && !p.timestamp.isAfter(end), + );
291-311: Nit: consider capping retained historyTo avoid unbounded growth in SharedPreferences, consider pruning by age/count (e.g., keep last N or last 90 days) during _saveLocationHistory().
src/mobile-v3/lib/src/app/profile/pages/location_data_view_screen.dart (1)
31-59: Avoid nested setState between _applyFilters and _applySortingMerged in the diff above. Prevents redundant rebuilds.
Also applies to: 51-59
src/mobile-v3/lib/src/app/dashboard/repository/dashboard_repository.dart (2)
122-124: Also notify survey triggers during background refresh.Air-quality updates from
_refreshInBackground()won’t propagate. Add the same notification there for consistency._airQualityController.add(dashboardResponse); - + loggy.info('Background refresh completed successfully'); + // Keep survey triggers in sync during background refresh + _notifySurveyTriggerService(dashboardResponse);
210-234: Validate updateAirQuality payload before invocation
- Guard on measurement.pm25?.value and measurement.aqiCategory being non-null; early-return (with a debug log) if either is missing to prevent silent failures in triggers.
- Verify siteDetails?.name or formattedName won’t both be null (or provide a default).
- Normalize timestamp to a consistent serialized format if downstream logic expects a string.
src/mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart (3)
4-6: Typo in filename “mananger”.Minor, but consider renaming file to
location_service_manager.dartin a follow-up to prevent future confusion.
67-69: Make the un-awaited init explicit (or await).If lints flag discarded futures, either mark intentional or await depending on UX requirements.
Option A (intentional fire-and-forget, add annotation comment):
- _initializeSurveySystem(); + // Intentionally not awaited: non-blocking startup. + _initializeSurveySystem();Option B (await inside a post-frame callback to avoid blocking initState):
- _initializeSurveySystem(); + WidgetsBinding.instance.addPostFrameCallback((_) async { + await _initializeSurveySystem(); + });
344-373: Don’t retain BuildContext in SurveyTriggerService
- _surveyTriggerService.setContext(context) stores a BuildContext that isn’t cleared in dispose(), risking leaks and lifecycle issues (src/.../survey_trigger_service.dart:49–51,54–57).
- Drop
_contextindispose()or remove the field entirely and instead pass aNavigatorState, callback, or emit events that the UI listens to.src/mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart (1)
162-174: Nit: construct destination as const.Minor perf/readability improvement.
- MaterialPageRoute( - builder: (context) => LocationPrivacyScreen()), + MaterialPageRoute( + builder: (context) => const LocationPrivacyScreen()),src/mobile-v3/lib/src/app/dashboard/services/location_service_mananger.dart (1)
44-46: Ensure single source of truth for SurveyTriggerService
InstantiatingSurveyTriggerService()directly here creates a separate instance. Inject or retrieve the shared singleton/DI‐managed instance so that allupdateLocationcalls feed into the same service pipeline.src/mobile-v3/lib/src/app/surveys/pages/survey_list_page.dart (2)
64-70: Small perf/readability nit: make the animation const.No behavior change.
return Center( child: CircularProgressIndicator( - valueColor: AlwaysStoppedAnimation<Color>(AppColors.primaryColor), + valueColor: const AlwaysStoppedAnimation<Color>(AppColors.primaryColor), ), );
77-134: Consider making the header scroll with the list.Wrapping header + list into a single CustomScrollView (Slivers) improves UX on smaller screens and enables pull-to-refresh from the very top.
src/mobile-v3/lib/src/app/surveys/example/air_quality_alert_test_page.dart (3)
156-166: PreferdebugPrint/logging overKeeps console noise manageable and consistent with app logging.
- onDismiss: () { - print('$category alert dismissed'); - }, + onDismiss: () => debugPrint('$category alert dismissed'),
204-213: Same logging nit for survey dialog callbacks.- onTakeSurvey: () { - print('User chose to take survey'); - }, - onDismiss: () { - print('User dismissed survey dialog'); - }, + onTakeSurvey: () => debugPrint('User chose to take survey'), + onDismiss: () => debugPrint('User dismissed survey dialog'),
242-248: Same logging nit for banner callbacks.- onTap: () { - print('User tapped survey banner'); - }, - onDismiss: () { - print('User dismissed survey banner'); - }, + onTap: () => debugPrint('User tapped survey banner'), + onDismiss: () => debugPrint('User dismissed survey banner'),src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart (2)
230-236: Improve copy for clarity and tone.- 'AirQo to use your precise location to locate the Air Quality of your nearest location', + 'Allow AirQo to use your precise location to show nearby air quality.',
317-327: Handle start/stop failures and keep switch in sync.If tracking start/stop throws, the switch will show the wrong state.
- onChanged: (value) async { - if (value) { - await _locationManager.startLocationTracking(); - } else { - await _locationManager.stopLocationTracking(); - } - setState(() { - _isTrackingActive = value; - }); - }, + onChanged: (value) async { + try { + if (value) { + await _locationManager.startLocationTracking(); + } else { + await _locationManager.stopLocationTracking(); + } + if (!mounted) return; + setState(() => _isTrackingActive = value); + } catch (e) { + _showSnackBar('Unable to ${value ? 'start' : 'stop'} tracking.'); + if (!mounted) return; + setState(() => _isTrackingActive = !value); + } + },src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (2)
246-260: Cache clearing may leave orphaned stats.Deleting stats keyed by response surveyIds won’t catch surveys with no responses. Consider a dedicated “clear box” in HiveRepository for
_surveyStatsBoxName.
63-85: Error mapping by substring is brittle.Prefer inspecting HTTP status codes or typed failures from
createAuthenticatedGetRequest.src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart (3)
49-57: Holding a BuildContext long-term is fragile.Prefer a global
navigatorKey(or a lightweight notification overlay) to avoid stale context issues.
371-384: Navigation wiring looks correct; DI for repository would improve testability.
386-422: Good UX touch to show alert before survey; consider canceling the delayed trigger if service is disposed.Minor edge-case polish.
src/mobile-v3/lib/src/app/surveys/pages/survey_detail_page.dart (5)
26-36: Repository lookup fallback can silently bypass app DICatching and instantiating a new SurveyRepository on failure may create an unmanaged instance and diverge from global DI state. Prefer an explicit optional RepositoryProvider (with assert/log) or make repository a required dependency for this page.
- } else { - try { - repo = RepositoryProvider.of<SurveyRepository>(context); - } catch (e) { - repo = SurveyRepository(); - } - } + } else { + repo = RepositoryProvider.of<SurveyRepository>(context, listen: false); + // Optionally: assert(repo != null, 'SurveyRepository not found in context'); + }
38-44: Bloc lifecycle OK; consider passing existingResponse to seed stateIf resume is intended when a response exists, seed the bloc on creation so the first frame reflects in-progress state without waiting for a tap.
- create: (context) => SurveyBloc(repo), + create: (context) => SurveyBloc(repo) + ..add( + existingResponse != null && (existingResponse!.isInProgress || existingResponse!.isCompleted) + ? ResumeSurvey(survey, existingResponse!) + : LoadSurvey(survey), + ),
121-129: Verify.withValues(alpha: …)availabilityFlutter
ColorexposeswithOpacity/withAlpha. If.withValuesisn’t a project-wide extension, these calls won’t compile. If it is an extension, ignore; otherwise swap towithOpacity(...).- color: Colors.black.withValues(alpha: 0.05), + color: Colors.black.withOpacity(0.05),Also applies to: 140-147, 249-252, 289-293, 321-323, 414-418
399-408: Minor polish on nav bar layout
- Redundant
flex: state.isFirstQuestion ? 1 : 1.- Consider disabling Submit/Next until current answer valid (already handled) and add tooltip for disabled state if needed.
- flex: state.isFirstQuestion ? 1 : 1, + // Equal flex; explicit not neededAlso applies to: 443-467
526-528: Use shared date formatter for consistency/i18nLeverage existing DateFormatters utility to match the rest of the app.
- String _formatDate(DateTime date) { - return '${date.day}/${date.month}/${date.year}'; - } + String _formatDate(DateTime date) => DateFormatters.formatDate(date);src/mobile-v3/lib/src/app/surveys/example/survey_test_page.dart (1)
308-321: Mounted checks around async UI opsSnackBars after awaits should verify
mountedto avoid setState/lookup after dispose.- ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar(Also applies to: 331-347, 387-405
src/mobile-v3/lib/src/app/shared/services/notification_manager.dart (2)
350-378: Assets and categories: verify coverageEnsure all SVG asset paths exist in pubspec and category strings map to your API (e.g., “U4SG”, “USG”, casing). Add tests/log for unknown categories.
- default: - return AppColors.primaryColor; + default: + // Consider logging unknown categories + return AppColors.primaryColor;Also applies to: 381-400
80-87:withValues(alpha: …)usageConfirm project-wide Color extension is imported. If not, replace with
withOpacity.- AppColors.primaryColor.withValues(alpha: 0.1) + AppColors.primaryColor.withOpacity(0.1)Also applies to: 239-247, 257-270, 324-331, 430-439, 435-439
src/mobile-v3/lib/src/app/profile/pages/data_sharing_screen.dart (4)
721-733: Immediate UI feedback on per-item toggleCurrently relies on parent to rebuild. Optionally update local list for snappier UX.
- value: point.isSharedWithResearchers, - onChanged: (value) { - widget.onUpdateSharing(point.id, value); - }, + value: point.isSharedWithResearchers, + onChanged: (value) { + // Optimistic UI + setState(() { + final i = _filteredHistory.indexWhere((p) => p.id == point.id); + if (i != -1) { + _filteredHistory[i] = _filteredHistory[i].copyWith( + isSharedWithResearchers: value, + ); + } + }); + widget.onUpdateSharing(point.id, value); + },Note: requires
copyWithonLocationDataPoint; otherwise re-create the instance.
126-134: Icon availability
Icons.checklistmay not exist on all Material icon sets. Use a widely-available alternative.- icon: const Icon(Icons.checklist), + icon: const Icon(Icons.done_all),
758-763: Font family portability
fontFamily: 'monospace'isn’t guaranteed in Flutter unless bundled. Either remove or use a bundled mono font.- fontFamily: 'monospace', + // Consider a bundled mono font (e.g., GoogleFonts.robotoMono())
842-867: Bulk updates: batching and UXLooping per item is fine for small lists; if this scales, consider a repository batch API to reduce I/O and update the list once at the end.
Also applies to: 886-905, 925-944
src/mobile-v3/lib/src/app/surveys/bloc/survey_bloc.dart (1)
91-127: Consider optimizing answer updates to avoid list recreationThe current implementation recreates the entire answers list on each update. For surveys with many questions, this could impact performance.
Consider using a more efficient approach for updating answers:
Future<void> _onAnswerQuestion(AnswerQuestion event, Emitter<SurveyState> emit) async { if (state is SurveyInProgress) { final currentState = state as SurveyInProgress; final updatedAnswers = Map<String, dynamic>.from(currentState.answers); updatedAnswers[event.questionId] = event.answer; // Create survey answer final surveyAnswer = SurveyAnswer( questionId: event.questionId, answer: event.answer, answeredAt: DateTime.now(), ); - // Update current response - final updatedResponseAnswers = List<SurveyAnswer>.from(currentState.currentResponse.answers); - final existingAnswerIndex = updatedResponseAnswers.indexWhere( - (a) => a.questionId == event.questionId, - ); - - if (existingAnswerIndex >= 0) { - updatedResponseAnswers[existingAnswerIndex] = surveyAnswer; - } else { - updatedResponseAnswers.add(surveyAnswer); - } + // More efficient: only modify what changed + final updatedResponseAnswers = currentState.currentResponse.answers + .where((a) => a.questionId != event.questionId) + .toList() + ..add(surveyAnswer); final updatedResponse = currentState.currentResponse.copyWith( answers: updatedResponseAnswers, ); emit(currentState.copyWith( answers: updatedAnswers, currentResponse: updatedResponse, )); loggy.info('Answered question: ${event.questionId}'); } }src/mobile-v3/lib/src/app/profile/pages/widgets/privacy_dialogs.dart (1)
467-483: Coordinate validation could be extracted for reusabilityThe coordinate validation logic is duplicated here and could be useful elsewhere in the app.
Consider extracting the validation logic to a utility function:
// In a utils file static bool isValidLatitude(double lat) => lat >= -90 && lat <= 90; static bool isValidLongitude(double lng) => lng >= -180 && lng <= 180;Then use it here:
- if (lat < -90 || lat > 90 || lng < -180 || lng > 180) { + if (!LocationUtils.isValidLatitude(lat) || !LocationUtils.isValidLongitude(lng)) { _showSnackBar( 'Please enter valid latitude (-90 to 90) and longitude (-180 to 180)'); return; }src/mobile-v3/lib/src/app/surveys/models/survey_response_model.dart (1)
121-127: getAnswerForQuestion could use more idiomatic DartUsing try-catch for flow control isn't ideal in Dart. The firstWhere method already has an orElse parameter.
SurveyAnswer? getAnswerForQuestion(String questionId) { - try { - return answers.firstWhere((answer) => answer.questionId == questionId); - } catch (e) { - return null; - } + return answers.cast<SurveyAnswer?>().firstWhere( + (answer) => answer?.questionId == questionId, + orElse: () => null, + ); }src/mobile-v3/lib/src/app/surveys/example/example_survey_data.dart (2)
10-60: Mark immutable literals as const to reduce rebuild/GC overhead.SurveyQuestion literals and options lists are compile-time constants—declare them const for cheaper builds.
Example for one block:
- SurveyQuestion( + const SurveyQuestion( id: 'activity_type', question: 'What activity were you doing when you received this survey?', type: QuestionType.multipleChoice, - options: [ + options: const [ 'Walking/Commuting outdoors', 'Exercising outdoors', 'Working outdoors', 'Relaxing outdoors', 'Indoor activities', 'Other', ], isRequired: true, ),Apply similarly to other SurveyQuestion literals in this file.
Also applies to: 80-108, 131-150, 151-164, 185-204, 205-213
239-257: Use a single timestamp for coherent sample context.currentLocation.timestamp and top-level timestamp are captured separately; a single captured now reduces test flakiness.
- static Map<String, dynamic> createSampleContextData() { - return { + static Map<String, dynamic> createSampleContextData() { + final now = DateTime.now().toIso8601String(); + return { 'currentLocation': { 'latitude': -0.3476, 'longitude': 32.5825, - 'timestamp': DateTime.now().toIso8601String(), + 'timestamp': now, 'accuracy': 10.0, 'altitude': 1200.0, 'altitudeAccuracy': 5.0, 'heading': 0.0, 'headingAccuracy': 0.0, 'speed': 0.0, 'speedAccuracy': 0.0, }, 'currentAirQuality': 82.5, 'currentAirQualityCategory': 'Unhealthy for Sensitive Groups', - 'timestamp': DateTime.now().toIso8601String(), + 'timestamp': now,Also applies to: 254-263
src/mobile-v3/lib/src/app/surveys/bloc/survey_event.dart (2)
42-50: Prefer Object? over dynamic for answer payload.Improves type safety without restricting value kinds.
-class AnswerQuestion extends SurveyEvent { +class AnswerQuestion extends SurveyEvent { final String questionId; - final dynamic answer; + final Object? answer; const AnswerQuestion(this.questionId, this.answer); @override List<Object?> get props => [questionId, answer]; }
31-39: Equatability of contextData map may be brittle.Including a Map in props relies on identity equality, which can cause duplicate events to be treated as different (or vice versa) depending on how the map instance is created.
Options:
- Normalize to a stable representation in props (e.g., JSON string), or
- Exclude contextData from props and keep only fields that affect behavior deterministically.
src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart (2)
65-67: Nit: guard progress percentage to max 100.0.If currentQuestionIndex overruns, progress can exceed 100.
- double get progressPercentage => - survey.questions.isNotEmpty ? (currentQuestionIndex + 1) / survey.questions.length * 100 : 0; + double get progressPercentage { + if (survey.questions.isEmpty) return 0; + final pct = ((currentQuestionIndex + 1) / survey.questions.length) * 100.0; + return pct.clamp(0.0, 100.0); + }
39-44: Consider making answers/contextData immutable in state.Exposing mutable maps can lead to in-place mutations that bypass state transitions.
Option: store unmodifiable views (e.g., Map.unmodifiable) or deep copies on construction and in copyWith.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (29)
src/mobile-v3/lib/main.dart(2 hunks)src/mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart(4 hunks)src/mobile-v3/lib/src/app/dashboard/repository/dashboard_repository.dart(4 hunks)src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart(1 hunks)src/mobile-v3/lib/src/app/dashboard/services/location_service_mananger.dart(4 hunks)src/mobile-v3/lib/src/app/dashboard/widgets/dashboard_app_bar.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/data_sharing_screen.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/location_data_view_screen.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/widgets/privacy_dialogs.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart(4 hunks)src/mobile-v3/lib/src/app/shared/services/notification_manager.dart(2 hunks)src/mobile-v3/lib/src/app/surveys/bloc/survey_bloc.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/bloc/survey_event.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/example/air_quality_alert_test_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/example/example_survey_data.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/example/survey_test_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_response_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/pages/survey_detail_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/pages/survey_list_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/widgets/survey_card.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/widgets/survey_progress_indicator.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/widgets/survey_question_widgets.dart(1 hunks)src/mobile-v3/lib/src/meta/utils/date_formatters.dart(1 hunks)
🔇 Additional comments (23)
src/mobile-v3/lib/src/app/dashboard/widgets/dashboard_app_bar.dart (1)
200-241: Handle HTTP URLs and remote SVGs in profile pictures
- Detect remote images with
profilePicture.startsWith('http')(covers both HTTP and HTTPS)- Render remote SVGs via
SvgPicture.networkwith a placeholder/fallback- Keep
SvgPicture.assetfor local SVG assetssrc/mobile-v3/lib/src/meta/utils/date_formatters.dart (1)
3-6: LGTM: simple and consistent datetime format helpersrc/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
29-45: Risky manual construction of geolocator.PositionGeolocator’s Position fields vary by version (e.g., isMocked required; altitude/heading accuracy fields may not exist). This may not compile or may break across platforms.
- Verify the geolocator version in pubspec and Position constructor signature.
- Consider decoupling: store a lightweight DTO for location (lat, lon, ts, accuracy) in JSON and keep Position only in runtime context (not serialized), or gate construction via a factory that adapts to the installed geolocator API.
- At minimum, null-guard timestamp in toJson.
Suggested minimal improvement for timestamp:
- 'timestamp': currentLocation!.timestamp.toIso8601String(), + 'timestamp': (currentLocation?.timestamp ?? timestamp).toIso8601String(),Also applies to: 56-77
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (1)
229-261: Good capture pipeline with privacy zone checkImmediate feedback via streams and persistence path looks sound.
src/mobile-v3/lib/src/app/profile/pages/location_data_view_screen.dart (1)
83-95: UI scaffolding looks solidColor theming, empty state, and list composition are cohesive.
src/mobile-v3/lib/main.dart (1)
10-10: Import looks fine.No conflicts detected; aligns with the new startup flow.
src/mobile-v3/lib/src/app/dashboard/repository/dashboard_repository.dart (1)
54-56: No action needed: SurveyTriggerService is a singleton
SurveyTriggerService uses a private_internalconstructor and factory returning a single_instance, so eachSurveyTriggerService()call yields the same instance.src/mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart (2)
74-76: Proper disposal.Good cleanup of services; prevents leaks.
41-43: Ignore singleton unification suggestion—SurveyTriggerService is already a singleton. The factory constructor in survey_trigger_service.dart returns the same_instancefor every call toSurveyTriggerService(), so there’s no split state across layers.Likely an incorrect or invalid review comment.
src/mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart (1)
42-114: Logout dialog UX refactor looks solid.Clear actions, themed correctly, and non-blocking flow on errors.
src/mobile-v3/lib/src/app/dashboard/services/location_service_mananger.dart (2)
165-167: LGTM: push location updates into the trigger service.Keeps triggers reactive to user movement.
238-242: Graceful stop.Cancelling and nulling the subscription is correct; logs help diagnose lifecycle.
src/mobile-v3/lib/src/app/surveys/pages/survey_list_page.dart (1)
48-60: Defensive handling of unknown states.Falling back to “empty” for any unknown state can hide bugs. Prefer an explicit
SurveyInitial/SurveysEmptybranch and a distinct fallback with a debug message.src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart (2)
911-915: Good: subscription cleanup in dispose.
399-455: UI empty-state looks solid and consistent with the app theme.src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (1)
114-153: Solid offline-first flow with local write-ahead + retry.src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart (1)
106-112: Nice: periodic time-based checks are isolated and cancellable.src/mobile-v3/lib/src/app/surveys/pages/survey_detail_page.dart (1)
269-270: Null-safety on completion date
existingResponse!.completedAt!assumes non-null. If the model ever flags completed without a timestamp (migration/backfill), this will throw. Guard or fallback format.- 'Thank you for your response! Completed on ${_formatDate(widget.existingResponse!.completedAt!)}', + 'Thank you for your response! Completed on ${_formatDate(widget.existingResponse?.completedAt ?? DateTime.now())}',src/mobile-v3/lib/src/app/surveys/widgets/survey_progress_indicator.dart (1)
224-258: Loading indicator widget is clean and reusableConcise, theming-aware, and matches the rest of the UI kit.
src/mobile-v3/lib/src/app/surveys/widgets/survey_card.dart (1)
5-128: Well-structured survey card implementationThe survey card widget is nicely implemented with good separation of concerns, proper dark mode support, and clear visual hierarchy. The use of helper methods for building different sections keeps the code clean and maintainable.
src/mobile-v3/lib/src/app/profile/pages/widgets/privacy_dialogs.dart (1)
443-447: Radius validation logic is solidGood validation for the radius bounds. The 10,000-meter limit is reasonable for privacy zones.
src/mobile-v3/lib/src/app/surveys/models/survey_response_model.dart (1)
1-215: Well-structured survey response modelsThe models are well-designed with proper JSON serialization, immutability through copyWith, and useful helper methods. The use of Equatable for value equality is a good choice for state management.
src/mobile-v3/lib/src/app/surveys/example/example_survey_data.dart (1)
168-171: Confirm daysOfWeek indexing convention (ISO 1–7 vs 0–6).The example assumes Monday=1..Friday=5. If the scheduler uses 0-based or a different convention, the trigger may never fire.
Use the script above to check scheduler expectations around "daysOfWeek".
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/dashboard/services/location_service_mananger.dart
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/surveys/widgets/survey_progress_indicator.dart
Outdated
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/surveys/widgets/survey_progress_indicator.dart
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/surveys/widgets/survey_question_widgets.dart
Outdated
Show resolved
Hide resolved
- Fix DateTime null safety issue in survey_bloc.dart - Add robust pollutant value parsing to handle num and String types - Optimize survey lookup using firstWhereOrNull - Enhance retry mechanism with proper attempt tracking - Add auth integration for user ID retrieval
- Add comprehensive survey system with dynamic question types - Implement location-based and air quality threshold survey triggers - Add enhanced privacy controls with granular location sharing options - Create survey progress tracking and submission retry mechanisms - Integrate contextual survey responses with user location data - Add air quality alert notifications with survey follow-ups - Implement survey completion tracking and analytics - Add privacy dialogs for location data management
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (15)
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (1)
306-309: Guard startup restore path with try/catchIf permission is missing at boot, startLocationTracking() will throw and can break init; log and continue.
- if (_isTrackingActive && !_isTrackingPaused && (_trackingTimer == null || !_trackingTimer!.isActive)) { - await startLocationTracking(); - } + if (_isTrackingActive && + !_isTrackingPaused && + (_trackingTimer == null || !_trackingTimer!.isActive)) { + try { + await startLocationTracking(); + } catch (e, s) { + loggy.warning('Could not restore location tracking on startup: $e', e, s); + } + }src/mobile-v3/lib/src/app/profile/pages/widgets/privacy_dialogs.dart (2)
437-505: Good fix: loading state now reset on all paths in _handleAddZone
Early-return bug addressed with try/finally and deferred snackbar. LGTM.
977-979: Use shared formatter utility for dates (consistency)- String _formatDate(DateTime date) { - return '${date.day.toString().padLeft(2, '0')}/${date.month.toString().padLeft(2, '0')}/${date.year}'; - } + String _formatDate(DateTime date) { + return DateFormatters.formatDate(date); + }src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
245-257: Epsilon-based comparison implementation looks solid.The implementation correctly uses epsilon tolerance for the 'equal' case and properly handles the 'greaterOrEqual' and 'lessOrEqual' cases with the tolerance. This addresses the previous review feedback comprehensively.
src/mobile-v3/lib/src/app/surveys/models/survey_model.dart (3)
52-66: Excellent defensive JSON parsing implementation.The refactored
fromJsonmethod now uses dedicated helper methods that provide robust type checking and error handling. This addresses the previous review concerns about unsafe casting and provides fallbacks for malformed data.
135-143: Safe conditions parsing implemented correctly.The
_parseConditionsMaphelper properly handles type conversion and provides appropriate fallbacks. This resolves the previous feedback about untyped conditions maps.
206-218: Robust parsing strategy with comprehensive fallbacks.The Survey parsing uses defensive helpers for all complex fields and provides sensible defaults. The approach of using helper methods like
_parseQuestionsList,_parseTrigger, and_parseDurationmakes the code more maintainable and less prone to runtime errors.src/mobile-v3/lib/src/app/surveys/example/air_quality_alert_test_page.dart (1)
12-15: Proper debug mode gating implemented.The implementation correctly uses
kDebugModeto prevent this test page from being accessible in production builds, which addresses the previous security concern.src/mobile-v3/lib/src/app/surveys/widgets/survey_question_widgets.dart (2)
195-216: Robust defensive validation for rating values.The implementation now includes proper bounds checking and type conversion with fallbacks, which addresses the previous concern about null maxValue handling. The clamping ensures values stay within reasonable ranges.
29-35: Proper text controller initialization and lifecycle management.The text controller is correctly initialized with the current answer and properly managed through the widget lifecycle. This resolves the previous issue where text inputs couldn't display existing answers.
src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (2)
2-2: Collection extensions import added correctly.The import for
package:collection/collection.dartenables safe nullable collection operations likefirstWhereOrNullused throughout the file.
94-94: Efficient nullable lookup implementation.Using
firstWhereOrNulldirectly instead ofwhere().firstOrNullis more efficient and follows the previous review suggestion.src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart (1)
3-3: Nice: addressed prior nits (collection import, firstWhereOrNull, robust parsing)Import, lookup, and pollutant parsing improvements look good.
Also applies to: 80-80, 141-157
src/mobile-v3/lib/src/app/surveys/example/survey_test_page.dart (1)
293-303: Avoid BlocProvider.value for newly created blocsUse
BlocProvider(create: ...)so disposal is managed automatically.void _openSurveyList() { try { - final bloc = SurveyBloc(_repository); Navigator.of(context).push( MaterialPageRoute( - builder: (context) => BlocProvider.value( - value: bloc, - child: const SurveyListPage(), - ), + builder: (context) => BlocProvider( + create: (_) => SurveyBloc(_repository), + child: const SurveyListPage(), + ), ), ); } catch (e) {src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart (1)
82-95: Validation improvements look solidRequired/emptiness checks and bounds validation are now in place.
Also applies to: 133-148
🧹 Nitpick comments (19)
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (4)
197-205: Capture immediately on start to avoid 5‑minute UX gapUsers shouldn’t wait for the first Timer tick. Capture once right after enabling (respecting pause).
- _trackingTimer = Timer.periodic(Duration(minutes: 5), (timer) async { + if (!_isTrackingPaused) { + await _captureLocationPoint(); + } + _trackingTimer = Timer.periodic(const Duration(minutes: 5), (timer) async { if (!_isTrackingPaused) { await _captureLocationPoint(); } });
262-268: Unbounded in-memory history — add retention policyPrevent growth and large SharedPreferences payloads. Keep a rolling window or cap the list.
_locationHistory.add(dataPoint); + // Retain only the most recent N points (e.g., 5000) + const maxPoints = 5000; + if (_locationHistory.length > maxPoints) { + _locationHistory.removeRange(0, _locationHistory.length - maxPoints); + }
91-95: Expose paused state via stream or structured statusOnly a bool “active” is streamed; UI can’t reliably reflect “paused”. Stream a small status model {active, paused} or a dedicated paused stream.
Also applies to: 104-106
117-124: IDs: prefer collision-safe UUIDs over timestampsTimestamps can collide on fast inserts. Consider uuid v4.
+import 'package:uuid/uuid.dart'; +final _uuid = const Uuid(); @@ - id: DateTime.now().millisecondsSinceEpoch.toString(), + id: _uuid.v4(), @@ - id: DateTime.now().millisecondsSinceEpoch.toString(), + id: _uuid.v4(),Note: requires adding uuid to pubspec.
Also applies to: 254-260
src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart (4)
45-54: Keep paused state in sync with manager
Update local paused flag when tracking status events arrive.void _setupTrackingListener() { _trackingSubscription = _locationManager.trackingStatusStream.listen((isActive) { if (mounted) { setState(() { _isTrackingActive = isActive; + _isTrackingPaused = _locationManager.isTrackingPaused; }); } }); }
107-115: After toggling OS location, re-evaluate real capability
Always refresh computed _locationEnabled at the end of the flow.- } finally { + } finally { if (mounted) { setState(() { _locationEnabled = shouldUpdateState ? value : _locationEnabled; _isProcessing = false; }); } + await _checkLocationStatus(); }
82-82: Prefer const Duration
Minor allocation win and consistency.- await Future.delayed(Duration(seconds: 1)); + await Future.delayed(const Duration(seconds: 1));
240-241: Copy tweak: clearer sentence- 'AirQo to use your precise location to locate the Air Quality of your nearest location', + 'Allow AirQo to use your precise location to show nearby air quality.',src/mobile-v3/lib/src/app/profile/pages/location_data_view_screen.dart (4)
42-60: Remove nested setState; sort inside a single state update
Less rebuild churn and simpler flow.- void _applyFilters() { - setState(() { - switch (_filterType) { - case 'shared': - _filteredHistory = _allHistory - .where((point) => point.isSharedWithResearchers) - .toList(); - break; - case 'private': - _filteredHistory = _allHistory - .where((point) => !point.isSharedWithResearchers) - .toList(); - break; - default: - _filteredHistory = List.from(_allHistory); - } - _applySorting(); - }); - } + void _applyFilters() { + List<LocationDataPoint> next; + switch (_filterType) { + case 'shared': + next = _allHistory.where((p) => p.isSharedWithResearchers).toList(); + break; + case 'private': + next = _allHistory.where((p) => !p.isSharedWithResearchers).toList(); + break; + default: + next = List.from(_allHistory); + } + setState(() { + _filteredHistory = next; + // inline sort + if (_sortOrder == 'newest') { + _filteredHistory.sort((a, b) => b.timestamp.compareTo(a.timestamp)); + } else { + _filteredHistory.sort((a, b) => a.timestamp.compareTo(b.timestamp)); + } + }); + } - void _applySorting() { - setState(() { - if (_sortOrder == 'newest') { - _filteredHistory.sort((a, b) => b.timestamp.compareTo(a.timestamp)); - } else { - _filteredHistory.sort((a, b) => a.timestamp.compareTo(b.timestamp)); - } - }); - } + void _applySorting() { + if (_sortOrder == 'newest') { + _filteredHistory.sort((a, b) => b.timestamp.compareTo(a.timestamp)); + } else { + _filteredHistory.sort((a, b) => a.timestamp.compareTo(b.timestamp)); + } + setState(() {}); + }Also applies to: 62-70
332-338: Combine sort change and sorting into one setState
Avoid sequential setStates on tap.- return GestureDetector( - onTap: () { - setState(() { - _sortOrder = value; - }); - _applySorting(); - }, + return GestureDetector( + onTap: () { + setState(() { + _sortOrder = value; + _applySorting(); + }); + },
447-454: Stabilize list items with keys
Prevents jank when deleting/filtering.- itemBuilder: (context, index) { - final point = _filteredHistory[index]; - return _buildLocationCard(point); - }, + itemBuilder: (context, index) { + final point = _filteredHistory[index]; + return KeyedSubtree( + key: ValueKey(point.id), + child: _buildLocationCard(point), + ); + },
6-14: Prefer ValueChanged for callback typing
Clearer intent than Function.- final Function(String pointId) onDeletePoint; + final ValueChanged<String> onDeletePoint;src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
29-54: Consider defensive parsing for Position creation.The
fromJsonmethod directly accesses nested properties without null checks. IflocDatacontains null values for required fields like longitude or latitude, this could throw runtime exceptions.factory SurveyTriggerContext.fromJson(Map<String, dynamic> json) { Position? location; if (json['currentLocation'] != null) { final locData = json['currentLocation']; - location = Position( - longitude: locData['longitude'], - latitude: locData['latitude'], - timestamp: DateTime.parse(locData['timestamp']), - accuracy: locData['accuracy'], - altitude: locData['altitude'] ?? 0.0, - altitudeAccuracy: locData['altitudeAccuracy'] ?? 0.0, - heading: locData['heading'] ?? 0.0, - headingAccuracy: locData['headingAccuracy'] ?? 0.0, - speed: locData['speed'] ?? 0.0, - speedAccuracy: locData['speedAccuracy'] ?? 0.0, - ); + try { + location = Position( + longitude: (locData['longitude'] as num).toDouble(), + latitude: (locData['latitude'] as num).toDouble(), + timestamp: DateTime.parse(locData['timestamp']), + accuracy: (locData['accuracy'] as num).toDouble(), + altitude: (locData['altitude'] as num?)?.toDouble() ?? 0.0, + altitudeAccuracy: (locData['altitudeAccuracy'] as num?)?.toDouble() ?? 0.0, + heading: (locData['heading'] as num?)?.toDouble() ?? 0.0, + headingAccuracy: (locData['headingAccuracy'] as num?)?.toDouble() ?? 0.0, + speed: (locData['speed'] as num?)?.toDouble() ?? 0.0, + speedAccuracy: (locData['speedAccuracy'] as num?)?.toDouble() ?? 0.0, + ); + } catch (e) { + location = null; + } }src/mobile-v3/lib/src/app/surveys/example/air_quality_alert_test_page.dart (1)
188-220: Consider extracting common survey creation logic.Both
_showSurveyDialogand_showSurveyBannercreate similar Survey objects with mostly identical structure. This duplication could be reduced for better maintainability.+ Survey _createTestSurvey(String id, String title, String description, SurveyQuestion question, Duration duration) { + return Survey( + id: id, + title: title, + description: description, + questions: [question], + trigger: SurveyTrigger( + type: SurveyTriggerType.airQualityThreshold, + conditions: {}, + ), + timeToComplete: duration, + isActive: true, + createdAt: DateTime.now(), + ); + } void _showSurveyDialog(BuildContext context) { - final testSurvey = Survey( - id: 'test_survey_dialog', - title: 'Air Quality Experience Survey', - description: 'Help us understand how air quality affects your daily activities and health.', - questions: [ - SurveyQuestion( - id: 'q1', - question: 'Did you receive an air quality alert today?', - type: QuestionType.yesNo, - isRequired: true, - ), - ], - trigger: SurveyTrigger( - type: SurveyTriggerType.airQualityThreshold, - conditions: {}, - ), - timeToComplete: const Duration(minutes: 2), - isActive: true, - createdAt: DateTime.now(), - ); + final testSurvey = _createTestSurvey( + 'test_survey_dialog', + 'Air Quality Experience Survey', + 'Help us understand how air quality affects your daily activities and health.', + SurveyQuestion( + id: 'q1', + question: 'Did you receive an air quality alert today?', + type: QuestionType.yesNo, + isRequired: true, + ), + const Duration(minutes: 2), + );src/mobile-v3/lib/src/app/surveys/widgets/survey_question_widgets.dart (1)
270-276: Consider defensive parsing for scale widget numeric values.The scale widget directly casts
minValueandmaxValueto double andcurrentAnswerwithout null checks. This could throw if the values are not numeric types.Widget _buildScale(BuildContext context) { final theme = Theme.of(context); - final minValue = (widget.question.minValue ?? 1).toDouble(); - final maxValue = (widget.question.maxValue ?? 10).toDouble(); - final currentValue = (widget.currentAnswer as double?) ?? minValue; + final minValue = ((widget.question.minValue as num?) ?? 1).toDouble(); + final maxValue = ((widget.question.maxValue as num?) ?? 10).toDouble(); + double currentValue = minValue; + try { + currentValue = (widget.currentAnswer as num?)?.toDouble() ?? minValue; + } catch (e) { + currentValue = minValue; + } + currentValue = currentValue.clamp(minValue, maxValue);src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (2)
17-17: Mock data flag should be configurable.The
_useMockDataflag is hardcoded tofalse. Consider making this configurable via environment variables or build configurations to facilitate development and testing workflows.- static const bool _useMockData = false; + static const bool _useMockData = bool.fromEnvironment('USE_MOCK_SURVEYS', defaultValue: false);
376-393: Response merging logic could be more explicit.The
_mergeResponsesmethod prioritizes API responses over cached ones, but this behavior isn't clearly documented. Consider adding documentation or making the merge strategy more explicit.+ /// Merges cached and API responses, with API responses taking precedence + /// for conflicts (same response ID). This ensures the server state is + /// authoritative while preserving local-only responses. List<SurveyResponse> _mergeResponses( List<SurveyResponse> cached, List<SurveyResponse> api ) {src/mobile-v3/lib/src/app/surveys/example/survey_test_page.dart (1)
321-326: Mounted guards for SnackBars across async flowsPrevent “called after dispose” errors in test helpers.
- ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text('Loaded ${surveys.length} example surveys'), backgroundColor: Colors.green, ), ); @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text('Error loading surveys: $e'), backgroundColor: Colors.red, ), ); @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( const SnackBar( content: Text('Failed to trigger survey'), backgroundColor: Colors.orange, ), ); @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text('Error triggering survey: $e'), backgroundColor: Colors.red, ), ); @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( const SnackBar( content: Text('Test data cleared'), backgroundColor: Colors.green, ), ); @@ - ScaffoldMessenger.of(context).showSnackBar( + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text('Error clearing data: $e'), backgroundColor: Colors.red, ), );Also applies to: 327-334, 345-352, 353-360, 398-404, 405-411
src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart (1)
64-67: Edge case: empty survey marks “last question” trueGuard for empty question lists.
- bool get isLastQuestion => currentQuestionIndex >= survey.questions.length - 1; + bool get isLastQuestion => + survey.questions.isEmpty || currentQuestionIndex >= survey.questions.length - 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
src/mobile-v3/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
src/mobile-v3/lib/main.dart(2 hunks)src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart(1 hunks)src/mobile-v3/lib/src/app/dashboard/services/location_service_mananger.dart(4 hunks)src/mobile-v3/lib/src/app/profile/pages/data_sharing_screen.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/location_data_view_screen.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/widgets/privacy_dialogs.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/bloc/survey_bloc.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/example/air_quality_alert_test_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/example/survey_test_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_response_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/pages/survey_list_page.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/widgets/survey_card.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/widgets/survey_progress_indicator.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/widgets/survey_question_widgets.dart(1 hunks)src/mobile-v3/pubspec.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/mobile-v3/lib/src/app/surveys/widgets/survey_progress_indicator.dart
- src/mobile-v3/lib/src/app/surveys/widgets/survey_card.dart
- src/mobile-v3/lib/main.dart
- src/mobile-v3/lib/src/app/surveys/pages/survey_list_page.dart
- src/mobile-v3/lib/src/app/surveys/bloc/survey_bloc.dart
- src/mobile-v3/lib/src/app/surveys/models/survey_response_model.dart
- src/mobile-v3/lib/src/app/dashboard/services/location_service_mananger.dart
- src/mobile-v3/lib/src/app/profile/pages/data_sharing_screen.dart
🔇 Additional comments (2)
src/mobile-v3/pubspec.yaml (1)
78-78: collection dependency is compatible and imports are correct. The SDK constraint (>=3.3.2 <4.0.0) supportscollection: ^1.19.1, and every use offirstWhereOrNullis backed by animport 'package:collection/collection.dart';. No changes needed.src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
199-201: Good implementation of epsilon-based equality comparison.The addition of the
_epsilonconstant with proper documentation addresses floating-point precision issues effectively. This aligns with best practices for numeric comparisons.
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart
Show resolved
Hide resolved
src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart
Outdated
Show resolved
Hide resolved
- Replace stored BuildContext with NavigationService for memory leak prevention - Add encrypted location storage using Hive with flutter_secure_storage - Implement proper retry logic with exponential backoff for survey submissions - Fix location-based triggers to respect enter/exit transitions - Add defensive error handling and input validation throughout - Improve type safety by replacing dynamic types where possible - Clean up unnecessary comments for better code clarity Key improvements: * Secure location data storage with AES encryption * Context-free navigation using global navigator key * Robust retry mechanism for failed survey submissions * Proper location geofence trigger logic * Enhanced error handling and validation * Memory leak prevention in singleton services
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (9)
src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
130-157: Enter/Exit transition handling looks solidTransition detection using previousPosition correctly honors triggerOnEnter/triggerOnExit and avoids first-fix false positives. Nice.
src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart (1)
115-160: Numeric parsing and bounds checks look correctint.tryParse/double.parse with NaN guard and min/max enforcement fix prior issues.
src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart (3)
71-86: Manual trigger now respects cooldown — goodUsing firstWhereOrNull and enforcing canTriggerSurvey before _triggerSurvey addresses duplicate prompts.
106-119: Periodic checks: immediate run + duplicate guard — goodCancelling any existing timer and firing an immediate check avoids the initial delay and duplicate timers.
166-201: AQ threshold triggers: robust parsing, alert + delayed trigger with re-check — goodParsing num/String, showing alert, and re-checking cooldown inside the Timer fixes prior pitfalls.
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (4)
178-186: Timer-based guard fixes the autostart bug—good catch
This replaces the brittle _isTrackingActive check.
300-303: Boot restore path re-initializes the timer when needed—approved
Matches the earlier recommendation.
32-39: Harden PrivacyZone.fromJson with strict casts
Prevents runtime type surprises.factory PrivacyZone.fromJson(Map<String, dynamic> json) => PrivacyZone( - id: json['id'], - name: json['name'], + id: json['id'] as String, + name: json['name'] as String, latitude: (json['latitude'] as num).toDouble(), longitude: (json['longitude'] as num).toDouble(), radius: (json['radius'] as num).toDouble(), - createdAt: DateTime.parse(json['createdAt']), + createdAt: DateTime.parse(json['createdAt'] as String), );
68-76: Harden LocationDataPoint.fromJson (id/bool casts)factory LocationDataPoint.fromJson(Map<String, dynamic> json) => LocationDataPoint( - id: json['id'], + id: json['id'] as String, latitude: (json['latitude'] as num).toDouble(), longitude: (json['longitude'] as num).toDouble(), - timestamp: DateTime.parse(json['timestamp']), + timestamp: DateTime.parse(json['timestamp'] as String), accuracy: json['accuracy'] != null ? (json['accuracy'] as num).toDouble() : null, - isSharedWithResearchers: json['isSharedWithResearchers'] ?? false, + isSharedWithResearchers: (json['isSharedWithResearchers'] as bool?) ?? false, );
🧹 Nitpick comments (12)
src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
211-275: Threshold comparisons: good epsilon use; make input tolerant and consider sustainedDuration
- Lower/normalize comparison to accept mixed-case inputs.
- sustainedDuration is modeled but unused here; either implement in the service layer or document as not yet enforced.
bool isTriggered(double currentValue) { - switch (comparison) { + switch (comparison.toLowerCase()) { case 'greater': return currentValue > threshold; case 'less': return currentValue < threshold; case 'equal': // Use epsilon-based comparison for floating-point equality return (currentValue - threshold).abs() <= _epsilon; case 'greaterorequal': // Greater than threshold OR within epsilon tolerance return currentValue > threshold || (currentValue - threshold).abs() <= _epsilon; case 'lessorequal': // Less than threshold OR within epsilon tolerance return currentValue < threshold || (currentValue - threshold).abs() <= _epsilon; default: return false; } }Is sustainedDuration enforced in SurveyTriggerService? If not, confirm intended behavior or defer/remove until implemented.
src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart (2)
65-67: Clamp progress to [0,100] to avoid edge driftA defensive clamp prevents accidental >100% due to off-by-one or index drift.
- double get progressPercentage => - survey.questions.isNotEmpty ? (currentQuestionIndex + 1) / survey.questions.length * 100 : 0; + double get progressPercentage { + if (survey.questions.isEmpty) return 0; + final pct = ((currentQuestionIndex + 1) / survey.questions.length) * 100.0; + return pct.clamp(0, 100); + }
170-176: Avoid shared Map references in copyWith when replacing answers/contextDataClone provided maps too to prevent external mutations affecting state.
return SurveyInProgress( survey: survey ?? this.survey, currentResponse: currentResponse ?? this.currentResponse, currentQuestionIndex: currentQuestionIndex ?? this.currentQuestionIndex, - answers: answers ?? Map<String, dynamic>.from(this.answers), - contextData: contextData ?? (this.contextData != null ? Map<String, dynamic>.from(this.contextData!) : null), + answers: answers != null ? Map<String, dynamic>.from(answers) : Map<String, dynamic>.from(this.answers), + contextData: contextData != null + ? Map<String, dynamic>.from(contextData) + : (this.contextData != null ? Map<String, dynamic>.from(this.contextData!) : null), );src/mobile-v3/lib/src/app/shared/services/navigation_service.dart (1)
21-42: Avoid constructing repositories in navigation layerCreating SurveyRepository here couples navigation to data layer and complicates testing/DI.
- Pass a repository via constructor/locator, or let SurveyDetailPage resolve it (e.g., Provider/GetIt).
- Short term: accept a SurveyRepository? parameter with a default injected instance.
src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart (1)
46-50: Timers after dispose can still fire; add a disposed flagA delayed Timer (e.g., AQ alert) can run after dispose. Guard callbacks with a _disposed flag.
class SurveyTriggerService with UiLoggy { @@ void dispose() { _periodicCheckTimer?.cancel(); _surveyTriggeredController.close(); loggy.info('SurveyTriggerService disposed'); } }Plus:
+ bool _disposed = false; void dispose() { + _disposed = true; _periodicCheckTimer?.cancel(); _surveyTriggeredController.close(); loggy.info('SurveyTriggerService disposed'); }And guard timer callback(s):
- Timer(const Duration(seconds: 3), () { + Timer(const Duration(seconds: 3), () { + if (_disposed) return; // Re-check eligibility before triggering (survey state may have changed) if (!canTriggerSurvey(survey.id)) { loggy.info('Survey ${survey.id} no longer eligible after delay - skipping trigger'); return; } _triggerSurvey(survey, contextData: contextData.toJson()); });src/mobile-v3/lib/src/app/dashboard/services/secure_location_storage.dart (1)
191-199: Simplify inclusive time-range deletion (avoid +/-1ms) and normalize to UTCClearer, inclusive bounds and resilient to differing time zones.
- final timestamp = DateTime.parse(point['timestamp']); - return timestamp.isAfter(start.subtract(Duration(milliseconds: 1))) && - timestamp.isBefore(end.add(Duration(milliseconds: 1))); + final ts = DateTime.parse(point['timestamp']).toUtc(); + final s = start.toUtc(); + final e = end.toUtc(); + return !ts.isBefore(s) && !ts.isAfter(e);src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (6)
190-198: Capture immediately on start to avoid a 5-minute blank window
Users see data right away._trackingTimer = Timer.periodic(Duration(minutes: 5), (timer) async { if (!_isTrackingPaused) { await _captureLocationPoint(); } }); + // Capture one point immediately + if (!_isTrackingPaused) { + await _captureLocationPoint(); + }
132-136: Single source of truth: persist zones via _savePrivacyZones()
Avoid double-write paths and potential divergence with storage helpers.Future<void> removePrivacyZone(String zoneId) async { _privacyZones.removeWhere((zone) => zone.id == zoneId); - await _secureStorage.removePrivacyZone(zoneId); + await _savePrivacyZones(); loggy.info('Removed privacy zone: $zoneId'); }
164-176: Same for history deletions: update in-memory list then save once
Keeps persistence logic centralized (and retention applied).Future<void> deleteLocationPoint(String pointId) async { _locationHistory.removeWhere((point) => point.id == pointId); - await _secureStorage.deleteLocationPoint(pointId); + await _saveLocationHistory(); loggy.info('Deleted location point: $pointId'); } Future<void> deleteLocationPointsInRange(DateTime start, DateTime end) async { _locationHistory.removeWhere((point) => - point.timestamp.isAfter(start.subtract(Duration(milliseconds: 1))) && - point.timestamp.isBefore(end.add(Duration(milliseconds: 1)))); - await _secureStorage.deleteLocationPointsInRange(start, end); + !point.timestamp.isBefore(start) && !point.timestamp.isAfter(end)); + await _saveLocationHistory(); loggy.info('Deleted location points between $start and $end'); }
433-439: Make dispose async and await storage cleanup
Ensures box is closed before shutdown.- void dispose() { + Future<void> dispose() async { _trackingTimer?.cancel(); _trackingStatusController.close(); _locationController.close(); - _secureStorage.dispose(); + await _secureStorage.dispose(); }
116-126: Validate privacy zone inputs
Defensive checks for lat/lng bounds and positive radius.Future<void> addPrivacyZone( String name, double lat, double lng, double radius) async { + if (radius <= 0) { + throw ArgumentError.value(radius, 'radius', 'must be > 0'); + } + if (lat < -90 || lat > 90 || lng < -180 || lng > 180) { + throw ArgumentError('Invalid latitude/longitude'); + } final zone = PrivacyZone(
182-186: Optional: request permission when denied to reduce UX friction
Call requestLocationPermission() on denied and proceed if granted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
src/mobile-v3/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
src/mobile-v3/lib/main.dart(2 hunks)src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart(1 hunks)src/mobile-v3/lib/src/app/dashboard/services/secure_location_storage.dart(1 hunks)src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart(1 hunks)src/mobile-v3/lib/src/app/shared/services/navigation_service.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart(1 hunks)src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart(1 hunks)src/mobile-v3/pubspec.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mobile-v3/pubspec.yaml
- src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart
- src/mobile-v3/lib/main.dart
- src/mobile-v3/lib/src/app/profile/pages/location_privacy_screen.dart
🔇 Additional comments (6)
src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart (1)
29-45: Position.fromJson: add null‐safety guards and defaults, confirm geolocator version
- Wrap raw
timestampinDateTime.tryParse(allowingnull) rather than unguardedDateTime.parse.- Default all numeric fields via
(locData['field'] ?? 0).toDouble()(accuracy, altitude, heading, speed, etc.).- Add
isMockedonly if your current geolocator version’sPositionconstructor includes it.- Extract JSON→Position parsing into a small helper to keep
fromJsonconcise.src/mobile-v3/lib/src/app/surveys/bloc/survey_state.dart (1)
96-113: Validation logic: typed and robust — good upgradeTyped helper, empty-string/list/map checks, and routing rating/scale to numeric validation are correct.
src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart (2)
121-141: Location triggers: correct enter/exit transition with previous positionPassing previousPosition into model.isTriggered and building a rich context is correct.
249-265: Hour/day triggers ignore repeat flag; confirm intentrepeat exists on the model but isn’t considered here. If repeat=false, you may want a one-time trigger at the first matching slot.
I can add a simple guard that blocks re-triggering once fired when repeat=false (tracked via history). Want a patch?
src/mobile-v3/lib/src/app/dashboard/services/secure_location_storage.dart (1)
1-7: Hive initialization confirmed.CacheManager.initialize()inmain.dartinvokesHive.initFlutter()before any box operations, so Hive is ready whenSecureLocationStorageruns.src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart (1)
1-5: Migration of location data to secure storage confirmed
Verified that the keysis_tracking_active,is_tracking_paused,privacy_zones, andlocation_historyare only defined and accessed viasecure_location_storage.dartusingFlutterSecureStorage, with no remainingSharedPreferencesusage for those location data keys elsewhere.
| late final SecureLocationStorage _secureStorage; | ||
| final StreamController<bool> _trackingStatusController = |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid LateInitializationError: create SecureLocationStorage eagerly
No reason to defer; box open remains lazy.
- late final SecureLocationStorage _secureStorage;
+ final SecureLocationStorage _secureStorage = SecureLocationStorage();
@@
// Initialize the service
Future<void> initialize() async {
- _secureStorage = SecureLocationStorage();
await _loadPrivacyZones();
await _loadLocationHistory();
await _loadTrackingSettings();
}Also applies to: 107-113
🤖 Prompt for AI Agents
In
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart
around lines 90-91 (and similarly 107-113), avoid declaring
SecureLocationStorage with late and instead initialize it eagerly inline;
replace "late final SecureLocationStorage _secureStorage;" with "final
SecureLocationStorage _secureStorage = SecureLocationStorage();" (or the
appropriate constructor call), and remove any separate assignment code paths
that expect delayed initialization so the instance is always created immediately
(box opening remains lazy so this is safe).
| loggy.info( | ||
| 'Location point captured: ${position.latitude}, ${position.longitude}'); | ||
| } catch (e) { |
There was a problem hiding this comment.
Do not log precise coordinates (PII)
Leakage risk. Downgrade to debug and obfuscate.
- loggy.info(
- 'Location point captured: ${position.latitude}, ${position.longitude}');
+ loggy.debug('Location point captured '
+ '(lat≈${position.latitude.toStringAsFixed(3)}, '
+ 'lon≈${position.longitude.toStringAsFixed(3)})');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| loggy.info( | |
| 'Location point captured: ${position.latitude}, ${position.longitude}'); | |
| } catch (e) { | |
| loggy.debug('Location point captured ' | |
| '(lat≈${position.latitude.toStringAsFixed(3)}, ' | |
| 'lon≈${position.longitude.toStringAsFixed(3)})'); | |
| } catch (e) { |
🤖 Prompt for AI Agents
In
src/mobile-v3/lib/src/app/dashboard/services/enhanced_location_service_manager.dart
around lines 261 to 263, the code currently logs exact latitude/longitude which
is PII; change the log level from info to debug and obfuscate the coordinates
before logging (for example round both latitude and longitude to 1–2 decimal
places or convert to a coarse geohash/region name) so the log never contains
full-precision coordinates; update the log call to include the obfuscated values
and retain a clear descriptive message.
| Future<Box> _getSecureBox() async { | ||
| return _boxLock.synchronized(() async { | ||
| if (_secureBox != null && _secureBox!.isOpen) { | ||
| return _secureBox!; | ||
| } | ||
|
|
||
| final encryptionKey = await _getOrCreateEncryptionKey(); | ||
| _secureBox = await Hive.openBox( | ||
| _locationBoxName, | ||
| encryptionCipher: HiveAesCipher(encryptionKey), | ||
| ); | ||
|
|
||
| return _secureBox!; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Recover gracefully on wrong encryption key/corruption; add compaction strategy
Opening the box will permanently fail if the app is reinstalled (key lost) while the encrypted box remains on disk. Add a recovery path that resets the box on key mismatch/corruption and enable compaction to keep the file small.
Future<Box> _getSecureBox() async {
return _boxLock.synchronized(() async {
if (_secureBox != null && _secureBox!.isOpen) {
return _secureBox!;
}
final encryptionKey = await _getOrCreateEncryptionKey();
- _secureBox = await Hive.openBox(
- _locationBoxName,
- encryptionCipher: HiveAesCipher(encryptionKey),
- );
+ try {
+ _secureBox = await Hive.openBox(
+ _locationBoxName,
+ encryptionCipher: HiveAesCipher(encryptionKey),
+ compactionStrategy: (entries, deleted) =>
+ deleted > 1000 || deleted > entries / 2,
+ );
+ } on HiveError catch (e) {
+ // Handle re-install/Keychain wipe: old box encrypted with a different key.
+ if (e.toString().toLowerCase().contains('encrypt') ||
+ e.toString().toLowerCase().contains('corrupt')) {
+ loggy.warning('Secure box open failed (likely wrong key). Resetting box.');
+ await Hive.deleteBoxFromDisk(_locationBoxName);
+ _secureBox = await Hive.openBox(
+ _locationBoxName,
+ encryptionCipher: HiveAesCipher(encryptionKey),
+ );
+ } else {
+ rethrow;
+ }
+ }
return _secureBox!;
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<Box> _getSecureBox() async { | |
| return _boxLock.synchronized(() async { | |
| if (_secureBox != null && _secureBox!.isOpen) { | |
| return _secureBox!; | |
| } | |
| final encryptionKey = await _getOrCreateEncryptionKey(); | |
| _secureBox = await Hive.openBox( | |
| _locationBoxName, | |
| encryptionCipher: HiveAesCipher(encryptionKey), | |
| ); | |
| return _secureBox!; | |
| }); | |
| } | |
| Future<Box> _getSecureBox() async { | |
| return _boxLock.synchronized(() async { | |
| if (_secureBox != null && _secureBox!.isOpen) { | |
| return _secureBox!; | |
| } | |
| final encryptionKey = await _getOrCreateEncryptionKey(); | |
| try { | |
| _secureBox = await Hive.openBox( | |
| _locationBoxName, | |
| encryptionCipher: HiveAesCipher(encryptionKey), | |
| compactionStrategy: (entries, deleted) => | |
| deleted > 1000 || deleted > entries / 2, | |
| ); | |
| } on HiveError catch (e) { | |
| // Handle re-install/Keychain wipe: old box encrypted with a different key. | |
| if (e.toString().toLowerCase().contains('encrypt') || | |
| e.toString().toLowerCase().contains('corrupt')) { | |
| loggy.warning('Secure box open failed (likely wrong key). Resetting box.'); | |
| await Hive.deleteBoxFromDisk(_locationBoxName); | |
| _secureBox = await Hive.openBox( | |
| _locationBoxName, | |
| encryptionCipher: HiveAesCipher(encryptionKey), | |
| ); | |
| } else { | |
| rethrow; | |
| } | |
| } | |
| return _secureBox!; | |
| }); | |
| } |
| Future<List<Map<String, dynamic>>> getPrivacyZones() async { | ||
| try { | ||
| final box = await _getSecureBox(); | ||
| final zones = box.get(_privacyZonesKey, defaultValue: <Map<String, dynamic>>[]); | ||
| return List<Map<String, dynamic>>.from(zones); | ||
| } catch (e) { | ||
| loggy.error('Error loading privacy zones: $e'); | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cast lists safely when reading from Hive
Avoid runtime type issues if Hive returns List/Map<dynamic,dynamic>.
Future<List<Map<String, dynamic>>> getPrivacyZones() async {
try {
final box = await _getSecureBox();
- final zones = box.get(_privacyZonesKey, defaultValue: <Map<String, dynamic>>[]);
- return List<Map<String, dynamic>>.from(zones);
+ final dynamic zones = box.get(_privacyZonesKey);
+ final List<dynamic> list = (zones as List?) ?? const <dynamic>[];
+ return list
+ .whereType<Map>()
+ .map((e) => Map<String, dynamic>.from(e as Map))
+ .toList();
} catch (e) {
loggy.error('Error loading privacy zones: $e');
return [];
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<List<Map<String, dynamic>>> getPrivacyZones() async { | |
| try { | |
| final box = await _getSecureBox(); | |
| final zones = box.get(_privacyZonesKey, defaultValue: <Map<String, dynamic>>[]); | |
| return List<Map<String, dynamic>>.from(zones); | |
| } catch (e) { | |
| loggy.error('Error loading privacy zones: $e'); | |
| return []; | |
| } | |
| } | |
| Future<List<Map<String, dynamic>>> getPrivacyZones() async { | |
| try { | |
| final box = await _getSecureBox(); | |
| final dynamic zones = box.get(_privacyZonesKey); | |
| final List<dynamic> list = (zones as List?) ?? const <dynamic>[]; | |
| return list | |
| .whereType<Map>() | |
| .map((e) => Map<String, dynamic>.from(e as Map)) | |
| .toList(); | |
| } catch (e) { | |
| loggy.error('Error loading privacy zones: $e'); | |
| return []; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/dashboard/services/secure_location_storage.dart
around lines 83 to 92, the current code assumes Hive returns
List<Map<String,dynamic>> which can cause runtime type errors when Hive returns
List<dynamic> or Map<dynamic,dynamic>; update the getter to treat the retrieved
value as a nullable List, iterate and safely convert each element to
Map<String,dynamic> (e.g. check type or cast each element and use
Map<String,dynamic>.from(element)) and return the mapped list, falling back to
an empty list if the stored value is null or contains non-map entries; keep the
try/catch and error logging as-is.
| Future<List<Map<String, dynamic>>> getLocationHistory() async { | ||
| try { | ||
| final box = await _getSecureBox(); | ||
| final history = box.get(_locationHistoryKey, defaultValue: <Map<String, dynamic>>[]); | ||
| return List<Map<String, dynamic>>.from(history); | ||
| } catch (e) { | ||
| loggy.error('Error loading location history: $e'); | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ditto: safe cast for location history list
Future<List<Map<String, dynamic>>> getLocationHistory() async {
try {
final box = await _getSecureBox();
- final history = box.get(_locationHistoryKey, defaultValue: <Map<String, dynamic>>[]);
- return List<Map<String, dynamic>>.from(history);
+ final dynamic history = box.get(_locationHistoryKey);
+ final List<dynamic> list = (history as List?) ?? const <dynamic>[];
+ return list
+ .whereType<Map>()
+ .map((e) => Map<String, dynamic>.from(e as Map))
+ .toList();
} catch (e) {
loggy.error('Error loading location history: $e');
return [];
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<List<Map<String, dynamic>>> getLocationHistory() async { | |
| try { | |
| final box = await _getSecureBox(); | |
| final history = box.get(_locationHistoryKey, defaultValue: <Map<String, dynamic>>[]); | |
| return List<Map<String, dynamic>>.from(history); | |
| } catch (e) { | |
| loggy.error('Error loading location history: $e'); | |
| return []; | |
| } | |
| } | |
| Future<List<Map<String, dynamic>>> getLocationHistory() async { | |
| try { | |
| final box = await _getSecureBox(); | |
| final dynamic history = box.get(_locationHistoryKey); | |
| final List<dynamic> list = (history as List?) ?? const <dynamic>[]; | |
| return list | |
| .whereType<Map>() | |
| .map((e) => Map<String, dynamic>.from(e as Map)) | |
| .toList(); | |
| } catch (e) { | |
| loggy.error('Error loading location history: $e'); | |
| return []; | |
| } | |
| } |
| NavigatorState? get _navigator => navigatorKey.currentState; | ||
| BuildContext? get currentContext => navigatorKey.currentContext; | ||
| bool get canNavigate => _navigator != null && currentContext != null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Overly strict canNavigate gate blocks pushes when context is null
Push/pop only need NavigatorState; notifications need context. Split the checks.
- BuildContext? get currentContext => navigatorKey.currentContext;
- bool get canNavigate => _navigator != null && currentContext != null;
+ BuildContext? get currentContext => navigatorKey.currentContext;
+ bool get hasNavigator => _navigator != null;
+ bool get hasContext => currentContext != null;Then update guards:
- For navigation (push/pop/replace): use hasNavigator.
- For notifications (showSurveyNotification/showAirQualityAlert): use hasContext.
Example:
- if (!canNavigate) {
+ if (!hasNavigator) {
loggy.warning('Cannot push route: Navigator not available');and
- if (!canNavigate) {
+ if (!hasContext) {
loggy.warning('Cannot show survey notification: Context not available');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NavigatorState? get _navigator => navigatorKey.currentState; | |
| BuildContext? get currentContext => navigatorKey.currentContext; | |
| bool get canNavigate => _navigator != null && currentContext != null; | |
| NavigatorState? get _navigator => navigatorKey.currentState; | |
| BuildContext? get currentContext => navigatorKey.currentContext; | |
| bool get hasNavigator => _navigator != null; | |
| bool get hasContext => currentContext != null; |
🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/shared/services/navigation_service.dart around
lines 17 to 20, the current canNavigate getter is too strict (requires both
NavigatorState and BuildContext) which prevents push/pop when context is null;
split this into two getters like hasNavigator (checks navigatorKey.currentState
!= null) and hasContext (checks navigatorKey.currentContext != null), keep or
rename existing canNavigate if needed for backwards compatibility, then update
method guards so navigation operations (push/pop/replace) use hasNavigator and
notification methods (showSurveyNotification/showAirQualityAlert) use
hasContext.
| Map<String, dynamic> toJson() { | ||
| return { | ||
| if (currentLocation != null) | ||
| 'currentLocation': { | ||
| 'longitude': currentLocation!.longitude, | ||
| 'latitude': currentLocation!.latitude, | ||
| 'timestamp': currentLocation!.timestamp.toIso8601String(), | ||
| 'accuracy': currentLocation!.accuracy, | ||
| 'altitude': currentLocation!.altitude, | ||
| 'altitudeAccuracy': currentLocation!.altitudeAccuracy, | ||
| 'heading': currentLocation!.heading, | ||
| 'headingAccuracy': currentLocation!.headingAccuracy, | ||
| 'speed': currentLocation!.speed, | ||
| 'speedAccuracy': currentLocation!.speedAccuracy, | ||
| }, | ||
| if (currentAirQuality != null) 'currentAirQuality': currentAirQuality, | ||
| if (currentAirQualityCategory != null) | ||
| 'currentAirQualityCategory': currentAirQualityCategory, | ||
| 'timestamp': timestamp.toIso8601String(), | ||
| if (additionalData != null) 'additionalData': additionalData, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safe timestamp serialization for Position
Position.timestamp can be null. The current code force-calls toIso8601String().
- 'timestamp': currentLocation!.timestamp.toIso8601String(),
+ 'timestamp': currentLocation!.timestamp?.toIso8601String() ?? DateTime.now().toIso8601String(),🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/surveys/models/survey_trigger_model.dart around
lines 56 to 77, the serialization force-calls toIso8601String() on
currentLocation!.timestamp which can be null; update the map construction to
guard the timestamp by either using a null-aware call (e.g. 'timestamp':
currentLocation!.timestamp?.toIso8601String()) or include the 'timestamp' key
only when currentLocation!.timestamp != null so you don't call toIso8601String()
on a null value.
| if (timeCondition.specificTime != null) { | ||
| final targetTime = timeCondition.specificTime!; | ||
| final timeDiff = now.difference(targetTime).abs(); | ||
|
|
||
| if (timeDiff.inMinutes < 5) { // Within 5 minutes of target time | ||
| _triggerSurvey(survey); | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specific time trigger fires early (before target) due to abs()
Using abs() triggers even if now is before targetTime. Require now >= targetTime.
- final timeDiff = now.difference(targetTime).abs();
-
- if (timeDiff.inMinutes < 5) { // Within 5 minutes of target time
+ if (now.isAfter(targetTime) || now.isAtSameMomentAs(targetTime)) {
+ final timeDiff = now.difference(targetTime);
+ if (timeDiff.inMinutes < 5) { // Within 5 minutes after target time
_triggerSurvey(survey);
continue;
- }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (timeCondition.specificTime != null) { | |
| final targetTime = timeCondition.specificTime!; | |
| final timeDiff = now.difference(targetTime).abs(); | |
| if (timeDiff.inMinutes < 5) { // Within 5 minutes of target time | |
| _triggerSurvey(survey); | |
| continue; | |
| } | |
| } | |
| if (timeCondition.specificTime != null) { | |
| final targetTime = timeCondition.specificTime!; | |
| if (now.isAfter(targetTime) || now.isAtSameMomentAs(targetTime)) { | |
| final timeDiff = now.difference(targetTime); | |
| if (timeDiff.inMinutes < 5) { // Within 5 minutes after target time | |
| _triggerSurvey(survey); | |
| continue; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart around
lines 223 to 231, the code uses abs() on now.difference(targetTime) which allows
triggers before the target time; change the condition to require now >=
targetTime and still be within the 5-minute window. Replace the abs()-based
check with a non-negative check (e.g. ensure !timeDiff.isNegative or
now.isAfter(targetTime) || now.isAtSameMomentAs(targetTime)) and then verify
timeDiff.inMinutes < 5 before calling _triggerSurvey(survey).
| /// Load trigger history from storage | ||
| Future<void> _loadTriggerHistory() async { | ||
| try { | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| final historyString = prefs.getString('survey_trigger_history'); | ||
|
|
||
| if (historyString != null) { | ||
| final historyList = jsonDecode(historyString) as List; | ||
| _triggerHistory = historyList | ||
| .map((json) => SurveyTriggerHistory.fromJson(json)) | ||
| .toList(); | ||
|
|
||
| loggy.info('Loaded ${_triggerHistory.length} trigger history entries'); | ||
| } | ||
| } catch (e) { | ||
| loggy.error('Error loading trigger history: $e'); | ||
| _triggerHistory = []; | ||
| } | ||
| } | ||
|
|
||
| /// Save trigger history to storage | ||
| Future<void> _saveTriggerHistory() async { | ||
| try { | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| final historyJson = _triggerHistory.map((h) => h.toJson()).toList(); | ||
| await prefs.setString('survey_trigger_history', jsonEncode(historyJson)); | ||
| } catch (e) { | ||
| loggy.error('Error saving trigger history: $e'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PII risk: storing precise location context in SharedPreferences
Trigger history includes lastContext with lat/lon/timestamps in plaintext storage. Use secure storage (e.g., flutter_secure_storage) or encrypt before persisting.
Directionally:
- final prefs = await SharedPreferences.getInstance();
- final historyString = prefs.getString('survey_trigger_history');
+ // TODO: migrate to secure storage
+ final prefs = await SharedPreferences.getInstance();
+ final historyString = prefs.getString('survey_trigger_history'); // consider decrypting hereAnd in save:
- await prefs.setString('survey_trigger_history', jsonEncode(historyJson));
+ // Consider encrypting before write or using flutter_secure_storage
+ await prefs.setString('survey_trigger_history', jsonEncode(historyJson));Happy to wire a Storage abstraction to swap implementations cleanly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Load trigger history from storage | |
| Future<void> _loadTriggerHistory() async { | |
| try { | |
| final prefs = await SharedPreferences.getInstance(); | |
| final historyString = prefs.getString('survey_trigger_history'); | |
| if (historyString != null) { | |
| final historyList = jsonDecode(historyString) as List; | |
| _triggerHistory = historyList | |
| .map((json) => SurveyTriggerHistory.fromJson(json)) | |
| .toList(); | |
| loggy.info('Loaded ${_triggerHistory.length} trigger history entries'); | |
| } | |
| } catch (e) { | |
| loggy.error('Error loading trigger history: $e'); | |
| _triggerHistory = []; | |
| } | |
| } | |
| /// Save trigger history to storage | |
| Future<void> _saveTriggerHistory() async { | |
| try { | |
| final prefs = await SharedPreferences.getInstance(); | |
| final historyJson = _triggerHistory.map((h) => h.toJson()).toList(); | |
| await prefs.setString('survey_trigger_history', jsonEncode(historyJson)); | |
| } catch (e) { | |
| loggy.error('Error saving trigger history: $e'); | |
| } | |
| } | |
| /// Load trigger history from storage | |
| Future<void> _loadTriggerHistory() async { | |
| try { | |
| // TODO: migrate to secure storage (e.g. flutter_secure_storage) | |
| final prefs = await SharedPreferences.getInstance(); | |
| final historyString = prefs.getString('survey_trigger_history'); // consider decrypting here | |
| if (historyString != null) { | |
| final historyList = jsonDecode(historyString) as List; | |
| _triggerHistory = historyList | |
| .map((json) => SurveyTriggerHistory.fromJson(json)) | |
| .toList(); | |
| loggy.info('Loaded ${_triggerHistory.length} trigger history entries'); | |
| } | |
| } catch (e) { | |
| loggy.error('Error loading trigger history: $e'); | |
| _triggerHistory = []; | |
| } | |
| } | |
| /// Save trigger history to storage | |
| Future<void> _saveTriggerHistory() async { | |
| try { | |
| // Consider encrypting before write or switching to secure storage | |
| final prefs = await SharedPreferences.getInstance(); | |
| final historyJson = _triggerHistory.map((h) => h.toJson()).toList(); | |
| await prefs.setString('survey_trigger_history', jsonEncode(historyJson)); | |
| } catch (e) { | |
| loggy.error('Error saving trigger history: $e'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/surveys/services/survey_trigger_service.dart around
lines 331 to 360, the trigger history is persisted to SharedPreferences
including precise lastContext (lat/lon/timestamps) which is PII; replace
plaintext SharedPreferences usage with a secure persistence strategy: introduce
a Storage abstraction (or use flutter_secure_storage) and use it for
_loadTriggerHistory and _saveTriggerHistory, or encrypt/de-identify the
lastContext before jsonEncode (e.g., round coordinates or remove timestamps)
then persist the encrypted/filtered payload; ensure deserialization
decrypts/restores or handles missing PII safely, and update error logging
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (1)
9-9: Remove unused http import.Not referenced anywhere; keep the surface small.
-import 'package:http/http.dart' as http;
🧹 Nitpick comments (5)
src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (5)
159-164: Build query via params instead of string concat.Lets BaseRepository handle encoding.
- final endpoint = surveyId != null - ? '$_surveyResponsesEndpoint?surveyId=$surveyId' - : _surveyResponsesEndpoint; - - final apiResponse = await createAuthenticatedGetRequest(endpoint, {}); + final Map<String, String> query = surveyId != null ? {'surveyId': surveyId} : {}; + final apiResponse = await createAuthenticatedGetRequest(_surveyResponsesEndpoint, query); final data = json.decode(apiResponse.body);
171-176: Avoid O(n²) cache writes; batch-save responses.Currently writes the whole list for each item. Save once.
- responses = _mergeResponses(cachedResponses, apiResponses); - - for (final response in apiResponses) { - await _cacheSurveyResponse(response); - } + responses = _mergeResponses(cachedResponses, apiResponses); + await _cacheSurveyResponses(responses);Add helper (place near _cacheSurveyResponse):
Future<void> _cacheSurveyResponses(List<SurveyResponse> responses) async { try { final responsesJson = responses.map((r) => r.toJson()).toList(); await HiveRepository.saveData(_surveyResponsesBoxName, 'responses', json.encode(responsesJson)); } catch (e) { loggy.error('Error caching survey responses: $e'); } }
216-267: Persist retry attempts across restarts (optional).Store attempt counters per response in cache (e.g., a tiny Hive map or a field on SurveyResponse) and reset on success to avoid hot-looping after app relaunch.
44-59: Validate HTTP status and improve error mapping.Check statusCode before decoding; map by code instead of String.contains on exception messages.
final response = await createAuthenticatedGetRequest(_surveysEndpoint, queryParams); - final data = json.decode(response.body); + if (response.statusCode < 200 || response.statusCode >= 300) { + throw Exception('HTTP ${response.statusCode}'); + } + final data = json.decode(response.body);Then replace the String.contains mapping with a status-aware flow (propagate/inspect status rather than message parsing).
Also applies to: 71-81
11-407: Add focused tests for repository behaviors.Cover: caching fallback on network failure, getSurvey dual-shape handling, merge logic, retryFailedSubmissions (success/failure paths, maxRetries), and clearCache semantics.
Do you want scaffolding for a minimal test suite exercising these paths?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart(1 hunks)
🔇 Additional comments (1)
src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart (1)
98-103: Handle both single-object and list API shapes for getSurvey.Be tolerant to either {survey:{...}} or {surveys:[...]}; current code assumes list only.
- if (data['success'] == true && data['surveys'] != null && data['surveys'].isNotEmpty) { - return Survey.fromJson(data['surveys'][0]); - } - - return null; + if (data['success'] == true) { + if (data['survey'] != null) { + return Survey.fromJson(data['survey']); + } + if (data['surveys'] is List && data['surveys'].isNotEmpty) { + return Survey.fromJson(data['surveys'][0]); + } + } + return null;
| try { | ||
|
|
||
| if (currentRetryCount > 0) { | ||
| final delayMs = _baseDelayMs * (1 << currentRetryCount); | ||
| loggy.info('Waiting ${delayMs}ms before retry attempt ${currentRetryCount + 1} for response ${response.id}'); | ||
| await Future.delayed(Duration(milliseconds: delayMs)); | ||
| } | ||
|
|
||
| _retryCount[response.id] = currentRetryCount + 1; | ||
|
|
||
| await submitSurveyResponse(response); | ||
|
|
||
| _retryCount.remove(response.id); | ||
| loggy.info('Successfully submitted response ${response.id} after ${currentRetryCount + 1} attempt(s)'); | ||
|
|
||
| } catch (e) { | ||
| final newRetryCount = _retryCount[response.id] ?? 0; | ||
| loggy.error('Failed to submit response ${response.id} (attempt $newRetryCount): $e'); | ||
|
|
||
| if (newRetryCount >= maxRetries) { | ||
| loggy.error('Response ${response.id} failed after $maxRetries attempts, marking as failed'); | ||
| _retryCount.remove(response.id); | ||
|
|
||
| } | ||
|
|
||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Retry logic treats failures as success (always clears counter).
submitSurveyResponse returns bool; no exception on failure. Current code removes retry entry and logs success regardless.
- try {
-
- if (currentRetryCount > 0) {
- final delayMs = _baseDelayMs * (1 << currentRetryCount);
- loggy.info('Waiting ${delayMs}ms before retry attempt ${currentRetryCount + 1} for response ${response.id}');
- await Future.delayed(Duration(milliseconds: delayMs));
- }
-
- _retryCount[response.id] = currentRetryCount + 1;
-
- await submitSurveyResponse(response);
-
- _retryCount.remove(response.id);
- loggy.info('Successfully submitted response ${response.id} after ${currentRetryCount + 1} attempt(s)');
-
- } catch (e) {
- final newRetryCount = _retryCount[response.id] ?? 0;
- loggy.error('Failed to submit response ${response.id} (attempt $newRetryCount): $e');
-
- if (newRetryCount >= maxRetries) {
- loggy.error('Response ${response.id} failed after $maxRetries attempts, marking as failed');
- _retryCount.remove(response.id);
-
- }
-
- continue;
- }
+ if (currentRetryCount > 0) {
+ final delayMs = _baseDelayMs * (1 << currentRetryCount);
+ loggy.info('Waiting ${delayMs}ms before retry attempt ${currentRetryCount + 1} for response ${response.id}');
+ await Future.delayed(Duration(milliseconds: delayMs));
+ }
+ final attemptNo = currentRetryCount + 1;
+ _retryCount[response.id] = attemptNo;
+ final success = await submitSurveyResponse(response);
+ if (success) {
+ _retryCount.remove(response.id);
+ loggy.info('Successfully submitted response ${response.id} after $attemptNo attempt(s)');
+ } else {
+ final newRetryCount = _retryCount[response.id] ?? attemptNo;
+ loggy.warning('Submission still failing for ${response.id} (attempt $newRetryCount/$maxRetries)');
+ if (newRetryCount >= maxRetries) {
+ loggy.error('Response ${response.id} failed after $maxRetries attempts; leaving cached as inProgress for future retries.');
+ _retryCount.remove(response.id);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| if (currentRetryCount > 0) { | |
| final delayMs = _baseDelayMs * (1 << currentRetryCount); | |
| loggy.info('Waiting ${delayMs}ms before retry attempt ${currentRetryCount + 1} for response ${response.id}'); | |
| await Future.delayed(Duration(milliseconds: delayMs)); | |
| } | |
| _retryCount[response.id] = currentRetryCount + 1; | |
| await submitSurveyResponse(response); | |
| _retryCount.remove(response.id); | |
| loggy.info('Successfully submitted response ${response.id} after ${currentRetryCount + 1} attempt(s)'); | |
| } catch (e) { | |
| final newRetryCount = _retryCount[response.id] ?? 0; | |
| loggy.error('Failed to submit response ${response.id} (attempt $newRetryCount): $e'); | |
| if (newRetryCount >= maxRetries) { | |
| loggy.error('Response ${response.id} failed after $maxRetries attempts, marking as failed'); | |
| _retryCount.remove(response.id); | |
| } | |
| continue; | |
| } | |
| } | |
| // Exponential backoff if this is a retry | |
| if (currentRetryCount > 0) { | |
| final delayMs = _baseDelayMs * (1 << currentRetryCount); | |
| loggy.info( | |
| 'Waiting ${delayMs}ms before retry attempt ${currentRetryCount + 1} for response ${response.id}' | |
| ); | |
| await Future.delayed(Duration(milliseconds: delayMs)); | |
| } | |
| // Record this attempt number | |
| final attemptNo = currentRetryCount + 1; | |
| _retryCount[response.id] = attemptNo; | |
| // Actually submit and check success | |
| final success = await submitSurveyResponse(response); | |
| if (success) { | |
| // On success, clear retry tracking | |
| _retryCount.remove(response.id); | |
| loggy.info( | |
| 'Successfully submitted response ${response.id} after $attemptNo attempt(s)' | |
| ); | |
| } else { | |
| // On failure, log and decide whether to keep or drop | |
| final newRetryCount = _retryCount[response.id] ?? attemptNo; | |
| loggy.warning( | |
| 'Submission still failing for ${response.id} (attempt $newRetryCount/$maxRetries)' | |
| ); | |
| if (newRetryCount >= maxRetries) { | |
| loggy.error( | |
| 'Response ${response.id} failed after $maxRetries attempts; leaving cached as inProgress for future retries.' | |
| ); | |
| _retryCount.remove(response.id); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart around
lines 233 to 260, the retry loop currently treats any call to
submitSurveyResponse as success because it unconditionally removes the
_retryCount entry and logs success; however submitSurveyResponse returns a bool
and does not throw on failure. Change the try block to capture the boolean
result from await submitSurveyResponse(response); only when it returns true
should you remove _retryCount[response.id] and log success; when it returns
false increment the retry counter (or set it if missing), log a failure for that
attempt, and only remove and mark failed after exceeding maxRetries; do not
clear the counter on a false result. Ensure catch still handles real exceptions,
and adjust logging to include attempt number consistently.
| Future<void> clearCache() async { | ||
| try { | ||
| await HiveRepository.deleteData(_surveysBoxName, 'surveys'); | ||
| await HiveRepository.deleteData(_surveyResponsesBoxName, 'responses'); | ||
|
|
||
| final responses = await _getCachedSurveyResponses(); | ||
| for (final response in responses) { | ||
| await HiveRepository.deleteData(_surveyStatsBoxName, response.surveyId); | ||
| } | ||
| loggy.info('Cleared all survey cache'); | ||
| } catch (e) { |
There was a problem hiding this comment.
clearCache deletes responses before using them to clear stats.
Read responses first, then delete; otherwise stats never get cleared.
Future<void> clearCache() async {
try {
- await HiveRepository.deleteData(_surveysBoxName, 'surveys');
- await HiveRepository.deleteData(_surveyResponsesBoxName, 'responses');
-
- final responses = await _getCachedSurveyResponses();
- for (final response in responses) {
- await HiveRepository.deleteData(_surveyStatsBoxName, response.surveyId);
- }
+ final responses = await _getCachedSurveyResponses(); // read before deletion
+ await HiveRepository.deleteData(_surveysBoxName, 'surveys');
+ for (final response in responses) {
+ await HiveRepository.deleteData(_surveyStatsBoxName, response.surveyId);
+ }
+ await HiveRepository.deleteData(_surveyResponsesBoxName, 'responses');
loggy.info('Cleared all survey cache');
} catch (e) {
loggy.error('Error clearing cache: $e');
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<void> clearCache() async { | |
| try { | |
| await HiveRepository.deleteData(_surveysBoxName, 'surveys'); | |
| await HiveRepository.deleteData(_surveyResponsesBoxName, 'responses'); | |
| final responses = await _getCachedSurveyResponses(); | |
| for (final response in responses) { | |
| await HiveRepository.deleteData(_surveyStatsBoxName, response.surveyId); | |
| } | |
| loggy.info('Cleared all survey cache'); | |
| } catch (e) { | |
| Future<void> clearCache() async { | |
| try { | |
| // Read all responses first so we can clear their stats before deleting them | |
| final responses = await _getCachedSurveyResponses(); | |
| // Now delete surveys, then per-survey stats, then the responses themselves | |
| await HiveRepository.deleteData(_surveysBoxName, 'surveys'); | |
| for (final response in responses) { | |
| await HiveRepository.deleteData(_surveyStatsBoxName, response.surveyId); | |
| } | |
| await HiveRepository.deleteData(_surveyResponsesBoxName, 'responses'); | |
| loggy.info('Cleared all survey cache'); | |
| } catch (e) { | |
| loggy.error('Error clearing cache: $e'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/mobile-v3/lib/src/app/surveys/repository/survey_repository.dart around
lines 269 to 279, clearCache currently deletes the responses box before reading
cached survey responses so the subsequent loop that deletes per-survey stats
finds no responses; fix by first calling _getCachedSurveyResponses() and
iterating to delete each survey's stats, then delete the responses and surveys
boxes (i.e., read responses -> delete stats for each response -> delete
responses and surveys), preserving the try/catch behavior.
| Future<void> _cacheSurveyResponse(SurveyResponse response) async { | ||
| try { | ||
| final responses = await _getCachedSurveyResponses(); | ||
|
|
||
| final existingIndex = responses.indexWhere((r) => r.id == response.id); | ||
| if (existingIndex >= 0) { | ||
| responses[existingIndex] = response; | ||
| } else { | ||
| responses.add(response); | ||
| } | ||
|
|
||
| final responsesJson = responses.map((r) => r.toJson()).toList(); | ||
| await HiveRepository.saveData(_surveyResponsesBoxName, 'responses', json.encode(responsesJson)); | ||
| } catch (e) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Encrypt cached survey responses if any PII is stored.
If answers can include PII, use an encrypted Hive box/cipher.
🏁 Script executed:
#!/bin/bash
# Check for encryption usage in HiveRepository and call sites
rg -n -C2 'HiveRepository|openBox|encryption|Cipher|secure' -- src/mobile-v3/libLength of output: 39879
🏁 Script executed:
#!/bin/bash
# Locate SurveyResponse model to inspect for PII fields
rg -nP 'class\s+SurveyResponse\b' -C5 -- src/mobile-v3/libLength of output: 1087
🏁 Script executed:
#!/bin/bash
# Locate SurveyAnswer definition to inspect for PII fields
rg -nP 'class\s+SurveyAnswer\b' -C10 -- src/mobile-v3/lib/src/app/surveys/models/survey_response_model.dartLength of output: 590
Encrypt survey response cache
SurveyResponse contains userId and arbitrary answers (free‐text), which may include PII. Persist them in an encrypted Hive box by opening with an encryptionCipher (e.g. HiveAesCipher) and storing the key securely (via flutter_secure_storage), or encapsulate in a dedicated SecureSurveyStorage.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit