Synchronize Map State Between Mapbox and Google Maps #409
Conversation
This commit introduces a comprehensive solution for synchronizing map state, including drawn features and camera position, between the Mapbox and Google Maps providers. Key changes include: - Extending the `MapDataContext` to include a `cameraState` field, creating a single source of truth for the map's view. - Implementing bidirectional camera state synchronization between Mapbox and Google Maps, including the necessary coordinate system conversions. - Creating a new `GoogleGeoJsonLayer` component to render GeoJSON data on the Google Map, allowing drawings made on Mapbox to be displayed on Google Maps. - Adding screenshot functionality for Google Maps using the Google Maps Static API. - Modifying the `updateDrawingContext` server action to persist both the `drawnFeatures` and `cameraState` to the database. - Making the Playwright tests in `tests/map.spec.ts` provider-aware to handle both Mapbox and Google Maps, preventing test failures when switching between providers.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 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. |
|
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThis pull request extends the map data context to include camera state tracking and integrates it across Mapbox, Google Maps, and chat components. Changes add a CameraState interface, update components to capture and synchronize camera position/rotation data, introduce a GoogleGeoJsonLayer component for Google Maps rendering, and enhance the header search button with provider-aware logic for both Mapbox and Google Maps. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Map as Map Component<br/>(Mapbox/3D)
participant Context as mapData Context
participant Chat as Chat Component
participant Action as updateDrawingContext<br/>Action
User->>Map: Interact with map<br/>(draw or move camera)
Map->>Map: Capture camera state<br/>(center, zoom, bearing, etc.)
Map->>Context: setMapData(cameraState)
Context->>Context: Update mapData.cameraState
Chat->>Context: Read drawnFeatures<br/>& cameraState
Chat->>Chat: Check both drawnFeatures<br/>& cameraState present
Chat->>Action: updateDrawingContext<br/>({ drawnFeatures, cameraState })
Action->>Action: Build message with<br/>both features & camera
Action->>Action: Persist to database
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
|
@coderabbitai get me a comprehensive review |
|
@ngoiyaeric, I'll perform a comprehensive review of this PR. I'll analyze the map state synchronization implementation, the new GeoJSON rendering layer for Google Maps, screenshot functionality, state persistence changes, and the test updates. ✅ Actions performedReview triggered.
|
|
@coderabbitai moving from google to mapbox loses context of map |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ngoiyaeric, let me investigate the map state synchronization implementation to identify why the context is being lost when switching from Google Maps to Mapbox. 🏁 Script executed: Length of output: 368 🏁 Script executed: Length of output: 155 🏁 Script executed: Length of output: 28757 @ngoiyaeric, I found the issue! When switching from Google Maps to Mapbox, the camera state is being lost because Mapbox doesn't read the Root CauseIn const currentMapCenterRef = useRef<{ center: [number, number]; zoom: number; pitch: number }>({
center: [position?.longitude ?? 0, position?.latitude ?? 0],
zoom: 2,
pitch: 0
});And then the map initialization (around line 570) uses these ref values: map.current = new mapboxgl.Map({
container: mapContainer.current,
style: 'mapbox://styles/mapbox/satellite-streets-v12',
center: currentMapCenterRef.current.center, // ❌ Uses ref, not context!
zoom: currentMapCenterRef.current.zoom, // ❌ Uses ref, not context!
pitch: currentMapCenterRef.current.pitch, // ❌ Uses ref, not context!
bearing: 0,
// ...
})Meanwhile, SolutionMapbox should check if In // Initialize map (only once)
useEffect(() => {
if (mapContainer.current && !map.current) {
let initialZoom = 2;
if (typeof window !== 'undefined' && window.innerWidth < 768) {
initialZoom = 1.3;
}
// ✅ Read from mapData.cameraState if available
const initialCenter: [number, number] = mapData.cameraState?.center
? [mapData.cameraState.center.lng, mapData.cameraState.center.lat]
: currentMapCenterRef.current.center;
const initialZoomValue = mapData.cameraState?.zoom ?? currentMapCenterRef.current.zoom ?? initialZoom;
const initialPitch = mapData.cameraState?.pitch ?? currentMapCenterRef.current.pitch;
const initialBearing = mapData.cameraState?.bearing ?? 0;
map.current = new mapboxgl.Map({
container: mapContainer.current,
style: 'mapbox://styles/mapbox/satellite-streets-v12',
center: initialCenter,
zoom: initialZoomValue,
pitch: initialPitch,
bearing: initialBearing,
maxZoom: 22,
attributionControl: true,
preserveDrawingBuffer: true
})
// ... rest of initializationAlso add Would you like me to create a code change plan to fix this issue? |
Thank you for catching this! You're right, the context is lost when switching from Google Maps back to Mapbox. I've identified the issue and will implement a fix now. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The biggest issues are over-eager persistence of map context (can spam DB/chat history) and incorrect/fragile Google Static Maps zoom computation, which will make screenshot capture unreliable. The camera state model currently mixes Mapbox and Google 3D concepts in one object, increasing the odds of stale/partial state and conversion bugs. Lastly, playwright.config.ts hardcodes a new port, likely breaking CI/local runs unless the environment matches.
Additional notes (2)
-
Security |
components/header-search-button.tsx:36-36
The Google Static Maps URL is constructed by string interpolation withoutencodeURIComponentand uses a hard-coded size. Ifcenterever becomes invalid/NaN you’ll generate bad requests; alsorange→zoomconversion is a rough heuristic and can yield out-of-bounds zoom values. Finally, this fetch happens from the browser directly to Google with the API key in the URL, which increases key exposure and makes it easier to scrape from logs/network history. -
Performance |
lib/hooks/use-deep-compare-effect.ts:18-22
useDeepCompareEffectnow includescallbackin the dependency array. This partially defeats the point of deep-compare dependencies becausecallbackis often re-created each render (unlessuseCallback), causing the effect to rerun even when deep dependencies didn’t change.
If the goal is to avoid stale closures, the usual pattern is storing the latest callback in a ref and invoking it from a stable effect triggered by deep-compared deps.
Summary of changes
What this PR changes
-
Persists richer map context into chat history
components/chat.tsx: server action call now sends a combined payload{ drawnFeatures, cameraState }instead of onlydrawnFeatures.lib/actions/chat.ts:updateDrawingContext(chatId, contextData)now stores the combined context payload viaJSON.stringify(contextData).
-
Adds Google Maps support for rendering drawings and capturing screenshots
components/map/google-geojson-layer.tsx(new): adds agoogle.maps.Datalayer that renders aGeoJSON.FeatureCollectiononto Google Maps.components/map/google-map.tsx: builds a memoized GeoJSONFeatureCollectionfrommapData.drawnFeatures, derivescameraOptionsfromcameraState, and rendersGoogleGeoJsonLayer.components/header-search-button.tsx: screenshot capture is now provider-aware:- Mapbox: grabs
canvas.toBlob(). - Google: fetches a Static Maps image using
NEXT_PUBLIC_GOOGLE_MAPS_API_KEYand the stored camera state.
- Mapbox: grabs
-
Synchronizes camera state between providers
components/map/map-data-context.tsx: introducesCameraStateandMapData.cameraState.components/map/mapbox-map.tsx: capturesbearingand writescameraStateintoMapData.components/map/map-3d.tsx: listens to 3D camera changes and writes a Google camera state representation intoMapData.
-
Test/runtime adjustments
tests/map.spec.ts: makes tests provider-aware and improves zoom assertions by waiting forzoomend.playwright.config.ts: changesbaseURLfromhttp://localhost:3000tohttp://localhost:3003.lib/hooks/use-deep-compare-effect.ts: includescallbackin the effect dependencies.
| // useEffect to call the server action when drawnFeatures changes | ||
| useEffect(() => { | ||
| if (id && mapData.drawnFeatures && mapData.drawnFeatures.length > 0) { | ||
| if (id && mapData.drawnFeatures && mapData.cameraState) { | ||
| console.log('Chat.tsx: drawnFeatures changed, calling updateDrawingContext', mapData.drawnFeatures); | ||
| updateDrawingContext(id, mapData.drawnFeatures); | ||
| updateDrawingContext(id, { | ||
| drawnFeatures: mapData.drawnFeatures, | ||
| cameraState: mapData.cameraState, | ||
| }); | ||
| } | ||
| }, [id, mapData.drawnFeatures]); | ||
| }, [id, mapData.drawnFeatures, mapData.cameraState]); |
There was a problem hiding this comment.
updateDrawingContext is fired on any change to drawnFeatures/cameraState, but the guard is too permissive: drawnFeatures could be [] and you’ll still create a new data message (potentially on every camera move once cameraState exists). This can spam the DB/chat history and make later retrieval ambiguous.
This is a correctness/behavior issue: the diff moved from length > 0 to merely checking truthiness, which changes semantics.
Suggestion
Restore a stricter guard and/or add throttling/debouncing so you don’t persist on every minor camera update. For example:
useEffect(() => {
if (!id) return;
if (!mapData.cameraState) return;
const features = mapData.drawnFeatures ?? [];
if (features.length === 0) return;
updateDrawingContext(id, { drawnFeatures: features, cameraState: mapData.cameraState });
}, [id, mapData.drawnFeatures, mapData.cameraState]);If you do want to persist camera changes, add a debounce (e.g. 500–1000ms) and only write when the camera settles (provider events already expose idle/zoomend equivalents). Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing debounced persistence.
| let blob: Blob | null = null; | ||
|
|
||
| if (mapProvider === 'mapbox') { | ||
| const canvas = map!.getCanvas() | ||
| blob = await new Promise<Blob | null>(resolve => { | ||
| canvas.toBlob(resolve, 'image/png') | ||
| }) | ||
| } else if (mapProvider === 'google') { | ||
| const apiKey = process.env.NEXT_PUBLIC_GOOGLE_MAPS_API_KEY | ||
| if (!apiKey || !mapData.cameraState) { | ||
| toast.error('Google Maps API key or camera state is not available.') | ||
| setIsAnalyzing(false) | ||
| return | ||
| } | ||
| const { center, range } = mapData.cameraState | ||
| const zoom = Math.round(Math.log2(40000000 / (range || 1))); | ||
|
|
||
| let staticMapUrl = `https://maps.googleapis.com/maps/api/staticmap?center=${center.lat},${center.lng}&zoom=${zoom}&size=640x480&maptype=satellite&key=${apiKey}`; | ||
|
|
||
| const response = await fetch(staticMapUrl); | ||
| if (!response.ok) { | ||
| throw new Error('Failed to fetch static map image.'); | ||
| } | ||
| blob = await response.blob(); | ||
| } |
There was a problem hiding this comment.
The Google Static Maps URL construction has multiple functional issues:
zoomderivation usesrangewith a hardcoded40000000andlog2which is not a valid mapping between Google’s zoom levels and the 3Drange(meters) used by Maps 3D. This will frequently produce unusable zoom values (too high/low) and can even go negative.- The URL uses
maptype=satellitebut omits things likescale=2(better quality) and doesn’t encode values;centershould be URL-encoded. - There’s no cap for Google Static Maps zoom (0–21 typically), so out-of-range will fail.
This will make screenshot capture flaky/incorrect in production.
Suggestion
Introduce a small, tested helper to map cameraState to Static Maps parameters, clamp zoom, and encode the URL. If you only have center reliably, prefer a conservative fixed zoom (or persist an explicit zoom for Google) rather than guessing from range.
Example:
const clamp = (n: number, min: number, max: number) => Math.min(max, Math.max(min, n));
function toStaticMapsUrl(center: {lat:number; lng:number}, zoom: number, apiKey: string) {
const z = clamp(Math.round(zoom), 0, 21);
const qs = new URLSearchParams({
center: `${center.lat},${center.lng}`,
zoom: String(z),
size: '640x480',
scale: '2',
maptype: 'satellite',
key: apiKey,
});
return `https://maps.googleapis.com/maps/api/staticmap?${qs.toString()}`;
}Then use mapData.cameraState.zoom ?? DEFAULT_ZOOM and only fall back to range if you can validate the mapping. Reply with "@CharlieHelps yes please" if you’d like me to add a commit with a helper + clamping/encoding.
| useEffect(() => { | ||
| if (!map) return; | ||
|
|
||
| // Remove existing layer if it exists | ||
| if (layerRef.current) { | ||
| layerRef.current.setMap(null); | ||
| } | ||
|
|
||
| // Create a new data layer | ||
| const newLayer = new google.maps.Data(); | ||
| layerRef.current = newLayer; | ||
|
|
||
| // Set styles for polygons and polylines | ||
| newLayer.setStyle(feature => { | ||
| const geometryType = feature.getGeometry()?.getType(); | ||
| if (geometryType === 'Polygon') { | ||
| return { | ||
| fillColor: '#088', | ||
| fillOpacity: 0.4, | ||
| strokeColor: '#088', | ||
| strokeWeight: 2, | ||
| }; | ||
| } | ||
| if (geometryType === 'LineString') { | ||
| return { | ||
| strokeColor: '#088', | ||
| strokeWeight: 2, | ||
| }; | ||
| } | ||
| return {}; | ||
| }); | ||
|
|
||
| // Add GeoJSON data to the layer | ||
| newLayer.addGeoJson(data); | ||
|
|
||
| // Set the map for the new layer | ||
| newLayer.setMap(map); | ||
|
|
||
| return () => { | ||
| if (newLayer) { | ||
| newLayer.setMap(null); | ||
| } | ||
| }; | ||
| }, [map, data]); |
There was a problem hiding this comment.
The GoogleGeoJsonLayer recreates the entire google.maps.Data layer on every data change. For frequent updates (draw edits, camera sync), this can be expensive and can also cause brief flicker.
You can keep a single Data layer instance for the component lifetime, then clear and re-add features on data changes (forEach + remove) instead of setMap(null) and new google.maps.Data() each time.
Suggestion
Create the Data layer once when map becomes available, then update its contents when data changes.
useEffect(() => {
if (!map || layerRef.current) return;
layerRef.current = new google.maps.Data({ map });
layerRef.current.setStyle(/* ... */);
return () => layerRef.current?.setMap(null);
}, [map]);
useEffect(() => {
const layer = layerRef.current;
if (!layer) return;
layer.forEach(f => layer.remove(f));
layer.addGeoJson(data);
}, [data]);Reply with "@CharlieHelps yes please" if you’d like me to add a commit refactoring this to avoid layer churn.
| useCallbackRef<google.maps.maps3d.Map3DElement>(); | ||
|
|
||
| useMap3DCameraEvents(map3DElement, p => { | ||
| const { center, range, heading, tilt } = p.detail; | ||
| const lat = center.lat(); | ||
| const lng = center.lng(); | ||
| setMapData(prevData => ({ | ||
| ...prevData, | ||
| cameraState: { | ||
| ...prevData.cameraState, | ||
| center: { lat, lng }, | ||
| range, | ||
| heading, | ||
| tilt | ||
| } | ||
| })); |
There was a problem hiding this comment.
Map3D writes cameraState using Google-flavored keys (range, tilt, heading), while Mapbox writes Mapbox-flavored keys (zoom, pitch, bearing). Mixing both into one interface makes it easy to end up with partially-populated state and incorrect conversions.
Also, the update currently spreads ...prevData.cameraState which can carry incompatible fields across providers (e.g., keeping an old zoom while updating range), which can break downstream logic that assumes one source of truth.
Suggestion
Normalize cameraState to a single canonical representation (recommended: {center, zoom, bearing, pitch} like Mapbox) and derive provider-specific fields at the edges. For Google Maps 3D events, compute an equivalent zoom (or store both but namespaced):
cameraState: {
provider: 'google',
center: {lat, lng},
google: { range, heading, tilt },
// optionally derived canonical values
}At minimum: stop spreading the previous camera state across provider updates; overwrite it explicitly per provider update to avoid stale fields.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that introduces a canonical camera model + provider adapters.
| reporter: 'html', | ||
| use: { | ||
| baseURL: 'http://localhost:3000', | ||
| baseURL: 'http://localhost:3003', | ||
| trace: 'on-first-retry', | ||
| }, |
There was a problem hiding this comment.
Changing Playwright baseURL to http://localhost:3003 hardcodes an environment assumption into the repo config and will break local runs/CI where the app still runs on 3000.
This should be driven by an env var with a sensible default.
Suggestion
Use process.env.PLAYWRIGHT_BASE_URL (or BASE_URL) with a fallback:
use: {
baseURL: process.env.PLAYWRIGHT_BASE_URL ?? 'http://localhost:3000',
trace: 'on-first-retry',
},Reply with "@CharlieHelps yes please" if you’d like me to add a commit updating the config + CI docs/env.
| const cameraOptions = useMemo(() => { | ||
| if (mapData.cameraState) { | ||
| const { center, zoom, pitch, bearing } = mapData.cameraState; | ||
| // Convert Mapbox zoom to Google Maps range (approximate) | ||
| const range = zoom ? 40000000 / Math.pow(2, zoom) : 20000000; | ||
| return { | ||
| center, | ||
| range, | ||
| tilt: pitch || 0, | ||
| heading: bearing || 0, | ||
| }; | ||
| } | ||
| if (mapData.targetPosition) { | ||
| return { center: mapData.targetPosition, range: 1000, tilt: 60, heading: 0 }; | ||
| } | ||
| return { center: { lat: 37.7749, lng: -122.4194 }, range: 1000, tilt: 60, heading: 0 }; | ||
| }, [mapData.cameraState, mapData.targetPosition]); |
There was a problem hiding this comment.
cameraOptions derivation mixes Mapbox and Google camera concepts and computes range from zoom with a magic constant. This risks unstable camera syncing (jumping/oscillation) when switching providers because Map3D also writes back range/heading/tilt into cameraState. You can end up with a loop where Mapbox→zoom→range→Map3D emits range→saved→recomputed, etc.
At minimum, store a provider-normalized camera model and convert at the boundary, or track which provider last authored the camera state to avoid feedback loops.
Suggestion
Introduce a normalized camera state schema (e.g., always store center + zoom + bearing + pitch), and in Google Map compute range from zoom only for the view component (do not persist range). Alternatively, persist both but add source: 'mapbox' | 'google' and only convert when consuming from the other provider.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing source tagging and boundary conversions to prevent feedback loops.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/map/mapbox-map.tsx (1)
318-337: Storebearingconsistently (don’t compute it and then drop it).
bearingis read (Line 323) and persisted tomapData.cameraState(Line 328-333), butcurrentMapCenterRef.currentis reassigned without bearing (Line 324), and its type omits it—easy to lose rotation state in later flows.Proposed patch
- const currentMapCenterRef = useRef<{ center: [number, number]; zoom: number; pitch: number }>({ center: [position?.longitude ?? 0, position?.latitude ?? 0], zoom: 2, pitch: 0 }); + const currentMapCenterRef = useRef<{ center: [number, number]; zoom: number; pitch: number; bearing: number }>({ + center: [position?.longitude ?? 0, position?.latitude ?? 0], + zoom: 2, + pitch: 0, + bearing: 0 + }); @@ const captureMapCenter = useCallback(() => { if (map.current) { const center = map.current.getCenter(); const zoom = map.current.getZoom(); const pitch = map.current.getPitch(); const bearing = map.current.getBearing(); - currentMapCenterRef.current = { center: [center.lng, center.lat], zoom, pitch }; + currentMapCenterRef.current = { center: [center.lng, center.lat], zoom, pitch, bearing }; setMapData(prevData => ({ ...prevData, cameraState: { center: { lat: center.lat, lng: center.lng }, zoom, pitch, bearing } })); } }, [setMapData])components/map/map-data-context.tsx (1)
5-25: Tighten GeoJSON typing (avoidany) for persist-safe cross-provider rendering.
CameraStateandcameraStatetyping are sound, butdrawnFeatures[].geometry: any(line 24) andmapFeature?: any(line 19) will create serialization and validation gaps across data sources.Suggested direction:
- Import and use proper GeoJSON types (
@types/geojsonor equivalent) forgeometryandmapFeaturefields instead ofany.- Consider making
drawnFeaturesnon-optional in the type definition (line 20) to match the provider's initialization pattern (line 41:drawnFeatures: []). This eliminates unnecessary optional checks downstream.components/map/map-3d.tsx (1)
15-45: Consider throttlingsetMapDataupdates on high-frequencycamerachangeevents.You're updating context on every
camerachangeevent (lines 32–41), which can be high-frequency during camera interaction. This may cause excessive re-renders depending on how context consumers are memoized. Consider throttling the context updates usingrequestAnimationFrameor a time-based debounce to reduce render pressure.The
useMap3DCameraEventshook correctly rebinds the listener when the handler changes (handler is in the dependency array), so the event listener will stay in sync withprops.onCameraChange.
🤖 Fix all issues with AI agents
In @components/header-search-button.tsx:
- Around line 21-25: In HeaderSearchButton, fix three client issues and add a
server proxy: update the disabled prop to use provider-aware logic
(disabled={isAnalyzing || !actions || (mapProvider === 'mapbox' && !map)}) so
Google buttons aren’t incorrectly disabled; move Google API key and cameraState
validation (the checks that currently happen after setMessages) to before
calling setMessages to avoid dangling user messages on failure; clamp derived
zoom using Math.min(21, Math.max(0, zoomValue)) to keep zoom in [0,21]; and
replace the direct browser fetch of the Google Static Maps URL with a call to a
new server-side API route that proxies the Google request (server endpoint
fetches the image with the API key, returns the image blob/base64 and proper
CORS headers) and update the client code to call that route and only append
messages/set state after the proxied fetch succeeds.
In @components/map/google-geojson-layer.tsx:
- Around line 27-44: The current newLayer.setStyle callback only handles
'Polygon' and 'LineString' so Point features render with defaults; update the
component to accept a styleOptions prop (e.g., { color, fillOpacity,
strokeWeight, pointIcon }) with sensible defaults and use those values inside
newLayer.setStyle (inspect feature.getGeometry()?.getType() as geometryType) to
add a branch for 'Point' that returns a marker/icon style (or circle marker)
using styleOptions.pointIcon or color for stroke/fill, and replace hard-coded
'#088' in the Polygon and LineString branches with styleOptions.color; ensure
the prop is typed/optional and defaults are applied when undefined.
- Around line 14-57: The effect currently recreates the Google Data layer on
every change of data (useEffect with layerRef and newLayer), so refactor to
create the Data layer once (on map ready) and reuse layerRef.current thereafter,
then implement incremental updates: compare incoming data to existing features
on layerRef.current (use Data.forEach, getId()/properties or geometry hashes) to
addGeoJson only new features and remove or update changed ones instead of
calling addGeoJson for all; if updates are very frequent, wrap the update logic
in a debounced handler (e.g., debounce the effect trigger or update function) to
reduce churn and avoid repeatedly calling setMap(null)/setMap(map).
In @lib/actions/chat.ts:
- Line 165: Replace the loose any types in updateDrawingContext with
well-defined interfaces: create types DrawnFeature, CameraState and
DrawingContextData (e.g., in lib/types/map.ts) and update the function signature
of updateDrawingContext(chatId: string, contextData: DrawingContextData) so
drawnFeatures is DrawnFeature[] and cameraState is CameraState; ensure any
internal uses (e.g., accessing feature.id, geometry, measurement and
cameraState.center/zoom/pitch/bearing) conform to the new interfaces and update
imports to reference the new types.
In @tests/map.spec.ts:
- Around line 25-50: Extract the Mapbox runtime/provider check into a reusable
helper (e.g., skipIfNotMapbox) and call it from tests instead of duplicating the
locator + test.skip logic: move the logic that awaits
page.locator('.mapboxgl-canvas').isVisible() and calls test.skip into a
top-level async function (suggested name skipIfNotMapbox) that returns a
boolean, and update the test('should zoom in and out using map controls', ...)
to call await skipIfNotMapbox(page) and early return if it returns false; ensure
the helper preserves the existing behavior (calling test.skip(true, ...) when
not Mapbox) and also keep the existing map presence check (Boolean((window as
any).map)) either inside the helper or as a separate small helper used in the
test.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
dev_server.logis excluded by!**/*.log
📒 Files selected for processing (11)
components/chat.tsxcomponents/header-search-button.tsxcomponents/map/google-geojson-layer.tsxcomponents/map/google-map.tsxcomponents/map/map-3d.tsxcomponents/map/map-data-context.tsxcomponents/map/mapbox-map.tsxlib/actions/chat.tslib/hooks/use-deep-compare-effect.tsplaywright.config.tstests/map.spec.ts
🧰 Additional context used
🧬 Code graph analysis (4)
components/map/google-geojson-layer.tsx (1)
components/map/map-context.tsx (1)
useMap(24-30)
components/chat.tsx (1)
lib/actions/chat.ts (1)
updateDrawingContext(165-205)
components/map/map-3d.tsx (3)
components/map/map-3d-types.ts (2)
Map3DProps(38-47)Map3DElement(23-30)components/map/map-data-context.tsx (1)
useMapData(50-56)lib/hooks/use-map-3d-camera-events.ts (1)
useMap3DCameraEvents(6-23)
components/header-search-button.tsx (2)
lib/store/settings.ts (1)
useSettingsStore(10-13)components/map/map-data-context.tsx (1)
useMapData(50-56)
🔇 Additional comments (6)
components/map/google-map.tsx (2)
38-52: LGTM! Clean GeoJSON transformation.The memoized featureCollection correctly transforms drawnFeatures into a GeoJSON FeatureCollection, preserving geometry, id, and measurement properties. The null-safety with
|| []and proper type assertions are well done.
83-83: LGTM! Proper integration of GeoJSON layer.The GoogleGeoJsonLayer is correctly placed within the APIProvider and receives the memoized featureCollection.
tests/map.spec.ts (2)
6-8: LGTM! Provider-aware test initialization.The test now waits for either Mapbox or Google Maps to load, making it compatible with both providers.
10-23: LGTM! Proper runtime provider detection.The test correctly checks for Mapbox presence and skips if unavailable, as drawing controls are Mapbox-specific.
lib/actions/chat.ts (1)
165-205: LGTM! Clean integration of camera state persistence.The function correctly extends the persistence logic to include both drawnFeatures and cameraState in the message content. The authentication flow, error handling, and database operations are properly implemented.
components/chat.tsx (1)
78-85: Consider the truthiness of empty arrays in the conditional check.The condition
if (id && mapData.drawnFeatures && mapData.cameraState)has a subtle issue:drawnFeaturesis initialized as an empty array[], which is falsy in JavaScript. This means the effect won't trigger when:
- Features are drawn but
cameraStateis undefinedcameraStateis set butdrawnFeaturesis emptySince
updateDrawingContextrequires both parameters and they're updated independently in map components, consider whether the condition should check for definedness rather than truthiness:if (id && mapData.drawnFeatures !== undefined && mapData.cameraState !== undefined). Alternatively, if persistence should only occur when features actually exist, explicitly check array length:if (id && mapData.drawnFeatures?.length && mapData.cameraState).
| export function HeaderSearchButton() { | ||
| const { map } = useMap() | ||
| const { mapProvider } = useSettingsStore() | ||
| const { mapData } = useMapData() | ||
| // Cast the actions to our defined interface to avoid build errors |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the header-search-button component
find . -name "header-search-button.tsx" -type fRepository: QueueLab/QCX
Length of output: 93
🌐 Web query:
Google Static Maps API CORS browser fetch support client-side
💡 Result:
Short answer: You cannot reliably fetch/inspect Google Maps Static API images from browser JavaScript because Google’s Maps web-service endpoints do not send CORS response headers (so fetch/XHR will be blocked and drawing the image into a canvas will taint it). Use an tag to display the static map, or else proxy the request through your own server (or use the Maps JavaScript client APIs) if you need to fetch/inspect image bytes or avoid CORS issues. [1][2][3]
References/sources:
- Maps Static API docs (usage for embedding images). [1]
- Multiple issues/threads showing Google Maps web-service endpoints do not return Access‑Control‑Allow‑Origin (CORS) for browser fetches. [2]
- Browser notes on cross‑origin images / canvas tainting (why an image without CORS headers can’t be read via canvas/fetch). [3]
🏁 Script executed:
# Once we find the file, let's examine the critical sections mentioned
cat -n components/header-search-button.tsx | head -130Repository: QueueLab/QCX
Length of output: 5391
Fix Google provider being unintentionally disabled, prevent dangling chat messages, and resolve CORS blocking of Static Maps fetch.
Google flow doesn't require map, but both buttons are disabled when !map (Line 108, 120). Additionally, the user message is appended before Google precondition checks (Line 51–57), causing a dangling message if the fetch fails. Most critically, the browser fetch() at line 78 will fail because Google Static Maps API does not return CORS headers—this is a hard blocker. The zoom derivation at line 74 is also unbounded and can produce invalid values (>21 or <0).
Required fixes:
- Make button disabled state provider-aware:
disabled={isAnalyzing || !actions || (mapProvider === 'mapbox' && !map)} - Move Google API key /
cameraStatevalidation beforesetMessages() - Clamp zoom to valid range:
Math.min(21, Math.max(0, zoomValue)) - Server-side proxy for Static Maps fetch — client-side fetch cannot work due to CORS restrictions. Use an API route to proxy the request and return the image blob.
Proposed patch outline
export function HeaderSearchButton() {
const { map } = useMap()
const { mapProvider } = useSettingsStore()
const { mapData } = useMapData()
const handleResolutionSearch = async () => {
if (mapProvider === 'mapbox' && !map) {
toast.error('Map is not available yet. Please wait for it to load.')
return
}
if (!actions) {
toast.error('Search actions are not available.')
return
}
setIsAnalyzing(true)
try {
+ // Validate preconditions BEFORE emitting user message
+ if (mapProvider === 'google') {
+ const apiKey = process.env.NEXT_PUBLIC_GOOGLE_MAPS_API_KEY
+ if (!apiKey || !mapData.cameraState) {
+ toast.error('Google Maps API key or camera state is not available.')
+ setIsAnalyzing(false)
+ return
+ }
+ }
+
setMessages(currentMessages => [
...currentMessages,
{
id: nanoid(),
component: <UserMessage content={[{ type: 'text', text: 'Analyze this map view.' }]} />
}
])
let blob: Blob | null = null;
if (mapProvider === 'mapbox') {
const canvas = map!.getCanvas()
blob = await new Promise<Blob | null>(resolve => {
canvas.toBlob(resolve, 'image/png')
})
} else if (mapProvider === 'google') {
const { center, range } = mapData.cameraState
- const zoom = Math.round(Math.log2(40000000 / (range || 1)));
+ const zoomGuess = Math.round(Math.log2(40000000 / Math.max(1, range ?? 1)))
+ const zoom = Math.min(21, Math.max(0, zoomGuess))
- let staticMapUrl = `https://maps.googleapis.com/maps/api/staticmap?center=${center.lat},${center.lng}&zoom=${zoom}&size=640x480&maptype=satellite&key=${apiKey}`;
-
- const response = await fetch(staticMapUrl);
+ // Use server-side proxy instead of direct fetch (CORS incompatible)
+ const response = await fetch('/api/static-map', {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({ center, zoom })
+ })
if (!response.ok) {
throw new Error('Failed to fetch static map image.');
}
blob = await response.blob();
}
- const desktopButton = (
+ const desktopButton = (
<Button
variant="ghost"
size="icon"
onClick={handleResolutionSearch}
- disabled={isAnalyzing || !map || !actions}
+ disabled={isAnalyzing || !actions || (mapProvider === 'mapbox' && !map)}
title="Analyze current map view"
>
{isAnalyzing ? (
<div className="h-5 w-5 animate-spin rounded-full border-b-2 border-current"></div>
) : (
<Search className="h-[1.2rem] w-[1.2rem]" />
)}
</Button>
)
const mobileButton = (
- <Button variant="ghost" size="sm" onClick={handleResolutionSearch} disabled={isAnalyzing || !map || !actions}>
+ <Button variant="ghost" size="sm" onClick={handleResolutionSearch} disabled={isAnalyzing || !actions || (mapProvider === 'mapbox' && !map)}>
<Search className="h-4 w-4 mr-2" />
Search
</Button>
)🤖 Prompt for AI Agents
In @components/header-search-button.tsx around lines 21 - 25, In
HeaderSearchButton, fix three client issues and add a server proxy: update the
disabled prop to use provider-aware logic (disabled={isAnalyzing || !actions ||
(mapProvider === 'mapbox' && !map)}) so Google buttons aren’t incorrectly
disabled; move Google API key and cameraState validation (the checks that
currently happen after setMessages) to before calling setMessages to avoid
dangling user messages on failure; clamp derived zoom using Math.min(21,
Math.max(0, zoomValue)) to keep zoom in [0,21]; and replace the direct browser
fetch of the Google Static Maps URL with a call to a new server-side API route
that proxies the Google request (server endpoint fetches the image with the API
key, returns the image blob/base64 and proper CORS headers) and update the
client code to call that route and only append messages/set state after the
proxied fetch succeeds.
| useEffect(() => { | ||
| if (!map) return; | ||
|
|
||
| // Remove existing layer if it exists | ||
| if (layerRef.current) { | ||
| layerRef.current.setMap(null); | ||
| } | ||
|
|
||
| // Create a new data layer | ||
| const newLayer = new google.maps.Data(); | ||
| layerRef.current = newLayer; | ||
|
|
||
| // Set styles for polygons and polylines | ||
| newLayer.setStyle(feature => { | ||
| const geometryType = feature.getGeometry()?.getType(); | ||
| if (geometryType === 'Polygon') { | ||
| return { | ||
| fillColor: '#088', | ||
| fillOpacity: 0.4, | ||
| strokeColor: '#088', | ||
| strokeWeight: 2, | ||
| }; | ||
| } | ||
| if (geometryType === 'LineString') { | ||
| return { | ||
| strokeColor: '#088', | ||
| strokeWeight: 2, | ||
| }; | ||
| } | ||
| return {}; | ||
| }); | ||
|
|
||
| // Add GeoJSON data to the layer | ||
| newLayer.addGeoJson(data); | ||
|
|
||
| // Set the map for the new layer | ||
| newLayer.setMap(map); | ||
|
|
||
| return () => { | ||
| if (newLayer) { | ||
| newLayer.setMap(null); | ||
| } | ||
| }; | ||
| }, [map, data]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider performance optimization for frequent data updates.
The current implementation recreates the entire Data layer on every data change (lines 18-24), which could cause performance issues if features are updated frequently.
Consider:
- Incremental updates: Check if features have actually changed before recreating the layer
- Delta updates: Add/remove only changed features instead of recreating everything
- Debouncing: If updates are very frequent, debounce the effect
♻️ Example with change detection
const map = useMap();
const layerRef = useRef<google.maps.Data | null>(null);
+ const prevDataRef = useRef<string>('');
useEffect(() => {
if (!map) return;
+ // Check if data actually changed
+ const dataString = JSON.stringify(data);
+ if (dataString === prevDataRef.current && layerRef.current) {
+ return;
+ }
+ prevDataRef.current = dataString;
// Remove existing layer if it exists
if (layerRef.current) {
layerRef.current.setMap(null);
}
// ... rest of the code
}, [map, data]);🤖 Prompt for AI Agents
In @components/map/google-geojson-layer.tsx around lines 14 - 57, The effect
currently recreates the Google Data layer on every change of data (useEffect
with layerRef and newLayer), so refactor to create the Data layer once (on map
ready) and reuse layerRef.current thereafter, then implement incremental
updates: compare incoming data to existing features on layerRef.current (use
Data.forEach, getId()/properties or geometry hashes) to addGeoJson only new
features and remove or update changed ones instead of calling addGeoJson for
all; if updates are very frequent, wrap the update logic in a debounced handler
(e.g., debounce the effect trigger or update function) to reduce churn and avoid
repeatedly calling setMap(null)/setMap(map).
| newLayer.setStyle(feature => { | ||
| const geometryType = feature.getGeometry()?.getType(); | ||
| if (geometryType === 'Polygon') { | ||
| return { | ||
| fillColor: '#088', | ||
| fillOpacity: 0.4, | ||
| strokeColor: '#088', | ||
| strokeWeight: 2, | ||
| }; | ||
| } | ||
| if (geometryType === 'LineString') { | ||
| return { | ||
| strokeColor: '#088', | ||
| strokeWeight: 2, | ||
| }; | ||
| } | ||
| return {}; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add support for Point geometries and make colors configurable.
The styling currently only handles Polygon and LineString geometries. Point features (markers) are not styled and will use default appearance.
- Add Point support for complete GeoJSON rendering:
♻️ Add Point geometry styling
newLayer.setStyle(feature => {
const geometryType = feature.getGeometry()?.getType();
if (geometryType === 'Polygon') {
return {
fillColor: '#088',
fillOpacity: 0.4,
strokeColor: '#088',
strokeWeight: 2,
};
}
if (geometryType === 'LineString') {
return {
strokeColor: '#088',
strokeWeight: 2,
};
}
+ if (geometryType === 'Point') {
+ return {
+ icon: {
+ path: google.maps.SymbolPath.CIRCLE,
+ fillColor: '#088',
+ fillOpacity: 0.8,
+ strokeColor: '#fff',
+ strokeWeight: 2,
+ scale: 8,
+ },
+ };
+ }
return {};
});- Make colors configurable by accepting a
styleOptionsprop instead of hard-coding#088:
♻️ Make colors configurable via props
interface GoogleGeoJsonLayerProps {
data: GeoJSON.FeatureCollection;
+ styleOptions?: {
+ fillColor?: string;
+ strokeColor?: string;
+ fillOpacity?: number;
+ strokeWeight?: number;
+ };
}
- export function GoogleGeoJsonLayer({ data }: GoogleGeoJsonLayerProps) {
+ export function GoogleGeoJsonLayer({
+ data,
+ styleOptions = {
+ fillColor: '#088',
+ strokeColor: '#088',
+ fillOpacity: 0.4,
+ strokeWeight: 2,
+ }
+ }: GoogleGeoJsonLayerProps) {
// ... then use styleOptions in the setStyle callback🤖 Prompt for AI Agents
In @components/map/google-geojson-layer.tsx around lines 27 - 44, The current
newLayer.setStyle callback only handles 'Polygon' and 'LineString' so Point
features render with defaults; update the component to accept a styleOptions
prop (e.g., { color, fillOpacity, strokeWeight, pointIcon }) with sensible
defaults and use those values inside newLayer.setStyle (inspect
feature.getGeometry()?.getType() as geometryType) to add a branch for 'Point'
that returns a marker/icon style (or circle marker) using styleOptions.pointIcon
or color for stroke/fill, and replace hard-coded '#088' in the Polygon and
LineString branches with styleOptions.color; ensure the prop is typed/optional
and defaults are applied when undefined.
| const cameraOptions = useMemo(() => { | ||
| if (mapData.cameraState) { | ||
| const { center, zoom, pitch, bearing } = mapData.cameraState; | ||
| // Convert Mapbox zoom to Google Maps range (approximate) | ||
| const range = zoom ? 40000000 / Math.pow(2, zoom) : 20000000; | ||
| return { | ||
| center, | ||
| range, | ||
| tilt: pitch || 0, | ||
| heading: bearing || 0, | ||
| }; | ||
| } | ||
| if (mapData.targetPosition) { | ||
| return { center: mapData.targetPosition, range: 1000, tilt: 60, heading: 0 }; | ||
| } | ||
| return { center: { lat: 37.7749, lng: -122.4194 }, range: 1000, tilt: 60, heading: 0 }; | ||
| }, [mapData.cameraState, mapData.targetPosition]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Mapbox zoom level to altitude meters conversion formula
💡 Result:
Short answer — two steps:
-
Meters per pixel at latitude φ and zoom z:
resolution = 156543.03392804062 * cos(φ * π/180) / 2^z. [1][2] -
Convert that to camera altitude (meters) for a perspective camera with viewport height H (pixels) and vertical field‑of‑view fov (radians):
altitude = (0.5 * H * resolution) / tan(fov/2).
Combine (substituting resolution):
altitude = (0.5 * H * 156543.03392804062 * cos(φ * π/180) / 2^z) / tan(fov/2).
Notes/sources:
- Mapbox/tiles zoom ↔ meters-per-pixel and zoom glossary. [1]
- standard Mercator resolution formula (156543.0339 * cos φ / 2^z). [2]
- Mapbox SDK provides MGLAltitudeForZoomLevel / MGLZoomLevelForAltitude; deck.gl / WebMercatorViewport describe altitude↔fov/viewport relations. Use those helpers when available. [3][4]
References:
[1] Mapbox zoom-level docs.
[2] Microsoft/Bing maps resolution formula.
[3] Mapbox SDK docs (MGLAltitudeForZoomLevel).
[4] deck.gl / WebMercatorViewport (camera altitude ⇄ fov/viewport).
🌐 Web query:
Google Maps 3D camera range altitude zoom level documentation
💡 Result:
Quick references from Google's docs:
- "Range" = camera distance (meters) between camera and map center; 0 m up to 63,170,000 m (max). See Maps 3D camera controls (Android/iOS/JS). (developers.google.com)
- Map center can include altitude (LatLngAltitude, meters above ground); default altitude = 63,170,000 m. (developers.google.com)
- Zoom level (2D APIs / SDK) is a separate concept: zoom 0 = whole world ≈256dp wide; each +1 doubles scale (zoom 15 ≈ streets, 20 ≈ buildings). Use getMinimumZoomLevel()/getMaximumZoomLevel() for device-specific limits. (developers.google.com)
- Camera restrictions / bounds, tilt, heading, and examples for setting range/altitude are shown in the Maps JavaScript 3D camera examples. (developers.google.com)
If you want direct code samples or a specific platform (JavaScript, Android, iOS), tell me which and I’ll pull the exact snippet.
Citations:
- 1: https://developers.google.com/maps/documentation/maps-3d/android-sdk/configure-camera-controls?utm_source=openai
- 2: https://developers.google.com/maps/documentation/javascript/reference/3d-map?utm_source=openai
- 3: https://developers.google.com/maps/documentation/android-sdk/views?utm_source=openai
- 4: https://developers.google.com/maps/documentation/javascript/3d/interaction?utm_source=openai
🌐 Web query:
Mapbox zoom to Google Maps camera distance conversion
💡 Result:
Short recipe + formulas.
-
Meters per pixel at latitude (web‑mercator):
MPP = 156543.03392 * cos(lat * π/180) / 2^zoom.
(If you use Mapbox GL JS 512px tiles, Mapbox shows values offset by +1 zoom vs 256px tiles—adjust accordingly.) [1][2] -
Convert MPP → camera range/altitude (distance between camera and look‑at point).
Let H = map viewport height in pixels, fovy = camera vertical field‑of‑view in radians.
The visible half‑height in meters at the center = (H/2) * MPP.
For a pinhole camera model: range ≈ ((H/2) * MPP) / tan(fovy/2).So putting it together:
range ≈ ( (H/2) * 156543.03392 * cos(lat * π/180) / 2^zoom ) / tan(fovy/2).(Some SDKs call this value altitude, range or camera distance; Mapbox/math.gl and Google Maps 3D use compatible web‑mercator math and convert between fovy ↔ altitude.) [3][4]
Notes:
- You must supply latitude, viewport height (pixels) and the camera fovy (or use the SDK’s default fovy) to get an absolute distance.
- Mapbox GL’s internal “altitude ↔ zoom” uses a specific default fovy; if you want exact parity, use the SDK’s conversion helpers (e.g. WebMercatorViewport/math.gl). [3][1]
Sources: Mapbox zoom & meters/pixel docs; web‑mercator meters‑per‑pixel formula; WebMercatorViewport / fovy↔altitude docs; Google Maps camera docs. [1][2][3][4]
The zoom-to-range conversion is fundamentally inaccurate and missing critical parameters.
The formula 40000000 / Math.pow(2, zoom) is a severe oversimplification. The correct conversion requires three additional factors: latitude-dependent adjustment (cos(latitude)), viewport height in pixels, and camera field-of-view angle.
The accurate formula is:
range ≈ ((H/2) * 156543.03392 * cos(lat * π/180) / 2^zoom) / tan(fov/2)
Where:
H= viewport height in pixelslat= map latitudefov= camera vertical field of view (radians)156543.03392= correct Mercator constant (not 40000000)
The current hardcoded constant is ~255× too high, and ignoring latitude causes the range to vary incorrectly across the map. This produces incorrect camera positioning, especially at high latitudes. Use a proper SDK conversion helper (e.g., WebMercatorViewport or similar utilities) or implement the complete formula accounting for viewport dimensions and camera parameters.
| // } | ||
|
|
||
| export async function updateDrawingContext(chatId: string, drawnFeatures: any[]) { | ||
| export async function updateDrawingContext(chatId: string, contextData: { drawnFeatures: any[], cameraState: any }) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve type safety for contextData parameter.
The function signature uses any for both drawnFeatures and cameraState, which loses type safety and makes the contract unclear.
Define proper types for better type safety:
♻️ Define proper interfaces
In a shared types file (e.g., lib/types/map.ts):
export interface DrawnFeature {
id: string;
geometry: GeoJSON.Geometry;
measurement?: {
length?: number;
area?: number;
};
}
export interface CameraState {
center: { lat: number; lng: number };
zoom?: number;
pitch?: number;
bearing?: number;
}
export interface DrawingContextData {
drawnFeatures: DrawnFeature[];
cameraState: CameraState;
}Then update the function signature:
- export async function updateDrawingContext(chatId: string, contextData: { drawnFeatures: any[], cameraState: any }) {
+ export async function updateDrawingContext(chatId: string, contextData: DrawingContextData) {🤖 Prompt for AI Agents
In @lib/actions/chat.ts at line 165, Replace the loose any types in
updateDrawingContext with well-defined interfaces: create types DrawnFeature,
CameraState and DrawingContextData (e.g., in lib/types/map.ts) and update the
function signature of updateDrawingContext(chatId: string, contextData:
DrawingContextData) so drawnFeatures is DrawnFeature[] and cameraState is
CameraState; ensure any internal uses (e.g., accessing feature.id, geometry,
measurement and cameraState.center/zoom/pitch/bearing) conform to the new
interfaces and update imports to reference the new types.
| } | ||
|
|
||
| useEffect(callback, [currentDependenciesRef.current]); | ||
| useEffect(callback, [currentDependenciesRef.current, callback]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/hooks/use-deep-compare-effect.tsRepository: QueueLab/QCX
Length of output: 842
🏁 Script executed:
rg -n "useDeepCompareEffect" --type ts --type tsx -C 2Repository: QueueLab/QCX
Length of output: 83
🏁 Script executed:
rg -n "useDeepCompareEffect" -C 2Repository: QueueLab/QCX
Length of output: 1418
🏁 Script executed:
web_searchRepository: QueueLab/QCX
Length of output: 106
🏁 Script executed:
cat -n components/map/map-3d.tsx | head -70 | tail -30Repository: QueueLab/QCX
Length of output: 1097
🏁 Script executed:
rg -A 5 "useDeepCompareEffect" components/map/map-3d.tsxRepository: QueueLab/QCX
Length of output: 663
🏁 Script executed:
rg -B 2 -A 8 "useDeepCompareEffect" --type tsRepository: QueueLab/QCX
Length of output: 2574
🏁 Script executed:
web_searchRepository: QueueLab/QCX
Length of output: 106
🏁 Script executed:
cat -n components/map/map-3d.tsx | sed -n '40,80p'Repository: QueueLab/QCX
Length of output: 1455
🏁 Script executed:
rg -B 10 "useDeepCompareEffect" components/map/map-3d.tsx | grep -E "(useCallback|useDeepCompareEffect|\(\)|=>)"Repository: QueueLab/QCX
Length of output: 256
🏁 Script executed:
cat -n lib/hooks/use-callback-ref.tsRepository: QueueLab/QCX
Length of output: 658
Hook implementation defeats its optimization purpose with inline callbacks.
The deep comparison logic is negated because the effect dependency array includes callback, which changes on every render in this component. Both usages pass inline arrow functions (() => { ... }) rather than memoized callbacks, causing the effect to re-run every render regardless of whether dependencies have deeply changed.
Additionally, currentDependenciesRef.current in the dependency array doesn't behave as intended—React tracks the ref object by reference (which never changes), not its .current property value. The ref mutation is invisible to React's dependency system.
The correct implementation uses a counter signal that increments when dependencies change:
🔧 Proposed fix using signal pattern
export function useDeepCompareEffect(
callback: React.EffectCallback,
dependencies: unknown[]
) {
- const currentDependenciesRef = useRef<unknown[]>(undefined);
+ const currentDependenciesRef = useRef<unknown[]>();
+ const signalRef = useRef(0);
if (!isEqual(currentDependenciesRef.current, dependencies)) {
currentDependenciesRef.current = dependencies;
+ signalRef.current += 1;
}
- useEffect(callback, [currentDependenciesRef.current, callback]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ useEffect(callback, [signalRef.current]);
}The eslint-disable comment is necessary because the signal ensures the effect runs with the latest callback closure when dependencies change deeply, making explicit callback inclusion redundant.
| use: { | ||
| baseURL: 'http://localhost:3000', | ||
| baseURL: 'http://localhost:3003', | ||
| trace: 'on-first-retry', | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for port references across the repository
rg -n --hidden -S "3000|3003|PORT=|playwright_base_url|PLAYWRIGHT_BASE_URL|next dev|--port" . 2>/dev/null | head -50Repository: QueueLab/QCX
Length of output: 45050
🏁 Script executed:
#!/bin/bash
# Check package.json scripts
cat -n package.json 2>/dev/null | grep -E "(dev|start|build|playwright)" -A 2 -B 2Repository: QueueLab/QCX
Length of output: 968
🏁 Script executed:
#!/bin/bash
# Find and check next.config files
fd -e "next.config" -e ".env*" -e "playwright.config.ts" --type fRepository: QueueLab/QCX
Length of output: 38
Use environment-driven configuration to make Playwright baseURL portable across different dev setups.
Currently baseURL is hardcoded to 3003, which happens to work because port 3000 is occupied in this environment. However, if port 3000 becomes available, the Next.js app will start there while Playwright attempts 3003, causing test failures. Additionally, next.config.mjs lists only http://localhost:3000 in allowedOrigins, creating a mismatch.
Proposed patch
export default defineConfig({
testDir: './tests',
@@
reporter: 'html',
use: {
- baseURL: 'http://localhost:3003',
+ baseURL: process.env.PLAYWRIGHT_BASE_URL ?? 'http://localhost:3000',
trace: 'on-first-retry',
},
@@
- /* webServer: {
+ /* webServer: {
command: process.env.CI ? 'npm run build && npm run start' : 'npm run dev',
- url: 'http://localhost:3000',
+ url: process.env.PLAYWRIGHT_BASE_URL ?? 'http://localhost:3000',
reuseExistingServer: !process.env.CI,
timeout: 600000,
}, */
});Also applies to: 36-41
| test('should zoom in and out using map controls', async ({ page }) => { | ||
| const isMapbox = await page.locator('.mapboxgl-canvas').isVisible(); | ||
| if (!isMapbox) { | ||
| test.skip(true, 'Zoom controls test is only for Mapbox'); | ||
| return; | ||
| } | ||
|
|
||
| const hasMap = await page.evaluate(() => Boolean((window as any).map)); | ||
| if (!hasMap) test.skip(true, 'Map instance not available on window for E2E'); | ||
|
|
||
| const getZoom = () => page.evaluate(() => (window as any).map.getZoom()); | ||
|
|
||
| const initialZoom = await getZoom(); | ||
|
|
||
| await page.click('.mapboxgl-ctrl-zoom-in'); | ||
| await page.waitForFunction(() => (window as any).map.getZoom() > initialZoom); | ||
| await page.evaluate(() => new Promise(res => (window as any).map.once('zoomend', res))); | ||
|
|
||
| const zoomedInZoom = await getZoom(); | ||
| expect(zoomedInZoom).toBeGreaterThan(initialZoom); | ||
|
|
||
| await page.click('.mapboxgl-ctrl-zoom-out'); | ||
| await page.waitForFunction(() => (window as any).map.getZoom() < zoomedInZoom); | ||
| await page.evaluate(() => new Promise(res => (window as any).map.once('zoomend', res))); | ||
|
|
||
| const zoomedOutZoom = await getZoom(); | ||
| expect(zoomedOutZoom).toBeLessThan(zoomedInZoom); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Excellent improvement with event-driven synchronization.
Replacing waitForFunction polling with map.once('zoomend') event listeners is more reliable and efficient. The runtime provider check ensures the test only runs for Mapbox.
Consider extracting the provider check into a helper function to reduce duplication:
♻️ Suggested refactor to reduce duplication
At the top of the test file, add:
async function skipIfNotMapbox(page: Page) {
const isMapbox = await page.locator('.mapboxgl-canvas').isVisible();
if (!isMapbox) {
test.skip(true, 'This test is only for Mapbox');
return false;
}
return true;
}Then in each test:
test('should toggle the map mode', async ({ page }) => {
- const isMapbox = await page.locator('.mapboxgl-canvas').isVisible();
- if (!isMapbox) {
- test.skip(true, 'Drawing mode test is only for Mapbox');
- return;
- }
+ if (!await skipIfNotMapbox(page)) return;
await page.click('[data-testid="map-toggle"]');
// ...
});🤖 Prompt for AI Agents
In @tests/map.spec.ts around lines 25 - 50, Extract the Mapbox runtime/provider
check into a reusable helper (e.g., skipIfNotMapbox) and call it from tests
instead of duplicating the locator + test.skip logic: move the logic that awaits
page.locator('.mapboxgl-canvas').isVisible() and calls test.skip into a
top-level async function (suggested name skipIfNotMapbox) that returns a
boolean, and update the test('should zoom in and out using map controls', ...)
to call await skipIfNotMapbox(page) and early return if it returns false; ensure
the helper preserves the existing behavior (calling test.skip(true, ...) when
not Mapbox) and also keep the existing map presence check (Boolean((window as
any).map)) either inside the helper or as a separate small helper used in the
test.
This commit fixes a bug where the map's camera context was lost when switching from the Google Maps provider back to the Mapbox provider. The `mapbox-map.tsx` component has been updated to read the `cameraState` from the `MapDataContext` upon initialization. This ensures that the map's view (center, zoom, pitch, and bearing) is preserved when switching back to Mapbox, providing a seamless user experience. The logic to convert Google Maps' `range` to Mapbox's `zoom` has also been included.
ngoiyaeric
force-pushed
the
feat/map-state-synchronization-689470674920205544
branch
from
9c9997a to
07edb21
Compare
ngoiyaeric
force-pushed
the
feat/map-state-synchronization-689470674920205544
branch
from
3d3ac53 to
07edb21
Compare
User description
This commit introduces a comprehensive solution for synchronizing map state, including drawn features and camera position, between the Mapbox and Google Maps providers, as well as adding screenshot functionality for Google Maps and persisting the combined state.
PR created automatically by Jules for task 689470674920205544 started by @ngoiyaeric
PR Type
Enhancement
Description
Synchronize map state (camera position) between Mapbox and Google Maps
Add GeoJSON rendering layer for Google Maps to display drawn features
Implement screenshot functionality for Google Maps using Static API
Persist both drawn features and camera state to database
Make Playwright tests provider-aware for both map implementations
Fix deep compare effect hook to include callback dependency
Diagram Walkthrough
File Walkthrough
8 files
Update server action to persist camera statePass camera state with drawn featuresAdd Google Maps screenshot via Static APINew component to render GeoJSON on Google MapsIntegrate GeoJSON layer and camera state syncCapture and sync Google Maps camera eventsAdd CameraState interface to contextCapture and sync Mapbox camera state1 files
Add callback to dependency array1 files
Update test server port configuration1 files
Make tests provider-aware for both map typesSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.