Skip to content

feat(map): Canvas foundation overlay for high-performance markers - #97

Closed
shree1071 wants to merge 1 commit into
KathiraveluLab:devfrom
shree1071:pr-1-canvas-clean
Closed

feat(map): Canvas foundation overlay for high-performance markers#97
shree1071 wants to merge 1 commit into
KathiraveluLab:devfrom
shree1071:pr-1-canvas-clean

Conversation

@shree1071

Copy link
Copy Markdown

Sets up a reusable, scale-aware HTML5 Canvas layer injected directly into Leaflet's overlay pane. Perfectly synchronizes with Leaflet's internal coordinate system during zoom and pan animations. Includes complete cleanup logic and high-DPI scaling. This sets the foundation for rendering 10,000+ points smoothly.

@shree1071 shree1071 closed this Jul 21, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a custom Leaflet canvas overlay hook (useCanvasOverlay) and a corresponding CanvasLayer component to render high-performance canvas graphics pinned to a map, along with unit tests. The review feedback focuses on resolving coordinate projection issues during panning by passing and using topLeftPoint in layer coordinates rather than container coordinates. Additionally, it suggests ensuring proper cleanup in useEffect when the overlay pane is missing, removing redundant CSS transform assignments, and using a more compatible DOM cleanup method (removeChild instead of remove).

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +5 to +7
export function useCanvasOverlay(
onDraw: (ctx: CanvasRenderingContext2D, map: L.Map) => void,
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

To prevent the canvas drawing from drifting and snapping during pan gestures, the drawing must be done in layer coordinates relative to the canvas's top-left layer point (topLeftPoint).

By passing topLeftPoint as a third argument to the onDraw callback, we allow drawing functions to easily project geographic coordinates into canvas-relative pixel coordinates. This ensures the drawing stays perfectly pinned to the map during panning.

Suggested change
export function useCanvasOverlay(
onDraw: (ctx: CanvasRenderingContext2D, map: L.Map) => void,
) {
export function useCanvasOverlay(
onDraw: (ctx: CanvasRenderingContext2D, map: L.Map, topLeftPoint: L.Point) => void,
) {

Comment on lines +30 to +32
const pane = map.getPanes().overlayPane;
if (!pane) return;
pane.appendChild(canvas);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If pane is not found, returning early from the useEffect prevents the cleanup function from being registered. This can lead to memory leaks or stale references if the hook is unmounted after a render where the pane was temporarily unavailable.

Always return a cleanup function from useEffect to ensure robust resource management.

Suggested change
const pane = map.getPanes().overlayPane;
if (!pane) return;
pane.appendChild(canvas);
const pane = map.getPanes().overlayPane;
if (!pane) {
return () => {
canvasRef.current = null;
};
}
pane.appendChild(canvas);

Comment on lines +77 to +79
// Call the user provided draw function
onDrawRef.current(ctx, map);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Pass topLeftPoint to the onDraw callback so that drawing functions can correctly project coordinates relative to the canvas origin.

Suggested change
// Call the user provided draw function
onDrawRef.current(ctx, map);
// Call the user provided draw function
onDrawRef.current(ctx, map, topLeftPoint);

Comment on lines +6 to +21
useCanvasOverlay((ctx: CanvasRenderingContext2D, map: L.Map) => {
// A mock drawing function to verify integration
// We will draw the actual data in PR 2
// Draw at a fixed geographic coordinate (e.g. center of Alaska)
// so we can verify it stays perfectly pinned to the map during zoom and pan
const fixedLocation = L.latLng(64.2008, -149.4937);
const point = map.latLngToContainerPoint(fixedLocation);

ctx.beginPath();
ctx.arc(point.x, point.y, 50, 0, Math.PI * 2);
ctx.fillStyle = 'rgba(255, 0, 0, 0.3)';
ctx.fill();
ctx.strokeStyle = 'red';
ctx.lineWidth = 2;
ctx.stroke();
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using map.latLngToContainerPoint(fixedLocation) returns viewport-relative container coordinates. Since the canvas is positioned in the map's overlayPane (which pans with the map), drawing at static container coordinates causes the marker to drift during panning and snap back only after the pan ends.

To fix this, use map.latLngToLayerPoint(fixedLocation) and subtract the canvas's topLeftPoint to get the correct canvas-relative coordinates. This keeps the drawing perfectly pinned to the geographic location during panning.

Suggested change
useCanvasOverlay((ctx: CanvasRenderingContext2D, map: L.Map) => {
// A mock drawing function to verify integration
// We will draw the actual data in PR 2
// Draw at a fixed geographic coordinate (e.g. center of Alaska)
// so we can verify it stays perfectly pinned to the map during zoom and pan
const fixedLocation = L.latLng(64.2008, -149.4937);
const point = map.latLngToContainerPoint(fixedLocation);
ctx.beginPath();
ctx.arc(point.x, point.y, 50, 0, Math.PI * 2);
ctx.fillStyle = 'rgba(255, 0, 0, 0.3)';
ctx.fill();
ctx.strokeStyle = 'red';
ctx.lineWidth = 2;
ctx.stroke();
});
useCanvasOverlay((ctx: CanvasRenderingContext2D, map: L.Map, topLeftPoint: L.Point) => {
// A mock drawing function to verify integration
// We will draw the actual data in PR 2
// Draw at a fixed geographic coordinate (e.g. center of Alaska)
// so we can verify it stays perfectly pinned to the map during zoom and pan
const fixedLocation = L.latLng(64.2008, -149.4937);
const layerPoint = map.latLngToLayerPoint(fixedLocation);
const point = layerPoint.subtract(topLeftPoint);
ctx.beginPath();
ctx.arc(point.x, point.y, 50, 0, Math.PI * 2);
ctx.fillStyle = 'rgba(255, 0, 0, 0.3)';
ctx.fill();
ctx.strokeStyle = 'red';
ctx.lineWidth = 2;
ctx.stroke();
});

Comment on lines +69 to +71
// Disable any transitions before snapping the canvas back to scale 1
canvas.style.transition = "none";
canvas.style.transform = `translate3d(${topLeftPoint.x}px, ${topLeftPoint.y}px, 0px) scale(1)`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The manual transform assignment canvas.style.transform = ... is redundant and potentially conflicting. L.DomUtil.setPosition(canvas, topLeftPoint) already positions the canvas element using CSS transforms (or top/left fallback) and implicitly resets the scale to 1 by overwriting the previous transform. You can safely remove this redundant line.

Suggested change
// Disable any transitions before snapping the canvas back to scale 1
canvas.style.transition = "none";
canvas.style.transform = `translate3d(${topLeftPoint.x}px, ${topLeftPoint.y}px, 0px) scale(1)`;
// Disable any transitions before snapping the canvas back to scale 1
canvas.style.transition = "none";

Comment on lines +131 to +133
if (canvasRef.current) {
canvasRef.current.remove();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using Element.prototype.remove() can sometimes fail or behave inconsistently in older browsers or certain test environments (like older jsdom) if the element is not currently attached to the DOM. Using canvas.parentNode?.removeChild(canvas) is a safer, more standard approach for DOM cleanup in Leaflet overlays.

Suggested change
if (canvasRef.current) {
canvasRef.current.remove();
}
if (canvasRef.current) {
if (canvasRef.current.parentNode) {
canvasRef.current.parentNode.removeChild(canvasRef.current);
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant