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

Code before:
import { ExcalidrawElement } from "../../element/types";
import * as utils from "../../packages/utils";

const elementFactory = (overrides = {}): ExcalidrawElement => ({
  id: "vWrqOAfkind2qcm7LDAGZ",
  type: "rectangle",
  x: 414,
  y: 237,
  width: 214,
  height: 214,
  angle: 0,
  strokeColor: "#000000",
  backgroundColor: "#15aabf",
  fillStyle: "hachure",
  strokeWidth: 1,
  strokeStyle: "solid",
  roughness: 1,
  opacity: 100,
  groupIds: [],
  strokeSharpness: "sharp",
  seed: 1041657908,
  version: 120,
  versionNonce: 1188004276,
  isDeleted: false,
  boundElementIds: null,
  ...overrides,
});
const diagramFactory = ({ overrides = {}, elementOverrides = {} } = {}) => ({
  type: "excalidraw",

Review comment (refactor):
we can move this to `fixtures` instead as it will be useful for other tests in future as well, we can remove the overrides as well and have a static fixture for elements of diff types or override in files where used if needed

Code after:
import * as utils from "../../packages/utils";
import { diagramFactory } from "../fixtures/diagramFixture";
import * as mockedSceneExportUtils from "../../scene/export";

jest.mock("../../scene/export", () => ({
  __esmodule: true,
  ...jest.requireActual("../../scene/export"),
  exportToSvg: jest.fn(),
}));

describe("exportToCanvas", () => {
  const EXPORT_PADDING = 10;

  it("with default arguments", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
    });

    expect(canvas.width).toBe(100 + 2 * EXPORT_PADDING);
    expect(canvas.height).toBe(100 + 2 * EXPORT_PADDING);
  });

  it("when custom width and height", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: () => ({ width: 200, height: 200, scale: 1 }),
    });

    expect(canvas.width).toBe(200);
Repository: excalidraw/excalidraw
File: src/tests/packages/utils.test.ts (TypeScript)

Code before:
  width: 214,
  height: 214,
  angle: 0,
  strokeColor: "#000000",
  backgroundColor: "#15aabf",
  fillStyle: "hachure",
  strokeWidth: 1,
  strokeStyle: "solid",
  roughness: 1,
  opacity: 100,
  groupIds: [],
  strokeSharpness: "sharp",
  seed: 1041657908,
  version: 120,
  versionNonce: 1188004276,
  isDeleted: false,
  boundElementIds: null,
  ...overrides,
});
const diagramFactory = ({ overrides = {}, elementOverrides = {} } = {}) => ({
  type: "excalidraw",
  version: 2,
  source: "https://excalidraw.com",
  elements: [
    elementFactory({ type: "rectangle", ...elementOverrides }),
    elementFactory({ type: "diamond", ...elementOverrides }),
    elementFactory({ type: "ellipse", ...elementOverrides }),
  ],
  appState: {
    viewBackgroundColor: "#ffffff",
    gridSize: null,
  },
  ...overrides,
});

describe("exportToCanvas", () => {
  const EXPORT_PADDING = 10;
  test("with default arguments", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: (width, height) => ({ width, height, scale: 1 }),
    });

    expect(canvas.width).toBe(100 + 2 * EXPORT_PADDING);
    expect(canvas.height).toBe(100 + 2 * EXPORT_PADDING);
  });
  test("when custom width and height", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: () => ({ width: 200, height: 200, scale: 1 }),
    });

Review comment (suggestion):
```suggestion

describe("exportToBlob", () => {
```

Code after:
}));

describe("exportToCanvas", () => {
  const EXPORT_PADDING = 10;

  it("with default arguments", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
    });

    expect(canvas.width).toBe(100 + 2 * EXPORT_PADDING);
    expect(canvas.height).toBe(100 + 2 * EXPORT_PADDING);
  });

  it("when custom width and height", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: () => ({ width: 200, height: 200, scale: 1 }),
    });

    expect(canvas.width).toBe(200);
    expect(canvas.height).toBe(200);
  });
});

describe("exportToBlob", () => {
  describe("mime type", () => {
    afterEach(jest.restoreAllMocks);

    it("should change image/jpg to image/jpeg", async () => {
      const blob = await utils.exportToBlob({
        ...diagramFactory(),
        getDimensions: (width, height) => ({ width, height, scale: 1 }),
        mimeType: "image/jpg",
      });
      expect(blob?.type).toBe("image/jpeg");
    });

    it("should default to image/png", async () => {
      const blob = await utils.exportToBlob({
        ...diagramFactory(),
      });
      expect(blob?.type).toBe("image/png");
    });

    it("should warn when using quality with image/png", async () => {
      const consoleSpy = jest
        .spyOn(console, "warn")
        .mockImplementationOnce(() => void 0);

      await utils.exportToBlob({
Repository: excalidraw/excalidraw
File: src/tests/packages/utils.test.ts (TypeScript)

Code before:
  seed: 1041657908,
  version: 120,
  versionNonce: 1188004276,
  isDeleted: false,
  boundElementIds: null,
  ...overrides,
});
const diagramFactory = ({ overrides = {}, elementOverrides = {} } = {}) => ({
  type: "excalidraw",
  version: 2,
  source: "https://excalidraw.com",
  elements: [
    elementFactory({ type: "rectangle", ...elementOverrides }),
    elementFactory({ type: "diamond", ...elementOverrides }),
    elementFactory({ type: "ellipse", ...elementOverrides }),
  ],
  appState: {
    viewBackgroundColor: "#ffffff",
    gridSize: null,
  },
  ...overrides,
});

describe("exportToCanvas", () => {
  const EXPORT_PADDING = 10;
  test("with default arguments", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: (width, height) => ({ width, height, scale: 1 }),
    });

    expect(canvas.width).toBe(100 + 2 * EXPORT_PADDING);
    expect(canvas.height).toBe(100 + 2 * EXPORT_PADDING);
  });
  test("when custom width and height", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: () => ({ width: 200, height: 200, scale: 1 }),
    });

    expect(canvas.width).toBe(200);
    expect(canvas.height).toBe(200);
  });
});
describe("exportToBlob", () => {
  describe("mime type", () => {
    afterEach(jest.restoreAllMocks);

    it("should change image/jpg to image/jpeg", async () => {
      const blob = await utils.exportToBlob({
        ...diagramFactory(),

Review comment (suggestion):
```suggestion
  it("with default arguments", () => {
```

Code after:
  });

  it("when custom width and height", () => {
    const canvas = utils.exportToCanvas({
      ...diagramFactory({ elementOverrides: { width: 100, height: 100 } }),
      getDimensions: () => ({ width: 200, height: 200, scale: 1 }),
    });

    expect(canvas.width).toBe(200);
    expect(canvas.height).toBe(200);
  });
});

describe("exportToBlob", () => {
  describe("mime type", () => {
    afterEach(jest.restoreAllMocks);

    it("should change image/jpg to image/jpeg", async () => {
      const blob = await utils.exportToBlob({
        ...diagramFactory(),
        getDimensions: (width, height) => ({ width, height, scale: 1 }),
        mimeType: "image/jpg",
      });
      expect(blob?.type).toBe("image/jpeg");
    });

    it("should default to image/png", async () => {
      const blob = await utils.exportToBlob({
        ...diagramFactory(),
      });
      expect(blob?.type).toBe("image/png");
    });

    it("should warn when using quality with image/png", async () => {
      const consoleSpy = jest
        .spyOn(console, "warn")
        .mockImplementationOnce(() => void 0);

      await utils.exportToBlob({
        ...diagramFactory(),
        mimeType: "image/png",
        quality: 1,
      });

      expect(consoleSpy).toHaveBeenCalledWith(
        '"quality" will be ignored for "image/png" mimeType',
      );
    });
  });
});
Repository: excalidraw/excalidraw
File: src/tests/utils.test.ts (TypeScript)

Code before:
import { NonDeletedExcalidrawElement } from "../element/types";
import * as exportUtils from "../scene/export";
import * as utils from "../utils";
import { diamondFixture, ellipseFixture } from "./fixtures/elementFixture";

describe("Test isTransparent", () => {
  it("should return true when color is rgb transparent", () => {
    expect(utils.isTransparent("#ff00")).toEqual(true);
    expect(utils.isTransparent("#fff00000")).toEqual(true);
    expect(utils.isTransparent("transparent")).toEqual(true);
  });

  it("should return false when color is not transparent", () => {
    expect(utils.isTransparent("#ced4da")).toEqual(false);
  });
});

describe("exportToSvg", () => {
  const ELEMENT_HEIGHT = 100;
  const ELEMENT_WIDTH = 100;
  const ELEMENTS = [
    { ...diamondFixture, height: ELEMENT_HEIGHT, width: ELEMENT_WIDTH },
    { ...ellipseFixture, height: ELEMENT_HEIGHT, width: ELEMENT_WIDTH },
  ] as NonDeletedExcalidrawElement[];

  it("with default arguments", () => {
    const svgElement = exportUtils.exportToSvg(ELEMENTS, {

Review comment (suggestion):
lets rename this file to `export.test.ts` instead to match the original file name.

Code after:
import * as utils from "../utils";

describe("Test isTransparent", () => {
  it("should return true when color is rgb transparent", () => {
    expect(utils.isTransparent("#ff00")).toEqual(true);
    expect(utils.isTransparent("#fff00000")).toEqual(true);
    expect(utils.isTransparent("transparent")).toEqual(true);
  });

  it("should return false when color is not transparent", () => {
    expect(utils.isTransparent("#ced4da")).toEqual(false);
  });
});
Repository: excalidraw/excalidraw
File: src/components/Modal.tsx (TypeScript)

Code before:
      role="dialog"
      aria-modal="true"
      onKeyDown={handleKeydown}
      aria-labelledby={props.labelledBy}
    >
      <div className="Modal__background" onClick={props.onCloseRequest}></div>
      <div
        className="Modal__content"
        style={{ "--max-width": `${props.maxWidth}px` }}
        tabIndex={0}
      >
        {props.children}
      </div>
    </div>,
    modalRoot,
  );
};

const useBodyRoot = (theme: AppState["theme"]) => {
  const [div, setDiv] = useState<HTMLDivElement | null>(null);

  const isMobile = useIsMobile();
  const isMobileRef = useRef(isMobile);
  isMobileRef.current = isMobile;

  const excalidrawContainer: HTMLDivElement | null = useExcalidrawContainer();

  useLayoutEffect(() => {
    if (div) {
      div.classList.toggle("excalidraw--mobile", isMobile);
    }
  }, [div, isMobile]);

  useLayoutEffect(() => {
    const isDarkTheme =
      !!excalidrawContainer?.classList.contains("theme--dark") ||
      theme === "dark";
    const div = document.createElement("div");

    div.classList.add("excalidraw", "excalidraw-modal-container");
    div.classList.toggle("excalidraw--mobile", isMobileRef.current);

    if (isDarkTheme) {
      div.classList.add("theme--dark");
      div.classList.add("theme--dark-background-none");
    }
    document.body.appendChild(div);

    setDiv(div);

    return () => {

Review comment (suggestion):
```suggestion
  const excalidrawContainer = useExcalidrawContainer();
```

unnecessary

Code after:
      role="dialog"
      aria-modal="true"
      onKeyDown={handleKeydown}
      aria-labelledby={props.labelledBy}
    >
      <div className="Modal__background" onClick={props.onCloseRequest}></div>
      <div
        className="Modal__content"
        style={{ "--max-width": `${props.maxWidth}px` }}
        tabIndex={0}
      >
        {props.children}
      </div>
    </div>,
    modalRoot,
  );
};

const useBodyRoot = (theme: AppState["theme"]) => {
  const [div, setDiv] = useState<HTMLDivElement | null>(null);

  const isMobile = useIsMobile();
  const isMobileRef = useRef(isMobile);
  isMobileRef.current = isMobile;

  const excalidrawContainer = useExcalidrawContainer();

  useLayoutEffect(() => {
    if (div) {
      div.classList.toggle("excalidraw--mobile", isMobile);
    }
  }, [div, isMobile]);

  useLayoutEffect(() => {
    const isDarkTheme =
      !!excalidrawContainer?.classList.contains("theme--dark") ||
      theme === "dark";
    const div = document.createElement("div");

    div.classList.add("excalidraw", "excalidraw-modal-container");
    div.classList.toggle("excalidraw--mobile", isMobileRef.current);

    if (isDarkTheme) {
      div.classList.add("theme--dark");
      div.classList.add("theme--dark-background-none");
    }
    document.body.appendChild(div);

    setDiv(div);

    return () => {
Repository: excalidraw/excalidraw
File: src/components/Modal.tsx (TypeScript)

Code before:
        {props.children}
      </div>
    </div>,
    modalRoot,
  );
};

const useBodyRoot = (theme: AppState["theme"]) => {
  const [div, setDiv] = useState<HTMLDivElement | null>(null);

  const isMobile = useIsMobile();
  const isMobileRef = useRef(isMobile);
  isMobileRef.current = isMobile;

  const excalidrawContainer: HTMLDivElement | null = useExcalidrawContainer();

  useLayoutEffect(() => {
    if (div) {
      div.classList.toggle("excalidraw--mobile", isMobile);
    }
  }, [div, isMobile]);

  useLayoutEffect(() => {
    const isDarkTheme =
      !!excalidrawContainer?.classList.contains("theme--dark") ||
      theme === "dark";
    const div = document.createElement("div");

    div.classList.add("excalidraw", "excalidraw-modal-container");
    div.classList.toggle("excalidraw--mobile", isMobileRef.current);

    if (isDarkTheme) {
      div.classList.add("theme--dark");
      div.classList.add("theme--dark-background-none");
    }
    document.body.appendChild(div);

    setDiv(div);

    return () => {
      document.body.removeChild(div);
    };
  }, [excalidrawContainer, theme]);

  return div;
};


Review comment (suggestion):
if we don't want to use `theme` only (since it's optional), it should nevertheless take precedence since it's explicit, so let's swap the order of checks

Code after:
        {props.children}
      </div>
    </div>,
    modalRoot,
  );
};

const useBodyRoot = (theme: AppState["theme"]) => {
  const [div, setDiv] = useState<HTMLDivElement | null>(null);

  const isMobile = useIsMobile();
  const isMobileRef = useRef(isMobile);
  isMobileRef.current = isMobile;

  const excalidrawContainer = useExcalidrawContainer();

  useLayoutEffect(() => {
    if (div) {
      div.classList.toggle("excalidraw--mobile", isMobile);
    }
  }, [div, isMobile]);

  useLayoutEffect(() => {
    const isDarkTheme =
      !!excalidrawContainer?.classList.contains("theme--dark") ||
      theme === "dark";
    const div = document.createElement("div");

    div.classList.add("excalidraw", "excalidraw-modal-container");
    div.classList.toggle("excalidraw--mobile", isMobileRef.current);

    if (isDarkTheme) {
      div.classList.add("theme--dark");
      div.classList.add("theme--dark-background-none");
    }
    document.body.appendChild(div);

    setDiv(div);

    return () => {
      document.body.removeChild(div);
    };
  }, [excalidrawContainer, theme]);

  return div;
};
Repository: excalidraw/excalidraw
File: src/components/Tooltip.tsx (TypeScript)

Code before:
import "./Tooltip.scss";

import React, { useEffect } from "react";

const getTooltipDiv = () => {
  const existingDiv = document.querySelector<HTMLDivElement>(
    ".ExcalidrawTooltip",
  );
  if (existingDiv) {
    return existingDiv;
  }
  const div = document.createElement("div");
  document.body.appendChild(div);
  div.classList.add("ExcalidrawTooltip");
  return div;
};

const updateTooltip = (
  item: HTMLDivElement,
  tooltip: HTMLDivElement,
  label: string,
  long: boolean,
) => {
  tooltip.classList.add("is-shown");
  tooltip.style.minWidth = long ? "50ch" : "10ch";
  tooltip.style.maxWidth = long ? "50ch" : "15ch";

  tooltip.textContent = label;

  const {
    x: itemX,
    bottom: itemBottom,
    top: itemTop,
    width: itemWidth,
  } = item.getBoundingClientRect();

  const {
    width: labelWidth,
    height: labelHeight,

Review comment (suggestion):
```suggestion
  div.classList.add("excalidraw-tooltip");
```
So that CSS class names are consistent and we can follow BEM

Code after:
import "./Tooltip.scss";

import React, { useEffect } from "react";

const getTooltipDiv = () => {
  const existingDiv = document.querySelector<HTMLDivElement>(
    ".excalidraw-tooltip",
  );
  if (existingDiv) {
    return existingDiv;
  }
  const div = document.createElement("div");
  document.body.appendChild(div);
  div.classList.add("excalidraw-tooltip");
  return div;
};

const updateTooltip = (
  item: HTMLDivElement,
  tooltip: HTMLDivElement,
  label: string,
  long: boolean,
) => {
  tooltip.classList.add("excalidraw-tooltip--visible");
  tooltip.style.minWidth = long ? "50ch" : "10ch";
  tooltip.style.maxWidth = long ? "50ch" : "15ch";

  tooltip.textContent = label;

  const {
    x: itemX,
    bottom: itemBottom,
    top: itemTop,
    width: itemWidth,
  } = item.getBoundingClientRect();

  const {
    width: labelWidth,
    height: labelHeight,
Repository: excalidraw/excalidraw
File: src/components/Tooltip.tsx (TypeScript)

Code before:
import "./Tooltip.scss";

import React, { useEffect } from "react";

const getTooltipDiv = () => {
  const existingDiv = document.querySelector<HTMLDivElement>(
    ".ExcalidrawTooltip",
  );
  if (existingDiv) {
    return existingDiv;
  }
  const div = document.createElement("div");
  document.body.appendChild(div);
  div.classList.add("ExcalidrawTooltip");
  return div;
};

const updateTooltip = (
  item: HTMLDivElement,
  tooltip: HTMLDivElement,
  label: string,
  long: boolean,
) => {
  tooltip.classList.add("is-shown");
  tooltip.style.minWidth = long ? "50ch" : "10ch";
  tooltip.style.maxWidth = long ? "50ch" : "15ch";

  tooltip.textContent = label;

  const {
    x: itemX,
    bottom: itemBottom,
    top: itemTop,
    width: itemWidth,
  } = item.getBoundingClientRect();

  const {
    width: labelWidth,
    height: labelHeight,
  } = tooltip.getBoundingClientRect();

  const viewportWidth = window.innerWidth;
  const viewportHeight = window.innerHeight;

  const margin = 5;

  const left = itemX + itemWidth / 2 - labelWidth / 2;
  const offsetLeft =
    left + labelWidth >= viewportWidth ? left + labelWidth - viewportWidth : 0;

Review comment (suggestion):
```suggestion
  tooltip.classList.add("excalidraw-tooltip--visible");
```

Code after:
import "./Tooltip.scss";

import React, { useEffect } from "react";

const getTooltipDiv = () => {
  const existingDiv = document.querySelector<HTMLDivElement>(
    ".excalidraw-tooltip",
  );
  if (existingDiv) {
    return existingDiv;
  }
  const div = document.createElement("div");
  document.body.appendChild(div);
  div.classList.add("excalidraw-tooltip");
  return div;
};

const updateTooltip = (
  item: HTMLDivElement,
  tooltip: HTMLDivElement,
  label: string,
  long: boolean,
) => {
  tooltip.classList.add("excalidraw-tooltip--visible");
  tooltip.style.minWidth = long ? "50ch" : "10ch";
  tooltip.style.maxWidth = long ? "50ch" : "15ch";

  tooltip.textContent = label;

  const {
    x: itemX,
    bottom: itemBottom,
    top: itemTop,
    width: itemWidth,
  } = item.getBoundingClientRect();

  const {
    width: labelWidth,
    height: labelHeight,
  } = tooltip.getBoundingClientRect();

  const viewportWidth = window.innerWidth;
  const viewportHeight = window.innerHeight;

  const margin = 5;

  const left = itemX + itemWidth / 2 - labelWidth / 2;
  const offsetLeft =
    left + labelWidth >= viewportWidth ? left + labelWidth - viewportWidth : 0;
Repository: excalidraw/excalidraw
File: src/excalidraw-app/index.tsx (TypeScript)

Code before:
  };

  return (
    <>
      <Excalidraw
        ref={excalidrawRefCallback}
        onChange={onChange}
        initialData={initialStatePromiseRef.current.promise}
        onCollabButtonClick={collabAPI?.onCollabButtonClick}
        isCollaborating={collabAPI?.isCollaborating()}
        onPointerUpdate={collabAPI?.onPointerUpdate}
        UIOptions={{
          canvasActions: {
            export: {
              onExportToBackend,
            },
          },
        }}
        renderTopRightUI={renderTopRightUI}
        renderFooter={renderFooter}
        langCode={langCode}
        renderCustomStats={renderCustomStats}
        detectScroll={false}
        handleKeyboardGlobally={true}
        onLibraryChange={onLibraryChange}
        autofocus
      />
      {excalidrawAPI && <CollabWrapper excalidrawAPI={excalidrawAPI} />}
      {errorMessage && (
        <ErrorDialog
          message={errorMessage}
          onClose={() => setErrorMessage("")}
        />
      )}
    </>
  );
};

const ExcalidrawApp = () => {
  return (
    <TopErrorBoundary>
      <CollabContextConsumer>
        <ExcalidrawWrapper />
      </CollabContextConsumer>
    </TopErrorBoundary>
  );
};

export default ExcalidrawApp;


Review comment (suggestion):
```suggestion
        autofocus={true}
```
for consistency

Code after:
  };

  return (
    <>
      <Excalidraw
        ref={excalidrawRefCallback}
        onChange={onChange}
        initialData={initialStatePromiseRef.current.promise}
        onCollabButtonClick={collabAPI?.onCollabButtonClick}
        isCollaborating={collabAPI?.isCollaborating()}
        onPointerUpdate={collabAPI?.onPointerUpdate}
        UIOptions={{
          canvasActions: {
            export: {
              onExportToBackend,
              renderCustomUI: (elements, appState) => {
                return (
                  <ExportToExcalidrawPlus
                    elements={elements}
                    appState={appState}
                    onError={(error) => {
                      excalidrawAPI?.updateScene({
                        appState: {
                          errorMessage: error.message,
                        },
                      });
                    }}
                  />
                );
              },
            },
          },
        }}
        renderTopRightUI={renderTopRightUI}
        renderFooter={renderFooter}
        langCode={langCode}
        renderCustomStats={renderCustomStats}
        detectScroll={false}
        handleKeyboardGlobally={true}
        onLibraryChange={onLibraryChange}
        autoFocus={true}
      />
      {excalidrawAPI && <CollabWrapper excalidrawAPI={excalidrawAPI} />}
      {errorMessage && (
        <ErrorDialog
          message={errorMessage}
          onClose={() => setErrorMessage("")}
        />
      )}
    </>
  );