Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
DATABASE_URL="postgresql://user:password@host:port/db"
NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN=pk.eyJ1IjoiZHVtbXkiLCJhIjoiY2p1bW15In0.ZHVtbXk=
Comment on lines 1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Remove committed Mapbox token from tracked .env.

Even public Mapbox tokens can be abused and tied to account quotas; committed .env files are a common leak vector. Keep the real token in deployment secrets and use a placeholder (or move to .env.example) in the repo.

🛠️ Suggested placeholder
-NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN=pk.eyJ1IjoiZHVtbXkiLCJhIjoiY2p1bW15In0.ZHVtbXk=
+NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN=YOUR_MAPBOX_PUBLIC_TOKEN
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
DATABASE_URL="postgresql://user:password@host:port/db"
NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN=pk.eyJ1IjoiZHVtbXkiLCJhIjoiY2p1bW15In0.ZHVtbXk=
DATABASE_URL="postgresql://user:password@host:port/db"
NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN=YOUR_MAPBOX_PUBLIC_TOKEN
🧰 Tools
🪛 dotenv-linter (4.0.0)

[warning] 1-1: [QuoteCharacter] The value has quote characters (', ")

(QuoteCharacter)

🤖 Prompt for AI Agents
In @.env around lines 1 - 2, Remove the committed Mapbox token by replacing the
real value for NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN with a placeholder (e.g.
NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN=YOUR_MAPBOX_TOKEN_HERE) or move the current .env
contents into .env.example, add .env to .gitignore so secrets aren’t tracked,
and update deployment/config to load the real token from environment/secret
storage; reference the NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN variable in the repo to
locate and update all usages and rotate the exposed token in your Mapbox
account.

13 changes: 12 additions & 1 deletion app/search/[id]/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ export default async function SearchPage({ params }: SearchPageProps) {
};
});

// Extract initial map data from the latest 'data' message
const latestDataMessage = [...dbMessages].reverse().find(m => m.role === 'data');
let initialMapData = undefined;
if (latestDataMessage) {
try {
initialMapData = JSON.parse(latestDataMessage.content);
} catch (e) {
console.error('Failed to parse initial map data:', e);
}
}
Comment on lines +62 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validate parsed map data structure before use.

The parsed JSON is passed directly to MapDataProvider without validating that it conforms to the expected MapData shape ({ drawnFeatures, markers, imageOverlays }). Malformed data in the database could cause runtime errors in child components that expect these arrays.

Consider adding schema validation or at minimum structural checks:

🛡️ Proposed fix to add basic validation
   // Extract initial map data from the latest 'data' message
   const latestDataMessage = [...dbMessages].reverse().find(m => m.role === 'data');
   let initialMapData = undefined;
   if (latestDataMessage) {
     try {
-      initialMapData = JSON.parse(latestDataMessage.content);
+      const parsed = JSON.parse(latestDataMessage.content);
+      // Basic validation: ensure expected arrays exist
+      if (
+        parsed &&
+        typeof parsed === 'object' &&
+        Array.isArray(parsed.drawnFeatures) &&
+        Array.isArray(parsed.imageOverlays)
+      ) {
+        initialMapData = parsed;
+      } else {
+        console.warn('Parsed map data has unexpected shape, ignoring');
+      }
     } catch (e) {
       console.error('Failed to parse initial map data:', e);
     }
   }
🤖 Prompt for AI Agents
In `@app/search/`[id]/page.tsx around lines 62 - 71, The parsed JSON assigned to
initialMapData (from latestDataMessage) must be validated to ensure it matches
the expected MapData shape before passing to MapDataProvider; update the parsing
block (latestDataMessage / initialMapData) to: try parsing, then check that the
result is an object and that drawnFeatures, markers, and imageOverlays exist and
are arrays (or coerce to empty arrays as a safe default); on validation failure,
log a clear error and set initialMapData to a safe fallback object ({
drawnFeatures: [], markers: [], imageOverlays: [] }) so MapDataProvider and
downstream components never receive malformed data.


return (
<AI
initialAIState={{
Expand All @@ -68,7 +79,7 @@ export default async function SearchPage({ params }: SearchPageProps) {
// If needed for styling or other logic, it can be set.
}}
>
<MapDataProvider>
<MapDataProvider initialData={initialMapData}>
<Chat id={id} />
</MapDataProvider>
</AI>
Expand Down
29 changes: 19 additions & 10 deletions components/chat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { MapDataProvider, useMapData } from './map/map-data-context'; // Add thi
import { updateDrawingContext } from '@/lib/actions/chat'; // Import the server action
import dynamic from 'next/dynamic'
import { HeaderSearchButton } from './header-search-button'
import { debounce } from 'lodash'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider importing from lodash/debounce for smaller bundles.

While modern bundlers tree-shake { debounce } from 'lodash', importing directly from lodash/debounce guarantees only the debounce module is included.

🔧 Suggested change
-import { debounce } from 'lodash'
+import debounce from 'lodash/debounce'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { debounce } from 'lodash'
import debounce from 'lodash/debounce'
🤖 Prompt for AI Agents
In `@components/chat.tsx` at line 22, Replace the current named import of debounce
from the full lodash package with a direct import of the debounce module to
reduce bundle size: change the import that brings in "debounce" (currently
imported as { debounce } from 'lodash') to a default import from
'lodash/debounce' (i.e., import debounce from 'lodash/debounce') and update any
usages of the debounce identifier accordingly in components/chat.tsx.


type ChatProps = {
id?: string // This is the chatId
Expand Down Expand Up @@ -89,21 +90,29 @@ export function Chat({ id }: ChatProps) {
}
}, [isSubmitting])

// useEffect to call the server action when drawnFeatures changes
// Debounced version of updateDrawingContext
const debouncedUpdateDrawingContext = useRef(
debounce((chatId: string, contextData: any) => {
console.log('Chat.tsx: calling debounced updateDrawingContext');
updateDrawingContext(chatId, contextData);
}, 2000)
).current;
Comment on lines +93 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Cancel debounced function on unmount to prevent stale writes.

The debounced function may fire after the component unmounts, potentially causing errors or writing to a stale chat. Add cleanup to cancel pending calls.

🛡️ Proposed fix
   const debouncedUpdateDrawingContext = useRef(
     debounce((chatId: string, contextData: any) => {
       console.log('Chat.tsx: calling debounced updateDrawingContext');
       updateDrawingContext(chatId, contextData);
     }, 2000)
   ).current;

+  useEffect(() => {
+    return () => {
+      debouncedUpdateDrawingContext.cancel();
+    };
+  }, [debouncedUpdateDrawingContext]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Debounced version of updateDrawingContext
const debouncedUpdateDrawingContext = useRef(
debounce((chatId: string, contextData: any) => {
console.log('Chat.tsx: calling debounced updateDrawingContext');
updateDrawingContext(chatId, contextData);
}, 2000)
).current;
// Debounced version of updateDrawingContext
const debouncedUpdateDrawingContext = useRef(
debounce((chatId: string, contextData: any) => {
console.log('Chat.tsx: calling debounced updateDrawingContext');
updateDrawingContext(chatId, contextData);
}, 2000)
).current;
useEffect(() => {
return () => {
debouncedUpdateDrawingContext.cancel();
};
}, [debouncedUpdateDrawingContext]);
🤖 Prompt for AI Agents
In `@components/chat.tsx` around lines 93 - 99, The debouncedUpdateDrawingContext
created via useRef(debounce(...)) can fire after the component unmounts; add a
cleanup useEffect that calls debouncedUpdateDrawingContext.cancel() on unmount
to prevent stale writes and errors, referencing the existing
debouncedUpdateDrawingContext and updateDrawingContext symbols (ensure the
debounce implementation provides a cancel method or wrap it so cancel is
available).


// useEffect to call the server action when map data changes
useEffect(() => {
if (id && mapData.drawnFeatures && mapData.cameraState) {
console.log('Chat.tsx: drawnFeatures changed, calling updateDrawingContext', mapData.drawnFeatures);
updateDrawingContext(id, {
drawnFeatures: mapData.drawnFeatures,
if (id && (mapData.drawnFeatures || mapData.imageOverlays) && mapData.cameraState) {
debouncedUpdateDrawingContext(id, {
drawnFeatures: mapData.drawnFeatures || [],
imageOverlays: mapData.imageOverlays || [],
cameraState: mapData.cameraState,
});
}
}, [id, mapData.drawnFeatures, mapData.cameraState]);
}, [id, mapData.drawnFeatures, mapData.imageOverlays, mapData.cameraState, debouncedUpdateDrawingContext]);
Comment on lines 102 to +110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Empty arrays trigger persistence unnecessarily.

The condition (mapData.drawnFeatures || mapData.imageOverlays) is truthy for empty arrays []. Consider checking array length to avoid writing empty state repeatedly.

🔧 Suggested improvement
   useEffect(() => {
-    if (id && (mapData.drawnFeatures || mapData.imageOverlays) && mapData.cameraState) {
+    const hasFeatures = mapData.drawnFeatures?.length > 0;
+    const hasOverlays = mapData.imageOverlays?.length > 0;
+    if (id && (hasFeatures || hasOverlays) && mapData.cameraState) {
       debouncedUpdateDrawingContext(id, {
         drawnFeatures: mapData.drawnFeatures || [],
         imageOverlays: mapData.imageOverlays || [],
         cameraState: mapData.cameraState,
       });
     }
   }, [id, mapData.drawnFeatures, mapData.imageOverlays, mapData.cameraState, debouncedUpdateDrawingContext]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (id && mapData.drawnFeatures && mapData.cameraState) {
console.log('Chat.tsx: drawnFeatures changed, calling updateDrawingContext', mapData.drawnFeatures);
updateDrawingContext(id, {
drawnFeatures: mapData.drawnFeatures,
if (id && (mapData.drawnFeatures || mapData.imageOverlays) && mapData.cameraState) {
debouncedUpdateDrawingContext(id, {
drawnFeatures: mapData.drawnFeatures || [],
imageOverlays: mapData.imageOverlays || [],
cameraState: mapData.cameraState,
});
}
}, [id, mapData.drawnFeatures, mapData.cameraState]);
}, [id, mapData.drawnFeatures, mapData.imageOverlays, mapData.cameraState, debouncedUpdateDrawingContext]);
useEffect(() => {
const hasFeatures = mapData.drawnFeatures?.length > 0;
const hasOverlays = mapData.imageOverlays?.length > 0;
if (id && (hasFeatures || hasOverlays) && mapData.cameraState) {
debouncedUpdateDrawingContext(id, {
drawnFeatures: mapData.drawnFeatures || [],
imageOverlays: mapData.imageOverlays || [],
cameraState: mapData.cameraState,
});
}
}, [id, mapData.drawnFeatures, mapData.imageOverlays, mapData.cameraState, debouncedUpdateDrawingContext]);
🤖 Prompt for AI Agents
In `@components/chat.tsx` around lines 102 - 110, The current useEffect calls
debouncedUpdateDrawingContext even when drawnFeatures or imageOverlays are empty
arrays because `(mapData.drawnFeatures || mapData.imageOverlays)` is truthy for
[] — update the condition in the useEffect (the block that references id,
mapData.drawnFeatures, mapData.imageOverlays, mapData.cameraState, and
debouncedUpdateDrawingContext) to check array lengths explicitly (e.g. only
proceed if (mapData.drawnFeatures?.length ?? 0) > 0 or
(mapData.imageOverlays?.length ?? 0) > 0) along with a valid cameraState and id
so empty arrays won’t trigger unnecessary persistence.


// Mobile layout
if (isMobile) {
return (
<MapDataProvider> {/* Add Provider */}
<>
<HeaderSearchButton />
<div className="mobile-layout-container">
<div className="mobile-map-section">
Expand Down Expand Up @@ -157,13 +166,13 @@ export function Chat({ id }: ChatProps) {
)}
</div>
</div>
</MapDataProvider>
</>
);
}

// Desktop layout
return (
<MapDataProvider> {/* Add Provider */}
<>
<HeaderSearchButton />
<div className="flex justify-start items-start">
{/* This is the new div for scrolling */}
Expand Down Expand Up @@ -221,6 +230,6 @@ export function Chat({ id }: ChatProps) {
{activeView ? <SettingsView /> : <MapProvider />}
</div>
</div>
</MapDataProvider>
</>
);
}
167 changes: 167 additions & 0 deletions components/map/image-overlay-layer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
'use client'

import { useEffect, useRef, useCallback } from 'react'
import mapboxgl from 'mapbox-gl'
import { useMap } from './map-context'
import { useMapData, ImageOverlay } from './map-data-context'
import { useMapToggle, MapToggleEnum } from '../map-toggle-context'
import { X } from 'lucide-react'
import { Button } from '../ui/button'
import { createRoot } from 'react-dom/client'

interface ImageOverlayLayerProps {
overlay: ImageOverlay;
}

export function ImageOverlayLayer({ overlay }: ImageOverlayLayerProps) {
const { map } = useMap()
const { setMapData } = useMapData()
const { mapType } = useMapToggle()
const isDrawingMode = mapType === MapToggleEnum.DrawingMode
const markersRef = useRef<mapboxgl.Marker[]>([])
const deleteMarkerRef = useRef<mapboxgl.Marker | null>(null)
const deleteRootRef = useRef<any>(null)

const sourceId = `image-source-${overlay.id}`
const layerId = `image-layer-${overlay.id}`

// Update overlay coordinates in global state
const updateCoordinates = useCallback((index: number, lngLat: [number, number]) => {
setMapData(prev => {
const overlays = prev.imageOverlays || []
const newOverlays = overlays.map(o => {
if (o.id === overlay.id) {
const newCoords = [...o.coordinates] as [[number, number], [number, number], [number, number], [number, number]]
newCoords[index] = lngLat
return { ...o, coordinates: newCoords }
}
return o
})
return { ...prev, imageOverlays: newOverlays }
})
}, [overlay.id, setMapData])

const removeOverlay = useCallback(() => {
setMapData(prev => ({
...prev,
imageOverlays: (prev.imageOverlays || []).filter(o => o.id !== overlay.id)
}))
}, [overlay.id, setMapData])

useEffect(() => {
if (!map) return

const onMapLoad = () => {
if (!map.getSource(sourceId)) {
map.addSource(sourceId, {
type: 'image',
url: overlay.url,
coordinates: overlay.coordinates
})

map.addLayer({
id: layerId,
type: 'raster',
source: sourceId,
paint: {
'raster-opacity': overlay.opacity || 0.7
}
})
Comment on lines +62 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

find . -name "image-overlay-layer.tsx" -type f

Repository: QueueLab/QCX

Length of output: 96


🏁 Script executed:

wc -l ./components/map/image-overlay-layer.tsx

Repository: QueueLab/QCX

Length of output: 100


🏁 Script executed:

sed -n '55,75p' ./components/map/image-overlay-layer.tsx

Repository: QueueLab/QCX

Length of output: 507


🏁 Script executed:

sed -n '90,105p' ./components/map/image-overlay-layer.tsx

Repository: QueueLab/QCX

Length of output: 446


🏁 Script executed:

grep -n 'overlay.opacity' ./components/map/image-overlay-layer.tsx

Repository: QueueLab/QCX

Length of output: 234


🏁 Script executed:

cat -n ./components/map/image-overlay-layer.tsx | sed -n '1,30p'

Repository: QueueLab/QCX

Length of output: 1366


🏁 Script executed:

grep -A 5 'interface ImageOverlay' ./components/map/map-data-context.tsx

Repository: QueueLab/QCX

Length of output: 246


Allow explicit opacity = 0.

overlay.opacity || 0.7 treats 0 as falsy and forces 0.7, preventing fully transparent overlays. Use ?? instead.

Suggested fix
-            'raster-opacity': overlay.opacity || 0.7
+            'raster-opacity': overlay.opacity ?? 0.7
-      map.setPaintProperty(layerId, 'raster-opacity', overlay.opacity || 0.7)
+      map.setPaintProperty(layerId, 'raster-opacity', overlay.opacity ?? 0.7)

Applies to lines 67 and 98.

🤖 Prompt for AI Agents
In `@components/map/image-overlay-layer.tsx` around lines 62 - 69, The raster
layer's opacity uses a truthy fallback (overlay.opacity || 0.7) which treats 0
as false and prevents fully transparent overlays; update both uses (the paint
'raster-opacity' in the map.addLayer call where layerId is added and the other
occurrence around line 98) to use the nullish coalescing operator
(overlay.opacity ?? 0.7) so that explicit 0 is honored while still defaulting to
0.7 when opacity is undefined or null.

}
}

if (map.isStyleLoaded()) {
onMapLoad()
} else {
map.on('load', onMapLoad)
}

return () => {
if (map.getLayer(layerId)) map.removeLayer(layerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
}, [map, sourceId, layerId, overlay.url]) // coordinates handled separately
Comment on lines +51 to +83

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect that adds the Mapbox source/layer subscribes to map.on('load', onMapLoad) when style isn’t loaded, but it never unregisters the listener. If the component unmounts before load fires, the handler can still run later and attempt to add sources/layers for an overlay that no longer exists.

You should call map.off('load', onMapLoad) in cleanup when you register it.

Suggestion

Track whether you registered the load handler and unregister it in cleanup.

Example:

let subscribed = false
if (!map.isStyleLoaded()) {
  map.on('load', onMapLoad)
  subscribed = true
} else {
  onMapLoad()
}

return () => {
  if (subscribed) map.off('load', onMapLoad)
  if (map.getLayer(layerId)) map.removeLayer(layerId)
  if (map.getSource(sourceId)) map.removeSource(sourceId)
}

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

Comment on lines +51 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

fd "image-overlay-layer.tsx" --type f

Repository: QueueLab/QCX

Length of output: 94


🏁 Script executed:

cat -n components/map/image-overlay-layer.tsx

Repository: QueueLab/QCX

Length of output: 6094


Fix Mapbox and React resource leaks in overlay lifecycle.

Three issues cause resource leaks when overlays are added/removed:

  1. The map.on('load', onMapLoad) listener (line 76) isn't detached in cleanup, leaving orphaned listeners if the component unmounts before the map style loads.
  2. The React root for the delete button (line 137–138) is never unmounted in cleanup (line 160–163), leaking the React tree.
  3. Opacity uses || instead of ?? (lines 67, 98), so setting opacity to 0 incorrectly defaults to 0.7 instead.

Additionally, consider wrapping forEach callbacks in explicit blocks for clarity (lines 107, 161).

Suggested fixes

Line 79–82: Add listener cleanup

     return () => {
+      map.off('load', onMapLoad)
       if (map.getLayer(layerId)) map.removeLayer(layerId)
       if (map.getSource(sourceId)) map.removeSource(sourceId)
     }

Lines 160–163: Unmount React root

     return () => {
       markersRef.current.forEach(m => m.remove())
       if (deleteMarkerRef.current) deleteMarkerRef.current.remove()
+      if (deleteRootRef.current) {
+        deleteRootRef.current.unmount()
+        deleteRootRef.current = null
+      }
     }

Lines 67, 98: Use nullish coalescing

-            'raster-opacity': overlay.opacity || 0.7
+            'raster-opacity': overlay.opacity ?? 0.7

Lines 107, 161: Wrap forEach callbacks explicitly

-    markersRef.current.forEach(m => m.remove())
+    markersRef.current.forEach(m => { m.remove() })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (!map) return
const onMapLoad = () => {
if (!map.getSource(sourceId)) {
map.addSource(sourceId, {
type: 'image',
url: overlay.url,
coordinates: overlay.coordinates
})
map.addLayer({
id: layerId,
type: 'raster',
source: sourceId,
paint: {
'raster-opacity': overlay.opacity || 0.7
}
})
}
}
if (map.isStyleLoaded()) {
onMapLoad()
} else {
map.on('load', onMapLoad)
}
return () => {
if (map.getLayer(layerId)) map.removeLayer(layerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
}, [map, sourceId, layerId, overlay.url]) // coordinates handled separately
useEffect(() => {
if (!map) return
const onMapLoad = () => {
if (!map.getSource(sourceId)) {
map.addSource(sourceId, {
type: 'image',
url: overlay.url,
coordinates: overlay.coordinates
})
map.addLayer({
id: layerId,
type: 'raster',
source: sourceId,
paint: {
'raster-opacity': overlay.opacity ?? 0.7
}
})
}
}
if (map.isStyleLoaded()) {
onMapLoad()
} else {
map.on('load', onMapLoad)
}
return () => {
map.off('load', onMapLoad)
if (map.getLayer(layerId)) map.removeLayer(layerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
}, [map, sourceId, layerId, overlay.url]) // coordinates handled separately
🤖 Prompt for AI Agents
In `@components/map/image-overlay-layer.tsx` around lines 51 - 83, The useEffect's
onMapLoad listener isn't removed on cleanup, the image layer opacity uses ||
causing 0 to default to 0.7, and the React root created for the delete button is
never unmounted; fix by (1) storing onMapLoad and, in the cleanup return, call
map.off('load', onMapLoad) if you previously attached it (or always call map.off
to be safe) alongside removing layer/source in useEffect, (2) change any
'raster-opacity': overlay.opacity || 0.7 (and the other occurrence) to use
nullish coalescing 'raster-opacity': overlay.opacity ?? 0.7, and (3) keep the
createRoot instance (e.g., deleteButtonRoot or similar variable used where you
call createRoot) in scope and call its unmount() in the same cleanup; also wrap
forEach callbacks in explicit { } blocks for clarity.


// Sync coordinates with map source
useEffect(() => {
if (!map) return
const source = map.getSource(sourceId) as mapboxgl.ImageSource
if (source) {
source.setCoordinates(overlay.coordinates)
}
}, [map, overlay.coordinates, sourceId])

// Sync opacity
useEffect(() => {
if (!map) return
if (map.getLayer(layerId)) {
map.setPaintProperty(layerId, 'raster-opacity', overlay.opacity || 0.7)
}
}, [map, overlay.opacity, layerId])

// Draggable markers for corners
useEffect(() => {
if (!map) return

// Clean up existing markers
markersRef.current.forEach(m => m.remove())
markersRef.current = []
if (deleteMarkerRef.current) {
deleteMarkerRef.current.remove()
deleteMarkerRef.current = null
}

if (isDrawingMode) {
// Add 4 corner markers
overlay.coordinates.forEach((coord, index) => {
const el = document.createElement('div')
el.className = 'w-4 h-4 bg-white border-2 border-primary rounded-full cursor-move shadow-md'

const marker = new mapboxgl.Marker({
element: el,
draggable: true
})
.setLngLat(coord)
.addTo(map)

marker.on('drag', () => {
const newLngLat = marker.getLngLat()
updateCoordinates(index, [newLngLat.lng, newLngLat.lat])
})

markersRef.current.push(marker)
})

// Add delete button near top-right corner
const deleteEl = document.createElement('div')
const root = createRoot(deleteEl)
deleteRootRef.current = root
root.render(
<Button
variant="destructive"
size="icon"
className="w-6 h-6 rounded-full shadow-lg"
onClick={removeOverlay}
>
<X size={14} />
</Button>
)

const deleteMarker = new mapboxgl.Marker({
element: deleteEl,
offset: [0, -20]
})
.setLngLat(overlay.coordinates[1]) // Near Top-right
.addTo(map)

deleteMarkerRef.current = deleteMarker
}

return () => {
markersRef.current.forEach(m => m.remove())
if (deleteMarkerRef.current) deleteMarkerRef.current.remove()
}
Comment on lines +23 to +163

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteRootRef is typed as any and never unmounted. Because you create a React root inside a Mapbox marker element, you should call root.unmount() during cleanup; otherwise you risk leaking React trees and event handlers when toggling drawing mode, changing overlay coordinates, or unmounting the layer.

Also, you remove the marker element in cleanup but don’t clear deleteRootRef.current, which makes repeated toggles harder to reason about.

Suggestion

Avoid any and always unmount the React root you create.

Suggested changes:

  • Type: const deleteRootRef = useRef<import('react-dom/client').Root | null>(null)
  • Cleanup:
return () => {
  markersRef.current.forEach(m => m.remove())
  markersRef.current = []

  deleteMarkerRef.current?.remove()
  deleteMarkerRef.current = null

  deleteRootRef.current?.unmount()
  deleteRootRef.current = null
}

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

}, [map, isDrawingMode, overlay.coordinates, updateCoordinates, removeOverlay])

Comment on lines +102 to +165

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set the delete marker location to overlay.coordinates[1] and comment it as “Near Top-right”, but the coordinate ordering is documented as [NW, NE, SE, SW], so [1] is exactly NE. That’s fine, but the marker won’t follow when coordinates change unless drawing mode is re-entered because you only create the delete marker inside the drawing-mode effect.

Since that effect depends on overlay.coordinates, it will re-run and recreate the delete marker on every drag (because you update global state on every drag). Combined with the lack of debouncing, this means you’re constantly destroying/recreating the React root and marker during drags.

This is avoidable churn.

Suggestion

Don’t recreate the delete marker/root on every coordinate update.

Option A: Create markers once when entering drawing mode, then update their positions in a separate effect that runs on overlay.coordinates.

Option B: Keep the current effect but move the delete marker position update to a lighter-weight branch:

// when already exists
deleteMarkerRef.current?.setLngLat(overlay.coordinates[1])

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

return null
}
12 changes: 10 additions & 2 deletions components/map/map-data-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ export interface CameraState {
heading?: number;
}

export interface ImageOverlay {
id: string;
url: string;
coordinates: [[number, number], [number, number], [number, number], [number, number]]; // [NW, NE, SE, SW]
opacity: number;
}

export interface MapData {
targetPosition?: { lat: number; lng: number } | null; // For flying to a location
cameraState?: CameraState; // For saving camera state
Expand All @@ -29,6 +36,7 @@ export interface MapData {
longitude: number;
title?: string;
}>;
imageOverlays?: ImageOverlay[];
}

interface MapDataContextType {
Expand All @@ -38,8 +46,8 @@ interface MapDataContextType {

const MapDataContext = createContext<MapDataContextType | undefined>(undefined);

export const MapDataProvider: React.FC<{ children: ReactNode }> = ({ children }) => {
const [mapData, setMapData] = useState<MapData>({ drawnFeatures: [], markers: [] });
export const MapDataProvider: React.FC<{ children: ReactNode; initialData?: MapData }> = ({ children, initialData }) => {
const [mapData, setMapData] = useState<MapData>(initialData || { drawnFeatures: [], markers: [], imageOverlays: [] });
Comment on lines +49 to +50

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MapDataProvider initializes state from initialData verbatim, which can leave drawnFeatures, markers, and imageOverlays as undefined depending on what the persisted payload contains. You partially work around this elsewhere with || [], but leaving the canonical state shape inconsistent tends to create subtle bugs (e.g., code that maps without guarding, or merge logic that assumes arrays exist).

Because initialData comes from JSON.parse, it’s especially important to normalize defaults at the boundary.

Suggestion

Normalize initialData into a full MapData object with array defaults rather than storing it raw.

For example:

export const MapDataProvider: React.FC<{ children: ReactNode; initialData?: MapData }> = ({ children, initialData }) => {
  const normalized: MapData = {
    drawnFeatures: [],
    markers: [],
    imageOverlays: [],
    ...initialData,
    drawnFeatures: initialData?.drawnFeatures ?? [],
    markers: initialData?.markers ?? [],
    imageOverlays: initialData?.imageOverlays ?? [],
  }

  const [mapData, setMapData] = useState<MapData>(normalized)
  ...
}

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

Comment on lines +49 to +50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Merge defaults when initialData is supplied.

initialData bypasses defaults, so partial data (older chats) can leave drawnFeatures, markers, or imageOverlays undefined. Merge with defaults to keep array invariants stable.

✅ Suggested fix
-export const MapDataProvider: React.FC<{ children: ReactNode; initialData?: MapData }> = ({ children, initialData }) => {
-  const [mapData, setMapData] = useState<MapData>(initialData || { drawnFeatures: [], markers: [], imageOverlays: [] });
+export const MapDataProvider: React.FC<{ children: ReactNode; initialData?: MapData }> = ({ children, initialData }) => {
+  const [mapData, setMapData] = useState<MapData>(() => ({
+    drawnFeatures: [],
+    markers: [],
+    imageOverlays: [],
+    ...initialData
+  }));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const MapDataProvider: React.FC<{ children: ReactNode; initialData?: MapData }> = ({ children, initialData }) => {
const [mapData, setMapData] = useState<MapData>(initialData || { drawnFeatures: [], markers: [], imageOverlays: [] });
export const MapDataProvider: React.FC<{ children: ReactNode; initialData?: MapData }> = ({ children, initialData }) => {
const [mapData, setMapData] = useState<MapData>(() => ({
drawnFeatures: [],
markers: [],
imageOverlays: [],
...initialData
}));
🤖 Prompt for AI Agents
In `@components/map/map-data-context.tsx` around lines 49 - 50, MapDataProvider
currently sets state to initialData directly which can omit
drawnFeatures/markers/imageOverlays; change the initialization in the
MapDataProvider (where mapData and setMapData are created) to merge initialData
with a default MapData object so missing fields fall back to empty arrays (e.g.
create a default like { drawnFeatures: [], markers: [], imageOverlays: [] } and
shallow-merge with initialData), and ensure any later setters that replace
mapData preserve those array defaults when merging updates.


return (
<MapDataContext.Provider value={{ mapData, setMapData }}>
Expand Down
4 changes: 4 additions & 0 deletions components/map/mapbox-map.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { useMapToggle, MapToggleEnum } from '../map-toggle-context'
import { useMapData } from './map-data-context'; // Add this import
import { useMapLoading } from '../map-loading-context'; // Import useMapLoading
import { useMap } from './map-context'
import { ImageOverlayLayer } from './image-overlay-layer'

mapboxgl.accessToken = process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN as string;

Expand Down Expand Up @@ -569,6 +570,9 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
onMouseUp={handleMouseUp}
onMouseLeave={handleMouseUp} // Clear timer if mouse leaves container while pressed
/>
{mapData.imageOverlays?.map(overlay => (
<ImageOverlayLayer key={overlay.id} overlay={overlay} />
))}
</div>
)
}
Loading