feat: Add offline mode support for ParseLive and new live query widgets #1101
Conversation
Adding Optional Index Parameter to ParseLiveListElementWidget
A RenderAnimatedSize was mutated in its own performLayout implementation.
Complete implementation of ParseLiveListPageView that matches your existing components like Live Grid and Live List
Forgot to add onPageChanged
I implemented pagination for both widgets to efficiently handle large datasets. This will enable loading data in chunks as users scroll through the content.
An option to use an AnimatedList or ListBuilder
Added CachedParseLiveList: This wrapper class manages caching for better performance. Simplified Data Flow: The implementation now uses a more direct approach where Stream.value(item) is used for smoother updates. Better State Management: All state updates are contained in setState calls with error handling. More Efficient Refresh: The refresh mechanism has been simplified to avoid animation issues. Performance Optimization: Using ListView.builder as the default implementation provides better performance.
Cached Data Access: Using CachedParseLiveList to store frequently accessed items. Pull-to-Refresh: Added RefreshIndicator for easy data refreshing. Direct Item Access: Simplified item access by using Stream.value(item) for better performance. Memory Management: Using the cache to manage memory efficiently based on the cacheSize parameter. Cache Updates: Added calls to updateCache and removeFromCache to maintain cache integrity. Error Handling: Better error reporting in debug mode.
fix lazyLoading in CachedParseLiveList
Fixing Lazy Loading in ParseLiveListPageView and ParseLiveGridWidget
Attempting to implement parse server off-line object
Updated ParseObjectOffline with some new methods
Safely cast and convert the list to List<String> before using it.
Introduces test_offline_mode.dart to verify offline caching features for Parse objects. Adds PR_CHECKLIST.md for pre-PR validation and updates README.md with new sections and examples for live query widgets and offline support.
ParseLiveSliverListWidget and ParseLiveSliverGridWidget now expose public State classes with refreshData and loadMoreData methods, accessible via GlobalKey for parent control. Documentation and README updated to reflect new usage patterns. Minor code cleanup and formatting improvements applied throughout.
|
🚀 Thanks for opening this pull request! |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds offline caching for ParseObjects, a ConnectivityHandlerMixin for online/offline widget flows, new cached/live Parse widgets (including sliver and page-view variants), and a Parse Dashboard analytics subsystem with endpoints, docs, and tests. Removes ethernet case from connectivity mapping. Changes
Sequence Diagram(s)sequenceDiagram
participant Widget as Live Widget
participant Handler as ConnectivityHandlerMixin
participant Conn as ConnectivityPlatform
participant Server as Parse Server
participant Cache as Local Cache (CoreStore)
Widget->>Handler: initConnectivityHandler()
Handler->>Conn: subscribe & checkConnectivity()
Conn-->>Handler: connectivity status (wifi/mobile/none)
alt Online
Handler->>Server: loadDataFromServer()
Server-->>Handler: objects
Handler->>Cache: _saveBatchToCache(objects)
Handler->>Widget: render server data
else Offline (offlineMode)
Handler->>Cache: loadDataFromCache()
Cache-->>Handler: cached objects
Handler->>Widget: render cached data
end
Conn->>Handler: connectivity change (lost)
Handler->>Widget: disposeLiveList()
Handler->>Cache: loadDataFromCache()
Handler->>Widget: render offline
Conn->>Handler: connectivity restored
Handler->>Server: loadDataFromServer()
Server-->>Handler: fresh objects
Handler->>Cache: syncLocalCacheWithServer / update cache
Handler->>Widget: update UI
Server->>Handler: live update (add/update/delete)
Handler->>Cache: persist change (if offline or to keep cache fresh)
Handler->>Widget: incremental update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive offline mode support and introduces three new live query widgets for the Parse Flutter SDK. The implementation includes local caching via ParseObjectOffline extension methods, automatic connectivity detection, and batch save operations for efficient caching.
Key Changes
- Three new live query widgets: ParseLiveSliverListWidget, ParseLiveSliverGridWidget, and ParseLiveListPageView
- Offline mode support with local caching and automatic connectivity fallback
- Connectivity handler mixin for consistent offline handling across widgets
- LRU memory management via CachedParseLiveList
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
packages/dart/lib/src/objects/parse_offline_object.dart |
New extension providing offline caching methods for ParseObject |
packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart |
Mixin for handling connectivity state and data loading transitions |
packages/flutter/lib/src/utils/parse_cached_live_list.dart |
LRU cache wrapper for ParseLiveList |
packages/flutter/lib/src/utils/parse_live_sliver_list.dart |
New sliver list widget with offline support |
packages/flutter/lib/src/utils/parse_live_sliver_grid.dart |
New sliver grid widget with offline support |
packages/flutter/lib/src/utils/parse_live_page_view.dart |
New PageView widget with offline support |
packages/flutter/lib/src/utils/parse_live_list.dart |
Updated with offline mode and pagination support |
packages/flutter/lib/src/utils/parse_live_grid.dart |
Updated with offline mode and pagination support |
packages/flutter/lib/src/analytics/* |
Unrelated analytics integration (should be separate PR) |
packages/flutter/pubspec.yaml |
Configuration changes including git dependency and publish_to: none |
packages/flutter/README.md |
Comprehensive documentation for new features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| repository: https://github.com/parse-community/Parse-SDK-Flutter | ||
| issue_tracker: https://github.com/parse-community/Parse-SDK-Flutter/issues | ||
| documentation: https://docs.parseplatform.org/dart/guide | ||
| publish_to: none |
There was a problem hiding this comment.
publish_to: none should be removed before merging. This prevents the dart package from being published to pub.dev.
| publish_to: none |
| import 'dart:async'; | ||
| import 'dart:convert'; | ||
| import 'package:flutter/foundation.dart'; | ||
| import 'package:parse_server_sdk/parse_server_sdk.dart'; | ||
|
|
||
| /// Analytics collection utility for Parse Dashboard integration | ||
| /// | ||
| /// This class provides methods to collect user, installation, and event data | ||
| /// that can be fed to Parse Dashboard analytics endpoints. | ||
| class ParseAnalytics { | ||
| static StreamController<Map<String, dynamic>>? _eventController; | ||
| static const String _eventsKey = 'parse_analytics_events'; | ||
|
|
||
| /// Initialize the analytics system | ||
| static Future<void> initialize() async { | ||
| _eventController ??= StreamController<Map<String, dynamic>>.broadcast(); | ||
| } | ||
|
|
||
| /// Get comprehensive user analytics for Parse Dashboard | ||
| static Future<Map<String, dynamic>> getUserAnalytics() async { | ||
| try { | ||
| final now = DateTime.now(); | ||
| final yesterday = now.subtract(const Duration(days: 1)); | ||
| final weekAgo = now.subtract(const Duration(days: 7)); | ||
| final monthAgo = now.subtract(const Duration(days: 30)); | ||
|
|
||
| // Get user count queries using QueryBuilder | ||
| final totalUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()); | ||
| final totalUsersResult = await totalUsersQuery.count(); | ||
| final totalUsers = totalUsersResult.count; | ||
|
|
||
| final activeUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', weekAgo); | ||
| final activeUsersResult = await activeUsersQuery.count(); | ||
| final activeUsers = activeUsersResult.count; | ||
|
|
||
| final dailyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', yesterday); | ||
| final dailyUsersResult = await dailyUsersQuery.count(); | ||
| final dailyUsers = dailyUsersResult.count; | ||
|
|
||
| final weeklyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', weekAgo); | ||
| final weeklyUsersResult = await weeklyUsersQuery.count(); | ||
| final weeklyUsers = weeklyUsersResult.count; | ||
|
|
||
| final monthlyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', monthAgo); | ||
| final monthlyUsersResult = await monthlyUsersQuery.count(); | ||
| final monthlyUsers = monthlyUsersResult.count; | ||
|
|
||
| return { | ||
| 'timestamp': now.millisecondsSinceEpoch, | ||
| 'total_users': totalUsers, | ||
| 'active_users': activeUsers, | ||
| 'daily_users': dailyUsers, | ||
| 'weekly_users': weeklyUsers, | ||
| 'monthly_users': monthlyUsers, | ||
| }; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error getting user analytics: $e'); | ||
| } | ||
| return { | ||
| 'timestamp': DateTime.now().millisecondsSinceEpoch, | ||
| 'total_users': 0, | ||
| 'active_users': 0, | ||
| 'daily_users': 0, | ||
| 'weekly_users': 0, | ||
| 'monthly_users': 0, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /// Get installation analytics for Parse Dashboard | ||
| static Future<Map<String, dynamic>> getInstallationAnalytics() async { | ||
| try { | ||
| final now = DateTime.now(); | ||
| final yesterday = now.subtract(const Duration(days: 1)); | ||
| final weekAgo = now.subtract(const Duration(days: 7)); | ||
| final monthAgo = now.subtract(const Duration(days: 30)); | ||
|
|
||
| // Get installation count queries | ||
| final totalInstallationsQuery = QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()); | ||
| final totalInstallationsResult = await totalInstallationsQuery.count(); | ||
| final totalInstallations = totalInstallationsResult.count; | ||
|
|
||
| final activeInstallationsQuery = QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | ||
| ..whereGreaterThan('updatedAt', weekAgo); | ||
| final activeInstallationsResult = await activeInstallationsQuery.count(); | ||
| final activeInstallations = activeInstallationsResult.count; | ||
|
|
||
| final dailyInstallationsQuery = QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | ||
| ..whereGreaterThan('updatedAt', yesterday); | ||
| final dailyInstallationsResult = await dailyInstallationsQuery.count(); | ||
| final dailyInstallations = dailyInstallationsResult.count; | ||
|
|
||
| final weeklyInstallationsQuery = QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | ||
| ..whereGreaterThan('updatedAt', weekAgo); | ||
| final weeklyInstallationsResult = await weeklyInstallationsQuery.count(); | ||
| final weeklyInstallations = weeklyInstallationsResult.count; | ||
|
|
||
| final monthlyInstallationsQuery = QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | ||
| ..whereGreaterThan('updatedAt', monthAgo); | ||
| final monthlyInstallationsResult = await monthlyInstallationsQuery.count(); | ||
| final monthlyInstallations = monthlyInstallationsResult.count; | ||
|
|
||
| return { | ||
| 'timestamp': now.millisecondsSinceEpoch, | ||
| 'total_installations': totalInstallations, | ||
| 'active_installations': activeInstallations, | ||
| 'daily_installations': dailyInstallations, | ||
| 'weekly_installations': weeklyInstallations, | ||
| 'monthly_installations': monthlyInstallations, | ||
| }; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error getting installation analytics: $e'); | ||
| } | ||
| return { | ||
| 'timestamp': DateTime.now().millisecondsSinceEpoch, | ||
| 'total_installations': 0, | ||
| 'active_installations': 0, | ||
| 'daily_installations': 0, | ||
| 'weekly_installations': 0, | ||
| 'monthly_installations': 0, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /// Track custom events for analytics | ||
| static Future<void> trackEvent(String eventName, [Map<String, dynamic>? parameters]) async { | ||
| try { | ||
| await initialize(); | ||
|
|
||
| final currentUser = await ParseUser.currentUser(); | ||
| final currentInstallation = await ParseInstallation.currentInstallation(); | ||
|
|
||
| final event = { | ||
| 'event_name': eventName, | ||
| 'parameters': parameters ?? {}, | ||
| 'timestamp': DateTime.now().millisecondsSinceEpoch, | ||
| 'user_id': currentUser?.objectId, | ||
| 'installation_id': currentInstallation.objectId, | ||
| }; | ||
|
|
||
| // Add to stream for real-time tracking | ||
| _eventController?.add(event); | ||
|
|
||
| // Store locally for later upload | ||
| await _storeEventLocally(event); | ||
|
|
||
| if (kDebugMode) { | ||
| print('Analytics event tracked: $eventName'); | ||
| } | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error tracking event: $e'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Get time series data for Parse Dashboard charts | ||
| static Future<List<List<num>>> getTimeSeriesData({ | ||
| required String metric, | ||
| required DateTime startDate, | ||
| required DateTime endDate, | ||
| String interval = 'day', | ||
| }) async { | ||
| try { | ||
| final data = <List<num>>[]; | ||
| final intervalDuration = interval == 'hour' | ||
| ? const Duration(hours: 1) | ||
| : const Duration(days: 1); | ||
|
|
||
| DateTime current = startDate; | ||
| while (current.isBefore(endDate)) { | ||
| final next = current.add(intervalDuration); | ||
| int value = 0; | ||
|
|
||
| switch (metric) { | ||
| case 'users': | ||
| final query = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', current) | ||
| ..whereLessThan('updatedAt', next); | ||
| final result = await query.count(); | ||
| value = result.count; | ||
| break; | ||
|
|
||
| case 'installations': | ||
| final query = QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | ||
| ..whereGreaterThan('updatedAt', current) | ||
| ..whereLessThan('updatedAt', next); | ||
| final result = await query.count(); | ||
| value = result.count; | ||
| break; | ||
| } | ||
|
|
||
| data.add([current.millisecondsSinceEpoch, value]); | ||
| current = next; | ||
| } | ||
|
|
||
| return data; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error getting time series data: $e'); | ||
| } | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /// Calculate user retention metrics | ||
| static Future<Map<String, double>> getUserRetention({DateTime? cohortDate}) async { | ||
| try { | ||
| final cohort = cohortDate ?? DateTime.now().subtract(const Duration(days: 30)); | ||
| final cohortEnd = cohort.add(const Duration(days: 1)); | ||
|
|
||
| // Get users who signed up in the cohort period | ||
| final cohortQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('createdAt', cohort) | ||
| ..whereLessThan('createdAt', cohortEnd); | ||
|
|
||
| final cohortUsers = await cohortQuery.find(); | ||
| if (cohortUsers.isEmpty) { | ||
| return {'day1': 0.0, 'day7': 0.0, 'day30': 0.0}; | ||
| } | ||
|
|
||
| final cohortUserIds = cohortUsers.map((user) => user.objectId!).toList(); | ||
|
|
||
| // Calculate retention | ||
| final day1Retention = await _calculateRetention(cohortUserIds, cohort, 1); | ||
| final day7Retention = await _calculateRetention(cohortUserIds, cohort, 7); | ||
| final day30Retention = await _calculateRetention(cohortUserIds, cohort, 30); | ||
|
|
||
| return { | ||
| 'day1': day1Retention, | ||
| 'day7': day7Retention, | ||
| 'day30': day30Retention, | ||
| }; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error calculating user retention: $e'); | ||
| } | ||
| return {'day1': 0.0, 'day7': 0.0, 'day30': 0.0}; | ||
| } | ||
| } | ||
|
|
||
| /// Get stream of real-time analytics events | ||
| static Stream<Map<String, dynamic>>? get eventsStream => _eventController?.stream; | ||
|
|
||
| /// Store event locally for offline support | ||
| static Future<void> _storeEventLocally(Map<String, dynamic> event) async { | ||
| try { | ||
| final coreStore = ParseCoreData().getStore(); | ||
| final existingEvents = await coreStore.getStringList(_eventsKey) ?? []; | ||
|
|
||
| existingEvents.add(jsonEncode(event)); | ||
|
|
||
| // Keep only last 1000 events | ||
| if (existingEvents.length > 1000) { | ||
| existingEvents.removeRange(0, existingEvents.length - 1000); | ||
| } | ||
|
|
||
| await coreStore.setStringList(_eventsKey, existingEvents); | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error storing event locally: $e'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Get locally stored events | ||
| static Future<List<Map<String, dynamic>>> getStoredEvents() async { | ||
| try { | ||
| final coreStore = ParseCoreData().getStore(); | ||
| final eventStrings = await coreStore.getStringList(_eventsKey) ?? []; | ||
|
|
||
| return eventStrings.map((eventString) { | ||
| try { | ||
| return jsonDecode(eventString) as Map<String, dynamic>; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error parsing stored event: $e'); | ||
| } | ||
| return <String, dynamic>{}; | ||
| } | ||
| }).where((event) => event.isNotEmpty).toList(); | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error getting stored events: $e'); | ||
| } | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /// Clear locally stored events | ||
| static Future<void> clearStoredEvents() async { | ||
| try { | ||
| final coreStore = ParseCoreData().getStore(); | ||
| await coreStore.remove(_eventsKey); | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error clearing stored events: $e'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Calculate retention for a specific period | ||
| static Future<double> _calculateRetention(List<String> cohortUserIds, DateTime cohortStart, int days) async { | ||
| try { | ||
| if (cohortUserIds.isEmpty) return 0.0; | ||
|
|
||
| final retentionDate = cohortStart.add(Duration(days: days)); | ||
| final retentionEnd = retentionDate.add(const Duration(days: 1)); | ||
|
|
||
| final retentionQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereContainedIn('objectId', cohortUserIds) | ||
| ..whereGreaterThan('updatedAt', retentionDate) | ||
| ..whereLessThan('updatedAt', retentionEnd); | ||
|
|
||
| final activeUsers = await retentionQuery.find(); | ||
|
|
||
| return activeUsers.length / cohortUserIds.length; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error calculating retention for day $days: $e'); | ||
| } | ||
| return 0.0; | ||
| } | ||
| } | ||
|
|
||
| /// Dispose resources | ||
| static void dispose() { | ||
| _eventController?.close(); | ||
| _eventController = null; | ||
| } | ||
| } | ||
|
|
||
| /// Event model for analytics | ||
| class AnalyticsEventData { | ||
| final String eventName; | ||
| final Map<String, dynamic> parameters; | ||
| final DateTime timestamp; | ||
| final String? userId; | ||
| final String? installationId; | ||
|
|
||
| AnalyticsEventData({ | ||
| required this.eventName, | ||
| this.parameters = const {}, | ||
| DateTime? timestamp, | ||
| this.userId, | ||
| this.installationId, | ||
| }) : timestamp = timestamp ?? DateTime.now(); | ||
|
|
||
| Map<String, dynamic> toJson() => { | ||
| 'event_name': eventName, | ||
| 'parameters': parameters, | ||
| 'timestamp': timestamp.millisecondsSinceEpoch, | ||
| 'user_id': userId, | ||
| 'installation_id': installationId, | ||
| }; | ||
|
|
||
| factory AnalyticsEventData.fromJson(Map<String, dynamic> json) => AnalyticsEventData( | ||
| eventName: json['event_name'] as String, | ||
| parameters: Map<String, dynamic>.from(json['parameters'] ?? {}), | ||
| timestamp: DateTime.fromMillisecondsSinceEpoch(json['timestamp'] as int), | ||
| userId: json['user_id'] as String?, | ||
| installationId: json['installation_id'] as String?, | ||
| ); | ||
| } | ||
|
|
||
|
|
||
|
|
||
| /* // Initialize analytics | ||
| await ParseAnalytics.initialize(); | ||
| // Track events | ||
| await ParseAnalytics.trackEvent('app_opened'); | ||
| await ParseAnalytics.trackEvent('purchase', {'amount': 9.99}); | ||
| // Get analytics data | ||
| final userStats = await ParseAnalytics.getUserAnalytics(); | ||
| final retention = await ParseAnalytics.getUserRetention(); | ||
| // Real-time event streaming | ||
| ParseAnalytics.eventsStream?.listen((event) { | ||
| print('New event: ${event['event_name']}'); | ||
| });*/ No newline at end of file |
There was a problem hiding this comment.
This entire analytics implementation appears unrelated to the PR's stated purpose of "Add offline mode support for ParseLive and new live query widgets". The PR description doesn't mention analytics features. Consider:
- Moving analytics to a separate PR for focused review
- Updating the PR description if analytics is intentionally included
- Explaining the relationship between analytics and offline mode if there is one
| import 'package:parse_server_sdk/parse_server_sdk.dart'; | ||
|
|
||
| void main() async { | ||
| // Initialize Parse | ||
| await Parse().initialize( | ||
| 'test_app_id', | ||
| 'https://test.com', | ||
| clientKey: 'test_client_key', | ||
| debug: false, | ||
| ); | ||
|
|
||
| print('=== Testing Offline Mode Functionality ===\n'); | ||
|
|
||
| // Test 1: Save single object to cache | ||
| print('Test 1: Save single object to cache'); | ||
| final testObject = ParseObject('TestClass'); | ||
| testObject.set('name', 'Test Object'); | ||
| testObject.objectId = 'test-id-1'; | ||
| await testObject.saveToLocalCache(); | ||
| print('✅ Single object saved to cache\n'); | ||
|
|
||
| // Test 2: Load single object from cache | ||
| print('Test 2: Load single object from cache'); | ||
| final loadedObject = await ParseObjectOffline.loadFromLocalCache( | ||
| 'TestClass', | ||
| 'test-id-1', | ||
| ); | ||
| if (loadedObject != null && | ||
| loadedObject.get<String>('name') == 'Test Object') { | ||
| print('✅ Single object loaded from cache successfully\n'); | ||
| } else { | ||
| print('❌ Failed to load object from cache\n'); | ||
| } | ||
|
|
||
| // Test 3: Save multiple objects efficiently | ||
| print('Test 3: Save multiple objects to cache'); | ||
| final objectsToSave = <ParseObject>[]; | ||
| for (int i = 1; i <= 5; i++) { | ||
| final obj = ParseObject('TestClass'); | ||
| obj.set('name', 'Object $i'); | ||
| obj.objectId = 'test-id-$i'; | ||
| objectsToSave.add(obj); | ||
| } | ||
| await ParseObjectOffline.saveAllToLocalCache('TestClass', objectsToSave); | ||
| print('✅ Multiple objects saved to cache\n'); | ||
|
|
||
| // Test 4: Load all objects from cache | ||
| print('Test 4: Load all objects from cache'); | ||
| final allCached = await ParseObjectOffline.loadAllFromLocalCache('TestClass'); | ||
| print('✅ Loaded ${allCached.length} objects from cache\n'); | ||
|
|
||
| // Test 5: Check if object exists in cache | ||
| print('Test 5: Check if object exists in cache'); | ||
| final exists = await ParseObjectOffline.existsInLocalCache( | ||
| 'TestClass', | ||
| 'test-id-1', | ||
| ); | ||
| if (exists) { | ||
| print('✅ Object existence check passed\n'); | ||
| } else { | ||
| print('❌ Object existence check failed\n'); | ||
| } | ||
|
|
||
| // Test 6: Update object in cache | ||
| print('Test 6: Update object in cache'); | ||
| await testObject.updateInLocalCache({'name': 'Updated Object'}); | ||
| final updatedObject = await ParseObjectOffline.loadFromLocalCache( | ||
| 'TestClass', | ||
| 'test-id-1', | ||
| ); | ||
| if (updatedObject?.get<String>('name') == 'Updated Object') { | ||
| print('✅ Object updated in cache successfully\n'); | ||
| } | ||
|
|
||
| // Test 7: Get all object IDs | ||
| print('Test 7: Get all object IDs from cache'); | ||
| final objectIds = await ParseObjectOffline.getAllObjectIdsInLocalCache( | ||
| 'TestClass', | ||
| ); | ||
| print('✅ Retrieved ${objectIds.length} object IDs from cache\n'); | ||
|
|
||
| // Test 8: Remove object from cache | ||
| print('Test 8: Remove object from cache'); | ||
| await testObject.removeFromLocalCache(); | ||
| final removedCheck = await ParseObjectOffline.existsInLocalCache( | ||
| 'TestClass', | ||
| 'test-id-1', | ||
| ); | ||
| if (!removedCheck) { | ||
| print('✅ Object removed from cache successfully\n'); | ||
| } | ||
|
|
||
| // Test 9: Clear all objects for a class | ||
| print('Test 9: Clear all objects for a class'); | ||
| await ParseObjectOffline.clearLocalCacheForClass('TestClass'); | ||
| final clearedObjects = await ParseObjectOffline.loadAllFromLocalCache( | ||
| 'TestClass', | ||
| ); | ||
| if (clearedObjects.isEmpty) { | ||
| print('✅ Cache cleared successfully\n'); | ||
| } | ||
|
|
||
| print('=== All Offline Mode Tests Completed Successfully! ==='); | ||
| } |
There was a problem hiding this comment.
Test files should be in the test/ directory, not in the package root. These manual test scripts (test_offline_mode.dart, test_offline.dart) should either be:
- Moved to proper test directories with proper test framework usage
- Moved to an
example/directory if they're meant as examples - Removed before merging if they're only for local development
| if (!item.containsKey(sdk.keyVarUpdatedAt)) { | ||
| fetchFutures.add(item.fetch().then((_) { |
There was a problem hiding this comment.
The lazy loading check uses !item.containsKey(sdk.keyVarUpdatedAt) which may not accurately determine if an item needs fetching. An item could have an updatedAt field but still be incomplete if lazy loading is enabled. Consider checking a more reliable indicator or using a dedicated flag to track whether the full object has been loaded.
| if (!item.containsKey(sdk.keyVarUpdatedAt)) { | |
| fetchFutures.add(item.fetch().then((_) { | |
| if (item['isFullyLoaded'] != true) { | |
| fetchFutures.add(item.fetch().then((_) { | |
| item['isFullyLoaded'] = true; |
| // Analytics Overview - Audience Metrics | ||
| app.get('/apps/:appSlug/analytics_content_audience', async (req, res) => { | ||
| try { | ||
| const { audienceType, at } = req.query; |
There was a problem hiding this comment.
Unused variable at.
| const { audienceType, at } = req.query; | |
| const { audienceType } = req.query; |
| // Slow Queries Endpoint | ||
| app.get('/apps/:appSlug/slow_queries', async (req, res) => { | ||
| try { | ||
| const { className, os, version, from, to } = req.query; |
There was a problem hiding this comment.
Unused variable from.
| const { className, os, version, from, to } = req.query; | |
| const { className, os, version } = req.query; |
| // Slow Queries Endpoint | ||
| app.get('/apps/:appSlug/slow_queries', async (req, res) => { | ||
| try { | ||
| const { className, os, version, from, to } = req.query; |
There was a problem hiding this comment.
Unused variable to.
| const { className, os, version, from, to } = req.query; | |
| const { className, os, version } = req.query; |
|
|
||
| // Helper function to generate time series data | ||
| async function generateTimeSeriesData(options) { | ||
| const { endpoint, audienceType, stride, from, to } = options; |
There was a problem hiding this comment.
Unused variable audienceType.
| const { endpoint, audienceType, stride, from, to } = options; | |
| const { endpoint, stride, from, to } = options; |
| const { audienceType, at } = req.query; | ||
| console.log(`Analytics audience request: ${audienceType}`); | ||
|
|
||
| let result = { total: 0, content: 0 }; |
There was a problem hiding this comment.
The initial value of result is unused, since it is always overwritten.
| let result = { total: 0, content: 0 }; | |
| let result; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
packages/flutter/lib/src/analytics/parse_analytics.dart-136-145 (1)
136-145: Potential PII leakage: user_id and installation_id stored in events.The event payload includes
user_idandinstallation_id. Depending on jurisdiction (GDPR/CCPA), storing user identifiers in analytics events may require user consent. Consider documenting this or providing an opt-out mechanism.At minimum, add documentation warning about PII:
/// Track custom events for analytics /// /// Note: Events include user_id and installation_id for attribution. /// Ensure appropriate user consent is obtained per applicable privacy regulations. static Future<void> trackEvent(String eventName, [Map<String, dynamic>? parameters]) async {packages/flutter/lib/src/analytics/parse_analytics.dart-163-210 (1)
163-210: Unbounded loop ingetTimeSeriesDatacould cause performance issues.If
startDateandendDatespan a long period with hourly intervals, this loop could execute thousands of iterations, each making a network request. Consider adding a limit or warning.static Future<List<List<num>>> getTimeSeriesData({ required String metric, required DateTime startDate, required DateTime endDate, String interval = 'day', }) async { try { + // Guard against excessive iterations + final maxIterations = interval == 'hour' ? 168 : 365; // 1 week hourly or 1 year daily + final estimatedIterations = endDate.difference(startDate).inHours ~/ + (interval == 'hour' ? 1 : 24); + if (estimatedIterations > maxIterations) { + if (kDebugMode) { + print('Warning: Time range too large ($estimatedIterations intervals). Consider a larger interval.'); + } + } + final data = <List<num>>[];packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart-40-51 (1)
40-51: Subscription callback does not await async handler, potentially causing race conditions.The
listencallback invokes_internalUpdateConnectionStatus(which is async) but doesn't await it. Multiple rapid connectivity changes could trigger overlapping async state updates, leading to inconsistent state.Consider using a queue or mutex pattern, or at minimum track pending operations:
_connectivitySubscription = _connectivity.onConnectivityChanged.listen((List<ConnectivityResult> results) { final newResult = results.contains(ConnectivityResult.mobile) ? ConnectivityResult.mobile : results.contains(ConnectivityResult.wifi) ? ConnectivityResult.wifi : results.contains(ConnectivityResult.none) ? ConnectivityResult.none : ConnectivityResult.other; - _internalUpdateConnectionStatus(newResult); + // Fire and forget, but consider debouncing for rapid changes + _internalUpdateConnectionStatus(newResult); });Alternatively, implement debouncing or a serial queue to handle rapid connectivity changes gracefully.
Committable suggestion skipped: line range outside the PR's diff.
packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart-42-48 (1)
42-48: Consider handlingConnectivityResult.ethernetandConnectivityResult.vpn.The current logic maps ethernet, VPN, and bluetooth connections to
ConnectivityResult.other, which may not be treated as "online" by the downstream logic (line 93). Users on ethernet or VPN would be incorrectly considered offline.final newResult = results.contains(ConnectivityResult.mobile) ? ConnectivityResult.mobile : results.contains(ConnectivityResult.wifi) ? ConnectivityResult.wifi + : results.contains(ConnectivityResult.ethernet) + ? ConnectivityResult.ethernet + : results.contains(ConnectivityResult.vpn) + ? ConnectivityResult.vpn - : results.contains(ConnectivityResult.none) - ? ConnectivityResult.none - : ConnectivityResult.other; + : results.contains(ConnectivityResult.none) + ? ConnectivityResult.none + : ConnectivityResult.other;And update the
isOnlinecheck at line 93:-bool isOnline = result == ConnectivityResult.mobile || result == ConnectivityResult.wifi; +bool isOnline = result == ConnectivityResult.mobile || + result == ConnectivityResult.wifi || + result == ConnectivityResult.ethernet || + result == ConnectivityResult.vpn;packages/dart/lib/src/objects/parse_offline_object.dart-1-165 (1)
1-165: Replace print statements with proper logging.The extension uses
print()statements throughout, which is not appropriate for library code. Consider using the Parse SDK's logger (ParseLogger) for consistent logging that respects the user's debug settings.Example:
ParseCoreData().logger.log( ParseLogLevel.info, 'Loaded object $objectId from local cache for $className' );This would:
- Respect the debug flag set during Parse initialization
- Provide consistent logging across the SDK
- Allow filtering by log level
- Avoid polluting production console output
packages/dart/lib/src/storage/core_store_sem_impl.dart-106-110 (1)
106-110: Remove commented-out code.The old implementation should be removed to keep the codebase clean.
-// @override -// Future<List<String>?> getStringList(String key) async { -// final List<String>? storedItem = await get(key); -// return storedItem; -// }packages/dart/lib/src/storage/core_store_memory.dart-50-53 (1)
50-53: Remove commented-out code.The old implementation should be removed for code cleanliness.
- // @override - // Future<List<String>?> getStringList(String key) async { - // return _data[key]; - // }packages/dart/lib/src/objects/parse_offline_object.dart-95-104 (1)
95-104: Add error handling for loading all cached objects.The loadAllFromLocalCache method should handle JSON decode failures gracefully to avoid losing all cached data due to a single corrupted entry.
static Future<List<ParseObject>> loadAllFromLocalCache(String className) async { final CoreStore coreStore = ParseCoreData().getStore(); final String cacheKey = 'offline_cache_$className'; final List<String> cached = await _getStringListAsStrings(coreStore, cacheKey); print('Loaded ${cached.length} objects from local cache for $className'); - return cached.map<ParseObject>((s) { - final jsonObj = json.decode(s); - return ParseObject(className).fromJson(jsonObj); - }).toList(); + final List<ParseObject> objects = []; + for (final s in cached) { + try { + final jsonObj = json.decode(s); + objects.add(ParseObject(className).fromJson(jsonObj)); + } catch (e) { + print('Error decoding cached object for $className: $e'); + // Skip corrupted entries but continue processing + } + } + return objects; }packages/dart/lib/src/objects/parse_offline_object.dart-106-120 (1)
106-120: Add error handling for cache update operation.The updateInLocalCache method should handle JSON operations safely.
Future<void> updateInLocalCache(Map<String, dynamic> updates) async { + try { final CoreStore coreStore = ParseCoreData().getStore(); final String cacheKey = 'offline_cache_$parseClassName'; final List<String> cached = await _getStringListAsStrings(coreStore, cacheKey); for (int i = 0; i < cached.length; i++) { - final jsonObj = json.decode(cached[i]); - if (jsonObj['objectId'] == objectId) { - jsonObj.addAll(updates); - cached[i] = json.encode(jsonObj); - break; + try { + final jsonObj = json.decode(cached[i]); + if (jsonObj['objectId'] == objectId) { + jsonObj.addAll(updates); + cached[i] = json.encode(jsonObj); + break; + } + } catch (e) { + print('Error decoding cached object during update: $e'); + continue; } } await coreStore.setStringList(cacheKey, cached); print('Updated object ${objectId ?? "(no objectId)"} in local cache for $parseClassName'); + } catch (e) { + print('Error updating object in local cache for $parseClassName: $e'); + rethrow; + } }packages/dart/lib/src/objects/parse_offline_object.dart-5-17 (1)
5-17: Add error handling and consider performance optimization.This method has two concerns:
- Missing error handling: JSON decoding can fail if the cached data is corrupted, which would crash the method.
- Performance: Loading the entire cache to find a single object is inefficient for large datasets.
Apply this diff to add error handling:
static Future<ParseObject?> loadFromLocalCache(String className, String objectId) async { final CoreStore coreStore = ParseCoreData().getStore(); final String cacheKey = 'offline_cache_$className'; final List<String> cached = await _getStringListAsStrings(coreStore, cacheKey); for (final s in cached) { - final jsonObj = json.decode(s); - if (jsonObj['objectId'] == objectId) { - print('Loaded object $objectId from local cache for $className'); - return ParseObject(className).fromJson(jsonObj); + try { + final jsonObj = json.decode(s); + if (jsonObj['objectId'] == objectId) { + print('Loaded object $objectId from local cache for $className'); + return ParseObject(className).fromJson(jsonObj); + } + } catch (e) { + print('Error decoding cached object for $className: $e'); + continue; } } return null; }Consider using a more efficient storage structure (e.g., separate key per object:
offline_cache_$className\_$objectId) to avoid loading all objects.packages/dart/lib/src/objects/parse_offline_object.dart-82-92 (1)
82-92: Add error handling for cache removal operation.The removeFromLocalCache method needs error handling to prevent crashes and ensure data integrity.
Future<void> removeFromLocalCache() async { + try { final CoreStore coreStore = ParseCoreData().getStore(); final String cacheKey = 'offline_cache_$parseClassName'; final List<String> cached = await _getStringListAsStrings(coreStore, cacheKey); cached.removeWhere((s) { - final jsonObj = json.decode(s); - return jsonObj['objectId'] == objectId; + try { + final jsonObj = json.decode(s); + return jsonObj['objectId'] == objectId; + } catch (e) { + print('Error decoding cached object during removal: $e'); + return false; + } }); await coreStore.setStringList(cacheKey, cached); print('Removed object ${objectId ?? "(no objectId)"} from local cache for $parseClassName'); + } catch (e) { + print('Error removing object from local cache for $parseClassName: $e'); + rethrow; + } }packages/dart/lib/src/objects/parse_offline_object.dart-152-158 (1)
152-158: Add error handling and progress tracking for sync operation.The syncLocalCacheWithServer method should handle failures gracefully and provide progress feedback for large datasets.
static Future<void> syncLocalCacheWithServer(String className) async { - final objects = await loadAllFromLocalCache(className); - for (final obj in objects) { - await obj.save(); + try { + final objects = await loadAllFromLocalCache(className); + int synced = 0; + int failed = 0; + for (final obj in objects) { + try { + await obj.save(); + synced++; + } catch (e) { + print('Error syncing object ${obj.objectId} for $className: $e'); + failed++; + } + } + print('Synced local cache with server for $className. Success: $synced, Failed: $failed'); + } catch (e) { + print('Error syncing local cache with server for $className: $e'); + rethrow; } - print('Synced local cache with server for $className'); }packages/dart/lib/src/objects/parse_offline_object.dart-20-32 (1)
20-32: Add error handling and consider concurrency control.This method needs protection against:
- JSON encoding/decoding failures: Can throw if data contains non-serializable objects
- Race conditions: Multiple concurrent saves could corrupt the cache
Add error handling:
Future<void> saveToLocalCache() async { + try { final CoreStore coreStore = ParseCoreData().getStore(); final String cacheKey = 'offline_cache_$parseClassName'; final List<String> cached = await _getStringListAsStrings(coreStore, cacheKey); // Remove any existing object with the same objectId cached.removeWhere((s) { - final jsonObj = json.decode(s); - return jsonObj['objectId'] == objectId; + try { + final jsonObj = json.decode(s); + return jsonObj['objectId'] == objectId; + } catch (e) { + print('Error decoding cached object during save: $e'); + return false; + } }); cached.add(json.encode(toJson(full: true))); await coreStore.setStringList(cacheKey, cached); print('Saved object ${objectId ?? "(no objectId)"} to local cache for $parseClassName'); + } catch (e) { + print('Error saving object to local cache for $parseClassName: $e'); + rethrow; + } }packages/flutter/lib/src/utils/parse_live_sliver_list.dart-264-289 (1)
264-289: Type safety issue: Stream events not converted viafromJson.Same issue as in
ParseLiveSliverGridWidget: stream event objects are directly inserted withoutfromJsonconversion, but cache-loaded objects usefromJson. Apply the same fix as suggested for the grid widget.packages/flutter/lib/src/utils/parse_live_page_view.dart-241-266 (1)
241-266: Type safety issue: Stream events not converted viafromJson.Same issue as in the sliver widgets: stream event objects are directly inserted without
fromJsonconversion. Apply the same fix pattern.packages/flutter/lib/src/utils/parse_live_page_view.dart-600-614 (1)
600-614: Potential listener leak when external PageController is provided.The listener
_checkForMoreDatais added whenwidget.paginationis true (line 104), but only removed whenwidget.pagination && widget.pageController == null(line 606-608). If a user provides their ownpageControllerwith pagination enabled, the listener is never removed.@override void dispose() { disposeConnectivityHandler(); disposeLiveList(); _noDataNotifier.dispose(); - // Remove listener only if we added it - if (widget.pagination && widget.pageController == null) { - _pageController.removeListener(_checkForMoreData); - } + // Remove listener if pagination is enabled (we always add it in that case) + if (widget.pagination) { + _pageController.removeListener(_checkForMoreData); + } // Dispose controller only if we created it if (widget.pageController == null) { _pageController.dispose(); } super.dispose(); }packages/flutter/lib/src/analytics/parse_analytics_endpoints.dart-230-276 (1)
230-276: Security: Hardcoded placeholder master key in generated code.The generated Dart Shelf handler contains
if (masterKey != 'your_master_key')which is a dangerous placeholder. Developers may copy this without changing it. Consider:
- Using an environment variable reference:
Platform.environment['PARSE_MASTER_KEY']- Adding a clear
// TODO: CHANGE THIScomment- Throwing an error if the key appears to be the placeholder
- if (masterKey != 'your_master_key') { + // SECURITY: Replace with your actual master key or use environment variable + final expectedKey = Platform.environment['PARSE_MASTER_KEY']; + if (expectedKey == null || masterKey != expectedKey) { return Response.forbidden(json.encode({'error': 'Unauthorized'})); }Committable suggestion skipped: line range outside the PR's diff.
packages/flutter/lib/src/utils/parse_live_sliver_grid.dart-354-375 (1)
354-375: Type safety issue: Stream events not converted viafromJson.When processing live stream events,
event.object(typesdk.ParseObject) is directly inserted into_items(typeList<T>). However, when loading from cache (line 173), objects are converted viawidget.fromJson(). This inconsistency may cause type errors whenTis a custom subclass.if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { - final addedItem = event.object; - setState(() { _items.insert(event.index, addedItem); }); - objectToCache = addedItem; + final addedItem = widget.fromJson(event.object.toJson(full: true)); + setState(() { _items.insert(event.index, addedItem); }); + objectToCache = addedItem; } // ... similar change for update events } else if (event is sdk.ParseLiveListUpdateEvent<sdk.ParseObject>) { - final updatedItem = event.object; + final updatedItem = widget.fromJson(event.object.toJson(full: true)); if (event.index >= 0 && event.index < _items.length) { setState(() { _items[event.index] = updatedItem; });packages/flutter/lib/src/utils/parse_live_list.dart-636-646 (1)
636-646: Scroll listener leak when using external ScrollController.The scroll listener is added unconditionally when
widget.paginationis true (line 156), but only removed whenwidget.scrollController == null(line 640). This causes a listener leak when an external controller is provided.@override void dispose() { disposeConnectivityHandler(); - if (widget.pagination && widget.scrollController == null) { + if (widget.pagination) { _scrollController.removeListener(_onScroll); } if (widget.scrollController == null) { _scrollController.dispose(); }packages/flutter/lib/src/utils/parse_live_list.dart-442-444 (1)
442-444: Page counter incremented before successful fetch can cause skipped pages on retry.Same issue as in parse_live_grid.dart:
_currentPage++happens before the async query. If the query fails, retrying will skip to the next page instead of retrying the failed one.try { - _currentPage++; - final skipCount = _currentPage * widget.pageSize; + final nextPage = _currentPage + 1; + final skipCount = nextPage * widget.pageSize; final nextPageQuery = QueryBuilder<T>.copy(widget.query) ..setAmountToSkip(skipCount) ..setLimit(widget.pageSize); // ... after successful setState ... + _currentPage = nextPage; // Only increment after successCommittable suggestion skipped: line range outside the PR's diff.
packages/flutter/lib/src/utils/parse_live_grid.dart-704-720 (1)
704-720: Scroll listener leak when using external ScrollController.The listener is added when
widget.pagination || widget.lazyLoading(line 149-151), but only removed whenwidget.scrollController == null(line 709). If an external controller is provided with pagination/lazy loading enabled, the listener is never removed, causing potential issues after disposal.@override void dispose() { disposeConnectivityHandler(); - if ((widget.pagination || widget.lazyLoading) && widget.scrollController == null) { + if (widget.pagination || widget.lazyLoading) { _scrollController.removeListener(_onScroll); } if (widget.scrollController == null) { _scrollController.dispose(); }packages/flutter/lib/src/utils/parse_live_list.dart-281-306 (1)
281-306: Implicit type cast fromParseObjectto genericTmay fail at runtime.Same issue as in the grid widget: stream events return
sdk.ParseObjectbut are assigned to variables expected to be typeT. Add explicit casts for type safety.if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { - final addedItem = event.object; + final addedItem = event.object as T; setState(() { _items.insert(event.index, addedItem); }); objectToCache = addedItem; } else if (event is sdk.ParseLiveListDeleteEvent<sdk.ParseObject>) { // ... } else if (event is sdk.ParseLiveListUpdateEvent<sdk.ParseObject>) { - final updatedItem = event.object; + final updatedItem = event.object as T;packages/flutter/lib/src/utils/parse_live_grid.dart-421-445 (1)
421-445: Implicit type cast fromParseObjectto genericTmay fail at runtime.The stream events are typed as
sdk.ParseLiveListAddEvent<sdk.ParseObject>, butevent.objectis inserted directly into_itemswhich expects typeT. IfTis a custom subclass ofParseObject, this implicit cast will fail.if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { - final addedItem = event.object; - setState(() { _items.insert(event.index, addedItem); }); - objectToCache = addedItem; + final addedItem = event.object as T; + setState(() { _items.insert(event.index, addedItem); }); + objectToCache = addedItem; } else if (event is sdk.ParseLiveListDeleteEvent<sdk.ParseObject>) { // ... } else if (event is sdk.ParseLiveListUpdateEvent<sdk.ParseObject>) { - final updatedItem = event.object; + final updatedItem = event.object as T;packages/flutter/lib/src/utils/parse_live_grid.dart-273-275 (1)
273-275: Page counter incremented before successful fetch can cause skipped pages on retry.
_currentPage++on line 274 happens before the async query. If the query fails, the page counter remains incremented, causing the retry to skip to the next page instead of retrying the failed page.try { - _currentPage++; final skipCount = _currentPage * widget.pageSize; + final nextPage = _currentPage + 1; + final skipCount = nextPage * widget.pageSize; final nextPageQuery = QueryBuilder<T>.copy(widget.query) ..setAmountToSkip(skipCount) ..setLimit(widget.pageSize); // ... after successful response handling ... + _currentPage = nextPage; // Only increment after successCommittable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (7)
packages/flutter/lib/src/utils/parse_cached_live_list.dart-99-104 (1)
99-104: Edge case: Empty cache with capacity 0.If
cacheSizeis passed as 0 (or negative), the condition_cache.length >= capacitywould always be true, and_accessOrder.firstwould throw on an empty set. Consider validatingcapacity > 0in the constructor.class _LRUCache<K, V> { - _LRUCache(this.capacity); + _LRUCache(this.capacity) : assert(capacity > 0, 'Cache capacity must be positive');Committable suggestion skipped: line range outside the PR's diff.
packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart-12-13 (1)
12-13:latesubscription may crash ifdisposeConnectivityHandleris called beforeinitConnectivityHandler.If
disposeConnectivityHandler()is called beforeinitConnectivityHandler()completes (or if init is never called), accessing_connectivitySubscriptionwill throw aLateInitializationError. Consider using a nullable type with null-check in dispose.-late StreamSubscription<List<ConnectivityResult>> _connectivitySubscription; +StreamSubscription<List<ConnectivityResult>>? _connectivitySubscription; void disposeConnectivityHandler() { - _connectivitySubscription.cancel(); + _connectivitySubscription?.cancel(); }Committable suggestion skipped: line range outside the PR's diff.
packages/flutter/lib/src/utils/parse_live_page_view.dart-375-398 (1)
375-398: Inefficient batch save: Always fetches when lazy loading.Unlike the sliver widgets which check if items need fetching, this implementation always calls
item.fetch()for every item whenlazyLoadingis true. This causes unnecessary network requests for items that may already have complete data.if (widget.lazyLoading) { for (final item in itemsToSave) { - fetchFutures.add(item.fetch().then((_) { - itemsToSaveFinal.add(item); - }).catchError((fetchError) { + // Only fetch if item doesn't have full data + if (!item.containsKey(sdk.keyVarUpdatedAt)) { + fetchFutures.add(item.fetch().then((_) { + itemsToSaveFinal.add(item); + }).catchError((fetchError) { + debugPrint('...'); + })); + } else { + itemsToSaveFinal.add(item); + } }Committable suggestion skipped: line range outside the PR's diff.
packages/flutter/lib/src/utils/parse_live_list.dart-347-372 (1)
347-372: Concurrent list mutation without synchronization.Same pattern as grid widget:
itemsToSaveFinalis mutated from concurrentFuture.then()callbacks. Consider collecting results fromFuture.wait()instead of mutating a shared list.packages/flutter/lib/src/utils/parse_live_grid.dart-211-226 (1)
211-226: Potential null pointer exception with force-unwrap operator.Line 212 uses
context.findRenderObject()!which will throw if the render object is null. While the try-catch handles this, it's better to use safe navigation.int _calculateVisibleMaxIndex(double offset) { - if (!mounted || !context.mounted || !context.findRenderObject()!.paintBounds.isFinite) { + final renderObject = context.findRenderObject(); + if (!mounted || !context.mounted || renderObject == null || !renderObject.paintBounds.isFinite) { return 0; }packages/flutter/lib/src/utils/parse_live_grid.dart-495-519 (1)
495-519: Concurrent list mutation without synchronization.
itemsToSaveFinalis mutated from multiple concurrentFuture.then()callbacks (lines 504-510). While Dart is single-threaded, the callbacks execute in unpredictable order duringFuture.wait(), and if any callback throws, items may be lost. Consider collecting results fromFuture.wait()instead.+ List<Future<T?>> fetchFutures = []; - List<Future<void>> fetchFutures = []; if (widget.lazyLoading) { for (final item in itemsToSave) { if (item.get<DateTime>(sdk.keyVarCreatedAt) == null && item.objectId != null) { - fetchFutures.add(item.fetch().then((_) { - itemsToSaveFinal.add(item); - }).catchError((fetchError) { + fetchFutures.add(item.fetch().then((_) => item).catchError((fetchError) { debugPrint('...'); + return null; })); } else { itemsToSaveFinal.add(item); } } if (fetchFutures.isNotEmpty) { - await Future.wait(fetchFutures); + final results = await Future.wait(fetchFutures); + itemsToSaveFinal.addAll(results.whereType<T>()); } }packages/flutter/lib/src/utils/parse_live_list.dart-707-716 (1)
707-716: Inconsistent behavior with comment regardingpreLoadedDataupdate.The comment on line 713 questions whether to keep or update
preLoadedData, then says "Let's update" but the code actually keeps the old value (_snapshot.preLoadedData). This inconsistency should be resolved.If the intent is to update
preLoadedDatawith new stream data:setState(() { _snapshot = sdk.ParseLiveListElementSnapshot<T>( loadedData: data, - preLoadedData: _snapshot.preLoadedData, // Keep original preLoadedData? Or update? Let's update. - // preLoadedData: data, + preLoadedData: data, // Update preLoadedData with latest data ); });
🧹 Nitpick comments (27)
packages/flutter/example/pubspec.yaml (1)
18-24: Consider removing or relocating commented-out dependency alternatives.These commented-out lines document alternative sources for parse packages but reduce pubspec.yaml clarity. Consider moving this documentation to a separate README or wiki if needed, or remove it if no longer relevant.
PR_CHECKLIST.md (1)
87-87: Align checklist metadata with actual versions and dependenciesThe checklist text doesn’t fully match the current codebase:
- Line 115:
Version: 10.2.0+butpackages/flutter/pubspec.yamlshowsversion: 10.6.2.- Line 116:
New Dependencies: None, but at least one new dependency (cross_filein the Dart package) and a changedparse_server_sdkdependency are introduced.I’d suggest updating these checklist fields so they accurately reflect the current version and dependency changes; this avoids confusion when someone uses this file as a release/check reference.
Also applies to: 94-101, 114-116
packages/flutter/lib/src/analytics/README.md (1)
19-70: Harmonize instance vs static API usage and keep endpoint docs in lockstep with codeThis README is well‑structured and gives good end‑to‑end coverage (client tracking, data retrieval, Express and Shelf integrations, and API reference). One thing to watch:
- Here you use
final analytics = ParseAnalytics.instance;with instance methods (analytics.trackEvent(...)), whereasUSAGE.mdprimarily demonstrates static calls (ParseAnalytics.trackEvent(...)). If the implementation only supports one of these idioms, the other example will confuse users.It’d be good to:
- Pick a primary usage style (instance vs static) that matches
parse_analytics.dartand make both README + USAGE consistent.- Re‑verify that the documented endpoint handlers and parameter names in the Express/Shelf examples match
ParseAnalyticsEndpointsexactly, so dashboard integrations don’t break on small naming mismatches.Also applies to: 72-153, 155-211, 232-257
packages/flutter/lib/src/utils/parse_cached_live_list.dart (2)
21-27: Null objectId check is good, but consider cache-first lookup for consistency.
getLoadedAtalways calls the underlying_parseLiveList.getLoadedAt(index)even if the item might already be cached. For consistency withgetPreLoadedAt, you could check the cache first usingidOf(index)to avoid redundant underlying calls.T? getLoadedAt(int index) { + final objectId = _parseLiveList.idOf(index); + if (objectId != 'NotFound' && _cache.contains(objectId)) { + return _cache.get(objectId); + } final result = _parseLiveList.getLoadedAt(index); if (result != null && result.objectId != null) { _cache.put(result.objectId!, result); } return result; }
30-44: Magic string 'NotFound' should be a constant.The string
'NotFound'appears to be a sentinel value from the underlyingParseLiveList.idOf()API. Extract this to a named constant to improve maintainability and reduce risk of typos.+/// Sentinel value returned by ParseLiveList.idOf() when object is not found +static const String _notFoundId = 'NotFound'; T? getPreLoadedAt(int index) { final objectId = _parseLiveList.idOf(index); // Try cache first - if (objectId != 'NotFound' && _cache.contains(objectId)) { + if (objectId != _notFoundId && _cache.contains(objectId)) { return _cache.get(objectId); }packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart (1)
96-106: State transitions call async methods without error handling.If
loadDataFromServer()orloadDataFromCache()throws, the exception propagates but_isOfflinestate has already been updated. Consider wrapping in try-catch to maintain consistent state on failure.if (isOnline && !wasOnline) { // --- Transitioning TO Online --- - _isOffline = false; debugPrint('$connectivityLogPrefix Transitioning Online: $result. Loading data from server...'); - await loadDataFromServer(); + try { + await loadDataFromServer(); + _isOffline = false; + } catch (e) { + debugPrint('$connectivityLogPrefix Error loading from server: $e'); + // Optionally fall back to cache or retry + } }packages/flutter/lib/src/analytics/parse_analytics.dart (4)
20-73: Performance: Five sequential network queries could be slow.
getUserAnalytics()makes 5 separatecount()queries sequentially. Consider usingFuture.waitto parallelize these queries for better performance.static Future<Map<String, dynamic>> getUserAnalytics() async { try { final now = DateTime.now(); final yesterday = now.subtract(const Duration(days: 1)); final weekAgo = now.subtract(const Duration(days: 7)); final monthAgo = now.subtract(const Duration(days: 30)); - // Get user count queries using QueryBuilder - final totalUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()); - final totalUsersResult = await totalUsersQuery.count(); - final totalUsers = totalUsersResult.count; - - final activeUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) - ..whereGreaterThan('updatedAt', weekAgo); - final activeUsersResult = await activeUsersQuery.count(); - final activeUsers = activeUsersResult.count; - // ... more sequential queries + // Parallelize queries for better performance + final results = await Future.wait([ + QueryBuilder<ParseUser>(ParseUser.forQuery()).count(), + (QueryBuilder<ParseUser>(ParseUser.forQuery())..whereGreaterThan('updatedAt', weekAgo)).count(), + (QueryBuilder<ParseUser>(ParseUser.forQuery())..whereGreaterThan('updatedAt', yesterday)).count(), + (QueryBuilder<ParseUser>(ParseUser.forQuery())..whereGreaterThan('updatedAt', weekAgo)).count(), + (QueryBuilder<ParseUser>(ParseUser.forQuery())..whereGreaterThan('updatedAt', monthAgo)).count(), + ]); + + return { + 'timestamp': now.millisecondsSinceEpoch, + 'total_users': results[0].count, + 'active_users': results[1].count, + 'daily_users': results[2].count, + 'weekly_users': results[3].count, + 'monthly_users': results[4].count, + };
252-270: Event storage truncation silently discards old events.When exceeding 1000 events, older events are removed without any callback or notification. This could cause data loss if events haven't been uploaded. Consider adding an upload mechanism or warning.
// Keep only last 1000 events if (existingEvents.length > 1000) { + if (kDebugMode) { + print('Analytics: Truncating ${existingEvents.length - 1000} old events'); + } existingEvents.removeRange(0, existingEvents.length - 1000); }
363-369:fromJsonmay throw on malformed input.The factory constructor assumes
json['event_name']is a non-nullStringandjson['timestamp']is a non-nullint. Malformed JSON will throw aTypeError. Consider defensive parsing.factory AnalyticsEventData.fromJson(Map<String, dynamic> json) => AnalyticsEventData( - eventName: json['event_name'] as String, + eventName: json['event_name'] as String? ?? '', parameters: Map<String, dynamic>.from(json['parameters'] ?? {}), - timestamp: DateTime.fromMillisecondsSinceEpoch(json['timestamp'] as int), + timestamp: json['timestamp'] != null + ? DateTime.fromMillisecondsSinceEpoch(json['timestamp'] as int) + : DateTime.now(), userId: json['user_id'] as String?, installationId: json['installation_id'] as String?, );
374-388: Remove commented-out example code from production file.This block should be moved to documentation (README or dartdoc) rather than left as a comment in the source file.
packages/flutter/test_offline.dart (2)
1-34: Test file should be moved to proper test directory and use a test framework.This file is located at
packages/flutter/test_offline.dartinstead ofpackages/flutter/test/. It also doesn't use Flutter's test framework (flutter_test), making it incompatible withflutter testand CI pipelines.Consider restructuring as a proper test:
// Move to: packages/flutter/test/offline_extension_test.dart import 'package:flutter_test/flutter_test.dart'; import 'package:parse_server_sdk/parse_server_sdk.dart'; void main() { setUpAll(() async { await Parse().initialize( 'test_app_id', 'https://test.com', clientKey: 'test_client_key', debug: false, ); }); test('ParseObjectOffline extension methods are available', () async { final object = ParseObject('TestClass'); object.set('name', 'Test Object'); await object.saveToLocalCache(); final cached = await ParseObjectOffline.loadAllFromLocalCache('TestClass'); expect(cached, isNotEmpty); }); }
5-10: Hardcoded test credentials should use environment variables or test fixtures.While this is a test file, using hardcoded credentials (even fake ones) sets a poor example. Consider using a test configuration pattern.
packages/dart/lib/src/storage/core_store_sem_impl.dart (1)
98-104: Consider adding error handling for type coercion.The type coercion logic improves robustness, but silently returning
nullfor unexpected types could mask data corruption issues. Consider logging a warning when encountering unexpected types to aid debugging.Future<List<String>?> getStringList(String key) async { final value = await get(key); if (value == null) return null; if (value is List<String>) return value; if (value is Iterable) return value.map((e) => e.toString()).toList(); + if (value != null) { + print('Warning: Unexpected type ${value.runtimeType} for key $key in getStringList'); + } return null; }packages/dart/test_extension.dart (1)
1-28: Consider converting to a proper unit test.This file appears to be a manual test script rather than an automated test. Consider:
- Using a testing framework (e.g.,
testpackage) to enable CI integration- Adding assertions to verify expected behavior instead of just printing results
- Verifying that cached data matches saved data
Example:
import 'package:test/test.dart'; import 'package:parse_server_sdk/parse_server_sdk.dart'; void main() { setUp(() async { await Parse().initialize(/*...*/); }); test('ParseObjectOffline extension methods work correctly', () async { var dietPlan = ParseObject('DietPlan') ..set('Name', 'Test') ..set('Fat', 50) ..objectId = 'test-id'; await dietPlan.saveToLocalCache(); var cached = await ParseObjectOffline.loadAllFromLocalCache('DietPlan'); expect(cached.length, greaterThan(0)); }); }packages/flutter/README.md (1)
65-256: Excellent documentation of new features!The documentation is comprehensive and well-structured, covering:
- Live query widget variants with clear examples
- Offline mode configuration and usage patterns
- Complete API reference for offline caching methods
One minor suggestion: Consider adding a troubleshooting section for common offline mode issues (e.g., cache not persisting, sync conflicts, storage limits).
packages/flutter/test_offline_mode.dart (1)
1-104: Consider converting to automated integration test.While this script provides good coverage of offline functionality, it should be converted to a proper test using the
testorflutter_testpackage. This would:
- Enable CI/CD integration
- Use assertions instead of print statements
- Automatically fail on errors
- Integrate with test coverage reporting
Example structure:
import 'package:test/test.dart'; import 'package:parse_server_sdk/parse_server_sdk.dart'; void main() { group('Offline Mode Tests', () { setUp(() async { await Parse().initialize(/*...*/); }); test('Save and load single object', () async { final obj = ParseObject('TestClass')..set('name', 'Test'); obj.objectId = 'test-id-1'; await obj.saveToLocalCache(); final loaded = await ParseObjectOffline.loadFromLocalCache( 'TestClass', 'test-id-1'); expect(loaded, isNotNull); expect(loaded!.get<String>('name'), equals('Test')); }); // ... more tests }); }packages/dart/lib/src/objects/parse_offline_object.dart (3)
35-79: Good implementation with efficient batch processing.This method demonstrates better practices than others:
- Uses a Map for efficient lookups and updates
- Includes error handling for individual object decode failures
- Provides useful logging of operation statistics
Consider wrapping the entire method in a try-catch to handle CoreStore failures:
static Future<void> saveAllToLocalCache(String className, List<ParseObject> objectsToSave) async { if (objectsToSave.isEmpty) return; + try { final CoreStore coreStore = ParseCoreData().getStore(); // ... existing code ... await coreStore.setStringList(cacheKey, updatedCachedStrings); print('Batch saved to local cache for $className. Added: $added, Updated: $updated, Total: ${updatedCachedStrings.length}'); + } catch (e) { + print('Error in batch save to local cache for $className: $e'); + rethrow; + } }
129-150: Consider optimizing cache lookup operations.Both
existsInLocalCacheandgetAllObjectIdsInLocalCacheiterate through the entire cache, which could be slow for large datasets. Consider:
- Maintaining a separate index of objectIds for faster lookups
- Using a different CoreStore key pattern (one key per object) for O(1) lookups
- Caching the object ID list in memory
Example alternative pattern:
// Store each object with its own key final objectKey = 'offline_cache_${className}_$objectId'; await coreStore.setString(objectKey, json.encode(toJson(full: true))); // Maintain an index final indexKey = 'offline_cache_${className}_index'; final objectIds = await coreStore.getStringList(indexKey) ?? []; if (!objectIds.contains(objectId)) { objectIds.add(objectId); await coreStore.setStringList(indexKey, objectIds); }
1-165: Consider adding concurrency control for cache operations.Multiple concurrent cache operations (save, update, remove) could lead to race conditions and data corruption. Consider:
- Using a lock/mutex for cache operations
- Implementing a queue for cache operations
- Documenting thread-safety guarantees
This is particularly important for the offline mode use case where multiple widgets might save/update cached data simultaneously.
packages/flutter/example/lib/live_list/main.dart (1)
2-4: Remove commented-out import.Line 4 contains a commented-out import that appears to be a debug artifact. This should be removed to keep the example clean.
import 'package:parse_server_sdk/parse_server_sdk.dart'; import 'package:parse_server_sdk_flutter/parse_server_sdk_flutter.dart' hide Parse; -// import 'package:parse_server_sdk_flutter/parse_server_sdk_flutter.dart';packages/flutter/lib/src/analytics/parse_analytics_endpoints.dart (2)
8-71: Reduce duplication in audience handler.The switch statement has repetitive structure where each case returns the same map pattern with
'total'and'content'keys containing identical values. Consider refactoring to reduce duplication.static Future<Map<String, dynamic>> handleAudienceRequest(String audienceType) async { try { final userAnalytics = await ParseAnalytics.getUserAnalytics(); final installationAnalytics = await ParseAnalytics.getInstallationAnalytics(); - - switch (audienceType) { - case 'total_users': - return { - 'total': userAnalytics['total_users'], - 'content': userAnalytics['total_users'] - }; - - case 'daily_users': - return { - 'total': userAnalytics['daily_users'], - 'content': userAnalytics['daily_users'] - }; - - // ... other cases ... - - default: - return {'total': 0, 'content': 0}; - } + + final analyticsMap = { + 'total_users': userAnalytics['total_users'], + 'daily_users': userAnalytics['daily_users'], + 'weekly_users': userAnalytics['weekly_users'], + 'monthly_users': userAnalytics['monthly_users'], + 'total_installations': installationAnalytics['total_installations'], + 'daily_installations': installationAnalytics['daily_installations'], + 'weekly_installations': installationAnalytics['weekly_installations'], + 'monthly_installations': installationAnalytics['monthly_installations'], + }; + + final value = analyticsMap[audienceType] ?? 0; + return {'total': value, 'content': value}; } catch (e) { // ... } }
121-169: Mock implementations may cause confusion in production.These billing and slow query handlers return hardcoded mock data. While comments indicate they should be replaced, there's no runtime indication that these are mocks. Consider:
- Throwing
UnimplementedErrorto force implementation before use- Adding a
@visibleForTestingannotation if these are only for testing- Logging a warning when these mocks are called in non-debug mode
static Map<String, dynamic> handleBillingStorageRequest() { assert(() { debugPrint('WARNING: Using mock billing storage data'); return true; }()); // ... existing mock implementation }packages/flutter/lib/src/utils/parse_live_sliver_list.dart (1)
329-347: Inconsistent lazy loading check across widgets.The check for whether an item needs fetching before caching differs between widgets:
- Sliver List (here):
!item.containsKey(sdk.keyVarUpdatedAt)- Sliver Grid:
item.get<DateTime>(sdk.keyVarCreatedAt) == null && item.objectId != nullThese should use the same logic for consistency. Consider extracting this check into a shared helper method in the mixin or a utility class.
packages/flutter/lib/src/utils/parse_live_page_view.dart (1)
509-535: Redundant preload calls.
_preloadAdjacentPagesis called both inonPageChanged(line 512) and initemBuilder(line 534). SinceitemBuilderis called for every visible item, this results in redundant preload calls. Consider removing one of these calls.itemBuilder: (context, index) { // Show loading indicator for the last item... if (widget.pagination && index >= _items.length) { return widget.loadingIndicator ?? ...; } - // Preload adjacent pages for smoother experience (only if online) - if (!isOffline) { - _preloadAdjacentPages(index); - } final item = _items[index];packages/flutter/lib/src/utils/parse_live_grid.dart (2)
19-39: Several constructor parameters are declared but unused in state logic.The comments indicate
removedItemBuilder,animationController,lazyBatchSize, andlazyTriggerOffsetare not used. This adds API surface without functionality.Consider either:
- Implementing the functionality these parameters are meant to support
- Removing them until needed (YAGNI principle)
- Marking them as
@Deprecatedwith a message about future implementation
677-702: Consider capturing_liveGridreference to avoid potential null issues.The method checks
_liveGrid == nullat line 678, but uses_liveGrid!at lines 687 and 689. While unlikely in practice, capturing the reference avoids repeated null assertions.void _triggerBatchLoading(int targetIndex) { - if (isOffline || !widget.lazyLoading || _liveGrid == null) return; + final liveGrid = _liveGrid; + if (isOffline || !widget.lazyLoading || liveGrid == null) return; final batchSize = widget.lazyBatchSize > 0 ? widget.lazyBatchSize : widget.crossAxisCount * 2; final startIdx = max(0, targetIndex - batchSize); final endIdx = min(_items.length - 1, targetIndex + batchSize); for (int i = startIdx; i <= endIdx; i++) { - if (i >= 0 && i < _liveGrid!.size && !_loadingIndices.contains(i) && _liveGrid!.getLoadedAt(i) == null) { + if (i >= 0 && i < liveGrid.size && !_loadingIndices.contains(i) && liveGrid.getLoadedAt(i) == null) { _loadingIndices.add(i); - _liveGrid!.getAt(i).first.then((loadedItem) { + liveGrid.getAt(i).first.then((loadedItem) {packages/flutter/lib/src/utils/parse_live_list.dart (1)
71-71: Unused parameters declared in widget constructors.
removedItemBuilder(line 71) anderror(line 676) are declared but noted as unused in state logic. Consider implementing or removing these to reduce API surface confusion.Also applies to: 676-676
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
PR_CHECKLIST.md(1 hunks)packages/dart/lib/parse_server_sdk.dart(1 hunks)packages/dart/lib/src/objects/parse_offline_object.dart(1 hunks)packages/dart/lib/src/storage/core_store_memory.dart(1 hunks)packages/dart/lib/src/storage/core_store_sem_impl.dart(1 hunks)packages/dart/pubspec.yaml(2 hunks)packages/dart/test_extension.dart(1 hunks)packages/flutter/ANALYTICS_INTEGRATION_GUIDE.md(1 hunks)packages/flutter/README.md(2 hunks)packages/flutter/example/lib/live_list/main.dart(2 hunks)packages/flutter/example/lib/pages/decision_page.dart(1 hunks)packages/flutter/example/pubspec.yaml(1 hunks)packages/flutter/example/test/data/repository/repository_mock_utils.dart(1 hunks)packages/flutter/lib/parse_server_sdk_flutter.dart(4 hunks)packages/flutter/lib/src/analytics/README.md(1 hunks)packages/flutter/lib/src/analytics/USAGE.md(1 hunks)packages/flutter/lib/src/analytics/example_server.js(1 hunks)packages/flutter/lib/src/analytics/parse_analytics.dart(1 hunks)packages/flutter/lib/src/analytics/parse_analytics_endpoints.dart(1 hunks)packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart(1 hunks)packages/flutter/lib/src/utils/parse_cached_live_list.dart(1 hunks)packages/flutter/lib/src/utils/parse_live_grid.dart(4 hunks)packages/flutter/lib/src/utils/parse_live_list.dart(5 hunks)packages/flutter/lib/src/utils/parse_live_page_view.dart(1 hunks)packages/flutter/lib/src/utils/parse_live_sliver_grid.dart(1 hunks)packages/flutter/lib/src/utils/parse_live_sliver_list.dart(1 hunks)packages/flutter/pubspec.yaml(3 hunks)packages/flutter/test/parse_connectivity_implementation_test.dart(3 hunks)packages/flutter/test_offline.dart(1 hunks)packages/flutter/test_offline_mode.dart(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/flutter/lib/src/analytics/example_server.js
[error] 37-37: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 42-44: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 49-51: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 56-58: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 63-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 69-71: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 76-78: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 83-85: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter 3.38, Windows
pastordee
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
… and live widgets
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (3)
packages/flutter/lib/src/utils/parse_live_sliver_list.dart (1)
374-393: Unreliable lazy loading check usingcontainsKey(keyVarUpdatedAt).This issue was flagged in a previous review. Checking
!item.containsKey(sdk.keyVarUpdatedAt)may not accurately determine if an item needs fetching, as an item could have anupdatedAtfield but still be incomplete when lazy loading is enabled.packages/flutter/lib/src/utils/parse_live_sliver_grid.dart (1)
467-486: Inconsistent lazy loading check usingcreatedAtinstead ofupdatedAt.This was flagged in a previous review. The sliver list widget checks
keyVarUpdatedAtwhile this grid widget checkskeyVarCreatedAt. Both approaches are unreliable, but they should at least be consistent across widgets.packages/flutter/lib/src/analytics/parse_analytics.dart (1)
1-10: Analytics implementation appears unrelated to PR objectives.This PR is titled "Add offline mode support for ParseLive and new live query widgets" but this analytics implementation doesn't appear related to offline mode or live query widgets. As noted in a previous review, consider moving this to a separate PR for focused review.
🧹 Nitpick comments (9)
packages/dart/lib/src/storage/core_store_memory.dart (1)
50-53: Remove or document the commented legacy code.The commented-out implementation serves no purpose if it's been replaced. Consider removing it to reduce clutter, or add a comment explaining why it's preserved.
Apply this diff to remove the commented code:
- // @override - // Future<List<String>?> getStringList(String key) async { - // return _data[key]; - // } -packages/dart/lib/src/storage/core_store_sem_impl.dart (1)
106-110: Remove or document the commented legacy code.Similar to the memory store implementation, this commented-out code should be removed to maintain consistency and reduce clutter.
Apply this diff to remove the commented code:
- // @override - // Future<List<String>?> getStringList(String key) async { - // final List<String>? storedItem = await get(key); - // return storedItem; - // } -packages/flutter/lib/src/utils/parse_live_list.dart (1)
71-72: Consider removing or implementingremovedItemBuilder.The comment notes that
removedItemBuilderis not currently used in the state logic. If this is intentional dead code, consider removing it to avoid confusion. Otherwise, implement the removal animation logic.packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart (1)
43-49: Connectivity priority differs from main library mapping.The mixin prioritizes
mobile > wifi > none > other, while_mapConnectivityin the main library useswifi > mobile > none. This inconsistency could cause different behavior depending on which code path handles connectivity.Consider aligning the priority order for consistency:
final newResult = results.contains(ConnectivityResult.wifi) ? ConnectivityResult.wifi : results.contains(ConnectivityResult.mobile) ? ConnectivityResult.mobile - : results.contains(ConnectivityResult.none) - ? ConnectivityResult.none - : ConnectivityResult.other; + : ConnectivityResult.none;Also applies to: 64-71
packages/flutter/test/src/utils/parse_live_widgets_test.dart (2)
128-159: Dispose controllers in tests to prevent resource leaks.The
ScrollControllercreated at line 133 is not disposed after the test. While Flutter test framework may handle this, it's best practice to dispose controllers explicitly, especially for complex widgets.testWidgets('should accept optional parameters', ( WidgetTester tester, ) async { // Arrange final query = QueryBuilder<TestObject>(TestObject()); final scrollController = ScrollController(); + addTearDown(() => scrollController.dispose()); // Act await tester.pumpWidget(The same applies to
PageControllerat line 355.
81-160: Consider adding tests for offline mode behavior.The test suite covers basic widget creation but doesn't test the core offline mode functionality introduced by this PR. Consider adding tests for:
- Behavior when
offlineMode: trueand connectivity changes to offline- Cache loading when offline
- Data synchronization when connectivity is restored
packages/flutter/lib/src/utils/parse_live_page_view.dart (1)
436-465: Inconsistent lazy loading behavior compared to other widgets.Unlike
ParseLiveSliverListWidgetandParseLiveSliverGridWidget, this implementation unconditionally fetches all items when lazy loading is enabled (line 441-456), without first checking if the item already has complete data. This could result in unnecessary network calls.Consider adding a check similar to the other widgets to skip fetching items that already have complete data, ensuring consistent behavior across all live widgets.
packages/flutter/lib/src/utils/parse_live_grid.dart (2)
19-19: Remove unused widget parameters or implement their functionality.Several widget parameters are declared but never used in the state implementation:
removedItemBuilder(line 19): Not used in any removal logicanimationController(line 25): Not used for item animations (see lines 723-725)lazyTriggerOffset(line 37): Despite the comment, this parameter is never referenced in scroll or lazy loading logicNote:
lazyBatchSizeIS used in_triggerBatchLoading(lines 785-787), so its comment is incorrect.Apply this diff to remove the unused parameters:
- this.removedItemBuilder, // Note: Not currently used in state logic this.listenOnAllSubItems, this.listeningIncludes, this.lazyLoading = true, this.preloadedColumns, this.excludedColumns, - this.animationController, // Note: Not currently used for item animations this.crossAxisCount = 3,And remove the field declarations:
final ChildBuilder<T>? childBuilder; - final ChildBuilder<T>? removedItemBuilder; final bool? listenOnAllSubItems; final List<String>? listeningIncludes; final bool lazyLoading; final List<String>? preloadedColumns; final List<String>? excludedColumns; - final AnimationController? animationController;And remove or update the lazyTriggerOffset if it's not intended for future use:
final int lazyBatchSize; - final double lazyTriggerOffset;Also applies to: 25-25, 37-37
271-273: Update misleading comment.The comment on line 272 says "Await is fine here as this whole function runs in the background" but the code doesn't actually await
_saveBatchToCache. While not awaiting is the correct choice (to allow background processing), the comment is confusing.Apply this diff to clarify:
if (results.isNotEmpty) { - // Use the existing batch save helper (it handles lazy fetching if needed) - // Await is fine here as this whole function runs in the background + // Use the existing batch save helper (runs in background) await _saveBatchToCache(results);Or if the intent was NOT to await:
if (results.isNotEmpty) { - // Use the existing batch save helper (it handles lazy fetching if needed) - // Await is fine here as this whole function runs in the background - await _saveBatchToCache(results); + // Trigger batch save in background without blocking + unawaited(_saveBatchToCache(results));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/dart/lib/src/objects/parse_offline_object.dart(1 hunks)packages/dart/lib/src/storage/core_store_memory.dart(1 hunks)packages/dart/lib/src/storage/core_store_sem_impl.dart(1 hunks)packages/dart/test/src/objects/parse_offline_object_test.dart(1 hunks)packages/dart/test_extension.dart(1 hunks)packages/flutter/example/lib/live_list/main.dart(1 hunks)packages/flutter/lib/parse_server_sdk_flutter.dart(3 hunks)packages/flutter/lib/src/analytics/parse_analytics.dart(1 hunks)packages/flutter/lib/src/analytics/parse_analytics_endpoints.dart(1 hunks)packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart(1 hunks)packages/flutter/lib/src/utils/parse_cached_live_list.dart(1 hunks)packages/flutter/lib/src/utils/parse_live_grid.dart(4 hunks)packages/flutter/lib/src/utils/parse_live_list.dart(5 hunks)packages/flutter/lib/src/utils/parse_live_page_view.dart(1 hunks)packages/flutter/lib/src/utils/parse_live_sliver_grid.dart(1 hunks)packages/flutter/lib/src/utils/parse_live_sliver_list.dart(1 hunks)packages/flutter/pubspec.yaml(3 hunks)packages/flutter/test/parse_connectivity_implementation_test.dart(0 hunks)packages/flutter/test/src/analytics/parse_analytics_test.dart(1 hunks)packages/flutter/test/src/mixins/connectivity_handler_mixin_test.dart(1 hunks)packages/flutter/test/src/utils/parse_live_widgets_test.dart(1 hunks)
💤 Files with no reviewable changes (1)
- packages/flutter/test/parse_connectivity_implementation_test.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/flutter/example/lib/live_list/main.dart
- packages/dart/test_extension.dart
- packages/flutter/lib/src/utils/parse_cached_live_list.dart
- packages/dart/lib/src/objects/parse_offline_object.dart
- packages/flutter/pubspec.yaml
- packages/flutter/lib/src/analytics/parse_analytics_endpoints.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Dart 3.10, Windows
🔇 Additional comments (35)
packages/dart/lib/src/storage/core_store_memory.dart (1)
42-48: LGTM — robust type handling for getStringList.The implementation correctly handles multiple value shapes (null, List, Iterable) and provides safe fallback behavior. The
toString()conversion for Iterable elements ensures compatibility with varying stored formats.packages/dart/lib/src/storage/core_store_sem_impl.dart (1)
98-104: LGTM — consistent string list handling across storage implementations.The implementation mirrors the memory store's robust type handling, ensuring consistent behavior across sembast and in-memory storage backends.
packages/dart/test/src/objects/parse_offline_object_test.dart (9)
1-22: LGTM — proper test setup and isolation.The test suite correctly initializes Parse in
setUpAlland ensures test isolation by clearing the cache before and after each test. Using a constanttestClassNamemaintains consistency across tests.
24-63: LGTM — comprehensive save and update tests.Tests correctly verify both initial save and update scenarios using appropriate verification methods (existence check and field value validation).
65-98: LGTM — thorough load operation coverage.Tests validate both successful load with field verification and null return for non-existent objects. Good null-safety handling at Line 82.
100-183: LGTM — excellent batch operation coverage.Tests comprehensively validate batch save scenarios including multiple new objects, mixed updates and additions, and proper handling of objects without objectId. The assertion at Line 155 correctly expects 3 objects after update operations.
185-213: LGTM — load all operations properly tested.Tests validate both populated cache and empty cache scenarios. Using a different class name ('EmptyClass') at Line 207 ensures proper isolation.
215-233: LGTM — remove operation correctly verified.Test validates removal by checking that
existsInLocalCachereturns false after the operation.
235-255: LGTM — field update operation validated.Test correctly verifies that
updateInLocalCacheupdates specific fields by loading the object and checking the modified values.
257-285: LGTM — existence checks cover both scenarios.Tests validate both positive (existing) and negative (non-existing) cases for the existence check operation.
287-343: LGTM — clear and ID retrieval operations properly tested.The
clearLocalCacheForClasstest verifies all objects are removed, whilegetAllObjectIdsInLocalCachetests cover both populated and empty cache scenarios. The use ofcontainsAllat Line 331 is appropriate for set-like validation.packages/flutter/test/src/mixins/connectivity_handler_mixin_test.dart (3)
10-35: Well-structured mock implementation.The
MockConnectivityPlatformproperly implements the platform interface with a broadcastStreamControllerfor simulating connectivity changes. Thedispose()method correctly closes the stream controller.
47-90: Test widget correctly implements mixin contract.The
TestConnectivityWidgetStateproperly implements all abstract methods fromConnectivityHandlerMixinand exposes counters for verification. The lifecycle methods (initState/dispose) correctly callinitConnectivityHandler()anddisposeConnectivityHandler().
120-289: Comprehensive test coverage for connectivity transitions.The test suite covers key scenarios: online/offline initialization, bidirectional transitions, mobile connectivity, disabled offline mode, and proper subscription disposal. The assertions use
greaterThanOrEqualToappropriately to account for potential multiple calls during initialization.packages/flutter/lib/src/utils/parse_live_list.dart (2)
3-9: LGTM - API additions are well-designed.The
ChildBuildertypedef extension with an optionalindexparameter maintains backward compatibility. TheLoadMoreStatusenum andFooterBuildertypedef provide clean abstractions for pagination state management.Also applies to: 17-22
614-692: Build method handles online/offline states well.The
ValueListenableBuilderpattern efficiently rebuilds only when data availability changes. The loading indicator logic correctly shows loading only when online and_liveListis not yet initialized. The offline fallback to_itemsprovides seamless UX.packages/flutter/lib/parse_server_sdk_flutter.dart (2)
136-146: Ethernet connections will be treated as offline.The
_mapConnectivitymethod no longer handlesConnectivityResult.ethernet. Users on wired connections will getParseConnectivityResult.none, triggering offline mode behavior. This may be intentional for mobile-focused apps but could cause unexpected behavior on desktop platforms.If ethernet should be treated as online, add it to the mapping:
sdk.ParseConnectivityResult _mapConnectivity( List<ConnectivityResult> results, ) { if (results.contains(ConnectivityResult.wifi)) { return sdk.ParseConnectivityResult.wifi; + } else if (results.contains(ConnectivityResult.ethernet)) { + return sdk.ParseConnectivityResult.wifi; // Treat ethernet as wifi/online } else if (results.contains(ConnectivityResult.mobile)) { return sdk.ParseConnectivityResult.mobile; } else { return sdk.ParseConnectivityResult.none; } }
25-27: New exports and part files properly declared.The analytics exports and new part file declarations for sliver widgets and page view are correctly structured.
Also applies to: 37-40
packages/flutter/test/src/analytics/parse_analytics_test.dart (2)
22-110: Good test coverage for ParseAnalytics API.Tests cover initialization (including idempotency), event tracking with various parameter types, stream availability, stored events retrieval, and clearing. The arrange/act/assert pattern is consistently applied.
123-219: Comprehensive AnalyticsEventData model tests.The tests verify construction with required and optional fields, JSON serialization/deserialization roundtrip, and null parameter handling. Timestamp handling with
millisecondsSinceEpochis correctly tested.packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart (2)
109-113:ConnectivityResult.otherandethernetare treated as offline.The
isOnlinecheck only considersmobileandwifias online states. This means VPN, Bluetooth tethering, or ethernet connections (represented asotherorethernet) will trigger offline mode, which may not be the intended behavior.If these connection types should be treated as online:
bool isOnline = result == ConnectivityResult.mobile || - result == ConnectivityResult.wifi; + result == ConnectivityResult.wifi || + result == ConnectivityResult.ethernet || + result == ConnectivityResult.vpn || + result == ConnectivityResult.other;
89-164: State transition logic is well-structured.The
_internalUpdateConnectionStatusmethod correctly handles all transition scenarios: online→offline, offline→online, initial state, and no-transition cases. The guard clause for unchanged status prevents unnecessary reloads. Debug logging aids troubleshooting.packages/flutter/lib/src/utils/parse_live_grid.dart (13)
88-106: LGTM!The addition of the optional
indexparameter todefaultChildBuilderenhances debugging and UI clarity. The implementation correctly handles the optional parameter.
123-141: LGTM!The mixin implementation correctly delegates to private methods and provides appropriate configuration values.
143-158: LGTM!The initialization correctly handles optional scroll controller creation and conditionally attaches scroll listeners based on feature enablement.
160-200: LGTM!The cache loading logic properly handles offline mode checks, deserialization errors, and state updates. The error handling around
fromJsondeserialization is particularly good.
202-226: LGTM!The scroll handler correctly implements both pagination and lazy loading with appropriate online/offline checks and distance-based triggers.
292-376: LGTM!The pagination logic correctly handles loading more data, prioritizes UI responsiveness by updating state before triggering background cache operations, and appropriately manages loading states.
565-634: LGTM!The batch caching logic appropriately handles lazy loading by pre-fetching incomplete items, uses concurrent fetching with
Future.wait, and includes proper error handling for individual fetch failures.
636-652: LGTM!The refresh logic correctly disposes existing resources and reloads data based on connectivity state.
654-689: LGTM!The build method correctly manages loading, empty, and data states while considering offline mode. The conditional footer rendering is appropriate for paginated views.
692-718: LGTM!The default footer implementation handles all load states appropriately and provides good UX with tap-to-retry on errors.
721-778: LGTM!The grid builder correctly handles both online (with stream-based updates) and offline (with direct item access) modes, and generates appropriate unique keys for items.
781-825: LGTM!The batch loading logic for lazy loading correctly manages loading state tracking, bounds checking, and asynchronous item loading with appropriate error handling.
827-844: LGTM!The dispose method properly cleans up all resources including mixin-managed connectivity handlers, conditionally-owned scroll controllers, and internal state.
| final activeUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', weekAgo); | ||
| final activeUsersResult = await activeUsersQuery.count(); | ||
| final activeUsers = activeUsersResult.count; | ||
|
|
||
| final dailyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', yesterday); | ||
| final dailyUsersResult = await dailyUsersQuery.count(); | ||
| final dailyUsers = dailyUsersResult.count; | ||
|
|
||
| final weeklyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', weekAgo); | ||
| final weeklyUsersResult = await weeklyUsersQuery.count(); | ||
| final weeklyUsers = weeklyUsersResult.count; |
There was a problem hiding this comment.
Duplicate queries for activeUsers and weeklyUsers.
Both activeUsersQuery (line 32-35) and weeklyUsersQuery (line 42-45) use the same filter (whereGreaterThan('updatedAt', weekAgo)), resulting in identical results and an unnecessary extra network call.
final activeUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery())
..whereGreaterThan('updatedAt', weekAgo);
final activeUsersResult = await activeUsersQuery.count();
final activeUsers = activeUsersResult.count;
final dailyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery())
..whereGreaterThan('updatedAt', yesterday);
final dailyUsersResult = await dailyUsersQuery.count();
final dailyUsers = dailyUsersResult.count;
- final weeklyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery())
- ..whereGreaterThan('updatedAt', weekAgo);
- final weeklyUsersResult = await weeklyUsersQuery.count();
- final weeklyUsers = weeklyUsersResult.count;
+ // weeklyUsers is the same as activeUsers (both use weekAgo)
+ final weeklyUsers = activeUsers;The same issue exists for activeInstallations and weeklyInstallations (lines 90-106).
📝 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.
| final activeUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', weekAgo); | |
| final activeUsersResult = await activeUsersQuery.count(); | |
| final activeUsers = activeUsersResult.count; | |
| final dailyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', yesterday); | |
| final dailyUsersResult = await dailyUsersQuery.count(); | |
| final dailyUsers = dailyUsersResult.count; | |
| final weeklyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', weekAgo); | |
| final weeklyUsersResult = await weeklyUsersQuery.count(); | |
| final weeklyUsers = weeklyUsersResult.count; | |
| final activeUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', weekAgo); | |
| final activeUsersResult = await activeUsersQuery.count(); | |
| final activeUsers = activeUsersResult.count; | |
| final dailyUsersQuery = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', yesterday); | |
| final dailyUsersResult = await dailyUsersQuery.count(); | |
| final dailyUsers = dailyUsersResult.count; | |
| // weeklyUsers is the same as activeUsers (both use weekAgo) | |
| final weeklyUsers = activeUsers; |
🤖 Prompt for AI Agents
packages/flutter/lib/src/analytics/parse_analytics.dart lines 32-45 and 90-106:
there are duplicate queries (activeUsers vs weeklyUsers both use
whereGreaterThan('updatedAt', weekAgo) and activeInstallations vs
weeklyInstallations likewise) causing redundant network calls; fix by removing
the duplicate query calls and reusing the already-counted result (e.g., compute
weeklyUsers once and assign activeUsers = weeklyUsers or vice versa) or, if the
intent was different time windows, correct the filter for the intended
timeframe; apply the same consolidation/correction for installations so only one
network call is made per unique filter.
| static Future<List<List<num>>> getTimeSeriesData({ | ||
| required String metric, | ||
| required DateTime startDate, | ||
| required DateTime endDate, | ||
| String interval = 'day', | ||
| }) async { | ||
| try { | ||
| final data = <List<num>>[]; | ||
| final intervalDuration = interval == 'hour' | ||
| ? const Duration(hours: 1) | ||
| : const Duration(days: 1); | ||
|
|
||
| DateTime current = startDate; | ||
| while (current.isBefore(endDate)) { | ||
| final next = current.add(intervalDuration); | ||
| int value = 0; | ||
|
|
||
| switch (metric) { | ||
| case 'users': | ||
| final query = QueryBuilder<ParseUser>(ParseUser.forQuery()) | ||
| ..whereGreaterThan('updatedAt', current) | ||
| ..whereLessThan('updatedAt', next); | ||
| final result = await query.count(); | ||
| value = result.count; | ||
| break; | ||
|
|
||
| case 'installations': | ||
| final query = | ||
| QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | ||
| ..whereGreaterThan('updatedAt', current) | ||
| ..whereLessThan('updatedAt', next); | ||
| final result = await query.count(); | ||
| value = result.count; | ||
| break; | ||
| } | ||
|
|
||
| data.add([current.millisecondsSinceEpoch, value]); | ||
| current = next; | ||
| } | ||
|
|
||
| return data; | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Error getting time series data: $e'); | ||
| } | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential performance issue: unbounded query loop in getTimeSeriesData.
The method iterates from startDate to endDate making a network query on each iteration. For hourly intervals over a month, this would make ~720 queries. Consider:
- Adding a maximum iteration limit
- Using server-side aggregation if available
- Batching queries or using caching
static Future<List<List<num>>> getTimeSeriesData({
required String metric,
required DateTime startDate,
required DateTime endDate,
String interval = 'day',
}) async {
try {
final data = <List<num>>[];
final intervalDuration = interval == 'hour'
? const Duration(hours: 1)
: const Duration(days: 1);
+ // Limit iterations to prevent excessive queries
+ const maxIterations = 100;
+ var iterations = 0;
+
DateTime current = startDate;
- while (current.isBefore(endDate)) {
+ while (current.isBefore(endDate) && iterations < maxIterations) {
+ iterations++;
final next = current.add(intervalDuration);📝 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.
| static Future<List<List<num>>> getTimeSeriesData({ | |
| required String metric, | |
| required DateTime startDate, | |
| required DateTime endDate, | |
| String interval = 'day', | |
| }) async { | |
| try { | |
| final data = <List<num>>[]; | |
| final intervalDuration = interval == 'hour' | |
| ? const Duration(hours: 1) | |
| : const Duration(days: 1); | |
| DateTime current = startDate; | |
| while (current.isBefore(endDate)) { | |
| final next = current.add(intervalDuration); | |
| int value = 0; | |
| switch (metric) { | |
| case 'users': | |
| final query = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', current) | |
| ..whereLessThan('updatedAt', next); | |
| final result = await query.count(); | |
| value = result.count; | |
| break; | |
| case 'installations': | |
| final query = | |
| QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | |
| ..whereGreaterThan('updatedAt', current) | |
| ..whereLessThan('updatedAt', next); | |
| final result = await query.count(); | |
| value = result.count; | |
| break; | |
| } | |
| data.add([current.millisecondsSinceEpoch, value]); | |
| current = next; | |
| } | |
| return data; | |
| } catch (e) { | |
| if (kDebugMode) { | |
| print('Error getting time series data: $e'); | |
| } | |
| return []; | |
| } | |
| } | |
| static Future<List<List<num>>> getTimeSeriesData({ | |
| required String metric, | |
| required DateTime startDate, | |
| required DateTime endDate, | |
| String interval = 'day', | |
| }) async { | |
| try { | |
| final data = <List<num>>[]; | |
| final intervalDuration = interval == 'hour' | |
| ? const Duration(hours: 1) | |
| : const Duration(days: 1); | |
| // Limit iterations to prevent excessive queries | |
| const maxIterations = 100; | |
| var iterations = 0; | |
| DateTime current = startDate; | |
| while (current.isBefore(endDate) && iterations < maxIterations) { | |
| iterations++; | |
| final next = current.add(intervalDuration); | |
| int value = 0; | |
| switch (metric) { | |
| case 'users': | |
| final query = QueryBuilder<ParseUser>(ParseUser.forQuery()) | |
| ..whereGreaterThan('updatedAt', current) | |
| ..whereLessThan('updatedAt', next); | |
| final result = await query.count(); | |
| value = result.count; | |
| break; | |
| case 'installations': | |
| final query = | |
| QueryBuilder<ParseInstallation>(ParseInstallation.forQuery()) | |
| ..whereGreaterThan('updatedAt', current) | |
| ..whereLessThan('updatedAt', next); | |
| final result = await query.count(); | |
| value = result.count; | |
| break; | |
| } | |
| data.add([current.millisecondsSinceEpoch, value]); | |
| current = next; | |
| } | |
| return data; | |
| } catch (e) { | |
| if (kDebugMode) { | |
| print('Error getting time series data: $e'); | |
| } | |
| return []; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/flutter/lib/src/analytics/parse_analytics.dart around lines 174 to
221, the current while-loop issues an unbounded network request per interval
which can produce hundreds of queries; add a safety cap on iterations (e.g.
compute maxBuckets = min( (endDate - startDate) / intervalDuration, a configured
MAX_BUCKETS like 1000) ) and break/return early if exceeded, and prefer a more
efficient approach: replace per-interval queries with either a single
server-side aggregated query (grouped by interval) if Parse supports
aggregation, or batch the time range into larger chunks (e.g. fetch counts for
N-day windows) and post-aggregate on the client; also consider caching recent
results and surface a clear error or partial-result indicator when the cap is
hit so callers can handle incomplete data.
| mixin ConnectivityHandlerMixin<T extends StatefulWidget> on State<T> { | ||
| // State variables managed by the mixin | ||
| bool _isOffline = false; | ||
| late StreamSubscription<List<ConnectivityResult>> _connectivitySubscription; |
There was a problem hiding this comment.
Potential crash if disposeConnectivityHandler() is called before subscription is assigned.
_connectivitySubscription is declared late and assigned in initConnectivityHandler(). If disposeConnectivityHandler() is called before initialization completes (e.g., widget disposed during async init), calling cancel() will throw a LateInitializationError.
- late StreamSubscription<List<ConnectivityResult>> _connectivitySubscription;
+ StreamSubscription<List<ConnectivityResult>>? _connectivitySubscription;
void disposeConnectivityHandler() {
- _connectivitySubscription.cancel();
+ _connectivitySubscription?.cancel();
}And update the assignment in initConnectivityHandler():
_connectivitySubscription = _connectivity.onConnectivityChanged.listen(...);Also applies to: 56-58
🤖 Prompt for AI Agents
packages/flutter/lib/src/mixins/connectivity_handler_mixin.dart lines 12 (and
similar at 56-58): _connectivitySubscription is declared late and may be
cancelled before it's assigned, causing a LateInitializationError; change its
type to a nullable StreamSubscription<List<ConnectivityResult>>? (remove
`late`), assign it in initConnectivityHandler() as shown (use
_connectivitySubscription = _connectivity.onConnectivityChanged.listen(...)),
and guard cancellations in disposeConnectivityHandler() with a null check (call
cancel() only if the subscription is non-null). Apply the same nullable +
null-check fix to the other subscription declarations and cancels at lines
56-58.
| int _calculateVisibleMaxIndex(double offset) { | ||
| if (!mounted || | ||
| !context.mounted || | ||
| !context.findRenderObject()!.paintBounds.isFinite) { | ||
| return 0; | ||
| } | ||
| try { | ||
| final itemWidth = | ||
| (MediaQuery.of(context).size.width - | ||
| (widget.crossAxisCount - 1) * widget.crossAxisSpacing - | ||
| (widget.padding?.horizontal ?? 0)) / | ||
| widget.crossAxisCount; | ||
| final itemHeight = | ||
| itemWidth / widget.childAspectRatio + widget.mainAxisSpacing; | ||
| if (itemHeight <= 0) return 0; // Avoid division by zero | ||
| final itemsPerRow = widget.crossAxisCount; | ||
| final rowsVisible = | ||
| (offset + MediaQuery.of(context).size.height) / itemHeight; | ||
| return min((rowsVisible * itemsPerRow).ceil(), _items.length - 1); | ||
| } catch (e) { | ||
| debugPrint('$connectivityLogPrefix Error calculating visible index: $e'); | ||
| return _items.isNotEmpty ? _items.length - 1 : 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential null pointer exception with force unwrap.
Line 231 uses context.findRenderObject()! with a force unwrap operator. While context.mounted is checked, this doesn't guarantee findRenderObject() returns non-null. If the widget hasn't been laid out yet, this could throw.
Apply this diff to safely handle the null case:
int _calculateVisibleMaxIndex(double offset) {
if (!mounted ||
!context.mounted ||
- !context.findRenderObject()!.paintBounds.isFinite) {
+ context.findRenderObject()?.paintBounds.isFinite != true) {
return 0;
}📝 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.
| int _calculateVisibleMaxIndex(double offset) { | |
| if (!mounted || | |
| !context.mounted || | |
| !context.findRenderObject()!.paintBounds.isFinite) { | |
| return 0; | |
| } | |
| try { | |
| final itemWidth = | |
| (MediaQuery.of(context).size.width - | |
| (widget.crossAxisCount - 1) * widget.crossAxisSpacing - | |
| (widget.padding?.horizontal ?? 0)) / | |
| widget.crossAxisCount; | |
| final itemHeight = | |
| itemWidth / widget.childAspectRatio + widget.mainAxisSpacing; | |
| if (itemHeight <= 0) return 0; // Avoid division by zero | |
| final itemsPerRow = widget.crossAxisCount; | |
| final rowsVisible = | |
| (offset + MediaQuery.of(context).size.height) / itemHeight; | |
| return min((rowsVisible * itemsPerRow).ceil(), _items.length - 1); | |
| } catch (e) { | |
| debugPrint('$connectivityLogPrefix Error calculating visible index: $e'); | |
| return _items.isNotEmpty ? _items.length - 1 : 0; | |
| } | |
| } | |
| int _calculateVisibleMaxIndex(double offset) { | |
| if (!mounted || | |
| !context.mounted || | |
| context.findRenderObject()?.paintBounds.isFinite != true) { | |
| return 0; | |
| } | |
| try { | |
| final itemWidth = | |
| (MediaQuery.of(context).size.width - | |
| (widget.crossAxisCount - 1) * widget.crossAxisSpacing - | |
| (widget.padding?.horizontal ?? 0)) / | |
| widget.crossAxisCount; | |
| final itemHeight = | |
| itemWidth / widget.childAspectRatio + widget.mainAxisSpacing; | |
| if (itemHeight <= 0) return 0; // Avoid division by zero | |
| final itemsPerRow = widget.crossAxisCount; | |
| final rowsVisible = | |
| (offset + MediaQuery.of(context).size.height) / itemHeight; | |
| return min((rowsVisible * itemsPerRow).ceil(), _items.length - 1); | |
| } catch (e) { | |
| debugPrint('$connectivityLogPrefix Error calculating visible index: $e'); | |
| return _items.isNotEmpty ? _items.length - 1 : 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/flutter/lib/src/utils/parse_live_grid.dart around lines 228 to 251,
the code force-unwraps context.findRenderObject() which can be null even when
context.mounted is true; change the guard to safely handle a null RenderObject
by replacing the force unwrap with a null-aware check (e.g., obtain final render
= context.findRenderObject(); and if render == null ||
!render.paintBounds.isFinite return 0), so that the function returns early on
null or non-finite paintBounds and avoids throwing a null-pointer exception.
| if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { | ||
| final addedItem = event.object; | ||
| setState(() { | ||
| _items.insert(event.index, addedItem); | ||
| }); | ||
| objectToCache = addedItem; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add bounds validation for list insertion.
Line 484 inserts at event.index without validating the index is within valid bounds [0, _items.length]. While the Parse SDK should provide valid indices, defensive programming suggests validating before insertion to prevent RangeError exceptions.
Apply this diff to add bounds validation:
if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) {
final addedItem = event.object;
- setState(() {
- _items.insert(event.index, addedItem);
- });
+ if (event.index >= 0 && event.index <= _items.length) {
+ setState(() {
+ _items.insert(event.index, addedItem);
+ });
+ } else {
+ debugPrint(
+ '$connectivityLogPrefix LiveList Add Event: Invalid index ${event.index}, list size ${_items.length}',
+ );
+ }
objectToCache = addedItem;📝 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 (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { | |
| final addedItem = event.object; | |
| setState(() { | |
| _items.insert(event.index, addedItem); | |
| }); | |
| objectToCache = addedItem; | |
| if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { | |
| final addedItem = event.object; | |
| if (event.index >= 0 && event.index <= _items.length) { | |
| setState(() { | |
| _items.insert(event.index, addedItem); | |
| }); | |
| } else { | |
| debugPrint( | |
| '$connectivityLogPrefix LiveList Add Event: Invalid index ${event.index}, list size ${_items.length}', | |
| ); | |
| } | |
| objectToCache = addedItem; |
🤖 Prompt for AI Agents
In packages/flutter/lib/src/utils/parse_live_grid.dart around lines 481 to 486,
the code inserts an item at event.index without validating the index; add a
bounds check to ensure event.index is between 0 and _items.length inclusive
before calling _items.insert. If the index is out of range, either clamp it to a
valid value (e.g., 0 or _items.length), or skip the insertion and optionally
log/debug a warning; perform the validation inside setState so you never call
insert with an invalid index.
| int _currentPage = 0; | ||
| bool _hasMoreData = true; | ||
|
|
||
| final Set<int> _loadingIndices = {}; // Used for lazy loading specific items |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused _loadingIndices field.
The _loadingIndices set is declared but never populated or read anywhere in the code. This appears to be dead code.
- final Set<int> _loadingIndices = {}; // Used for lazy loading specific items📝 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.
| final Set<int> _loadingIndices = {}; // Used for lazy loading specific items |
🤖 Prompt for AI Agents
In packages/flutter/lib/src/utils/parse_live_sliver_grid.dart around line 126,
the private field "final Set<int> _loadingIndices = {};" is unused dead code;
remove this field declaration from the file and run the Dart analyzer/formatter
to ensure no references remain and to update import/unused warnings.
| liveGrid.stream.listen( | ||
| (event) { | ||
| if (!mounted) return; | ||
|
|
||
| T? objectToCache; | ||
|
|
||
| try { | ||
| if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { | ||
| final addedItem = event.object; | ||
| setState(() { | ||
| _items.insert(event.index, addedItem); | ||
| }); | ||
| objectToCache = addedItem; | ||
| } else if (event is sdk.ParseLiveListDeleteEvent<sdk.ParseObject>) { | ||
| if (event.index >= 0 && event.index < _items.length) { | ||
| final removedItem = _items.removeAt(event.index); | ||
| setState(() {}); | ||
| if (widget.offlineMode) { | ||
| removedItem.removeFromLocalCache().catchError((e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error removing item ${removedItem.objectId} from cache: $e', | ||
| ); | ||
| }); | ||
| } | ||
| } | ||
| } else if (event is sdk.ParseLiveListUpdateEvent<sdk.ParseObject>) { | ||
| final updatedItem = event.object; | ||
| if (event.index >= 0 && event.index < _items.length) { | ||
| setState(() { | ||
| _items[event.index] = updatedItem; | ||
| }); | ||
| objectToCache = updatedItem; | ||
| } | ||
| } | ||
|
|
||
| if (widget.offlineMode && objectToCache != null) { | ||
| objectToCache.saveToLocalCache().catchError((e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error saving stream update for ${objectToCache?.objectId} to cache: $e', | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| _noDataNotifier.value = _items.isEmpty; | ||
| } catch (e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error processing stream event: $e', | ||
| ); | ||
| } | ||
| }, | ||
| onError: (error) { | ||
| debugPrint('$connectivityLogPrefix LiveList Stream Error: $error'); | ||
| if (mounted) { | ||
| setState(() {}); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Store and cancel the stream subscription to prevent memory leaks.
Same issue as in ParseLiveSliverListWidget - the stream listener subscription is not stored or cancelled, leading to potential memory leaks and duplicate event processing on refresh.
Apply the same fix pattern as suggested for the sliver list widget: store the subscription and cancel it in dispose() and before creating a new one.
🤖 Prompt for AI Agents
In packages/flutter/lib/src/utils/parse_live_sliver_grid.dart around lines 391
to 447, the StreamSubscription returned by liveGrid.stream.listen is not saved
or cancelled which causes memory leaks and duplicate event handling; store the
subscription in a field (e.g. _liveGridSubscription), assign it when calling
listen, cancel the existing subscription before creating a new one, and ensure
you cancel it in the widget's dispose() method (and null it after cancelling) so
the listener is properly cleaned up on refresh/unmount.
| liveList.stream.listen( | ||
| (event) { | ||
| if (!mounted) return; // Avoid processing if widget is disposed | ||
|
|
||
| T? objectToCache; // For single item cache updates from stream | ||
|
|
||
| try { | ||
| // Wrap event processing in try-catch | ||
| if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { | ||
| final addedItem = event.object; | ||
| setState(() { | ||
| _items.insert(event.index, addedItem); | ||
| }); | ||
| objectToCache = addedItem; | ||
| } else if (event is sdk.ParseLiveListDeleteEvent<sdk.ParseObject>) { | ||
| if (event.index >= 0 && event.index < _items.length) { | ||
| final removedItem = _items.removeAt(event.index); | ||
| setState(() {}); | ||
| if (widget.offlineMode) { | ||
| // Remove deleted item from cache immediately | ||
| removedItem.removeFromLocalCache().catchError((e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error removing item ${removedItem.objectId} from cache: $e', | ||
| ); | ||
| }); | ||
| } | ||
| } else { | ||
| debugPrint( | ||
| '$connectivityLogPrefix LiveList Delete Event: Invalid index ${event.index}, list size ${_items.length}', | ||
| ); | ||
| } | ||
| } else if (event is sdk.ParseLiveListUpdateEvent<sdk.ParseObject>) { | ||
| final updatedItem = event.object; | ||
| if (event.index >= 0 && event.index < _items.length) { | ||
| setState(() { | ||
| _items[event.index] = updatedItem; | ||
| }); | ||
| objectToCache = updatedItem; | ||
| } else { | ||
| debugPrint( | ||
| '$connectivityLogPrefix LiveList Update Event: Invalid index ${event.index}, list size ${_items.length}', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Save single updates from stream immediately if offline mode is on | ||
| if (widget.offlineMode && objectToCache != null) { | ||
| objectToCache.saveToLocalCache().catchError((e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error saving stream update for ${objectToCache?.objectId} to cache: $e', | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| _noDataNotifier.value = _items.isEmpty; | ||
| } catch (e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error processing stream event: $e', | ||
| ); | ||
| } | ||
| }, | ||
| onError: (error) { | ||
| debugPrint('$connectivityLogPrefix LiveList Stream Error: $error'); | ||
| if (mounted) { | ||
| setState(() { | ||
| /* Potentially update state to show error */ | ||
| }); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Store and cancel the stream subscription to prevent memory leaks.
The stream listener is created but the subscription is never stored or cancelled in dispose(). If _loadData() is called multiple times (e.g., on refresh), each call creates a new listener without canceling the previous one, leading to duplicate event processing and memory leaks.
class ParseLiveSliverListWidgetState<T extends sdk.ParseObject>
extends State<ParseLiveSliverListWidget<T>>
with ConnectivityHandlerMixin<ParseLiveSliverListWidget<T>> {
CachedParseLiveList<T>? _liveList;
final ValueNotifier<bool> _noDataNotifier = ValueNotifier<bool>(true);
final List<T> _items = <T>[];
+ StreamSubscription<sdk.ParseLiveListEvent<sdk.ParseObject>>? _streamSubscription;
LoadMoreStatus _loadMoreStatus = LoadMoreStatus.idle;Then store and cancel the subscription:
- liveList.stream.listen(
+ _streamSubscription?.cancel();
+ _streamSubscription = liveList.stream.listen(
(event) {And in dispose():
@override
void dispose() {
disposeConnectivityHandler();
+ _streamSubscription?.cancel();
_liveList?.dispose();
_noDataNotifier.dispose();
super.dispose();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/flutter/lib/src/utils/parse_live_sliver_list.dart around lines 282
to 351, the stream.listen subscription is created but never stored or cancelled,
causing duplicate listeners and memory leaks when _loadData() is called multiple
times; add a class-level StreamSubscription<T>? _liveListSub field, before
creating a new subscription cancel any existing _liveListSub?.cancel(), assign
the returned subscription to _liveListSub when calling
liveList.stream.listen(...), and override dispose() to await/ cancel
_liveListSub and set it to null so the listener is properly cleaned up.
| try { | ||
| // Wrap event processing in try-catch | ||
| if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) { | ||
| final addedItem = event.object; | ||
| setState(() { | ||
| _items.insert(event.index, addedItem); | ||
| }); | ||
| objectToCache = addedItem; | ||
| } else if (event is sdk.ParseLiveListDeleteEvent<sdk.ParseObject>) { | ||
| if (event.index >= 0 && event.index < _items.length) { | ||
| final removedItem = _items.removeAt(event.index); | ||
| setState(() {}); | ||
| if (widget.offlineMode) { | ||
| // Remove deleted item from cache immediately | ||
| removedItem.removeFromLocalCache().catchError((e) { | ||
| debugPrint( | ||
| '$connectivityLogPrefix Error removing item ${removedItem.objectId} from cache: $e', | ||
| ); | ||
| }); | ||
| } | ||
| } else { | ||
| debugPrint( | ||
| '$connectivityLogPrefix LiveList Delete Event: Invalid index ${event.index}, list size ${_items.length}', | ||
| ); | ||
| } | ||
| } else if (event is sdk.ParseLiveListUpdateEvent<sdk.ParseObject>) { | ||
| final updatedItem = event.object; | ||
| if (event.index >= 0 && event.index < _items.length) { | ||
| setState(() { | ||
| _items[event.index] = updatedItem; | ||
| }); | ||
| objectToCache = updatedItem; | ||
| } else { | ||
| debugPrint( | ||
| '$connectivityLogPrefix LiveList Update Event: Invalid index ${event.index}, list size ${_items.length}', | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe type cast from ParseObject to T in stream event handling.
The stream event's object property is typed as sdk.ParseObject, but it's being inserted directly into _items which is List<T>. While Dart's type system may allow this at compile time due to covariance, this could cause issues if the runtime type doesn't match.
Consider explicit casting with type checking:
if (event is sdk.ParseLiveListAddEvent<sdk.ParseObject>) {
- final addedItem = event.object;
+ final addedItem = event.object as T;
setState(() {
_items.insert(event.index, addedItem);
});This makes the cast explicit and will fail fast with a clear error if there's a type mismatch.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/flutter/lib/src/utils/parse_live_sliver_list.dart around lines 288
to 325, the code inserts sdk.ParseObject instances directly into _items
(List<T>) which is an unsafe runtime cast; update the event handling to perform
an explicit type check and cast before mutating _items and setting
objectToCache: for each branch that reads event.object, verify event.object is T
(using `is T`), then cast to T and use that value; if the check fails, log a
clear debug/error message and skip inserting/updating/caching to avoid runtime
type errors.
| group('dispose', () { | ||
| test('should dispose resources without throwing', () { | ||
| // Act & Assert - should not throw (dispose is synchronous void) | ||
| expect(() => ParseAnalytics.dispose(), returnsNormally); | ||
|
|
||
| // Re-initialize for other tests | ||
| ParseAnalytics.initialize(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing await for ParseAnalytics.initialize() after dispose.
On line 118, ParseAnalytics.initialize() is called without await. Since initialize() is async, subsequent tests in the suite may start before initialization completes, potentially causing flaky test failures.
test('should dispose resources without throwing', () {
// Act & Assert - should not throw (dispose is synchronous void)
expect(() => ParseAnalytics.dispose(), returnsNormally);
// Re-initialize for other tests
- ParseAnalytics.initialize();
+ // Note: This runs after tearDown, consider moving to a separate test
+ // or making this test async and awaiting
});Alternatively, if re-initialization is needed for cleanup, make the test async:
- test('should dispose resources without throwing', () {
+ test('should dispose resources without throwing', () async {
expect(() => ParseAnalytics.dispose(), returnsNormally);
- ParseAnalytics.initialize();
+ await ParseAnalytics.initialize();
});📝 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.
| group('dispose', () { | |
| test('should dispose resources without throwing', () { | |
| // Act & Assert - should not throw (dispose is synchronous void) | |
| expect(() => ParseAnalytics.dispose(), returnsNormally); | |
| // Re-initialize for other tests | |
| ParseAnalytics.initialize(); | |
| }); | |
| }); | |
| group('dispose', () { | |
| test('should dispose resources without throwing', () async { | |
| // Act & Assert - should not throw (dispose is synchronous void) | |
| expect(() => ParseAnalytics.dispose(), returnsNormally); | |
| // Re-initialize for other tests | |
| await ParseAnalytics.initialize(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In packages/flutter/test/src/analytics/parse_analytics_test.dart around lines
112 to 120, the test calls ParseAnalytics.initialize() after disposing without
awaiting it; make the test async and await the re-initialization to ensure
initialization completes before other tests run. Change the test signature to
async (e.g., test('...', () async { ... })) and call await
ParseAnalytics.initialize(); after the dispose assertion so re-initialization is
complete before the suite continues.
- Add CoreStore tests for getStringList method and basic operations - Add ParseAnalyticsEndpoints tests for audience, analytics, retention handlers - Add ParseLiveListElementSnapshot and LoadMoreStatus tests - Improves test coverage for PR parse-community#1101 offline mode support
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/dart/test/src/storage/core_store_test.dart (1)
12-109: getStringList tests are thorough; consider adding coverage for Iterable coercionThe
CoreStore getStringListgroup nicely covers missing keys, empty lists, special characters, Unicode, overwrite behavior, long lists, and non-list values returningnull. This matches the intended semantics and will catch most regressions.Given the updated
getStringListlogic also handles genericIterablevalues (per the PR description/summary), you may want to add one or two tests that explicitly exercise that coercion path (e.g., when underlying storage returns anIterableinstead of aList<String>), so future changes to storage backends don’t silently regress this behavior.packages/flutter/test/src/utils/parse_cached_live_list_test.dart (1)
42-73: Simplify redundant enum tests.The LoadMoreStatus tests are overly verbose with significant redundancy. Lines 51-65 check array positions individually, which duplicates the validation already done in lines 43-48.
Consider consolidating to a single, more maintainable test:
group('LoadMoreStatus', () { - test('should have all expected enum values', () { - expect(LoadMoreStatus.values.length, 4); - expect(LoadMoreStatus.idle, isNotNull); - expect(LoadMoreStatus.loading, isNotNull); - expect(LoadMoreStatus.noMoreData, isNotNull); - expect(LoadMoreStatus.error, isNotNull); - }); - - test('idle should be first value', () { - expect(LoadMoreStatus.values[0], LoadMoreStatus.idle); - }); - - test('loading should be second value', () { - expect(LoadMoreStatus.values[1], LoadMoreStatus.loading); - }); - - test('noMoreData should be third value', () { - expect(LoadMoreStatus.values[2], LoadMoreStatus.noMoreData); - }); - - test('error should be fourth value', () { - expect(LoadMoreStatus.values[3], LoadMoreStatus.error); - }); - - test('enum values should have correct names', () { + test('should have all expected enum values with correct names', () { + expect(LoadMoreStatus.values.length, 4); + expect(LoadMoreStatus.values, [ + LoadMoreStatus.idle, + LoadMoreStatus.loading, + LoadMoreStatus.noMoreData, + LoadMoreStatus.error, + ]); expect(LoadMoreStatus.idle.name, 'idle'); expect(LoadMoreStatus.loading.name, 'loading'); expect(LoadMoreStatus.noMoreData.name, 'noMoreData'); expect(LoadMoreStatus.error.name, 'error'); }); });packages/flutter/test/src/analytics/parse_analytics_endpoints_test.dart (4)
22-112: Audience endpoint tests are thorough; consider a small helper to reduce duplicationThe individual tests for each audience type plus the unknown-type case give good coverage of the API surface. The repeated pattern for the successful cases (
isA<Map<String, dynamic>>(),'total'and'content'keys) could be centralized into a private helper to keep this suite easier to extend.For example, you could introduce a helper (outside the group):
Future<void> _expectAudienceResponse(String type) async { final result = await ParseAnalyticsEndpoints.handleAudienceRequest(type); expect(result, isA<Map<String, dynamic>>()); expect(result.containsKey('total'), isTrue); expect(result.containsKey('content'), isTrue); }And then simplify one of the tests within this range:
- test('should handle total_users request', () async { - final result = await ParseAnalyticsEndpoints.handleAudienceRequest( - 'total_users', - ); - - expect(result, isA<Map<String, dynamic>>()); - expect(result.containsKey('total'), isTrue); - expect(result.containsKey('content'), isTrue); - }); + test('should handle total_users request', () async { + await _expectAudienceResponse('total_users'); + });The same pattern can be applied to the remaining audience-type tests.
114-162: Analytics endpoint tests cover main permutations; optional: add a failure/unknown-path caseThe tests here nicely exercise multiple endpoints (
audience,installations, custom) and both daily and hourly intervals, and assert the expected response envelope viarequested_data. If the implementation distinguishes invalid endpoints or intervals, you might add one more test to lock in that behavior:+ test('should handle unknown endpoint gracefully', () async { + final result = await ParseAnalyticsEndpoints.handleAnalyticsRequest( + endpoint: 'unknown', + startDate: DateTime.now().subtract(const Duration(days: 7)), + endDate: DateTime.now(), + interval: 'day', + ); + + expect(result, isA<Map<String, dynamic>>()); + // Optionally assert on a specific field if the implementation exposes one. + });This would give you a regression check if behavior for unsupported endpoints ever changes.
186-221: Billing tests are well-structured; you may optionally assert numeric typesThe billing tests for storage, database, and data transfer do a good job of validating the response shape and the
unitsvalues (GB/TB). If you want slightly stronger guarantees, you could also assert thattotalandlimitare numeric (e.g.,isA<num>()), to catch accidental stringification:expect(result['total'], isA<num>()); expect(result['limit'], isA<num>());That’s purely optional but would make these tests catch more regressions if the implementation changes.
223-258: Slow-queries tests cover parameters; consider asserting more on the filtered caseThe slow-queries tests exercise the no-parameter,
className-only, and “all parameters” variants, which is good functional coverage. For the final test, you can get a bit more value by asserting on the returned data, not just non-emptiness:test('should return slow queries list with all parameters', () { final result = ParseAnalyticsEndpoints.handleSlowQueriesRequest( className: 'TestClass', os: 'iOS', version: '1.0.0', from: DateTime.now().subtract(const Duration(days: 7)), to: DateTime.now(), ); expect(result, isA<List<Map<String, dynamic>>>()); expect(result.isNotEmpty, isTrue); + expect(result.first['className'], 'TestClass'); + // Optionally: assert os/version if they’re reflected in the payload. });That keeps the test aligned with the intention of those extra filters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/dart/test/src/storage/core_store_test.dart(1 hunks)packages/flutter/test/src/analytics/parse_analytics_endpoints_test.dart(1 hunks)packages/flutter/test/src/utils/parse_cached_live_list_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Dart 3.10, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Flutter 3.38, Ubuntu
🔇 Additional comments (10)
packages/dart/test/src/storage/core_store_test.dart (3)
1-10: Test setup correctly isolates CoreStoreMemoryImp per testUsing a shared
late CoreStoreMemoryImpwithsetUpthat instantiates and clears before each test gives good isolation and keeps tests deterministic. No changes needed.
111-166: Good coverage of basic CoreStore primitives and key lifecycleThe basic operations group validates all the core primitives (string, int, double, bool), existence checks, removal, and clearing. This is a solid smoke test suite for
CoreStoreMemoryImpand should help quickly flag regressions in the in-memory implementation.
168-198: Edge-case tests nicely complement the main suitesThe null-return checks across all getters and the “special key names” loop are useful edge cases that often get missed. They should help ensure both API contracts and key-encoding remain stable across refactors and backend implementations. No changes needed.
packages/flutter/test/src/utils/parse_cached_live_list_test.dart (3)
6-18: LGTM! Test helper class follows Parse SDK patterns.The
TestCacheObjectimplementation correctly extendsParseObjectand implementsParseCloneablewith appropriate factory methods.
20-36: LGTM! Test setup is appropriate.The test initialization correctly sets up Flutter test bindings, mock SharedPreferences, and Parse configuration for the test environment.
104-165: Add documentation toParseLiveListElementSnapshotexplaining the lazy loading pattern.The
hasDatavshasPreLoadedDatadistinction is intentional and part of the lazy loading architecture:preLoadedDatacontains the initial query result (with onlypreloadedColumnsfetched), whileloadedDatacontains the fully loaded object after lazy loading completes. However, this semantic difference is documented inParseLiveListbut not inParseLiveListElementSnapshotitself, making it unclear for API users.Consider adding a class-level documentation comment to
ParseLiveListElementSnapshotexplaining:
- The purpose of each property in the context of lazy loading
- When
hasDatavshasPreLoadedDatashould be checked in UI code- A reference to
ParseLiveList'slazyLoadingandpreloadedColumnsparameterspackages/flutter/test/src/analytics/parse_analytics_endpoints_test.dart (4)
5-20: Parse test setup and initialization look correctUsing
TestWidgetsFlutterBinding.ensureInitialized(), mockSharedPreferences, and a singlesetUpAllParse().initialize(...)call is a clean pattern for these analytics tests and should avoid state leaks between tests.
164-184: Retention tests validate both default and cohort-specific flowsThe retention tests are clear and lightweight, and asserting both the map type and the presence of
day1,day7, andday30for with/withoutcohortDategives good signal without over-constraining implementation details.
261-270: Express middleware snippet assertions look appropriateThe
getExpressMiddlewaretest checks that the generated string contains the middleware identifier, master-key header, and the audience content key, which are the important integration points; this is a good balance between robustness and flexibility in how the snippet is formatted.
272-290: Dart Shelf handler snippet tests are comprehensiveThese tests assert that the generated handler string includes its own entrypoint name, expected analytics keys,
Response.notFound, and the essentialdart:convert/shelfimports. That gives strong guarantees the sample code doesn’t regress structurally while still allowing internal formatting changes.
| group('ChildBuilder typedef', () { | ||
| test('should accept widget builder function with optional index', () { | ||
| // Test that ChildBuilder<T> works with the expected signature | ||
| Widget testBuilder( | ||
| dynamic context, | ||
| ParseLiveListElementSnapshot<TestCacheObject> snapshot, [ | ||
| int? index, | ||
| ]) { | ||
| return const SizedBox(); | ||
| } | ||
|
|
||
| expect(testBuilder, isA<Function>()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix type safety: use BuildContext instead of dynamic.
Line 79 uses dynamic for the context parameter, which bypasses type checking. Since this is a widget builder, it should use BuildContext.
Apply this diff:
test('should accept widget builder function with optional index', () {
// Test that ChildBuilder<T> works with the expected signature
Widget testBuilder(
- dynamic context,
+ BuildContext context,
ParseLiveListElementSnapshot<TestCacheObject> snapshot, [
int? index,
]) {
return const SizedBox();
}
expect(testBuilder, isA<Function>());
});Additionally, consider whether this test adds sufficient value. It only verifies that a function exists, not that it matches the typedef signature. A stronger test would verify the typedef can be assigned:
test('should match ChildBuilder typedef signature', () {
ChildBuilder<TestCacheObject> builder = (context, snapshot, [index]) {
return const SizedBox();
};
expect(builder, isNotNull);
});| group('FooterBuilder typedef', () { | ||
| test('should accept widget builder function', () { | ||
| Widget testFooterBuilder( | ||
| dynamic context, | ||
| LoadMoreStatus status, | ||
| void Function()? onRetry, | ||
| ) { | ||
| return const SizedBox(); | ||
| } | ||
|
|
||
| expect(testFooterBuilder, isA<Function>()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix type safety: use BuildContext instead of dynamic.
Line 93 uses dynamic for the context parameter, which bypasses type checking. This should be BuildContext for widget builders.
Apply this diff:
test('should accept widget builder function', () {
Widget testFooterBuilder(
- dynamic context,
+ BuildContext context,
LoadMoreStatus status,
void Function()? onRetry,
) {
return const SizedBox();
}
expect(testFooterBuilder, isA<Function>());
});As with the ChildBuilder test, consider strengthening this by directly assigning to the typedef:
test('should match FooterBuilder typedef signature', () {
FooterBuilder builder = (context, status, onRetry) {
return const SizedBox();
};
expect(builder, isNotNull);
});🤖 Prompt for AI Agents
In packages/flutter/test/src/utils/parse_cached_live_list_test.dart around lines
90 to 102, the FooterBuilder test uses a parameter typed as dynamic for the
context which bypasses type safety; change the context parameter type from
dynamic to BuildContext, and optionally replace the current test with one that
assigns a closure to FooterBuilder (e.g., FooterBuilder builder = (context,
status, onRetry) => const SizedBox();) then assert the builder is not null to
ensure the signature matches the typedef.
1 participant
Summary
This PR adds comprehensive offline mode support and introduces three new live query widgets for displaying Parse objects in Flutter applications.
New Features
New Widgets
Offline Mode Support
ParseObjectOfflineextension methodsCachedParseLiveListNew API
offlineModeparameter on all live query widgetsrefreshData()andloadMoreData()public methods on sliver widget statesConnectivityHandlerMixinfor consistent offline handling across widgetsChanges
New Files (Flutter Package)
lib/src/utils/parse_live_sliver_list.dartlib/src/utils/parse_live_sliver_grid.dartlib/src/utils/parse_live_page_view.dartlib/src/utils/parse_cached_live_list.dartlib/src/mixins/connectivity_handler_mixin.dartNew Files (Dart Package)
lib/src/objects/parse_offline_object.dartDocumentation
Testing
Breaking Changes
None - fully backward compatible
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.