ronantakizawa/codereview-bench huggingface dataset:
Repository: motion-canvas/motion-canvas
File: packages/2d/src/lib/components/Icon.ts (TypeScript)

Code before:

  /**
   * Create the URL that will be used as the Image source
   * @returns Address to Iconify API for the requested Icon.
   */
  @computed()
  protected svgUrl(): string {
    const iconPathSegment = this.icon().replace(':', '/');
    const encodedColorValue = encodeURIComponent(this.color().hex());
    // Iconify API is documented here: https://docs.iconify.design/api/svg.html#color
    return `https://api.iconify.design/${iconPathSegment}.svg?color=${encodedColorValue}`;
  }

  /**
   * overrides `Image.src` getter
   */
  protected getSrc(): string {
    return this.svgUrl();
  }

  /**
   * overrides `Image.src` setter to warn the user that the value
   * is not used
   */
  protected setSrc() {
    if (!this.init) {
      return;
    }
    useLogger().warn(
      "The Icon Component does not accept setting the `src`. If you need access to `src`, use '<Img/>` instead.",
    );
  }
}


Review comment (suggestion):
I think simply checking if the newly set `src` is `null` should be enough

Code after:
  protected svgUrl(): string {
    const iconPathSegment = this.icon().replace(':', '/');
    const encodedColorValue = encodeURIComponent(this.color().hex());
    // Iconify API is documented here: https://docs.iconify.design/api/svg.html#color
    return `https://api.iconify.design/${iconPathSegment}.svg?color=${encodedColorValue}`;
  }

  /**
   * overrides `Image.src` getter
   */
  protected getSrc(): string {
    return this.svgUrl();
  }

  /**
   * overrides `Image.src` setter to warn the user that the value
   * is not used
   */
  protected setSrc(src: string | null) {
    if (src === null) {
      return;
    }
    useLogger().warn(
      "The Icon Component does not accept setting the `src`. If you need access to `src`, use '<Img/>` instead.",
    );
  }
}
Repository: motion-canvas/motion-canvas
File: packages/ui/src/components/controls/NumberInput.tsx (TypeScript)

Code before:
  }, [value]);

  const handleDrag = useCallback(
    (event: MouseEvent) => {
      setCurrentValue(value => {
        // ignore very large jumps in movement, as they often result from
        // mouse acceleration issues
        if (Math.abs(event.movementX) > 100) {
          return value;
        }

        const newValue = clamp(min, max, value + event.movementX * step);
        handleChange?.(newValue);
        return newValue;
      });
    },
    [min, max, step],
  );

  useDocumentEvent('pointerlockchange', () => {
    if (document.pointerLockElement === inputRef.current) {
      document.addEventListener('pointermove', handleDrag);
    } else {
      document.removeEventListener('pointermove', handleDrag);
    }
  });

  return (
    <input
      type={'number'}
      ref={inputRef}
      min={min}
      max={max}
      step={step}
      value={currentValue}
      onChangeCapture={event => {
        const value = parseInt((event.target as HTMLInputElement).value);
        handleChangeCapture?.(value);
      }}
      onChange={event => {
        const value = parseInt((event.target as HTMLInputElement).value);
        console.log(value);

        handleChange?.(value);
      }}
      onPointerDown={event => {
        if (!active && event.button === MouseButton.Left) {
          event.preventDefault();
          event.stopPropagation();
          event.currentTarget.setPointerCapture(event.pointerId);


Review comment (suggestion):
This captures `handleDrag` and won't update it even when `min`, `max`, or `step` change.

Code after:
        // mouse acceleration issues
        if (Math.abs(event.movementX) > 100) {
          return value;
        }

        return clamp(min, max, value + event.movementX * step);
      });
    },
    [min, max, step],
  );

  useDocumentEvent(
    'pointerlockchange',
    useCallback(() => {
      if (document.pointerLockElement === inputRef.current) {
        document.addEventListener('pointermove', handleDrag);
      } else {
        document.removeEventListener('pointermove', handleDrag);
      }
    }, [handleDrag]),
  );

  return (
    <input
      type={'number'}
      ref={inputRef}
      min={min}
      max={max}
      step={step}
      value={currentValue}
      onChangeCapture={() => onChange?.(parseInt(inputRef.current.value))}
      onPointerDown={event => {
        if (
          document.activeElement !== inputRef.current &&
          event.button === MouseButton.Left
        ) {
          event.preventDefault();
          event.stopPropagation();
          event.currentTarget.setPointerCapture(event.pointerId);

          setStartX(event.clientX);
        }
      }}
      onPointerMove={event => {
        if (
          document.activeElement !== inputRef.current &&
          event.currentTarget.hasPointerCapture(event.pointerId)
        ) {
          event.preventDefault();
          event.stopPropagation();
Repository: motion-canvas/motion-canvas
File: packages/ui/src/components/controls/NumberInputSelect.tsx (TypeScript)

Code before:
import {NumberInput} from './NumberInput';
import {Select, SelectProps} from './Select';

export type NumberInputSelectProps = Omit<
  JSX.HTMLAttributes<HTMLInputElement>,
  'min' | 'max' | 'step' | 'value' | 'onChange' | 'onChangeCapture'
> &
  SelectProps<number> & {
    value: number;
    min?: number;
    max?: number;
    step?: number;
    onChangeCapture?: (value: number) => void;
  };

export function NumberInputSelect({
  options,
  value,
  onChange,
  ...props
}: NumberInputSelectProps) {
  return (
    <div className={styles.inputSelect}>
      <NumberInput
        value={value}
        onChange={value => onChange(value)}
        {...props}
      />
      <Select value={value} options={options} onChange={onChange} />
    </div>
  );
}


Review comment (suggestion):
Couldn't it just be this?
```suggestion
        onChange={onChange}
```

Code after:
import {NumberInput} from './NumberInput';
import {Select, SelectProps} from './Select';

export type NumberInputSelectProps = Omit<
  JSX.HTMLAttributes<HTMLInputElement>,
  'value' | 'onChange' | 'min' | 'max' | 'step'
> &
  SelectProps<number> & {
    value: number;
    min?: number;
    max?: number;
    step?: number;
  };

export function NumberInputSelect({
  options,
  value,
  onChange,
  ...props
}: NumberInputSelectProps) {
  return (
    <div className={styles.inputSelect}>
      <NumberInput value={value} onChange={onChange} {...props} />
      <Select value={value} options={options} onChange={onChange} />
    </div>
  );
}
Repository: motion-canvas/motion-canvas
File: packages/core/src/meta/ObjectMetaField.ts (TypeScript)

Code before:
    this.ignoreChange = true;
    for (const [key, fieldValue] of Object.entries(value)) {
      const field = this.fields.get(key);
      if (field) {
        field.set(fieldValue);
      } else {
        this.customFields[key] = fieldValue;
      }
    }
    this.ignoreChange = false;
    this.handleChange();
  }

  public override serialize(): ValueOf<T> {
    return {
      ...this.transform('serialize'),
      ...this.customFields,
    };
  }

  public override clone(): this {
    const cloned = new (<any>this.constructor)(
      this.name,
      this.transform('clone'),
    );
    cloned.customFields = structuredClone(this.customFields);

    return cloned;
  }

  protected handleChange = () => {
    if (this.ignoreChange) return;
    this.value.current = {
      ...this.transform('get'),
      ...this.customFields,
    };
  };

  protected transform<TKey extends CallableKeys<MetaField<any>>>(
    fn: TKey,
  ): TransformationOf<T, TKey> {
    const transformed = Object.fromEntries(
      Array.from(this.fields, ([name, field]) => [name, field[fn]()]),
    ) as TransformationOf<T, TKey>;

    return transformed;
  }
}

/**
 * {@inheritDoc ObjectMetaFieldInternal}

Review comment (bug):
After testing it, it doesn't seem to work as expected.
The initial value of the meta field is set in the constructor, which doesn't have access to `customFields` so the value ends up not including them.
Here's a simple reproduction:
```ts
const field = new ObjectMetaField('example', {
  bool: new BoolMetaField('bool', false),
});
field.set({
  bool: true,
  custom: 'custom',
});

const clone = field.clone();

// This fails:
console.assert(clone.get().custom === 'custom');
```
I think `customFields` should be passed as an optional argument to the constructor instead of being assigned after the object has been created.

Code after:
    this.ignoreChange = true;
    for (const [key, fieldValue] of Object.entries(value)) {
      const field = this.fields.get(key);
      if (field) {
        field.set(fieldValue);
      } else {
        this.customFields[key] = fieldValue;
      }
    }
    this.ignoreChange = false;
    this.handleChange();
  }

  public override serialize(): ValueOf<T> {
    return {
      ...this.transform('serialize'),
      ...this.customFields,
    };
  }

  public override clone(): this {
    const cloned = new (<any>this.constructor)(
      this.name,
      this.transform('clone'),
    );
    cloned.set(structuredClone(this.customFields));

    return cloned;
  }

  protected handleChange = () => {
    if (this.ignoreChange) return;
    this.value.current = {
      ...this.transform('get'),
      ...this.customFields,
    };
  };

  protected transform<TKey extends CallableKeys<MetaField<any>>>(
    fn: TKey,
  ): TransformationOf<T, TKey> {
    const transformed = Object.fromEntries(
      Array.from(this.fields, ([name, field]) => [name, field[fn]()]),
    ) as TransformationOf<T, TKey>;

    return transformed;
  }
}

/**
 * {@inheritDoc ObjectMetaFieldInternal}
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/Sounds.ts (TypeScript)

Code before:
   * Trim the audio file to a specific portion of it.
   *
   * @param start - The offset in seconds to play from.
   * @param end - The offset in seconds to play to.
   */
  public trim(start?: number, end?: number): this {
    this.settings.start = start;
    this.settings.end = end;
    return this;
  }

  /**
   * Set the amplification of the played sound.
   *
   * @param db - The gain in dB.
   */
  public gain(db: number): this {
    this.settings.gain = db;
    return this;
  }

  /**
   * Pitch shift the played sound.
   * This also affects the duration of the sound.
   *
   * @param cents - The pitch shift in cents.
   */
  public detune(cents: number): this {
    this.settings.detune = cents;
    return this;
  }

  /**
   * Change the playback rate of the sound.
   * This also affects the percieved pitch of the sound.
   *
   * @param rate - The new playback rate.
   */
  public playbackRate(rate: number): this {
    this.settings.playbackRate = rate;
    return this;
  }

  /**
   * Play the configured sound at the current frame.
   *
   * @param offset - An offset in seconds from the current frame.
   */
  public play(offset?: number) {
    useScene().sounds.add(this.settings, offset);
  }

Review comment (suggestion):
```suggestion
   *
   * @remarks
   * This also affects the duration of the sound.
```

Code after:
   * Trim the audio file to a specific portion of it.
   *
   * @param start - The offset in seconds to play from.
   * @param end - The offset in seconds to play to.
   */
  public trim(start?: number, end?: number): this {
    this.settings.start = start;
    this.settings.end = end;
    return this;
  }

  /**
   * Set the amplification of the played sound.
   *
   * @param db - The gain in dB.
   */
  public gain(db: number): this {
    this.settings.gain = db;
    return this;
  }

  /**
   * Pitch shift the played sound.
   *
   * @remarks
   * This also affects the duration of the sound.
   *
   * @param cents - The pitch shift in cents.
   */
  public detune(cents: number): this {
    this.settings.detune = cents;
    return this;
  }

  /**
   * Change the playback rate of the sound.
   *
   * @remarks
   * This also affects the percieved pitch of the sound.
   *
   * @param rate - The new playback rate.
   */
  public playbackRate(rate: number): this {
    this.settings.playbackRate = rate;
    return this;
  }

  /**
   * Play the configured sound at the current frame.
   *
   * @param offset - An offset in seconds from the current frame. Defaults to 0.
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/Sounds.ts (TypeScript)

Code before:
   *
   * @param db - The gain in dB.
   */
  public gain(db: number): this {
    this.settings.gain = db;
    return this;
  }

  /**
   * Pitch shift the played sound.
   * This also affects the duration of the sound.
   *
   * @param cents - The pitch shift in cents.
   */
  public detune(cents: number): this {
    this.settings.detune = cents;
    return this;
  }

  /**
   * Change the playback rate of the sound.
   * This also affects the percieved pitch of the sound.
   *
   * @param rate - The new playback rate.
   */
  public playbackRate(rate: number): this {
    this.settings.playbackRate = rate;
    return this;
  }

  /**
   * Play the configured sound at the current frame.
   *
   * @param offset - An offset in seconds from the current frame.
   */
  public play(offset?: number) {
    useScene().sounds.add(this.settings, offset);
  }
}

/**
 * {@inheritDoc SoundBuilder."constructor"}
 */
export function sound(audio: string | SoundBuilder) {
  return new SoundBuilder(audio);
}

export class Sounds {
  private sounds: Sound[] = [];

  public constructor(private readonly scene: Scene) {

Review comment (suggestion):
```suggestion
   *
   * @remarks
   * This also affects the percieved pitch of the sound.
```

Code after:
   *
   * @param db - The gain in dB.
   */
  public gain(db: number): this {
    this.settings.gain = db;
    return this;
  }

  /**
   * Pitch shift the played sound.
   *
   * @remarks
   * This also affects the duration of the sound.
   *
   * @param cents - The pitch shift in cents.
   */
  public detune(cents: number): this {
    this.settings.detune = cents;
    return this;
  }

  /**
   * Change the playback rate of the sound.
   *
   * @remarks
   * This also affects the percieved pitch of the sound.
   *
   * @param rate - The new playback rate.
   */
  public playbackRate(rate: number): this {
    this.settings.playbackRate = rate;
    return this;
  }

  /**
   * Play the configured sound at the current frame.
   *
   * @param offset - An offset in seconds from the current frame. Defaults to 0.
   */
  public play(offset?: number) {
    useScene().sounds.add(this.settings, offset);
  }
}

/**
 * Begin configuring a sound to be played back.
 */
export function sound(audio: string | SoundBuilder) {
  return new SoundBuilder(audio);
}
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/Sounds.ts (TypeScript)

Code before:
  /**
   * Pitch shift the played sound.
   * This also affects the duration of the sound.
   *
   * @param cents - The pitch shift in cents.
   */
  public detune(cents: number): this {
    this.settings.detune = cents;
    return this;
  }

  /**
   * Change the playback rate of the sound.
   * This also affects the percieved pitch of the sound.
   *
   * @param rate - The new playback rate.
   */
  public playbackRate(rate: number): this {
    this.settings.playbackRate = rate;
    return this;
  }

  /**
   * Play the configured sound at the current frame.
   *
   * @param offset - An offset in seconds from the current frame.
   */
  public play(offset?: number) {
    useScene().sounds.add(this.settings, offset);
  }
}

/**
 * {@inheritDoc SoundBuilder."constructor"}
 */
export function sound(audio: string | SoundBuilder) {
  return new SoundBuilder(audio);
}

export class Sounds {
  private sounds: Sound[] = [];

  public constructor(private readonly scene: Scene) {
    this.scene.onReset.subscribe(this.handleReset);
    this.scene.onReset.subscribe(this.handleReload);
  }

  public add(settings: SoundSettings, offset?: number) {
    const playbackTime = this.scene.playback.time + (offset ?? 0);
    this.sounds.push({offset: playbackTime, ...settings});
  }

Review comment (suggestion):
```suggestion
   * @param offset - An offset in seconds from the current frame. Defaults to 0.
```

Code after:
  /**
   * Pitch shift the played sound.
   *
   * @remarks
   * This also affects the duration of the sound.
   *
   * @param cents - The pitch shift in cents.
   */
  public detune(cents: number): this {
    this.settings.detune = cents;
    return this;
  }

  /**
   * Change the playback rate of the sound.
   *
   * @remarks
   * This also affects the percieved pitch of the sound.
   *
   * @param rate - The new playback rate.
   */
  public playbackRate(rate: number): this {
    this.settings.playbackRate = rate;
    return this;
  }

  /**
   * Play the configured sound at the current frame.
   *
   * @param offset - An offset in seconds from the current frame. Defaults to 0.
   */
  public play(offset?: number) {
    useScene().sounds.add(this.settings, offset);
  }
}

/**
 * Begin configuring a sound to be played back.
 */
export function sound(audio: string | SoundBuilder) {
  return new SoundBuilder(audio);
}

export class Sounds {
  private sounds: Sound[] = [];

  public constructor(private readonly scene: Scene) {
    this.scene.onReset.subscribe(this.handleReset);
    this.scene.onReset.subscribe(this.handleReload);
  }
Repository: motion-canvas/motion-canvas
File: packages/vite-plugin/src/utils.ts (TypeScript)

Code before:
import fg from 'fast-glob';
import fs from 'fs';
import path from 'path';
import {ProjectData} from './plugins';

export async function createMeta(metaPath: string) {
  if (!fs.existsSync(metaPath)) {
    await fs.promises.writeFile(
      metaPath,
      JSON.stringify({version: 0}, undefined, 2),
      'utf8',
    );
  }
}

export function getProjects(project: string | string[]): ProjectData[] {
  const list: ProjectData[] = [];
  const projectList = expandFilePaths(project);
  for (const url of projectList) {
    const {name, dir} = path.posix.parse(url);
    const metaFile = `${name}.meta`;
    const metaData = getMeta(path.join(dir, metaFile));
    const filePath = [...dir.split('/').slice(2), name].join('/');
    const data = {
      name: metaData?.name ?? filePath,
      fileName: name,
      url,
      filePath,
    };
    list.push(data);
  }

  return list;
}

function expandFilePaths(filePaths: string[] | string): string[] {
  const expandedFilePaths = [];

  for (const filePath of typeof filePaths === 'string'
    ? [filePaths]
    : filePaths) {
    if (fg.isDynamicPattern(filePath)) {
      const matchingFilePaths = fg.sync(filePath, {onlyFiles: true});
      expandedFilePaths.push(...matchingFilePaths);
    } else {
      expandedFilePaths.push(filePath);
    }
  }

Review comment (suggestion):
We don't know if people use `src` to store their projects, someone could use a `videoA/project.ts`, `videoB/project.ts` structure with no `src`.
I think we should use the entire path here and not strip the `src`:
```suggestion
    const filePath = path.posix.join(dir, name);
```

Code after:
import fg from 'fast-glob';
import fs from 'fs';
import path from 'path';
import {ProjectData} from './plugins';

export async function createMeta(metaPath: string) {
  if (!fs.existsSync(metaPath)) {
    await fs.promises.writeFile(
      metaPath,
      JSON.stringify({version: 0}, undefined, 2),
      'utf8',
    );
  }
}

export function getProjects(project: string | string[]): ProjectData[] {
  const list: ProjectData[] = [];
  const projectList = expandFilePaths(project);
  for (const filePath of projectList) {
    const {name, dir} = path.posix.parse(filePath);
    const metaFile = `${name}.meta`;
    const metaData = getMeta(path.join(dir, metaFile));
    const url = path.posix.join(dir, name);
    const data = {
      name: metaData?.name ?? url,
      fileName: name,
      url,
      filePath,
    };
    list.push(data);
  }

  return list;
}

function expandFilePaths(filePaths: string[] | string): string[] {
  const expandedFilePaths = [];

  for (const filePath of typeof filePaths === 'string'
    ? [filePaths]
    : filePaths) {
    if (fg.isDynamicPattern(filePath)) {
      const matchingFilePaths = fg.sync(filePath, {onlyFiles: true});
      expandedFilePaths.push(...matchingFilePaths);
    } else {
      expandedFilePaths.push(filePath);
    }
  }
Repository: mozilla/pdf.js
File: src/display/editor/ink.js (JavaScript)

Code before:
    if (event.button !== 0) {
      return;
    }

    this.canvas.removeEventListener("pointerup", this.#boundCanvasPointerup);
    this.canvas.addEventListener("pointerdown", this.#boundCanvasPointerdown);

    if (this.isInEditMode() && this.currentPath.length !== 0) {
      event.stopPropagation();
      this.#endDrawing(event);

      // Since the ink editor covers all of the page and we want to be able
      // to select another editor, we just put this one in the background.
      this.setInBackground();
    }
  }

  /**
   * onpointerleave callback for the canvas we're drawing on.
   * @param {PointerEvent} event
   */
  canvasPointerleave(event) {
    if (event.relatedTarget === null) {
      // It can happen if the right-click menu is opened.
      return;
    }
    this.#endDrawing(event);
    this.setInBackground();
  }

  /**
   * End the drawing.
   * @param {PointerEvent} event
   */
  #endDrawing(event) {
    this.#stopDrawing(event.offsetX, event.offsetY);

    this.canvas.removeEventListener(
      "pointerleave",
      this.#boundCanvasPointerleave
    );
    this.canvas.removeEventListener(
      "pointermove",
      this.#boundCanvasPointermove
    );

    this.addToAnnotationStorage();
  }

  /**
   * Create the canvas element.

Review comment (bug):
I really think that we should just *stop drawing* in this case, e.g. if the context-menu is opened, since having drawing continue if the mouse if moved outside of the context-menu seems unexpected/wrong.

Code after:

  /**
   * oncontextmenu callback for the canvas we're drawing on.
   * @param {PointerEvent} event
   */
  canvasContextMenu(event) {
    event.preventDefault();
  }

  /**
   * onpointermove callback for the canvas we're drawing on.
   * @param {PointerEvent} event
   */
  canvasPointermove(event) {
    event.preventDefault();
    this.#draw(event.offsetX, event.offsetY);
  }

  /**
   * onpointerup callback for the canvas we're drawing on.
   * @param {PointerEvent} event
   */
  canvasPointerup(event) {
    event.preventDefault();
    this.#endDrawing(event);
  }

  /**
   * onpointerleave callback for the canvas we're drawing on.
   * @param {PointerEvent} event
   */
  canvasPointerleave(event) {
    this.#endDrawing(event);
  }

  /**
   * End the drawing.
   * @param {PointerEvent} event
   */
  #endDrawing(event) {
    this.canvas.removeEventListener(
      "pointerleave",
      this.#boundCanvasPointerleave
    );
    this.canvas.removeEventListener(
      "pointermove",
      this.#boundCanvasPointermove
    );
    this.canvas.removeEventListener("pointerup", this.#boundCanvasPointerup);
    this.canvas.addEventListener("pointerdown", this.#boundCanvasPointerdown);