Add Mapbox Compare Slider to Drawing Mode #454
Conversation
- Installed mapbox-gl-compare and added type declarations. - Implemented dual map synchronization in Mapbox component. - Enabled split-screen comparison when entering Drawing Mode. - Synced drawn geometries and measurement labels between both maps. - Added pointer-events optimization for drawing over the comparison layer. - Ensured proper cleanup of map instances and comparison controls. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
|
WalkthroughAdds a side-by-side "after-map" comparison using mapbox-gl-compare, mirrors drawn features and dynamic labels from the main map to the after-map, and manages lifecycle (init, sync, cleanup) when entering/exiting DrawingMode. Also adds TypeScript types and a runtime dependency. Changes
Sequence DiagramsequenceDiagram
participant User
participant MapComponent as Map Component
participant MainMap as Main Map
participant AfterMap as After Map
participant Compare as Compare Control
participant DrawLayer as Draw Layer
User->>MapComponent: Enter DrawingMode
MapComponent->>AfterMap: Initialize after-map instance
MapComponent->>Compare: Create Compare control (MainMap, AfterMap)
User->>DrawLayer: Draw polygon/line on MainMap
DrawLayer->>MapComponent: Emit draw update
MapComponent->>MapComponent: syncDrawingsToAfterMap()
MapComponent->>AfterMap: Update draw-mirror GeoJSON source
MapComponent->>AfterMap: Render polygon area / line distance labels
User->>Compare: Drag comparison slider
Compare->>MainMap: Update visible area
Compare->>AfterMap: Update visible area
User->>MapComponent: Exit DrawingMode
MapComponent->>Compare: Destroy Compare control
MapComponent->>AfterMap: Cleanup after-map and markers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/map/mapbox-map.tsx (1)
767-816: Duplicate useEffect with overlapping cleanup logic.This
useEffect(lines 767-816) duplicates the cleanup logic from the earlieruseEffect(lines 576-717). Both have nearly identical dependency arrays and cleanup code. This will cause:
- Double cleanup attempts on unmount
- Potential errors from removing already-removed controls
- Code maintenance burden
🐛 Proposed fix — remove the duplicate effect
Remove lines 767-816 entirely. The cleanup logic is already handled by the initialization
useEffectat lines 576-717.- // Cleanup for the main useEffect - useEffect(() => { - // ... existing useEffect logic ... - return () => { - // ... existing cleanup logic ... - if (longPressTimerRef.current) { // Cleanup timer on component unmount - clearTimeout(longPressTimerRef.current); - longPressTimerRef.current = null; - } - // ... existing cleanup logic for map and geolocation ... - if (map.current) { - map.current.off('moveend', captureMapCenter) - - if (drawRef.current) { - try { - map.current.off('draw.create', updateMeasurementLabels) - map.current.off('draw.delete', updateMeasurementLabels) - map.current.off('draw.update', updateMeasurementLabels) - map.current.removeControl(drawRef.current) - } catch (e) { - console.log('Draw control already removed') - } - } - - Object.values(polygonLabelsRef.current).forEach(marker => marker.remove()) - Object.values(lineLabelsRef.current).forEach(marker => marker.remove()) - - stopRotation() - setIsMapLoaded(false) - setMap(null) - map.current.remove() - map.current = null - } - - if (geolocationWatchIdRef.current !== null) { - navigator.geolocation.clearWatch(geolocationWatchIdRef.current) - geolocationWatchIdRef.current = null - } - }; - }, [ - handleUserInteraction, - startRotation, - stopRotation, - mapType, // mapType is already here, good. - updateMeasurementLabels, - setupGeolocationWatcher, - captureMapCenter, - setupDrawingTools, - setIsMapLoaded, - setMap - ]);If
longPressTimerRefcleanup is needed, add it to the existing cleanup block at line 700:afterMapMarkersRef.current.forEach(m => m.remove()); afterMapMarkersRef.current = []; + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current); + longPressTimerRef.current = null; + } + map.current.remove()
🤖 Fix all issues with AI agents
In `@components/map/mapbox-map.tsx`:
- Line 3: The inline comment on the import line is stale; update the import
statement that references useEffect, useRef, useCallback, useState so it no
longer contains the incorrect comment "// Removed useState" — either remove the
comment entirely or change it to accurately reflect the current imports (refer
to the import line that includes useState to locate the change).
- Around line 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.
- Around line 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.
- Around line 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.
- Around line 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.
In `@lib/types/mapbox-gl-compare.d.ts`:
- Around line 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.
In `@package.json`:
- 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".
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
components/map/mapbox-map.tsxlib/types/mapbox-gl-compare.d.tspackage.json
🧰 Additional context used
🧬 Code graph analysis (1)
components/map/mapbox-map.tsx (3)
lib/types/mapbox-gl-compare.d.ts (1)
Compare(9-20)components/map/google-map.tsx (1)
features(38-52)components/map/google-geojson-layer.tsx (1)
map(14-57)
🪛 Biome (2.1.2)
lib/types/mapbox-gl-compare.d.ts
[error] 2-2: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
components/map/mapbox-map.tsx
[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)
🔇 Additional comments (3)
components/map/mapbox-map.tsx (3)
5-6: LGTM on imports.The
mapbox-gl-compareimport and CSS inclusion are correctly placed.
22-26: LGTM on new refs.The refs for
afterMapContainer,afterMap,compareRef, andafterMapMarkersRefare appropriately defined for managing the comparison feature lifecycle.
504-508: LGTM on drawing sync effect.This effect correctly triggers synchronization when
drawnFeatureschange while inDrawingMode.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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] || {} | ||
| })) | ||
| }); |
There was a problem hiding this comment.
🧹 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.
| const setupAfterMapLayers = useCallback(() => { | ||
| if (!afterMap.current) return; | ||
|
|
||
| // Add source for mirrored drawings | ||
| afterMap.current.addSource('draw-mirror', { | ||
| type: 'geojson', | ||
| data: { | ||
| type: 'FeatureCollection', | ||
| features: [] | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
| 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.
| 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; | ||
|
|
||
| // Create the compare control | ||
| compareRef.current = new Compare( | ||
| map.current, | ||
| afterMap.current, | ||
| afterMapContainer.current.parentElement, | ||
| { | ||
| orientation: 'vertical', | ||
| mousemove: false | ||
| } | ||
| ); | ||
|
|
||
| // 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 = ''; | ||
| } | ||
| } | ||
| }, [mapType, setupAfterMapLayers]); |
There was a problem hiding this comment.
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.
| <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} | ||
| /> |
There was a problem hiding this comment.
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.
| <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.
| on(type: string, fn: Function): this; | ||
| off(type: string, fn: Function): this; |
There was a problem hiding this comment.
🧹 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.
| 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.
| "lottie-react": "^2.4.1", | ||
| "lucide-react": "^0.507.0", | ||
| "mapbox-gl": "^3.11.0", | ||
| "mapbox-gl-compare": "^0.4.2", |
There was a problem hiding this comment.
🧩 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".
|
@coderabbitai there is no map render |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main risk is non-idempotent Mapbox source/layer creation (addSource/addLayer) which can throw and break the compare view in dev (Strict Mode) or on re-init paths. The mirrored GeoJSON uses a fragile geometry↔properties pairing and stuffs large objects into properties, increasing payload/noise. Line label placement is off by one for many lines and should use a true midpoint. The local mapbox-gl-compare type declaration leaks Function (effectively any) and should be tightened.
Additional notes (1)
- Maintainability |
components/map/mapbox-map.tsx:464-476
Compare is constructed withafterMapContainer.current.parentElementas the container. In your JSX,afterMapContaineris a sibling of the main map container inside the same parent, soparentElementis the outer wrapper (#comparison-container). That’s correct, but it creates a tight coupling to DOM structure.
If the markup changes (e.g., wrapper added for overlays), compare could attach to the wrong node. You already assign an id="comparison-container"; use a ref for the wrapper and pass it explicitly to Compare to avoid reliance on parentElement.
Summary of changes
What changed
🆕 Split-screen compare in Drawing Mode
- Added a secondary “after” Mapbox instance (
afterMap) and container (afterMapContainer) that is only mounted whenmapType === MapToggleEnum.DrawingMode. - Integrated
mapbox-gl-compareto render a vertical comparison slider between the existing map and the secondary map.
🔁 Mirroring drawings + labels
- Introduced
draw-mirrorGeoJSON source and layers on the secondary map to render mirrored polygons/lines. - Added
syncDrawingsToAfterMap()to:- push drawing geometries into
draw-mirror - recreate measurement markers on the secondary map (
afterMapMarkersRef).
- push drawing geometries into
🧹 Cleanup behavior
- Added teardown for
Compare,afterMap, and mirrored markers when leaving Drawing Mode and when unmounting the main map.
📦 Dependencies / types
- Added
mapbox-gl-comparedependency. - Added
lib/types/mapbox-gl-compare.d.tsmodule declaration for TypeScript consumption.
| 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] || {} | ||
| })) | ||
| }); | ||
|
|
There was a problem hiding this comment.
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.
| } 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]; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | ||
|
|
||
| // Create the compare control | ||
| compareRef.current = new Compare( | ||
| map.current, | ||
| afterMap.current, | ||
| afterMapContainer.current.parentElement, | ||
| { | ||
| orientation: 'vertical', | ||
| mousemove: false | ||
| } | ||
| ); | ||
|
|
||
| // 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 = ''; | ||
| } | ||
| } | ||
| }, [mapType, setupAfterMapLayers]); |
There was a problem hiding this comment.
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.
| 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]; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | ||
| setSlider(x: number): void; | ||
| } |
There was a problem hiding this comment.
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.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 28456 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 145 🏁 Script executed: Length of output: 0 🌐 Web query: 💡 Result: A minimal <!doctype html>
<html>
<head>
<meta charset="utf-8" />
<meta name="viewport" content="initial-scale=1,maximum-scale=1,user-scalable=no" />
<link href="https://api.mapbox.com/mapbox-gl-js/v3.18.0/mapbox-gl.css" rel="stylesheet" />
<script src="https://api.mapbox.com/mapbox-gl-js/v3.18.0/mapbox-gl.js"></script>
<script src="https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-compare/v0.4.0/mapbox-gl-compare.js"></script>
<link href="https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-compare/v0.4.0/mapbox-gl-compare.css" rel="stylesheet" />
<style>
html, body { margin: 0; height: 100%; }
`#comparison-container` { position: relative; height: 100%; width: 100%; }
.map { position: absolute; top: 0; bottom: 0; width: 100%; }
</style>
</head>
<body>
<div id="comparison-container">
<div id="before" class="map"></div>
<div id="after" class="map"></div>
</div>
<script>
mapboxgl.accessToken = "YOUR_MAPBOX_ACCESS_TOKEN";
const beforeMap = new mapboxgl.Map({
container: "before",
style: "mapbox://styles/mapbox/light-v11",
center: [0, 0],
zoom: 1
});
const afterMap = new mapboxgl.Map({
container: "after",
style: "mapbox://styles/mapbox/dark-v11",
center: [0, 0],
zoom: 1
});
new mapboxgl.Compare(beforeMap, afterMap, "#comparison-container", {
// mousemove: true,
// orientation: "vertical" // or "horizontal"
});
</script>
</body>
</html>Sources: Mapbox GL JS “Swipe between maps” example [1] and the
The ProblemOn line 502, the Compare control is created with the wrong container: compareRef.current = new Compare(
map.current,
afterMap.current,
afterMapContainer.current.parentElement, // ❌ Wrong!
{ orientation: 'vertical', mousemove: false }
);The The FixChange line 502 to pass the correct container: compareRef.current = new Compare(
map.current,
afterMap.current,
'#comparison-container', // ✅ Correct! or document.getElementById('comparison-container')
{ orientation: 'vertical', mousemove: false }
);Alternatively, you could create a ref for the comparison container: // At the top with other refs:
const comparisonContainer = useRef<HTMLDivElement>(null);
// Then in the JSX:
<div ref={comparisonContainer} className="relative h-full w-full overflow-hidden rounded-l-lg" id="comparison-container">
// And in the Compare initialization:
compareRef.current = new Compare(
map.current,
afterMap.current,
comparisonContainer.current!,
{ orientation: 'vertical', mousemove: false }
);Either approach will work. The key is that the Compare library needs the container that wraps both map divs, not a level above it. |
Thank you for the feedback. I've fixed an issue where the map was failing to render due to a server-side rendering crash in the |
- Fixed ReferenceError: window is not defined by late-importing mapbox-gl-compare on the client side. - Updated map container layout to use inset-0 for more reliable rendering. - Addressed PR feedback regarding map rendering issues. - Cleaned up comments and refined synchronization logic. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/map/mapbox-map.tsx (1)
772-822: Critical: Duplicate useEffect with placeholder code will cause double execution.This
useEffect(lines 772-822) has the same dependency array as the main initialization effect (lines 582-723). The placeholder comments like// ... existing useEffect logic ...suggest this was accidentally committed incomplete code. Having two effects with identical dependencies will:
- Execute the same cleanup logic twice on unmount
- Potentially cause race conditions during initialization
- Call
map.current.remove()twice, causing errorsRemove this entire duplicate effect block (lines 772-822).
🤖 Fix all issues with AI agents
In `@components/map/mapbox-map.tsx`:
- Around line 9-13: The variable Compare is currently typed as any losing type
safety; change its declaration to retain the imported CompareClass type and cast
the late require accordingly (keep the existing window check). Specifically,
replace the any usage on Compare with a nullable typed reference to CompareClass
(or typeof CompareClass as appropriate) and assert the result of
require('mapbox-gl-compare') to that type so callers of Compare keep proper type
information.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/map/mapbox-map.tsx
🧰 Additional context used
🪛 Biome (2.1.2)
components/map/mapbox-map.tsx
[error] 827-834: 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)
🔇 Additional comments (2)
components/map/mapbox-map.tsx (2)
509-514: LGTM!The sync effect correctly triggers drawing synchronization when
drawnFeatureschange while in DrawingMode.
691-701: LGTM!Proper cleanup of comparison resources in the main effect's cleanup function ensures no resource leaks on full component unmount.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
components/map/mapbox-map.tsx
Outdated
| // Late-import mapbox-gl-compare to avoid SSR issues | ||
| let Compare: any = null; | ||
| if (typeof window !== 'undefined') { | ||
| Compare = require('mapbox-gl-compare'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type safety lost with any.
The CompareClass type is imported but Compare is assigned any. Use a type assertion to retain type information:
-let Compare: any = null;
+let Compare: typeof CompareClass | null = null;
if (typeof window !== 'undefined') {
- Compare = require('mapbox-gl-compare');
+ Compare = require('mapbox-gl-compare') as typeof CompareClass;
}🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 9 - 13, The variable Compare is
currently typed as any losing type safety; change its declaration to retain the
imported CompareClass type and cast the late require accordingly (keep the
existing window check). Specifically, replace the any usage on Compare with a
nullable typed reference to CompareClass (or typeof CompareClass as appropriate)
and assert the result of require('mapbox-gl-compare') to that type so callers of
Compare keep proper type information.
|
@coderabbitai there is no mapbox render anymore debug this |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 28583
Problems Identified
The FixReplace the comparison map initialization code (lines 477-517) with this corrected version: // 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 || !mapContainer.current || !afterMapContainer.current) return;
// Clean up any previous styles on main map container
const mainContainer = mapContainer.current;
mainContainer.style.clip = '';
mainContainer.style.clipPath = '';
// Wait a frame to ensure both maps are fully rendered
requestAnimationFrame(() => {
if (!map.current || !afterMap.current || !mapContainer.current?.parentElement) return;
try {
// Create the compare control with the parent container
compareRef.current = new Compare(
map.current,
afterMap.current,
mapContainer.current.parentElement, // Use mapContainer's parent (the comparison-container)
{
orientation: 'vertical',
mousemove: false
}
);
console.log('Compare control initialized successfully');
// Setup layers on afterMap
setupAfterMapLayers();
} catch (error) {
console.error('Error initializing Compare control:', error);
}
});
});
}
// Cleanup when leaving DrawingMode
if (mapType !== MapToggleEnum.DrawingMode) {
if (compareRef.current) {
try {
compareRef.current.remove();
} catch (e) {
console.error('Error removing compare:', e);
}
compareRef.current = null;
}
if (afterMap.current) {
afterMap.current.remove();
afterMap.current = null;
}
afterMapMarkersRef.current.forEach(m => m.remove());
afterMapMarkersRef.current = [];
// Clean up main map container styles
if (map.current) {
const container = map.current.getContainer();
if (container) {
container.style.clip = '';
container.style.clipPath = '';
}
}
}
}, [mapType, setupAfterMapLayers]);And update the JSX (lines 757-774) to remove return (
<div className="relative h-full w-full overflow-hidden rounded-l-lg" id="comparison-container">
<div
ref={mapContainer}
className="absolute inset-0"
onMouseDown={handleMouseDown}
onMouseUp={handleMouseUp}
onMouseLeave={handleMouseUp}
/>
{mapType === MapToggleEnum.DrawingMode && (
<div
ref={afterMapContainer}
className="absolute inset-0"
/>
)}
</div>
)Key Changes
Try these changes and let me know if the maps render correctly now. Check the browser console for the "Compare control initialized successfully" message or any error messages. |
I've investigated the rendering issue. It was caused by a server-side execution of the |
- Moved mapbox-gl-compare requirement into a client-side useEffect to prevent window ReferenceError during SSR. - Robustified the Compare constructor access to handle potential default export wrapping. - Ensured stable map container dimensions by using h-full w-full by default. - Added error handling for compare control initialization. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/map/mapbox-map.tsx (1)
773-823: Critical: Duplicate useEffect causes double cleanup and errors.This
useEffect(lines 773-823) has the same dependency array as the main initializationuseEffect(lines 583-724) and contains duplicate cleanup logic. This will cause:
- Double execution of cleanup when dependencies change or on unmount
map.current.remove()called twice, throwing errors on the second call- All marker/label cleanup running twice
This appears to be residual code from development that should be removed entirely.
🐛 Proposed fix — remove the duplicate useEffect
- // Cleanup for the main useEffect - useEffect(() => { - // ... existing useEffect logic ... - return () => { - // ... existing cleanup logic ... - if (longPressTimerRef.current) { // Cleanup timer on component unmount - clearTimeout(longPressTimerRef.current); - longPressTimerRef.current = null; - } - // ... existing cleanup logic for map and geolocation ... - if (map.current) { - map.current.off('moveend', captureMapCenter) - - if (drawRef.current) { - try { - map.current.off('draw.create', updateMeasurementLabels) - map.current.off('draw.delete', updateMeasurementLabels) - map.current.off('draw.update', updateMeasurementLabels) - map.current.removeControl(drawRef.current) - } catch (e) { - console.log('Draw control already removed') - } - } - - Object.values(polygonLabelsRef.current).forEach(marker => marker.remove()) - Object.values(lineLabelsRef.current).forEach(marker => marker.remove()) - - stopRotation() - setIsMapLoaded(false) - setMap(null) - map.current.remove() - map.current = null - } - - if (geolocationWatchIdRef.current !== null) { - navigator.geolocation.clearWatch(geolocationWatchIdRef.current) - geolocationWatchIdRef.current = null - } - }; - }, [ - handleUserInteraction, - startRotation, - stopRotation, - mapType, // mapType is already here, good. - updateMeasurementLabels, - setupGeolocationWatcher, - captureMapCenter, - setupDrawingTools, - setIsMapLoaded, - setMap - ]);Instead, add the
longPressTimerRefcleanup to the existing main initialization useEffect's cleanup (around line 670):return () => { + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current); + longPressTimerRef.current = null; + } if (map.current) { map.current.off('moveend', captureMapCenter)
🤖 Fix all issues with AI agents
In `@components/map/mapbox-map.tsx`:
- Around line 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.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/map/mapbox-map.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/map/mapbox-map.tsx (2)
components/map/geojson-layer.tsx (1)
map(16-97)components/map/google-map.tsx (1)
features(38-52)
🪛 Biome (2.1.2)
components/map/mapbox-map.tsx
[error] 828-835: 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)
🔇 Additional comments (4)
components/map/mapbox-map.tsx (4)
21-25: New refs for comparison map look appropriate.The refs for
afterMapContainer,afterMap,compareRef, andafterMapMarkersRefare correctly initialized for managing the comparison slider lifecycle.
69-118: Sync function logic is sound.The
syncDrawingsToAfterMapfunction correctly mirrors drawings and labels to the after-map. The marker cleanup before recreation prevents accumulation.
510-515: Sync effect is correctly scoped.The effect appropriately triggers synchronization only when in DrawingMode and when
drawnFeatureschanges.
692-703: Cleanup for comparison resources correctly added.The cleanup for
compareRef,afterMap, andafterMapMarkersRefis appropriately placed in the main initialization effect's cleanup function to handle component unmount.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| {mapType === MapToggleEnum.DrawingMode && ( | ||
| <div | ||
| ref={afterMapContainer} | ||
| className="absolute inset-0 pointer-events-none" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
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.
User description
This change implements a Mapbox split-screen comparison slider that activates specifically when the user enters Drawing Mode. It allows comparing the default Satellite Streets view with a standard Streets view while keeping all drawings, measurements, and labels perfectly synced on both sides.
PR created automatically by Jules for task 11900621403850848071 started by @ngoiyaeric
PR Type
Enhancement
Description
Adds Mapbox split-screen comparison slider for Drawing Mode
Implements dual map synchronization with Streets view
Mirrors drawn geometries and measurement labels on comparison map
Automatically activates/deactivates slider when entering/exiting Drawing Mode
Diagram Walkthrough
File Walkthrough
mapbox-gl-compare.d.ts
Add mapbox-gl-compare TypeScript type definitionslib/types/mapbox-gl-compare.d.ts
settings
package.json
Add mapbox-gl-compare package dependencypackage.json
mapbox-map.tsx
Implement dual map synchronization and comparison slidercomponents/map/mapbox-map.tsx
labels
Mode toggle
rendering
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.