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

Code before:
import clsx from "clsx";

// TODO: It might be "clever" to add option.icon to the existing component <ButtonSelect />
export const ButtonIconSelect = <T extends Object>({
  options,
  value,
  onChange,
  group,
}: {
  options: { value: T; text: string; icon: JSX.Element; testId?: string }[];
  value: T | null;
  onChange: (value: T) => void;
  group: string;
}) => (
  <div className="buttonList buttonListIcon">
    {options.map((option) => (
      <label
        key={option.text}
        className={clsx({ active: value === option.value })}
        title={option.text}
      >
        <input
          type="radio"
          name={group}
          onChange={() => onChange(option.value)}
          checked={value === option.value}
          data-testid={option.testId}
        />
        {option.icon}
      </label>
    ))}
  </div>
);


Review comment (suggestion):
we should move all buttons to `<button>`, away from radio inputs. But I didn't want to do it in this PR.

Code after:
    type?: "radio" | "button";
  } & (
    | { type?: "radio"; group: string; onChange: (value: T) => void }
    | {
        type: "button";
        onClick: (
          value: T,
          event: React.MouseEvent<HTMLButtonElement, MouseEvent>,
        ) => void;
      }
  ),
) => (
  <div className="buttonList buttonListIcon">
    {props.options.map((option) =>
      props.type === "button" ? (
        <button
          key={option.text}
          onClick={(event) => props.onClick(option.value, event)}
          className={clsx({
            active: option.active ?? props.value === option.value,
          })}
          data-testid={option.testId}
          title={option.text}
        >
          {option.icon}
        </button>
      ) : (
        <label
          key={option.text}
          className={clsx({ active: props.value === option.value })}
          title={option.text}
        >
          <input
            type="radio"
            name={props.group}
            onChange={() => props.onChange(option.value)}
            checked={props.value === option.value}
            data-testid={option.testId}
          />
          {option.icon}
        </label>
      ),
    )}
  </div>
);
Repository: excalidraw/excalidraw
File: src/element/newElement.ts (TypeScript)

Code before:
  | "version"
  | "versionNonce"
  | "link"
  | "strokeStyle"
  | "fillStyle"
  | "strokeColor"
  | "backgroundColor"
  | "roughness"
  | "strokeWidth"
  | "roundness"
  | "locked"
  | "opacity"
>;

const _newElementBase = <T extends ExcalidrawElement>(
  type: T["type"],
  {
    x,
    y,
    strokeColor = oc.black,
    backgroundColor = "transparent",
    fillStyle = "hachure",
    strokeWidth = 1,
    strokeStyle = "solid",
    roughness = 1,
    opacity = 100,
    width = 0,
    height = 0,
    angle = 0,
    groupIds = [],
    roundness = null,
    boundElements = null,
    link = null,
    locked = false,
    ...rest
  }: ElementConstructorOpts & Omit<Partial<ExcalidrawGenericElement>, "type">,
) => {
  // assign type to guard against excess properties
  const element: Merge<ExcalidrawGenericElement, { type: T["type"] }> = {
    id: rest.id || randomId(),
    type,
    x,
    y,
    width,
    height,
    angle,
    strokeColor,
    backgroundColor,
    fillStyle,
    strokeWidth,
    strokeStyle,

Review comment (suggestion):
I know it's copied from above, but since we're touching on this, we should reuse `getDefaultAppState()` values, or factor those values from the function return value into constants similarly as we do with `DEFAULT_TEXT_ALIGN`

Code after:
  | "version"
  | "versionNonce"
  | "link"
  | "strokeStyle"
  | "fillStyle"
  | "strokeColor"
  | "backgroundColor"
  | "roughness"
  | "strokeWidth"
  | "roundness"
  | "locked"
  | "opacity"
>;

const _newElementBase = <T extends ExcalidrawElement>(
  type: T["type"],
  {
    x,
    y,
    strokeColor = DEFAULT_ELEMENT_PROPS.strokeColor,
    backgroundColor = DEFAULT_ELEMENT_PROPS.backgroundColor,
    fillStyle = DEFAULT_ELEMENT_PROPS.fillStyle,
    strokeWidth = DEFAULT_ELEMENT_PROPS.strokeWidth,
    strokeStyle = DEFAULT_ELEMENT_PROPS.strokeStyle,
    roughness = DEFAULT_ELEMENT_PROPS.roughness,
    opacity = DEFAULT_ELEMENT_PROPS.opacity,
    width = 0,
    height = 0,
    angle = 0,
    groupIds = [],
    roundness = null,
    boundElements = null,
    link = null,
    locked = DEFAULT_ELEMENT_PROPS.locked,
    ...rest
  }: ElementConstructorOpts & Omit<Partial<ExcalidrawGenericElement>, "type">,
) => {
  // assign type to guard against excess properties
  const element: Merge<ExcalidrawGenericElement, { type: T["type"] }> = {
    id: rest.id || randomId(),
    type,
    x,
    y,
    width,
    height,
    angle,
    strokeColor,
    backgroundColor,
    fillStyle,
    strokeWidth,
    strokeStyle,
Repository: excalidraw/excalidraw
File: src/tests/binding.test.tsx (TypeScript)

Code before:
    expect(container).toEqual(
      expect.objectContaining({
        boundElements: expect.arrayContaining([
          {
            type: "text",
            id: text1.id,
          },
          {
            type: "arrow",
            id: arrow1.id,
          },
          {
            type: "arrow",
            id: arrow2.id,
          },
        ]),
      }),
    );

    expect(arrow1.startBinding?.elementId).toBe(rectangle1.id);
    expect(arrow1.endBinding?.elementId).toBe(container.id);
    expect(arrow2.startBinding?.elementId).toBe(container.id);
    expect(arrow2.endBinding?.elementId).toBe(rectangle1.id);
  });
  
  it("should text be centered when binding to a rectangle", async () => {
    const rectangle1 = API.createElement({
      type: "rectangle",
      id: "rectangle1",
      width: 100,
      height: 100,
    });

    const text1 = API.createElement({
      type: "text",
      id: "text1",
      text: "Snorf",
    });

    h.elements = [rectangle1, text1];

    API.setSelectedElements([rectangle1, text1]);

    expect(h.state.selectedElementIds[rectangle1.id, text1.id]).toBe(true);

    h.app.actionManager.executeAction(actionBindText);

    const container = h.elements.at(-2)!;
    
    expect(container.type).toBe("rectangle");
    expect(text1.x).toBe(container.x+container.width/2-text1.width/2);

Review comment (refactor):
can you move this test to `textwysiwyg.test.tsx` ?, 
All text containers related tests we keep in the above file though can decouple later but for now lets keep it in same place so its easier to decouple later as well.
This file deals with tests related to `binding.ts`

EDIT - Here is a concern I have https://github.com/excalidraw/excalidraw/issues/6434#issuecomment-1508034121 so lets resolve this first before resuming

Code after:
    expect(container).toEqual(
      expect.objectContaining({
        boundElements: expect.arrayContaining([
          {
            type: "text",
            id: text1.id,
          },
          {
            type: "arrow",
            id: arrow1.id,
          },
          {
            type: "arrow",
            id: arrow2.id,
          },
        ]),
      }),
    );

    expect(arrow1.startBinding?.elementId).toBe(rectangle1.id);
    expect(arrow1.endBinding?.elementId).toBe(container.id);
    expect(arrow2.startBinding?.elementId).toBe(container.id);
    expect(arrow2.endBinding?.elementId).toBe(rectangle1.id);
  });
});
Repository: excalidraw/excalidraw
File: src/components/Trans.tsx (TypeScript)

Code before:
      }
    } else {
      console.warn(
        `Trans: missed to pass in variable ${match} for interpolating ${format}`,
      );
    }
  });

  if (stack.length !== 1) {
    console.warn(`Trans: stack not empty for interpolating ${format}`);
  }

  return stack[0].children;
};

const Trans = ({
  i18nKey,
  children,
  ...props
}: {
  i18nKey: string;
  [key: string]: React.ReactNode | React.FC<any>;
}) => {
  const { t } = useI18n();

  return <>{getTransChildren(t(i18nKey), props)}</>;
};

export default Trans;


Review comment (suggestion):
@ad1992 the reason for using `createElement` is that React complains if the `children` don't have the `key` prop.

`<>{[child1, child2]}</>` is translated to `createElement(React.Fragment, { children: [child1, child2] })` and if `child1` or `child2` don't have `key` prop, React will complain.

`<>{child1}{child2}</>` is translated to `createElement(React.Fragment, {}, child1, child2)` (which is what we want to do).

Code after:
            ...item.children,
          );
          const fn = props[item.name];
          if (typeof fn === "function") {
            stack[stack.length - 1].children.push(fn(itemChildren));
          }
        } else {
          console.warn(
            `Trans: unexpected end tag ${match} for interpolating ${format}`,
          );
        }
      } else if (keyMatch !== null) {
        // The match is for {{key}}. Check if the key is present in props and set
        // the prop value as children of last stack item e.g. format = "Hello
        // {{name}}", key = "name" and props.name = "Excalidraw" then its prop
        // value will be pushed to "name"'s children so it's rendered on DOM as
        // "Hello Excalidraw"
        const name = keyMatch[1];
        if (props.hasOwnProperty(name)) {
          stack[stack.length - 1].children.push(props[name] as React.ReactNode);
        } else {
          console.warn(
            `Trans: key ${name} not in props for interpolating ${format}`,
          );
        }
      } else {
        // If none of cases match means we just need to push the string
        // to stack eg - "Hello {{name}} Whats up?" "Hello", "Whats up" will be pushed
        stack[stack.length - 1].children.push(match);
      }
    });

  if (stack.length !== 1) {
    console.warn(`Trans: stack not empty for interpolating ${format}`);
  }

  return stack[0].children;
};

/*
Trans component is used for translating JSX.

```json
{
  "example1": "Hello {{audience}}",
  "example2": "Please <link>click the button</link> to continue.",
  "example3": "Please <link>click {{location}}</link> to continue.",
  "example4": "Please <link>click <bold>{{location}}</bold></link> to continue.",
}
```
Repository: excalidraw/excalidraw
File: src/components/LayerUI.tsx (TypeScript)

Code before:
        canvas={canvas}
        setAppState={setAppState}
      />
    );
  };

  const renderImageExportDialog = () => {
    if (!UIOptions.canvasActions.saveAsImage) {
      return null;
    }

    const createExporter =
      (type: ExportType): ExportCB =>
      async (exportedElements) => {
        trackEvent("export", type, "ui");
        const fileHandle = await exportCanvas(
          type,
          exportedElements,
          appState,
          files,
          {
            exportBackground: appState.exportBackground,
            name: appState.name,
            viewBackgroundColor: appState.viewBackgroundColor,
          },
        )
          .catch(muteFSAbortError)
          .catch((error) => {
            console.error(error);
            setAppState({ errorMessage: error.message });
          });

        if (
          appState.exportEmbedScene &&
          fileHandle &&
          isImageFileHandle(fileHandle)
        ) {
          setAppState({ fileHandle });
        }
      };

    return (
      <ImageExportDialog
        elements={elements}
        appState={appState}
        setAppState={setAppState}
        files={files}
        actionManager={actionManager}
        onExportToPng={createExporter("png")}
        onExportToSvg={createExporter("svg")}
        onExportToClipboard={createExporter("clipboard")}

Review comment (suggestion):
This shouldn't matter now (as it's not actually any change in code, just types), and we will be splitting the canvas hopefully soon (the PR in in progress).

An alternative is to pass a `getAppState()` getter into `LayerUI` compo.

Code after:
      <JSONExportDialog
        elements={elements}
        appState={appState}
        files={files}
        actionManager={actionManager}
        exportOpts={UIOptions.canvasActions.export}
        canvas={canvas}
        setAppState={setAppState}
      />
    );
  };

  const renderImageExportDialog = () => {
    if (!UIOptions.canvasActions.saveAsImage) {
      return null;
    }

    const createExporter =
      (type: ExportType): ExportCB =>
      async (exportedElements) => {
        trackEvent("export", type, "ui");
        const fileHandle = await exportCanvas(
          type,
          exportedElements,
          // FIXME once we split UI canvas from element canvas
          appState as AppState,
          files,
          {
            exportBackground: appState.exportBackground,
            name: appState.name,
            viewBackgroundColor: appState.viewBackgroundColor,
          },
        )
          .catch(muteFSAbortError)
          .catch((error) => {
            console.error(error);
            setAppState({ errorMessage: error.message });
          });

        if (
          appState.exportEmbedScene &&
          fileHandle &&
          isImageFileHandle(fileHandle)
        ) {
          setAppState({ fileHandle });
        }
      };

    return (
      <ImageExportDialog
        elements={elements}
Repository: excalidraw/excalidraw
File: src/components/App.tsx (TypeScript)

Code before:
   * Zooms on canvas viewport center
   */
  zoomCanvas = (
    /** decimal fraction between 0.1 (10% zoom) and 30 (3000% zoom) */
    value: number,
  ) => {
    this.setState({
      ...getStateForZoom(
        {
          viewportX: this.state.width / 2 + this.state.offsetLeft,
          viewportY: this.state.height / 2 + this.state.offsetTop,
          nextZoom: getNormalizedZoom(value),
        },
        this.state,
      ),
    });
  };

  private cancelInProgresAnimation: (() => void) | null = null;

  scrollToContent = (
    target:
      | ExcalidrawElement
      | readonly ExcalidrawElement[] = this.scene.getNonDeletedElements(),
    opts?:
      | {
          fitToContent?: boolean;
          fitToViewport?: never;
          viewportZoomFactor?: never;
          animate?: boolean;
          duration?: number;
        }
      | {
          fitToContent?: never;
          fitToViewport?: boolean;
          /** when fitToViewport=true, how much screen should the content cover,
           * between 0.1 (10%) and 1 (100%)
           */
          viewportZoomFactor?: number;
          animate?: boolean;
          duration?: number;
        },
  ) => {
    this.cancelInProgresAnimation?.();

    // convert provided target into ExcalidrawElement[] if necessary
    const targetElements = Array.isArray(target) ? target : [target];

    let zoom = this.state.zoom;
    let scrollX = this.state.scrollX;
    let scrollY = this.state.scrollY;

Review comment (suggestion):
we should rename to something like `fitContent`. `fitToContent` sounds it should work like `fitToViewport` currently does. TBD.

Code after:
   */
  zoomCanvas = (
    /** decimal fraction between 0.1 (10% zoom) and 30 (3000% zoom) */
    value: number,
  ) => {
    this.setState({
      ...getStateForZoom(
        {
          viewportX: this.state.width / 2 + this.state.offsetLeft,
          viewportY: this.state.height / 2 + this.state.offsetTop,
          nextZoom: getNormalizedZoom(value),
        },
        this.state,
      ),
    });
  };

  private cancelInProgresAnimation: (() => void) | null = null;

  scrollToContent = (
    target:
      | ExcalidrawElement
      | readonly ExcalidrawElement[] = this.scene.getNonDeletedElements(),
    opts?:
      | {
          fitToContent?: boolean;
          fitToViewport?: never;
          viewportZoomFactor?: never;
          animate?: boolean;
          duration?: number;
        }
      | {
          fitToContent?: never;
          fitToViewport?: boolean;
          /** when fitToViewport=true, how much screen should the content cover,
           * between 0.1 (10%) and 1 (100%)
           */
          viewportZoomFactor?: number;
          animate?: boolean;
          duration?: number;
        },
  ) => {
    this.cancelInProgresAnimation?.();

    // convert provided target into ExcalidrawElement[] if necessary
    const targetElements = Array.isArray(target) ? target : [target];

    let zoom = this.state.zoom;
    let scrollX = this.state.scrollX;
    let scrollY = this.state.scrollY;
Repository: excalidraw/excalidraw
File: src/global.d.ts (TypeScript)

Code before:
// --------------------------------------------------------------------------—

// https://github.com/nodeca/image-blob-reduce/issues/23#issuecomment-783271848
declare module "image-blob-reduce" {
  import { PicaResizeOptions, Pica } from "pica";
  namespace ImageBlobReduce {
    interface ImageBlobReduce {
      toBlob(file: File, options: ImageBlobReduceOptions): Promise<Blob>;
      _create_blob(
        this: { pica: Pica },
        env: {
          out_canvas: HTMLCanvasElement;
          out_blob: Blob;
        },
      ): Promise<any>;
    }

    interface ImageBlobReduceStatic {
      new (options?: any): ImageBlobReduce;

      (options?: any): ImageBlobReduce;
    }

    interface ImageBlobReduceOptions extends PicaResizeOptions {
      max: number;
    }
  }
  const reduce: ImageBlobReduce.ImageBlobReduceStatic;
  export = reduce;
}

declare namespace jest {
  interface Expect {
    toBeNonNaNNumber(): void;
  }
}


Review comment (suggestion):
this should be vitest now

EDIT - its still using expect from jest probably as I have kept the assertion library `@testing-library/jest-dom` as few tests depend on it so once we remove it this will get replaced too

Code after:
        env: {
          out_canvas: HTMLCanvasElement;
          out_blob: Blob;
        },
      ): Promise<any>;
    }

    interface ImageBlobReduceStatic {
      new (options?: any): ImageBlobReduce;

      (options?: any): ImageBlobReduce;
    }

    interface ImageBlobReduceOptions extends PicaResizeOptions {
      max: number;
    }
  }
  const reduce: ImageBlobReduce.ImageBlobReduceStatic;
  export = reduce;
}

declare namespace jest {
  interface Expect {
    toBeNonNaNNumber(): void;
  }
}
Repository: excalidraw/excalidraw
File: src/element/textWysiwyg.test.tsx (TypeScript)

Code before:
      expect(
        (h.elements[1] as ExcalidrawTextElementWithContainer).verticalAlign,
      ).toBe(VERTICAL_ALIGN.BOTTOM);

      // Attempt to Bind 2nd text using text tool
      UI.clickTool("text");
      mouse.clickAt(
        rectangle.x + rectangle.width / 2,
        rectangle.y + rectangle.height / 2,
      );
      editor = getTextEditor();
      await new Promise((r) => setTimeout(r, 0));
      updateTextEditor(editor, "Excalidraw");
      editor.blur();

      expect(h.elements.length).toBe(3);
      expect(rectangle.boundElements).toStrictEqual([
        { id: h.elements[1].id, type: "text" },
      ]);
      text = h.elements[2] as ExcalidrawTextElementWithContainer;
      expect(text.containerId).toBe(null);
      expect(text.text).toBe("Excalidraw");
    });
  });

  it.only("should update labeled arrow version", async () => {
    await render(<ExcalidrawApp />);
    const arrow = UI.createElement("arrow", {
      width: 300,
      height: 0,
    });

    mouse.select(arrow);
    Keyboard.keyPress(KEYS.ENTER);
    let editor = getTextEditor();
    await new Promise((r) => setTimeout(r, 0));
    updateTextEditor(editor, "Hello");
    editor.blur();

    const { version } = arrow;

    mouse.select(arrow);
    Keyboard.keyPress(KEYS.ENTER);
    editor = getTextEditor();
    await new Promise((r) => setTimeout(r, 0));
    updateTextEditor(editor, "Hello\nworld!");
    editor.blur();

    expect(arrow.version).toEqual(version + 1);
  });
});

Review comment (suggestion):
```suggestion
  it("should update labeled arrow version", async () => {
```
nooo :p

Code after:
        (h.elements[1] as ExcalidrawTextElementWithContainer).verticalAlign,
      ).toBe(VERTICAL_ALIGN.BOTTOM);

      // Attempt to Bind 2nd text using text tool
      UI.clickTool("text");
      mouse.clickAt(
        rectangle.x + rectangle.width / 2,
        rectangle.y + rectangle.height / 2,
      );
      editor = getTextEditor();
      await new Promise((r) => setTimeout(r, 0));
      updateTextEditor(editor, "Excalidraw");
      editor.blur();

      expect(h.elements.length).toBe(3);
      expect(rectangle.boundElements).toStrictEqual([
        { id: h.elements[1].id, type: "text" },
      ]);
      text = h.elements[2] as ExcalidrawTextElementWithContainer;
      expect(text.containerId).toBe(null);
      expect(text.text).toBe("Excalidraw");
    });
  });

  it("should bump the version of labelled arrow when label updated", async () => {
    await render(<ExcalidrawApp />);
    const arrow = UI.createElement("arrow", {
      width: 300,
      height: 0,
    });

    mouse.select(arrow);
    Keyboard.keyPress(KEYS.ENTER);
    let editor = getTextEditor();
    await new Promise((r) => setTimeout(r, 0));
    updateTextEditor(editor, "Hello");
    editor.blur();

    const { version } = arrow;

    mouse.select(arrow);
    Keyboard.keyPress(KEYS.ENTER);
    editor = getTextEditor();
    await new Promise((r) => setTimeout(r, 0));
    updateTextEditor(editor, "Hello\nworld!");
    editor.blur();

    expect(arrow.version).toEqual(version + 1);
  });
});