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

Code before:
  ExcalidrawLinearElement,
  ExcalidrawTextElement,
} from "../element/types";
import { getSelectedElements } from "../scene";
import { register } from "./register";

export const actionUnbindText = register({
  name: "unbindText",
  contextItemLabel: "labels.unbindText",
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
    );
    selectedElements.forEach((element) => {
      const boundTextElement = getBoundTextElement(element);
      if (boundTextElement) {
        mutateElement(boundTextElement as ExcalidrawTextElement, {
          containerId: null,
        });
        mutateElement(element, {
          boundElements: element.boundElements?.filter(
            (ele) =>
              !isTextElement(
                ele as ExcalidrawTextElement | ExcalidrawLinearElement,
              ),
          ),
        });
      }
    });
    return {
      elements,
      appState,
      commitToHistory: true,
    };
  },
});


Review comment (suggestion):
to make this future-proof, we should only remove the target element using the `boundTextElement.id`

Code after:
import { getSelectedElements } from "../scene";
import { getFontString } from "../utils";
import { register } from "./register";

export const actionUnbindText = register({
  name: "unbindText",
  contextItemLabel: "labels.unbindText",
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
    );
    selectedElements.forEach((element) => {
      const boundTextElement = getBoundTextElement(element);
      if (boundTextElement) {
        const { width, height, baseline } = measureText(
          boundTextElement.originalText,
          getFontString(boundTextElement),
        );
        mutateElement(boundTextElement as ExcalidrawTextElement, {
          containerId: null,
          width,
          height,
          baseline,
          text: boundTextElement.originalText,
        });
        mutateElement(element, {
          boundElements: element.boundElements?.filter(
            (ele) => ele.id !== boundTextElement.id,
          ),
        });
      }
    });
    return {
      elements,
      appState,
      commitToHistory: true,
    };
  },
});
Repository: excalidraw/excalidraw
File: src/utils.ts (TypeScript)

Code before:
  };
  ret.flush = () => {
    clearTimeout(handle);
    if (lastArgs) {
      const _lastArgs = lastArgs;
      lastArgs = null;
      fn(..._lastArgs);
    }
  };
  ret.cancel = () => {
    lastArgs = null;
    clearTimeout(handle);
  };
  return ret;
};

// https://github.com/lodash/lodash/blob/es/chunk.js
export const chunk = <T extends any>(
  array: readonly T[],
  size: number,
): T[][] => {
  if (!array.length || size < 1) {
    return [];
  }
  let index = 0;
  let resIndex = 0;
  const result = Array(Math.ceil(array.length / size));
  while (index < array.length) {
    result[resIndex++] = array.slice(index, (index += size));
  }
  return result;
};

export const selectNode = (node: Element) => {
  const selection = window.getSelection();
  if (selection) {
    const range = document.createRange();
    range.selectNodeContents(node);
    selection.removeAllRanges();
    selection.addRange(range);
  }
};

export const removeSelection = () => {
  const selection = window.getSelection();
  if (selection) {
    selection.removeAllRanges();
  }
};

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

Review comment (bug):
I've disabled the rAF in tests as we'd have to rewrite a lot of them (we're often manually triggering multiple `pointermove` events to test several things within a single test case).

I don't think testing the rAF behavior would yield anything useful, but I may be wrong.

Code after:
  };
  ret.flush = () => {
    clearTimeout(handle);
    if (lastArgs) {
      const _lastArgs = lastArgs;
      lastArgs = null;
      fn(..._lastArgs);
    }
  };
  ret.cancel = () => {
    lastArgs = null;
    clearTimeout(handle);
  };
  return ret;
};

// throttle callback to execute once per animation frame
export const throttleRAF = <T extends any[]>(fn: (...args: T) => void) => {
  let handle: number | null = null;
  let lastArgs: T | null = null;
  let callback: ((...args: T) => void) | null = null;
  const ret = (...args: T) => {
    if (process.env.NODE_ENV === "test") {
      fn(...args);
      return;
    }
    lastArgs = args;
    callback = fn;
    if (handle === null) {
      handle = window.requestAnimationFrame(() => {
        handle = null;
        lastArgs = null;
        callback = null;
        fn(...args);
      });
    }
  };
  ret.flush = () => {
    if (handle !== null) {
      cancelAnimationFrame(handle);
      handle = null;
    }
    if (lastArgs) {
      const _lastArgs = lastArgs;
      const _callback = callback;
      lastArgs = null;
      callback = null;
      if (_callback !== null) {
        _callback(..._lastArgs);
      }
    }
Repository: excalidraw/excalidraw
File: src/components/App.tsx (TypeScript)

Code before:
    if (!isExcalidrawActive || isWritableElement(event.target)) {
      return;
    }
    this.copyAll();
    event.preventDefault();
  });

  private cutAll = () => {
    this.copyAll();
    this.actionManager.executeAction(actionDeleteSelected);
  };

  private copyAll = () => {
    copyToClipboard(this.scene.getElements(), this.state, this.files);
  };

  private static resetTapTwice() {
    didTapTwice = false;
  }

  private onTapStart = (event: TouchEvent) => {
    // fix for Apple Pencil Scribble
    if (!isAndroid) {
      event.preventDefault();
    }

    if (!didTapTwice) {
      didTapTwice = true;
      clearTimeout(tappedTwiceTimer);
      tappedTwiceTimer = window.setTimeout(
        App.resetTapTwice,
        TAP_TWICE_TIMEOUT,
      );
      return;
    }
    // insert text only if we tapped twice with a single finger
    // event.touches.length === 1 will also prevent inserting text when user's zooming
    if (didTapTwice && event.touches.length === 1) {
      const [touch] = event.touches;
      // @ts-ignore
      this.handleCanvasDoubleClick({
        clientX: touch.clientX,
        clientY: touch.clientY,
      });
      didTapTwice = false;
      clearTimeout(tappedTwiceTimer);
    }
    if (isAndroid) {
      event.preventDefault();
    }


Review comment (suggestion):
```suggestion
    // On Android, preventing the event would disable contextMenu on tap-hold
    if (!isAndroid) {
      event.preventDefault();
    }
```

Code after:
    if (!isExcalidrawActive || isWritableElement(event.target)) {
      return;
    }
    this.copyAll();
    event.preventDefault();
  });

  private cutAll = () => {
    this.copyAll();
    this.actionManager.executeAction(actionDeleteSelected);
  };

  private copyAll = () => {
    copyToClipboard(this.scene.getElements(), this.state, this.files);
  };

  private static resetTapTwice() {
    didTapTwice = false;
  }

  private onTapStart = (event: TouchEvent) => {
    // fix for Apple Pencil Scribble
    // On Android, preventing the event would disable contextMenu on tap-hold
    if (!isAndroid) {
      event.preventDefault();
    }

    if (!didTapTwice) {
      didTapTwice = true;
      clearTimeout(tappedTwiceTimer);
      tappedTwiceTimer = window.setTimeout(
        App.resetTapTwice,
        TAP_TWICE_TIMEOUT,
      );
      return;
    }
    // insert text only if we tapped twice with a single finger
    // event.touches.length === 1 will also prevent inserting text when user's zooming
    if (didTapTwice && event.touches.length === 1) {
      const [touch] = event.touches;
      // @ts-ignore
      this.handleCanvasDoubleClick({
        clientX: touch.clientX,
        clientY: touch.clientY,
      });
      didTapTwice = false;
      clearTimeout(tappedTwiceTimer);
    }
    if (isAndroid) {
      event.preventDefault();
    }
Repository: excalidraw/excalidraw
File: src/components/App.tsx (TypeScript)

Code before:
      clearTimeout(tappedTwiceTimer);
      tappedTwiceTimer = window.setTimeout(
        App.resetTapTwice,
        TAP_TWICE_TIMEOUT,
      );
      return;
    }
    // insert text only if we tapped twice with a single finger
    // event.touches.length === 1 will also prevent inserting text when user's zooming
    if (didTapTwice && event.touches.length === 1) {
      const [touch] = event.touches;
      // @ts-ignore
      this.handleCanvasDoubleClick({
        clientX: touch.clientX,
        clientY: touch.clientY,
      });
      didTapTwice = false;
      clearTimeout(tappedTwiceTimer);
    }

    if (event.touches.length === 2) {
      this.setState({
        selectedElementIds: {},
      });
    }
  };

  private onTapEnd = (event: TouchEvent) => {
    if (event.touches.length > 0) {
      this.setState({
        previousSelectedElementIds: {},
        selectedElementIds: this.state.previousSelectedElementIds,
      });
    }
  };

  private pasteFromClipboard = withBatchedUpdates(
    async (event: ClipboardEvent | null) => {
      // #686
      const target = document.activeElement;
      const isExcalidrawActive =
        this.excalidrawContainerRef.current?.contains(target);
      if (!isExcalidrawActive) {
        return;
      }

      const elementUnderCursor = document.elementFromPoint(cursorX, cursorY);
      if (
        // if no ClipboardEvent supplied, assume we're pasting via contextMenu
        // thus these checks don't make sense
        event &&

Review comment (suggestion):
```suggestion
      event.preventDefault();
```

we don't actually need this check as it will do nothing Darwin

Code after:
    }

    if (!didTapTwice) {
      didTapTwice = true;
      clearTimeout(tappedTwiceTimer);
      tappedTwiceTimer = window.setTimeout(
        App.resetTapTwice,
        TAP_TWICE_TIMEOUT,
      );
      return;
    }
    // insert text only if we tapped twice with a single finger
    // event.touches.length === 1 will also prevent inserting text when user's zooming
    if (didTapTwice && event.touches.length === 1) {
      const [touch] = event.touches;
      // @ts-ignore
      this.handleCanvasDoubleClick({
        clientX: touch.clientX,
        clientY: touch.clientY,
      });
      didTapTwice = false;
      clearTimeout(tappedTwiceTimer);
    }
    if (isAndroid) {
      event.preventDefault();
    }

    if (event.touches.length === 2) {
      this.setState({
        selectedElementIds: {},
      });
    }
  };

  private onTapEnd = (event: TouchEvent) => {
    if (event.touches.length > 0) {
      this.setState({
        previousSelectedElementIds: {},
        selectedElementIds: this.state.previousSelectedElementIds,
      });
    }
  };

  private pasteFromClipboard = withBatchedUpdates(
    async (event: ClipboardEvent | null) => {
      // #686
      const target = document.activeElement;
      const isExcalidrawActive =
        this.excalidrawContainerRef.current?.contains(target);
      if (!isExcalidrawActive) {
        return;
Repository: excalidraw/excalidraw
File: src/actions/actionToggleZenMode.tsx (TypeScript)

Code before:
import { CODES, KEYS } from "../keys";
import { register } from "./register";

export const actionToggleZenMode = register({
  name: "zenMode",
  trackEvent: { category: "canvas" },
  perform(elements, appState) {
    return {
      appState: {
        ...appState,
        zenModeEnabled: !this.checked!(appState),
      },
      commitToHistory: false,
    };
  },
  checked: (appState) => appState.zenModeEnabled,
  contextItemLabel: "buttons.zenMode",
  keyTest: (event) =>
    !event[KEYS.CTRL_OR_CMD] && event.altKey && event.code === CODES.Z,
});


Review comment (suggestion):
I've changed the event category/action/label here and also for videMode and gridMode, which means we'll lose continuity with past data. IMO ok.

Code after:
import { CODES, KEYS } from "../keys";
import { register } from "./register";

export const actionToggleZenMode = register({
  name: "zenMode",
  trackEvent: {
    category: "canvas",
    predicate: (appState) => !appState.zenModeEnabled,
  },
  perform(elements, appState) {
    return {
      appState: {
        ...appState,
        zenModeEnabled: !this.checked!(appState),
      },
      commitToHistory: false,
    };
  },
  checked: (appState) => appState.zenModeEnabled,
  contextItemLabel: "buttons.zenMode",
  keyTest: (event) =>
    !event[KEYS.CTRL_OR_CMD] && event.altKey && event.code === CODES.Z,
});
Repository: excalidraw/excalidraw
File: src/components/ColorPicker.tsx (TypeScript)

Code before:
        <button
          className="color-picker-label-swatch"
          aria-label={label}
          style={color ? { "--swatch-color": color } : undefined}
          onClick={() => setActive(!isActive)}
          ref={pickerButton}
        />
        <ColorInput
          color={color}
          label={label}
          onChange={(color) => {
            handleOnChange(color);
          }}
        />
      </div>
      <React.Suspense fallback="">
        {isActive ? (
          <Popover
            onCloseRequest={(event) =>
              event.target !== pickerButton.current && setActive(false)
            }
          >
            <Picker
              colors={colors[type]}
              color={color || null}
              onChange={(changedColor) => {
                handleOnChange(changedColor);
              }}
              onClose={() => {
                setActive(false);
                pickerButton.current?.focus();
              }}
              label={label}
              showInput={false}
              type={type}
              elements={elements}
            />
          </Popover>
        ) : null}
      </React.Suspense>
    </div>
  );
};


Review comment (suggestion):
moved inside `Picker` compo, and I'm also initializing it once on mount. This has the added benefit that the custom color doesn't disappear if you switch to a different color (and it's the only element on canvas using that color) — that way you can still switch back to it before you close the popup.

Code after:
          onClick={() => setActive(!isActive)}
          ref={pickerButton}
        />
        <ColorInput
          color={color}
          label={label}
          onChange={(color) => {
            onChange(color);
          }}
        />
      </div>
      <React.Suspense fallback="">
        {isActive ? (
          <Popover
            onCloseRequest={(event) =>
              event.target !== pickerButton.current && setActive(false)
            }
          >
            <Picker
              colors={colors[type]}
              color={color || null}
              onChange={(changedColor) => {
                onChange(changedColor);
              }}
              onClose={() => {
                setActive(false);
                pickerButton.current?.focus();
              }}
              label={label}
              showInput={false}
              type={type}
              elements={elements}
            />
          </Popover>
        ) : null}
      </React.Suspense>
    </div>
  );
};
Repository: excalidraw/excalidraw
File: src/actions/actionStyles.test.tsx (TypeScript)

Code before:
import ExcalidrawApp from "../excalidraw-app";
import { t } from "../i18n";
import { CODES } from "../keys";
import { API } from "../tests/helpers/api";
import { Keyboard, Pointer, UI } from "../tests/helpers/ui";
import { fireEvent, render, screen } from "../tests/test-utils";
import { copiedStyles } from "./actionStyles";

const { h } = window;

const mouse = new Pointer("mouse");

describe("actionStyles", () => {
  beforeEach(async () => {
    await render(<ExcalidrawApp />);
  });
  it("copy & pasting styles via keyboard", () => {
    UI.clickTool("rectangle");
    mouse.down(10, 10);
    mouse.up(20, 20);

    UI.clickTool("rectangle");
    mouse.down(10, 10);
    mouse.up(20, 20);

    // Change some styles of second rectangle
    UI.clickLabeledElement("Stroke");
    UI.clickLabeledElement(t("colors.c92a2a"));
    UI.clickLabeledElement("Background");
    UI.clickLabeledElement(t("colors.e64980"));
    // Fill style
    fireEvent.click(screen.getByTitle("Cross-hatch"));
    // Stroke width
    fireEvent.click(screen.getByTitle("Bold"));
    // Stroke style
    fireEvent.click(screen.getByTitle("Dotted"));
    // Roughness
    fireEvent.click(screen.getByTitle("Cartoonist"));
    // Opacity
    fireEvent.change(screen.getByLabelText("Opacity"), {
      target: { value: "60" },
    });

Review comment (suggestion):
```suggestion
  
  it("should copy & paste styles via keyboard", () => {
```

Code after:
import ExcalidrawApp from "../excalidraw-app";
import { t } from "../i18n";
import { CODES } from "../keys";
import { API } from "../tests/helpers/api";
import { Keyboard, Pointer, UI } from "../tests/helpers/ui";
import { fireEvent, render, screen } from "../tests/test-utils";
import { copiedStyles } from "./actionStyles";

const { h } = window;

const mouse = new Pointer("mouse");

describe("actionStyles", () => {
  beforeEach(async () => {
    await render(<ExcalidrawApp />);
  });
  it("should copy & paste styles via keyboard", () => {
    UI.clickTool("rectangle");
    mouse.down(10, 10);
    mouse.up(20, 20);

    UI.clickTool("rectangle");
    mouse.down(10, 10);
    mouse.up(20, 20);

    // Change some styles of second rectangle
    UI.clickLabeledElement("Stroke");
    UI.clickLabeledElement(t("colors.c92a2a"));
    UI.clickLabeledElement("Background");
    UI.clickLabeledElement(t("colors.e64980"));
    // Fill style
    fireEvent.click(screen.getByTitle("Cross-hatch"));
    // Stroke width
    fireEvent.click(screen.getByTitle("Bold"));
    // Stroke style
    fireEvent.click(screen.getByTitle("Dotted"));
    // Roughness
    fireEvent.click(screen.getByTitle("Cartoonist"));
    // Opacity
    fireEvent.change(screen.getByLabelText("Opacity"), {
      target: { value: "60" },
    });
Repository: excalidraw/excalidraw
File: src/utils.ts (TypeScript)

Code before:
    }
    context.beginPath();
    context.arc(
      eraserCanvasCache.width / 2,
      eraserCanvasCache.height / 2,
      5,
      0,
      2 * Math.PI,
    );
    context.stroke();
  };
  if (!eraserCanvasCache) {
    drawCanvas();
  } else {
    const context = eraserCanvasCache.getContext("2d")!;
    const strokeStyle = context.strokeStyle;
    if (theme === THEME.LIGHT && strokeStyle !== oc.black) {
      drawCanvas();
    } else if (theme === THEME.DARK && strokeStyle !== oc.white) {
      drawCanvas();
    }
  }

  const previewDataURL = eraserCanvasCache.toDataURL(MIME_TYPES.svg) as DataURL;

  setCursor(canvas, `url(${previewDataURL}) 4 4, auto`);
};

export const setCursorForShape = (
  canvas: HTMLCanvasElement | null,
  appState: AppState,
) => {
  if (!canvas) {
    return;
  }
  if (appState.elementType === "selection") {
    resetCursor(canvas);
  } else if (appState.elementType === "eraser") {
    setEraserCursor(canvas, appState.theme);
    // do nothing if image tool is selected which suggests there's
    // a image-preview set as the cursor
  } else if (appState.elementType !== "image") {
    canvas.style.cursor = CURSOR_TYPE.CROSSHAIR;
  }
};

export const isFullScreen = () =>
  document.fullscreenElement?.nodeName === "HTML";

export const allowFullScreen = () =>
  document.documentElement.requestFullscreen();

Review comment (suggestion):
didn't find a doc for this, but it seems the `x y` positions (`4 4`) are negative offsets from top left. So if we want to center the cursor to the center of the circle, we want to do `${cursorImageSizePx/2} ${cursorImageSizePx/2}`.

Code after:
    context.beginPath();
    context.arc(
      eraserCanvasCache.width / 2,
      eraserCanvasCache.height / 2,
      5,
      0,
      2 * Math.PI,
    );
    context.fillStyle = isDarkTheme ? oc.black : oc.white;
    context.fill();
    context.strokeStyle = isDarkTheme ? oc.white : oc.black;
    context.stroke();
    previewDataURL = eraserCanvasCache.toDataURL(MIME_TYPES.svg) as DataURL;
  };
  if (!eraserCanvasCache || eraserCanvasCache.theme !== theme) {
    drawCanvas();
  }

  setCursor(
    canvas,
    `url(${previewDataURL}) ${cursorImageSizePx / 2} ${
      cursorImageSizePx / 2
    }, auto`,
  );
};

export const setCursorForShape = (
  canvas: HTMLCanvasElement | null,
  appState: AppState,
) => {
  if (!canvas) {
    return;
  }
  if (appState.elementType === "selection") {
    resetCursor(canvas);
  } else if (appState.elementType === "eraser") {
    setEraserCursor(canvas, appState.theme);
    // do nothing if image tool is selected which suggests there's
    // a image-preview set as the cursor
  } else if (appState.elementType !== "image") {
    canvas.style.cursor = CURSOR_TYPE.CROSSHAIR;
  }
};

export const isFullScreen = () =>
  document.fullscreenElement?.nodeName === "HTML";

export const allowFullScreen = () =>
  document.documentElement.requestFullscreen();

export const exitFullScreen = () => document.exitFullscreen();
Repository: excalidraw/excalidraw
File: src/utils.ts (TypeScript)

Code before:
    }
    context.stroke();

    context.beginPath();
    context.arc(
      eraserCanvasCache.width / 2,
      eraserCanvasCache.height / 2,
      7,
      0,
      2 * Math.PI,
    );
    context.strokeStyle = oc.white;
    context.stroke();

    previewDataURL = eraserCanvasCache.toDataURL(MIME_TYPES.svg) as DataURL;
  };
  if (!eraserCanvasCache) {
    drawCanvas();
  } else {
    const context = eraserCanvasCache.getContext("2d")!;
    const strokeStyle = context.strokeStyle;
    if (theme === THEME.LIGHT && strokeStyle !== oc.black) {
      drawCanvas();
    } else if (theme === THEME.DARK && strokeStyle !== oc.white) {
      drawCanvas();
    }
  }

  setCursor(
    canvas,
    `url(${previewDataURL}) ${cursorImageSizePx / 2} ${
      cursorImageSizePx / 2
    }, auto`,
  );
};

export const setCursorForShape = (
  canvas: HTMLCanvasElement | null,
  appState: AppState,
) => {
  if (!canvas) {
    return;
  }
  if (appState.elementType === "selection") {
    resetCursor(canvas);
  } else if (appState.elementType === "eraser") {
    setEraserCursor(canvas, appState.theme);
    // do nothing if image tool is selected which suggests there's
    // a image-preview set as the cursor
  } else if (appState.elementType !== "image") {
    canvas.style.cursor = CURSOR_TYPE.CROSSHAIR;

Review comment (performance):
since we now have two outlines, this check now always returns `true` in light theme. Instead of getting the context and checking strokeStyle, I'd maybe just cache what theme the cursor was last rendered in.

Code after:
  if (!eraserCanvasCache || eraserCanvasCache.theme !== theme) {
    drawCanvas();
  }

  setCursor(
    canvas,
    `url(${previewDataURL}) ${cursorImageSizePx / 2} ${
      cursorImageSizePx / 2
    }, auto`,
  );
};

export const setCursorForShape = (
  canvas: HTMLCanvasElement | null,
  appState: AppState,
) => {
  if (!canvas) {
    return;
  }
  if (appState.elementType === "selection") {
    resetCursor(canvas);
  } else if (appState.elementType === "eraser") {
    setEraserCursor(canvas, appState.theme);
    // do nothing if image tool is selected which suggests there's
    // a image-preview set as the cursor
  } else if (appState.elementType !== "image") {
    canvas.style.cursor = CURSOR_TYPE.CROSSHAIR;
  }
};

export const isFullScreen = () =>
  document.fullscreenElement?.nodeName === "HTML";

export const allowFullScreen = () =>
  document.documentElement.requestFullscreen();

export const exitFullScreen = () => document.exitFullscreen();

export const getShortcutKey = (shortcut: string): string => {
  shortcut = shortcut
    .replace(/\bAlt\b/i, "Alt")
    .replace(/\bShift\b/i, "Shift")
    .replace(/\b(Enter|Return)\b/i, "Enter")
    .replace(/\bDel\b/i, "Delete");

  if (isDarwin) {
    return shortcut
      .replace(/\bCtrlOrCmd\b/i, "Cmd")
      .replace(/\bAlt\b/i, "Option");
  }
  return shortcut.replace(/\bCtrlOrCmd\b/i, "Ctrl");