fix(map): preserve map view on entering drawing mode #294
Conversation
When activating the drawing mode, the map would zoom out to the globe level instead of maintaining the current zoom and position. This was caused by the `setupDrawingTools` function not restoring the map's view state after adding the MapboxDraw control. This commit fixes the issue by explicitly restoring the map's center, zoom, and pitch from a stored reference immediately after the drawing control is added. A `flyTo` with a duration of 0 is used to make the transition seamless for the user.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMap initialization now uses the existing zoom value from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as Map init
participant Ref as currentMapCenterRef
participant Mapbox as Mapbox Map
rect #eef8ff
Init->>Ref: read zoom (currentMapCenterRef.current.zoom)
Note right of Init: no longer assign initialZoom -> ref
Init->>Mapbox: setZoom(using ref.zoom), setCenter(ref.center), setPitch(ref.pitch)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
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 Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Using
flyTo({ duration: 0 })is heavier than necessary for an instantaneous camera update;jumpTois purpose-built and avoids transition semantics. - Directly destructuring
currentMapCenterRef.currentwithout a null/undefined guard risks a runtime error if the ref is unset. - The rest of the change is straightforward and targets the root cause effectively.
Summary of changes
What changed
- After adding the
MapboxDrawcontrol, the code now restores the map view by readingcenter,zoom, andpitchfromcurrentMapCenterRef.currentand callingmap.current.flyTo({ duration: 0 })to make the transition instantaneous.
Affected area
components/map/mapbox-map.tsx: Added logic to restore map view state immediately afteraddControl.
components/map/mapbox-map.tsx
Outdated
| // Restore the map's view state | ||
| const { center, zoom, pitch } = currentMapCenterRef.current | ||
| map.current.flyTo({ | ||
| center, | ||
| zoom, | ||
| pitch, | ||
| duration: 0 // Fly instantly | ||
| }) |
There was a problem hiding this comment.
Restoring the camera is the right move, but using flyTo({ duration: 0 }) still routes through the transition system and emits transition-related events. For an instantaneous, side-effect-minimized update, jumpTo is the canonical Mapbox GL API. Also, the code destructures from currentMapCenterRef.current without a guard; if that ref is ever unset, this will throw at runtime. Guarding the ref and switching to jumpTo will make this more robust and intent-clear.
Suggestion
Consider guarding the ref and using jumpTo instead of flyTo for an instantaneous restore:
// Restore the map's view state
const state = currentMapCenterRef.current
if (state) {
map.current.jumpTo({
center: state.center,
zoom: state.zoom,
pitch: state.pitch
})
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
When activating the drawing mode, the map would zoom out to the globe level instead of maintaining the current zoom and position. This was caused by the `setupDrawingTools` function not restoring the map's view state after adding the MapboxDraw control. This commit fixes the issue by explicitly restoring the map's center, zoom, and pitch from a stored reference immediately after the drawing control is added. A `flyTo` with a duration of 0 is used to make the transition seamless for the user. This is wrapped in a `setTimeout` to ensure the view restoration happens after any asynchronous initialization of the MapboxDraw control, addressing a potential timing issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
components/map/mapbox-map.tsx
Outdated
| // Restore the map's view state | ||
| const { center, zoom, pitch } = currentMapCenterRef.current | ||
| map.current.flyTo({ | ||
| center, | ||
| zoom, | ||
| pitch, | ||
| duration: 0 // Fly instantly | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick
Fully preserve camera and avoid unnecessary animation: include bearing and use jumpTo.
Include bearing in the saved/restored state and prefer jumpTo for an instant, no-anim camera update.
Apply this diff within the shown lines:
- const { center, zoom, pitch } = currentMapCenterRef.current
- map.current.flyTo({
- center,
- zoom,
- pitch,
- duration: 0 // Fly instantly
- })
+ const { center, zoom, pitch, bearing } = currentMapCenterRef.current
+ map.current.jumpTo({
+ center,
+ zoom,
+ pitch,
+ bearing,
+ })Additionally update the saved state (outside this hunk):
- Extend the ref type and initial value (Line 29):
const currentMapCenterRef = useRef<{ center: [number, number]; zoom: number; pitch: number; bearing: number }>({
center: [position?.longitude ?? 0, position?.latitude ?? 0],
zoom: 2,
pitch: 0,
bearing: 0,
});- Capture bearing too (Lines 326–331):
const center = map.current.getCenter();
const zoom = map.current.getZoom();
const pitch = map.current.getPitch();
const bearing = map.current.getBearing();
currentMapCenterRef.current = { center: [center.lng, center.lat], zoom, pitch, bearing };- Initialize map bearing from the ref (Line 414):
bearing: currentMapCenterRef.current.bearing,When activating the drawing mode, the map would zoom out to the globe level instead of maintaining the current zoom and position. This was caused by the `setupDrawingTools` function not restoring the map's view state after adding the MapboxDraw control, likely due to a timing issue where the control's initialization overrides the map's view. This commit fixes the issue by explicitly restoring the map's center, zoom, and pitch from a stored reference. The restoration is wrapped in a `setTimeout` to ensure it executes after the drawing control's initialization is complete. Direct setters (`setZoom`, `setCenter`, `setPitch`) are used for a more robust state update.
The root cause of the map view resetting upon entering drawing mode was the continuous destruction and recreation of the MapboxDraw control. This reset the control's state and triggered a map view update that was difficult to override. This commit refactors the component's architecture to address this fundamental issue. The MapboxDraw instance is now created only once when the map is initialized and is preserved for the component's entire lifecycle. Changes include: - Moving `new MapboxDraw()` to the main `useEffect` hook. - Attaching draw-related event listeners only once. - Simplifying `setupDrawingTools` to only add/remove the existing control from the map. - Retaining the view restoration logic (`setZoom`, `setCenter`) as a safeguard for when the control is added back to the map. This new approach is more efficient and robust, and it definitively solves the view reset problem.
The root cause of the map view resetting upon entering drawing mode was the continuous destruction and recreation of the MapboxDraw control. This reset the control's state and triggered a map view update that was difficult to override. This commit refactors the component's architecture to address this fundamental issue. The MapboxDraw instance is now created only once when the map is initialized and is preserved for the component's entire lifecycle. When switching modes, the control is now simply added or removed from the map. This robust solution includes: - Creating the `MapboxDraw` instance only once on map load. - Correctly managing event listeners by attaching them when the control is added and detaching them when it's removed. - A safeguard `setTimeout` to restore the map's center, zoom, and pitch, ensuring the view is preserved even if adding the control causes a momentary view reset. This new approach is more efficient and robust, and it definitively solves the view reset problem.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/map/mapbox-map.tsx(5 hunks)
🔇 Additional comments (7)
components/map/mapbox-map.tsx (7)
236-239: Good: add the existing Draw instance once the map is ready.This preserves the control instance across mode toggles and avoids re-instantiation churn.
241-244: Event wiring looks correct.Attaching draw events to update labels is appropriate and pairs with the cleanup below.
332-332: Comment-only change is fine.Keeping the instance and only removing the control matches the reuse strategy.
335-353: Solid teardown: save features, detach events, remove control, and clean labels.This prevents listener duplication and preserves drawings across mode switches.
419-428: Good: single Draw instance stored in a ref.This reduces GC pressure and avoids control re-creation bugs on toggles.
477-485: Dependencies look correct.Including handlers used inside the effect avoids stale closures.
245-254: Replace delayed multi-call camera updates with a single jumpTo (no timeout).Avoids arbitrary timeouts and reduces event churn from three separate setters. Use one atomic camera update; optionally include bearing to fully preserve state.
Apply this diff within these lines:
- // Restore the map's view state after a brief delay. - setTimeout(() => { - if (map.current) { - const { center, zoom, pitch } = currentMapCenterRef.current; - map.current.setZoom(zoom); - map.current.setPitch(pitch); - map.current.setCenter(center); - } - }, 150); + // Restore the map's view state instantly with a single camera update. + if (map.current && currentMapCenterRef.current) { + const { center, zoom, pitch } = currentMapCenterRef.current; + map.current.jumpTo({ center, zoom, pitch }); + }Additionally (outside this hunk), consider preserving bearing end-to-end:
// Line 29: extend ref type and init const currentMapCenterRef = useRef<{ center: [number, number]; zoom: number; pitch: number; bearing: number }>({ center: [position?.longitude ?? 0, position?.latitude ?? 0], zoom: 2, pitch: 0, bearing: 0, }); // Lines 290–297: capture bearing too const bearing = map.current.getBearing(); currentMapCenterRef.current = { center: [center.lng, center.lat], zoom, pitch, bearing }; // Line 381: initialize map bearing from ref bearing: currentMapCenterRef.current.bearing, // And here (above diff), include bearing: // map.current.jumpTo({ center, zoom, pitch, bearing: currentMapCenterRef.current.bearing });
components/map/mapbox-map.tsx
Outdated
| // Restore previous drawings if they exist. | ||
| if (drawingFeatures.current) { | ||
| drawRef.current.set(drawingFeatures.current); | ||
| // Update labels after restoring features. | ||
| setTimeout(updateMeasurementLabels, 100); | ||
| } | ||
| }, [updateMeasurementLabels]) | ||
| }, [updateMeasurementLabels]); |
There was a problem hiding this comment.
🧹 Nitpick
Prefer render-synchronized label refresh over setTimeout.
Use map.once('idle') to run after features are applied, eliminating timing guesses.
- if (drawingFeatures.current) {
- drawRef.current.set(drawingFeatures.current);
- // Update labels after restoring features.
- setTimeout(updateMeasurementLabels, 100);
- }
+ if (drawingFeatures.current) {
+ drawRef.current.set(drawingFeatures.current);
+ // Update labels after features render.
+ if (map.current) map.current.once('idle', updateMeasurementLabels);
+ }📝 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.
| // Restore previous drawings if they exist. | |
| if (drawingFeatures.current) { | |
| drawRef.current.set(drawingFeatures.current); | |
| // Update labels after restoring features. | |
| setTimeout(updateMeasurementLabels, 100); | |
| } | |
| }, [updateMeasurementLabels]) | |
| }, [updateMeasurementLabels]); | |
| // Restore previous drawings if they exist. | |
| if (drawingFeatures.current) { | |
| drawRef.current.set(drawingFeatures.current); | |
| // Update labels after features render. | |
| if (map.current) map.current.once('idle', updateMeasurementLabels); | |
| } | |
| }, [updateMeasurementLabels]); |
🤖 Prompt for AI Agents
In components/map/mapbox-map.tsx around lines 255 to 261, replace the
setTimeout-based label refresh with a render-synchronized approach: after
restoring features via drawRef.current.set(...), register a one-time idle
listener on the Mapbox map (e.g., map.once('idle', ...)) to call
updateMeasurementLabels so labels update after the map has finished rendering;
ensure you use the correct map instance reference (map or mapRef.current) and
remove the setTimeout call.
2 participants
User description
When activating the drawing mode, the map would zoom out to the globe level instead of maintaining the current zoom and position.
This was caused by the
setupDrawingToolsfunction not restoring the map's view state after adding the MapboxDraw control.This commit fixes the issue by explicitly restoring the map's center, zoom, and pitch from a stored reference immediately after the drawing control is added. A
flyTowith a duration of 0 is used to make the transition seamless for the user.PR Type
Bug fix
Description
Fix map view preservation when entering drawing mode
Restore center, zoom, and pitch after adding MapboxDraw control
Use instant flyTo transition for seamless user experience
Diagram Walkthrough
File Walkthrough
mapbox-map.tsx
Restore map view after drawing control setupcomponents/map/mapbox-map.tsx
flyTowith zero duration for instant transitioncurrentMapCenterRefSummary by CodeRabbit