Conversation
WalkthroughReplaces visible-bounds callback with a rendered-features station-count flow: MapBoxMapView now aggregates deviceCount from point annotations on map idle and notifies as a stations count; Explorer and Search view models/views updated to consume an optional stations count and show localized "no stations" text. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MapView as MapBoxMapView
participant Mapbox as MapboxMap
participant Coordinator
participant ExplorerVM
participant SearchVM as ExplorerSearchViewModel
participant SearchView
User->>MapView: pan/zoom -> map idle
MapView->>Mapbox: read cameraState & rendered features
Note right of MapView: aggregate `deviceCount` from feature.customData
MapView->>Coordinator: didChangeVisibleStationsCount(count)
Coordinator->>ExplorerVM: didUpdateStationsCount(count)
alt explorerData.totalDevices > 0
ExplorerVM->>SearchVM: updateStations(count)
alt count == 0
SearchVM->>SearchView: stationsCount = nil
SearchVM->>SearchView: stationsCountDescription = LocalizableString.Search.noStationsInArea
else
SearchVM->>SearchView: stationsCount = "N"
SearchVM->>SearchView: stationsCountDescription = LocalizableString.Search.stationsInArea
end
else
ExplorerVM--x SearchVM: skip updateStations
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift(2 hunks)PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift(1 hunks)PresentationLayer/UIComponents/Screens/Explorer/Search/ExplorerSearchViewModel.swift(2 hunks)PresentationLayer/UIComponents/Screens/Explorer/Search/SearchView+Content.swift(3 hunks)wxm-ios/Resources/Localizable/Localizable.xcstrings(1 hunks)wxm-ios/Resources/Localizable/LocalizableString+Search.swift(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift (2)
WeatherXMTests/PresentationLayer/ViewModels/ExplorerViewModelTests.swift (1)
explorerData(59-64)PresentationLayer/UIComponents/Screens/Explorer/Search/ExplorerSearchViewModel.swift (1)
updateStations(111-114)
PresentationLayer/UIComponents/Screens/Explorer/Search/ExplorerSearchViewModel.swift (1)
WeatherXMTests/PresentationLayer/Extensions/NumericTests.swift (1)
localizedFormatted(42-47)
🪛 GitHub Check: swiftLint
PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift
[warning] 131-131:
Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration (opening_brace)
⏰ 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). (1)
- GitHub Check: WeatherXM | Unit tests | wxm-ios - iOS
🔇 Additional comments (9)
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift (1)
182-184: LGTM! Correctly handles antemeridian crossing.Using
coordinateBoundsUnwrappedinstead ofcoordinateBoundsproperly handles viewports that cross the antemeridian (±180° longitude). This directly addresses the PR objective to ensure accurate station counting when the visible area contains the International Date Line.wxm-ios/Resources/Localizable/Localizable.xcstrings (1)
9353-9363: LGTM on the new localization entry.The key aligns with the existing naming scheme and the copy matches the surrounding search strings.
PresentationLayer/UIComponents/Screens/Explorer/Search/ExplorerSearchViewModel.swift (4)
24-25: LGTM!Making
stationsCountoptional enables the UI to conditionally render the count, supporting the "initially hidden" requirement. The addition ofstationsCountDescriptionprovides flexibility to show different messages for zero vs. non-zero counts.
112-113: LGTM!The logic correctly implements the PR requirement to show "no stations in the area" instead of "0 stations in the area" by setting
stationsCountto nil andstationsCountDescriptionto the localized "no stations" message when count is zero.
24-25: Property changes correctly support conditional display.Making
stationsCountoptional enables the UI to conditionally render the counter—showing nothing whennilrather than displaying "0". The newstationsCountDescriptionproperty provides the appropriate localized text based on whether stations are present.These changes align with the PR objectives to initially hide the counter and display context-appropriate text.
112-113: Logic correctly implements "no stations" text and conditional hiding.The implementation properly addresses the PR requirements:
- Line 112: Sets
stationsCounttonilwhen the count is 0, which hides the counter in the UI (addressing "initially hide the stations counter text").- Line 113: Uses
LocalizableString.Search.noStationsInAreawhen count is 0, changing the text from "0 stations in the area" to "no stations in the area" as specified.The use of
count.localizedFormattedensures proper number formatting with locale-specific grouping separators.wxm-ios/Resources/Localizable/LocalizableString+Search.swift (1)
20-20: LGTM!The new localization case follows the existing pattern and supports the updated UI requirement for displaying "no stations in area" when the count is zero.
Also applies to: 54-55
PresentationLayer/UIComponents/Screens/Explorer/Search/SearchView+Content.swift (1)
24-97: LGTM!The refactored
nonActiveViewcorrectly implements conditional rendering with smooth transitions:
- Initially hides both stations count and description (when nil)
- Shows only description when count is zero ("no stations in area")
- Shows both count and description when count is positive
The
.transition(.opacity.animation(.easeIn))on both text elements and the.animation(.easeIn, value: viewModel.stationsCount)provide smooth UX when the content appears/disappears.PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift (1)
119-134: Verify antemeridian handling in map bounds logic
No custom longitude-wrapping code or tests were found around lines 119–134 in ExplorerViewModel.swift. Ensure Mapbox’sCoordinateBounds.containsLongitudecorrectly handles spans across the ±180° meridian and add tests for map views crossing the antemeridian.
PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift
Outdated
Show resolved
Hide resolved
Code Coverage Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
WeatherXMTests/PresentationLayer/ViewModels/ExplorerSearchViewModelTests.swift (1)
78-83: Expand test coverage for the zero stations case.The test validates the non-zero count scenario but doesn't cover the "no stations" case, which is a key requirement per the PR objectives. Consider adding tests for:
updateStations(count: 0)— verifystationsCountisnilandstationsCountDescriptioncontains the localized "no stations in area" messagestationsCountDescriptionproperty for both zero and non-zero counts- State transitions (e.g., from 1,000 stations to 0)
Example test additions:
@Test func updateStationsWithZeroCount() { viewModel.updateStations(count: 0) #expect(viewModel.stationsCount == nil) #expect(viewModel.stationsCountDescription != nil) // Verify the description contains expected localized text } @Test func updateStationsTransition() { viewModel.updateStations(count: 100) #expect(viewModel.stationsCount != nil) viewModel.updateStations(count: 0) #expect(viewModel.stationsCount == nil) #expect(viewModel.stationsCountDescription != nil) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
WeatherXMTests/PresentationLayer/ViewModels/ExplorerSearchViewModelTests.swift(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). (1)
- GitHub Check: WeatherXM | Unit tests | wxm-ios - iOS
🔇 Additional comments (1)
WeatherXMTests/PresentationLayer/ViewModels/ExplorerSearchViewModelTests.swift (1)
79-79: LGTM! Correctly updated for optional stationsCount.The test now correctly expects
nilfor the initial state, aligning with the production change that makesstationsCountoptional.
Code Coverage Summary
|
There was a problem hiding this comment.
I'm not sure the antimeridian issue is correct, the number in this screenshot for example seems very small.
The code I've been using is this if that helps:
onExplorerData.value?.publicHexes?.sumOf {
val containsLon = if (westLon < eastLon && eastLon - westLon <= 180) {
it.center.lon in westLon..eastLon
} else {
it.center.lon <= westLon || it.center.lon >= eastLon
}
if (it.center.lat in southLat..northLat && containsLon) {
it.deviceCount ?: 0
} else {
0
}
}
|
@PavlosTze, in which simulator did you test it? I can't reproduce it on my device |
It's this, but if it's working OK in a device then maybe it's a simulator issue so we can proceed. |
Code Coverage Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift (1)
119-122: LGTM: gated UI update meets “hide initially”.Only forwards counts after explorer data is present.
🧹 Nitpick comments (2)
PresentationLayer/UIComponents/Screens/Explorer/ExplorerFactory.swift (1)
35-39: Good alignment of IDs and customData across features.
- Adds cellIndex to heatmap features and deviceCount into customData on polygons/points; PointAnnotation id aligns with polygon id, enabling dedup by featureId in queries. Looks correct.
Optional: Use MapBoxConstants.heatmapSource for the GeoJSONSource id to avoid string drift elsewhere:
var geoJsonSource = GeoJSONSource(id: MapBoxConstants.heatmapSource)Also applies to: 70-71, 77-79
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift (1)
182-205: Visible count aggregation logic looks sound; fix trailing whitespace.
Summing deviceCount from rendered features, keyed by featureId, avoids double counting between polygon/point layers. Correct approach for antemeridian cases.
SwiftLint flags trailing whitespace at Line 204; remove it.
You could restrict layerIds to the point layer for clarity/perf if not needed from polygons/heatmap.
Consider early-exiting on empty features to avoid computing intermediates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift(5 hunks)PresentationLayer/UIComponents/Screens/Explorer/ExplorerFactory.swift(2 hunks)PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift(1 hunks)PresentationLayer/Utils/MapBox/MapBoxConstants.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift (1)
PresentationLayer/UIComponents/Screens/Explorer/Search/ExplorerSearchViewModel.swift (1)
updateStations(111-114)
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift (1)
PresentationLayer/UIComponents/Screens/Explorer/ExplorerViewModel.swift (1)
didUpdateStationsCount(119-123)
🪛 GitHub Check: swiftLint
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift
[warning] 204-204:
Trailing Whitespace Violation: Lines should not have trailing whitespace (trailing_whitespace)
⏰ 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). (1)
- GitHub Check: WeatherXM | Unit tests | wxm-ios - iOS
🔇 Additional comments (5)
PresentationLayer/Utils/MapBox/MapBoxConstants.swift (1)
24-24: LGTM: constant for custom data key.Matches usage in MapBoxMapView feature.properties lookup.
PresentationLayer/UIComponents/MapBox/MapBoxMapView.swift (4)
100-102: Delegate method rename wired correctly.Coordinator forwards the visible stations count to the VM.
158-159: No behavioral change.Whitespace-only adjustment is fine.
298-298: Minor symbol text sizing tweak.Zoom stop at 8: 0.1 is fine; verify legibility on low-DPI simulators.
397-398: No stale delegate methods remain
AllMapViewControllerDelegateconformers (onlyCoordinator) implementdidChangeVisibleStationsCount, and nodidChangeVisibleBoundsusages were found.
Code Coverage Summary
|
|
@PavlosTze please take a look about the stations counter functionality we discussed |
3 participants
Why?
Handle some cases in the explorer tab as described in the task
How?
Testing
Additional context
fixes fe-2021
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests