ronantakizawa/codereview-bench huggingface dataset:
Repository: motion-canvas/motion-canvas
File: packages/core/src/utils/debug.test.ts (TypeScript)

Code before:
    [
      'Arrays',
      [5, 'test', Vector2.one],
      {message: '[5,"test",{"x":1,"y":1}]', object: [5, 'test', Vector2.one]},
    ],
    ['Empty Array', [], {message: '[]', object: []}],
    [
      'Objects',
      {foo: 'bar', baz: 'qux'},
      {message: '{"foo":"bar","baz":"qux"}', object: {foo: 'bar', baz: 'qux'}},
    ],
    ['Empty Objects', {}, {message: '{}', object: {}}],
  ])('log: %s', (_, payload, expected) => {
    const promise = new Promise<void>((resolve, reject) => {
      useLogger().onLogged.subscribe(actualPayload => {
        expect(actualPayload).toEqual<LogPayload>({
          level: LogLevel.Debug,
          ...expected,
        });
        resolve();
      });
    });

    debug(payload);

    return promise;
  });
});


Review comment (bug):
Logging is synchronous, so I think we could just use a mock function:
```suggestion
    const spy = vi.fn();
    useLogger().onLogged.subscribe(spy);

    debug(payload);

    expect(spy).toHaveBeenCalledWith({
      level: LogLevel.Debug,
      ...expected,
    });
```

Code after:
import {describe, test, beforeEach, expect, vi} from 'vitest';
import {setProject, useLogger, debug} from '../utils';
import {Project, ProjectMetadata} from '../Project';
import {Meta} from '../Meta';
import {LogLevel, LogPayload} from '../Logger';
import {Rect, Vector2} from '../types';

describe('debug()', () => {
  beforeEach(() => {
    setProject(new Project({name: 'test', scenes: []}));
  });

  test.each([
    ['Strings', 'test', {message: 'test'}],
    ['Numbers', 5, {message: '5'}],
    ['Null', null, {message: 'null'}],
    ['Undefined', undefined, {message: 'undefined'}],
    ['NaN', NaN, {message: 'NaN'}],
    ['Vector2', Vector2.one, {message: '{"x":1,"y":1}', object: Vector2.one}],
    [
      'Rect',
      new Rect([10, 20, 30, 40]),
      {
        message: '{"x":10,"y":20,"width":30,"height":40}',
        object: new Rect([10, 20, 30, 40]),
      },
    ],
    [
      'Arrays',
      [5, 'test', Vector2.one],
      {message: '[5,"test",{"x":1,"y":1}]', object: [5, 'test', Vector2.one]},
    ],
    ['Empty Array', [], {message: '[]', object: []}],
    [
      'Objects',
      {foo: 'bar', baz: 'qux'},
      {message: '{"foo":"bar","baz":"qux"}', object: {foo: 'bar', baz: 'qux'}},
    ],
    ['Empty Objects', {}, {message: '{}', object: {}}],
  ])('log: %s', (_, payload, expected) => {
    const spy = vi.fn();
    useLogger().onLogged.subscribe(spy);

    debug(payload);

    expect(spy).toHaveBeenNthCalledWith(1, {
      level: LogLevel.Debug,
      ...expected,
    });
  });
});
Repository: motion-canvas/motion-canvas
File: packages/core/src/utils/debug.test.ts (TypeScript)

Code before:
import {describe, test, beforeEach, expect} from 'vitest';
import {setProject, useLogger, debug} from '../utils';
import {Project, ProjectMetadata} from '../Project';
import {Meta} from '../Meta';
import {LogLevel, LogPayload} from '../Logger';
import {Rect, Vector2} from '../types';

describe('debug()', () => {
  beforeEach(() => {
    setProject(
      new Project('test', new Meta<ProjectMetadata>('test'), {scenes: []}),
    );
  });

  test.each([
    ['Strings', 'test', {message: 'test'}],
    ['Numbers', 5, {message: '5'}],
    ['Null', null, {message: 'null'}],
    ['Undefined', undefined, {message: 'undefined'}],
    ['NaN', NaN, {message: 'NaN'}],
    ['Vector2', Vector2.one, {message: '{"x":1,"y":1}', object: Vector2.one}],
    [
      'Rect',
      new Rect([10, 20, 30, 40]),
      {
        message: '{"x":10,"y":20,"width":30,"height":40}',
        object: new Rect([10, 20, 30, 40]),
      },
    ],
    [
      'Arrays',
      [5, 'test', Vector2.one],
      {message: '[5,"test",{"x":1,"y":1}]', object: [5, 'test', Vector2.one]},
    ],
    ['Empty Array', [], {message: '[]', object: []}],
    [

Review comment (suggestion):
This is a deprecated signature, we should pass a config object instead:
```suggestion
      setProject(new Project({name: 'test', scenes: []}));
```
Also, metadata is not necessary.

Code after:
import {describe, test, beforeEach, expect, vi} from 'vitest';
import {setProject, useLogger, debug} from '../utils';
import {Project, ProjectMetadata} from '../Project';
import {Meta} from '../Meta';
import {LogLevel, LogPayload} from '../Logger';
import {Rect, Vector2} from '../types';

describe('debug()', () => {
  beforeEach(() => {
    setProject(new Project({name: 'test', scenes: []}));
  });

  test.each([
    ['Strings', 'test', {message: 'test'}],
    ['Numbers', 5, {message: '5'}],
    ['Null', null, {message: 'null'}],
    ['Undefined', undefined, {message: 'undefined'}],
    ['NaN', NaN, {message: 'NaN'}],
    ['Vector2', Vector2.one, {message: '{"x":1,"y":1}', object: Vector2.one}],
    [
      'Rect',
      new Rect([10, 20, 30, 40]),
      {
        message: '{"x":10,"y":20,"width":30,"height":40}',
        object: new Rect([10, 20, 30, 40]),
      },
    ],
    [
      'Arrays',
      [5, 'test', Vector2.one],
      {message: '[5,"test",{"x":1,"y":1}]', object: [5, 'test', Vector2.one]},
    ],
    ['Empty Array', [], {message: '[]', object: []}],
    [
      'Objects',
      {foo: 'bar', baz: 'qux'},
Repository: motion-canvas/motion-canvas
File: packages/examples/src/scenes/components-switch.tsx (TypeScript)

Code before:
} from '@motion-canvas/core/lib/signals';
import {createRef} from '@motion-canvas/core/lib/utils';
import {all} from '@motion-canvas/core/lib/flow';

export interface SwitchProps extends NodeProps {
  initialState?: SignalValue<boolean>;
  accent?: SignalValue<PossibleColor>;
}

export class Switch extends Node {
  @initial(false)
  @signal()
  public declare readonly initialState: SimpleSignal<boolean, this>;

  @initial('#68ABDF')
  @signal()
  public declare readonly accent: ColorSignal<this>;

  private isOn: boolean;
  private readonly indicatorPosition = createSignal(0);
  private readonly offColor = new Color('#242424');
  private readonly indicator = createRef<Circle>();
  private readonly container = createRef<Rect>();

  public constructor(props?: SwitchProps) {
    super({
      ...props,
    });

    this.indicatorPosition(this.initialState ? 50 : -50);
    this.isOn = this.initialState();

    this.add(
      <>
        <Rect
          ref={this.container}
          fill={this.offColor}
          size={[200, 100]}
          radius={100}
        >
          <Circle
            x={() => this.indicatorPosition()}
            ref={this.indicator}
            size={[80, 80]}
            fill="#ffffff"
          />
        </Rect>
      </>,
    );
  }


Review comment (suggestion):
Super minor detail: Is there a reason why you create a new object and spread the `props` instead of just passing the props directly?

```ts
super(props);
```

Code after:
} from '@motion-canvas/core/lib/signals';
import {createRef} from '@motion-canvas/core/lib/utils';
import {all} from '@motion-canvas/core/lib/flow';

export interface SwitchProps extends NodeProps {
  initialState?: SignalValue<boolean>;
  accent?: SignalValue<PossibleColor>;
}

export class Switch extends Node {
  @initial(false)
  @signal()
  public declare readonly initialState: SimpleSignal<boolean, this>;

  @initial('#68ABDF')
  @signal()
  public declare readonly accent: ColorSignal<this>;

  private isOn: boolean;
  private readonly indicatorPosition = createSignal(0);
  private readonly offColor = new Color('#242424');
  private readonly indicator = createRef<Circle>();
  private readonly container = createRef<Rect>();

  public constructor(props?: SwitchProps) {
    super({
      ...props,
    });

    this.indicatorPosition(this.initialState ? 50 : -50);
    this.isOn = this.initialState();

    this.add(
      <Rect
        ref={this.container}
        fill={this.offColor}
        size={[200, 100]}
        radius={100}
      >
        <Circle
          x={() => this.indicatorPosition()}
          ref={this.indicator}
          size={[80, 80]}
          fill="#ffffff"
        />
      </Rect>,
    );
  }

  public *toggle(Duration: number) {
    yield* all(
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Icon.ts (TypeScript)

Code before:
import {Image, ImageProps} from '../components/Image';
import {colorSignal, computed, initial, signal} from '../decorators';
import {SignalValue, SimpleSignal} from '@motion-canvas/core/lib/signals';
import {useLogger} from '@motion-canvas/core/lib/utils';
import {ColorSignal} from '@motion-canvas/core/lib/types';

export interface IconProps extends ImageProps {
  /**
   * Expects an Icon Identifier.
   * You can find identifiers on [Icônes](https://icones.js.org/collection/all).
   * They can look like this:
   * * mdi:language-typescript
   * * ph:anchor-simple-bold
   * * ph:activity-bold
   */
  icon: SignalValue<string>;

  /**
   * Color of the Icon. Defaults to white.
   * Provide the color in one of the following formats:
   * * named color like `red`, `darkgray`, …
   * * hexadecimal string with # `#bada55`,`#141414`
   * Values can be either RGB or RGBA: `#bada55` or `#bada55aa` (latter is partially transparent)
   * The shorthand version (e.g. `#abc` for `#aabbcc`) is also possible
   */
  color?: SignalValue<string>;
}

/**
 * An Icon Component that provides an easy access to over 150k icons.
 * See https://icones.js.org/collection/all for all available Icons.
 */
export class Icon extends Image {
  /**
   * Expects an Icon Identifier.

Review comment (suggestion):
The properties from the interfaces should inherit the documentation from the class:
```suggestion
  /**
   * {@inheritDoc Icon.icon}
   */
```

Code after:
import {Image, ImageProps} from '../components/Image';
import {colorSignal, computed, initial, signal} from '../decorators';
import {SignalValue, SimpleSignal} from '@motion-canvas/core/lib/signals';
import {useLogger} from '@motion-canvas/core/lib/utils';
import {ColorSignal, PossibleColor} from '@motion-canvas/core/lib/types';

export interface IconProps extends ImageProps {
  /**
   * {@inheritDoc Icon.icon}
   */
  icon: SignalValue<string>;

  /**
   * {@inheritDoc Icon.color}
   */
  color?: SignalValue<PossibleColor>;
}

/**
 * An Icon Component that provides an easy access to over 150k icons.
 * See https://icones.js.org/collection/all for all available Icons.
 */
export class Icon extends Image {
  /**
   * The identifier of the icon.
   *
   * @remarks
   * You can find identifiers on [Icônes](https://icones.js.org).
   * They can look like this:
   * * `mdi:language-typescript`
   * * `ph:anchor-simple-bold`
   * * `ph:activity-bold`
   */
  @signal()
  public declare icon: SimpleSignal<string, this>;
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/TimeEvents.ts (TypeScript)

Code before:
    };
    this.registeredEvents[name] = this.lookup[name];
    this.events.current = Object.values(this.registeredEvents);
    this.didEventsChange = true;
    this.scene.reload();
  }

  /**
   * Register a time event.
   *
   * @param name - The name of the event.
   *
   * @returns The absolute frame at which the event should occur.
   *
   * @internal
   */
  public register(name: string): number {
    if (!this.collisionLookup.has(name)) {
      this.collisionLookup.add(name);
    } else {
      useLogger().error(
        'name: "' +
          name +
          '"' +
          ' has already been used for another event name.',
      );
      return 0;
    }

    const initialTime = this.scene.project.framesToSeconds(
      this.scene.project.frame - this.scene.firstFrame,
    );
    if (!this.lookup[name]) {
      this.didEventsChange = true;
      this.lookup[name] = {
        name,
        initialTime,
        targetTime: initialTime,
        offset: 0,
        stack: new Error().stack,
      };
    } else {
      let changed = false;
      const event = {...this.lookup[name]};

      const stack = new Error().stack;
      if (event.stack !== stack) {
        event.stack = stack;
        changed = true;
      }


Review comment (suggestion):
Three things:
- you can access the logger through `this.scene.project.logger`
- it's better to use template literals for messages like this
- you can add a stack trace to the log to show people where in the code this event was:
```suggestion
      this.scene.project.logger.error({
        message: `name "${name}" has already been used for another event name.`,
        stack: new Error().stack,
      });
```

Code after:
    this.registeredEvents[name] = this.lookup[name];
    this.events.current = Object.values(this.registeredEvents);
    this.didEventsChange = true;
    this.scene.reload();
  }

  /**
   * Register a time event.
   *
   * @param name - The name of the event.
   *
   * @returns The absolute frame at which the event should occur.
   *
   * @internal
   */
  public register(name: string): number {
    if (this.collisionLookup.has(name)) {
      this.scene.project.logger.error({
        message: `name "${name}" has already been used for another event name.`,
        stack: new Error().stack,
      });
      return 0;
    }

    this.collisionLookup.add(name);

    const initialTime = this.scene.project.framesToSeconds(
      this.scene.project.frame - this.scene.firstFrame,
    );
    if (!this.lookup[name]) {
      this.didEventsChange = true;
      this.lookup[name] = {
        name,
        initialTime,
        targetTime: initialTime,
        offset: 0,
        stack: new Error().stack,
      };
    } else {
      let changed = false;
      const event = {...this.lookup[name]};

      const stack = new Error().stack;
      if (event.stack !== stack) {
        event.stack = stack;
        changed = true;
      }

      if (event.initialTime !== initialTime) {
        event.initialTime = initialTime;
        changed = true;
Repository: motion-canvas/motion-canvas
File: packages/core/src/tweening/spring.ts (TypeScript)

Code before:
  }
  if (spring.damping < 0) {
    useLogger().error(
      new Error('Spring damping must be greater or equal to 0.'),
    );
    return;
  }

  const project = useProject();
  const thread = useThread();

  const startTime = thread.time();

  // figure out the step length for each frame
  const timeStep = 1 / project.framerate;

  settleTolerance = settleTolerance ?? 0.001;
  let position = from;
  let velocity = spring.initalVelocity ?? 0;
  let settled = false;

  onProgress(from, 0);
  while (!settled) {
    const positionDelta = position - to;

    // using hooks law: F=-kx; with k being the spring constant and x the offset to the settling position
    const force = -spring.stiffness * positionDelta - spring.damping * velocity;

    //updating the velocity
    velocity += (force / spring.mass) * timeStep;

    // apply velocity to the position
    position += velocity * timeStep;

    // the simulation would run forever but is often barely noticable after some seconds
    // we fix this by checking if the delta delta between $to and $position is smaller than the given $settleTolerance
    // but we also need to check if the velocity is below the tolerance too
    if (
      Math.abs(to) - Math.abs(position) < settleTolerance &&
      Math.abs(velocity) < settleTolerance
    ) {
      settled = true;
    }

    const time = project.time - startTime;
    const value = position;

    if (time > 0) {
      onProgress(value, time);
    }
    yield;

Review comment (suggestion):
```suggestion
    // Using hooks law: F=-kx; with k being the spring constant and x the offset 
    // to the settling position
```

Code after:
  settleToleranceOrOnProgress: number | ProgressFunction,
  onProgressOrOnEnd?: ProgressFunction,
  onEnd?: ProgressFunction,
): ThreadGenerator {
  const settleTolerance =
    typeof settleToleranceOrOnProgress === 'number'
      ? settleToleranceOrOnProgress
      : 0.001;

  onEnd =
    typeof settleToleranceOrOnProgress === 'number' ? onEnd : onProgressOrOnEnd;

  const onProgress: ProgressFunction = (value: number, time: number) => {
    if (typeof settleToleranceOrOnProgress === 'function') {
      settleToleranceOrOnProgress(value, time);
    } else if (typeof onProgressOrOnEnd === 'function') {
      onProgressOrOnEnd(value, time);
    }
  };

  spring = spring ?? {
    mass: 0.05,
    stiffness: 10,
    damping: 0.5,
  };

  if (spring.mass <= 0) {
    useLogger().error(new Error('Spring mass must be greater than 0.'));
    return;
  }
  if (spring.stiffness < 0) {
    useLogger().error(
      new Error('Spring stiffness must be greater or equal to 0.'),
    );
    return;
  }
  if (spring.damping < 0) {
    useLogger().error(
      new Error('Spring damping must be greater or equal to 0.'),
    );
    return;
  }

  const project = useProject();
  const thread = useThread();

  let position = from;
  let velocity = spring.initialVelocity ?? 0;

  const update = (dt: number) => {
    if (spring === null) {
Repository: motion-canvas/motion-canvas
File: packages/core/src/tweening/spring.ts (TypeScript)

Code before:
import {decorate, threadable} from '../decorators';
import {ThreadGenerator} from '../threading';
import {useProject, useThread, useLogger} from '../utils';

decorate(spring, threadable());
export function* spring(
  spring: Spring | null,
  from: number,
  to: number,
  settleTolerance: number | null,
  onProgress: (value: number, time: number) => void,
  onEnd?: (value: number, time: number) => void,
): ThreadGenerator {
  spring = spring ?? {
    mass: 0.05,
    stiffness: 10,
    damping: 0.5,
  };

  if (spring.mass <= 0) {
    useLogger().error(new Error('Spring mass must be greater 0.'));
    return;
  }
  if (spring.stiffness < 0) {
    useLogger().error(
      new Error('Spring stiffness must be greater or equal to 0.'),
    );
    return;
  }
  if (spring.damping < 0) {
    useLogger().error(
      new Error('Spring damping must be greater or equal to 0.'),
    );
    return;
  }

  const project = useProject();
  const thread = useThread();

  const startTime = thread.time();

  // figure out the step length for each frame
  const timeStep = 1 / project.framerate;

  settleTolerance = settleTolerance ?? 0.001;
  let position = from;

Review comment (suggestion):
```suggestion
    useLogger().error(new Error('Spring mass must be greater than 0.'));
```

Code after:
import {decorate, threadable} from '../decorators';
import {ThreadGenerator} from '../threading';
import {useProject, useThread, useLogger} from '../utils';

type ProgressFunction = (value: number, time: number) => void;

decorate(spring, threadable());
export function spring(
  spring: Spring | null,
  from: number,
  to: number,
  settleTolerance: number,
  onProgress: ProgressFunction,
  onEnd?: ProgressFunction,
): ThreadGenerator;

export function spring(
  spring: Spring | null,
  from: number,
  to: number,
  onProgress: ProgressFunction,
  onEnd?: ProgressFunction,
): ThreadGenerator;

export function* spring(
  spring: Spring | null,
  from: number,
  to: number,
  settleToleranceOrOnProgress: number | ProgressFunction,
  onProgressOrOnEnd?: ProgressFunction,
  onEnd?: ProgressFunction,
): ThreadGenerator {
  const settleTolerance =
    typeof settleToleranceOrOnProgress === 'number'
      ? settleToleranceOrOnProgress
      : 0.001;

  onEnd =
    typeof settleToleranceOrOnProgress === 'number' ? onEnd : onProgressOrOnEnd;

  const onProgress: ProgressFunction = (value: number, time: number) => {
    if (typeof settleToleranceOrOnProgress === 'function') {
      settleToleranceOrOnProgress(value, time);
    } else if (typeof onProgressOrOnEnd === 'function') {
      onProgressOrOnEnd(value, time);
    }
Repository: motion-canvas/motion-canvas
File: packages/core/src/tweening/spring.ts (TypeScript)

Code before:
  stiffness: number;
  damping: number;
  initalVelocity?: number;
}

export function makeSpring(
  mass: number,
  stiffness: number,
  damping: number,
  initalVelocity?: number,
): Spring {
  return {
    mass,
    stiffness,
    damping,
    initalVelocity,
  };
}

export const beatSpring: Spring = makeSpring(0.13, 5.7, 1.2, 10.0);
export const ploppSpring: Spring = makeSpring(0.2, 20.0, 0.68, 0.0);
export const bounceSpring: Spring = makeSpring(0.08, 4.75, 0.05, 0.0);
export const swingSpring: Spring = makeSpring(0.39, 19.85, 2.82, 0.0);
export const jumpSpring: Spring = makeSpring(0.04, 10.0, 0.7, 8.0);
export const strikeSpring: Spring = makeSpring(0.03, 20.0, 0.9, 4.8);
export const smoothSpring: Spring = makeSpring(0.16, 15.35, 1.88, 0.0);


Review comment (suggestion):
`const`s that are not functions should use PascalCase:
```suggestion
export const BeatSpring: Spring = makeSpring(0.13, 5.7, 1.2, 10.0);
export const PlopSpring: Spring = makeSpring(0.2, 20.0, 0.68, 0.0);
export const BounceSpring: Spring = makeSpring(0.08, 4.75, 0.05, 0.0);
export const SwingSpring: Spring = makeSpring(0.39, 19.85, 2.82, 0.0);
export const JumpSpring: Spring = makeSpring(0.04, 10.0, 0.7, 8.0);
export const StrikeSpring: Spring = makeSpring(0.03, 20.0, 0.9, 4.8);
export const SmoothSpring: Spring = makeSpring(0.16, 15.35, 1.88, 0.0);
```

Code after:
    position += velocity * dt;
  };

  // Set simulation constant framerate
  const simulationFrames = 120;

  // Calculate a timestep based on on the simulation framerate
  const timeStep = 1 / simulationFrames;

  onProgress(from, 0);

  const startTime = thread.time();
  let simulationTime = startTime;

  let settled = false;
  while (!settled) {
    while (simulationTime < project.time) {
      const difference = project.time - simulationTime;

      if (timeStep > difference) {
        update(difference);
        simulationTime = project.time;
      } else {
        update(timeStep);
        simulationTime += timeStep;
      }

      // Perform the check during every iteration:
      if (
        Math.abs(to - position) < settleTolerance &&
        Math.abs(velocity) < settleTolerance
      ) {
        // Set the thread time to simulation time:
        thread.time(simulationTime);
        settled = true;
        // Break out when settled
        break;
      }
    }

    // Only yield if we haven't settled yet.
    if (!settled) {
      onProgress(position, project.time - startTime);
      yield;
    }
  }

  onProgress(to, project.time - startTime);
  onEnd?.(to, project.time - startTime);
}