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

Code before:
  DependencyContext,
  SignalValue,
  SimpleSignal,
} from '@motion-canvas/core/lib/signals';

export interface VideoProps extends RectProps {
  /**
   * {@inheritDoc Video.src}
   */
  src?: SignalValue<string>;
  /**
   * {@inheritDoc Video.alpha}
   */
  alpha?: SignalValue<number>;
  /**
   * {@inheritDoc Video.smoothing}
   */
  smoothing?: SignalValue<boolean>;
  /**
   * {@inheritDoc Video.loop}
   */
  loop?: SignalValue<boolean>;
  /**
   * {@inheritDoc Video.playbackRate}
   */
  playbackRate?: SignalValue<number>;
  /**
   * The starting time for this video in seconds.
   */
  time?: SignalValue<number>;
  play?: boolean;
}

export class Video extends Rect {
  private static readonly pool: Record<string, HTMLVideoElement> = {};

  /**
   * The source of this video.
   *
   * @example
   * Using a local video:
   * ```tsx
   * import video from './example.mp4';
   * // ...
   * view.add(<Video src={video} />)
   * ```
   * Loading an image from the internet:
   * ```tsx
   * view.add(<Video src="https://example.com/video.mp4" />)
   * ```
   */

Review comment (suggestion):
The current implementation won't work with functions and there's no way to make it work at the moment.
So it should be just `number`:
```suggestion
  playbackRate?: number;
```

Code after:
  DependencyContext,
  SignalValue,
  SimpleSignal,
  isReactive,
} from '@motion-canvas/core/lib/signals';
import reactivePlaybackRate from './__logs__/reactive-playback-rate.md';

export interface VideoProps extends RectProps {
  /**
   * {@inheritDoc Video.src}
   */
  src?: SignalValue<string>;
  /**
   * {@inheritDoc Video.alpha}
   */
  alpha?: SignalValue<number>;
  /**
   * {@inheritDoc Video.smoothing}
   */
  smoothing?: SignalValue<boolean>;
  /**
   * {@inheritDoc Video.loop}
   */
  loop?: SignalValue<boolean>;
  /**
   * {@inheritDoc Video.playbackRate}
   */
  playbackRate?: number;
  /**
   * The starting time for this video in seconds.
   */
  time?: SignalValue<number>;
  play?: boolean;
}

export class Video extends Rect {
  private static readonly pool: Record<string, HTMLVideoElement> = {};

  /**
   * The source of this video.
   *
   * @example
   * Using a local video:
   * ```tsx
   * import video from './example.mp4';
   * // ...
   * view.add(<Video src={video} />)
   * ```
   * Loading an image from the internet:
   * ```tsx
   * view.add(<Video src="https://example.com/video.mp4" />)
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Video.ts (TypeScript)

Code before:
    const video = this.video();
    if (video.readyState < 2) return;

    video.currentTime = value;
    this.lastTime = value;
    if (video.seeking) {
      DependencyContext.collectPromise(
        new Promise<void>(resolve => {
          const listener = () => {
            resolve();
            video.removeEventListener('seeked', listener);
          };
          video.addEventListener('seeked', listener);
        }),
      );
    }
  }

  protected setPlaybackRate(playbackRate: number) {
    let value: number;
    if (isReactive(playbackRate)) {
      value = playbackRate();
      useLogger().warn({
        message: 'Invalid value set as the playback rate',
        remarks: reactivePlaybackRate,
        inspect: this.key,
      });
    } else {
      value = playbackRate;
    }
    this.playbackRate.context.setter(value);

    if (this.playing()) {
      if (value === 0) {
        this.pause();
      } else {
        const time = useThread().time;
        const start = time();
        const offset = this.time();
        this.time(() => this.clampTime(offset + (time() - start) * value));
      }
    }
  }

  public play() {
    const time = useThread().time;
    const start = time();
    const offset = this.time();
    const playbackRate = this.playbackRate();
    this.playing(true);
    this.time(() => this.clampTime(offset + (time() - start) * playbackRate));

Review comment (suggestion):
Forgot about this but we should also add a stack trace here:
```suggestion
        inspect: this.key,
        stack: new Error().stack,
```

Code after:
    const video = this.video();
    if (video.readyState < 2) return;

    video.currentTime = value;
    this.lastTime = value;
    if (video.seeking) {
      DependencyContext.collectPromise(
        new Promise<void>(resolve => {
          const listener = () => {
            resolve();
            video.removeEventListener('seeked', listener);
          };
          video.addEventListener('seeked', listener);
        }),
      );
    }
  }

  protected setPlaybackRate(playbackRate: number) {
    let value: number;
    if (isReactive(playbackRate)) {
      value = playbackRate();
      useLogger().warn({
        message: 'Invalid value set as the playback rate',
        remarks: reactivePlaybackRate,
        inspect: this.key,
        stack: new Error().stack,
      });
    } else {
      value = playbackRate;
    }
    this.playbackRate.context.setter(value);

    if (this.playing()) {
      if (value === 0) {
        this.pause();
      } else {
        const time = useThread().time;
        const start = time();
        const offset = this.time();
        this.time(() => this.clampTime(offset + (time() - start) * value));
      }
    }
  }

  public play() {
    const time = useThread().time;
    const start = time();
    const offset = this.time();
    const playbackRate = this.playbackRate();
    this.playing(true);
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/GeneratorScene.ts (TypeScript)

Code before:
import {createSignal, DependencyContext, SimpleSignal} from '../signals';
import {SceneMetadata} from './SceneMetadata';
import {Slides} from './Slides';

export interface ThreadGeneratorFactory<T> {
  (view: T): ThreadGenerator;
}

/**
 * The default implementation of the {@link Scene} interface.
 *
 * Uses generators to control the animation.
 */
export abstract class GeneratorScene<T>
  implements Scene<ThreadGeneratorFactory<T>>, Threadable
{
  public readonly name: string;
  public readonly playback: PlaybackStatus;
  public readonly logger: Logger;
  public readonly meta: SceneMetadata;
  public readonly timeEvents: TimeEvents;
  public readonly slides: Slides;
  public readonly variables: Variables;
  public random: Random;
  public creationStack?: string;
  public readonly belowPrevious: SimpleSignal<boolean, this>;

  public get firstFrame() {
    return this.cache.current.firstFrame;
  }

  public get lastFrame() {
    return this.firstFrame + this.cache.current.duration;
  }

  public get onCacheChanged() {
    return this.cache.subscribable;
  }
  private readonly cache = new ValueDispatcher<CachedSceneData>({
    firstFrame: 0,
    transitionDuration: 0,
    duration: 0,
    lastFrame: 0,
  });

  public get onReloaded() {
    return this.reloaded.subscribable;
  }
  private readonly reloaded = new EventDispatcher<void>();

  public get onRecalculated() {

Review comment (performance):
Scenes should not use signals directly as it may lead to memory leaks.
You should store the raw `SignalValue` instead, and use `unwrap` to access it whenever necessary.

Code after:
import {DependencyContext, SignalValue} from '../signals';
import {SceneMetadata} from './SceneMetadata';
import {Slides} from './Slides';

export interface ThreadGeneratorFactory<T> {
  (view: T): ThreadGenerator;
}

/**
 * The default implementation of the {@link Scene} interface.
 *
 * Uses generators to control the animation.
 */
export abstract class GeneratorScene<T>
  implements Scene<ThreadGeneratorFactory<T>>, Threadable
{
  public readonly name: string;
  public readonly playback: PlaybackStatus;
  public readonly logger: Logger;
  public readonly meta: SceneMetadata;
  public readonly timeEvents: TimeEvents;
  public readonly slides: Slides;
  public readonly variables: Variables;
  public random: Random;
  public creationStack?: string;
  public previousOnTop: SignalValue<boolean>;

  public get firstFrame() {
    return this.cache.current.firstFrame;
  }

  public get lastFrame() {
    return this.firstFrame + this.cache.current.duration;
  }

  public get onCacheChanged() {
    return this.cache.subscribable;
  }
  private readonly cache = new ValueDispatcher<CachedSceneData>({
    firstFrame: 0,
    transitionDuration: 0,
    duration: 0,
    lastFrame: 0,
  });

  public get onReloaded() {
    return this.reloaded.subscribable;
  }
  private readonly reloaded = new EventDispatcher<void>();

  public get onRecalculated() {
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Txt.ts (TypeScript)

Code before:
    const y = box.y + box.height / 2;
    context.save();
    context.textBaseline = 'middle';

    if (this.lineWidth() <= 0) {
      context.fillText(text, box.x, y);
    } else if (this.strokeFirst()) {
      context.strokeText(text, box.x, y);
      context.fillText(text, box.x, y);
    } else {
      context.fillText(text, box.x, y);
      context.strokeText(text, box.x, y);
    }

    context.restore();
  }

  protected override getCacheBBox(): BBox {
    const cache = super.getCacheBBox();

    if (!this.element.hasChildNodes) return cache;

    const size = this.computedSize();
    const range = document.createRange();
    range.setStartBefore(this.element.firstChild!);
    range.setEndAfter(this.element.lastChild!);
    const bbox = range.getBoundingClientRect();

    return new BBox(-size.width / 2, -size.height / 2, bbox.width, bbox.height);
  }

  @computed()
  protected formattedText() {
    Txt.formatter.innerText = this.text();
    return Txt.formatter.innerText;
  }

  protected override updateLayout() {
    this.applyFont();
    this.applyFlex();

    // Make sure the text is aligned correctly even if the text is smaller than
    // the container.
    if (this.justifyContent.isInitial()) {
      this.element.style.justifyContent =
        this.styles.getPropertyValue('text-align');
    }

    const wrap =
      this.styles.whiteSpace !== 'nowrap' && this.styles.whiteSpace !== 'pre';


Review comment (suggestion):
Wouldn't this work?
```ts
range.selectNodeContents(this.element);
```

Code after:
    const y = box.y + box.height / 2;
    context.save();
    context.textBaseline = 'middle';

    if (this.lineWidth() <= 0) {
      context.fillText(text, box.x, y);
    } else if (this.strokeFirst()) {
      context.strokeText(text, box.x, y);
      context.fillText(text, box.x, y);
    } else {
      context.fillText(text, box.x, y);
      context.strokeText(text, box.x, y);
    }

    context.restore();
  }

  protected override getCacheBBox(): BBox {
    const size = this.computedSize();
    const range = document.createRange();
    range.selectNodeContents(this.element);
    const bbox = range.getBoundingClientRect();

    const lineWidth = this.lineWidth();
    // We take the default value of the miterLimit as 10.
    const miterLimitCoefficient = this.lineJoin() === 'miter' ? 0.5 * 10 : 0.5;

    return new BBox(
      -size.width / 2,
      -size.height / 2,
      bbox.width,
      bbox.height,
    ).expand(lineWidth * miterLimitCoefficient);
  }

  @computed()
  protected formattedText() {
    Txt.formatter.innerText = this.text();
    return Txt.formatter.innerText;
  }

  protected override updateLayout() {
    this.applyFont();
    this.applyFlex();

    // Make sure the text is aligned correctly even if the text is smaller than
    // the container.
    if (this.justifyContent.isInitial()) {
      this.element.style.justifyContent =
        this.styles.getPropertyValue('text-align');
    }
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Txt.ts (TypeScript)

Code before:

    if (this.lineWidth() <= 0) {
      context.fillText(text, box.x, y);
    } else if (this.strokeFirst()) {
      context.strokeText(text, box.x, y);
      context.fillText(text, box.x, y);
    } else {
      context.fillText(text, box.x, y);
      context.strokeText(text, box.x, y);
    }

    context.restore();
  }

  protected override getCacheBBox(): BBox {
    const cache = super.getCacheBBox();

    if (!this.element.hasChildNodes) return cache;

    const size = this.computedSize();
    const range = document.createRange();
    range.setStartBefore(this.element.firstChild!);
    range.setEndAfter(this.element.lastChild!);
    const bbox = range.getBoundingClientRect();

    return new BBox(-size.width / 2, -size.height / 2, bbox.width, bbox.height);
  }

  @computed()
  protected formattedText() {
    Txt.formatter.innerText = this.text();
    return Txt.formatter.innerText;
  }

  protected override updateLayout() {
    this.applyFont();
    this.applyFlex();

    // Make sure the text is aligned correctly even if the text is smaller than
    // the container.
    if (this.justifyContent.isInitial()) {
      this.element.style.justifyContent =
        this.styles.getPropertyValue('text-align');
    }

    const wrap =
      this.styles.whiteSpace !== 'nowrap' && this.styles.whiteSpace !== 'pre';

    if (wrap) {
      this.element.innerText = '';


Review comment (suggestion):
We should expand the signature of `BBox.fromSizeCentered` and use it here isntead:
```suggestion
    return BBox.fromSizeCentered(bbox);
```

Code after:

    if (this.lineWidth() <= 0) {
      context.fillText(text, box.x, y);
    } else if (this.strokeFirst()) {
      context.strokeText(text, box.x, y);
      context.fillText(text, box.x, y);
    } else {
      context.fillText(text, box.x, y);
      context.strokeText(text, box.x, y);
    }

    context.restore();
  }

  protected override getCacheBBox(): BBox {
    const size = this.computedSize();
    const range = document.createRange();
    range.selectNodeContents(this.element);
    const bbox = range.getBoundingClientRect();

    const lineWidth = this.lineWidth();
    // We take the default value of the miterLimit as 10.
    const miterLimitCoefficient = this.lineJoin() === 'miter' ? 0.5 * 10 : 0.5;

    return new BBox(
      -size.width / 2,
      -size.height / 2,
      bbox.width,
      bbox.height,
    ).expand(lineWidth * miterLimitCoefficient);
  }

  @computed()
  protected formattedText() {
    Txt.formatter.innerText = this.text();
    return Txt.formatter.innerText;
  }

  protected override updateLayout() {
    this.applyFont();
    this.applyFlex();

    // Make sure the text is aligned correctly even if the text is smaller than
    // the container.
    if (this.justifyContent.isInitial()) {
      this.element.style.justifyContent =
        this.styles.getPropertyValue('text-align');
    }

    const wrap =
      this.styles.whiteSpace !== 'nowrap' && this.styles.whiteSpace !== 'pre';
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Txt.ts (TypeScript)

Code before:
  protected drawText(
    context: CanvasRenderingContext2D,
    text: string,
    box: BBox,
  ) {
    const y = box.y + box.height / 2;
    context.save();
    context.textBaseline = 'middle';

    if (this.lineWidth() <= 0) {
      context.fillText(text, box.x, y);
    } else if (this.strokeFirst()) {
      context.strokeText(text, box.x, y);
      context.fillText(text, box.x, y);
    } else {
      context.fillText(text, box.x, y);
      context.strokeText(text, box.x, y);
    }

    context.restore();
  }

  protected override getCacheBBox(): BBox {
    const cache = super.getCacheBBox();

    if (!this.element.hasChildNodes) return cache;

    const size = this.computedSize();
    const range = document.createRange();
    range.selectNodeContents(this.element);
    const bbox = range.getBoundingClientRect();

    const lineWidth = this.lineWidth();
    // We take the default value of the miterLimit as 10.
    const miterLimitCoefficient = 0.5 * 10;

    return new BBox(
      -size.width / 2,
      -size.height / 2,
      bbox.width,
      bbox.height,
    ).expand(lineWidth * miterLimitCoefficient);
  }

  @computed()
  protected formattedText() {
    Txt.formatter.innerText = this.text();
    return Txt.formatter.innerText;
  }

  protected override updateLayout() {

Review comment (suggestion):
One final thing, these lines shouldn't be necessary now because we no longer use children for selection.
Additionally, `hasChildNodes` is a function, not a property, so right now this if statement is always false anyway.


Code after:
  protected drawText(
    context: CanvasRenderingContext2D,
    text: string,
    box: BBox,
  ) {
    const y = box.y + box.height / 2;
    context.save();
    context.textBaseline = 'middle';

    if (this.lineWidth() <= 0) {
      context.fillText(text, box.x, y);
    } else if (this.strokeFirst()) {
      context.strokeText(text, box.x, y);
      context.fillText(text, box.x, y);
    } else {
      context.fillText(text, box.x, y);
      context.strokeText(text, box.x, y);
    }

    context.restore();
  }

  protected override getCacheBBox(): BBox {
    const size = this.computedSize();
    const range = document.createRange();
    range.selectNodeContents(this.element);
    const bbox = range.getBoundingClientRect();

    const lineWidth = this.lineWidth();
    // We take the default value of the miterLimit as 10.
    const miterLimitCoefficient = this.lineJoin() === 'miter' ? 0.5 * 10 : 0.5;

    return new BBox(
      -size.width / 2,
      -size.height / 2,
      bbox.width,
      bbox.height,
    ).expand(lineWidth * miterLimitCoefficient);
  }

  @computed()
  protected formattedText() {
    Txt.formatter.innerText = this.text();
    return Txt.formatter.innerText;
  }

  protected override updateLayout() {
    this.applyFont();
    this.applyFlex();

    // Make sure the text is aligned correctly even if the text is smaller than
Repository: motion-canvas/motion-canvas
File: packages/core/src/app/Player.ts (TypeScript)

Code before:
  public toggleAudio(value: boolean = this.playerState.current.muted): void {
    if (value === this.playerState.current.muted) {
      this.playerState.current = {
        ...this.playerState.current,
        muted: !value,
      };
    }
  }

  public setAudioVolume(value: number): void {
    if (value !== this.playerState.current.volume) {
      this.playerState.current = {
        ...this.playerState.current,
        volume: value,
      };
    }
  }

  public addAudioVolume(value: number): void {
    const newValue = clamp(0, 1, this.playerState.current.volume + value);
    if (newValue !== this.playerState.current.volume) {
      this.playerState.current = {
        ...this.playerState.current,
        volume: newValue,
      };
    }
  }

  public setSpeed(value: number) {
    if (value !== this.playerState.current.speed) {
      this.playback.speed = value;
      this.playback.reload();
      this.playerState.current = {
        ...this.playerState.current,
        speed: value,
      };
      this.requestRecalculation();
    }
  }

  public setVariables(variables: Record<string, unknown>) {
    for (const scene of this.playback.onScenesRecalculated.current) {
      scene.variables.updateSignals(variables);
    }
  }

  /**
   * Activate the player.
   *
   * @remarks
   * A player needs to be active in order for the update loop to run. Each

Review comment (suggestion):
Once `setAudioVolume` clamps the value, this could be written as simply:
```suggestion
    this.setAudioVolume(this.playerState.current.volume + value);
```

Code after:
  public toggleAudio(value: boolean = this.playerState.current.muted): void {
    if (value === this.playerState.current.muted) {
      this.playerState.current = {
        ...this.playerState.current,
        muted: !value,
      };
    }
  }

  public setAudioVolume(value: number): void {
    const clampedValue = clamp(0, 1, value);
    if (clampedValue !== this.playerState.current.volume) {
      this.playerState.current = {
        ...this.playerState.current,
        volume: clampedValue,
      };
    }
  }

  public addAudioVolume(value: number): void {
    this.setAudioVolume(this.playerState.current.volume + value);
  }

  public setSpeed(value: number) {
    if (value !== this.playerState.current.speed) {
      this.playback.speed = value;
      this.playback.reload();
      this.playerState.current = {
        ...this.playerState.current,
        speed: value,
      };
      this.requestRecalculation();
    }
  }

  public setVariables(variables: Record<string, unknown>) {
    for (const scene of this.playback.onScenesRecalculated.current) {
      scene.variables.updateSignals(variables);
    }
  }

  /**
   * Activate the player.
   *
   * @remarks
   * A player needs to be active in order for the update loop to run. Each
   * player is active by default.
   */
  public activate() {
    this.active = true;
    this.request();
Repository: motion-canvas/motion-canvas
File: packages/docs/src/components/Api/Comment/index.tsx (TypeScript)

Code before:
  full = true,
}: {
  comment: JSONOutput.Comment;
  full?: boolean;
}) {
  const remarks = useMemo(() => {
    return comment?.blockTags?.find(({tag}) => tag === '@remarks');
  }, [comment]);

  return (
    <>
      {full && <Experimental comment={comment} />}
      <Summary id={comment?.summaryId} />
      <Summary id={remarks?.contentId} />
      {full && <FullComment comment={comment} />}
    </>
  );
}

function Experimental({comment}: {comment: JSONOutput.Comment}) {
  const experimental = useMemo(
    () => comment?.modifierTags?.includes('@experimental'),
    [comment],
  );

  return <>{experimental && <ExperimentalWarning />}</>;
}

function FullComment({comment}: {comment: JSONOutput.Comment}) {
  const [collapsed, setCollapsed] = useState(true);
  const preview = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@preview'),
    [comment],
  );
  const examples = useMemo(
    () => comment?.blockTags?.filter(({tag}) => tag === '@example') ?? [],
    [comment],
  );
  const defaultValue = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@defaultValue'),
    [comment],
  );
  const deprecated = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@deprecated'),
    [comment],
  );
  const seeAlso = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@see'),
    [comment],
  );


Review comment (suggestion):
(Nit) From what I understand of JSX, it's good practice to write this as:
```
{experimental ? <ExperimentalWarning /> : null}
```
This prevents `experimental` leaking into the DOM in certain conditions.

Code after:
  full = true,
}: {
  comment: JSONOutput.Comment;
  full?: boolean;
}) {
  const remarks = useMemo(() => {
    return comment?.blockTags?.find(({tag}) => tag === '@remarks');
  }, [comment]);

  return (
    <>
      {full && <Experimental comment={comment} />}
      <Summary id={comment?.summaryId} />
      <Summary id={remarks?.contentId} />
      {full && <FullComment comment={comment} />}
    </>
  );
}

function Experimental({comment}: {comment: JSONOutput.Comment}) {
  const experimental = useMemo(
    () => comment?.modifierTags?.includes('@experimental'),
    [comment],
  );

  return experimental ? <ExperimentalWarning /> : null;
}

function FullComment({comment}: {comment: JSONOutput.Comment}) {
  const [collapsed, setCollapsed] = useState(true);
  const preview = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@preview'),
    [comment],
  );
  const examples = useMemo(
    () => comment?.blockTags?.filter(({tag}) => tag === '@example') ?? [],
    [comment],
  );
  const defaultValue = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@defaultValue'),
    [comment],
  );
  const deprecated = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@deprecated'),
    [comment],
  );
  const seeAlso = useMemo(
    () => comment?.blockTags?.find(({tag}) => tag === '@see'),
    [comment],
  );