Fix/trip display - #1058
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMobile routes now share latest-trip filtering and measurement normalization. Mobile map layers render validated sensor points and localized trip legends. Sensor and trip views now support improved empty states, scrolling, accessibility, and latest-trip indicators. ChangesMobile trip map flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceIdRoute
participant TripHelpers
participant SensorRoute
participant MobileOverview
participant MapLegend
DeviceIdRoute->>TripHelpers: select latest mobile trip points
TripHelpers-->>DeviceIdRoute: return selected locations
SensorRoute->>TripHelpers: filter normalized measurements
TripHelpers-->>SensorRoute: return measurements from latest trips
SensorRoute->>MobileOverview: provide mobile locations and sensor data
MobileOverview->>MapLegend: provide localized trip items and color callback
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/map/layers/mobile/mobile-box-layer.tsx`:
- Around line 42-46: Update the sensorData filter used to build mappableData so
it also requires both location coordinates to pass Number.isFinite, while
preserving the existing non-null measurement location/value and finite value
checks. Use the coordinate fields from measurement.location before creating
point features.
In `@app/components/map/layers/mobile/mobile-box-view.tsx`:
- Around line 103-118: Update the sensorData filtering used by minValue and
maxValue to exclude measurements whose location is null, matching
MobileBoxLayer’s rendered point set. Preserve the existing value-null and
finite-number checks so the legend bounds continue to use only valid rendered
measurements.
In `@app/components/map/layers/mobile/mobile-overview-layer.tsx`:
- Around line 207-212: Update the empty-data branch in the clusteredTrips update
flow to also reset highlightedTrip, hoveredCluster, and popupInfo alongside the
existing source and legend state resets. Ensure the next non-empty update cannot
reuse stale hover, highlight, or popup interaction state.
In `@app/routes/explore`.$deviceId.$sensorId.$.tsx:
- Around line 19-29: Update the RawMeasurement.time type to Date | null, then
exclude measurements with null times when constructing and applying
latestPointTimes in the measurement filtering flow. Preserve valid dated
measurements and prevent toISOString() from being called for null-time records.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0a26c2d-d847-4201-a6ce-aa974c4aa13c
📒 Files selected for processing (9)
app/components/map/layers/mobile/mobile-box-layer.tsxapp/components/map/layers/mobile/mobile-box-view.tsxapp/components/map/layers/mobile/mobile-overview-layer.tsxapp/components/map/layers/mobile/mobile-overview-legend.tsxapp/lib/mobile-box-helper.tsapp/routes/explore.$deviceId.$sensorId.$.tsxapp/routes/explore.$deviceId.tsxpublic/locales/de/mobile-map.jsonpublic/locales/en/mobile-map.json
| const mappableData = sensorData.filter( | ||
| (measurement) => | ||
| measurement.location !== null && | ||
| measurement.value !== null && | ||
| Number.isFinite(Number(measurement.value)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate coordinates before creating point features.
The filter accepts a non-null location with NaN or infinite coordinates. Those values create invalid GeoJSON positions and can prevent MapLibre from rendering the source. Filter both coordinates with Number.isFinite.
Proposed fix
const mappableData = sensorData.filter(
(measurement) =>
measurement.location !== null &&
+ Number.isFinite(measurement.location.x) &&
+ Number.isFinite(measurement.location.y) &&
measurement.value !== null &&
Number.isFinite(Number(measurement.value)),
)📝 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.
| const mappableData = sensorData.filter( | |
| (measurement) => | |
| measurement.location !== null && | |
| measurement.value !== null && | |
| Number.isFinite(Number(measurement.value)), | |
| const mappableData = sensorData.filter( | |
| (measurement) => | |
| measurement.location !== null && | |
| Number.isFinite(measurement.location.x) && | |
| Number.isFinite(measurement.location.y) && | |
| measurement.value !== null && | |
| Number.isFinite(Number(measurement.value)), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/map/layers/mobile/mobile-box-layer.tsx` around lines 42 - 46,
Update the sensorData filter used to build mappableData so it also requires both
location coordinates to pass Number.isFinite, while preserving the existing
non-null measurement location/value and finite value checks. Use the coordinate
fields from measurement.location before creating point features.
| const sensorData = Array.isArray(sensor.data) | ||
| ? sensor.data.filter( | ||
| (measurement) => | ||
| measurement.value !== null && | ||
| Number.isFinite(Number(measurement.value)), | ||
| ) | ||
| : [] | ||
|
|
||
| const minValue = Math.min(...sensorData.map((d) => Number(d.value))) | ||
| const maxValue = Math.max(...sensorData.map((d) => Number(d.value))) | ||
| const minValue = | ||
| sensorData.length > 0 | ||
| ? Math.min(...sensorData.map((d) => Number(d.value))) | ||
| : 0 | ||
| const maxValue = | ||
| sensorData.length > 0 | ||
| ? Math.max(...sensorData.map((d) => Number(d.value))) | ||
| : 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Calculate legend bounds from the rendered point set.
MobileBoxLayer excludes measurements with location === null before it calculates point colors. Legend includes their values in minValue and maxValue. If such a measurement contains an endpoint value, the legend range does not match the range used for rendered points. Filter out measurements without a location here too.
Proposed fix
? sensor.data.filter(
(measurement) =>
+ measurement.location !== null &&
measurement.value !== null &&
Number.isFinite(Number(measurement.value)),
)📝 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.
| const sensorData = Array.isArray(sensor.data) | |
| ? sensor.data.filter( | |
| (measurement) => | |
| measurement.value !== null && | |
| Number.isFinite(Number(measurement.value)), | |
| ) | |
| : [] | |
| const minValue = Math.min(...sensorData.map((d) => Number(d.value))) | |
| const maxValue = Math.max(...sensorData.map((d) => Number(d.value))) | |
| const minValue = | |
| sensorData.length > 0 | |
| ? Math.min(...sensorData.map((d) => Number(d.value))) | |
| : 0 | |
| const maxValue = | |
| sensorData.length > 0 | |
| ? Math.max(...sensorData.map((d) => Number(d.value))) | |
| : 0 | |
| const sensorData = Array.isArray(sensor.data) | |
| ? sensor.data.filter( | |
| (measurement) => | |
| measurement.location !== null && | |
| measurement.value !== null && | |
| Number.isFinite(Number(measurement.value)), | |
| ) | |
| : [] | |
| const minValue = | |
| sensorData.length > 0 | |
| ? Math.min(...sensorData.map((d) => Number(d.value))) | |
| : 0 | |
| const maxValue = | |
| sensorData.length > 0 | |
| ? Math.max(...sensorData.map((d) => Number(d.value))) | |
| : 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/map/layers/mobile/mobile-box-view.tsx` around lines 103 - 118,
Update the sensorData filtering used by minValue and maxValue to exclude
measurements whose location is null, matching MobileBoxLayer’s rendered point
set. Preserve the existing value-null and finite-number checks so the legend
bounds continue to use only valid rendered measurements.
| if (clusteredTrips.length === 0) { | ||
| setSourceData(null) | ||
| setExpandedSourceData(null) | ||
| setLegendItems([]) | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear interaction state with empty map data.
Line 207 clears the map sources but retains highlightedTrip, hoveredCluster, and popupInfo. If a user hovers a trip before the data becomes empty, the next non-empty update can render the old popup and highlight the wrong trip number. Reset these states in this branch.
Proposed fix
if (clusteredTrips.length === 0) {
setSourceData(null)
setExpandedSourceData(null)
setLegendItems([])
+ setHighlightedTrip(null)
+ setHoveredCluster(null)
+ setPopupInfo(null)
return
}📝 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 (clusteredTrips.length === 0) { | |
| setSourceData(null) | |
| setExpandedSourceData(null) | |
| setLegendItems([]) | |
| return | |
| } | |
| if (clusteredTrips.length === 0) { | |
| setSourceData(null) | |
| setExpandedSourceData(null) | |
| setLegendItems([]) | |
| setHighlightedTrip(null) | |
| setHoveredCluster(null) | |
| setPopupInfo(null) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/map/layers/mobile/mobile-overview-layer.tsx` around lines 207
- 212, Update the empty-data branch in the clusteredTrips update flow to also
reset highlightedTrip, hoveredCluster, and popupInfo alongside the existing
source and legend state resets. Ensure the next non-empty update cannot reuse
stale hover, highlight, or popup interaction state.
| type RawMeasurement = { | ||
| sensorId: string | ||
| locationId: bigint | null | ||
| time: Date | ||
| value: number | null | ||
| location: { | ||
| id: bigint | ||
| x: number | ||
| y: number | ||
| } | null | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve nullable measurement times before trip filtering.
SensorWithMeasurementData['data'] permits time: null, but RawMeasurement narrows it to Date. The casts on Lines 103 and 121 do not make a null value valid. If a mobile measurement has a location and time: null, toISOString() throws and fails the route loader.
Set RawMeasurement.time to Date | null. Exclude null-time measurements when building and applying latestPointTimes.
Proposed fix
type RawMeasurement = {
sensorId: string
locationId: bigint | null
- time: Date
+ time: Date | null
value: number | null
location: {
id: bigint
x: number
y: number
} | null
}
- const locationPoints: LocationPoint[] = normalizedData
- .filter((measurement) => measurement.location !== null)
+ const locationPoints: LocationPoint[] = normalizedData
+ .filter(
+ (measurement) =>
+ measurement.location !== null && measurement.time !== null,
+ )
.map((measurement) => ({
geometry: {
x: measurement.location!.x,
y: measurement.location!.y,
},
time: measurement.time.toISOString(),
}))
@@
return normalizedData.filter(
(measurement) =>
measurement.location !== null &&
+ measurement.time !== null &&
latestPointTimes.has(measurement.time.toISOString()),
)Also applies to: 51-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/explore`.$deviceId.$sensorId.$.tsx around lines 19 - 29, Update
the RawMeasurement.time type to Date | null, then exclude measurements with null
times when constructing and applying latestPointTimes in the measurement
filtering flow. Preserve valid dated measurements and prevent toISOString() from
being called for null-time records.
Coverage Report
File CoverageNo changed files found. |
1 participant
Type of Change
Implementation
Checklist
devbranchAdditional Information
Summary by CodeRabbit