critical

The isDragging flag is used to determine the visual state of the minimap's scrollbar. Using useRef for this is incorrect because ref updates do not trigger re-renders. This causes a bug where the scrollbar remains highlighted after a drag operation is completed, as the component does not re-render to reflect the change in isDragging state.

To fix this, isDragging should be managed with useState. This will ensure the component re-renders when the dragging state changes. You will need to update the hook's return type and pass the isDragging state (not a ref) to useMinimapDraw. This change also makes the useEffect on lines 111-118 redundant and it should be removed.

high

This useEffect adds a global mouseup listener to set isDragging.current to false. This is redundant with the logic in handleWindowMouseUp, which already handles cleaning up the dragging state and its associated event listeners.

More importantly, as mentioned in another comment, isDragging should be a state variable. Once that is addressed, this useEffect becomes entirely unnecessary and should be removed to avoid confusion and redundant event listeners.

high

The dependency array for drawMinimap is incorrect. It should depend on drawScrollBox which it calls, and isDragging which determines the color. The current flattened dependency list is incomplete and hard to maintain.

Assuming isDragging is converted to a state variable as per my other comment, the dependency array should be updated to include drawScrollBox and isDragging. This will ensure drawMinimap is re-created and the minimap is redrawn when the dragging state or other drawing-related data changes.

medium

The dependency array for handleMouseMove is missing ROW_HEIGHT, which is used on line 78. Although ROW_HEIGHT is currently stable because it's memoized with an empty dependency array, it's important to follow the rules of hooks and include all dependencies. This prevents subtle bugs if ROW_HEIGHT's calculation ever changes.

medium

Minor correction: The filename in the comment is useMinimapDraw.js, but the file extension is .ts.

critical

The current implementation for handling drag events has critical flaws related to event listener management and state closures in React. Adding event listeners inside a useCallback like handleMouseDown and removing them in another can lead to memory leaks if the function references change between adding and removing. Furthermore, handleWindowMouseMove has a stale closure over the isDragging state because it's not included in its dependency array, which will prevent dragging from working at all.

A much safer and more idiomatic React approach is to use a useEffect hook to manage side effects like window event listeners. The effect should be dependent on the isDragging state.

import { useCallback, useState, useEffect } from "react";
type UseDragScrollParams = {
 height: number;
 totalLines: number;
 viewportHeight: number;
 ROW_HEIGHT: number;
 onScroll: (scrollTop: number) => void;
 containerRef: React.RefObject<HTMLDivElement>;
};
type UseDragScrollReturn = {
 handleMouseDown: (e: React.MouseEvent) => void;
 isDragging: boolean;
};
export function useDragScroll({
 height,
 totalLines,
 viewportHeight,
 ROW_HEIGHT,
 onScroll,
 containerRef,
}: UseDragScrollParams): UseDragScrollReturn {
 const [isDragging, setIsDragging] = useState(false);
 const handleBoundaryScroll = useCallback((e: MouseEvent | React.MouseEvent<HTMLDivElement>["nativeEvent"], rect: DOMRect): void => {
 const relativeY = e.clientY - rect.top;
 const maxScroll = Math.max(0, (totalLines * ROW_HEIGHT) - height);
 const cursorBottom = e.clientY + viewportHeight / 2;
 let scrollTop;
 if (cursorBottom > rect.bottom) {
 const constrainedRelativeY = rect.bottom - rect.top - viewportHeight / 2;
 scrollTop = ((constrainedRelativeY - viewportHeight / 2) / height) * totalLines * ROW_HEIGHT;
 } else {
 scrollTop = ((relativeY - viewportHeight / 2) / height) * totalLines * ROW_HEIGHT;
 }
 if (!Number.isNaN(scrollTop) && Number.isFinite(scrollTop)) {
 onScroll(Math.min(maxScroll, Math.max(0, scrollTop)));
 }
 }, [height, totalLines, viewportHeight, ROW_HEIGHT, onScroll]);
 useEffect(() => {
 const handleWindowMouseMove = (e: MouseEvent) => {
 if (!containerRef.current) return;
 const rect = containerRef.current.getBoundingClientRect();
 handleBoundaryScroll(e, rect);
 };
 const handleWindowMouseUp = () => {
 setIsDragging(false);
 };
 if (isDragging) {
 window.addEventListener("mousemove", handleWindowMouseMove);
 window.addEventListener("mouseup", handleWindowMouseUp);
 }
 return () => {
 window.removeEventListener("mousemove", handleWindowMouseMove);
 window.removeEventListener("mouseup", handleWindowMouseUp);
 };
 }, [isDragging, containerRef, handleBoundaryScroll]);
 const handleMouseDown = useCallback((e: React.MouseEvent) => {
 if (!containerRef.current) return;
 
 setIsDragging(true);
 const rect = containerRef.current.getBoundingClientRect();
 handleBoundaryScroll(e.nativeEvent, rect);
 }, [containerRef, handleBoundaryScroll]);
 return {
 handleMouseDown,
 isDragging,
 };
}

high

The current drawing logic is inefficient. drawScrollBox calls drawDifferencesInMinimap, which iterates over all diff lines. In DiffMinimap.tsx, handleMouseMove calls drawMinimap (which calls drawScrollBox) and then potentially drawScrollBox again. This results in redrawing all the diff lines multiple times on every mouse movement, causing performance issues.

The drawing of diffs should be separated from drawing the scrollbar. drawMinimap should be responsible for drawing the diffs once, and then drawing the scrollbar. drawScrollBox should only draw the scrollbar rectangle. This will also simplify the dependency array of drawMinimap.

 // Draw the scroll box
 const drawScrollBox = useCallback((ctx: CanvasRenderingContext2D, color: string) => {
 const totalContentHeight = totalLines * ROW_HEIGHT;
 const viewportTop = (currentScrollTop / totalContentHeight) * height;
 ctx.fillStyle = color;
 ctx.fillRect(0, viewportTop, miniMapWidth, viewportHeight);
 }, [totalLines, ROW_HEIGHT, currentScrollTop, height, miniMapWidth, viewportHeight]);
 const drawMinimap = useCallback(() => {
 const canvas = canvasRef.current;
 if (!canvas)
 return;
 const ctx = canvas.getContext("2d");
 if (!ctx)
 return;
 ctx.clearRect(0, 0, canvas.width, canvas.height);
 drawDifferencesInMinimap(ctx);
 if (!isDragging) {
 drawScrollBox(ctx, MINIMAP_SCROLL_COLOR);
 }
 else {
 drawScrollBox(ctx, MINIMAP_HOVER_SCROLL_COLOR);
 }
 }, [canvasRef, drawDifferencesInMinimap, isDragging, drawScrollBox]);

high

This useEffect calls drawMinimap whenever drawMinimap changes. However, the DiffMinimap component, which uses this hook, already has an identical useEffect to call drawMinimap. This results in drawMinimap being called twice for every change, which is redundant and inefficient. The responsibility of when to draw should lie with the component using the hook. This useEffect should be removed.