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

Code before:
    _elementsToAdd.push(element);

    const boundTextElement = getBoundTextElement(element);
    if (boundTextElement) {
      _elementsToAdd.push(boundTextElement);
    }
  }

  let nextElements = allElements.slice();
  // Optimisation since findIndex on "newElements" is slow
  const nextElementsIndex = nextElements.reduce(
    (acc: Record<string, number | undefined>, element, index) => {
      acc[element.id] = index;
      return acc;
    },
    {},
  );

  const frameBoundary = findIndex(nextElements, (e) => e.frameId === frame.id);
  for (const element of omitGroupsContainingFrames(
    allElements,
    _elementsToAdd,
  )) {
    if (element.frameId !== frame.id && !isFrameElement(element)) {
      mutateElement(
        element,
        {
          frameId: frame.id,
        },
        false,
      );

      const frameIndex = nextElementsIndex[frame.id] ?? -1;
      const elementIndex = nextElementsIndex[element.id] ?? -1;

      if (elementIndex < frameBoundary) {
        nextElements = [
          ...nextElements.slice(0, elementIndex),
          ...nextElements.slice(elementIndex + 1, frameBoundary),
          element,
          ...nextElements.slice(frameBoundary),
        ];
      } else if (elementIndex > frameIndex) {
        nextElements = [
          ...nextElements.slice(0, frameIndex),
          element,
          ...nextElements.slice(frameIndex, elementIndex),
          ...nextElements.slice(elementIndex + 1),
        ];
      }
    }

Review comment (performance):
turns out we can't cache because we mutate the `nextElements` in each iteration, reodering things around

Code after:
    _elementsToAdd.push(element);

    const boundTextElement = getBoundTextElement(element);
    if (boundTextElement) {
      _elementsToAdd.push(boundTextElement);
    }
  }

  let nextElements = allElements.slice();

  const frameBoundary = findIndex(nextElements, (e) => e.frameId === frame.id);
  for (const element of omitGroupsContainingFrames(
    allElements,
    _elementsToAdd,
  )) {
    if (element.frameId !== frame.id && !isFrameElement(element)) {
      mutateElement(
        element,
        {
          frameId: frame.id,
        },
        false,
      );

      const frameIndex = findIndex(nextElements, (e) => e.id === frame.id);
      const elementIndex = findIndex(nextElements, (e) => e.id === element.id);

      if (elementIndex < frameBoundary) {
        nextElements = [
          ...nextElements.slice(0, elementIndex),
          ...nextElements.slice(elementIndex + 1, frameBoundary),
          element,
          ...nextElements.slice(frameBoundary),
        ];
      } else if (elementIndex > frameIndex) {
        nextElements = [
          ...nextElements.slice(0, frameIndex),
          element,
          ...nextElements.slice(frameIndex, elementIndex),
          ...nextElements.slice(elementIndex + 1),
        ];
      }
    }
  }

  return nextElements;
};

export const removeElementsFromFrame = (
  allElements: ExcalidrawElementsIncludingDeleted,
  elementsToRemove: NonDeletedExcalidrawElement[],
Repository: excalidraw/excalidraw
File: src/math.ts (TypeScript)

Code before:
  }

  // p1, q1 and p2 are colinear and q2 lies on segment p1q1
  if (o2 === 0 && isPointWithinBounds(p1, q2, q1)) {
    return true;
  }

  // p2, q2 and p1 are colinear and p1 lies on segment p2q2
  if (o3 === 0 && isPointWithinBounds(p2, p1, q2)) {
    return true;
  }

  // p2, q2 and q1 are colinear and q1 lies on segment p2q2
  if (o4 === 0 && isPointWithinBounds(p2, q1, q2)) {
    return true;
  }

  return false;
};

// TODO: Rounding this point causes some shake when free drawing
export const getGridPoint = (
  x: number,
  y: number,
  gridSize: number | null,
  snapToGrid: boolean = false,
): [number, number] => {
  if (gridSize && snapToGrid) {
    return [
      Math.round(x / gridSize) * gridSize,
      Math.round(y / gridSize) * gridSize,
    ];
  }
  return [x, y];
};

export const getCornerRadius = (x: number, element: ExcalidrawElement) => {
  if (
    element.roundness?.type === ROUNDNESS.PROPORTIONAL_RADIUS ||
    element.roundness?.type === ROUNDNESS.LEGACY
  ) {
    return x * DEFAULT_PROPORTIONAL_RADIUS;
  }

  if (element.roundness?.type === ROUNDNESS.ADAPTIVE_RADIUS) {
    const fixedRadiusSize = element.roundness?.value ?? DEFAULT_ADAPTIVE_RADIUS;

    const CUTOFF_SIZE = fixedRadiusSize / DEFAULT_PROPORTIONAL_RADIUS;

    if (x <= CUTOFF_SIZE) {
      return x * DEFAULT_PROPORTIONAL_RADIUS;

Review comment (suggestion):
we can reuse the `gridSize` for this, by assigning `null` if we don't want to snap to grid. Not the most intuitive parameter (as it's currently being used already), but your proposal makes the grid snapping dependent on two separate parameters so it's making it even less clear.

Code after:
  }

  // p1, q1 and p2 are colinear and q2 lies on segment p1q1
  if (o2 === 0 && isPointWithinBounds(p1, q2, q1)) {
    return true;
  }

  // p2, q2 and p1 are colinear and p1 lies on segment p2q2
  if (o3 === 0 && isPointWithinBounds(p2, p1, q2)) {
    return true;
  }

  // p2, q2 and q1 are colinear and q1 lies on segment p2q2
  if (o4 === 0 && isPointWithinBounds(p2, q1, q2)) {
    return true;
  }

  return false;
};

// TODO: Rounding this point causes some shake when free drawing
export const getGridPoint = (
  x: number,
  y: number,
  gridSize: number | null,
): [number, number] => {
  if (gridSize) {
    return [
      Math.round(x / gridSize) * gridSize,
      Math.round(y / gridSize) * gridSize,
    ];
  }
  return [x, y];
};

export const getCornerRadius = (x: number, element: ExcalidrawElement) => {
  if (
    element.roundness?.type === ROUNDNESS.PROPORTIONAL_RADIUS ||
    element.roundness?.type === ROUNDNESS.LEGACY
  ) {
    return x * DEFAULT_PROPORTIONAL_RADIUS;
  }

  if (element.roundness?.type === ROUNDNESS.ADAPTIVE_RADIUS) {
    const fixedRadiusSize = element.roundness?.value ?? DEFAULT_ADAPTIVE_RADIUS;

    const CUTOFF_SIZE = fixedRadiusSize / DEFAULT_PROPORTIONAL_RADIUS;

    if (x <= CUTOFF_SIZE) {
      return x * DEFAULT_PROPORTIONAL_RADIUS;
    }
Repository: excalidraw/excalidraw
File: src/constants.ts (TypeScript)

Code before:
export const DRAGGING_THRESHOLD = 10; // px
export const LINE_CONFIRM_THRESHOLD = 8; // px
export const ELEMENT_SHIFT_TRANSLATE_AMOUNT = 5;
export const ELEMENT_TRANSLATE_AMOUNT = 1;
export const TEXT_TO_CENTER_SNAP_THRESHOLD = 30;
export const SHIFT_LOCKING_ANGLE = Math.PI / 12;
export const CURSOR_TYPE = {
  TEXT: "text",
  CROSSHAIR: "crosshair",
  GRABBING: "grabbing",
  GRAB: "grab",
  POINTER: "pointer",
  MOVE: "move",
  AUTO: "",
};
export const POINTER_BUTTON = {
  MAIN: 0,
  WHEEL: 1,
  SECONDARY: 2,
  TOUCH: -1,
} as const;

export const POINTER_EVENTS = {
  enabled: "all",
  disabled: "none",
  // asserted as any so it canbe freely assigned to React Element
  // "pointerEnvets" CSS prop
  inheritFromUI: "var(--ui-pointerEvents)" as any,
} as const;

export enum EVENT {
  COPY = "copy",
  PASTE = "paste",
  CUT = "cut",
  KEYDOWN = "keydown",
  KEYUP = "keyup",
  MOUSE_MOVE = "mousemove",
  RESIZE = "resize",
  UNLOAD = "unload",
  FOCUS = "focus",
  BLUR = "blur",
  DRAG_OVER = "dragover",
  DROP = "drop",
  GESTURE_END = "gestureend",
  BEFORE_UNLOAD = "beforeunload",
  GESTURE_START = "gesturestart",
  GESTURE_CHANGE = "gesturechange",
  POINTER_MOVE = "pointermove",
  POINTER_DOWN = "pointerdown",
  POINTER_UP = "pointerup",
  STATE_CHANGE = "statechange",

Review comment (suggestion):
```suggestion
  // asserted as any so it can be freely assigned to React Element
```

Code after:
export const DRAGGING_THRESHOLD = 10; // px
export const LINE_CONFIRM_THRESHOLD = 8; // px
export const ELEMENT_SHIFT_TRANSLATE_AMOUNT = 5;
export const ELEMENT_TRANSLATE_AMOUNT = 1;
export const TEXT_TO_CENTER_SNAP_THRESHOLD = 30;
export const SHIFT_LOCKING_ANGLE = Math.PI / 12;
export const CURSOR_TYPE = {
  TEXT: "text",
  CROSSHAIR: "crosshair",
  GRABBING: "grabbing",
  GRAB: "grab",
  POINTER: "pointer",
  MOVE: "move",
  AUTO: "",
};
export const POINTER_BUTTON = {
  MAIN: 0,
  WHEEL: 1,
  SECONDARY: 2,
  TOUCH: -1,
} as const;

export const POINTER_EVENTS = {
  enabled: "all",
  disabled: "none",
  // asserted as any so it can be freely assigned to React Element
  // "pointerEnvets" CSS prop
  inheritFromUI: "var(--ui-pointerEvents)" as any,
} as const;

export enum EVENT {
  COPY = "copy",
  PASTE = "paste",
  CUT = "cut",
  KEYDOWN = "keydown",
  KEYUP = "keyup",
  MOUSE_MOVE = "mousemove",
  RESIZE = "resize",
  UNLOAD = "unload",
  FOCUS = "focus",
  BLUR = "blur",
  DRAG_OVER = "dragover",
  DROP = "drop",
  GESTURE_END = "gestureend",
  BEFORE_UNLOAD = "beforeunload",
  GESTURE_START = "gesturestart",
  GESTURE_CHANGE = "gesturechange",
  POINTER_MOVE = "pointermove",
  POINTER_DOWN = "pointerdown",
  POINTER_UP = "pointerup",
  STATE_CHANGE = "statechange",
Repository: excalidraw/excalidraw
File: src/tests/tool.test.tsx (TypeScript)

Code before:
    expect(excalidrawAPI.setActiveTool).toBe(h.app.setActiveTool);
  });

  it("should set tool type", async () => {
    expect(h.state.activeTool.type).toBe("selection");
    excalidrawAPI.setActiveTool({ type: "rectangle" });
    expect(h.state.activeTool.type).toBe("rectangle");

    mouse.down(10, 10);
    mouse.up(20, 20);

    expect(h.state.activeTool.type).toBe("selection");
  });

  it("should support tool locking", async () => {
    expect(h.state.activeTool.type).toBe("selection");
    excalidrawAPI.setActiveTool({ type: "rectangle", locked: true });
    expect(h.state.activeTool.type).toBe("rectangle");

    mouse.down(10, 10);
    mouse.up(20, 20);

    expect(h.state.activeTool.type).toBe("rectangle");
  });

  it("should setting custom tool", async () => {
    expect(h.state.activeTool.type).toBe("selection");
    excalidrawAPI.setActiveTool({ type: "custom", customType: "comment" });
    expect(h.state.activeTool.type).toBe("custom");
    expect(h.state.activeTool.customType).toBe("comment");
  });
});


Review comment (suggestion):
```suggestion
  it("should set custom tool", async () => {
```

Code after:
    expect(excalidrawAPI.setActiveTool).toBe(h.app.setActiveTool);
  });

  it("should set the active tool type", async () => {
    expect(h.state.activeTool.type).toBe("selection");
    excalidrawAPI.setActiveTool({ type: "rectangle" });
    expect(h.state.activeTool.type).toBe("rectangle");

    mouse.down(10, 10);
    mouse.up(20, 20);

    expect(h.state.activeTool.type).toBe("selection");
  });

  it("should support tool locking", async () => {
    expect(h.state.activeTool.type).toBe("selection");
    excalidrawAPI.setActiveTool({ type: "rectangle", locked: true });
    expect(h.state.activeTool.type).toBe("rectangle");

    mouse.down(10, 10);
    mouse.up(20, 20);

    expect(h.state.activeTool.type).toBe("rectangle");
  });

  it("should set custom tool", async () => {
    expect(h.state.activeTool.type).toBe("selection");
    excalidrawAPI.setActiveTool({ type: "custom", customType: "comment" });
    expect(h.state.activeTool.type).toBe("custom");
    expect(h.state.activeTool.customType).toBe("comment");
  });
});
Repository: excalidraw/excalidraw
File: packages/excalidraw/scene/Scene.ts (TypeScript)

Code before:
    this.elements = syncInvalidIndices(_nextElements);
    this.elementsMap.clear();
    this.elements.forEach((element) => {
      if (isFrameLikeElement(element)) {
        nextFrameLikes.push(element);
      }
      this.elementsMap.set(element.id, element);
      Scene.mapElementToScene(element, this);
    });
    const nonDeletedElements = getNonDeletedElements(this.elements);
    this.nonDeletedElements = nonDeletedElements.elements;
    this.nonDeletedElementsMap = nonDeletedElements.elementsMap;

    this.frames = nextFrameLikes;
    this.nonDeletedFramesLikes = getNonDeletedElements(this.frames).elements;

    this.informMutation();
  }

  informMutation() {
    this.versionNonce = randomInteger();

    for (const callback of Array.from(this.callbacks)) {
      callback();
    }
  }

  addCallback(cb: SceneStateCallback): SceneStateCallbackRemover {
    if (this.callbacks.has(cb)) {
      throw new Error();
    }

    this.callbacks.add(cb);

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

  destroy() {
    this.elements = [];
    this.nonDeletedElements = [];
    this.nonDeletedFramesLikes = [];
    this.frames = [];
    this.elementsMap.clear();
    this.selectedElementsCache.selectedElementIds = null;
    this.selectedElementsCache.elements = null;
    this.selectedElementsCache.cache.clear();

Review comment (bug):
`informMutation()` was a bit unclear and incorrect as the it didn't have to relate to mutation, and we generally just care about notifying that an update to the scene happened

Code after:

    if (import.meta.env.DEV || import.meta.env.MODE === ENV.TEST) {
      // throw on invalid indices in test / dev to potentially detect cases were we forgot to sync moved elements
      validateFractionalIndices(_nextElements.map((x) => x.index));
    }

    this.elements = syncInvalidIndices(_nextElements);
    this.elementsMap.clear();
    this.elements.forEach((element) => {
      if (isFrameLikeElement(element)) {
        nextFrameLikes.push(element);
      }
      this.elementsMap.set(element.id, element);
      Scene.mapElementToScene(element, this);
    });
    const nonDeletedElements = getNonDeletedElements(this.elements);
    this.nonDeletedElements = nonDeletedElements.elements;
    this.nonDeletedElementsMap = nonDeletedElements.elementsMap;

    this.frames = nextFrameLikes;
    this.nonDeletedFramesLikes = getNonDeletedElements(this.frames).elements;

    this.triggerUpdate();
  }

  triggerUpdate() {
    this.sceneNonce = randomInteger();

    for (const callback of Array.from(this.callbacks)) {
      callback();
    }
  }

  onUpdate(cb: SceneStateCallback): SceneStateCallbackRemover {
    if (this.callbacks.has(cb)) {
      throw new Error();
    }

    this.callbacks.add(cb);

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

  destroy() {
    this.elements = [];
    this.nonDeletedElements = [];
Repository: excalidraw/excalidraw
File: packages/excalidraw/tests/test-utils.ts (TypeScript)

Code before:
expect.extend({
  toBeNonNaNNumber(received) {
    const pass = typeof received === "number" && !isNaN(received);
    if (pass) {
      return {
        message: () => `expected ${received} not to be a non-NaN number`,
        pass: true,
      };
    }
    return {
      message: () => `expected ${received} to be a non-NaN number`,
      pass: false,
    };
  },
});

/**
 * Serializer for IEE754 float pointing numbers to avoid random failures due to tiny precision differences
 */
expect.addSnapshotSerializer({
  serialize(val, config, indentation, depth, refs, printer) {
    return printer(val.toFixed(5), config, indentation, depth, refs);
  },
  test(val) {
    return (
      typeof val === "number" && !Number.isNaN(val) && !Number.isInteger(val)
    );
  },
});


Review comment (suggestion):
```suggestion
      typeof val === "number" && !Number.isNaN(val) && !Number.isInteger(val) && Number.isFinite(val)
```

Infinity

(line length won't lint)

Code after:
expect.extend({
  toBeNonNaNNumber(received) {
    const pass = typeof received === "number" && !isNaN(received);
    if (pass) {
      return {
        message: () => `expected ${received} not to be a non-NaN number`,
        pass: true,
      };
    }
    return {
      message: () => `expected ${received} to be a non-NaN number`,
      pass: false,
    };
  },
});

/**
 * Serializer for IEE754 float pointing numbers to avoid random failures due to tiny precision differences
 */
expect.addSnapshotSerializer({
  serialize(val, config, indentation, depth, refs, printer) {
    return printer(val.toFixed(5), config, indentation, depth, refs);
  },
  test(val) {
    return (
      typeof val === "number" &&
      Number.isFinite(val) &&
      !Number.isNaN(val) &&
      !Number.isInteger(val)
    );
  },
});
Repository: excalidraw/excalidraw
File: excalidraw-app/vite.config.mts (TypeScript)

Code before:
import { defineConfig, loadEnv } from "vite";
import react from "@vitejs/plugin-react";
import svgrPlugin from "vite-plugin-svgr";
import { ViteEjsPlugin } from "vite-plugin-ejs";
import { VitePWA } from "vite-plugin-pwa";
import checker from "vite-plugin-checker";
import { createHtmlPlugin } from "vite-plugin-html";
import { woff2BrowserPlugin } from "./scripts/woff2-vite-plugins";

// To load .env.local variables
const envVars = loadEnv("", `../`);
// https://vitejs.dev/config/
export default defineConfig({
  server: {
    port: Number(envVars.VITE_APP_PORT || 3000),
    // open the browser
    open: true,
  },
  // We need to specify the envDir since now there are no
  //more located in parallel with the vite.config.ts file but in parent dir
  envDir: "../",
  build: {
    outDir: "build",
    rollupOptions: {
      output: {
        assetFileNames(chunkInfo) {
          if (chunkInfo?.name?.endsWith(".woff2")) {
            // put on root so we are flexible about the CDN path
            return '[name]-[hash][extname]';
          }

          return 'assets/[name]-[hash][extname]';
        },

Review comment (suggestion):
I wanted to keep `woff2-vite-plugin` alongside with `woff2-esbuild-plugin`, as those two are essentially the same thing, just one configured for `excalidraw-app` build with vite+rollup and other for packages like `@excalidraw/excalidraw` and `@excalidraw/utils` build with pure esbuild. Thus keeping both on root.

Could we still keep both together? Notice that the current location in script for vite & esbuild build plugins also isn't ideal (they don't really belong to `scripts`).

<img width="306" alt="image" src="https://github.com/user-attachments/assets/cb7c42ea-5e07-44c5-9dd5-f53bbdd6f790">


Code after:
import { defineConfig, loadEnv } from "vite";
import react from "@vitejs/plugin-react";
import svgrPlugin from "vite-plugin-svgr";
import { ViteEjsPlugin } from "vite-plugin-ejs";
import { VitePWA } from "vite-plugin-pwa";
import checker from "vite-plugin-checker";
import { createHtmlPlugin } from "vite-plugin-html";
import { woff2BrowserPlugin } from "../scripts/woff2/woff2-vite-plugins";

// To load .env.local variables
const envVars = loadEnv("", `../`);
// https://vitejs.dev/config/
export default defineConfig({
  server: {
    port: Number(envVars.VITE_APP_PORT || 3000),
    // open the browser
    open: true,
  },
  // We need to specify the envDir since now there are no
  //more located in parallel with the vite.config.ts file but in parent dir
  envDir: "../",
  build: {
    outDir: "build",
    rollupOptions: {
      output: {
        assetFileNames(chunkInfo) {
          if (chunkInfo?.name?.endsWith(".woff2")) {
            // put on root so we are flexible about the CDN path
            return "[name]-[hash][extname]";
          }

          return "assets/[name]-[hash][extname]";
        },
Repository: excalidraw/excalidraw
File: packages/excalidraw/actions/actionFlip.ts (TypeScript)

Code before:
  label: "labels.flipHorizontal",
  icon: flipHorizontal,
  trackEvent: { category: "element" },
  perform: (elements, appState, _, app) => {
    return {
      elements: updateFrameMembershipOfSelectedElements(
        flipSelectedElements(
          elements,
          app.scene.getNonDeletedElementsMap(),
          appState,
          "horizontal",
          app,
        ),
        appState,
        app,
      ),
      appState,
      storeAction: StoreAction.CAPTURE,
    };
  },
  keyTest: (event) => event.shiftKey && event.code === CODES.H,
  predicate: (_, appState) => {
    if (appState.selectedLinearElement) {
      // No flippin' single selected elbow arrows!
      return !appState.selectedLinearElement?.elbowed;
    }

    return true;
  },
});

export const actionFlipVertical = register({
  name: "flipVertical",
  label: "labels.flipVertical",
  icon: flipVertical,
  trackEvent: { category: "element" },
  perform: (elements, appState, _, app) => {
    return {
      elements: updateFrameMembershipOfSelectedElements(
        flipSelectedElements(
          elements,
          app.scene.getNonDeletedElementsMap(),
          appState,
          "vertical",
          app,
        ),
        appState,
        app,
      ),
      appState,
      storeAction: StoreAction.CAPTURE,

Review comment (suggestion):
Provided we indeed want to prevent this (see https://github.com/excalidraw/excalidraw/pull/8324#issuecomment-2278370214), this will work only for contextMenu. Also it doesn't handle cases where you select multiple elbow arrows alone.



Code after:

export const actionFlipHorizontal = register({
  name: "flipHorizontal",
  label: "labels.flipHorizontal",
  icon: flipHorizontal,
  trackEvent: { category: "element" },
  perform: (elements, appState, _, app) => {
    return {
      elements: updateFrameMembershipOfSelectedElements(
        flipSelectedElements(
          elements,
          app.scene.getNonDeletedElementsMap(),
          appState,
          "horizontal",
          app,
        ),
        appState,
        app,
      ),
      appState,
      storeAction: StoreAction.CAPTURE,
    };
  },
  keyTest: (event) => event.shiftKey && event.code === CODES.H,
});

export const actionFlipVertical = register({
  name: "flipVertical",
  label: "labels.flipVertical",
  icon: flipVertical,
  trackEvent: { category: "element" },
  perform: (elements, appState, _, app) => {
    return {
      elements: updateFrameMembershipOfSelectedElements(
        flipSelectedElements(
          elements,
          app.scene.getNonDeletedElementsMap(),
          appState,
          "vertical",
          app,
        ),
        appState,
        app,
      ),
      appState,
      storeAction: StoreAction.CAPTURE,
    };
  },
  keyTest: (event) =>
    event.shiftKey && event.code === CODES.V && !event[KEYS.CTRL_OR_CMD],
});