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]);

// 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}
/>
{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