Conversation
…locally and syncing when connectivity is restored. - Removed direct API calls from BiteReportController and BreedingSiteReportController. - Integrated ReportSyncService for handling report submissions and queuing when offline. - Added error handling and success dialogs for report submissions. - Created new service (report_sync_service.dart) to manage pending reports and sync them with the server. - Updated BackgroundTracking to include functionality for syncing pending fixes. - Adjusted dependencies in pubspec.yaml and updated pubspec.lock for path_provider.
JohnPalmer
requested review from
Copilot and
epou
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This pull request refactors the mosquito tracking app's report submission and background location tracking to support offline persistence with automatic retry. It introduces client-side coordinate masking for background location fixes, adds a comprehensive report queue service, and updates the iOS project configuration for production deployment.
Key changes include:
- Implementing offline-first architecture with local persistence for both user reports and background location fixes
- Adding device-side coordinate masking (0.025° grid) for background tracking to ensure privacy even before server sync
- Refactoring report controllers to use the new ReportSyncService instead of direct API calls
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pubspec.yaml | Adds path_provider as a direct dependency for accessing application directories to store queued data |
| pubspec.lock | Updates path_provider dependency type from transitive to direct main |
| lib/utils/BackgroundTracking.dart | Implements offline persistence for background location fixes with coordinate masking, queue management, and connectivity-aware retry logic |
| lib/services/report_sync_service.dart | Introduces new service to queue and sync user reports (adult, bite, breeding) when offline with automatic retry |
| lib/pages/reports/sites/breeding_site_report_controller.dart | Refactors to use ReportSyncService instead of direct API calls, adds queued submission handling |
| lib/pages/reports/bite/bite_report_controller.dart | Refactors to use ReportSyncService instead of direct API calls, adds queued submission handling |
| lib/pages/reports/adult/adult_report_controller.dart | Refactors to use ReportSyncService instead of direct API calls, adds queued submission handling and campaign display logic |
| lib/pages/main/home_page.dart | Triggers pending report sync when home page loads to flush any queued reports |
| lib/pages/main/drawer_and_header.dart | Adds syncPendingFixes call when configuring background tracking |
| lib/main.dart | Initializes ReportSyncService at app startup and adds syncPendingFixes handler to callback dispatcher |
| ios/Runner/Info.plist | Reformats indentation and changes CFBundleIdentifier to use build variable substitution |
| ios/Runner.xcodeproj/project.pbxproj | Updates development team, bundle identifier to production value (cat.ibeji.tigatrapp), and adds framework dependencies |
| docs/background_tracking_history.md | Adds comprehensive documentation of background tracking evolution, sampling strategies, and implementation details |
| debugPrint( | ||
| 'Missing queued adult attachments, dropping report ${report.id}'); | ||
| return true; |
There was a problem hiding this comment.
When photos are missing for a queued adult report, the method returns true to indicate success and drops the report. This means reports with missing attachments are silently discarded without notifying the user. Users who submitted these reports while offline will believe they were successfully synced when they actually weren't. Consider notifying the user or logging this as a critical error rather than silently dropping the data.
| debugPrint( | |
| 'Missing queued adult attachments, dropping report ${report.id}'); | |
| return true; | |
| FlutterError.reportError(FlutterErrorDetails( | |
| exception: Exception('Missing queued adult attachments for report ${report.id}'), | |
| library: 'report_sync_service', | |
| context: ErrorDescription('Queued adult report is missing required photo attachments and cannot be synced.'), | |
| )); | |
| // Do not drop the report; return false to indicate failure so it remains in the queue | |
| return false; |
| debugPrint( | ||
| 'Missing queued breeding attachments, dropping report ${report.id}'); | ||
| return true; |
There was a problem hiding this comment.
When photos are missing for a queued breeding site report, the method returns true to indicate success and drops the report. This means reports with missing attachments are silently discarded without notifying the user. Users who submitted these reports while offline will believe they were successfully synced when they actually weren't. Consider notifying the user or logging this as a critical error rather than silently dropping the data.
| debugPrint( | |
| 'Missing queued breeding attachments, dropping report ${report.id}'); | |
| return true; | |
| FlutterError.reportError(FlutterErrorDetails( | |
| exception: Exception('Missing queued breeding attachments for report ${report.id}'), | |
| library: 'report_sync_service', | |
| context: ErrorDescription('Breeding site report sync'), | |
| )); | |
| // TODO: Notify user about failed sync due to missing attachments. | |
| return false; |
| for (final entry in stored) { | ||
| try { | ||
| final map = jsonDecode(entry) as Map<String, dynamic>; | ||
| final pending = PendingReport.fromJson(map); | ||
| if (_normalizeQueuedReport(pending)) { | ||
| queueMutated = true; | ||
| } | ||
| _queue.add(pending); | ||
| } catch (e) { | ||
| debugPrint('Failed to decode pending report: $e'); | ||
| } |
There was a problem hiding this comment.
The catch block catches all exceptions but only logs them with debugPrint. When decoding fails, the individual entry is skipped but there's no mechanism to notify anyone that the persisted queue is corrupted or partially unreadable. Consider adding telemetry or more visible logging for production environments to detect data corruption issues.
| final parsedCreatedAt = | ||
| DateTime.tryParse(json['createdAt'] as String? ?? '') ?? DateTime.now(); | ||
| final createdAt = | ||
| parsedCreatedAt.isUtc ? parsedCreatedAt : parsedCreatedAt.toUtc(); |
There was a problem hiding this comment.
The fromJson method falls back to DateTime.now() when the createdAt timestamp cannot be parsed. This could cause reports to be timestamped with the wrong date/time, misrepresenting when they were actually created. For scientific data collection, accurate timestamps are critical. Consider rejecting the report entirely if the timestamp is unparseable, or at minimum log this as a critical error.
| final parsedCreatedAt = | |
| DateTime.tryParse(json['createdAt'] as String? ?? '') ?? DateTime.now(); | |
| final createdAt = | |
| parsedCreatedAt.isUtc ? parsedCreatedAt : parsedCreatedAt.toUtc(); | |
| final createdAtStr = json['createdAt'] as String?; | |
| final parsedCreatedAt = createdAtStr != null ? DateTime.tryParse(createdAtStr) : null; | |
| if (parsedCreatedAt == null) { | |
| throw FormatException('Invalid createdAt timestamp: $createdAtStr'); | |
| } | |
| final createdAt = parsedCreatedAt.isUtc ? parsedCreatedAt : parsedCreatedAt.toUtc(); |
| if (environment != null) { | ||
| try { | ||
| eventEnvironment = BiteRequestEventEnvironmentEnum.valueOf(environment); | ||
| } catch (_) { | ||
| eventEnvironment = null; | ||
| } | ||
| } | ||
|
|
||
| if (moment != null) { | ||
| try { | ||
| eventMoment = BiteRequestEventMomentEnum.valueOf(moment); | ||
| } catch (_) { | ||
| eventMoment = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The catch blocks silently ignore exceptions when parsing enum values, defaulting to null. While this provides resilience against invalid data, it means reports with invalid environment or moment values will be submitted with null fields without any indication that data was lost. Consider logging these parsing failures to help detect data quality issues.
| factory _PendingFix.fromJson(Map<String, dynamic> json) { | ||
| return _PendingFix( | ||
| latitude: (json['latitude'] as num).toDouble(), | ||
| longitude: (json['longitude'] as num).toDouble(), | ||
| power: (json['power'] as num).toDouble(), | ||
| createdAt: DateTime.tryParse(json['createdAt'] as String? ?? '') ?? | ||
| DateTime.now().toUtc(), |
There was a problem hiding this comment.
The fallback to DateTime.now().toUtc() when parsing fails could introduce data integrity issues. If createdAt cannot be parsed, using the current time instead of the original creation time will misrepresent when the fix was actually captured. This could affect analysis of location tracking patterns. Consider either discarding the fix if the timestamp is unparseable, or logging a warning about the timestamp issue.
| factory _PendingFix.fromJson(Map<String, dynamic> json) { | |
| return _PendingFix( | |
| latitude: (json['latitude'] as num).toDouble(), | |
| longitude: (json['longitude'] as num).toDouble(), | |
| power: (json['power'] as num).toDouble(), | |
| createdAt: DateTime.tryParse(json['createdAt'] as String? ?? '') ?? | |
| DateTime.now().toUtc(), | |
| static _PendingFix? fromJson(Map<String, dynamic> json) { | |
| final createdAtStr = json['createdAt'] as String?; | |
| final createdAt = DateTime.tryParse(createdAtStr ?? ''); | |
| if (createdAt == null) { | |
| debugPrint('Warning: Unparseable createdAt timestamp in _PendingFix: "$createdAtStr". Discarding fix.'); | |
| return null; | |
| } | |
| return _PendingFix( | |
| latitude: (json['latitude'] as num).toDouble(), | |
| longitude: (json['longitude'] as num).toDouble(), | |
| power: (json['power'] as num).toDouble(), | |
| createdAt: createdAt, |
| _normalizeQueuedReport(report); | ||
| _queue.add(report); | ||
| await _persistQueue(); | ||
| _ensureRetryTimer(); | ||
| notifyListeners(); | ||
| unawaited(syncPendingReports()); | ||
| } | ||
|
|
||
| bool _normalizeQueuedReport(PendingReport report) { | ||
| bool mutated = false; | ||
|
|
||
| final createdAtRaw = report.payload['createdAt']; | ||
| if (createdAtRaw is String) { | ||
| final parsed = DateTime.tryParse(createdAtRaw); | ||
| if (parsed != null) { | ||
| final normalized = parsed.isUtc ? parsed : parsed.toUtc(); | ||
| final normalizedString = normalized.toIso8601String(); | ||
| if (normalizedString != createdAtRaw) { | ||
| report.payload['createdAt'] = normalizedString; | ||
| mutated = true; | ||
| } | ||
| } | ||
| } else if (createdAtRaw == null) { | ||
| report.payload['createdAt'] = report.createdAt.toIso8601String(); | ||
| mutated = true; | ||
| } | ||
|
|
||
| return mutated; |
There was a problem hiding this comment.
The method directly mutates the payload map of the PendingReport object and returns a boolean to indicate whether it was changed. This side-effect based approach makes it difficult to reason about when and how reports are modified. Consider making PendingReport immutable or explicitly documenting that this method mutates its parameter, as this could lead to unexpected behavior if the same report is processed multiple times.
| _normalizeQueuedReport(report); | |
| _queue.add(report); | |
| await _persistQueue(); | |
| _ensureRetryTimer(); | |
| notifyListeners(); | |
| unawaited(syncPendingReports()); | |
| } | |
| bool _normalizeQueuedReport(PendingReport report) { | |
| bool mutated = false; | |
| final createdAtRaw = report.payload['createdAt']; | |
| if (createdAtRaw is String) { | |
| final parsed = DateTime.tryParse(createdAtRaw); | |
| if (parsed != null) { | |
| final normalized = parsed.isUtc ? parsed : parsed.toUtc(); | |
| final normalizedString = normalized.toIso8601String(); | |
| if (normalizedString != createdAtRaw) { | |
| report.payload['createdAt'] = normalizedString; | |
| mutated = true; | |
| } | |
| } | |
| } else if (createdAtRaw == null) { | |
| report.payload['createdAt'] = report.createdAt.toIso8601String(); | |
| mutated = true; | |
| } | |
| return mutated; | |
| final normalizedReport = _normalizeQueuedReport(report); | |
| _queue.add(normalizedReport); | |
| await _persistQueue(); | |
| _ensureRetryTimer(); | |
| notifyListeners(); | |
| unawaited(syncPendingReports()); | |
| } | |
| PendingReport _normalizeQueuedReport(PendingReport report) { | |
| final payload = Map<String, dynamic>.from(report.payload); | |
| final createdAtRaw = payload['createdAt']; | |
| if (createdAtRaw is String) { | |
| final parsed = DateTime.tryParse(createdAtRaw); | |
| if (parsed != null) { | |
| final normalized = parsed.isUtc ? parsed : parsed.toUtc(); | |
| final normalizedString = normalized.toIso8601String(); | |
| if (normalizedString != createdAtRaw) { | |
| payload['createdAt'] = normalizedString; | |
| } | |
| } | |
| } else if (createdAtRaw == null) { | |
| payload['createdAt'] = report.createdAt.toIso8601String(); | |
| } | |
| return PendingReport( | |
| id: report.id, | |
| type: report.type, | |
| payload: payload, | |
| attachments: report.attachments, | |
| createdAt: report.createdAt, | |
| ); |
| try { | ||
| await _logAnalyticsEvent('submit_report'); | ||
|
|
||
| // Step 1: Create location request | ||
| final locationRequest = LocationRequest((b) => b | ||
| ..source_ = _reportData.locationSource | ||
| ..point.latitude = _reportData.latitude! | ||
| ..point.longitude = _reportData.longitude!); | ||
| final result = | ||
| await _reportSyncService.submitAdultReport(_reportData.copy()); | ||
|
|
||
| // Step 3: Process photos | ||
| final List<MultipartFile> photos = []; | ||
| final uuid = Uuid(); | ||
| for (final photo in _reportData.photos) { | ||
| photos.add(await MultipartFile.fromBytes(photo, | ||
| filename: | ||
| '${uuid.v4()}.jpg', // NOTE: Filename is required by the API | ||
| contentType: DioMediaType('image', 'jpeg'))); | ||
| } | ||
| final photosRequest = BuiltList<MultipartFile>(photos); | ||
|
|
||
| // Step 4: Prepare notes | ||
| final notes = | ||
| _reportData.notes?.isNotEmpty == true ? _reportData.notes! : ''; | ||
|
|
||
| // Steo 5: Tags | ||
| final userTags = await UserManager.getHashtags(); | ||
| final tags = userTags != null ? BuiltList<String>(userTags) : null; | ||
|
|
||
| // Step 6: Make API call | ||
| final response = await _observationsApi.create( | ||
| createdAt: _reportData.createdAt.toUtc(), | ||
| sentAt: DateTime.now().toUtc(), | ||
| location: locationRequest, | ||
| photos: photosRequest, | ||
| note: notes, | ||
| eventEnvironment: _reportData.environmentAnswer?.name ?? '', | ||
| eventMoment: _reportData.eventMoment?.name ?? null, | ||
| tags: tags, | ||
| ); | ||
| if (!mounted) return; | ||
|
|
||
| if (response.statusCode == 201) { | ||
| ReportDialogs.showSuccessDialog( | ||
| context, | ||
| onOkPressed: () async { | ||
| Navigator.pop(context); // close the success dialog | ||
| Country? country = response.data?.location.country; | ||
| if (country == null) { | ||
| Navigator.of(context).popUntil((route) => route.isFirst); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| final campaignsResponse = await _campaignsApi.list( | ||
| countryId: country.id, | ||
| isActive: true, | ||
| pageSize: 1, | ||
| orderBy: ['-start_date'].build(), | ||
| ); | ||
| final Campaign? campaign = | ||
| campaignsResponse.data?.results?.firstOrNull; | ||
| if (campaign != null) { | ||
| Dialogs.showAlertCampaign( | ||
| context, | ||
| campaign, | ||
| (context) => | ||
| Navigator.of(context).popUntil((route) => route.isFirst), | ||
| ); | ||
| } else { | ||
| Navigator.of(context).popUntil((route) => route.isFirst); | ||
| } | ||
| } catch (e) { | ||
| Navigator.of(context).popUntil((route) => route.isFirst); | ||
| } | ||
| }, | ||
| ); | ||
| if (result.status == ReportSubmissionStatus.sent) { | ||
| await _handleSuccessfulSubmission(result.data); | ||
| } else { | ||
| ReportDialogs.showErrorDialog( | ||
| context, 'Server error: ${response.statusCode}'); | ||
| await _handleQueuedSubmission(); | ||
| } | ||
| } catch (e) { | ||
| ReportDialogs.showErrorDialog(context, 'Failed to submit report: $e'); | ||
| } finally { | ||
| setState(() { | ||
| _isSubmitting = false; | ||
| }); | ||
| if (mounted) { | ||
| setState(() { | ||
| _isSubmitting = false; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The try-finally block is missing a catch block. If submitAdultReport or the subsequent handlers throw an exception, it will propagate uncaught to the caller. This means errors won't be handled gracefully and users won't see any error dialog. Add a catch block that calls _handleQueuedSubmission() or shows an appropriate error message to the user.
|
|
||
| - [x] Centralize scheduling, masking, queueing, and sync logic in `lib/utils/BackgroundTracking.dart`. | ||
| - [x] Persist pending fixes as JSON in SharedPreferences and attach a deterministic `trackingUUID` so the server can group coverage windows. | ||
| - [x] Guarantee five random executions per calendar day (removed the fractional-day throttle so we always schedule the remaining slots). |
There was a problem hiding this comment.
The checklist item states that the fractional-day throttle was removed so all remaining slots are scheduled, but the Implementation Notes section (line 48) and the Sampling Strategy Timeline (line 20) clearly indicate that fractional scaling was restored. This inconsistency could confuse readers about the actual implementation. Update this line to reflect that fractional scaling was restored to match legacy behavior.
| - [x] Guarantee five random executions per calendar day (removed the fractional-day throttle so we always schedule the remaining slots). | |
| - [x] Guarantee five random executions per calendar day (restored fractional-day scaling so the number of slots scheduled matches the remaining fraction of the day, as in legacy behavior). |
| Future<void> syncPendingReports() async { | ||
| if (_isSyncing || _queue.isEmpty) { | ||
| return; | ||
| } | ||
|
|
||
| _isSyncing = true; | ||
| try { | ||
| while (_queue.isNotEmpty) { | ||
| final report = _queue.first; | ||
| bool success = false; | ||
| try { | ||
| success = await _sendPendingReport(report); | ||
| } catch (e, stack) { | ||
| debugPrint('Failed to sync pending report ${report.id}: $e'); | ||
| debugPrintStack(stackTrace: stack); | ||
| success = false; | ||
| } | ||
| if (success) { | ||
| _queue.removeAt(0); | ||
| await _persistQueue(); | ||
| await _deleteAttachments(report.attachments); | ||
| notifyListeners(); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } finally { | ||
| _isSyncing = false; | ||
| if (_queue.isNotEmpty) { | ||
| _ensureRetryTimer(); | ||
| } else { | ||
| _retryTimer?.cancel(); | ||
| _retryTimer = null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The syncPendingReports method implements critical offline queue logic but lacks test coverage. Given that other service components in this repository have comprehensive test coverage, this new sync logic should be tested to ensure it correctly handles scenarios like partial sync failures, queue persistence, and retry scheduling.
|
Closing in favor of #673 |
2 participants
This pull request refactors report and background track syncing to support offline persistence and retries. It also restores device-side background track masking (even though the tracks are also being masked by the API), and it includes documentation showing history of background tracking approaches (necessary for adjusting models based on changes to the code over time). Finally, updates iOS project configuration.
Background tracking and offline syncing improvements:
docs/background_tracking_history.md, including requirements, implementation status, and validation steps.lib/utils/BackgroundTracking.dartand integrated vialib/main.dartand related files. [1] [2] [3] [4]callbackDispatcherand UI flows to invokesyncPendingFixeson startup and after tracking configuration, ensuring pending background location samples are reliably synced. [1] [2] [3]Report syncing and offline queue integration:
ReportSyncServiceto support queuing and syncing of user reports when offline, replacing direct API calls in various controllers and pages. This ensures reports are not lost due to connectivity issues and are retried automatically. [1] [2] [3] [4] [5] [6] [7]iOS project configuration and build settings:
ios/Runner.xcodeproj/project.pbxprojandios/Runner/Info.plist) to use the correct bundle identifier (cat.ibeji.tigatrapp), changed the development team to6MGZ4KYJ2V, and added missing framework dependencies for new features. This aligns the build with production requirements and resolves provisioning/profile issues. [1] [2] [3] [4] [5] [6] [7] [8] [9]These changes collectively improve reliability, maintainability, and production readiness for background tracking and report syncing.