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

Code before:
              ...prevState,
              editingGroupId: selectedGroupId,
              selectedElementIds: { [hitElement!.id]: true },
              selectedGroupIds: {},
            },
            this.scene.getNonDeletedElements(),
          ),
        );
        return;
      }
    }

    resetCursor(this.canvas);
    if (!event[KEYS.CTRL_OR_CMD] && !this.state.viewModeEnabled) {
      const container = getTextBindableContainerAtPosition(
        this.scene.getNonDeletedElements(),
        this.state,
        sceneX,
        sceneY,
      );
      if (container) {
        if (
          isArrowElement(container) ||
          hasBoundTextElement(container) ||
          !isTransparent(
            (container as ExcalidrawTextContainer).backgroundColor,
          ) ||
          isHittingElementNotConsideringBoundingBox(container, this.state, [
            sceneX,
            sceneY,
          ])
        ) {
          const midPoint = getContainerCenter(container, this.state);

          sceneX = midPoint.x;
          sceneY = midPoint.y;
        }
      }
      this.startTextEditing({
        sceneX,
        sceneY,
        insertAtParentCenter: !event.altKey,
        container,
      });
    }
  };

  private getElementLinkAtPosition = (
    scenePointer: Readonly<{ x: number; y: number }>,
    hitElement: NonDeletedExcalidrawElement | null,
  ): ExcalidrawElement | undefined => {

Review comment (suggestion):
I don't like this.

Didn't know of this TS behavior. Can be fixed by changing the return type of `hasBoundTextElement` to:

```
: element is MarkNonNullable<ExcalidrawBindableElement, "boundElements">
```

Code after:
              ...prevState,
              editingGroupId: selectedGroupId,
              selectedElementIds: { [hitElement!.id]: true },
              selectedGroupIds: {},
            },
            this.scene.getNonDeletedElements(),
          ),
        );
        return;
      }
    }

    resetCursor(this.canvas);
    if (!event[KEYS.CTRL_OR_CMD] && !this.state.viewModeEnabled) {
      const container = getTextBindableContainerAtPosition(
        this.scene.getNonDeletedElements(),
        this.state,
        sceneX,
        sceneY,
      );
      if (container) {
        if (
          isArrowElement(container) ||
          hasBoundTextElement(container) ||
          !isTransparent(container.backgroundColor) ||
          isHittingElementNotConsideringBoundingBox(container, this.state, [
            sceneX,
            sceneY,
          ])
        ) {
          const midPoint = getContainerCenter(container, this.state);

          sceneX = midPoint.x;
          sceneY = midPoint.y;
        }
      }
      this.startTextEditing({
        sceneX,
        sceneY,
        insertAtParentCenter: !event.altKey,
        container,
      });
    }
  };

  private getElementLinkAtPosition = (
    scenePointer: Readonly<{ x: number; y: number }>,
    hitElement: NonDeletedExcalidrawElement | null,
  ): ExcalidrawElement | undefined => {
    // Reversing so we traverse the elements in decreasing order
    // of z-index
Repository: excalidraw/excalidraw
File: src/element/textElement.ts (TypeScript)

Code before:
      isHittingElementNotConsideringBoundingBox(elements[index], appState, [
        x,
        y,
      ])
    ) {
      hitElement = elements[index];
      break;
    } else if (x1 < x && x < x2 && y1 < y && y < y2) {
      hitElement = elements[index];
      break;
    }
  }

  return isTextBindableContainer(hitElement, false) ? hitElement : null;
};

const VALID_CONTAINER_TYPES = [
  "rectangle",
  "ellipse",
  "diamond",
  "image",
  "arrow",
];

export const isValidTextContainer = (element: ExcalidrawElement) =>
  VALID_CONTAINER_TYPES.includes(element.type);

export const computeContainerHeightForBoundText = (
  boundTextElementHeight: number,
  containerType: typeof VALID_CONTAINER_TYPES[number],
) => {
  if (containerType === "ellipse") {
    return Math.round(
      ((boundTextElementHeight + BOUND_TEXT_PADDING * 2) / Math.sqrt(2)) * 2,
    );
  }
  if (containerType === "arrow") {
    return boundTextElementHeight + BOUND_TEXT_PADDING * 8 * 2;
  }
  if (containerType === "diamond") {
    return 2 * (boundTextElementHeight + BOUND_TEXT_PADDING * 2);
  }
  return boundTextElementHeight + BOUND_TEXT_PADDING * 2;
};

export const computeContainerWidthForBoundText = (
  boundTextElementWidth: number,
  containerType: typeof VALID_CONTAINER_TYPES[number],
) => {
  if (containerType === "ellipse") {
    return Math.round(

Review comment (performance):
```suggestion
const VALID_CONTAINER_TYPES = new Set([
  "rectangle",
  "ellipse",
  "diamond",
  "image",
  "arrow",
]);

export const isValidTextContainer = (element: ExcalidrawElement) =>
  VALID_CONTAINER_TYPES.has(element.type);
```

Set has a time complexity o(1)

Code after:
        x,
        y,
      ])
    ) {
      hitElement = elements[index];
      break;
    } else if (x1 < x && x < x2 && y1 < y && y < y2) {
      hitElement = elements[index];
      break;
    }
  }

  return isTextBindableContainer(hitElement, false) ? hitElement : null;
};

const VALID_CONTAINER_TYPES = new Set([
  "rectangle",
  "ellipse",
  "diamond",
  "image",
  "arrow",
]);

export const isValidTextContainer = (element: ExcalidrawElement) =>
  VALID_CONTAINER_TYPES.has(element.type);

export const computeContainerDimensionForBoundText = (
  dimension: number,
  containerType: ExtractSetType<typeof VALID_CONTAINER_TYPES>,
) => {
  dimension = Math.ceil(dimension);
  const padding = BOUND_TEXT_PADDING * 2;

  if (containerType === "ellipse") {
    return Math.round(((dimension + padding) / Math.sqrt(2)) * 2);
  }
  if (containerType === "arrow") {
    return dimension + padding * 8;
  }
  if (containerType === "diamond") {
    return 2 * (dimension + padding);
  }
  return dimension + padding;
};

export const getMaxContainerWidth = (container: ExcalidrawElement) => {
  const width = getContainerDims(container).width;
  if (isArrowElement(container)) {
    const containerWidth = width - BOUND_TEXT_PADDING * 8 * 2;
    if (containerWidth <= 0) {
      const boundText = getBoundTextElement(container);
Repository: excalidraw/excalidraw
File: src/element/textElement.ts (TypeScript)

Code before:
      isArrowElement(elements[index]) &&
      isHittingElementNotConsideringBoundingBox(elements[index], appState, [
        x,
        y,
      ])
    ) {
      hitElement = elements[index];
      break;
    } else if (x1 < x && x < x2 && y1 < y && y < y2) {
      hitElement = elements[index];
      break;
    }
  }

  return isTextBindableContainer(hitElement, false) ? hitElement : null;
};

const VALID_CONTAINER_TYPES = new Set([
  "rectangle",
  "ellipse",
  "diamond",
  "image",
  "arrow",
]);

export type ValidContainerType<T> = T extends Set<infer U> ? U : never;

export const isValidTextContainer = (element: ExcalidrawElement) =>
  VALID_CONTAINER_TYPES.has(element.type);

export const computeContainerDimensionForBoundText = (
  dimension: number,
  containerType: ValidContainerType<typeof VALID_CONTAINER_TYPES>,
) => {
  dimension = Math.ceil(dimension);
  const padding = BOUND_TEXT_PADDING * 2;

  if (containerType === "ellipse") {
    return Math.round(((dimension + padding) / Math.sqrt(2)) * 2);
  }
  if (containerType === "arrow") {
    return dimension + padding * 8;
  }
  if (containerType === "diamond") {
    return 2 * (dimension + padding);
  }
  return dimension + padding;
};

export const getMaxContainerWidth = (container: ExcalidrawElement) => {
  const width = getContainerDims(container).width;

Review comment (refactor):
let's move this to `src/global.d.ts` and make into a utility type. And could be slightly improved:

```ts
type ExtractSetType<T extends Set<any>> = T extends Set<infer U> ? U : never;
```



Code after:
      isArrowElement(elements[index]) &&
      isHittingElementNotConsideringBoundingBox(elements[index], appState, [
        x,
        y,
      ])
    ) {
      hitElement = elements[index];
      break;
    } else if (x1 < x && x < x2 && y1 < y && y < y2) {
      hitElement = elements[index];
      break;
    }
  }

  return isTextBindableContainer(hitElement, false) ? hitElement : null;
};

const VALID_CONTAINER_TYPES = new Set([
  "rectangle",
  "ellipse",
  "diamond",
  "image",
  "arrow",
]);

export const isValidTextContainer = (element: ExcalidrawElement) =>
  VALID_CONTAINER_TYPES.has(element.type);

export const computeContainerDimensionForBoundText = (
  dimension: number,
  containerType: ExtractSetType<typeof VALID_CONTAINER_TYPES>,
) => {
  dimension = Math.ceil(dimension);
  const padding = BOUND_TEXT_PADDING * 2;

  if (containerType === "ellipse") {
    return Math.round(((dimension + padding) / Math.sqrt(2)) * 2);
  }
  if (containerType === "arrow") {
    return dimension + padding * 8;
  }
  if (containerType === "diamond") {
    return 2 * (dimension + padding);
  }
  return dimension + padding;
};

export const getMaxContainerWidth = (container: ExcalidrawElement) => {
  const width = getContainerDims(container).width;
  if (isArrowElement(container)) {
    const containerWidth = width - BOUND_TEXT_PADDING * 8 * 2;
Repository: excalidraw/excalidraw
File: src/constants.ts (TypeScript)

Code before:
import cssVariables from "./css/variables.module.scss";
import { AppProps } from "./types";
import { FontFamilyValues } from "./element/types";

export const isDarwin = /Mac|iPod|iPhone|iPad/.test(navigator.platform);
export const isWindows = /^Win/.test(navigator.platform);
export const isAndroid = /\b(android)\b/i.test(navigator.userAgent);
export const isFirefox =
  "netscape" in window &&
  navigator.userAgent.indexOf("rv:") > 1 &&
  navigator.userAgent.indexOf("Gecko") > 1;
export const isChrome = navigator.userAgent.indexOf("Chrome") !== -1;
export const isSafari =
  !isChrome && navigator.userAgent.indexOf("Safari") !== -1;
// keeping function so it can be mocked in test
export const isBrave = () =>
  (navigator as any).brave &&
  (navigator as any).brave.isBrave &&
  (navigator as any).brave.isBrave.name === "isBrave";

export const APP_NAME = "Excalidraw";

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;


Review comment (suggestion):
```suggestion
export const isBrave = () =>
  (navigator as any).brave?.isBrave?.name === "isBrave";
```

Code after:
import cssVariables from "./css/variables.module.scss";
import { AppProps } from "./types";
import { FontFamilyValues } from "./element/types";

export const isDarwin = /Mac|iPod|iPhone|iPad/.test(navigator.platform);
export const isWindows = /^Win/.test(navigator.platform);
export const isAndroid = /\b(android)\b/i.test(navigator.userAgent);
export const isFirefox =
  "netscape" in window &&
  navigator.userAgent.indexOf("rv:") > 1 &&
  navigator.userAgent.indexOf("Gecko") > 1;
export const isChrome = navigator.userAgent.indexOf("Chrome") !== -1;
export const isSafari =
  !isChrome && navigator.userAgent.indexOf("Safari") !== -1;
// keeping function so it can be mocked in test
export const isBrave = () =>
  (navigator as any).brave?.isBrave?.name === "isBrave";

export const APP_NAME = "Excalidraw";

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 enum EVENT {
  COPY = "copy",
Repository: excalidraw/excalidraw
File: src/element/textElement.test.ts (TypeScript)

Code before:

  it("should work with emojis", () => {
    const text = "😀";
    const maxWidth = 1;
    const res = wrapText(text, font, maxWidth);
    expect(res).toBe("😀");
  });

  it("should show the text correctly when max width reached", () => {
    const text = "Hello😀";
    const maxWidth = 10;
    const res = wrapText(text, font, maxWidth);
    expect(res).toBe("H\ne\nl\nl\no\n😀");
  });

  describe("When text doesn't contain new lines", () => {
    const text = "Hello whats up";

    [
      {
        desc: "break all words when width of each word is less than container width",
        width: 80,
        res: `Hello \nwhats \nup`,
      },
      {
        desc: "break all characters when width of each character is less than container width",
        width: 25,
        res: `H
e
l
l
o
w
h
a
t
s
u
p`,
      },
      {
        desc: "break words as per the width",

        width: 140,
        res: `Hello whats \nup`,
      },
      {
        desc: "fit the container",

        width: 250,
        res: "Hello whats up",

Review comment (suggestion):
had to replace with single-line strings because the test relied on a trailing whitespace, which we're stripping on file save (or at least should be): https://github.com/excalidraw/excalidraw/blob/master/.editorconfig#L12

Code after:
    const res = wrapText(text, font, maxWidth);
    expect(res).toBe(text);
  });

  it("should work with emojis", () => {
    const text = "😀";
    const maxWidth = 1;
    const res = wrapText(text, font, maxWidth);
    expect(res).toBe("😀");
  });

  it("should show the text correctly when max width reached", () => {
    const text = "Hello😀";
    const maxWidth = 10;
    const res = wrapText(text, font, maxWidth);
    expect(res).toBe("H\ne\nl\nl\no\n😀");
  });

  describe("When text doesn't contain new lines", () => {
    const text = "Hello whats up";

    [
      {
        desc: "break all words when width of each word is less than container width",
        width: 80,
        res: `Hello \nwhats \nup`,
      },
      {
        desc: "break all characters when width of each character is less than container width",
        width: 25,
        res: `H
e
l
l
o
w
h
a
t
s
u
p`,
      },
      {
        desc: "break words as per the width",

        width: 140,
        res: `Hello whats \nup`,
      },
      {
        desc: "fit the container",
Repository: excalidraw/excalidraw
File: src/tests/clipboard.test.tsx (TypeScript)

Code before:
    containerId: container.id,
    originalText:
      "Excalidraw is a virtual opensource whiteboard for sketching hand-drawn like diagrams",
  };

  it("should fix ellipse bounding box", async () => {
    const data = JSON.stringify({
      type: "excalidraw/clipboard",
      elements: [container, textElement],
    });
    setClipboardText(data);
    pasteWithCtrlCmdShiftV();

    await waitFor(async () => {
      await sleep(1);
      expect(h.elements.length).toEqual(2);
      const container = h.elements[0];
      expect(container.height).toBe(368);
      expect(container.width).toBe(166);
    });
  });

  it("should fix diamond bounding box", async () => {
    const data = JSON.stringify({
      type: "excalidraw/clipboard",
      elements: [
        {
          ...container,
          type: "diamond",
        },
        textElement,
      ],
    });
    setClipboardText(data);
    pasteWithCtrlCmdShiftV();

    await waitFor(async () => {
      await sleep(1);
      expect(h.elements.length).toEqual(2);
      const container = h.elements[0];
      expect(container.height).toBe(770);
      expect(container.width).toBe(166);
    });
  });
});


Review comment (bug):
don't know, but I believe the previous 354 number was incorrect. There were some bugs related to previous implementation which could be it. Or not.

(applies to the `770` assertion below as well)

Code after:
    width: 154,
    height: 175,
    fontSize: 20,
    fontFamily: 1,
    text: "Excalidraw is a\nvirtual \nopensource \nwhiteboard for \nsketching \nhand-drawn like\ndiagrams",
    baseline: 168,
    textAlign: "center",
    verticalAlign: "middle",
    containerId: container.id,
    originalText:
      "Excalidraw is a virtual opensource whiteboard for sketching hand-drawn like diagrams",
  };

  it("should fix ellipse bounding box", async () => {
    const data = JSON.stringify({
      type: "excalidraw/clipboard",
      elements: [container, textElement],
    });
    setClipboardText(data);
    pasteWithCtrlCmdShiftV();

    await waitFor(async () => {
      await sleep(1);
      expect(h.elements.length).toEqual(2);
      const container = h.elements[0];
      expect(container.height).toBe(368);
      expect(container.width).toBe(166);
    });
  });

  it("should fix diamond bounding box", async () => {
    const data = JSON.stringify({
      type: "excalidraw/clipboard",
      elements: [
        {
          ...container,
          type: "diamond",
        },
        textElement,
      ],
    });
    setClipboardText(data);
    pasteWithCtrlCmdShiftV();

    await waitFor(async () => {
      await sleep(1);
      expect(h.elements.length).toEqual(2);
      const container = h.elements[0];
      expect(container.height).toBe(770);
      expect(container.width).toBe(166);
    });
Repository: excalidraw/excalidraw
File: src/element/textElement.ts (TypeScript)

Code before:
  // characters hence we assume that each letteris 10px
  if (isTestEnv()) {
    return width * 10;
  }
  return width;
};

export const getTextWidth = (text: string, font: FontString) => {
  const lines = text.replace(/\r\n?/g, "\n").split("\n");
  let width = 0;
  lines.forEach((line) => {
    width = Math.max(width, getLineWidth(line, font));
  });
  return width;
};

export const getTextHeight = (text: string, font: FontString) => {
  const lines = text.replace(/\r\n?/g, "\n").split("\n");
  const lineHeight = getApproxLineHeight(font);
  return lineHeight * lines.length;
};

export const wrapText = (text: string, font: FontString, maxWidth: number) => {
  if (!maxWidth) {
    return text;
  }

  const lines: Array<string> = [];
  const originalLines = text.split("\n");
  const spaceWidth = getLineWidth(" ", font);

  let currentLine = "";
  let currentLineWidthTillNow = 0;

  const push = (str: string) => {
    if (str.trim()) {
      lines.push(str);
    }
  };

  const resetParams = () => {
    currentLine = "";
    currentLineWidthTillNow = 0;
  };

  originalLines.forEach((originalLine) => {
    const currentLineWidth = getTextWidth(originalLine, font);

    //Push the line if its <= maxWidth
    if (currentLineWidth <= maxWidth) {
      lines.push(originalLine);

Review comment (suggestion):
This looks like a hacky fix. The `width` of the element should never be computed to `NaN`, can we instead make sure that width is never set to `NaN` ?
cc @dwelle 

Code after:
  // characters hence we assume that each letteris 10px
  if (isTestEnv()) {
    return width * 10;
  }
  return width;
};

export const getTextWidth = (text: string, font: FontString) => {
  const lines = text.replace(/\r\n?/g, "\n").split("\n");
  let width = 0;
  lines.forEach((line) => {
    width = Math.max(width, getLineWidth(line, font));
  });
  return width;
};

export const getTextHeight = (text: string, font: FontString) => {
  const lines = text.replace(/\r\n?/g, "\n").split("\n");
  const lineHeight = getApproxLineHeight(font);
  return lineHeight * lines.length;
};

export const wrapText = (text: string, font: FontString, maxWidth: number) => {
  // if maxWidth is not finite or NaN which can happen in case of bugs in
  // computation, we need to make sure we don't continue as we'll end up
  // in an infinite loop
  if (!Number.isFinite(maxWidth) || maxWidth < 0) {
    return text;
  }

  const lines: Array<string> = [];
  const originalLines = text.split("\n");
  const spaceWidth = getLineWidth(" ", font);

  let currentLine = "";
  let currentLineWidthTillNow = 0;

  const push = (str: string) => {
    if (str.trim()) {
      lines.push(str);
    }
  };

  const resetParams = () => {
    currentLine = "";
    currentLineWidthTillNow = 0;
  };

  originalLines.forEach((originalLine) => {
    const currentLineWidth = getTextWidth(originalLine, font);
Repository: excalidraw/excalidraw
File: src/analytics.ts (TypeScript)

Code before:
export const trackEvent = (
  category: string,
  action: string,
  label?: string,
  value?: number,
) => {
  if (
    (typeof process !== "undefined" && process.env?.JEST_WORKER_ID) ||
    !window
  ) {
    return;
  }

  // Uncomment the next line to track locally
  // console.log("Track Event", { category, action, label, value });

  if (process.env?.REACT_APP_GOOGLE_ANALYTICS_ID && window.gtag) {
    try {
      window.gtag("event", action, {
        event_category: category,
        event_label: label,
        value,
      });
    } catch (error) {
      console.error("error logging to ga", error);
    }
  }

  // MATOMO event tracking _paq must be same as the one in index.html
  if (window._paq) {
    window._paq.push(["trackEvent", category, action, label, value]);
  }
};


Review comment (suggestion):
can't do `!window` if `window` is not available, hence the `typeof window`

Code after:
export const trackEvent = (
  category: string,
  action: string,
  label?: string,
  value?: number,
) => {
  try {
    // Uncomment the next line to track locally
    // console.log("Track Event", { category, action, label, value });

    if (typeof window === "undefined" || process.env.JEST_WORKER_ID) {
      return;
    }

    if (process.env.REACT_APP_GOOGLE_ANALYTICS_ID && window.gtag) {
      window.gtag("event", action, {
        event_category: category,
        event_label: label,
        value,
      });
    }

    // MATOMO event tracking _paq must be same as the one in index.html
    if (window._paq) {
      window._paq.push(["trackEvent", category, action, label, value]);
    }
  } catch (error) {
    console.error("error during analytics", error);
  }
};
Repository: excalidraw/excalidraw
File: src/analytics.ts (TypeScript)

Code before:
export const trackEvent = (
  category: string,
  action: string,
  label?: string,
  value?: number,
) => {
  try {
    // Uncomment the next line to track locally
    // console.log("Track Event", { category, action, label, value });

    if (
      typeof process === "undefined" ||
      typeof window === "undefined" ||
      process.env?.JEST_WORKER_ID
    ) {
      return;
    }

    if (process.env?.REACT_APP_GOOGLE_ANALYTICS_ID && window.gtag) {
      window.gtag("event", action, {
        event_category: category,
        event_label: label,
        value,
      });
    }

    // MATOMO event tracking _paq must be same as the one in index.html
    if (window._paq) {
      window._paq.push(["trackEvent", category, action, label, value]);
    }
  } catch (error) {
    console.error("error during analytics", error);
  }
};


Review comment (suggestion):
@ad1992 how is this check that was introduce two years ago working? `process.env.*` is interpolated during build. `process` will always be undefined. I wonder how are we getting any tracking data at all since this can't be working (and it's not).

Code after:
export const trackEvent = (
  category: string,
  action: string,
  label?: string,
  value?: number,
) => {
  try {
    // Uncomment the next line to track locally
    // console.log("Track Event", { category, action, label, value });

    if (typeof window === "undefined" || process.env.JEST_WORKER_ID) {
      return;
    }

    if (process.env.REACT_APP_GOOGLE_ANALYTICS_ID && window.gtag) {
      window.gtag("event", action, {
        event_category: category,
        event_label: label,
        value,
      });
    }

    // MATOMO event tracking _paq must be same as the one in index.html
    if (window._paq) {
      window._paq.push(["trackEvent", category, action, label, value]);
    }
  } catch (error) {
    console.error("error during analytics", error);
  }
};