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

Code before:
      textElement = selectedElements[0];
      container = selectedElements[1];
    } else {
      textElement = selectedElements[1] as ExcalidrawTextElement;
      container = selectedElements[0] as ExcalidrawTextContainer;
    }
    mutateElement(textElement!, {
      containerId: container.id,
      verticalAlign: VERTICAL_ALIGN.MIDDLE,
    });
    mutateElement(container, {
      boundElements: (container.boundElements || []).concat({
        type: "text",
        id: textElement.id,
      }),
    });
    redrawTextBoundingBox(textElement, container);
    const updatedElements = elements.slice();
    const textElementIndex = updatedElements.findIndex(
      (ele) => ele.id === textElement.id,
    );
    updatedElements.splice(textElementIndex, 1);
    const containerIndex = updatedElements.findIndex(
      (ele) => ele.id === container!.id,
    );
    updatedElements.splice(containerIndex + 1, 0, textElement!);
    return {
      elements: updatedElements,
      appState: { ...appState, selectedElementIds: { [container.id]: true } },
      commitToHistory: true,
    };
  },
});


Review comment (suggestion):
```suggestion
    updatedElements.splice(containerIndex + 1, 0, textElement);
```

Code after:
        isTextElement(selectedElements[1]);

      let bindingContainer;
      if (isTextBindableContainer(selectedElements[0])) {
        bindingContainer = selectedElements[0];
      } else if (isTextBindableContainer(selectedElements[1])) {
        bindingContainer = selectedElements[1];
      }
      if (
        textElement &&
        bindingContainer &&
        getBoundTextElement(bindingContainer) === null
      ) {
        return true;
      }
    }
    return false;
  },
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
    );

    let textElement: ExcalidrawTextElement;
    let container: ExcalidrawTextContainer;

    if (
      isTextElement(selectedElements[0]) &&
      isTextBindableContainer(selectedElements[1])
    ) {
      textElement = selectedElements[0];
      container = selectedElements[1];
    } else {
      textElement = selectedElements[1] as ExcalidrawTextElement;
      container = selectedElements[0] as ExcalidrawTextContainer;
    }
    mutateElement(textElement, {
      containerId: container.id,
      verticalAlign: VERTICAL_ALIGN.MIDDLE,
    });
    mutateElement(container, {
      boundElements: (container.boundElements || []).concat({
        type: "text",
        id: textElement.id,
      }),
    });
    redrawTextBoundingBox(textElement, container);
    const updatedElements = elements.slice();
    const textElementIndex = updatedElements.findIndex(
      (ele) => ele.id === textElement.id,
Repository: excalidraw/excalidraw
File: src/actions/actionBoundText.tsx (TypeScript)

Code before:
      isTextBindableContainer(selectedElements[1])
    ) {
      textElement = selectedElements[0];
      container = selectedElements[1];
    } else {
      textElement = selectedElements[1] as ExcalidrawTextElement;
      container = selectedElements[0] as ExcalidrawTextContainer;
    }
    mutateElement(textElement!, {
      containerId: container.id,
      verticalAlign: VERTICAL_ALIGN.MIDDLE,
    });
    mutateElement(container, {
      boundElements: (container.boundElements || []).concat({
        type: "text",
        id: textElement.id,
      }),
    });
    redrawTextBoundingBox(textElement, container);
    const updatedElements = elements.slice();
    const textElementIndex = updatedElements.findIndex(
      (ele) => ele.id === textElement.id,
    );
    updatedElements.splice(textElementIndex, 1);
    const containerIndex = updatedElements.findIndex(
      (ele) => ele.id === container!.id,
    );
    updatedElements.splice(containerIndex + 1, 0, textElement!);
    return {
      elements: updatedElements,
      appState: { ...appState, selectedElementIds: { [container.id]: true } },
      commitToHistory: true,
    };
  },
});


Review comment (suggestion):
```suggestion
      (ele) => ele.id === container.id,
```

Code after:
      const textElement =
        isTextElement(selectedElements[0]) ||
        isTextElement(selectedElements[1]);

      let bindingContainer;
      if (isTextBindableContainer(selectedElements[0])) {
        bindingContainer = selectedElements[0];
      } else if (isTextBindableContainer(selectedElements[1])) {
        bindingContainer = selectedElements[1];
      }
      if (
        textElement &&
        bindingContainer &&
        getBoundTextElement(bindingContainer) === null
      ) {
        return true;
      }
    }
    return false;
  },
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
    );

    let textElement: ExcalidrawTextElement;
    let container: ExcalidrawTextContainer;

    if (
      isTextElement(selectedElements[0]) &&
      isTextBindableContainer(selectedElements[1])
    ) {
      textElement = selectedElements[0];
      container = selectedElements[1];
    } else {
      textElement = selectedElements[1] as ExcalidrawTextElement;
      container = selectedElements[0] as ExcalidrawTextContainer;
    }
    mutateElement(textElement, {
      containerId: container.id,
      verticalAlign: VERTICAL_ALIGN.MIDDLE,
    });
    mutateElement(container, {
      boundElements: (container.boundElements || []).concat({
        type: "text",
        id: textElement.id,
      }),
    });
    redrawTextBoundingBox(textElement, container);
    const updatedElements = elements.slice();
Repository: excalidraw/excalidraw
File: src/actions/actionBoundText.tsx (TypeScript)

Code before:
  },
});

export const actionBindText = register({
  name: "bindText",
  contextItemLabel: "labels.bindText",
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
    );

    let textElement: ExcalidrawTextElement;
    let container: ExcalidrawTextContainer;

    if (
      isTextElement(selectedElements[0]) &&
      isTextBindableContainer(selectedElements[1])
    ) {
      textElement = selectedElements[0];
      container = selectedElements[1];
    } else {
      textElement = selectedElements[1] as ExcalidrawTextElement;
      container = selectedElements[0] as ExcalidrawTextContainer;
    }
    mutateElement(textElement!, {
      containerId: container.id,
      verticalAlign: VERTICAL_ALIGN.MIDDLE,
    });
    mutateElement(container, {
      boundElements: (container.boundElements || []).concat({
        type: "text",
        id: textElement.id,
      }),
    });
    redrawTextBoundingBox(textElement, container);
    const updatedElements = elements.slice();
    const textElementIndex = updatedElements.findIndex(
      (ele) => ele.id === textElement.id,
    );
    updatedElements.splice(textElementIndex, 1);
    const containerIndex = updatedElements.findIndex(
      (ele) => ele.id === container!.id,
    );
    updatedElements.splice(containerIndex + 1, 0, textElement!);
    return {
      elements: updatedElements,
      appState: { ...appState, selectedElementIds: { [container.id]: true } },
      commitToHistory: true,
    };
  },

Review comment (suggestion):
```suggestion
    mutateElement(textElement, {
```

Code after:
      }
    });
    return {
      elements,
      appState,
      commitToHistory: true,
    };
  },
});

export const actionBindText = register({
  name: "bindText",
  contextItemLabel: "labels.bindText",
  contextItemPredicate: (elements, appState) => {
    const selectedElements = getSelectedElements(elements, appState);

    if (selectedElements.length === 2) {
      const textElement =
        isTextElement(selectedElements[0]) ||
        isTextElement(selectedElements[1]);

      let bindingContainer;
      if (isTextBindableContainer(selectedElements[0])) {
        bindingContainer = selectedElements[0];
      } else if (isTextBindableContainer(selectedElements[1])) {
        bindingContainer = selectedElements[1];
      }
      if (
        textElement &&
        bindingContainer &&
        getBoundTextElement(bindingContainer) === null
      ) {
        return true;
      }
    }
    return false;
  },
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
    );

    let textElement: ExcalidrawTextElement;
    let container: ExcalidrawTextContainer;

    if (
      isTextElement(selectedElements[0]) &&
      isTextBindableContainer(selectedElements[1])
    ) {
      textElement = selectedElements[0];
Repository: excalidraw/excalidraw
File: src/actions/actionCanvas.tsx (TypeScript)

Code before:
    <div style={{ marginInlineStart: "0.25rem" }}>
      <DarkModeToggle
        value={appState.theme}
        onChange={(theme) => {
          updateData(theme);
        }}
      />
    </div>
  ),
  keyTest: (event) => event.altKey && event.shiftKey && event.code === CODES.D,
});

export const actionErase = register({
  name: "eraser",
  perform: (elements, appState) => {
    return {
      appState: {
        ...appState,
        selectedElementIds: {},
        selectedGroupIds: {},
        activeTool: { type: isEraserActive(appState) ? "selection" : "eraser" },
      },
      commitToHistory: true,
    };
  },
  keyTest: (event) => event.key === KEYS.E,
  PanelComponent: ({ elements, appState, updateData, data }) => (
    <ToolButton
      type="button"
      icon={eraser}
      className={clsx("eraser", { active: isEraserActive(appState) })}
      title={`${t("toolBar.eraser")}-${getShortcutKey("E")}`}
      aria-label={t("toolBar.eraser")}
      onClick={() => {
        updateData(null);
      }}
      size={data?.size || "medium"}
    ></ToolButton>
  ),
});


Review comment (suggestion):
```suggestion
            appState.activeTool.type === "eraser" 
```
This comment can be removed as its a typescript thingy

Code after:
    <div style={{ marginInlineStart: "0.25rem" }}>
      <DarkModeToggle
        value={appState.theme}
        onChange={(theme) => {
          updateData(theme);
        }}
      />
    </div>
  ),
  keyTest: (event) => event.altKey && event.shiftKey && event.code === CODES.D,
});

export const actionErase = register({
  name: "eraser",
  perform: (elements, appState) => {
    return {
      appState: {
        ...appState,
        selectedElementIds: {},
        selectedGroupIds: {},
        activeTool: {
          type: isEraserActive(appState)
            ? appState.activeTool.lastActiveToolBeforeEraser ?? "selection"
            : "eraser",
          lastActiveToolBeforeEraser:
            appState.activeTool.type === "eraser" //node throws incorrect type error when using isEraserActive()
              ? undefined
              : appState.activeTool.type,
        },
      },
      commitToHistory: true,
    };
  },
  keyTest: (event, appState) => {
    return (
      event.key === KEYS.E ||
      (event.key === KEYS.ESCAPE && isEraserActive(appState))
    );
  },
  PanelComponent: ({ elements, appState, updateData, data }) => (
    <ToolButton
      type="button"
      icon={eraser}
      className={clsx("eraser", { active: isEraserActive(appState) })}
      title={`${t("toolBar.eraser")}-${getShortcutKey("E")}`}
      aria-label={t("toolBar.eraser")}
      onClick={() => {
        updateData(null);
      }}
      size={data?.size || "medium"}
    ></ToolButton>
Repository: excalidraw/excalidraw
File: src/components/App.tsx (TypeScript)

Code before:
  };

  removePointer = (event: React.PointerEvent<HTMLElement> | PointerEvent) => {
    if (touchTimeout) {
      this.resetContextMenuTimer();
    }

    gesture.pointers.delete(event.pointerId);
  };

  toggleLock = (source: "keyboard" | "ui" = "ui") => {
    if (!this.state.activeTool.locked) {
      trackEvent(
        "toolbar",
        "toggleLock",
        `${source} (${this.deviceType.isMobile ? "mobile" : "desktop"})`,
      );
    }
    this.setState((prevState) => {
      return {
        activeTool: {
          locked: !prevState.activeTool.locked,
          type: prevState.activeTool.locked
            ? "selection"
            : prevState.activeTool.type,
        },
      };
    });
  };

  togglePenMode = () => {
    this.setState((prevState) => {
      return {
        penMode: !prevState.penMode,
      };
    });
  };

  toggleZenMode = () => {
    this.actionManager.executeAction(actionToggleZenMode);
  };

  toggleStats = () => {
    this.actionManager.executeAction(actionToggleStats);
  };

  scrollToContent = (
    target:
      | ExcalidrawElement
      | readonly ExcalidrawElement[] = this.scene.getElements(),
  ) => {

Review comment (suggestion):
this overwrites `lastToolBeforeEraser` or what it's called. I'm thinking we should make that prop required (but nullable)

Code after:
  };

  removePointer = (event: React.PointerEvent<HTMLElement> | PointerEvent) => {
    if (touchTimeout) {
      this.resetContextMenuTimer();
    }

    gesture.pointers.delete(event.pointerId);
  };

  toggleLock = (source: "keyboard" | "ui" = "ui") => {
    if (!this.state.activeTool.locked) {
      trackEvent(
        "toolbar",
        "toggleLock",
        `${source} (${this.deviceType.isMobile ? "mobile" : "desktop"})`,
      );
    }
    this.setState((prevState) => {
      return {
        activeTool: {
          ...prevState.activeTool,
          locked: !prevState.activeTool.locked,
          type: prevState.activeTool.locked
            ? "selection"
            : prevState.activeTool.type,
        },
      };
    });
  };

  togglePenMode = () => {
    this.setState((prevState) => {
      return {
        penMode: !prevState.penMode,
      };
    });
  };

  toggleZenMode = () => {
    this.actionManager.executeAction(actionToggleZenMode);
  };

  toggleStats = () => {
    this.actionManager.executeAction(actionToggleStats);
  };

  scrollToContent = (
    target:
      | ExcalidrawElement
      | readonly ExcalidrawElement[] = this.scene.getElements(),
Repository: excalidraw/excalidraw
File: src/actions/actionClipboard.tsx (TypeScript)

Code before:
          }),
        },
        commitToHistory: false,
      };
    } catch (error: any) {
      console.error(error);
      return {
        appState: {
          ...appState,
          errorMessage: error.message,
        },
        commitToHistory: false,
      };
    }
  },
  contextItemLabel: "labels.copyAsPng",
  keyTest: (event) => event.code === CODES.C && event.altKey && event.shiftKey,
});

export const copyAllTextNodesAsText = register({
  name: "copyAllTextNodesAsText",
  trackEvent: { category: "element" },
  perform: (elements) => {
    const text = (
      getNonDeletedElements(elements) as ExcalidrawTextElement[]
    ).reduce((acc, element) => `${acc}${element.text}\n`, "");
    copyTextToSystemClipboard(text);
    return {
      commitToHistory: false,
    };
  },
  contextItemLabel: "labels.copyAllTextNodesAsText",
});


Review comment (suggestion):
this should be `false` since we are not including bound text elements

Code after:
        },
        commitToHistory: false,
      };
    } catch (error: any) {
      console.error(error);
      return {
        appState: {
          ...appState,
          errorMessage: error.message,
        },
        commitToHistory: false,
      };
    }
  },
  contextItemLabel: "labels.copyAsPng",
  keyTest: (event) => event.code === CODES.C && event.altKey && event.shiftKey,
});

export const copyAllTextNodesAsText = register({
  name: "copyAllTextNodesAsText",
  trackEvent: { category: "element" },
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
      true,
    );

    const text = selectedElements.reduce((acc, element) => {
      if (isTextElement(element)) {
        return `${acc}${element.text}\n\n`;
      }
      return acc;
    }, "");
    copyTextToSystemClipboard(text);
    return {
      commitToHistory: false,
    };
  },
  contextItemLabel: "labels.copyAllTextNodesAsText",
});
Repository: excalidraw/excalidraw
File: src/actions/actionClipboard.tsx (TypeScript)

Code before:
    } catch (error: any) {
      console.error(error);
      return {
        appState: {
          ...appState,
          errorMessage: error.message,
        },
        commitToHistory: false,
      };
    }
  },
  contextItemLabel: "labels.copyAsPng",
  keyTest: (event) => event.code === CODES.C && event.altKey && event.shiftKey,
});

export const copyAllTextNodesAsText = register({
  name: "copyAllTextNodesAsText",
  trackEvent: { category: "element" },
  perform: (elements) => {
    const text = (
      getNonDeletedElements(elements) as ExcalidrawTextElement[]
    ).reduce((acc, element) => `${acc}${element.text}\n`, "");
    copyTextToSystemClipboard(text);
    return {
      commitToHistory: false,
    };
  },
  contextItemLabel: "labels.copyAllTextNodesAsText",
});


Review comment (suggestion):
```suggestion
      if (isTextElement(element)) {
```
since we have a util already

Code after:
      console.error(error);
      return {
        appState: {
          ...appState,
          errorMessage: error.message,
        },
        commitToHistory: false,
      };
    }
  },
  contextItemLabel: "labels.copyAsPng",
  keyTest: (event) => event.code === CODES.C && event.altKey && event.shiftKey,
});

export const copyAllTextNodesAsText = register({
  name: "copyAllTextNodesAsText",
  trackEvent: { category: "element" },
  perform: (elements, appState) => {
    const selectedElements = getSelectedElements(
      getNonDeletedElements(elements),
      appState,
      true,
    );

    const text = selectedElements.reduce((acc, element) => {
      if (isTextElement(element)) {
        return `${acc}${element.text}\n\n`;
      }
      return acc;
    }, "");
    copyTextToSystemClipboard(text);
    return {
      commitToHistory: false,
    };
  },
  contextItemLabel: "labels.copyAllTextNodesAsText",
});
Repository: excalidraw/excalidraw
File: src/data/blob.ts (TypeScript)

Code before:
        file = createFile(ab, mimeType, file.name);
      }
    }
    // when the file is an image, make sure the extension corresponds to the
    // actual mimeType (this is an edge case, but happens sometime)
  } else if (isSupportedImageFile(file)) {
    const ab = await blobToArrayBuffer(file);
    const mimeType = getActualMimeTypeFromImage(ab);
    if (mimeType && mimeType !== file.type) {
      file = createFile(ab, mimeType, file.name);
    }
  }

  return file;
};

export const blobToArrayBuffer = (blob: Blob): Promise<ArrayBuffer> => {
  if ("arrayBuffer" in blob) {
    return blob.arrayBuffer();
  }
  // Safari
  return new Promise((resolve, reject) => {
    const reader = new FileReader();
    reader.onload = (event) => {
      if (!event.target?.result) {
        return reject(new Error("couldn't convert blob to ArrayBuffer"));
      }
      resolve(event.target.result as ArrayBuffer);
    };
    reader.readAsArrayBuffer(blob);
  });
};


Review comment (suggestion):
```suggestion
        return reject(new Error("Couldn't convert blob to ArrayBuffer"));
```

Code after:
        file = createFile(buffer, mimeType, file.name);
      }
    }
    // when the file is an image, make sure the extension corresponds to the
    // actual mimeType (this is an edge case, but happens sometime)
  } else if (isSupportedImageFile(file)) {
    const buffer = await blobToArrayBuffer(file);
    const mimeType = getActualMimeTypeFromImage(buffer);
    if (mimeType && mimeType !== file.type) {
      file = createFile(buffer, mimeType, file.name);
    }
  }

  return file;
};

export const blobToArrayBuffer = (blob: Blob): Promise<ArrayBuffer> => {
  if ("arrayBuffer" in blob) {
    return blob.arrayBuffer();
  }
  // Safari
  return new Promise((resolve, reject) => {
    const reader = new FileReader();
    reader.onload = (event) => {
      if (!event.target?.result) {
        return reject(new Error("Couldn't convert blob to ArrayBuffer"));
      }
      resolve(event.target.result as ArrayBuffer);
    };
    reader.readAsArrayBuffer(blob);
  });
};
Repository: excalidraw/excalidraw
File: src/excalidraw-app/app_constants.ts (TypeScript)

Code before:
// time constants (ms)
export const SAVE_TO_LOCAL_STORAGE_TIMEOUT = 300;
export const INITIAL_SCENE_UPDATE_TIMEOUT = 5000;
export const FILE_UPLOAD_TIMEOUT = 300;
export const LOAD_IMAGES_TIMEOUT = 500;
export const SYNC_FULL_SCENE_INTERVAL_MS = 20000;
export const SYNC_BROWSER_TABS_TIMEOUT = 50;
export const CURSOR_SYNC_TIMEOUT = 33; // ~30fps
export const DELETED_ELEMENT_TIMEOUT = 86_400_000;

export const FILE_UPLOAD_MAX_BYTES = 3 * 1024 * 1024; // 3 MiB
// 1 year (https://stackoverflow.com/a/25201898/927631)
export const FILE_CACHE_MAX_AGE_SEC = 31536000;

export const WS_EVENTS = {
  SERVER_VOLATILE: "server-volatile-broadcast",
  SERVER: "server-broadcast",
};

export enum WS_SCENE_EVENT_TYPES {
  INIT = "SCENE_INIT",
  UPDATE = "SCENE_UPDATE",
}

export const FIREBASE_STORAGE_PREFIXES = {
  shareLinkFiles: `/files/shareLinks`,
  collabFiles: `/files/rooms`,
};

export const ROOM_ID_BYTES = 10;

export const STORAGE_KEYS = {
  LOCAL_STORAGE_ELEMENTS: "excalidraw",
  LOCAL_STORAGE_APP_STATE: "excalidraw-state",

Review comment (suggestion):
```suggestion
export const DELETED_ELEMENT_TIMEOUT = 24 * 60 * 60 * 1000; // 1 day
```
So its clear whats this number for

Code after:
// time constants (ms)
export const SAVE_TO_LOCAL_STORAGE_TIMEOUT = 300;
export const INITIAL_SCENE_UPDATE_TIMEOUT = 5000;
export const FILE_UPLOAD_TIMEOUT = 300;
export const LOAD_IMAGES_TIMEOUT = 500;
export const SYNC_FULL_SCENE_INTERVAL_MS = 20000;
export const SYNC_BROWSER_TABS_TIMEOUT = 50;
export const CURSOR_SYNC_TIMEOUT = 33; // ~30fps
export const DELETED_ELEMENT_TIMEOUT = 24 * 60 * 60 * 1000; // 1 day

export const FILE_UPLOAD_MAX_BYTES = 3 * 1024 * 1024; // 3 MiB
// 1 year (https://stackoverflow.com/a/25201898/927631)
export const FILE_CACHE_MAX_AGE_SEC = 31536000;

export const WS_EVENTS = {
  SERVER_VOLATILE: "server-volatile-broadcast",
  SERVER: "server-broadcast",
};

export enum WS_SCENE_EVENT_TYPES {
  INIT = "SCENE_INIT",
  UPDATE = "SCENE_UPDATE",
}

export const FIREBASE_STORAGE_PREFIXES = {
  shareLinkFiles: `/files/shareLinks`,
  collabFiles: `/files/rooms`,
};

export const ROOM_ID_BYTES = 10;

export const STORAGE_KEYS = {
  LOCAL_STORAGE_ELEMENTS: "excalidraw",
  LOCAL_STORAGE_APP_STATE: "excalidraw-state",