Make zoom icons conditional on drawing mode #455
Conversation
This change ensures that the Mapbox navigation controls (zoom in, zoom out, and compass) are only visible when the map is in 'Draw & Measure' mode, aligning their visibility with the drawing tools. - Added `navControlRef` to manage the `NavigationControl` instance. - Modified map initialization and `mapType` change handler to add/remove the control based on the current mode. - Updated E2E tests to switch to drawing mode before verifying zoom controls. - Improved cleanup logic to ensure controls are removed on unmount. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughThe PR moves NavigationControl initialization/removal into DrawingMode lifecycle inside Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant Mapbox as MapboxComponent
participant Map as MapInstance
participant Draw as DrawControl
participant Nav as NavigationControl
User->>Mapbox: click map toggle -> select DrawingMode
Mapbox->>Mapbox: set mapTypeRef = DrawingMode
Mapbox->>Draw: initialize & add to Map
Mapbox->>Nav: create NavigationControl (store in navControlRef)
Mapbox->>Map: addControl(Nav, "top-left")
Note over Mapbox,Map: User finishes drawing / switches mode
User->>Mapbox: switch away from DrawingMode
Mapbox->>Draw: remove DrawControl (cleanup)
Mapbox->>Nav: try remove NavigationControl (safe try-catch)
Mapbox->>Mapbox: clear navControlRef, clear timers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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:
|
||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/map.spec.ts (1)
38-49: Consider adding a wait for the navigation control to appear after switching to drawing mode.After switching to drawing mode (lines 39-40), the test immediately proceeds to check the map instance and interact with zoom controls. Since the
NavigationControlis added asynchronously when mode changes, there could be a race condition where the control hasn't been added to the DOM yet.♻️ Suggested fix
// Switch to drawing mode to make zoom controls visible await page.click('[data-testid="map-toggle"]'); await page.click('[data-testid="map-mode-draw"]'); + + // Wait for zoom controls to appear after mode switch + await page.waitForSelector('.mapboxgl-ctrl-zoom-in'); const hasMap = await page.evaluate(() => Boolean((window as any).map));components/map/mapbox-map.tsx (1)
609-668: Remove duplicate useEffect with empty body.This
useEffecthas an empty body (line 611 only contains a placeholder comment) but includes a full cleanup function that duplicates the cleanup logic from the earlieruseEffectat lines 509-548. Both effects have identical dependency arrays, which means:
- The effect body does nothing useful
- Cleanup runs twice on unmount, attempting to remove already-removed controls
- This appears to be a copy-paste artifact or incomplete refactoring
While the try-catch blocks prevent crashes, this is redundant code 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') - } - } - - if (navControlRef.current) { - try { - map.current.removeControl(navControlRef.current) - navControlRef.current = null - } catch (e) { - console.log('Navigation 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 function around line 537:stopRotation() setIsMapLoaded(false) // Reset map loaded state on cleanup setMap(null) // Clear map instance from context map.current.remove() map.current = null } + + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current); + longPressTimerRef.current = null; + } if (geolocationWatchIdRef.current !== null) {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/map/mapbox-map.tsxtests/map.spec.ts
🔇 Additional comments (4)
components/map/mapbox-map.tsx (4)
23-23: LGTM!Good addition of
navControlRefto track theNavigationControlinstance. This enables proper lifecycle management for adding/removing the control based on map mode.
371-386: LGTM!The mode-based lifecycle management for
NavigationControlis well-implemented:
- Guard condition prevents duplicate controls
- Try-catch handles edge cases where control might already be removed
- Ref is properly nulled after removal
456-459: LGTM!Conditional initialization correctly adds
NavigationControlonly when the initial mode isDrawingMode, consistent with the mode-change handler.
523-531: LGTM!Cleanup logic correctly removes the
NavigationControland nulls the ref with appropriate error handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
The navigation control lifecycle is now spread across init, map-type changes, and multiple cleanup blocks, with broad try/catch masking genuine ordering bugs; this should be consolidated and guarded on map.current. The removal/addition logic currently assumes the map instance is always present when toggling modes, which can break during initialization/teardown. There’s also a likely policy mismatch: removing the window.innerWidth > 768 condition may unintentionally enable zoom controls on mobile in drawing mode while the E2E suite still assumes mobile should never show them. The E2E test should explicitly wait for controls to render to reduce flakiness.
Additional notes (1)
- Maintainability |
tests/map.spec.ts:38-44
The test switches modes and then immediately clicks.mapboxgl-ctrl-zoom-in. This may be flaky if the control is added asynchronously relative to the UI click handler / map style updates. Consider asserting the control is visible before interacting to reduce timing-related failures.
Summary of changes
What changed
components/map/mapbox-map.tsx
- Introduced a persistent
navControlRef(useRef<mapboxgl.NavigationControl | null>(null)) to manage MapboxNavigationControllifecycle. - Updated map mode switching logic:
- When
mapType === MapToggleEnum.DrawingMode, the map now addsNavigationControl(zoom/compass) in'top-left'. - When leaving drawing mode, it removes the
NavigationControl(wrapped intry/catch) and nulls the ref.
- When
- Updated initial map creation to add
NavigationControlonly in drawing mode (replacing the previouswindow.innerWidth > 768condition). - Expanded cleanup/unmount logic to remove the
NavigationControlif present.
tests/map.spec.ts
- Updated the zoom controls E2E test to switch into “Draw & Measure” mode via
[data-testid="map-toggle"]→[data-testid="map-mode-draw"]before interacting with.mapboxgl-ctrl-zoom-in.
components/map/mapbox-map.tsx
Outdated
| if (mapType === MapToggleEnum.DrawingMode) { | ||
| navControlRef.current = new mapboxgl.NavigationControl(); | ||
| map.current.addControl(navControlRef.current, 'top-left'); | ||
| } |
There was a problem hiding this comment.
The initial condition for showing zoom controls changed from window.innerWidth > 768 (desktop-only) to “drawing mode only”. If drawing mode is available on mobile/tablet, this will now show zoom controls there too, which contradicts the E2E test’s assumption that zoom controls are not visible on mobile. Either the UI policy should remain “desktop + drawing mode”, or tests should be updated to reflect the new intended behavior across breakpoints.
Suggestion
If the intended product rule is both "desktop" and "drawing mode", restore the breakpoint check:
if (mapType === MapToggleEnum.DrawingMode && window.innerWidth > 768) {
navControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navControlRef.current, 'top-left');
}Alternatively, if drawing mode should show zoom controls even on mobile, adjust the E2E test skip logic accordingly. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
|
@coderabbitai the resolution search stopped working. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 24003 🏁 Script executed: Length of output: 19881 🏁 Script executed: Length of output: 11446
To help diagnose the issue, could you provide more details about what specifically isn't working:
The PR changes only affect when the zoom controls (NavigationControl) are visible - they should now only appear in Draw & Measure mode. However, this shouldn't affect the map instance availability for the resolution search feature. Let me investigate potential timing issues: 🏁 Script executed: Length of output: 4646 🏁 Script executed: Length of output: 3787 🏁 Script executed: Length of output: 8180 🏁 Script executed: Length of output: 4285
Root CauseThe
Without the portal target, the resolution search button cannot render on mobile devices. Important NoteThis issue is NOT caused by this PR's changes. The PR only modified when zoom controls appear (now only in Draw & Measure mode). This appears to be a pre-existing issue. Questions to help resolve this:
SolutionTo fix the mobile resolution search, add the missing portal div to the header's mobile section: {/* Mobile menu buttons */}
<div className="flex md:hidden gap-2">
<div id="mobile-header-search-portal" /> {/* ADD THIS LINE */}
<a href="https://buy.stripe.com/3cIaEX3tRcur9EM7tbasg00" target="_blank" rel="noopener noreferrer">
<Button variant="ghost" size="icon">
<TentTree className="h-[1.2rem] w-[1.2rem]" />
</Button>
</a>
<ProfileToggle/>
</div>Would you like me to create an issue to track adding the mobile portal, or would you prefer to fix this in a follow-up PR? Also, please confirm whether the desktop version is working for you. |
I've fixed this regression. The issue was that my previous change caused the map instance to be destroyed and recreated every time the map mode was switched. This led to the loss of GeoJSON layers and other transient map state, which made the resolution search results disappear. I've now stabilized the map initialization so it only runs once on mount, and mode switches correctly update the controls without destroying the map. |
This commit fixes a regression where the Mapbox instance was being destroyed and recreated on every mode switch. This was caused by including `mapType` in the dependency array of the initialization `useEffect`, whose cleanup function removed the map. - Removed `mapType` from initialization `useEffect` dependencies. - Added `mapTypeRef` to track the current mode without re-triggering map initialization. - Stabilized callbacks used in the initialization effect. - Removed redundant cleanup `useEffect` at the end of `mapbox-map.tsx`. - Ensured resolution search results (GeoJSON layers) persist through mode switches. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@components/map/mapbox-map.tsx`:
- Around line 558-569: The dependency array intentionally omits position,
mapData.cameraState, and mapType to avoid recreating the map; add an eslint
disable comment to document this and suppress react-hooks/exhaustive-deps
warnings (e.g., place a // eslint-disable-next-line react-hooks/exhaustive-deps
with a short rationale) immediately above the useEffect that depends on
captureMapCenter, handleUserInteraction, setIsMapLoaded, setMap,
setupDrawingTools, setupGeolocationWatcher, startRotation, stopRotation, and
updateMeasurementLabels so reviewers and linters understand the intentional
omission.
- Around line 493-500: Add a defensive null-check for map.current before calling
map.current.addControl inside the MapToggleEnum.DrawingMode branch: ensure when
mapTypeRef.current === MapToggleEnum.DrawingMode and after setupDrawingTools()
you only call map.current.addControl(...) if map.current is truthy (and
handle/log the unexpected null case); update the block that assigns
navControlRef.current and calls addControl to guard on map.current to match
other defensive patterns in this file.
📜 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 (1)
components/map/map-3d.tsx (1)
props(18-94)
🔇 Additional comments (6)
components/map/mapbox-map.tsx (6)
23-23: LGTM!Good use of a ref to track the NavigationControl instance, consistent with the existing
drawRefpattern.
38-42: LGTM!Good pattern to maintain a current reference to
mapTypefor use in callbacks, avoiding stale closure issues while keeping callbacks stable.
230-240: LGTM!Correctly using
mapTypeRef.currentto access the latest mode value without requiringmapTypein the dependency array, keeping the callback stable.
304-321: LGTM!Consistent application of the
mapTypeRefpattern to keep this callback stable.
376-414: LGTM!The NavigationControl lifecycle is well-managed:
- Properly guards against duplicate controls with the null check
- Defensive try-catch for removal
- Correctly clears the ref after removal
514-540: LGTM!Thorough cleanup implementation:
- Timer cleanup prevents potential memory leaks
- Both control refs are properly cleaned up with try-catch
- Refs are explicitly nulled after removal
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Initial setup for the current mode | ||
| if (mapTypeRef.current === MapToggleEnum.DrawingMode) { | ||
| setupDrawingTools() | ||
| if (!navControlRef.current) { | ||
| navControlRef.current = new mapboxgl.NavigationControl(); | ||
| map.current.addControl(navControlRef.current, 'top-left'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a defensive null check for map.current.
While the early return at line 470 typically ensures map.current exists, adding a guard before addControl would be consistent with the defensive try-catch patterns used elsewhere in this file.
♻️ Suggested improvement
// Initial setup for the current mode
if (mapTypeRef.curr
4D1C
ent === MapToggleEnum.DrawingMode) {
setupDrawingTools()
- if (!navControlRef.current) {
+ if (!navControlRef.current && map.current) {
navControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navControlRef.current, 'top-left');
}
}📝 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.
| // Initial setup for the current mode | |
| if (mapTypeRef.current === MapToggleEnum.DrawingMode) { | |
| setupDrawingTools() | |
| if (!navControlRef.current) { | |
| navControlRef.current = new mapboxgl.NavigationControl(); | |
| map.current.addControl(navControlRef.current, 'top-left'); | |
| } | |
| } | |
| // Initial setup for the current mode | |
| if (mapTypeRef.current === MapToggleEnum.DrawingMode) { | |
| setupDrawingTools() | |
| if (!navControlRef.current && map.current) { | |
| navControlRef.current = new mapboxgl.NavigationControl(); | |
| map.current.addControl(navControlRef.current, 'top-left'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 493 - 500, Add a defensive
null-check for map.current before calling map.current.addControl inside the
MapToggleEnum.DrawingMode branch: ensure when mapTypeRef.current ===
MapToggleEnum.DrawingMode and after setupDrawingTools() you only call
map.current.addControl(...) if map.current is truthy (and handle/log the
unexpected null case); update the block that assigns navControlRef.current and
calls addControl to guard on map.current to match other defensive patterns in
this file.
| }, [ | ||
| handleUserInteraction, | ||
| startRotation, | ||
| stopRotation, | ||
| mapType, | ||
| updateMeasurementLabels, | ||
| BEA4 | setupGeolocationWatcher, | |
| captureMapCenter, | ||
| setupDrawingTools, | ||
| captureMapCenter, | ||
| handleUserInteraction, | ||
| setIsMapLoaded, | ||
| setMap | ||
| setMap, | ||
| setupDrawingTools, | ||
| setupGeolocationWatcher, | ||
| startRotation, | ||
| stopRotation, | ||
| updateMeasurementLabels | ||
| // Removed position, mapData.cameraState and mapType to avoid map recreation | ||
| ]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Intentional dependency omissions are documented - consider adding eslint disable comment.
The comment explains why position, mapData.cameraState, and mapType are omitted to prevent map recreation. This is a valid architectural decision for this component.
If the react-hooks/exhaustive-deps eslint rule flags this, consider adding an explicit disable comment with explanation:
♻️ Suggested improvement
}, [
captureMapCenter,
handleUserInteraction,
setIsMapLoaded,
setMap,
setupDrawingTools,
setupGeolocationWatcher,
startRotation,
stopRotation,
updateMeasurementLabels
- // Removed position, mapData.cameraState and mapType to avoid map recreation
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally omit position, mapData.cameraState, and mapType to prevent map recreation on mode switches
])📝 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.
| }, [ | |
| handleUserInteraction, | |
| startRotation, | |
| stopRotation, | |
| mapType, | |
| updateMeasurementLabels, | |
| setupGeolocationWatcher, | |
| captureMapCenter, | |
| setupDrawingTools, | |
| captureMapCenter, | |
| handleUserInteraction, | |
| setIsMapLoaded, | |
| setMap | |
| setMap, | |
| setupDrawingTools, | |
| setupGeolocationWatcher, | |
| startRotation, | |
| stopRotation, | |
| updateMeasurementLabels | |
| // Removed position, mapData.cameraState and mapType to avoid map recreation | |
| ]) | |
| }, [ | |
| captureMapCenter, | |
| handleUserInteraction, | |
| setIsMapLoaded, | |
| setMap, | |
| setupDrawingTools, | |
| setupGeolocationWatcher, | |
| startRotation, | |
| stopRotation, | |
| updateMeasurementLabels | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally omit position, mapData.cameraState, and mapType to prevent map recreation on mode switches | |
| ]) |
🤖 Prompt for AI Agents
In `@components/map/mapbox-map.tsx` around lines 558 - 569, The dependency array
intentionally omits position, mapData.cameraState, and mapType to avoid
recreating the map; add an eslint disable comment to document this and suppress
react-hooks/exhaustive-deps warnings (e.g., place a // eslint-disable-next-line
react-hooks/exhaustive-deps with a short rationale) immediately above the
useEffect that depends on captureMapCenter, handleUserInteraction,
setIsMapLoaded, setMap, setupDrawingTools, setupGeolocationWatcher,
startRotation, stopRotation, and updateMeasurementLabels so reviewers and
linters understand the intentional omission.
User description
The zoom icons (NavigationControl) in the Mapbox component are now conditionally rendered. They only appear when the user selects 'Draw & Measure' mode, which is when the drawing tools also appear on the right side of the map. This avoids UI clutter in other modes. E2E tests have been updated to reflect this new behavior.
PR created automatically by Jules for task 2971763392819839843 started by @ngoiyaeric
PR Type
Enhancement
Description
Zoom controls now conditionally render only in Draw & Measure mode
Added
navControlRefto manage NavigationControl lifecycleUpdated map initialization to check drawing mode before adding controls
Enhanced cleanup logic to safely remove controls on unmount and mode changes
Updated E2E tests to switch to drawing mode before verifying zoom controls
Diagram Walkthrough
flowchart LR A["Map Initialization"] --> B{"Drawing Mode?"} B -->|Yes| C["Add NavigationControl"] B -->|No| D["Skip NavigationControl"] E["Mode Change Handler"] --> F{"Switch to Drawing?"} F -->|Yes| C F -->|No| G["Remove NavigationControl"] C --> H["Zoom Controls Visible"] D --> I["Zoom Controls Hidden"] G --> IFile Walkthrough
mapbox-map.tsx
Conditional zoom controls based on drawing modecomponents/map/mapbox-map.tsx
navControlRefuseRef to track NavigationControl instanceonly in DrawingMode
mapTypechange handler to add/remove controls based on modetransitions
remove NavigationControl
visibility
map.spec.ts
Update E2E tests for conditional zoom controlstests/map.spec.ts
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.