Fix location search #2429
Fix location search #2429
Conversation
…ts and places from the AirQualityResponse
…and enhance search functionality
📝 WalkthroughWalkthroughThis pull request introduces several modifications across different components of a mobile application. The changes primarily focus on enhancing search functionality in the map page, updating text displays, and making minor adjustments to widget implementations. The modifications range from removing personalized text to improving location search capabilities and refining data presentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant MapPage
participant SearchController
participant AirQualityLocations
User->>MapPage: Enter search query
MapPage->>SearchController: Process query
SearchController->>AirQualityLocations: Search local measurements
AirQualityLocations-->>SearchController: Return matching locations
SearchController-->>MapPage: Display search results
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mobile-v3/lib/src/app/map/pages/map_page.dart (2)
43-74: Well-implemented search functionality with room for optimization.The search implementation is thorough and handles null safety well. Consider these optimizations:
- Convert the query to lowercase once at the start instead of in each field check
- Consider using a computed/indexed search field that combines all searchable fields
List<Measurement> searchAirQualityLocations( String query, List<Measurement> measurements) { query = query.toLowerCase(); + // Cache the lowercase values to avoid repeated conversions + return measurements.where((measurement) { + if (measurement.siteDetails == null) return false; + + final details = measurement.siteDetails!; + final searchFields = [ + details.city, + details.locationName, + details.name, + details.searchName, + details.formattedName, + details.town, + details.district + ]; + + return searchFields + .where((field) => field != null) + .any((field) => field!.toLowerCase().contains(query)); + }).toList(); - return measurements.where((measurement) { - if (measurement.siteDetails != null) { - return (measurement.siteDetails!.city?.toLowerCase().contains(query) ?? - false) || - (measurement.siteDetails!.locationName - ?.toLowerCase() - .contains(query) ?? - false) || - (measurement.siteDetails!.name?.toLowerCase().contains(query) ?? - false) || - (measurement.siteDetails!.searchName - ?.toLowerCase() - .contains(query) ?? - false) || - (measurement.siteDetails!.formattedName - ?.toLowerCase() - .contains(query) ?? - false) || - (measurement.siteDetails!.town?.toLowerCase().contains(query) ?? - false) || - (measurement.siteDetails!.district?.toLowerCase().contains(query) ?? - false); - } - return false; - }).toList();
946-994: Remove commented-out code.Consider removing the commented-out code since the new implementation is now stable. This will improve code maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mobile-v3/lib/src/app/dashboard/widgets/analytics_card.dart(1 hunks)mobile-v3/lib/src/app/dashboard/widgets/analytics_forecast_widget.dart(1 hunks)mobile-v3/lib/src/app/learn/pages/lesson_finished.dart(1 hunks)mobile-v3/lib/src/app/map/pages/map_page.dart(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- mobile-v3/lib/src/app/dashboard/widgets/analytics_forecast_widget.dart
🔇 Additional comments (4)
mobile-v3/lib/src/app/learn/pages/lesson_finished.dart (1)
14-14: Fix text formatting and verify change relevance.
- There's an extra space between "Job" and "!".
- This change appears unrelated to the PR's objective of fixing location search functionality.
- Text("👋🏼 Great Job !", + Text("👋🏼 Great Job!",Could you clarify how this change relates to fixing the location search functionality?
mobile-v3/lib/src/app/map/pages/map_page.dart (3)
769-784: Clean implementation of search input handling.The code properly handles both empty and non-empty search inputs, with clear separation between Google Places and local search results.
863-944: Clean UI implementation for search results.The separation between local and Google Places results improves user experience. The implementation is clean and well-structured.
1100-1157: Robust implementation of measurements display.The code handles null cases well and provides appropriate fallback values. The empty state message improves user experience.
| @@ -95,7 +95,7 @@ class AnalyticsCard extends StatelessWidget { | |||
| child: Column( | |||
| crossAxisAlignment: CrossAxisAlignment.start, | |||
| children: [ | |||
| Text(measurement.siteDetails!.locationName ?? "", | |||
| Text(measurement.siteDetails!.name ?? "", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent property usage detected across codebase
The SiteDetails model contains both name and locationName properties, but they're being used inconsistently:
mobile-v3/lib/src/app/dashboard/widgets/analytics_card.dartusesnamemobile-v3/lib/src/app/map/pages/map_page.dartuseslocationName
This suggests an incomplete migration between these properties.
🔗 Analysis chain
Verify data model property change across codebase.
The change from locationName to name suggests a data model update. Let's ensure this change is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of locationName to ensure complete migration
rg "siteDetails.*locationName" --type dart
Length of output: 141
Script:
#!/bin/bash
# Search for SiteDetails class definition
ast-grep --pattern 'class SiteDetails {
$$$
}'
Length of output: 50
Script:
#!/bin/bash
# Search for SiteDetails class and its properties
rg "class SiteDetails" --type dart -A 10
Length of output: 955
Script:
#!/bin/bash
# Search for SiteDetails class with more context
rg "class SiteDetails" --type dart -A 30
Length of output: 2704
There was a problem hiding this comment.
Thanks @Mozart299 ,
- Are we using
search_nameornameof a Site? - Have you already had a sync with @Codebmk on this subject?
Hi @Mozart299 |
|
@Baalmart Jordan had been using location_name but it was too generic. search_name is also too generic as well. So I used name. I got this from logging the API response. |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
Bug Fixes
Enhancements
Chores