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

Code before:
        size={data?.size || "medium"}
        disabled={isUndoStackEmpty}
        data-testid="button-undo"
      />
    );
  },
});

export const createRedoAction: ActionCreator = (history, store) => ({
  name: "redo",
  label: "buttons.redo",
  icon: RedoIcon,
  trackEvent: { category: "history" },
  viewMode: false,
  perform: (elements, appState, _, app) =>
    executeHistoryAction(app, appState, () =>
      history.redo(
        arrayToMap(elements) as SceneElementsMap, // TODO: #7348 refactor action manager to already include `SceneElementsMap`
        appState,
        store.snapshot,
      ),
    ),
  keyTest: (event) =>
    (event[KEYS.CTRL_OR_CMD] &&
      event.shiftKey &&
      (isInEnglishAlphabet(event.key) && event.key.toLowerCase() === KEYS.Z || !isInEnglishAlphabet(event.key) && event.code === CODES.Z)) ||
    (isWindows && event.ctrlKey && !event.shiftKey && (isInEnglishAlphabet(event.key) && event.key.toLowerCase() === KEYS.Y || !isInEnglishAlphabet(event.key) && event.code === CODES.Y)),
  PanelComponent: ({ updateData, data }) => {
    const { isRedoStackEmpty } = useEmitter(
      history.onHistoryChangedEmitter,
      new HistoryChangedEvent(
        history.isUndoStackEmpty,
        history.isRedoStackEmpty,
      ),
    );

    return (
      <ToolButton
        type="button"
        icon={RedoIcon}
        aria-label={t("buttons.redo")}
        onClick={updateData}
        size={data?.size || "medium"}
        disabled={isRedoStackEmpty}
        data-testid="button-redo"
      />
    );
  },
});


Review comment (suggestion):
Similarly, as above:
```suggestion
      (event.key.toLowerCase() === KEYS.Z || (!isAlphabetic(event.key) && event.code === CODES.Z))) ||
```

Code after:
        data-testid="button-undo"
      />
    );
  },
});

export const createRedoAction: ActionCreator = (history, store) => ({
  name: "redo",
  label: "buttons.redo",
  icon: RedoIcon,
  trackEvent: { category: "history" },
  viewMode: false,
  perform: (elements, appState, _, app) =>
    executeHistoryAction(app, appState, () =>
      history.redo(
        arrayToMap(elements) as SceneElementsMap, // TODO: #7348 refactor action manager to already include `SceneElementsMap`
        appState,
        store.snapshot,
      ),
    ),
  keyTest: (event) =>
    (event[KEYS.CTRL_OR_CMD] && event.shiftKey && matchKey(event, KEYS.Z)) ||
    (isWindows && event.ctrlKey && !event.shiftKey && matchKey(event, KEYS.Y)),
  PanelComponent: ({ updateData, data }) => {
    const { isRedoStackEmpty } = useEmitter(
      history.onHistoryChangedEmitter,
      new HistoryChangedEvent(
        history.isUndoStackEmpty,
        history.isRedoStackEmpty,
      ),
    );

    return (
      <ToolButton
        type="button"
        icon={RedoIcon}
        aria-label={t("buttons.redo")}
        onClick={updateData}
        size={data?.size || "medium"}
        disabled={isRedoStackEmpty}
        data-testid="button-redo"
      />
    );
  },
});
Repository: excalidraw/excalidraw
File: packages/excalidraw/actions/actionHistory.tsx (TypeScript)

Code before:
        disabled={isUndoStackEmpty}
        data-testid="button-undo"
      />
    );
  },
});

export const createRedoAction: ActionCreator = (history, store) => ({
  name: "redo",
  label: "buttons.redo",
  icon: RedoIcon,
  trackEvent: { category: "history" },
  viewMode: false,
  perform: (elements, appState, _, app) =>
    executeHistoryAction(app, appState, () =>
      history.redo(
        arrayToMap(elements) as SceneElementsMap, // TODO: #7348 refactor action manager to already include `SceneElementsMap`
        appState,
        store.snapshot,
      ),
    ),
  keyTest: (event) =>
    (event[KEYS.CTRL_OR_CMD] &&
      event.shiftKey &&
      (isInEnglishAlphabet(event.key) && event.key.toLowerCase() === KEYS.Z || !isInEnglishAlphabet(event.key) && event.code === CODES.Z)) ||
    (isWindows && event.ctrlKey && !event.shiftKey && (isInEnglishAlphabet(event.key) && event.key.toLowerCase() === KEYS.Y || !isInEnglishAlphabet(event.key) && event.code === CODES.Y)),
  PanelComponent: ({ updateData, data }) => {
    const { isRedoStackEmpty } = useEmitter(
      history.onHistoryChangedEmitter,
      new HistoryChangedEvent(
        history.isUndoStackEmpty,
        history.isRedoStackEmpty,
      ),
    );

    return (
      <ToolButton
        type="button"
        icon={RedoIcon}
        aria-label={t("buttons.redo")}
        onClick={updateData}
        size={data?.size || "medium"}
        disabled={isRedoStackEmpty}
        data-testid="button-redo"
      />
    );
  },
});


Review comment (suggestion):
```suggestion
    (isWindows && event.ctrlKey && !event.shiftKey && (event.key.toLowerCase() === KEYS.Y || (!isAlphabetic(event.key) && event.code === CODES.Y))),
```

Code after:
      />
    );
  },
});

export const createRedoAction: ActionCreator = (history, store) => ({
  name: "redo",
  label: "buttons.redo",
  icon: RedoIcon,
  trackEvent: { category: "history" },
  viewMode: false,
  perform: (elements, appState, _, app) =>
    executeHistoryAction(app, appState, () =>
      history.redo(
        arrayToMap(elements) as SceneElementsMap, // TODO: #7348 refactor action manager to already include `SceneElementsMap`
        appState,
        store.snapshot,
      ),
    ),
  keyTest: (event) =>
    (event[KEYS.CTRL_OR_CMD] && event.shiftKey && matchKey(event, KEYS.Z)) ||
    (isWindows && event.ctrlKey && !event.shiftKey && matchKey(event, KEYS.Y)),
  PanelComponent: ({ updateData, data }) => {
    const { isRedoStackEmpty } = useEmitter(
      history.onHistoryChangedEmitter,
      new HistoryChangedEvent(
        history.isUndoStackEmpty,
        history.isRedoStackEmpty,
      ),
    );

    return (
      <ToolButton
        type="button"
        icon={RedoIcon}
        aria-label={t("buttons.redo")}
        onClick={updateData}
        size={data?.size || "medium"}
        disabled={isRedoStackEmpty}
        data-testid="button-redo"
      />
    );
  },
});
Repository: excalidraw/excalidraw
File: packages/excalidraw/data/index.ts (TypeScript)

Code before:
  },
) => {
  if (elements.length === 0) {
    throw new Error(t("alerts.cannotExportEmptyCanvas"));
  }
  if (type === "svg" || type === "clipboard-svg") {
    const svgPromise = exportToSvg(
      elements,
      {
        exportBackground,
        exportWithDarkMode: appState.exportWithDarkMode,
        viewBackgroundColor,
        exportPadding,
        exportScale: appState.exportScale,
        exportEmbedScene: appState.exportEmbedScene && type === "svg",
      },
      files,
      { exportingFrame },
    );

    if (type === "svg") {
      return fileSave(
        svgPromise.then((svg) => {
          return new Blob([SVG_DOCUMENT_PREAMBLE + svg.outerHTML], {
            type: MIME_TYPES.svg,
          });
        }),
        {
          description: "Export to SVG",
          name,
          extension: appState.exportEmbedScene ? "excalidraw.svg" : "svg",
          mimeTypes: [IMAGE_MIME_TYPES.svg],
          fileHandle,
        },
      );
    } else if (type === "clipboard-svg") {
      const svg = await svgPromise.then((svg) => svg.outerHTML);
      try {
        await copyTextToSystemClipboard(svg);
      } catch (e) {
        throw new Error(t("errors.copyToSystemClipboardFailed"));
      }
      return;
    }
  }

  const tempCanvas = exportToCanvas(elements, appState, files, {
    exportBackground,
    viewBackgroundColor,
    exportPadding,
    exportingFrame,

Review comment (suggestion):
```suggestion
          // adding SVG preamble so that older software parse the SVG file
          // properly
          return new Blob([SVG_DOCUMENT_PREAMBLE + svg.outerHTML], {
```

Code after:
  },
) => {
  if (elements.length === 0) {
    throw new Error(t("alerts.cannotExportEmptyCanvas"));
  }
  if (type === "svg" || type === "clipboard-svg") {
    const svgPromise = exportToSvg(
      elements,
      {
        exportBackground,
        exportWithDarkMode: appState.exportWithDarkMode,
        viewBackgroundColor,
        exportPadding,
        exportScale: appState.exportScale,
        exportEmbedScene: appState.exportEmbedScene && type === "svg",
      },
      files,
      { exportingFrame },
    );

    if (type === "svg") {
      return fileSave(
        svgPromise.then((svg) => {
          // adding SVG preamble so that older software parse the SVG file
          // properly
          return new Blob([SVG_DOCUMENT_PREAMBLE + svg.outerHTML], {
            type: MIME_TYPES.svg,
          });
        }),
        {
          description: "Export to SVG",
          name,
          extension: appState.exportEmbedScene ? "excalidraw.svg" : "svg",
          mimeTypes: [IMAGE_MIME_TYPES.svg],
          fileHandle,
        },
      );
    } else if (type === "clipboard-svg") {
      const svg = await svgPromise.then((svg) => svg.outerHTML);
      try {
        await copyTextToSystemClipboard(svg);
      } catch (e) {
        throw new Error(t("errors.copyToSystemClipboardFailed"));
      }
      return;
    }
  }

  const tempCanvas = exportToCanvas(elements, appState, files, {
    exportBackground,
    viewBackgroundColor,
Repository: excalidraw/excalidraw
File: packages/excalidraw/eraser/index.ts (TypeScript)

Code before:
            this.elementsToErase.add(element.containerId);
          }

          this.elementsToErase.add(element.id);
        }
      }
    }

    return Array.from(this.elementsToErase);
  }

  endPath(): void {
    super.endPath();
    super.clearTrails();
    this.elementsToErase.clear();
    this.groupsToErase.clear();
  }
}

const eraserTest = (
  pathSegment: LineSegment<GlobalPoint>,
  element: ExcalidrawElement,
  elementsMap: ElementsMap,
): boolean => {
  const lastPoint = pathSegment[1];
  const boundBoxDiagonal = Math.hypot(element.width, element.height);
  const threshold = 1;
  if (
    shouldTestInside(element) &&
    isPointInElement(lastPoint, element, elementsMap)
  ) {
    return true;
  }

  // for freedraw-points, lines and arrows of shared endpoints
  if (boundBoxDiagonal < threshold) {
    const tolerance =
      element.type === "freedraw"
        ? element.strokeWidth * 2.125
        : element.strokeWidth / 2;
    const [cx, cy] = [
      element.x + element.width / 2,
      element.y + element.height / 2,
    ];
    const distance = Math.hypot(lastPoint[0] - cx, lastPoint[1] - cy);
    const hitRadius = Math.max(element.width, element.height) / 2 + tolerance;

    if (distance <= hitRadius) {
      return true;
    }
  }

Review comment (suggestion):
There are idiomatic bounding box checking functions in the `bounds.ts` file, specifically `doBoundsIntersect()`. Please use that here.

You can see an example of early bailout in `collision.ts`'s `bindingBorderTest()` function.

Code after:
              this.app.scene.getNonDeletedElementsMap(),
              shallowestGroupId,
            );

            for (const elementInGroup of elementsInGroup) {
              this.elementsToErase.add(elementInGroup.id);
            }
            this.groupsToErase.add(shallowestGroupId);
          }

          if (hasBoundTextElement(element)) {
            const boundText = getBoundTextElementId(element);

            if (boundText) {
              this.elementsToErase.add(boundText);
            }
          }

          if (isBoundToContainer(element)) {
            this.elementsToErase.add(element.containerId);
          }

          this.elementsToErase.add(element.id);
        }
      }
    }

    return Array.from(this.elementsToErase);
  }

  endPath(): void {
    super.endPath();
    super.clearTrails();
    this.elementsToErase.clear();
    this.groupsToErase.clear();
  }
}

const eraserTest = (
  pathSegment: LineSegment<GlobalPoint>,
  element: ExcalidrawElement,
  elementsMap: ElementsMap,
  zoom: number,
): boolean => {
  const lastPoint = pathSegment[1];

  // PERF: Do a quick bounds intersection test first because it's cheap
  const threshold = isFreeDrawElement(element) ? 15 : element.strokeWidth / 2;
  const segmentBounds = [
    Math.min(pathSegment[0][0], pathSegment[1][0]) - threshold,
    Math.min(pathSegment[0][1], pathSegment[1][1]) - threshold,
Repository: excalidraw/excalidraw
File: packages/common/src/binary-heap.ts (TypeScript)

Code before:
export class BinaryHeap<T> {
  private content: T[] = [];

  constructor(private scoreFunction: (node: T) => number) {}

  sinkDown(idx: number) {
    const node = this.content[idx];
    while (idx > 0) {
      const parentN = ((idx + 1) >> 1) - 1;
      const parent = this.content[parentN];
      if (this.scoreFunction(node) < this.scoreFunction(parent)) {
        this.content[parentN] = node;
        this.content[idx] = parent;
        idx = parentN; // TODO: Optimize
      } else {
        break;
      }
    }
  }

  bubbleUp(idx: number) {
    const length = this.content.length;
    const node = this.content[idx];
    const score = this.scoreFunction(node);

    while (true) {
      const child2N = (idx + 1) << 1;
      const child1N = child2N - 1;
      let swap = null;
      let child1Score = 0;

      if (child1N < length) {
        const child1 = this.content[child1N];
        child1Score = this.scoreFunction(child1);
        if (child1Score < score) {
          swap = child1N;
        }
      }


Review comment (refactor):
~Wouldn't this override intermediate parents if there are multiple iterations needed? Since you no longer store the values in the intermediate "parent" slots, just at the last iteration, that is.~

~I understand that this is used only in push() and it will always work the way it is used, but then make this a private method and add a comment that this is ONLY correct if used with the last index. In fact, it might be better to move this whole function into push(). One less fn call and avoids the confusion completely.~

Nevermind, I was mistaken.

Code after:
export class BinaryHeap<T> {
  private content: T[] = [];

  constructor(private scoreFunction: (node: T) => number) {}

  sinkDown(idx: number) {
    const node = this.content[idx];
    const nodeScore = this.scoreFunction(node);
    while (idx > 0) {
      const parentN = ((idx + 1) >> 1) - 1;
      const parent = this.content[parentN];
      if (nodeScore < this.scoreFunction(parent)) {
        this.content[idx] = parent;
        idx = parentN; // TODO: Optimize
      } else {
        break;
      }
    }
    this.content[idx] = node;
  }

  bubbleUp(idx: number) {
    const length = this.content.length;
    const node = this.content[idx];
    const score = this.scoreFunction(node);

    while (true) {
      const child1N = ((idx + 1) << 1) - 1;
      const child2N = child1N + 1;
      let smallestIdx = idx;
      let smallestScore = score;

      // Check left child
      if (child1N < length) {
        const child1Score = this.scoreFunction(this.content[child1N]);
        if (child1Score < smallestScore) {
          smallestIdx = child1N;
          smallestScore = child1Score;
        }
Repository: excalidraw/excalidraw
File: packages/common/src/binary-heap.ts (TypeScript)

Code before:
        if (child1Score < score) {
          swap = child1N;
        }
      }

      if (child2N < length) {
        const child2 = this.content[child2N];
        const child2Score = this.scoreFunction(child2);
        if (child2Score < (swap === null ? score : child1Score)) {
          swap = child2N;
        }
      }

      if (swap !== null) {
        this.content[idx] = this.content[swap];
        this.content[swap] = node;
        idx = swap; // TODO: Optimize
      } else {
        break;
      }
    }
  }

  push(node: T) {
    this.content.push(node);
    this.sinkDown(this.content.length - 1);
  }

  pop(): T | null {
    if (this.content.length === 0) {
      return null;
    }

    const result = this.content[0];
    const end = this.content.pop()!;

    if (this.content.length > 0) {
      this.content[0] = end;
      this.bubbleUp(0);
    }

    return result;
  }

  remove(node: T) {
    if (this.content.length === 0) {
      return;
    }

    const i = this.content.indexOf(node);
    const end = this.content.pop()!;

Review comment (suggestion):
~Same iss as with sinkDown here too? This can't be moved into another funtion because remove uses it too, but I'm considering removing the `remove()` function since it is not used anyway. In fact, remove it and embed this in pop() so no confusion.~

Nevermind!

Code after:
        const child1Score = this.scoreFunction(this.content[child1N]);
        if (child1Score < smallestScore) {
          smallestIdx = child1N;
          smallestScore = child1Score;
        }
      }

      // Check right child
      if (child2N < length) {
        const child2Score = this.scoreFunction(this.content[child2N]);
        if (child2Score < smallestScore) {
          smallestIdx = child2N;
        }
      }

      if (smallestIdx === idx) {
        break;
      }

      // Move the smaller child up, continue finding position for node
      this.content[idx] = this.content[smallestIdx];
      idx = smallestIdx;
    }

    // Place node in its final position
    this.content[idx] = node;
  }

  push(node: T) {
    this.content.push(node);
    this.sinkDown(this.content.length - 1);
  }

  pop(): T | null {
    if (this.content.length === 0) {
      return null;
    }

    const result = this.content[0];
    const end = this.content.pop()!;

    if (this.content.length > 0) {
      this.content[0] = end;
      this.bubbleUp(0);
    }

    return result;
  }

  remove(node: T) {
    if (this.content.length === 0) {
Repository: excalidraw/excalidraw
File: packages/excalidraw/components/HintViewer.tsx (TypeScript)

Code before:
    const hints = getHints({
      appState,
      isMobile,
      device,
      app,
    });

    if (!hints) {
      return null;
    }

    const hint = Array.isArray(hints)
      ? hints
          .map((hint) => {
            return getShortcutKey(hint).replace(/\. ?$/, "");
          })
          .join(". ")
      : getShortcutKey(hints);

    const hintElement = React.createElement(
      React.Fragment,
      {},
      ...nodesFromTextWithTags(hint, {
        shortcut: (el) => <span className="HintViewer__key">{el}</span>,
      }),
    );

    return (
      <div className="HintViewer">
        <span>{hintElement}</span>
      </div>
    );
  },
);


Review comment (suggestion):
Why not just do this:

```jsx
const hint_JSX = hint
  .split(/(Space|Alt|Enter|Cmd|Option|Ctrl)/g)
  .map((part, index) => {
    if (index % 2 === 1) {
      return (
        <span key={index} className="HintViewer__key">
          {part}
        </span>
      );
    }
    return part;
  });

return (
  <div className="HintViewer">
    <span>{hint_JSX}</span>
  </div>
);
```

~~Simpler~~ faster and we don't need to modify the translation files.

Code after:

    if (selectedElements.length === 1) {
      if (isLinearElement(selectedElements[0])) {
        if (appState.selectedLinearElement?.isEditing) {
          return appState.selectedLinearElement.selectedPointsIndices
            ? t("hints.lineEditor_pointSelected", {
                shortcut_1: getTaggedShortcutKey("Delete"),
                shortcut_2: getTaggedShortcutKey(["CtrlOrCmd", "D"]),
              })
            : t("hints.lineEditor_nothingSelected", {
                shortcut_1: getTaggedShortcutKey("Shift"),
                shortcut_2: getTaggedShortcutKey("Alt"),
              });
        }
        return isLineElement(selectedElements[0])
          ? t("hints.lineEditor_line_info", {
              shortcut: getTaggedShortcutKey("Enter"),
            })
          : t("hints.lineEditor_info", {
              shortcut_1: getTaggedShortcutKey("CtrlOrCmd"),
              shortcut_2: getTaggedShortcutKey(["CtrlOrCmd", "Enter"]),
            });
      }
      if (
        !appState.newElement &&
        !appState.selectedElementsAreBeingDragged &&
        isTextBindableContainer(selectedElements[0])
      ) {
        const bindTextToElement = t("hints.bindTextToElement", {
          shortcut: getTaggedShortcutKey("Enter"),
        });
        const createFlowchart = t("hints.createFlowchart", {
          shortcut: getTaggedShortcutKey("CtrlOrCmd"),
        });
        if (isFlowchartNodeElement(selectedElements[0])) {
          if (
            isNodeInFlowchart(
              selectedElements[0],
              app.scene.getNonDeletedElementsMap(),
            )
          ) {
            return [bindTextToElement, createFlowchart];
          }

          return [bindTextToElement, createFlowchart];
        }

        return bindTextToElement;
      }
    }
  }
Repository: excalidraw/excalidraw
File: packages/element/src/binding.ts (TypeScript)

Code before:
          arrow.x + arrow.points[arrow.points.length - 1][0],
          arrow.y + arrow.points[arrow.points.length - 1][1],
        );

  return [startPoint, endPoint];
};

export const getArrowLocalFixedPoints = (
  arrow: ExcalidrawElbowArrowElement,
  elementsMap: ElementsMap,
) => {
  const [startPoint, endPoint] = getGlobalFixedPoints(arrow, elementsMap);

  return [
    LinearElementEditor.pointFromAbsoluteCoords(arrow, startPoint, elementsMap),
    LinearElementEditor.pointFromAbsoluteCoords(arrow, endPoint, elementsMap),
  ];
};

export const isFixedPoint = (
  fixedPoint: any,
): fixedPoint is FixedPointBinding["fixedPoint"] => {
  return (
    Array.isArray(fixedPoint) &&
    fixedPoint.length === 2 &&
    fixedPoint.every((coord) => typeof coord === "number")
  );
};

export const normalizeFixedPoint = <T extends FixedPoint>(
  fixedPoint: T,
): FixedPoint => {
  const point: FixedPoint = isFixedPoint(fixedPoint)
    ? fixedPoint
    : [0.5001, 0.5001];

  // Do not allow a precise 0.5 for fixed point ratio
  // to avoid jumping arrow heading due to floating point imprecision
  if (Math.abs(point[0] - 0.5) < 0.0001 || Math.abs(point[1] - 0.5) < 0.0001) {
    return point.map((ratio) =>
      Math.abs(ratio - 0.5) < 0.0001 ? 0.5001 : ratio,
    ) as FixedPoint;
  }

  return point;
};

type Side =
  | "top"
  | "top-right"
  | "right"

Review comment (suggestion):
```suggestion
    fixedPoint.every((coord) => coord && typeof coord === "number")
```
to guard against NaN if we're already doing it this granularly

Code after:
          arrow.x + arrow.points[arrow.points.length - 1][0],
          arrow.y + arrow.points[arrow.points.length - 1][1],
        );

  return [startPoint, endPoint];
};

export const getArrowLocalFixedPoints = (
  arrow: ExcalidrawElbowArrowElement,
  elementsMap: ElementsMap,
) => {
  const [startPoint, endPoint] = getGlobalFixedPoints(arrow, elementsMap);

  return [
    LinearElementEditor.pointFromAbsoluteCoords(arrow, startPoint, elementsMap),
    LinearElementEditor.pointFromAbsoluteCoords(arrow, endPoint, elementsMap),
  ];
};

export const isFixedPoint = (
  fixedPoint: any,
): fixedPoint is FixedPointBinding["fixedPoint"] => {
  return (
    Array.isArray(fixedPoint) &&
    fixedPoint.length === 2 &&
    fixedPoint.every((coord) => Number.isFinite(coord))
  );
};

export const normalizeFixedPoint = <T extends FixedPoint>(
  fixedPoint: T,
): FixedPoint => {
  if (!isFixedPoint(fixedPoint)) {
    return [0.5001, 0.5001];
  }

  const EPSILON = 0.0001;

  // Do not allow a precise 0.5 for fixed point ratio
  // to avoid jumping arrow heading due to floating point imprecision
  if (
    Math.abs(fixedPoint[0] - 0.5) < EPSILON ||
    Math.abs(fixedPoint[1] - 0.5) < EPSILON
  ) {
    return fixedPoint.map((ratio) =>
      Math.abs(ratio - 0.5) < EPSILON ? 0.5001 : ratio,
    ) as FixedPoint;
  }

  return fixedPoint;
};