ronantakizawa/codereview-bench huggingface dataset:
Repository: excalidraw/excalidraw
File: src/packages/excalidraw/index.tsx (TypeScript)

Code before:
import { defaultLang } from "../../i18n";
import { DEFAULT_UI_OPTIONS } from "../../constants";

const Excalidraw = (props: ExcalidrawProps) => {
  const {
    onChange,
    initialData,
    excalidrawRef,
    onCollabButtonClick,
    isCollaborating,
    onPointerUpdate,
    renderTopRightUI,
    renderFooter,
    langCode = defaultLang.code,
    viewModeEnabled,
    zenModeEnabled,
    gridModeEnabled,
    libraryReturnUrl,
    theme,
    name,
    renderCustomStats,
    onPaste,
    detectScroll = true,
    handleKeyboardGlobally = false,
    onLibraryChange,
    autofocus,
  } = props;

  const canvasActions = props.UIOptions?.canvasActions;

  const UIOptions: AppProps["UIOptions"] = {
    canvasActions: {
      ...DEFAULT_UI_OPTIONS.canvasActions,
      ...canvasActions,
    },
  };

  if (canvasActions?.export) {
    UIOptions.canvasActions.export.saveFileToDisk =
      canvasActions.export?.saveFileToDisk ||
      DEFAULT_UI_OPTIONS.canvasActions.export.saveFileToDisk;
  }

  useEffect(() => {
    // Block pinch-zooming on iOS outside of the content area
    const handleTouchMove = (event: TouchEvent) => {
      // @ts-ignore
      if (typeof event.scale === "number" && event.scale !== 1) {
        event.preventDefault();
      }
    };

Review comment (suggestion):
```suggestion
    autofocus = false,
```
lets make it `false` by default and required prop for `App.tsx`

Code after:
import { defaultLang } from "../../i18n";
import { DEFAULT_UI_OPTIONS } from "../../constants";

const Excalidraw = (props: ExcalidrawProps) => {
  const {
    onChange,
    initialData,
    excalidrawRef,
    onCollabButtonClick,
    isCollaborating,
    onPointerUpdate,
    renderTopRightUI,
    renderFooter,
    langCode = defaultLang.code,
    viewModeEnabled,
    zenModeEnabled,
    gridModeEnabled,
    libraryReturnUrl,
    theme,
    name,
    renderCustomStats,
    onPaste,
    detectScroll = true,
    handleKeyboardGlobally = false,
    onLibraryChange,
    autoFocus = false,
  } = props;

  const canvasActions = props.UIOptions?.canvasActions;

  const UIOptions: AppProps["UIOptions"] = {
    canvasActions: {
      ...DEFAULT_UI_OPTIONS.canvasActions,
      ...canvasActions,
    },
  };

  if (canvasActions?.export) {
    UIOptions.canvasActions.export.saveFileToDisk =
      canvasActions.export?.saveFileToDisk ||
      DEFAULT_UI_OPTIONS.canvasActions.export.saveFileToDisk;
  }

  useEffect(() => {
    // Block pinch-zooming on iOS outside of the content area
    const handleTouchMove = (event: TouchEvent) => {
      // @ts-ignore
      if (typeof event.scale === "number" && event.scale !== 1) {
        event.preventDefault();
      }
    };
Repository: excalidraw/excalidraw
File: src/components/ActiveFile.tsx (TypeScript)

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

import React from "react";
import Stack from "../components/Stack";
import { ToolButton } from "../components/ToolButton";
import { save, file } from "../components/icons";
import { t } from "../i18n";

type ActiveFileProps = {
  fileName?: string;
  onClick: () => void;
};

export const ActiveFile = ({ fileName, onClick }: ActiveFileProps) => (
  <Stack.Row className="ActiveFile" gap={1} align="center">
    <span className="ActiveFile__fileName">
      {file}
      <span>{fileName}</span>
    </span>
    <ToolButton
      type="icon"
      icon={save}
      title={t("buttons.save")}
      aria-label={t("buttons.save")}
      onClick={onClick}
      data-testid="save-button"
    />
  </Stack.Row>
);


Review comment (suggestion):
lets rename this to be `onSave` instead to be more clear

Code after:
import React from "react";
import Stack from "../components/Stack";
import { ToolButton } from "../components/ToolButton";
import { save, file } from "../components/icons";
import { t } from "../i18n";

import "./ActiveFile.scss";

type ActiveFileProps = {
  fileName?: string;
  onSave: () => void;
};

export const ActiveFile = ({ fileName, onSave }: ActiveFileProps) => (
  <Stack.Row className="ActiveFile" gap={1} align="center">
    <span className="ActiveFile__fileName">
      {file}
      <span>{fileName}</span>
    </span>
    <ToolButton
      type="icon"
      icon={save}
      title={t("buttons.save")}
      aria-label={t("buttons.save")}
      onClick={onSave}
      data-testid="save-button"
    />
  </Stack.Row>
);
Repository: excalidraw/excalidraw
File: src/components/ActiveFile.tsx (TypeScript)

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

import React from "react";
import Stack from "../components/Stack";
import { ToolButton } from "../components/ToolButton";
import { save, file } from "../components/icons";
import { t } from "../i18n";

type ActiveFileProps = {
  fileName?: string;
  onClick: () => void;
};

export const ActiveFile = ({ fileName, onClick }: ActiveFileProps) => (
  <Stack.Row className="ActiveFile" gap={1} align="center">
    <span className="ActiveFile__fileName">
      {file}
      <span>{fileName}</span>
    </span>
    <ToolButton
      type="icon"
      icon={save}
      title={t("buttons.save")}
      aria-label={t("buttons.save")}
      onClick={onClick}
      data-testid="save-button"
    />
  </Stack.Row>
);


Review comment (suggestion):
```suggestion
import React from "react";
import Stack from "../components/Stack";
import { ToolButton } from "../components/ToolButton";
import { save, file } from "../components/icons";
import { t } from "../i18n";

import "./ActiveFile.scss";
```

For consistency, as we are habituated seeing the first line `import React...` :)

Code after:
import React from "react";
import Stack from "../components/Stack";
import { ToolButton } from "../components/ToolButton";
import { save, file } from "../components/icons";
import { t } from "../i18n";

import "./ActiveFile.scss";

type ActiveFileProps = {
  fileName?: string;
  onSave: () => void;
};

export const ActiveFile = ({ fileName, onSave }: ActiveFileProps) => (
  <Stack.Row className="ActiveFile" gap={1} align="center">
    <span className="ActiveFile__fileName">
      {file}
      <span>{fileName}</span>
    </span>
    <ToolButton
      type="icon"
      icon={save}
      title={t("buttons.save")}
      aria-label={t("buttons.save")}
      onClick={onSave}
      data-testid="save-button"
    />
  </Stack.Row>
);
Repository: excalidraw/excalidraw
File: src/tests/packages/utils.test.ts (TypeScript)

Code before:
  });

  it("with deleted elements", async () => {
    await utils.exportToSvg({
      ...diagramFactory({
        overrides: { appState: void 0 },
        elementOverrides: { isDeleted: true },
      }),
    });

    expect(passedElements().length).toBe(0);
  });

  it("with exportPadding", async () => {
    await utils.exportToSvg({
      ...diagramFactory({ overrides: { appState: { name: "diagram name" } } }),
      exportPadding: 0,
    });

    expect(passedElements().length).toBe(3);
    expect(passedOptions()).toEqual(
      expect.objectContaining({ exportPadding: 0 }),
    );
  });

  it("with embedScene", async () => {
    await utils.exportToSvg({
      ...diagramFactory({
        overrides: {
          appState: { name: "diagram name", exportEmbedScene: true },
        },
      }),
    });

    expect(passedElements().length).toBe(3);
    expect(passedOptions().metadata).toMatchSnapshot();
  });
});


Review comment (suggestion):
```suggestion
  it("with exportEmbedScene", async () => {
```

Code after:
  });

  it("with deleted elements", async () => {
    await utils.exportToSvg({
      ...diagramFactory({
        overrides: { appState: void 0 },
        elementOverrides: { isDeleted: true },
      }),
    });

    expect(passedElements().length).toBe(0);
  });

  it("with exportPadding", async () => {
    await utils.exportToSvg({
      ...diagramFactory({ overrides: { appState: { name: "diagram name" } } }),
      exportPadding: 0,
    });

    expect(passedElements().length).toBe(3);
    expect(passedOptions()).toEqual(
      expect.objectContaining({ exportPadding: 0 }),
    );
  });

  it("with exportEmbedScene", async () => {
    await utils.exportToSvg({
      ...diagramFactory({
        overrides: {
          appState: { name: "diagram name", exportEmbedScene: true },
        },
      }),
    });

    expect(passedElements().length).toBe(3);
    expect(passedOptions().exportEmbedScene).toBe(true);
  });
});
Repository: excalidraw/excalidraw
File: src/components/LibraryUnit.tsx (TypeScript)

Code before:
  const ref = useRef<HTMLDivElement | null>(null);
  useEffect(() => {
    const elementsToRender = elements || pendingElements;
    if (!elementsToRender) {
      return;
    }
    let svg: SVGSVGElement;
    const current = ref.current!;

    const getSvg = async () => {
      svg = await exportToSvg(elementsToRender, {
        exportBackground: false,
        viewBackgroundColor: oc.white,
      });
      for (const child of ref.current!.children) {
        if (child.tagName !== "svg") {
          continue;
        }
        ref.current!.removeChild(child);
      }
      ref.current!.appendChild(svg);
    };

    getSvg();
    return () => {
      current.removeChild(svg);
    };
  }, [elements, pendingElements]);

  const [isHovered, setIsHovered] = useState(false);
  const isMobile = useIsMobile();

  const adder = (isHovered || isMobile) && pendingElements && (
    <div className="library-unit__adder">{PLUS_ICON}</div>
  );

  return (
    <div
      className={clsx("library-unit", {
        "library-unit__active": elements || pendingElements,
      })}
      onMouseEnter={() => setIsHovered(true)}
      onMouseLeave={() => setIsHovered(false)}
    >
      <div
        className={clsx("library-unit__dragger", {
          "library-unit__pulse": !!pendingElements,
        })}
        ref={ref}
        draggable={!!elements}
        onClick={!!elements || !!pendingElements ? onClick : undefined}

Review comment (suggestion):
prob fine, but if wanted to guarantee this won't throw, we should wrap this call in in `svg` null check.

Code after:
  const ref = useRef<HTMLDivElement | null>(null);
  useEffect(() => {
    const elementsToRender = elements || pendingElements;
    if (!elementsToRender) {
      return;
    }
    let svg: SVGSVGElement;
    const current = ref.current!;

    (async () => {
      svg = await exportToSvg(elementsToRender, {
        exportBackground: false,
        viewBackgroundColor: oc.white,
      });
      for (const child of ref.current!.children) {
        if (child.tagName !== "svg") {
          continue;
        }
        current!.removeChild(child);
      }
      current!.appendChild(svg);
    })();

    return () => {
      if (svg) {
        current.removeChild(svg);
      }
    };
  }, [elements, pendingElements]);

  const [isHovered, setIsHovered] = useState(false);
  const isMobile = useIsMobile();

  const adder = (isHovered || isMobile) && pendingElements && (
    <div className="library-unit__adder">{PLUS_ICON}</div>
  );

  return (
    <div
      className={clsx("library-unit", {
        "library-unit__active": elements || pendingElements,
      })}
      onMouseEnter={() => setIsHovered(true)}
      onMouseLeave={() => setIsHovered(false)}
    >
      <div
        className={clsx("library-unit__dragger", {
          "library-unit__pulse": !!pendingElements,
        })}
        ref={ref}
        draggable={!!elements}
Repository: excalidraw/excalidraw
File: src/components/LibraryUnit.tsx (TypeScript)

Code before:
  onClick: () => void;
}) => {
  const ref = useRef<HTMLDivElement | null>(null);
  useEffect(() => {
    const elementsToRender = elements || pendingElements;
    if (!elementsToRender) {
      return;
    }
    let svg: SVGSVGElement;
    const current = ref.current!;

    const getSvg = async () => {
      svg = await exportToSvg(elementsToRender, {
        exportBackground: false,
        viewBackgroundColor: oc.white,
      });
      for (const child of ref.current!.children) {
        if (child.tagName !== "svg") {
          continue;
        }
        ref.current!.removeChild(child);
      }
      ref.current!.appendChild(svg);
    };

    getSvg();
    return () => {
      current.removeChild(svg);
    };
  }, [elements, pendingElements]);

  const [isHovered, setIsHovered] = useState(false);
  const isMobile = useIsMobile();

  const adder = (isHovered || isMobile) && pendingElements && (
    <div className="library-unit__adder">{PLUS_ICON}</div>
  );

  return (
    <div
      className={clsx("library-unit", {
        "library-unit__active": elements || pendingElements,
      })}
      onMouseEnter={() => setIsHovered(true)}
      onMouseLeave={() => setIsHovered(false)}
    >
      <div
        className={clsx("library-unit__dragger", {
          "library-unit__pulse": !!pendingElements,
        })}
        ref={ref}

Review comment (suggestion):
I'd instead either do a regular `promise.then`, or use an IIFE.

Also, instead of asserting the `ref.current` in the callback, you can reuse the `current` local variable.

Code after:
  onClick: () => void;
}) => {
  const ref = useRef<HTMLDivElement | null>(null);
  useEffect(() => {
    const elementsToRender = elements || pendingElements;
    if (!elementsToRender) {
      return;
    }
    let svg: SVGSVGElement;
    const current = ref.current!;

    (async () => {
      svg = await exportToSvg(elementsToRender, {
        exportBackground: false,
        viewBackgroundColor: oc.white,
      });
      for (const child of ref.current!.children) {
        if (child.tagName !== "svg") {
          continue;
        }
        current!.removeChild(child);
      }
      current!.appendChild(svg);
    })();

    return () => {
      if (svg) {
        current.removeChild(svg);
      }
    };
  }, [elements, pendingElements]);

  const [isHovered, setIsHovered] = useState(false);
  const isMobile = useIsMobile();

  const adder = (isHovered || isMobile) && pendingElements && (
    <div className="library-unit__adder">{PLUS_ICON}</div>
  );

  return (
    <div
      className={clsx("library-unit", {
        "library-unit__active": elements || pendingElements,
      })}
      onMouseEnter={() => setIsHovered(true)}
      onMouseLeave={() => setIsHovered(false)}
    >
      <div
        className={clsx("library-unit__dragger", {
          "library-unit__pulse": !!pendingElements,
        })}
Repository: excalidraw/excalidraw
File: scripts/updateReadme.js (JavaScript)

Code before:
const fs = require("fs");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
let data = fs.readFileSync(`${excalidrawDir}/README_NEXT.md`, "utf8");

// remove note for unstable release
data = data.replace(
  /<!-- stable-readme-start-->[\s\S]*?<!-- stable-readme-end-->/,
  "",
);

// replace "excalidraw-next" with "excalidraw"
data = data.replace(/excalidraw-next/g, "excalidraw");
data = data.trim();

const demoIndex = data.indexOf("### Demo");
const excalidrawNextNote =
  "#### Note\n\n**If you don't want to wait for the next stable release and try out the unreleased changes you can use [@excalidraw/excalidraw-next](https://www.npmjs.com/package/@excalidraw/excalidraw-next).**\n\n";
// Add excalidraw next note to try out for unreleased changes
data = data.substr(0, demoIndex) + excalidrawNextNote + data.substr(demoIndex);

// update readme
fs.writeFileSync(`${excalidrawDir}/README.md`, data, "utf8");


Review comment (suggestion):
shouldn't we have named this `unstable-readme-start`? 😀

Code after:
const fs = require("fs");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
let data = fs.readFileSync(`${excalidrawDir}/README_NEXT.md`, "utf8");

// remove note for unstable release
data = data.replace(
  /<!-- unstable-readme-start-->[\s\S]*?<!-- unstable-readme-end-->/,
  "",
);

// replace "excalidraw-next" with "excalidraw"
data = data.replace(/excalidraw-next/g, "excalidraw");
data = data.trim();

const demoIndex = data.indexOf("### Demo");
const excalidrawNextNote =
  "#### Note\n\n**If you don't want to wait for the next stable release and try out the unreleased changes you can use [@excalidraw/excalidraw-next](https://www.npmjs.com/package/@excalidraw/excalidraw-next).**\n\n";
// Add excalidraw next note to try out for unreleased changes
data = data.slice(0, demoIndex) + excalidrawNextNote + data.slice(demoIndex);

// update readme
fs.writeFileSync(`${excalidrawDir}/README.md`, data, "utf8");
Repository: excalidraw/excalidraw
File: scripts/updateReadme.js (JavaScript)

Code before:
const fs = require("fs");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
let data = fs.readFileSync(`${excalidrawDir}/README_NEXT.md`, "utf8");

// remove note for unstable release
data = data.replace(
  /<!-- stable-readme-start-->[\s\S]*?<!-- stable-readme-end-->/,
  "",
);

// replace "excalidraw-next" with "excalidraw"
data = data.replace(/excalidraw-next/g, "excalidraw");
data = data.trim();

const demoIndex = data.indexOf("### Demo");
const excalidrawNextNote =
  "#### Note\n\n**If you don't want to wait for the next stable release and try out the unreleased changes you can use [@excalidraw/excalidraw-next](https://www.npmjs.com/package/@excalidraw/excalidraw-next).**\n\n";
// Add excalidraw next note to try out for unreleased changes
data = data.substr(0, demoIndex) + excalidrawNextNote + data.substr(demoIndex);

// update readme
fs.writeFileSync(`${excalidrawDir}/README.md`, data, "utf8");


Review comment (suggestion):
```suggestion
data = data.slice(0, demoIndex) + excalidrawNextNote + data.slice(demoIndex);
```

Pretty pls. `.substr`/`.substring` should be relegated to history :)

Code after:
const fs = require("fs");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
let data = fs.readFileSync(`${excalidrawDir}/README_NEXT.md`, "utf8");

// remove note for unstable release
data = data.replace(
  /<!-- unstable-readme-start-->[\s\S]*?<!-- unstable-readme-end-->/,
  "",
);

// replace "excalidraw-next" with "excalidraw"
data = data.replace(/excalidraw-next/g, "excalidraw");
data = data.trim();

const demoIndex = data.indexOf("### Demo");
const excalidrawNextNote =
  "#### Note\n\n**If you don't want to wait for the next stable release and try out the unreleased changes you can use [@excalidraw/excalidraw-next](https://www.npmjs.com/package/@excalidraw/excalidraw-next).**\n\n";
// Add excalidraw next note to try out for unreleased changes
data = data.slice(0, demoIndex) + excalidrawNextNote + data.slice(demoIndex);

// update readme
fs.writeFileSync(`${excalidrawDir}/README.md`, data, "utf8");
Repository: excalidraw/excalidraw
File: scripts/release.js (JavaScript)

Code before:
const fs = require("fs");
const util = require("util");
const exec = util.promisify(require("child_process").exec);
const updateReadme = require("./updateReadme");
const updateChangelog = require("./updateChangelog");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
const excalidrawPackage = `${excalidrawDir}/package.json`;

const updatePackageVersion = () => {
  const pkg = require(excalidrawPackage);
  pkg.version = nextVersion;
  const content = `${JSON.stringify(pkg, null, 2)}\n`;
  fs.writeFileSync(excalidrawPackage, content, "utf-8");
};

const release = async () => {
  try {
    updateReadme();
    await updateChangelog(nextVersion);
    updatePackageVersion();
    await exec(`git add -u`);
    await exec(
      `git commit -m "docs: release excalidraw@exclidraw@${nextVersion}  🎉"`,
    );
    /* eslint-disable no-console */
    console.log("Done!");
  } catch (e) {
    console.error(e);
    process.exit(1);
  }
};

const nextVersion = process.argv.slice(2)[0];
if (!nextVersion) {
  console.error("Pass the next version to release!");
  process.exit(1);

Review comment (suggestion):
I'd pass `nextVersion` as an arg for clarity instead of a global

Code after:
const fs = require("fs");
const util = require("util");
const exec = util.promisify(require("child_process").exec);
const updateReadme = require("./updateReadme");
const updateChangelog = require("./updateChangelog");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
const excalidrawPackage = `${excalidrawDir}/package.json`;

const updatePackageVersion = (nextVersion) => {
  const pkg = require(excalidrawPackage);
  pkg.version = nextVersion;
  const content = `${JSON.stringify(pkg, null, 2)}\n`;
  fs.writeFileSync(excalidrawPackage, content, "utf-8");
};

const release = async (nextVersion) => {
  try {
    updateReadme();
    await updateChangelog(nextVersion);
    updatePackageVersion();
    await exec(`git add -u`);
    await exec(
      `git commit -m "docs: release excalidraw@excalidraw@${nextVersion}  🎉"`,
    );
    /* eslint-disable no-console */
    console.log("Done!");
  } catch (e) {
    console.error(e);
    process.exit(1);
  }
};

const nextVersion = process.argv.slice(2)[0];
if (!nextVersion) {
  console.error("Pass the next version to release!");
  process.exit(1);
Repository: excalidraw/excalidraw
File: scripts/release.js (JavaScript)

Code before:
const fs = require("fs");
const util = require("util");
const exec = util.promisify(require("child_process").exec);
const updateReadme = require("./updateReadme");
const updateChangelog = require("./updateChangelog");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
const excalidrawPackage = `${excalidrawDir}/package.json`;

const updatePackageVersion = () => {
  const pkg = require(excalidrawPackage);
  pkg.version = nextVersion;
  const content = `${JSON.stringify(pkg, null, 2)}\n`;
  fs.writeFileSync(excalidrawPackage, content, "utf-8");
};

const release = async () => {
  try {
    updateReadme();
    await updateChangelog(nextVersion);
    updatePackageVersion();
    await exec(`git add -u`);
    await exec(
      `git commit -m "docs: release excalidraw@exclidraw@${nextVersion}  🎉"`,
    );
    /* eslint-disable no-console */
    console.log("Done!");
  } catch (e) {
    console.error(e);
    process.exit(1);
  }
};

const nextVersion = process.argv.slice(2)[0];
if (!nextVersion) {
  console.error("Pass the next version to release!");
  process.exit(1);
}
release(nextVersion);


Review comment (suggestion):
```suggestion
      `git commit -m "docs: release @excalidraw/excalidraw@${nextVersion}  🎉"`,
```

Code after:
const fs = require("fs");
const util = require("util");
const exec = util.promisify(require("child_process").exec);
const updateReadme = require("./updateReadme");
const updateChangelog = require("./updateChangelog");

const excalidrawDir = `${__dirname}/../src/packages/excalidraw`;
const excalidrawPackage = `${excalidrawDir}/package.json`;

const updatePackageVersion = (nextVersion) => {
  const pkg = require(excalidrawPackage);
  pkg.version = nextVersion;
  const content = `${JSON.stringify(pkg, null, 2)}\n`;
  fs.writeFileSync(excalidrawPackage, content, "utf-8");
};

const release = async (nextVersion) => {
  try {
    updateReadme();
    await updateChangelog(nextVersion);
    updatePackageVersion();
    await exec(`git add -u`);
    await exec(
      `git commit -m "docs: release excalidraw@excalidraw@${nextVersion}  🎉"`,
    );
    /* eslint-disable no-console */
    console.log("Done!");
  } catch (e) {
    console.error(e);
    process.exit(1);
  }
};

const nextVersion = process.argv.slice(2)[0];
if (!nextVersion) {
  console.error("Pass the next version to release!");
  process.exit(1);
}
release(nextVersion);