Add dynamic tree map with geohash-based efficient fetching #29
Conversation
- Add ExploreTreesMapPage with interactive flutter_map integration - Implement GeohashService for efficient spatial queries and tree clustering - Add TreeMapService for blockchain data fetching with geohash caching - Create NearbyTreesWidget showing trees around user's location - Add MapFilterWidget with species, status, and date filters - Add MapSearchWidget for searching trees by ID, species, or coordinates - Add TreeHeatmapLayer for density visualization - Update home page with quick actions and nearby trees section - Update trees page with 'Explore Map' button - Add route for /explore-map Features: - Geohash-based spatial indexing for efficient tree queries - Tree clustering at lower zoom levels for better performance - Real-time location tracking with user position marker - Filter trees by alive/deceased status, species, care count - Search by tree ID, species name, geohash, or coordinates - Responsive tree detail panel with quick navigation - Pagination support for loading large datasets - Cache management for optimized data fetching
- Add proper paint() implementation with radial gradient circles - Use MapCamera for coordinate-to-screen conversion - Add additive blending for overlapping heat points - Filter points to only render visible area for performance - Update shouldRepaint to check camera position changes
- Add mounted check at start of _loadNearbyTrees - Add mounted check after getCurrentLocationWithTimeout await - Add mounted check after getTreesNearLocation await - Add mounted check in catch block before setState - Prevents setState on unmounted widget errors
- Use dart_geohash native neighbor() function instead of custom delta-based calculation - Add O(1) duplicate check using Set for tree cache (fix O(n×m) performance issue) - Add sentinel pattern in MapFilterOptions.copyWith to allow clearing nullable fields - Fix unsafe substring call for short geohashes in tree details panel - Fix map controller race condition with _mapReady flag and pending center - Remove unused _geohashService field from ExploreTreesMapPage
Prevents accessing _mapController.camera before map is initialized, avoiding potential errors when called from _loadTrees() before _onMapReady()
|
Warning Rate limit exceeded@kartikeyg0104 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a new Explore Trees map feature: route and page, map-backed tree fetching and clustering services, geohash utilities, map UI widgets (filters, search, heatmap, nearby trees), and integrations from home/trees/recent widgets to navigate to the map. Changes
sequenceDiagram
participant User
participant ExploreMapPage
participant WalletProvider
participant TreeMapService
participant GeohashService
participant Blockchain
User->>ExploreMapPage: Open /explore-map
ExploreMapPage->>WalletProvider: Check connection
alt wallet not connected
ExploreMapPage->>User: Show connect-wallet prompt
else wallet connected
ExploreMapPage->>TreeMapService: fetchTreesInBounds(SW,NE,zoom)
TreeMapService->>GeohashService: getGeohashesInBounds(SW,NE,precision)
GeohashService-->>TreeMapService: geohash prefixes
TreeMapService->>TreeMapService: check cache for prefixes
loop for uncached prefixes
TreeMapService->>Blockchain: fetch trees for prefix(es)
Blockchain-->>TreeMapService: raw tree data
TreeMapService->>TreeMapService: convert, cache by geohash
end
TreeMapService-->>ExploreMapPage: List<MapTreeData>
ExploreMapPage->>TreeMapService: clusterTrees(trees, zoom)
TreeMapService-->>ExploreMapPage: List<TreeCluster>
ExploreMapPage->>User: Render clusters/markers, heatmap, overlays
end
User->>ExploreMapPage: Pan/zoom or apply filter/search
ExploreMapPage->>TreeMapService: fetch/cluster as needed
TreeMapService-->>ExploreMapPage: Updated trees/clusters
ExploreMapPage->>User: Updated map display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
lib/widgets/nft_display_utils/recent_trees_widget.dart (1)
375-376: Consider using RouteConstants for maintainability.The route is hardcoded as '/explore-map'. For consistency and maintainability, consider using
RouteConstants.exploreMapPathinstead, which was added in this PR.- // Navigate to explore map page - context.push('/explore-map'); + context.push(RouteConstants.exploreMapPath);lib/pages/home_page.dart (1)
75-103: Use RouteConstants for route paths.The route paths are hardcoded as strings. For consistency and maintainability, use the
RouteConstantsdefined in the PR.child: _buildActionButton( context, icon: Icons.map, label: 'Explore Map', - onTap: () => context.push('/explore-map'), + onTap: () => context.push(RouteConstants.exploreMapPath), isPrimary: true, ), ), const SizedBox(width: 12), Expanded( child: _buildActionButton( context, icon: Icons.forest, label: 'All Trees', - onTap: () => context.push('/trees'), + onTap: () => context.push(RouteConstants.allTreesPath), isPrimary: false, ), ),lib/pages/trees_page.dart (1)
84-112: Use RouteConstants for the route path.The route path is hardcoded. For consistency with the rest of the codebase, use
RouteConstants.exploreMapPath.onPressed: () { - context.push('/explore-map'); + context.push(RouteConstants.exploreMapPath); },lib/widgets/map_widgets/nearby_trees_widget.dart (2)
217-236: Use RouteConstants for navigation.The routes are hardcoded. For consistency, use the appropriate RouteConstants.
ElevatedButton.icon( - onPressed: () => context.push('/mint-nft'), + onPressed: () => context.push(RouteConstants.mintNftPath), icon: const Icon(Icons.add), label: const Text('Plant Tree'), style: ElevatedButton.styleFrom( backgroundColor: getThemeColors(context)['primary'], foregroundColor: Colors.white, ), ), const SizedBox(width: 12), OutlinedButton.icon( - onPressed: () => context.push('/explore-map'), + onPressed: () => context.push(RouteConstants.exploreMapPath), icon: const Icon(Icons.map), label: const Text('Explore Map'), ),
266-274: Use RouteConstants for the route path.For consistency with other navigation calls, use
RouteConstants.exploreMapPath.TextButton( - onPressed: () => context.push('/explore-map'), + onPressed: () => context.push(RouteConstants.exploreMapPath), child: Text( 'View All', style: TextStyle( color: getThemeColors(context)['primary'], ), ), ),lib/pages/explore_trees_map_page.dart (5)
55-71: Add mounted check before initial setState and surface initialization errors.The first
setStateat line 56 executes synchronously ininitStatecontext so it's safe, but the catch block silently swallows errors without setting_errorMessage, leaving users unaware of initialization failures.} catch (e) { logger.e('Error initializing map: $e'); + if (mounted) { + setState(() { + _errorMessage = 'Failed to initialize map: $e'; + }); + } } finally {
220-231: Shallow equality check may miss content changes.The length-based comparison at line 228 won't detect when species names change but count remains the same. Consider using
listEqualsor comparing contents if this edge case matters.- if (_availableSpecies.length != species.length) { - _availableSpecies = species; - } + // Using simple assignment; list comparison overhead not worth it for this use case + _availableSpecies = species;
254-274: Consider wrapping pagination call in try-catch for robustness.
_onMapMoveis an async method called from the map'sonPositionChangedcallback. WhilefetchAllTreeshas internal error handling, exceptions fromProvider.ofor state updates could go unhandled.Future<void> _onMapMove() async { _updateVisibleTrees(); // Load more trees if needed if (!_isLoadingMore && _treeMapService.allTrees.length < _treeMapService.totalTreeCount) { - setState(() => _isLoadingMore = true); - - final walletProvider = Provider.of<WalletProvider>(context, listen: false); - await _treeMapService.fetchAllTrees( - walletProvider: walletProvider, - offset: _treeMapService.allTrees.length, - limit: 50, - ); - - _updateVisibleTrees(); - - if (mounted) { - setState(() => _isLoadingMore = false); + try { + setState(() => _isLoadingMore = true); + + final walletProvider = Provider.of<WalletProvider>(context, listen: false); + await _treeMapService.fetchAllTrees( + walletProvider: walletProvider, + offset: _treeMapService.allTrees.length, + limit: 50, + ); + + _updateVisibleTrees(); + } finally { + if (mounted) { + setState(() => _isLoadingMore = false); + } } } }
380-385: AddconsttoTextStylefor minor optimization.Text( 'Loading trees...', - style: TextStyle( + style: const TextStyle( color: Colors.white, fontSize: 16, ),
670-687: Zoom controls bypass map-ready guard.Lines 675 and 685 directly call
_mapController.move()without checking_mapReady, which could throw if triggered before the map is initialized. Consider guarding or using_moveMapTowith current center._buildControlButton( context, icon: Icons.add, onTap: () { + if (!_mapReady) return; final newZoom = (_currentZoom + 1).clamp(3.0, 18.0); _mapController.move(_mapController.camera.center, newZoom); }, ), const SizedBox(height: 8), // Zoom out _buildControlButton( context, icon: Icons.remove, onTap: () { + if (!_mapReady) return; final newZoom = (_currentZoom - 1).clamp(3.0, 18.0); _mapController.move(_mapController.camera.center, newZoom); }, ),lib/widgets/map_widgets/map_filter_widget.dart (1)
83-94: Consider syncing with parent wheninitialOptionschanges.If the parent widget updates
initialOptions(e.g., resetting filters externally),_optionswon't update since it's only set ininitState. AdddidUpdateWidgetif external resets are expected.@override void initState() { super.initState(); _options = widget.initialOptions; } + @override + void didUpdateWidget(MapFilterWidget oldWidget) { + super.didUpdateWidget(oldWidget); + if (widget.initialOptions != oldWidget.initialOptions) { + _options = widget.initialOptions; + } + } + void _updateOptions(MapFilterOptions newOptions) {lib/widgets/map_widgets/tree_heatmap_layer.dart (1)
109-116: Consider comparing tree list identity for more robust repaint detection.
shouldRepaintcompares onlytrees.length, which could miss repaint when trees are replaced with a same-sized list. If this causes stale rendering, compare list identity or use a version counter.@override bool shouldRepaint(covariant _HeatmapPainter oldDelegate) { - return trees.length != oldDelegate.trees.length || + return trees != oldDelegate.trees || camera.zoom != oldDelegate.camera.zoom || camera.center != oldDelegate.camera.center || opacity != oldDelegate.opacity || radius != oldDelegate.radius; }lib/services/tree_map_service.dart (2)
246-263: Comment claims O(n) but implementation is O(n×m).The nested loop at lines 256-262 iterates over both trees and geohashes, making it O(n×m) rather than the claimed "O(n) instead of O(n×m)". The performance is acceptable since m (geohashes in view) is typically small, but the comment is misleading.
- // Convert geohashes to Set for O(1) prefix matching - final geohashSet = geohashes.toSet(); - - // Single pass over all trees - O(n) instead of O(n×m) + // Single pass over trees, checking against each requested geohash + // O(n×m) where m is typically small (visible geohashes) for (final tree in _allTrees) {
315-316:neighborGeohashesis computed but never used.The neighbor geohashes are calculated at line 316 but not utilized for filtering or cache lookup, making this dead code. Either remove it or use it to optimize the search.
Future<List<MapTreeData>> getTreesNearLocation({ required WalletProvider walletProvider, required double latitude, required double longitude, double radiusMeters = 5000, }) async { - final centerGeohash = _geohashService.encode(latitude, longitude); - final neighborGeohashes = _geohashService.getNeighbors(centerGeohash); // Ensure we have trees loaded if (_allTrees.isEmpty) { await fetchAllTrees(walletProvider: walletProvider, limit: 100); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/main.dart(2 hunks)lib/pages/explore_trees_map_page.dart(1 hunks)lib/pages/home_page.dart(3 hunks)lib/pages/trees_page.dart(1 hunks)lib/services/geohash_service.dart(1 hunks)lib/services/tree_map_service.dart(1 hunks)lib/utils/constants/route_constants.dart(1 hunks)lib/widgets/map_widgets/map_filter_widget.dart(1 hunks)lib/widgets/map_widgets/map_search_widget.dart(1 hunks)lib/widgets/map_widgets/nearby_trees_widget.dart(1 hunks)lib/widgets/map_widgets/tree_heatmap_layer.dart(1 hunks)lib/widgets/nft_display_utils/recent_trees_widget.dart(1 hunks)
🔇 Additional comments (41)
lib/main.dart (1)
6-6: LGTM!The route is properly configured with appropriate constants and follows the established routing pattern in the application.
Also applies to: 72-78
lib/utils/constants/route_constants.dart (1)
4-4: LGTM!The route constants are properly defined and follow the existing naming conventions in the file.
Also applies to: 12-12
lib/pages/home_page.dart (1)
33-57: LGTM!The conditional rendering based on wallet connection state is appropriate, and the NearbyTreesWidget parameters (10km radius, 8 max trees) provide a good user experience.
lib/widgets/map_widgets/nearby_trees_widget.dart (2)
44-106: LGTM!Excellent async safety with mounted checks after every await. The 15-second timeout for location fetching is appropriate, and error handling is comprehensive.
108-113: LGTM!The distance formatting logic is clear and user-friendly, correctly handling both meter and kilometer ranges.
lib/widgets/map_widgets/map_search_widget.dart (4)
51-61: LGTM!The focus listener correctly uses a mounted check after the delayed setState, preventing potential issues if the widget is disposed during the delay.
79-93: LGTM!The tree ID search correctly uses regex validation and safe parsing with
int.tryParse. The logic is sound.
128-140: LGTM!The coordinate parsing includes proper validation for latitude and longitude ranges, ensuring only valid geographic coordinates are accepted.
96-109: LGTM!The species search appropriately limits results to 5 items and includes duplicate checking to ensure a tree doesn't appear multiple times in the results.
lib/services/geohash_service.dart (4)
16-24: LGTM!The coordinate order swapping correctly handles the GeoHasher API's longitude-first convention, returning proper LatLng objects with latitude-first ordering.
56-85: LGTM!The neighbor calculation properly uses the dart_geohash library's Direction enum and includes appropriate error handling for edge cases at poles and the date line.
88-101: Good overlap strategy for complete coverage.The 0.8 step multiplier ensures geohash cells overlap, providing complete coverage of the bounding box at the cost of some redundancy. This is a reasonable trade-off for spatial queries.
104-113: LGTM!The zoom-to-precision mapping appropriately increases spatial resolution at higher zoom levels, providing efficient geohash granularity for the map visualization.
lib/pages/explore_trees_map_page.dart (17)
1-21: LGTM!Clean imports and standard StatefulWidget definition. The page structure follows Flutter conventions.
23-46: LGTM!Good use of the
_mapReadyflag with_pendingCenter/_pendingZoomto handle the race condition between map initialization and location retrieval. The sentinel pattern prevents premature map operations.
73-94: LGTM!Proper
mountedcheck after async operation and graceful fallback to default center on location failure.
96-123: LGTM!The deferred move pattern with
_pendingCenter/_pendingZoomcorrectly handles the async timing between location acquisition and map readiness.
125-152: LGTM!Proper wallet connectivity check and error handling. The
_updateVisibleTreescall at line 143 is protected by the method's internalmountedguard.
154-185: LGTM!Proper guards for
mountedand_mapReadyprevent accessing the camera before initialization. The bounds filtering and clustering logic is clear.
187-218: LGTM!Filter logic correctly handles status, species, care count, and date range filters. The timestamp conversion from seconds to milliseconds is correct.
233-252: LGTM!Filter and search result handlers are straightforward. Different zoom levels for tree vs. non-tree results provide appropriate detail.
276-295: LGTM!Cluster tap behavior appropriately differentiates between single trees (show details) and clusters (zoom in). The +2 zoom increment provides good progressive drilling.
297-309: LGTM!Clean use of
Consumer<WalletProvider>to reactively switch between map content and wallet connection prompt.
311-363: LGTM!Well-structured map setup with proper
onMapReadycallback, gesture-only position change handling, and conditional marker layers. The OpenStreetMap tile layer includes appropriateuserAgentPackageName.
422-526: LGTM!Comprehensive overlay system with appropriate conditional rendering and position adjustments based on panel visibility.
528-544: LGTM!The marker builder correctly differentiates between single trees and clusters. The force unwrap at line 540 is safe due to the
isSingleguard.
546-614: LGTM!Good visual differentiation for tree status (alive/deceased), selection state, and cluster density (green/orange/red based on count).
739-943: LGTM!Comprehensive tree details panel with proper null handling, safe substring operation for geohash display (line 906-908), and clean navigation to full details page.
970-1059: LGTM!Wallet connection prompt with proper async handling,
mountedchecks, and user-friendly error feedback via SnackBar.
1061-1065: LGTM!Proper disposal of
MapControllerto prevent memory leaks.lib/widgets/map_widgets/map_filter_widget.dart (3)
5-60: LGTM!Excellent use of the sentinel pattern to distinguish "not provided" from "explicitly null" in
copyWith. This allows proper clearing of nullable filter fields.
100-322: LGTM!Well-structured expandable filter panel with proper mutual exclusivity for status filters and clean species dropdown implementation.
324-437: LGTM!
QuickFilterBarprovides intuitive one-tap toggles. The copyWith sentinel pattern correctly handles explicitnullvalues to clear filters.lib/widgets/map_widgets/tree_heatmap_layer.dart (3)
6-48: LGTM!Clean heatmap layer with appropriate zoom-based visibility threshold and dynamic radius scaling.
119-171: LGTM!Good alternative density visualization with appropriate size scaling and
IgnorePointerto prevent blocking map interactions.
173-234: LGTM!Simple and effective legend widget. The white background ensures visibility against the map.
lib/services/tree_map_service.dart (5)
60-77: LGTM!Clean cluster model with convenient accessors for count, total tree count, and single-tree detection.
79-120: LGTM!Well-designed caching strategy with O(1) duplicate detection via parallel
Set<int>tracking. The singleton pattern is appropriate for a shared service.
122-180: LGTM!Efficient bounds-based fetching with cache-first strategy and proper bounds filtering of results.
182-230: LGTM!Proper pagination handling with safe substring operation at lines 207-209 using
clampto prevent index-out-of-bounds.
269-306: LGTM!Clean clustering logic with proper handling of short/empty geohashes and correct center point calculation via coordinate averaging.
Tree is alive when death == 0 (not set) or death >= now (future timestamp). Previous logic incorrectly used > instead of >= for comparison.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/services/tree_map_service.dart (2)
234-269: Consider optimization for the nested loop.The nested loop (lines 252-264) iterates over all trees for each requested geohash, resulting in O(n×m) complexity where n = number of trees and m = number of geohashes. While the comment acknowledges this is a temporary solution pending backend indexing, consider optimizing by:
- Building a prefix tree (trie) of geohashes for O(log n) lookups
- Or, iterating trees once and checking all prefixes per tree
Current approach:
for (final tree in _allTrees) { for (final geohash in geohashSet) { if (tree.geoHash.startsWith(geohash) || ...) { // O(n×m) } } }Alternative (O(n) with single pass):
for (final tree in _allTrees) { final encodedGeohash = _geohashService.encode(tree.latitude, tree.longitude); for (final geohash in geohashSet) { final len = geohash.length; if ((tree.geoHash.length >= len && tree.geoHash.substring(0, len) == geohash) || (encodedGeohash.length >= len && encodedGeohash.substring(0, len) == geohash)) { _addTreeToCache(geohash, tree); break; // Early exit once matched } } }
272-308: Well-implemented clustering with safe string handling.The clustering logic correctly transitions from individual markers at high zoom (>= 15) to geohash-based clusters at lower zoom. The safe substring handling (line 289-291) prevents index errors.
Optional enhancement: Consider weighting cluster centers by
numberOfTreesinstead of treating each MapTreeData equally:final totalWeight = clusterTrees.fold(0, (sum, t) => sum + t.numberOfTrees); final centerLat = clusterTrees.fold(0.0, (sum, t) => sum + t.latitude * t.numberOfTrees) / totalWeight; final centerLng = clusterTrees.fold(0.0, (sum, t) => sum + t.longitude * t.numberOfTrees) / totalWeight;This would give more visual weight to NFTs representing multiple trees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/tree_map_service.dart(1 hunks)
🔇 Additional comments (5)
lib/services/tree_map_service.dart (5)
62-79: LGTM!The
TreeClusterclass is well-designed with convenient getters. ThetotalTreeCountcorrectly sums thenumberOfTreesfield from all trees in the cluster, accounting for NFTs that may represent multiple physical trees.
81-122: Well-designed caching strategy.The dual-map approach (
_treeCachefor data,_treeCacheIdsfor O(1) duplicate detection) is an excellent optimization. The singleton pattern is correctly implemented, and theclearCachemethod properly resets all state.
125-182: Robust bounds-based fetching with defensive filtering.The implementation correctly:
- Calculates optimal geohash precision from zoom
- Reuses cached data
- Fetches missing geohashes
- Applies a final bounds filter (lines 172-177) to ensure accuracy despite geohash approximations
The defensive bounds filtering at the end is especially good practice.
185-232: Solid paginated fetching with safe string handling.The implementation properly handles pagination and includes defensive substring handling (lines 208-212) with
.clamp(1, tree.geoHash.length)to prevent index errors. The cache population per geohash with O(1) duplicate checking is efficient.
35-55: TheisAlivelogic is correct and semantically sound.The condition
death == 0 || death >= nowproperly handles both unset death values (never died) and scheduled death timestamps (future dates). The blockchain contract supports future death timestamps, treating them as trees that will die at a specified time but are currently alive. This interpretation is confirmed by consistent usage across the codebase (user_nfts_widget displays "Deceased" only whendeath < now, confirming thatdeath >= nowmeans alive).One minor inconsistency exists:
recent_trees_widget.dartusesdeath > nowinstead ofdeath >= now, which differs on the boundary case whendeath == now. Consider aligning this to use>=for consistency.
- Split _convertCoordinate into _convertLatitude (-90 offset) and _convertLongitude (-180 offset) for correct decoding - Use neighborGeohashes to pre-filter trees before distance calculations for better performance in getTreesNearLocation()
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
lib/services/tree_map_service.dart (1)
108-110: Consider returning an unmodifiable view forallTrees(avoid external mutation of service state).
Not required, but returning_allTreesdirectly makes it easy for callers to mutate internal state accidentally.
- Add type conversion helpers (_asInt, _asString) for contract data - Handle both plantingDate and planting field names - Remove _hasMore gate in fetchTreesInBounds for proper cache population - Fix clamp() to int type mismatch for substring - Return unmodifiable list from allTrees getter - Include centerGeohash in nearby prefilter - Cache distances to avoid recalculating during sort - Fix misleading O(n) complexity comment
|
@Zahnentferner @chandansgowda @bhavik-mangla @a-singh09 Please review this pull request and if changes required please let me know. |
1 participant
closes #24
Overview
This PR implements a comprehensive dynamic map feature that allows users to explore trees around them using an interactive map interface. The implementation uses geohash-based spatial indexing for efficient tree queries and flutter_map (open-source) for map rendering.
🎯 Problem Statement
✨ Features Implemented
1. Interactive Map Page (
/explore-map)2. Geohash-Based Spatial Indexing
3. Tree Clustering
4. Filtering & Search
5. Nearby Trees Widget
🏗️ Architecture
System Flow Diagram
Geohash Spatial Indexing
Tree Clustering Algorithm
Data Flow for Map Interaction
🔧 Technical Highlights
Performance Optimizations
Set<int>for tree IDs per geohashSafety Measures
copyWithfor nullable filter clearingCode Quality
neighbor()functionSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.