Implement Mapbox route rendering via GeoJsonLayer#562
Implement Mapbox route rendering via GeoJsonLayer#562ngoiyaeric wants to merge 1 commit intomainfrom
Conversation
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughThis pull request adds support for rendering routes as GeoJSON linestrings on maps. It introduces a new linestring layer component, extends type definitions to include routeGeoJSON fields, extracts route geometry from geospatial responses, and renders routes declaratively through a GeoJsonLayer component in the map interface. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant Geospatial as Geospatial Tool
participant Handler as Query Handler
participant Context as Map Data Context
participant Map as Mapbox Map
Client->>Geospatial: Request location/route
Geospatial->>Geospatial: Parse response & extract<br/>route geometry
Geospatial->>Geospatial: Populate routeGeoJSON<br/>in McpResponse
Geospatial-->>Handler: Return McpResponse<br/>with routeGeoJSON
Handler->>Handler: Propagate routeGeoJSON<br/>to mapFeature
Handler->>Context: Update mapFeature<br/>with routeGeoJSON
Context-->>Map: mapFeature with<br/>routeGeoJSON available
Map->>Map: Render GeoJsonLayer<br/>for linestring geometry
Map-->>Client: Display route on map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/map/map-data-context.tsx`:
- Around line 19-25: Tighten the overly-permissive mapFeature shape by replacing
the broad index signature with an explicit interface (e.g., MapFeature) that
lists known optional properties (place_name, mapUrl) and types routeGeoJSON as a
GeoJSON Geometry/Feature (use GeoJSON.Geometry | GeoJSON.Feature | null) instead
of any; if you still need extensibility, use a safer type like Record<string,
unknown> or a discriminated union for different feature kinds rather than [key:
string]: any so type errors are not masked (update the mapFeature type
declaration and any usages that reference mapFeature/routeGeoJSON).
In `@components/map/map-query-handler.tsx`:
- Line 16: routeGeoJSON is currently typed as any which removes type safety;
change its type to an appropriate GeoJSON type (e.g., GeoJSON.FeatureCollection
| GeoJSON.Feature<GeoJSON.LineString> | GeoJSON.Geometry) to match the Mapbox
Directions API output. Import the GeoJSON types from the 'geojson' package (or
use your project’s geo types) and update the routeGeoJSON declaration and any
usages in the MapQueryHandler component / functions to use the chosen specific
type, adjusting casts or parsing where necessary.
In `@components/map/mapbox-map.tsx`:
- Around line 14-15: The inline comment on the import of GeoJsonLayer is
incorrect; update the comment (or remove it) so it accurately describes that the
line imports GeoJsonLayer rather than useMapLoading — locate the import
statements for useMapLoading and GeoJsonLayer in the module (symbols:
useMapLoading, GeoJsonLayer) and change the comment to either "// Import
GeoJsonLayer" or delete the misleading comment.
In `@lib/agents/tools/geospatial.tsx`:
- Line 28: The property routeGeoJSON is typed as any which loses type safety;
replace its type with the proper GeoJSON Geometry type (import Geometry from the
'geojson' package or `@types/geojson`) and update the declaration of
routeGeoJSON?: any to routeGeoJSON?: Geometry so callers and consumers (e.g., in
geospatial.tsx) have correct shape validation and autocompletion.
- Around line 411-421: The route center extraction leaves lat/lng undefined when
route.geometry.coordinates is missing or empty, causing MapQueryHandler to
reject the whole mcpData (including valid routeGeoJSON); update the block that
handles parsedData.routes (around parsedData.routes[0],
route.geometry.coordinates, and the assignment to mcpData) to defensively set a
fallback center: if coordinates array is empty or missing, use the first
available waypoint or omit the location field entirely (or set a safe default
like 0/0 or a bounding-box center) so that routeGeoJSON is still preserved and
passed to MapQueryHandler; ensure mcpData.location is only included when
latitude and longitude are valid numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44cf10ea-6925-48f6-85eb-49e80593248c
📒 Files selected for processing (5)
components/map/geojson-layer.tsxcomponents/map/map-data-context.tsxcomponents/map/map-query-handler.tsxcomponents/map/mapbox-map.tsxlib/agents/tools/geospatial.tsx
📜 Review details
🔇 Additional comments (7)
components/map/map-query-handler.tsx (1)
44-48: LGTM!The mapFeature construction correctly propagates
routeGeoJSONfrom the tool output using appropriate optional chaining to handle potentially nullish values.lib/agents/tools/geospatial.tsx (2)
397-404: LGTM!The route geometry extraction correctly handles both possible response structures from the Mapbox Directions API: top-level
geometryand nestedroutes[0].geometry.
408-410: LGTM!The
routeGeoJSONis properly included in mcpData for both theresultsandlocationresponse paths, ensuring route geometry propagates through all code paths.components/map/geojson-layer.tsx (2)
66-83: LGTM!The linestring layer implementation is well-structured:
- Correct filter syntax using
['any', ...]to match bothLineStringandMultiLineStringgeometry types- Appropriate styling with rounded line joins/caps for smooth route rendering
- Proper z-ordering (added after polygon layers, before point layers)
23-23: LGTM!The linestring layer ID is properly defined and cleanup correctly removes the layer before the source, maintaining proper resource management.
Also applies to: 114-114
components/map/mapbox-map.tsx (2)
554-556: LGTM!The comment clearly explains the architectural shift from imperative route drawing to declarative GeoJsonLayer rendering, which improves maintainability.
594-606: LGTM!The declarative route rendering is well-implemented:
- Properly guards against missing
routeGeoJSONwith conditional rendering- Correctly wraps the raw geometry in a GeoJSON
FeatureCollectionstructure as required byGeoJsonLayer- Uses a stable
idfor consistent layer managementOne consideration: the
GeoJsonLayeris rendered as a sibling to the map containerdiv, which is fine since it usesuseMap()context internally and renders nothing to the DOM itself.
| // TODO: Add other relevant map data types later (e.g., poiList) | ||
| mapFeature?: { | ||
| place_name?: string; | ||
| mapUrl?: string; | ||
| routeGeoJSON?: any; | ||
| [key: string]: any; | ||
| } | null; // Generic feature from MCP hook's processLocationQuery |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider tightening the mapFeature type.
The index signature [key: string]: any is overly permissive and could mask type errors. If additional properties are needed, consider explicitly defining them or using a discriminated union. Also, routeGeoJSON could benefit from a proper GeoJSON Geometry type for better type safety.
♻️ Suggested type improvement
+import type { Geometry } from 'geojson';
+
export interface MapData {
targetPosition?: { lat: number; lng: number } | null;
cameraState?: CameraState;
currentTimezone?: string;
// TODO: Add other relevant map data types later (e.g., poiList)
mapFeature?: {
place_name?: string;
mapUrl?: string;
- routeGeoJSON?: any;
- [key: string]: any;
+ routeGeoJSON?: Geometry;
} | null;📝 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.
| // TODO: Add other relevant map data types later (e.g., poiList) | |
| mapFeature?: { | |
| place_name?: string; | |
| mapUrl?: string; | |
| routeGeoJSON?: any; | |
| [key: string]: any; | |
| } | null; // Generic feature from MCP hook's processLocationQuery | |
| import type { Geometry } from 'geojson'; | |
| export interface MapData { | |
| targetPosition?: { lat: number; lng: number } | null; | |
| cameraState?: CameraState; | |
| currentTimezone?: string; | |
| // TODO: Add other relevant map data types later (e.g., poiList) | |
| mapFeature?: { | |
| place_name?: string; | |
| mapUrl?: string; | |
| routeGeoJSON?: Geometry; | |
| } | null; // Generic feature from MCP hook's processLocationQuery |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/map/map-data-context.tsx` around lines 19 - 25, Tighten the
overly-permissive mapFeature shape by replacing the broad index signature with
an explicit interface (e.g., MapFeature) that lists known optional properties
(place_name, mapUrl) and types routeGeoJSON as a GeoJSON Geometry/Feature (use
GeoJSON.Geometry | GeoJSON.Feature | null) instead of any; if you still need
extensibility, use a safer type like Record<string, unknown> or a discriminated
union for different feature kinds rather than [key: string]: any so type errors
are not masked (update the mapFeature type declaration and any usages that
reference mapFeature/routeGeoJSON).
| address?: string; | ||
| }; | ||
| mapUrl?: string; | ||
| routeGeoJSON?: any; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more specific type for routeGeoJSON.
Using any loses type safety. Since this represents GeoJSON geometry from the Mapbox Directions API, consider using a proper GeoJSON type.
♻️ Suggested type improvement
+import type { Geometry } from 'geojson';
+
interface McpResponseData {
location: {
latitude?: number;
longitude?: number;
place_name?: string;
address?: string;
};
mapUrl?: string;
- routeGeoJSON?: any;
+ routeGeoJSON?: Geometry;
}📝 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.
| routeGeoJSON?: any; | |
| import type { Geometry } from 'geojson'; | |
| interface McpResponseData { | |
| location: { | |
| latitude?: number; | |
| longitude?: number; | |
| place_name?: string; | |
| address?: string; | |
| }; | |
| mapUrl?: string; | |
| routeGeoJSON?: Geometry; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/map/map-query-handler.tsx` at line 16, routeGeoJSON is currently
typed as any which removes type safety; change its type to an appropriate
GeoJSON type (e.g., GeoJSON.FeatureCollection |
GeoJSON.Feature<GeoJSON.LineString> | GeoJSON.Geometry) to match the Mapbox
Directions API output. Import the GeoJSON types from the 'geojson' package (or
use your project’s geo types) and update the routeGeoJSON declaration and any
usages in the MapQueryHandler component / functions to use the chosen specific
type, adjusting casts or parsing where necessary.
| import { useMapLoading } from '../map-loading-context' | ||
| import { GeoJsonLayer } from './geojson-layer'; // Import useMapLoading |
There was a problem hiding this comment.
Fix the misleading inline comment.
The comment says "Import useMapLoading" but the line actually imports GeoJsonLayer. The useMapLoading import is on line 14.
📝 Proposed fix
import { useMapLoading } from '../map-loading-context'
-import { GeoJsonLayer } from './geojson-layer'; // Import useMapLoading
+import { GeoJsonLayer } from './geojson-layer';
import { useMap } from './map-context'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/map/mapbox-map.tsx` around lines 14 - 15, The inline comment on
the import of GeoJsonLayer is incorrect; update the comment (or remove it) so it
accurately describes that the line imports GeoJsonLayer rather than
useMapLoading — locate the import statements for useMapLoading and GeoJsonLayer
in the module (symbols: useMapLoading, GeoJsonLayer) and change the comment to
either "// Import GeoJsonLayer" or delete the misleading comment.
| interface McpResponse { | ||
| location: Location; | ||
| mapUrl?: string; | ||
| routeGeoJSON?: any; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a proper GeoJSON Geometry type.
This is consistent with other files, but using any loses type safety. The GeoJSON Geometry type would provide better validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/agents/tools/geospatial.tsx` at line 28, The property routeGeoJSON is
typed as any which loses type safety; replace its type with the proper GeoJSON
Geometry type (import Geometry from the 'geojson' package or `@types/geojson`) and
update the declaration of routeGeoJSON?: any to routeGeoJSON?: Geometry so
callers and consumers (e.g., in geospatial.tsx) have correct shape validation
and autocompletion.
| } else if (parsedData.routes && parsedData.routes.length > 0) { | ||
| // It's a routing response, pick first route coordinates for map center | ||
| const route = parsedData.routes[0]; | ||
| let lat, lng; | ||
| if (route.geometry && route.geometry.coordinates && route.geometry.coordinates.length > 0) { | ||
| // Pick a point roughly in the middle, or the start | ||
| const coords = route.geometry.coordinates[Math.floor(route.geometry.coordinates.length / 2)]; | ||
| lng = coords[0]; | ||
| lat = coords[1]; | ||
| } | ||
| mcpData = { location: { latitude: lat, longitude: lng, place_name: "Route", address: "Generated Route" }, routeGeoJSON }; |
There was a problem hiding this comment.
Route center coordinates may be undefined, causing downstream issues.
When extracting the route center point, lat and lng remain undefined if route.geometry.coordinates is empty or missing. This undefined location will be passed to MapQueryHandler, which checks typeof latitude === 'number' and will reject the data, clearing mapFeature entirely—including the valid routeGeoJSON. This means routes with empty coordinate arrays will silently fail to render.
🛠️ Proposed fix to handle empty coordinates
} else if (parsedData.routes && parsedData.routes.length > 0) {
// It's a routing response, pick first route coordinates for map center
const route = parsedData.routes[0];
- let lat, lng;
+ let lat: number | undefined, lng: number | undefined;
if (route.geometry && route.geometry.coordinates && route.geometry.coordinates.length > 0) {
// Pick a point roughly in the middle, or the start
const coords = route.geometry.coordinates[Math.floor(route.geometry.coordinates.length / 2)];
- lng = coords[0];
- lat = coords[1];
+ if (Array.isArray(coords) && coords.length >= 2) {
+ lng = coords[0];
+ lat = coords[1];
+ }
+ }
+ // Fallback: if no valid center, use first coordinate pair
+ if (lat === undefined || lng === undefined) {
+ const firstCoord = route.geometry?.coordinates?.[0];
+ if (Array.isArray(firstCoord) && firstCoord.length >= 2) {
+ lng = firstCoord[0];
+ lat = firstCoord[1];
+ }
}
mcpData = { location: { latitude: lat, longitude: lng, place_name: "Route", address: "Generated Route" }, routeGeoJSON };📝 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.
| } else if (parsedData.routes && parsedData.routes.length > 0) { | |
| // It's a routing response, pick first route coordinates for map center | |
| const route = parsedData.routes[0]; | |
| let lat, lng; | |
| if (route.geometry && route.geometry.coordinates && route.geometry.coordinates.length > 0) { | |
| // Pick a point roughly in the middle, or the start | |
| const coords = route.geometry.coordinates[Math.floor(route.geometry.coordinates.length / 2)]; | |
| lng = coords[0]; | |
| lat = coords[1]; | |
| } | |
| mcpData = { location: { latitude: lat, longitude: lng, place_name: "Route", address: "Generated Route" }, routeGeoJSON }; | |
| } else if (parsedData.routes && parsedData.routes.length > 0) { | |
| // It's a routing response, pick first route coordinates for map center | |
| const route = parsedData.routes[0]; | |
| let lat: number | undefined, lng: number | undefined; | |
| if (route.geometry && route.geometry.coordinates && route.geometry.coordinates.length > 0) { | |
| // Pick a point roughly in the middle, or the start | |
| const coords = route.geometry.coordinates[Math.floor(route.geometry.coordinates.length / 2)]; | |
| if (Array.isArray(coords) && coords.length >= 2) { | |
| lng = coords[0]; | |
| lat = coords[1]; | |
| } | |
| } | |
| // Fallback: if no valid center, use first coordinate pair | |
| if (lat === undefined || lng === undefined) { | |
| const firstCoord = route.geometry?.coordinates?.[0]; | |
| if (Array.isArray(firstCoord) && firstCoord.length >= 2) { | |
| lng = firstCoord[0]; | |
| lat = firstCoord[1]; | |
| } | |
| } | |
| mcpData = { location: { latitude: lat, longitude: lng, place_name: "Route", address: "Generated Route" }, routeGeoJSON }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/agents/tools/geospatial.tsx` around lines 411 - 421, The route center
extraction leaves lat/lng undefined when route.geometry.coordinates is missing
or empty, causing MapQueryHandler to reject the whole mcpData (including valid
routeGeoJSON); update the block that handles parsedData.routes (around
parsedData.routes[0], route.geometry.coordinates, and the assignment to mcpData)
to defensively set a fallback center: if coordinates array is empty or missing,
use the first available waypoint or omit the location field entirely (or set a
safe default like 0/0 or a bounding-box center) so that routeGeoJSON is still
preserved and passed to MapQueryHandler; ensure mcpData.location is only
included when latitude and longitude are valid numbers.
2 participants
This PR implements the missing
drawRouteTODO by extracting route geometry from the Mapbox Directions API, passing it through the MapContext, extendingGeoJsonLayerto supportLineStringelements, and declaratively mounting it on the Mapbox instance.PR created automatically by Jules for task 18234505032010150291 started by @ngoiyaeric
Summary by CodeRabbit
New Features
Refactor