-
-
Notifications
You must be signed in to change notification settings - Fork 6
Synchronize Map State Between Mapbox and Google Maps #409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
326418d
07edb21
6572b8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ import { AI } from '@/app/actions' | |
| import { nanoid } from 'nanoid' | ||
| import { UserMessage } from './user-message' | ||
| import { toast } from 'react-toastify' | ||
| import { useSettingsStore } from '@/lib/store/settings' | ||
| import { useMapData } from './map/map-data-context' | ||
|
|
||
| // Define an interface for the actions to help TypeScript during build | ||
| interface HeaderActions { | ||
|
|
@@ -18,6 +20,8 @@ interface HeaderActions { | |
|
|
||
| export function HeaderSearchButton() { | ||
| const { map } = useMap() | ||
| const { mapProvider } = useSettingsStore() | ||
| const { mapData } = useMapData() | ||
| // Cast the actions to our defined interface to avoid build errors | ||
|
Comment on lines
21
to
25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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:
💡 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 References/sources:
🏁 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 Required fixes:
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 |
||
| const actions = useActions<typeof AI>() as unknown as HeaderActions | ||
| const [, setMessages] = useUIState<typeof AI>() | ||
|
|
@@ -32,12 +36,11 @@ export function HeaderSearchButton() { | |
| }, []) | ||
|
|
||
| const handleResolutionSearch = async () => { | ||
| if (!map) { | ||
| if (mapProvider === 'mapbox' && !map) { | ||
| toast.error('Map is not available yet. Please wait for it to load.') | ||
| return | ||
| } | ||
| if (!actions) { | ||
| // This should theoretically not happen if the component is used correctly | ||
| toast.error('Search actions are not available.') | ||
| return | ||
| } | ||
|
|
@@ -53,10 +56,31 @@ export function HeaderSearchButton() { | |
| } | ||
| ]) | ||
|
|
||
| const canvas = map.getCanvas() | ||
| const blob = await new Promise<Blob | null>(resolve => { | ||
| canvas.toBlob(resolve, 'image/png') | ||
| }) | ||
| 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(); | ||
| } | ||
|
Comment on lines
+59
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Google Static Maps URL construction has multiple functional issues:
This will make screenshot capture flaky/incorrect in production. SuggestionIntroduce a small, tested helper to map 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 |
||
|
|
||
| if (!blob) { | ||
| throw new Error('Failed to capture map image.') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| 'use client'; | ||
|
|
||
| import { useEffect, useRef } from 'react'; | ||
| import { useMap } from '@vis.gl/react-google-maps'; | ||
|
|
||
| interface GoogleGeoJsonLayerProps { | ||
| data: GeoJSON.FeatureCollection; | ||
| } | ||
|
|
||
| export function GoogleGeoJsonLayer({ data }: GoogleGeoJsonLayerProps) { | ||
| const map = useMap(); | ||
| const layerRef = useRef<google.maps.Data | null>(null); | ||
|
|
||
| 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 {}; | ||
| }); | ||
|
Comment on lines
+27
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add support for Point geometries and make colors configurable. The styling currently only handles
♻️ 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 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 |
||
|
|
||
| // 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]); | ||
|
Comment on lines
+14
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The You can keep a single SuggestionCreate the 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.
Comment on lines
+14
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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:
♻️ 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 |
||
|
|
||
| return null; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,13 @@ | ||
| 'use client' | ||
|
|
||
| import { APIProvider } from '@vis.gl/react-google-maps' | ||
| import { useEffect } from 'react' | ||
| import { useEffect, useMemo } from 'react' | ||
| import { useToast } from '@/components/ui/hooks/use-toast' | ||
| import { useMapData } from './map-data-context' | ||
| import { useSettingsStore } from '@/lib/store/settings' | ||
| import { useMapLoading } from '../map-loading-context'; | ||
| import { Map3D } from './map-3d' | ||
| import { GoogleGeoJsonLayer } from './google-geojson-layer' | ||
|
|
||
| export function GoogleMapComponent() { | ||
| const { toast } = useToast() | ||
|
|
@@ -34,21 +35,52 @@ export function GoogleMapComponent() { | |
| }; | ||
| }, [setIsMapLoaded]); | ||
|
|
||
| const featureCollection = useMemo(() => { | ||
| const features = mapData.drawnFeatures?.map(df => ({ | ||
| type: 'Feature' as const, | ||
| geometry: df.geometry, | ||
| properties: { | ||
| id: df.id, | ||
| measurement: df.measurement | ||
| } | ||
| })) || []; | ||
|
|
||
| return { | ||
| type: 'FeatureCollection' as const, | ||
| features, | ||
| }; | ||
| }, [mapData.drawnFeatures]); | ||
|
|
||
| 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]); | ||
|
Comment on lines
+54
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. SuggestionIntroduce a normalized camera state schema (e.g., always store Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing
Comment on lines
+54
to
+70
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Short answer — two steps:
Combine (substituting resolution): Notes/sources:
References: 🌐 Web query:
💡 Result: Quick references from Google's docs:
If you want direct code samples or a specific platform (JavaScript, Android, iOS), tell me which and I’ll pull the exact snippet. Citations:
🌐 Web query:
💡 Result: Short recipe + formulas.
Notes:
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 The accurate formula is: Where:
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., |
||
|
|
||
| if (!apiKey) { | ||
| return null | ||
| } | ||
|
|
||
| const cameraOptions = mapData.targetPosition | ||
| ? { center: mapData.targetPosition, range: 1000, tilt: 60, heading: 0 } | ||
| : { center: { lat: 37.7749, lng: -122.4194 }, range: 1000, tilt: 60, heading: 0 }; | ||
|
|
||
| return ( | ||
| <APIProvider apiKey={apiKey} version="alpha"> | ||
| <Map3D | ||
| style={{ width: '100%', height: '100%' }} | ||
| cameraOptions={cameraOptions} | ||
| mode="SATELLITE" | ||
| /> | ||
| <GoogleGeoJsonLayer data={featureCollection} /> | ||
| </APIProvider> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,18 +12,33 @@ import {useCallbackRef} from '@/lib/hooks/use-callback-ref'; | |
| import {useMap3DCameraEvents} from '@/lib/hooks/use-map-3d-camera-events'; | ||
| import {useDeepCompareEffect} from '@/lib/hooks/use-deep-compare-effect'; | ||
| import type {Map3DProps} from './map-3d-types'; | ||
| import { useMapData } from './map-data-context'; | ||
|
|
||
| export const Map3D = forwardRef( | ||
| ( | ||
| props: Map3DProps, | ||
| forwardedRef: ForwardedRef<google.maps.maps3d.Map3DElement | null> | ||
| ) => { | ||
| useMapsLibrary('maps3d'); | ||
| const { setMapData } = useMapData(); | ||
|
|
||
| const [map3DElement, map3dRef] = | ||
| 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 | ||
| } | ||
| })); | ||
|
Comment on lines
26
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, the update currently spreads SuggestionNormalize 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. |
||
| if (!props.onCameraChange) return; | ||
|
|
||
| props.onCameraChange(p); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +0,0 @@ | ||
| $ next dev --turbo | ||
| ▲ Next.js 15.3.6 (Turbopack) | ||
| - Local: http://localhost:3000 | ||
| - Network: http://192.168.0.2:3000 | ||
| - Environments: .env | ||
|
|
||
| ✓ Starting... | ||
| Attention: Next.js now collects completely anonymous telemetry regarding usage. | ||
| This information is used to shape Next.js' roadmap and prioritize features. | ||
| You can learn more, including how to opt-out if you'd not like to participate in this anonymous program, by visiting the following URL: | ||
| https://nextjs.org/telemetry | ||
|
|
||
| ✓ Compiled middleware in 388ms | ||
| ✓ Ready in 1859ms | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,7 +162,7 @@ export async function saveChat(chat: OldChatType, userId: string): Promise<strin | |
| // return null; | ||
| // } | ||
|
|
||
| export async function updateDrawingContext(chatId: string, drawnFeatures: any[]) { | ||
| export async function updateDrawingContext(chatId: string, contextData: { drawnFeatures: any[], cameraState: any }) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Improve type safety for contextData parameter. The function signature uses Define proper types for better type safety: ♻️ Define proper interfacesIn a shared types file (e.g., 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 |
||
| 'use server'; | ||
| console.log('[Action] updateDrawingContext called for chatId:', chatId); | ||
|
|
||
|
|
@@ -178,7 +178,7 @@ export async function updateDrawingContext(chatId: string, drawnFeatures: any[]) | |
| // id: `drawnData-${Date.now().toString()}`, // Let DB generate UUID | ||
| userId: userId, | ||
| role: 'data' as 'user' | 'assistant' | 'system' | 'tool' | 'data', // Cast 'data' if not in standard roles | ||
| content: JSON.stringify(drawnFeatures), // Store features as stringified JSON | ||
| content: JSON.stringify(contextData), // Store both features and camera state as stringified JSON | ||
| // type: 'drawing_context', // This field is not in the Drizzle 'messages' schema. | ||
| // If `type` is important, the schema needs to be updated or content needs to reflect it. | ||
| // For now, we'll assume 'content' holds the necessary info and role='data' signifies it. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,5 +18,5 @@ export function useDeepCompareEffect( | |
| currentDependenciesRef.current = dependencies; | ||
| } | ||
|
|
||
| useEffect(callback, [currentDependenciesRef.current]); | ||
| useEffect(callback, [currentDependenciesRef.current, callback]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 Additionally, 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 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ export default defineConfig({ | |
| workers: process.env.CI ? 1 : undefined, | ||
| reporter: 'html', | ||
| use: { | ||
| baseURL: 'http://localhost:3000', | ||
| baseURL: 'http://localhost:3003', | ||
| trace: 'on-first-retry', | ||
| }, | ||
|
Comment on lines
9
to
13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing Playwright This should be driven by an env var with a sensible default. SuggestionUse 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.
Comment on lines
10
to
13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 Currently 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 |
||
| projects: [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateDrawingContextis fired on any change todrawnFeatures/cameraState, but the guard is too permissive:drawnFeaturescould be[]and you’ll still create a newdatamessage (potentially on every camera move oncecameraStateexists). This can spam the DB/chat history and make later retrieval ambiguous.This is a correctness/behavior issue: the diff moved from
length > 0to 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:
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.