ronantakizawa/codereview-bench huggingface dataset:
Repository: excalidraw/excalidraw
File: src/utils.ts (TypeScript)

Code before:
    selection.removeAllRanges();
  }
};

export const distance = (x: number, y: number) => Math.abs(x - y);

export const updateActiveTool = (
  appState: Pick<AppState, "activeTool">,
  data: (
    | { type: typeof SHAPES[number]["value"] | "eraser" }
    | { type: "custom"; customType: string }
  ) & { lastActiveToolBeforeEraser?: LastActiveToolBeforeEraser },
): AppState["activeTool"] => {
  if (data.type === "custom") {
    return {
      ...appState.activeTool,
      type: "custom",
      customType: data.customType,
    };
  }

  return {
    ...appState.activeTool,
    lastActiveToolBeforeEraser:
      data.lastActiveToolBeforeEraser ||
      appState.activeTool.lastActiveToolBeforeEraser,
    type: data.type,
    customType: null,
  };
};

export const resetCursor = (canvas: HTMLCanvasElement | null) => {
  if (canvas) {
    canvas.style.cursor = "";
  }
};

export const setCursor = (canvas: HTMLCanvasElement | null, cursor: string) => {
  if (canvas) {
    canvas.style.cursor = cursor;
  }
};

let eraserCanvasCache: any;
let previewDataURL: string;
export const setEraserCursor = (
  canvas: HTMLCanvasElement | null,
  theme: AppState["theme"],
) => {
  const cursorImageSizePx = 20;


Review comment (suggestion):
this will not work when setting `lastActiveToolBeforeEraser` to `null`

Code after:
    selection.removeAllRanges();
  }
};

export const distance = (x: number, y: number) => Math.abs(x - y);

export const updateActiveTool = (
  appState: Pick<AppState, "activeTool">,
  data: (
    | { type: typeof SHAPES[number]["value"] | "eraser" }
    | { type: "custom"; customType: string }
  ) & { lastActiveToolBeforeEraser?: LastActiveToolBeforeEraser },
): AppState["activeTool"] => {
  if (data.type === "custom") {
    return {
      ...appState.activeTool,
      type: "custom",
      customType: data.customType,
    };
  }

  return {
    ...appState.activeTool,
    lastActiveToolBeforeEraser:
      data.lastActiveToolBeforeEraser === undefined
        ? appState.activeTool.lastActiveToolBeforeEraser
        : data.lastActiveToolBeforeEraser,
    type: data.type,
    customType: null,
  };
};

export const resetCursor = (canvas: HTMLCanvasElement | null) => {
  if (canvas) {
    canvas.style.cursor = "";
  }
};

export const setCursor = (canvas: HTMLCanvasElement | null, cursor: string) => {
  if (canvas) {
    canvas.style.cursor = cursor;
  }
};

let eraserCanvasCache: any;
let previewDataURL: string;
export const setEraserCursor = (
  canvas: HTMLCanvasElement | null,
  theme: AppState["theme"],
) => {
  const cursorImageSizePx = 20;
Repository: excalidraw/excalidraw
File: src/components/Toast.tsx (TypeScript)

Code before:
import { useCallback, useEffect, useRef } from "react";
import { TOAST_TIMEOUT } from "../constants";
import { close } from "./icons";
import "./Toast.scss";
import { ToolButton } from "./ToolButton";

export const Toast = ({
  message,
  clearToast,
  closable = true,
  // pass null to prevent auto dismiss
  duration,
}: {
  message: string;
  clearToast: () => void;
  closable?: boolean;
  duration?: number | null;
}) => {
  const timerRef = useRef<number>(0);
  const toastDuration = duration === undefined ? TOAST_TIMEOUT : duration;
  const scheduleTimeout = useCallback(() => {
    if (!toastDuration) {
      return;
    }
    timerRef.current = window.setTimeout(() => clearToast(), toastDuration);
  }, [clearToast, toastDuration]);

  useEffect(() => {
    if (!toastDuration) {
      return;
    }
    scheduleTimeout();
    return () => clearTimeout(timerRef.current);
  }, [scheduleTimeout, message, toastDuration]);

  const onMouseEnter = toastDuration
    ? () => clearTimeout(timerRef?.current)

Review comment (suggestion):
While we can support `duration`, it's not needed right now. We should have a `autohide=true` prop. If `false`, display the `X` button and don't autohide.

Code after:
import { useCallback, useEffect, useRef } from "react";
import { close } from "./icons";
import "./Toast.scss";
import { ToolButton } from "./ToolButton";

const DEFAULT_TOAST_TIMEOUT = 5000;

export const Toast = ({
  message,
  clearToast,
  closable = false,
  // To prevent autoclose, pass duration as Infinity
  duration = DEFAULT_TOAST_TIMEOUT,
}: {
  message: string;
  clearToast: () => void;
  closable?: boolean;
  duration?: number;
}) => {
  const timerRef = useRef<number>(0);
  const shouldAutoClose = duration !== Infinity;
  const scheduleTimeout = useCallback(() => {
    if (!shouldAutoClose) {
      return;
    }
    timerRef.current = window.setTimeout(() => clearToast(), duration);
  }, [clearToast, duration, shouldAutoClose]);

  useEffect(() => {
    if (!shouldAutoClose) {
      return;
    }
    scheduleTimeout();
    return () => clearTimeout(timerRef.current);
  }, [scheduleTimeout, message, duration, shouldAutoClose]);

  const onMouseEnter = shouldAutoClose
Repository: excalidraw/excalidraw
File: src/components/App.tsx (TypeScript)

Code before:
      return {
        penMode: !prevState.penMode,
      };
    });
  };

  toggleZenMode = () => {
    this.actionManager.executeAction(actionToggleZenMode);
  };

  scrollToContent = (
    target:
      | ExcalidrawElement
      | readonly ExcalidrawElement[] = this.scene.getNonDeletedElements(),
  ) => {
    this.setState({
      ...calculateScrollCenter(
        Array.isArray(target) ? target : [target],
        this.state,
        this.canvas,
      ),
    });
  };

  clearToast = () => {
    this.setState({ toast: { message: null } });
  };

  setToast = (message: string, closable?: false, duration?: number) => {
    this.setState({ toast: { message, closable, duration } });
  };

  restoreFileFromShare = async () => {
    try {
      const webShareTargetCache = await caches.open("web-share-target");

      const response = await webShareTargetCache.match("shared-file");
      if (response) {
        const blob = await response.blob();
        const file = new File([blob], blob.name || "", { type: blob.type });
        this.loadFileToCanvas(file, null);
        await webShareTargetCache.delete("shared-file");
        window.history.replaceState(null, APP_NAME, window.location.pathname);
      }
    } catch (error: any) {
      this.setState({ errorMessage: error.message });
    }
  };

  /** adds supplied files to existing files in the appState */
  public addFiles: ExcalidrawImperativeAPI["addFiles"] = withBatchedUpdates(

Review comment (suggestion):
should clear `duration` & `closable` just so that the state is consistent. For that matter, I'd make the type into a discriminated union in the src/types.ts`, something like:

```
toast:
  | { message: string; closable?: boolean; duration?: number }
  | { message: null; closable: undefined; duration: undefined };
```

This will guard against forgetting to reset as above.


Code after:
    }
    this.setState((prevState) => {
      return {
        activeTool: {
          ...prevState.activeTool,
          ...updateActiveTool(
            this.state,
            prevState.activeTool.locked
              ? { type: "selection" }
              : prevState.activeTool,
          ),
          locked: !prevState.activeTool.locked,
        },
      };
    });
  };

  togglePenMode = () => {
    this.setState((prevState) => {
      return {
        penMode: !prevState.penMode,
      };
    });
  };

  toggleZenMode = () => {
    this.actionManager.executeAction(actionToggleZenMode);
  };

  scrollToContent = (
    target:
      | ExcalidrawElement
      | readonly ExcalidrawElement[] = this.scene.getNonDeletedElements(),
  ) => {
    this.setState({
      ...calculateScrollCenter(
        Array.isArray(target) ? target : [target],
        this.state,
        this.canvas,
      ),
    });
  };

  setToast = (
    toast: {
      message: string;
      closable?: boolean;
      duration?: number;
    } | null,
  ) => {
    this.setState({ toast });
Repository: excalidraw/excalidraw
File: src/components/App.tsx (TypeScript)

Code before:
    scenePointerX: number,
    scenePointerY: number,
  ) {
    const element = LinearElementEditor.getElement(
      linearElementEditor.elementId,
    );

    if (!element) {
      return;
    }
    if (this.state.selectedLinearElement) {
      let hoverPointIndex = -1;
      let midPointHovered = false;
      if (
        isHittingElementNotConsideringBoundingBox(element, this.state, [
          scenePointerX,
          scenePointerY,
        ])
      ) {
        hoverPointIndex = LinearElementEditor.getPointIndexUnderCursor(
          element,
          this.state.zoom,
          scenePointerX,
          scenePointerY,
        );
        midPointHovered = !!LinearElementEditor.isHittingMidPoint(
          linearElementEditor,
          { x: scenePointerX, y: scenePointerY },
          this.state,
        );

        if (hoverPointIndex >= 0 || midPointHovered) {
          setCursor(this.canvas, CURSOR_TYPE.POINTER);
        } else {
          setCursor(this.canvas, CURSOR_TYPE.MOVE);
        }
      } else if (
        shouldShowBoundingBox([element]) &&
        isHittingElementBoundingBoxWithoutHittingElement(
          element,
          this.state,
          scenePointerX,
          scenePointerY,
        )
      ) {
        setCursor(this.canvas, CURSOR_TYPE.MOVE);
      }

      if (
        this.state.selectedLinearElement.hoverPointIndex !== hoverPointIndex
      ) {

Review comment (suggestion):
```suggestion
        midPointHovered = LinearElementEditor.isHittingMidPoint(
```

let's make this always return a `boolean` instead

Code after:
    linearElementEditor: LinearElementEditor,
    scenePointerX: number,
    scenePointerY: number,
  ) {
    const element = LinearElementEditor.getElement(
      linearElementEditor.elementId,
    );

    if (!element) {
      return;
    }
    if (this.state.selectedLinearElement) {
      let hoverPointIndex = -1;
      let midPointHovered = false;
      if (
        isHittingElementNotConsideringBoundingBox(element, this.state, [
          scenePointerX,
          scenePointerY,
        ])
      ) {
        hoverPointIndex = LinearElementEditor.getPointIndexUnderCursor(
          element,
          this.state.zoom,
          scenePointerX,
          scenePointerY,
        );
        midPointHovered = LinearElementEditor.isHittingMidPoint(
          linearElementEditor,
          { x: scenePointerX, y: scenePointerY },
          this.state,
        );

        if (hoverPointIndex >= 0 || midPointHovered) {
          setCursor(this.canvas, CURSOR_TYPE.POINTER);
        } else {
          setCursor(this.canvas, CURSOR_TYPE.MOVE);
        }
      } else if (
        shouldShowBoundingBox([element]) &&
        isHittingElementBoundingBoxWithoutHittingElement(
          element,
          this.state,
          scenePointerX,
          scenePointerY,
        )
      ) {
        setCursor(this.canvas, CURSOR_TYPE.MOVE);
      }

      if (
        this.state.selectedLinearElement.hoverPointIndex !== hoverPointIndex
Repository: excalidraw/excalidraw
File: src/types.ts (TypeScript)

Code before:
  name?: string;
  renderCustomStats?: (
    elements: readonly NonDeletedExcalidrawElement[],
    appState: AppState,
  ) => JSX.Element;
  UIOptions?: {
    dockedSidebarBreakpoint?: number;
    canvasActions?: CanvasActions;
  };
  detectScroll?: boolean;
  handleKeyboardGlobally?: boolean;
  onLibraryChange?: (libraryItems: LibraryItems) => void | Promise<any>;
  autoFocus?: boolean;
  generateIdForFile?: (file: File) => string | Promise<string>;
  onLinkOpen?: (
    element: NonDeletedExcalidrawElement,
    event: CustomEvent<{
      nativeEvent: MouseEvent | React.PointerEvent<HTMLCanvasElement>;
    }>,
  ) => void;
  onPointerDown?: (
    activeTool: AppState["activeTool"],
    pointerDownState: PointerDownState,
  ) => void;
  onScrollChange?: (scrollX: number, scrollY: number) => void;
  defaultTheme?: Theme;
}

export type SceneData = {
  elements?: ImportedDataState["elements"];
  appState?: ImportedDataState["appState"];
  collaborators?: Map<string, Collaborator>;
  commitToHistory?: boolean;
};

export enum UserIdleState {
  ACTIVE = "active",
  AWAY = "away",
  IDLE = "idle",
}

export type ExportOpts = {
  saveFileToDisk?: boolean;
  onExportToBackend?: (
    exportedElements: readonly NonDeletedExcalidrawElement[],
    appState: AppState,
    files: BinaryFiles,
    canvas: HTMLCanvasElement | null,
  ) => void;
  renderCustomUI?: (
    exportedElements: readonly NonDeletedExcalidrawElement[],

Review comment (suggestion):
we should use `THEME.LIGHT` instead as the default theme instead of providing another prop. Also lets make a constant `DEFAULT_APP_THEME` with value `THEME.LIGHT`

Code after:
  name?: string;
  renderCustomStats?: (
    elements: readonly NonDeletedExcalidrawElement[],
    appState: AppState,
  ) => JSX.Element;
  UIOptions?: {
    dockedSidebarBreakpoint?: number;
    canvasActions?: CanvasActions;
  };
  detectScroll?: boolean;
  handleKeyboardGlobally?: boolean;
  onLibraryChange?: (libraryItems: LibraryItems) => void | Promise<any>;
  autoFocus?: boolean;
  generateIdForFile?: (file: File) => string | Promise<string>;
  onLinkOpen?: (
    element: NonDeletedExcalidrawElement,
    event: CustomEvent<{
      nativeEvent: MouseEvent | React.PointerEvent<HTMLCanvasElement>;
    }>,
  ) => void;
  onPointerDown?: (
    activeTool: AppState["activeTool"],
    pointerDownState: PointerDownState,
  ) => void;
  onScrollChange?: (scrollX: number, scrollY: number) => void;
}

export type SceneData = {
  elements?: ImportedDataState["elements"];
  appState?: ImportedDataState["appState"];
  collaborators?: Map<string, Collaborator>;
  commitToHistory?: boolean;
};

export enum UserIdleState {
  ACTIVE = "active",
  AWAY = "away",
  IDLE = "idle",
}

export type ExportOpts = {
  saveFileToDisk?: boolean;
  onExportToBackend?: (
    exportedElements: readonly NonDeletedExcalidrawElement[],
    appState: AppState,
    files: BinaryFiles,
    canvas: HTMLCanvasElement | null,
  ) => void;
  renderCustomUI?: (
    exportedElements: readonly NonDeletedExcalidrawElement[],
    appState: AppState,
Repository: excalidraw/excalidraw
File: src/scene/Scene.ts (TypeScript)

Code before:
    this.callbacks.add(cb);

    return () => {
      if (!this.callbacks.has(cb)) {
        throw new Error();
      }
      this.callbacks.delete(cb);
    };
  }

  destroy() {
    Scene.sceneMapById.forEach((scene, elementKey) => {
      if (scene === this) {
        Scene.sceneMapById.delete(elementKey);
      }
    });
    // done not for memory leaks, but to guard against possible late fires
    // (I guess?)
    this.callbacks.clear();
  }

  insertElementAtIndex(element: ExcalidrawElement, index: number) {
    const nextElements = [
      ...this.elements.slice(0, index),
      element,
      ...this.elements.slice(index),
    ];
    this.replaceAllElements(nextElements);
  }

  getElementIndex(elementId: string) {
    return this.elements.findIndex((element) => element.id === elementId);
  }
}

export default Scene;


Review comment (bug):
btw, this now means that if ever call this function as you did before the change, passing a result directly from `findIndex`, it will not work. So I'd add a check for `index > -1` and probably throw (so we can discover the bug instead of getting an undefined behavior).

Code after:
    this.callbacks.add(cb);

    return () => {
      if (!this.callbacks.has(cb)) {
        throw new Error();
      }
      this.callbacks.delete(cb);
    };
  }

  destroy() {
    Scene.sceneMapById.forEach((scene, elementKey) => {
      if (scene === this) {
        Scene.sceneMapById.delete(elementKey);
      }
    });
    // done not for memory leaks, but to guard against possible late fires
    // (I guess?)
    this.callbacks.clear();
  }

  insertElementAtIndex(element: ExcalidrawElement, index: number) {
    if (!Number.isFinite(index) || index < 0) {
      throw new Error(
        "insertElementAtIndex can only be called with index >= 0",
      );
    }
    const nextElements = [
      ...this.elements.slice(0, index),
      element,
      ...this.elements.slice(index),
    ];
    this.replaceAllElements(nextElements);
  }

  getElementIndex(elementId: string) {
    return this.elements.findIndex((element) => element.id === elementId);
  }
}

export default Scene;
Repository: excalidraw/excalidraw
File: src/scene/Scene.ts (TypeScript)

Code before:
      throw new Error();
    }

    this.callbacks.add(cb);

    return () => {
      if (!this.callbacks.has(cb)) {
        throw new Error();
      }
      this.callbacks.delete(cb);
    };
  }

  destroy() {
    Scene.sceneMapById.forEach((scene, elementKey) => {
      if (scene === this) {
        Scene.sceneMapById.delete(elementKey);
      }
    });
    // done not for memory leaks, but to guard against possible late fires
    // (I guess?)
    this.callbacks.clear();
  }

  insertElementAtIndex(element: ExcalidrawElement, index: number) {
    if (Number.isFinite(index) && index < 0) {
      throw new Error(
        "insertElementAtIndex can only be called with index >= 0",
      );
    }
    const nextElements = [
      ...this.elements.slice(0, index),
      element,
      ...this.elements.slice(index),
    ];
    this.replaceAllElements(nextElements);
  }

  getElementIndex(elementId: string) {
    return this.elements.findIndex((element) => element.id === elementId);
  }
}

export default Scene;


Review comment (suggestion):
```suggestion
    if (!Number.isFinite(index) || index < 0) {
```

Code after:
      throw new Error();
    }

    this.callbacks.add(cb);

    return () => {
      if (!this.callbacks.has(cb)) {
        throw new Error();
      }
      this.callbacks.delete(cb);
    };
  }

  destroy() {
    Scene.sceneMapById.forEach((scene, elementKey) => {
      if (scene === this) {
        Scene.sceneMapById.delete(elementKey);
      }
    });
    // done not for memory leaks, but to guard against possible late fires
    // (I guess?)
    this.callbacks.clear();
  }

  insertElementAtIndex(element: ExcalidrawElement, index: number) {
    if (!Number.isFinite(index) || index < 0) {
      throw new Error(
        "insertElementAtIndex can only be called with index >= 0",
      );
    }
    const nextElements = [
      ...this.elements.slice(0, index),
      element,
      ...this.elements.slice(index),
    ];
    this.replaceAllElements(nextElements);
  }

  getElementIndex(elementId: string) {
    return this.elements.findIndex((element) => element.id === elementId);
  }
}

export default Scene;
Repository: excalidraw/excalidraw
File: src/components/dropdownMenu/DropdownMenuItem.tsx (TypeScript)

Code before:

const DropdownMenuItem = ({
  icon,
  onSelect,
  children,
  shortcut,
  className,
  "aria-label": ariaLabel,

  ...rest
}: {
  icon?: JSX.Element;
  onSelect: () => void;
  children: React.ReactNode;
  shortcut?: string;
  className?: string;
  "aria-label"?: string;
} & React.ButtonHTMLAttributes<HTMLButtonElement>) => {
  return (
    <button
      {...rest}
      onClick={onSelect}
      type="button"
      className={getDrodownMenuItemClassName(className)}
      aria-label={ariaLabel}
      title={ariaLabel}
    >
      <MenuItemContent icon={icon} shortcut={shortcut}>
        {children}
      </MenuItemContent>
    </button>
  );
};

export default DropdownMenuItem;
DropdownMenuItem.displayName = "DropdownMenuItem";


Review comment (suggestion):
```suggestion
      title={rest.title ?? rest['aria-label']}
```

either remove or the above

Code after:

const DropdownMenuItem = ({
  icon,
  onSelect,
  children,
  shortcut,
  className,
  ...rest
}: {
  icon?: JSX.Element;
  onSelect: () => void;
  children: React.ReactNode;
  shortcut?: string;
  className?: string;
} & React.ButtonHTMLAttributes<HTMLButtonElement>) => {
  return (
    <button
      {...rest}
      onClick={onSelect}
      type="button"
      className={getDrodownMenuItemClassName(className)}
      title={rest.title ?? rest["aria-label"]}
    >
      <MenuItemContent icon={icon} shortcut={shortcut}>
        {children}
      </MenuItemContent>
    </button>
  );
};

export default DropdownMenuItem;
DropdownMenuItem.displayName = "DropdownMenuItem";
Repository: excalidraw/excalidraw
File: src/components/LayerUI.tsx (TypeScript)

Code before:
  showExitZenModeBtn,
  renderTopRightUI,
  renderCustomStats,
  renderCustomSidebar,
  libraryReturnUrl,
  UIOptions,
  focusContainer,
  library,
  id,
  onImageAction,
  renderWelcomeScreen,
  children,
}: LayerUIProps) => {
  const device = useDevice();

  const tunnels = useMemo(() => {
    return {
      mainMenuTunnel: tunnel(),
      welcomeScreenMenuHintTunnel: tunnel(),
      welcomeScreenToolbarHintTunnel: tunnel(),
      welcomeScreenHelpHintTunnel: tunnel(),
      welcomeScreenCenterTunnel: tunnel(),
      footerCenterTunnel: tunnel(),
      jotaiScope: Symbol(),
    };
  }, []);

  const renderJSONExportDialog = () => {
    if (!UIOptions.canvasActions.export) {
      return null;
    }

    return (
      <JSONExportDialog
        elements={elements}
        appState={appState}
        files={files}
        actionManager={actionManager}
        exportOpts={UIOptions.canvasActions.export}
        canvas={canvas}
        setAppState={setAppState}
      />
    );
  };

  const renderImageExportDialog = () => {
    if (!UIOptions.canvasActions.saveAsImage) {
      return null;
    }

    const createExporter =

Review comment (suggestion):
I've put it in `LayerUI`. We can surely factor it out into a hook, but it should live in the UI, not the App since we're trying to decouple the editor from UI.

Code after:
  children,
}: LayerUIProps) => {
  const device = useDevice();

  const tunnels = useInitializeTunnels();

  const renderJSONExportDialog = () => {
    if (!UIOptions.canvasActions.export) {
      return null;
    }

    return (
      <JSONExportDialog
        elements={elements}
        appState={appState}
        files={files}
        actionManager={actionManager}
        exportOpts={UIOptions.canvasActions.export}
        canvas={canvas}
        setAppState={setAppState}
      />
    );
  };

  const renderImageExportDialog = () => {
    if (!UIOptions.canvasActions.saveAsImage) {
      return null;
    }

    const createExporter =
      (type: ExportType): ExportCB =>
      async (exportedElements) => {
        trackEvent("export", type, "ui");
        const fileHandle = await exportCanvas(
          type,
          exportedElements,
          appState,
          files,
          {
            exportBackground: appState.exportBackground,
            name: appState.name,
            viewBackgroundColor: appState.viewBackgroundColor,
          },
        )
          .catch(muteFSAbortError)
          .catch((error) => {
            console.error(error);
            setAppState({ errorMessage: error.message });
          });

        if (