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
5 changes: 5 additions & 0 deletions bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

205 changes: 201 additions & 4 deletions components/map/mapbox-map.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
'use client'

import { useEffect, useRef, useCallback } from 'react' // Removed useState
import { useEffect, useRef, useCallback, useState } from 'react'
import mapboxgl from 'mapbox-gl'
import 'mapbox-gl-compare/dist/mapbox-gl-compare.css'
import MapboxDraw from '@mapbox/mapbox-gl-draw'
import * as turf from '@turf/turf'
import { toast } from 'react-toastify'
Expand All @@ -17,7 +18,11 @@ mapboxgl.accessToken = process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN as string;

export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number; } }> = ({ position }) => {
const mapContainer = useRef<HTMLDivElement>(null)
const afterMapContainer = useRef<HTMLDivElement>(null)
const map = useRef<mapboxgl.Map | null>(null)
const afterMap = useRef<mapboxgl.Map | null>(null)
const compareRef = useRef<any>(null)
const afterMapMarkersRef = useRef<mapboxgl.Marker[]>([])
const { setMap } = useMap()
const drawRef = useRef<MapboxDraw | null>(null)
const rotationFrameRef = useRef<number | null>(null)
Expand Down Expand Up @@ -61,6 +66,107 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
}, [])

// Create measurement labels for all features
const syncDrawingsToAfterMap = useCallback(() => {
if (!afterMap.current || !afterMap.current.getSource('draw-mirror')) return;

const source = afterMap.current.getSource('draw-mirror') as mapboxgl.GeoJSONSource;
const features = mapData.drawnFeatures?.map(df => df.geometry) || [];

source.setData({
type: 'FeatureCollection',
features: features.map((g, i) => ({
type: 'Feature',
geometry: g,
properties: mapData.drawnFeatures?.[i] || {}
}))
});
Comment on lines +73 to +82
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

Redundant geometry extraction.

Line 74 extracts only geometries, but lines 78-82 rebuild features using the original drawnFeatures array anyway. The intermediate features variable is unnecessary.

♻️ Simplified approach
     const source = afterMap.current.getSource('draw-mirror') as mapboxgl.GeoJSONSource;
-    const features = mapData.drawnFeatures?.map(df => df.geometry) || [];
 
     source.setData({
       type: 'FeatureCollection',
-      features: features.map((g, i) => ({
+      features: (mapData.drawnFeatures || []).map(df => ({
         type: 'Feature',
-        geometry: g,
-        properties: mapData.drawnFeatures?.[i] || {}
+        geometry: df.geometry,
+        properties: { id: df.id, measurement: df.measurement, type: df.type }
       }))
     });
🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 74 - 83, The local features
variable is redundant—remove the intermediate const features and map directly
over mapData.drawnFeatures (or an empty array fallback) when calling
source.setData; for example, use (mapData.drawnFeatures || []).map(df => ({
type: 'Feature', geometry: df.geometry, properties: df || {} })) so you still
use df.geometry and preserve the original feature properties as before.


Comment on lines +69 to +83

Choose a reason for hiding this comment

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

Mirroring features is currently built by pairing mapData.drawnFeatures?.map(df => df.geometry) and then indexing back into mapData.drawnFeatures?.[i] for properties. If drawnFeatures order ever changes or contains non-geometry entries, this risks mismatched geometry↔properties. Also, you're setting properties to the entire drawnFeatures[i] object, which likely includes geometry again and other non-serializable/unused fields, increasing payload and making the GeoJSON properties noisy.

Suggestion

Build the mirrored FeatureCollection directly from drawnFeatures and keep properties minimal (e.g., id, measurement, type).

const features = (mapData.drawnFeatures ?? []).map(df => ({
  type: 'Feature' as const,
  geometry: df.geometry,
  properties: {
    id: df.id,
    measurement: df.measurement,
    featureType: df.type,
  },
}))

source.setData({ type: 'FeatureCollection', features })

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

// Mirror labels
afterMapMarkersRef.current.forEach(m => m.remove());
afterMapMarkersRef.current = [];

mapData.drawnFeatures?.forEach(feature => {
let coords: [number, number] | null = null;
if (feature.type === 'Polygon') {
const centroid = turf.centroid(feature.geometry);
coords = centroid.geometry.coordinates as [number, number];
} else if (feature.type === 'LineString') {
const line = feature.geometry.coordinates;
const midIndex = Math.floor(line.length / 2) - 1;
coords = (midIndex >= 0 ? line[midIndex] : line[0]) as [number, number];
}
Comment on lines +93 to +97

Choose a reason for hiding this comment

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

For LineString label placement, midIndex = Math.floor(line.length / 2) - 1 biases toward the first half and can produce -1 for very short lines. You fall back to line[0], but for longer lines you’re not actually selecting the midpoint vertex. This will look off for many line lengths.

Suggestion

Use the true midpoint vertex (or a proper along-line midpoint) for more stable placement.

Simple vertex midpoint:

const midIndex = Math.floor(line.length / 2)
coords = line[midIndex] as [number, number]

More accurate geometric midpoint:

const ls = turf.lineString(line)
const len = turf.length(ls)
const mid = turf.along(ls, len / 2)
coords = mid.geometry.coordinates as [number, number]

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

Comment on lines +92 to +97

Choose a reason for hiding this comment

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

The LineString label placement is likely off-by-one and can produce -1 for short lines. Math.floor(line.length / 2) - 1 makes the midpoint skew toward the start and for line.length === 1 yields -1 (you guard and fall back, but the index math is still odd). You probably want the actual midpoint index Math.floor((line.length - 1) / 2).

More importantly, this duplicates label placement logic already present in updateMeasurementLabels for the primary map. Divergence here will cause labels to appear in different places between the two maps over time.

Suggestion

Fix midpoint math and centralize label coordinate selection so both maps use the same function.

Example:

const getLabelCoord = (f: DrawnFeature): [number, number] | null => {
  if (f.type === 'Polygon') {
    const c = turf.centroid(f.geometry).geometry.coordinates
    return c as [number, number]
  }
  if (f.type === 'LineString') {
    const coords = f.geometry.coordinates
    const mid = Math.floor((coords.length - 1) / 2)
    return (coords[mid] ?? coords[0]) as [number, number]
  }
  return null
}

Then use it for both the primary labels and the mirrored labels to avoid drift. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.


if (coords && afterMap.current) {
const el = document.createElement('div');
el.className = feature.type === 'Polygon' ? 'area-label' : 'distance-label';
el.style.background = 'rgba(255, 255, 255, 0.8)'
el.style.padding = '4px 8px'
el.style.borderRadius = '4px'
el.style.fontSize = '12px'
el.style.fontWeight = 'bold'
el.style.color = '#333333'
el.style.boxShadow = '0 2px 4px rgba(0,0,0,0.2)'
el.style.pointerEvents = 'none'
el.textContent = feature.measurement;

const marker = new mapboxgl.Marker({ element: el })
.setLngLat(coords)
.addTo(afterMap.current);
afterMapMarkersRef.current.push(marker);
}
});
}, [mapData.drawnFeatures]);

const setupAfterMapLayers = useCallback(() => {
if (!afterMap.current) return;

// Add source for mirrored drawings
afterMap.current.addSource('draw-mirror', {
type: 'geojson',
data: {
type: 'FeatureCollection',
features: []
}
});
Comment on lines +120 to +130
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

Guard against duplicate source/layer creation.

If setupAfterMapLayers is called when the source already exists (e.g., due to effect re-runs), addSource will throw. Add existence checks before creating the source and layers.

🐛 Proposed fix
   const setupAfterMapLayers = useCallback(() => {
     if (!afterMap.current) return;
 
+    // Avoid duplicate source/layer creation
+    if (afterMap.current.getSource('draw-mirror')) return;
+
     // Add source for mirrored drawings
     afterMap.current.addSource('draw-mirror', {
📝 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
const setupAfterMapLayers = useCallback(() => {
if (!afterMap.current) return;
// Add source for mirrored drawings
afterMap.current.addSource('draw-mirror', {
type: 'geojson',
data: {
type: 'FeatureCollection',
features: []
}
});
const setupAfterMapLayers = useCallback(() => {
if (!afterMap.current) return;
// Avoid duplicate source/layer creation
if (afterMap.current.getSource('draw-mirror')) return;
// Add source for mirrored drawings
afterMap.current.addSource('draw-mirror', {
type: 'geojson',
data: {
type: 'FeatureCollection',
features: []
}
});
🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 121 - 131, The
setupAfterMapLayers function currently calls
afterMap.current.addSource('draw-mirror', ...) and may call addLayer without
checking for existing map objects; guard against duplicate creation by first
checking afterMap.current.getSource('draw-mirror') and
afterMap.current.getLayer('<layer-id>') (use the actual layer ids you add) and
only call addSource/addLayer when they do not exist, otherwise update the
existing source via setData or skip layer creation; update references in
setupAfterMapLayers accordingly to prevent addSource/addLayer throwing on
re-runs.


// Add layers for polygons and lines
afterMap.current.addLayer({
id: 'draw-mirror-polygons',
type: 'fill',
source: 'draw-mirror',
filter: ['==', '$type', 'Polygon'],
paint: {
'fill-color': '#fbb03b',
'fill-opacity': 0.1
}
});

afterMap.current.addLayer({
id: 'draw-mirror-polygons-outline',
type: 'line',
source: 'draw-mirror',
filter: ['==', '$type', 'Polygon'],
paint: {
'line-color': '#fbb03b',
'line-width': 2
}
});

afterMap.current.addLayer({
id: 'draw-mirror-lines',
type: 'line',
source: 'draw-mirror',
filter: ['==', '$type', 'LineString'],
paint: {
'line-color': '#fbb03b',
'line-width': 2
}
});

// Initial sync
syncDrawingsToAfterMap();
}, [syncDrawingsToAfterMap]);

const updateMeasurementLabels = useCallback(() => {
if (!map.current || !drawRef.current) return

Expand Down Expand Up @@ -335,6 +441,79 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
}
}, [setMapData])

// Handle comparison map initialization
useEffect(() => {
if (mapType === MapToggleEnum.DrawingMode && map.current && afterMapContainer.current && !afterMap.current) {
const center = map.current.getCenter();
const zoom = map.current.getZoom();
const pitch = map.current.getPitch();
const bearing = map.current.getBearing();

afterMap.current = new mapboxgl.Map({
container: afterMapContainer.current,
style: 'mapbox://styles/mapbox/streets-v12',
center: [center.lng, center.lat],
zoom: zoom,
pitch: pitch,
bearing: bearing,
maxZoom: 22,
attributionControl: false,
});

afterMap.current.on('load', () => {
if (!map.current || !afterMap.current || !afterMapContainer.current?.parentElement) return;

try {
// Late-import mapbox-gl-compare to avoid SSR issues
const CompareModule = require('mapbox-gl-compare');
const CompareConstructor = CompareModule.default || CompareModule;

// Create the compare control
compareRef.current = new CompareConstructor(
map.current,
afterMap.current,
afterMapContainer.current.parentElement,
{
orientation: 'vertical',
mousemove: false
}
);

// Setup layers on afterMap
setupAfterMapLayers();
} catch (error) {
console.error('Error initializing mapbox-gl-compare:', error);
}
});
}

// Cleanup when leaving DrawingMode
if (mapType !== MapToggleEnum.DrawingMode) {
if (compareRef.current) {
compareRef.current.remove();
compareRef.current = null;
}
if (afterMap.current) {
afterMap.current.remove();
afterMap.current = null;
}
afterMapMarkersRef.current.forEach(m => m.remove());
afterMapMarkersRef.current = [];

if (map.current) {
const container = map.current.getContainer();
if (container) container.style.clip = '';
}
}
}, [mapType, setupAfterMapLayers]);
Comment on lines 445 to 508
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

Missing cleanup function in useEffect.

This effect creates map instances and controls but doesn't return a cleanup function. If the component unmounts while in DrawingMode, the afterMap and compareRef will leak. The cleanup logic at lines 484-500 only runs on re-renders when mapType changes, not on unmount.

🐛 Proposed fix — add a cleanup return
       // Setup layers on afterMap
       setupAfterMapLayers();
     });
   }
 
   // Cleanup when leaving DrawingMode
   if (mapType !== MapToggleEnum.DrawingMode) {
     if (compareRef.current) {
       compareRef.current.remove();
       compareRef.current = null;
     }
     if (afterMap.current) {
       afterMap.current.remove();
       afterMap.current = null;
     }
     afterMapMarkersRef.current.forEach(m => m.remove());
     afterMapMarkersRef.current = [];
 
     if (map.current) {
       const container = map.current.getContainer();
       if (container) container.style.clip = '';
     }
   }
+
+  return () => {
+    // Cleanup on unmount
+    if (compareRef.current) {
+      compareRef.current.remove();
+      compareRef.current = null;
+    }
+    if (afterMap.current) {
+      afterMap.current.remove();
+      afterMap.current = null;
+    }
+    afterMapMarkersRef.current.forEach(m => m.remove());
+    afterMapMarkersRef.current = [];
+  };
 }, [mapType, setupAfterMapLayers]);

Note: The main initialization useEffect (lines 576-717) does include cleanup for these refs, but relying on that creates a fragile ordering dependency between effects.

🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 446 - 501, The useEffect watching
mapType creates afterMap (afterMap.current) and the compare control
(compareRef.current) but doesn't return a cleanup, so unmounting while
MapToggleEnum.DrawingMode will leak those resources; modify that useEffect to
return a cleanup function that mirrors the existing non-DrawingMode teardown: if
compareRef.current call remove() and null it, if afterMap.current call remove()
and null it, remove and clear afterMapMarkersRef.current, and reset
map.current.getContainer().style.clip if map.current exists; ensure you still
call setupAfterMapLayers() only when afterMap is initialized.

Comment on lines 445 to 508

Choose a reason for hiding this comment

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

The compare init effect creates afterMap and registers a 'load' listener, but there’s no explicit removal of that listener if mapType changes quickly or the component unmounts before the map loads. While afterMap.remove() will usually clean up, this pattern is prone to racey behavior and makes it easier to accidentally create duplicate controls if load fires after state changes.

Suggestion

Capture the handler and unregister it in a cleanup function scoped to this effect. Also consider gating creation with a local isCancelled flag.

useEffect(() => {
  if (mapType !== MapToggleEnum.DrawingMode || !map.current || !afterMapContainer.current || afterMap.current) return

  const m = new mapboxgl.Map({ /* ... */ })
  afterMap.current = m

  let cancelled = false
  const onLoad = () => {
    if (cancelled || !map.current || !afterMap.current || !afterMapContainer.current?.parentElement) return
    compareRef.current = new Compare(map.current, afterMap.current, afterMapContainer.current.parentElement, { orientation: 'vertical', mousemove: false })
    setupAfterMapLayers()
  }

  m.on('load', onLoad)

  return () => {
    cancelled = true
    m.off('load', onLoad)
  }
}, [mapType, setupAfterMapLayers])

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


// Effect to sync drawings when they change
useEffect(() => {
if (mapType === MapToggleEnum.DrawingMode) {
syncDrawingsToAfterMap();
}
}, [mapData.drawnFeatures, mapType, syncDrawingsToAfterMap]);

// Set up idle rotation checker
useEffect(() => {
const checkIdle = setInterval(() => {
Expand Down Expand Up @@ -510,6 +689,18 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
stopRotation()
setIsMapLoaded(false) // Reset map loaded state on cleanup
setMap(null) // Clear map instance from context

if (compareRef.current) {
compareRef.current.remove()
compareRef.current = null
}
if (afterMap.current) {
afterMap.current.remove()
afterMap.current = null
}
afterMapMarkersRef.current.forEach(m => m.remove());
afterMapMarkersRef.current = [];

map.current.remove()
map.current = null
}
Expand Down Expand Up @@ -633,14 +824,20 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number


return (
<div className="relative h-full w-full">
<div className="relative h-full w-full overflow-hidden rounded-l-lg" id="comparison-container">
<div
ref={mapContainer}
className="h-full w-full overflow-hidden rounded-l-lg"
className={mapType === MapToggleEnum.DrawingMode ? "absolute inset-0" : "h-full w-full"}
onMouseDown={handleMouseDown}
onMouseUp={handleMouseUp}
onMouseLeave={handleMouseUp} // Clear timer if mouse leaves container while pressed
onMouseLeave={handleMouseUp}
/>
Comment on lines 828 to 834
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

Add accessibility attributes to interactive container.

The map container div has mouse event handlers but lacks ARIA attributes. Screen readers won't understand its purpose.

♿ Suggested fix
     <div
       ref={mapContainer}
       className="absolute top-0 bottom-0 w-full"
+      role="application"
+      aria-label="Interactive map"
       onMouseDown={handleMouseDown}
       onMouseUp={handleMouseUp}
       onMouseLeave={handleMouseUp}
     />
📝 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
<div
ref={mapContainer}
className="h-full w-full overflow-hidden rounded-l-lg"
className="absolute top-0 bottom-0 w-full"
onMouseDown={handleMouseDown}
onMouseUp={handleMouseUp}
onMouseLeave={handleMouseUp} // Clear timer if mouse leaves container while pressed
onMouseLeave={handleMouseUp}
/>
<div
ref={mapContainer}
className="absolute top-0 bottom-0 w-full"
role="application"
aria-label="Interactive map"
onMouseDown={handleMouseDown}
onMouseUp={handleMouseUp}
onMouseLeave={handleMouseUp}
/>
🧰 Tools
🪛 Biome (2.1.2)

[error] 821-828: Static Elements should not be interactive.

To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.

(lint/a11y/noStaticElementInteractions)

🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 821 - 827, The map container div
(ref=mapContainer) has mouse handlers (handleMouseDown, handleMouseUp) but no
ARIA/keyboard accessibility: add appropriate attributes like role="region" (or
role="application" if interactive), a descriptive aria-label, and tabIndex={0}
on the same element, and wire keyboard equivalents (e.g., onKeyDown/onKeyUp or
a11y wrappers that call handleMouseDown/handleMouseUp) so keyboard and
screen‑reader users can interact; update the element that currently uses
ref={mapContainer} and the event handlers to include these attributes and
keyboard handler references.

{mapType === MapToggleEnum.DrawingMode && (
<div
ref={afterMapContainer}
className="absolute inset-0 pointer-events-none"
/>
)}
Comment on lines +835 to +840
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

pointer-events-none blocks Compare slider interaction.

The pointer-events-none class on the afterMapContainer prevents the Compare control's slider from receiving mouse/touch events, which is the root cause of the "no map render" issue reported in PR comments.

The Compare control needs to capture pointer events on both map containers to function correctly.

🐛 Proposed fix
       {mapType === MapToggleEnum.DrawingMode && (
         <div
           ref={afterMapContainer}
-          className="absolute inset-0 pointer-events-none"
+          className="absolute inset-0"
         />
       )}
🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 835 - 840, The afterMapContainer
div rendered when mapType === MapToggleEnum.DrawingMode uses the CSS class
pointer-events-none which prevents the Compare control's slider from receiving
events; remove or change that class so the afterMapContainer allows pointer
events (e.g., replace pointer-events-none with pointer-events-auto or
conditionally omit it when the Compare control is active) so both map containers
can capture pointer events and the Compare slider functions; locate the JSX that
renders afterMapContainer inside the MapToggleEnum.DrawingMode branch and update
the className logic accordingly.

</div>
)
}
21 changes: 21 additions & 0 deletions lib/types/mapbox-gl-compare.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
declare module 'mapbox-gl-compare' {
import { Map } from 'mapbox-gl';

export interface CompareOptions {
orientation?: 'vertical' | 'horizontal';
mousemove?: boolean;
}

export default class Compare {
constructor(
a: Map,
b: Map,
container: string | HTMLElement,
options?: CompareOptions
);
remove(): void;
on(type: string, fn: Function): this;
off(type: string, fn: Function): this;
Comment on lines +17 to +18
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 using stricter types for event handlers.

Using Function type is too permissive. Consider defining specific event types for better type safety.

♻️ Suggested improvement
-    on(type: string, fn: Function): this;
-    off(type: string, fn: Function): this;
+    on(type: 'slideend', fn: (event: { currentTarget: Compare }) => void): this;
+    off(type: 'slideend', fn: (event: { currentTarget: Compare }) => void): this;

Alternatively, if multiple event types are supported, use a union type or generic constraint.

📝 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
on(type: string, fn: Function): this;
off(type: string, fn: Function): this;
on(type: 'slideend', fn: (event: { currentTarget: Compare }) => void): this;
off(type: 'slideend', fn: (event: { currentTarget: Compare }) => void): this;
🤖 Prompt for AI Agents
In `@lib/types/mapbox-gl-compare.d.ts` around lines 17 - 18, The on/off method
signatures use the overly-broad Function type — update them to use stricter,
typed event handlers: define an EventMap (e.g., interface CompareEvents {
compare?: (evt: CompareEvent) => void; move?: (evt: MouseEvent) => void; ... })
or a generic like <K extends keyof EventMap>(type: K, fn: EventMap[K]) and then
change the signatures for on and off to on<K extends keyof EventMap>(type: K,
fn: EventMap[K]): this and off<K extends keyof EventMap>(type: K, fn:
EventMap[K]): this so callers get proper argument types and autocomplete for the
on/off methods.

setSlider(x: number): void;
}
Comment on lines +1 to +20

Choose a reason for hiding this comment

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

The type declaration for Compare#on/off uses fn: Function, which is effectively any and undermines type safety across the codebase. Since this is a shared .d.ts, it’s worth keeping it strict to avoid type leakage.

Also, the event type: string could be narrowed to known events if you want, but even a typed callback signature is an improvement.

Suggestion

Replace Function with a safer callable type (and optionally narrow event names).

export default class Compare {
  // ...
  on(type: string, fn: (...args: unknown[]) => void): this
  off(type: string, fn: (...args: unknown[]) => void): this
}

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

}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
"lottie-react": "^2.4.1",
"lucide-react": "^0.507.0",
"mapbox-gl": "^3.11.0",
"mapbox-gl-compare": "^0.4.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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the peer dependencies, dependencies, and metadata for mapbox-gl-compare v0.4.2
curl -s https://registry.npmjs.org/mapbox-gl-compare/0.4.2 | jq '.peerDependencies, .dependencies, .version'

echo "---"

# Check the latest version available and its details
curl -s https://registry.npmjs.org/mapbox-gl-compare/latest | jq '.peerDependencies, .dependencies, .version'

echo "---"

# Check all versions available
curl -s https://registry.npmjs.org/mapbox-gl-compare | jq '.["dist-tags"]'

Repository: QueueLab/QCX

Length of output: 205


Test mapbox-gl-compare v0.4.2 compatibility with mapbox-gl v3.x before deployment.

The mapbox-gl-compare package doesn't declare mapbox-gl as a peer dependency, and v0.4.2 (the latest version) appears unmaintained. This version predates mapbox-gl v3 and may not work correctly with the project's mapbox-gl v3.11.0 dependency. Confirm in testing that the compare functionality works as expected.

🤖 Prompt for AI Agents
In `@package.json` at line 76, Verify compatibility of the "mapbox-gl-compare"
dependency with our "mapbox-gl" v3.11.0 by running the interactive compare
control in the app and automated integration tests: confirm the compare
slider/control renders, layers sync correctly (pan/zoom/paint/opacity), and no
console errors occur; if issues appear, either lock "mapbox-gl" to a compatible
version or replace "mapbox-gl-compare" with a maintained alternative (or
fork/patch it) and add a peerDependency declaration for "mapbox-gl" in
package.json to document compatibility; update tests to cover compare
functionality and commit the package.json change referencing "mapbox-gl-compare"
and "mapbox-gl".

"next": "15.3.6",
"next-themes": "^0.3.0",
"open-codex": "^0.1.30",
Expand Down