Coordinate Rendering and Map Panning #434
Conversation
- Update MapData to support multiple markers with unique IDs - Enhance Mapbox component to synchronize markers with MapData context - Create CoordinateLink component for interactive coordinate labels - Implement coordinate detection in BotMessage for automatic marker placement and panning - Update MapQueryHandler to add tool-provided locations to the map markers - Fix TypeScript error in BotMessage component for production build compatibility
|
👋 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.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThe changes introduce an interactive coordinate link feature that detects latitude and longitude patterns within message content, renders them as clickable links to Google Maps, and synchronizes corresponding markers on a Mapbox map display using a deduplicating marker system with unique coordinate-based identifiers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Message
participant CoordinateLink
participant MapDataContext
participant MapboxMap
participant GoogleMaps
User->>Message: View message with coordinates
Message->>Message: Parse text for coordinate patterns
Message->>CoordinateLink: Render detected coordinates
CoordinateLink-->>User: Display as clickable link
User->>CoordinateLink: Click coordinate link
CoordinateLink->>MapDataContext: Update map state with marker
MapDataContext->>MapDataContext: Add/update marker with ID
MapDataContext->>MapboxMap: Notify of marker changes
MapboxMap->>MapboxMap: Synchronize markers on map
MapboxMap-->>User: Display marker on map
CoordinateLink->>GoogleMaps: Navigate to coordinates (new tab)
GoogleMaps-->>User: Show maps view
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Labels
Poem
🚥 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. Comment |
|
|
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The biggest issue is a fatal XSS risk in Mapbox via Popup().setHTML(...) using tool/AI-provided strings. Additionally, CoordinateLink auto-pans on mount, which will likely cause unpredictable/jumpy map behavior when chat content renders or re-renders. Coordinate parsing in BotMessage is also brittle and may accidentally transform content inside code-like text nodes, and marker dedupe IDs should be normalized to avoid float-precision duplicates.
Additional notes (1)
- Compatibility |
components/map/map-data-context.tsx:26-30
This change addsid: stringtoMapData.markersitems. Any existing marker creation paths elsewhere in the codebase must now provideid. In this diff, you updatedCoordinateLinkandMapQueryHandler, andMapboxreadsmarkerData.id, but ensure there are no other writers that still push{ latitude, longitude }only; otherwise markers may silently break/dedupe incorrectly.
If you expect backward compatibility, consider making id optional and deriving one as a fallback at the render layer.
Summary of changes
What this PR changes
- Adds a new client component
components/map/coordinate-link.tsxthat:- renders coordinates as an
<a>link to Google Maps - on mount, adds a marker to
MapDataContextand setstargetPositionto pan the map - on click, re-sets
targetPositionto re-pan
- renders coordinates as an
- Extends
MapData.markersitems to include a requiredid. - Updates
MapQueryHandlerto:- create a marker from tool-provided
latitude/longitude - dedupe markers by
id - set
targetPositionand keepmapFeaturemetadata
- create a marker from tool-provided
- Updates
Mapboxto synchronize Mapbox GL markers withmapData.markersvia a newmarkersRefand a marker syncuseEffect. - Updates
BotMessagemarkdown rendering to detect inline coordinate patterns and replace them withCoordinateLinkcomponents. - Updates
server.log(runtime output changes).
| useEffect(() => { | ||
| // Automatically add marker and pan to it when the component mounts | ||
| setMapData(prev => { | ||
| const exists = prev.markers?.some(m => m.id === id); | ||
| const newMarkers = exists ? prev.markers : [...(prev.markers || []), { id, latitude: lat, longitude: lng, title: label || id }]; | ||
|
|
||
| return { | ||
| ...prev, | ||
| targetPosition: { lat, lng }, | ||
| markers: newMarkers | ||
| }; | ||
| }); | ||
| }, [lat, lng, id, label, setMapData]); |
There was a problem hiding this comment.
The useEffect always sets targetPosition on mount, which means simply rendering historical chat messages containing coordinates will keep panning the map as the user scrolls / as React re-mounts nodes. This can create a very jumpy UX and makes map position dependent on render order rather than explicit user intent.
This is especially problematic because BotMessage will create a CoordinateLink for every match, so a single message with multiple coordinates can pan multiple times during initial render.
Suggestion
Consider not panning on mount. Split responsibilities:
- On mount: only ensure the marker exists.
- On click: pan (
targetPosition) and optionally select/focus the marker.
Example:
useEffect(() => {
setMapData(prev => {
const exists = prev.markers?.some(m => m.id === id)
if (exists) return prev
return {
...prev,
markers: [
...(prev.markers || []),
{ id, latitude: lat, longitude: lng, title: label || id }
]
}
})
}, [id, lat, lng, label, setMapData])
const handleClick = (e: React.MouseEvent) => {
e.preventDefault()
setMapData(prev => ({ ...prev, targetPosition: { lat, lng } }))
}If you still want auto-pan, gate it behind a flag (e.g. autoPanOnMount?: boolean) and only enable it for tool outputs, not for every coordinate mention.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const id = `${lat},${lng}`; | ||
|
|
||
| useEffect(() => { | ||
| // Automatically add marker and pan to it when the component mounts | ||
| setMapData(prev => { | ||
| const exists = prev.markers?.some(m => m.id === id); | ||
| const newMarkers = exists ? prev.markers : [...(prev.markers || []), { id, latitude: lat, longitude: lng, title: label || id }]; | ||
|
|
There was a problem hiding this comment.
The id is derived from raw lat,lng numbers. The same real-world coordinate can appear with different precision (e.g. 37.77 vs 37.7700) and become duplicate markers. Also, floating-point stringification can be inconsistent across sources.
A stable ID strategy (rounding to a fixed precision for IDs, or using an explicit unique ID from the tool when available) will prevent marker spam and make dedupe reliable.
Suggestion
Normalize coordinate IDs to a fixed precision (and consider including a namespace) so dedupe is consistent:
const norm = (n: number) => n.toFixed(6)
const id = `coord:${norm(lat)},${norm(lng)}`Then use the same normalization in MapQueryHandler when generating IDs.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Effect to synchronize markers from MapDataContext | ||
| useEffect(() => { | ||
| if (!map.current || !initializedRef.current) return; | ||
|
|
||
| // Remove markers that are no longer in mapData | ||
| const currentMarkerIds = new Set(mapData.markers?.map(m => m.id) || []); | ||
| Object.keys(markersRef.current).forEach(id => { | ||
| if (!currentMarkerIds.has(id)) { | ||
| markersRef.current[id].remove(); | ||
| delete markersRef.current[id]; | ||
| } | ||
| }); | ||
|
|
||
| // Add or update markers from mapData | ||
| mapData.markers?.forEach(markerData => { | ||
| if (!markersRef.current[markerData.id]) { | ||
| const marker = new mapboxgl.Marker() | ||
| .setLngLat([markerData.longitude, markerData.latitude]) | ||
| .setPopup(markerData.title ? new mapboxgl.Popup().setHTML(`<h3>${markerData.title}</h3>`) : null) | ||
| .addTo(map.current!); | ||
| markersRef.current[markerData.id] = marker; | ||
| } else { | ||
| markersRef.current[markerData.id].setLngLat([markerData.longitude, markerData.latitude]); | ||
| } | ||
| }); | ||
| }, [mapData.markers]); |
There was a problem hiding this comment.
Using setHTML(`<h3>${markerData.title}</h3>`) injects untrusted content into the DOM. title comes from AI/tool output (place_name) and can contain HTML/JS payloads.
This is a direct XSS vector inside the map popup.
Suggestion
Avoid setHTML for untrusted strings. Use setDOMContent and assign textContent:
const popup = markerData.title
? (() => {
const el = document.createElement('div')
const h3 = document.createElement('h3')
h3.textContent = markerData.title
el.appendChild(h3)
return new mapboxgl.Popup().setDOMContent(el)
})()
: undefined
const marker = new mapboxgl.Marker()
.setLngLat([markerData.longitude, markerData.latitude])
.setPopup(popup ?? null)
.addTo(map.current!)Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Add or update markers from mapData | ||
| mapData.markers?.forEach(markerData => { | ||
| if (!markersRef.current[markerData.id]) { | ||
| const marker = new mapboxgl.Marker() | ||
| .setLngLat([markerData.longitude, markerData.latitude]) | ||
| .setPopup(markerData.title ? new mapboxgl.Popup().setHTML(`<h3>${markerData.title}</h3>`) : null) | ||
| .addTo(map.current!); | ||
| markersRef.current[markerData.id] = marker; | ||
| } else { | ||
| markersRef.current[markerData.id].setLngLat([markerData.longitude, markerData.latitude]); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This effect only updates marker position for existing markers, but it does not update popup/title when markerData.title changes. If the same id is reused with a new title (e.g., tool later resolves a nicer place name), the UI will remain stale.
Also, marker id is derived from raw floats elsewhere; small precision differences can accidentally create duplicates that look identical on the map.
Suggestion
Update popup when title changes, and consider normalizing IDs to a fixed precision.
- Popup update approach:
const existing = markersRef.current[markerData.id]
existing.setLngLat([markerData.longitude, markerData.latitude])
if (markerData.title) {
// recreate popup safely (see XSS comment) and reattach
}- ID normalization (shared helper used by
CoordinateLink+MapQueryHandler):
const coordId = (lat: number, lng: number) => `${lat.toFixed(6)},${lng.toFixed(6)}`Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| components={{ | ||
| text: (props) => { | ||
| const { children } = props; | ||
| if (typeof children !== 'string') return <>{children}</>; | ||
| const value = children; | ||
| const coordRegex = /(-?\d+\.\d+),\s*(-?\d+\.\d+)/g; | ||
| const parts = []; | ||
| let lastIndex = 0; | ||
| let match; | ||
|
|
||
| while ((match = coordRegex.exec(value)) !== null) { | ||
| const lat = parseFloat(match[1]); | ||
| const lng = parseFloat(match[2]); | ||
|
|
||
| // Basic validation for lat/lng ranges | ||
| if (lat >= -90 && lat <= 90 && lng >= -180 && lng <= 180) { | ||
| if (match.index > lastIndex) { | ||
| parts.push(value.substring(lastIndex, match.index)); | ||
| } | ||
| parts.push( | ||
| <CoordinateLink | ||
| key={`${lat}-${lng}-${match.index}`} | ||
| lat={lat} | ||
| lng={lng} | ||
| /> | ||
| ); | ||
| lastIndex = coordRegex.lastIndex; | ||
| } | ||
| } | ||
|
|
||
| if (lastIndex < value.length) { | ||
| parts.push(value.substring(lastIndex)); | ||
| } | ||
|
|
||
| return <>{parts.length > 0 ? parts : value}</>; | ||
| } | ||
| }} |
There was a problem hiding this comment.
The coordinate detection regex only matches decimals with a dot (e.g. 12.34, 56.78). It will miss many common coordinate formats such as integers (40, -74), degrees/minutes/seconds, or coordinates wrapped in parentheses.
More importantly, this replaces all matching text nodes, including ones inside code blocks/inline code depending on how react-markdown emits nodes. That can corrupt code samples or JSON snippets that contain numbers.
Finally, parts is built as an untyped array; mixing strings and React elements is fine, but the logic always returns parts even when there are no matches (parts.length > 0 becomes true because the full string is pushed at the end). That makes the : value branch effectively dead and can hide bugs.
Suggestion
Tighten the behavior:
- Only transform when at least one valid coordinate match was found.
- Skip transformations for
code/inlineCodenodes by overriding those components (or by moving this logic to a remark/rehype plugin that can respect AST node types). - Expand the regex minimally to support integer degrees:
const coordRegex = /(-?\d{1,2}(?:\.\d+)?),\s*(-?\d{1,3}(?:\.\d+)?)/gAnd track whether you inserted any CoordinateLink:
let inserted = false
...
if (valid) {
inserted = true
...
}
...
return <>{inserted ? parts : value}</>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const id = `${latitude},${longitude}`; | ||
| setMapData(prevData => { | ||
| const exists = prevData.markers?.some(m => m.id === id); | ||
| const newMarkers = exists ? prevData.markers : [...(prevData.markers || []), { id, latitude, longitude, title: place_name || id }]; | ||
|
|
||
| return { | ||
| ...prevData, | ||
| targetPosition: { lat: latitude, lng: longitude }, | ||
| markers: newMarkers, | ||
| // Optionally store more info from mcp_response if needed by MapboxMap component later | ||
| mapFeature: { | ||
| place_name, | ||
| // Potentially add mapUrl or other details from toolOutput.mcp_response | ||
| mapUrl: toolOutput.mcp_response?.mapUrl | ||
| } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
id is computed as ${latitude},${longitude} directly from floats. Tool outputs often vary slightly between calls; this can lead to duplicated markers that represent the same real-world location.
Also, place_name is used as title and later used in Mapbox popup creation (currently via HTML). Even after fixing XSS, you probably still want a consistent normalization strategy shared with CoordinateLink.
Suggestion
Introduce and reuse a shared helper to generate marker IDs with fixed precision, e.g. coordId(lat, lng) using toFixed(6).
const id = coordId(latitude, longitude)That keeps deduping stable and avoids near-duplicate markers.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
|
@coderabbitai do a full review of the pull request |
|
✅ Actions performedFull review triggered. |
@coderabbitai its not rendering these on the map.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2815 🏁 Script executed: Length of output: 1357 🏁 Script executed: Length of output: 23658 🏁 Script executed: Length of output: 1686 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 90 🏁 Script executed: Length of output: 4048 🏁 Script executed: Length of output: 12894 🏁 Script executed: Length of output: 114 🏁 Script executed: Length of output: 1702
The fix is to use the paragraph ( 'use client'
import { StreamableValue, useStreamableValue } from 'ai/rsc'
import { MemoizedReactMarkdown } from './ui/markdown'
import rehypeExternalLinks from 'rehype-external-links'
import remarkGfm from 'remark-gfm'
import remarkMath from 'remark-math'
import rehypeKatex from 'rehype-katex'
import 'katex/dist/katex.min.css'
import { CoordinateLink } from './map/coordinate-link'
import React from 'react'
export function BotMessage({ content }: { content: StreamableValue<string> }) {
const [data, error, pending] = useStreamableValue(content)
// Currently, sometimes error occurs after finishing the stream.
if (error) return <div>Error</div>
//modify the content to render LaTeX equations
const processedData = preprocessLaTeX(data || '')
// Helper function to process text and replace coordinates with CoordinateLink
const processTextForCoordinates = (text: string): React.ReactNode[] => {
const coordRegex = /(-?\d+\.\d+),\s*(-?\d+\.\d+)/g;
const parts: React.ReactNode[] = [];
let lastIndex = 0;
let match;
while ((match = coordRegex.exec(text)) !== null) {
const lat = parseFloat(match[1]);
const lng = parseFloat(match[2]);
// Basic validation for lat/lng ranges
if (lat >= -90 && lat <= 90 && lng >= -180 && lng <= 180) {
if (match.index > lastIndex) {
parts.push(text.substring(lastIndex, match.index));
}
parts.push(
<CoordinateLink
key={`${lat}-${lng}-${match.index}`}
lat={lat}
lng={lng}
/>
);
lastIndex = coordRegex.lastIndex;
}
}
if (lastIndex < text.length) {
parts.push(text.substring(lastIndex));
}
return parts.length > 0 ? parts : [text];
};
// Recursively process children to find and replace coordinates
const processChildren = (children: React.ReactNode): React.ReactNode => {
return React.Children.map(children, (child) => {
if (typeof child === 'string') {
return processTextForCoordinates(child);
}
if (React.isValidElement(child) && child.props.children) {
return React.cloneElement(child as React.ReactElement<any>, {
children: processChildren(child.props.children)
});
}
return child;
});
};
return (
<div className="overflow-x-auto">
<MemoizedReactMarkdown
rehypePlugins={[[rehypeExternalLinks, { target: '_blank' }], rehypeKatex]}
remarkPlugins={[remarkGfm, remarkMath]}
className="prose-sm prose-neutral prose-a:text-accent-foreground/50"
components={{
p: ({ children, ...props }) => {
return <p {...props}>{processChildren(children)}</p>;
},
li: ({ children, ...props }) => {
return <li {...props}>{processChildren(children)}</li>;
}
}}
>
{processedData}
</MemoizedReactMarkdown>
</div>
)
}
// Preprocess LaTeX equations to be rendered by KaTeX
// ref: https://github.com/remarkjs/react-markdown/issues/785
const preprocessLaTeX = (content: string) => {
const blockProcessedContent = content.replace(
/\\\[([\s\S]*?)\\\]/g,
(_, equation) => `$$${equation}$$`
)
const inlineProcessedContent = blockProcessedContent.replace(
/\\\(([\s\S]*?)\\\)/g,
(_, equation) => `$${equation}$`
)
return inlineProcessedContent
}Key changes:
This approach ensures that coordinates like "40.7128, -74.0060" appearing in the chat will be detected, converted to clickable CoordinateLink components, and rendered as markers on the map. 💡 Use |
Thank you for the feedback. I've updated the coordinate detection logic to support formats with degree symbols and N/S/E/W suffixes (e.g., '65.2500° N, 52.7500° W'). It should now correctly render these locations on the map. |
…ffixes - Update regex in BotMessage to handle formats like '65.2500° N, 52.7500° W' - Implement suffix parsing to correctly convert to decimal degrees - Enhance validation to avoid false positives on plain numbers without context
|
@coderabbitai its still not rendering the coordinates as links |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4039 🏁 Script executed: Length of output: 7865 🏁 Script executed: Length of output: 1357 🏁 Script executed: Length of output: 168
Here's the corrected implementation for 'use client'
import { StreamableValue, useStreamableValue } from 'ai/rsc'
import { MemoizedReactMarkdown } from './ui/markdown'
import rehypeExternalLinks from 'rehype-external-links'
import remarkGfm from 'remark-gfm'
import remarkMath from 'remark-math'
import rehypeKatex from 'rehype-katex'
import 'katex/dist/katex.min.css'
import { CoordinateLink } from './map/coordinate-link'
export function BotMessage({ content }: { content: StreamableValue<string> }) {
const [data, error, pending] = useStreamableValue(content)
// Currently, sometimes error occurs after finishing the stream.
if (error) return <div>Error</div>
// First process LaTeX equations
let processedData = preprocessLaTeX(data || '')
// Then process coordinates - convert them to special markdown links
processedData = preprocessCoordinates(processedData)
return (
<div className="overflow-x-auto">
<MemoizedReactMarkdown
rehypePlugins={[[rehypeExternalLinks, { target: '_blank' }], rehypeKatex]}
remarkPlugins={[remarkGfm, remarkMath]}
className="prose-sm prose-neutral prose-a:text-accent-foreground/50"
components={{
a: ({ node, href, children, ...props }) => {
// Check if this is a coordinate link (our special format)
if (href && href.startsWith('coord://')) {
const coords = href.replace('coord://', '').split(',')
if (coords.length === 2) {
const lat = parseFloat(coords[0])
const lng = parseFloat(coords[1])
if (!isNaN(lat) && !isNaN(lng)) {
return (
<CoordinateLink
key={`${lat}-${lng}`}
lat={lat}
lng={lng}
label={children as string}
/>
)
}
}
}
// Regular link
return <a href={href} {...props}>{children}</a>
}
}}
>
{processedData}
</MemoizedReactMarkdown>
</div>
)
}
// Preprocess coordinates to convert them into markdown links
const preprocessCoordinates = (content: string): string => {
// Match coordinate patterns: decimal numbers with comma separation
// Examples: 40.7128, -74.0060 or -33.8688, 151.2093
const coordRegex = /(-?\d+\.\d+)\s*,\s*(-?\d+\.\d+)/g
return content.replace(coordRegex, (match, lat, lng) => {
const latNum = parseFloat(lat)
const lngNum = parseFloat(lng)
// Validate lat/lng ranges
if (latNum >= -90 && latNum <= 90 && lngNum >= -180 && lngNum <= 180) {
// Convert to special markdown link format
return `[${match}](coord://${lat},${lng})`
}
// Not valid coordinates, return unchanged
return match
})
}
// Preprocess LaTeX equations to be rendered by KaTeX
// ref: https://github.com/remarkjs/react-markdown/issues/785
const preprocessLaTeX = (content: string) => {
const blockProcessedContent = content.replace(
/\\\[([\s\S]*?)\\\]/g,
(_, equation) => `$$${equation}$$`
)
const inlineProcessedContent = blockProcessedContent.replace(
/\\\(([\s\S]*?)\\\)/g,
(_, equation) => `$${equation}$`
)
return inlineProcessedContent
}How this works:
This approach is more reliable because:
The coordinates in your chat should now appear as clickable links and automatically add markers to the map! 🗺️ |
I've further improved the coordinate detection logic using |
- Use matchAll for more reliable coordinate extraction in BotMessage - Make CoordinateLink more prominent with highlighted styling - Implement visible labels for Mapbox markers using custom HTML elements - Ensure coordinate detection handles multiple variants of degree symbols
This change ensures that whenever coordinates (latitude and longitude) are mentioned in AI responses or returned by tools, they are automatically rendered as markers on the map and the map pans to that location. Coordinates in the chat are also clickable links that can re-trigger the panning.
PR created automatically by Jules for task 14070818144172209940 started by @ngoiyaeric
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.