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

Code before:
import type {Scene} from './Scene';
import {createSignal, SimpleSignal} from '@motion-canvas/core/lib/signals';

export class Variables {
  private signals: {[key: string]: SimpleSignal<any>} = {};

  public constructor(private readonly scene: Scene) {}

  /**
   * Get variable signal if exists or create signal if not
   *
   * @param name - The name of the variable.
   */
  public get(name: string) {
    const variables = this.scene.project.getVariables();
    if (!(name in variables)) {
      this.scene.project.logger.warn(
        `Variable ${name} has not been set in project ${this.scene.project.name}`,
      );
      return null;
    }
    if (!(name in this.signals)) {
      this.signals[name] = createSignal(variables[name]);
    }

    return this.signals[name];
  }

  /**
   * Update all signals with new project variable values.
   */
  public updateSignals = () => {
    const variables = this.scene.project.getVariables();
    Object.keys(variables).map(variableName => {
      if (variableName in this.signals) {
        this.signals[variableName](variables[variableName]);
      }
    });
  };

Review comment (suggestion):
This method should have a generic type representing the returned value:
```ts
public get<T>(name: string): SimpleSignal<T> {
```

Code after:
import type {Scene} from './Scene';
import {createSignal, SimpleSignal} from '../signals';

export class Variables {
  private signals: {[key: string]: SimpleSignal<any>} = {};

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

  /**
   * Get variable signal if exists or create signal if not
   *
   * @param name - The name of the variable.
   */
  public get<T>(name: string, initial: T): SimpleSignal<T> {
    const variables = this.scene.project.getVariables();
    if (!(name in variables)) {
      this.scene.project.logger.warn(
        `Variable ${name} has not been set in project ${this.scene.project.name}`,
      );
      this.signals[name] = createSignal(initial);
    }
    if (!(name in this.signals)) {
      this.signals[name] = createSignal(variables[name]);
    }

    return this.signals[name];
  }

  /**
   * Update all signals with new project variable values.
   */
  public updateSignals = () => {
    const variables = this.scene.project.getVariables();
    Object.keys(variables).map(variableName => {
      if (variableName in this.signals) {
        this.signals[variableName](variables[variableName]);
      }
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/Variables.ts (TypeScript)

Code before:
import type {Scene} from './Scene';
import {createSignal, SimpleSignal} from '@motion-canvas/core/lib/signals';

export class Variables {
  private signals: {[key: string]: SimpleSignal<any>} = {};

  public constructor(private readonly scene: Scene) {}

  /**
   * Get variable signal if exists or create signal if not
   *
   * @param name - The name of the variable.
   */
  public get(name: string) {
    const variables = this.scene.project.getVariables();
    if (!(name in variables)) {
      this.scene.project.logger.warn(
        `Variable ${name} has not been set in project ${this.scene.project.name}`,
      );
      return null;
    }
    if (!(name in this.signals)) {
      this.signals[name] = createSignal(variables[name]);
    }

    return this.signals[name];
  }

  /**
   * Update all signals with new project variable values.
   */
  public updateSignals = () => {
    const variables = this.scene.project.getVariables();
    Object.keys(variables).map(variableName => {
      if (variableName in this.signals) {
        this.signals[variableName](variables[variableName]);
      }
    });
  };

  /**
   * Destroy all stored signals.
   */
  public destroySignals = () => {

Review comment (suggestion):
I'd consider adding another argument to this method to specify the default value of a variable:
```
public get<T>(name: string, initial: T): SimpleSignal<T> {
```
and return it here instead.

Code after:
import type {Scene} from './Scene';
import {createSignal, SimpleSignal} from '../signals';

export class Variables {
  private signals: {[key: string]: SimpleSignal<any>} = {};

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

  /**
   * Get variable signal if exists or create signal if not
   *
   * @param name - The name of the variable.
   */
  public get<T>(name: string, initial: T): SimpleSignal<T> {
    const variables = this.scene.project.getVariables();
    if (!(name in variables)) {
      this.scene.project.logger.warn(
        `Variable ${name} has not been set in project ${this.scene.project.name}`,
      );
      this.signals[name] = createSignal(initial);
    }
    if (!(name in this.signals)) {
      this.signals[name] = createSignal(variables[name]);
    }

    return this.signals[name];
  }

  /**
   * Update all signals with new project variable values.
   */
  public updateSignals = () => {
    const variables = this.scene.project.getVariables();
    Object.keys(variables).map(variableName => {
      if (variableName in this.signals) {
        this.signals[variableName](variables[variableName]);
      }
    });
  };

  /**
   * Reset all stored signals.
Repository: motion-canvas/motion-canvas
File: packages/core/src/scenes/Variables.ts (TypeScript)

Code before:
import type {Scene} from './Scene';
import {createSignal, SimpleSignal} from '../signals';

export class Variables {
  private signals: {[key: string]: SimpleSignal<any>} = {};

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

  /**
   * Get variable signal if exists or create signal if not
   *
   * @param name - The name of the variable.
   */
  public get<T>(name: string, initial: T): SimpleSignal<T> | null {
    const variables = this.scene.project.getVariables();
    if (!(name in variables)) {
      this.scene.project.logger.warn(
        `Variable ${name} has not been set in project ${this.scene.project.name}`,
      );
      this.signals[name] = createSignal(initial);
    }
    if (!(name in this.signals)) {
      this.signals[name] = createSignal(variables[name]);
    }

    return this.signals[name];
  }

  /**
   * Update all signals with new project variable values.
   */
  public updateSignals = () => {
    const variables = this.scene.project.getVariables();
    Object.keys(variables).map(variableName => {
      if (variableName in this.signals) {
        this.signals[variableName](variables[variableName]);
      }
    });
  };

Review comment (suggestion):
One last tiny issue, I believe it's no longer possible for this method to return null:
```suggestion
  public get<T>(name: string, initial: T): SimpleSignal<T> {
```

Code after:
import type {Scene} from './Scene';
import {createSignal, SimpleSignal} from '../signals';

export class Variables {
  private signals: {[key: string]: SimpleSignal<any>} = {};

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

  /**
   * Get variable signal if exists or create signal if not
   *
   * @param name - The name of the variable.
   */
  public get<T>(name: string, initial: T): SimpleSignal<T> {
    const variables = this.scene.project.getVariables();
    if (!(name in variables)) {
      this.scene.project.logger.warn(
        `Variable ${name} has not been set in project ${this.scene.project.name}`,
      );
      this.signals[name] = createSignal(initial);
    }
    if (!(name in this.signals)) {
      this.signals[name] = createSignal(variables[name]);
    }

    return this.signals[name];
  }

  /**
   * Update all signals with new project variable values.
   */
  public updateSignals = () => {
    const variables = this.scene.project.getVariables();
    Object.keys(variables).map(variableName => {
      if (variableName in this.signals) {
        this.signals[variableName](variables[variableName]);
      }
    });
  };
Repository: motion-canvas/motion-canvas
File: packages/2d/src/utils/patchSignal.ts (TypeScript)

Code before:
  name?: string,
) {
  if (parser) {
    signal.setParser(parser.bind(owner));
  }

  if (name && owner) {
    const setter = owner?.[`set${capitalize(name)}`];
    if (setter) {
      signal.set = (...args: any[]) => {
        setter.apply(owner, args);
        return owner;
      };
    }
    const getter = owner?.[`get${capitalize(name)}`];
    if (getter) {
      signal.get = getter.bind(owner);
    }
    const tweener = owner?.[`tween${capitalize(name)}`];
    if (tweener) {
      signal.doTween = tweener.bind(owner);
    }
    const computer = owner?.[`compute${capitalize(name)}`];
    if (computer) {
      signal.compute = computer.bind(owner);
      if (signal['initial'] === undefined) {
        signal.setInitial(signal.compute!);
      }
    }
  }
}


Review comment (suggestion):
I think we can just make `initial` public since there's already a setter for it and now we're also getting it.

Code after:
import {capitalize} from '@motion-canvas/core/lib/utils';
import {SignalContext} from '@motion-canvas/core/lib/signals';

export function patchSignal<TSetterValue, TValue extends TSetterValue>(
  signal: SignalContext<TSetterValue, TValue>,
  parser?: (value: TSetterValue) => TValue,
  owner?: any,
  name?: string,
) {
  if (parser) {
    signal.setParser(parser.bind(owner));
  }

  if (name && owner) {
    const setter = owner?.[`set${capitalize(name)}`];
    if (setter) {
      signal.set = (...args: any[]) => {
        setter.apply(owner, args);
        return owner;
      };
    }
    const getter = owner?.[`get${capitalize(name)}`];
    if (getter) {
      signal.get = getter.bind(owner);
    }
    const tweener = owner?.[`tween${capitalize(name)}`];
    if (tweener) {
      signal.doTween = tweener.bind(owner);
    }
  }
}
Repository: motion-canvas/motion-canvas
File: packages/2d/src/utils/patchSignal.ts (TypeScript)

Code before:
  signal: SignalContext<TSetterValue, TValue>,
  parser?: (value: TSetterValue) => TValue,
  owner?: any,
  name?: string,
) {
  if (parser) {
    signal.setParser(parser.bind(owner));
  }

  if (name && owner) {
    const setter = owner?.[`set${capitalize(name)}`];
    if (setter) {
      signal.set = (...args: any[]) => {
        setter.apply(owner, args);
        return owner;
      };
    }
    const getter = owner?.[`get${capitalize(name)}`];
    if (getter) {
      signal.get = getter.bind(owner);
    }
    const tweener = owner?.[`tween${capitalize(name)}`];
    if (tweener) {
      signal.doTween = tweener.bind(owner);
    }
    const computer = owner?.[`compute${capitalize(name)}`];
    if (computer) {
      signal.compute = computer.bind(owner);
      if (signal['initial'] === undefined) {
        signal.setInitial(signal.compute!);
      }
    }
  }
}


Review comment (suggestion):
Maybe it should be called `initial[Property]` or `default[Property]` or `getDefault[Property]`?
`compute` may be a bit confusing as it is only used when the signal is not overridden by the user.

Code after:
  signal: SignalContext<TSetterValue, TValue>,
  parser?: (value: TSetterValue) => TValue,
  owner?: any,
  name?: string,
) {
  if (parser) {
    signal.setParser(parser.bind(owner));
  }

  if (name && owner) {
    const setter = owner?.[`set${capitalize(name)}`];
    if (setter) {
      signal.set = (...args: any[]) => {
        setter.apply(owner, args);
        return owner;
      };
    }
    const getter = owner?.[`get${capitalize(name)}`];
    if (getter) {
      signal.get = getter.bind(owner);
    }
    const tweener = owner?.[`tween${capitalize(name)}`];
    if (tweener) {
      signal.doTween = tweener.bind(owner);
    }
  }
}
Repository: motion-canvas/motion-canvas
File: packages/core/src/signals/SignalContext.ts (TypeScript)

Code before:

  public toSignal(): Signal<TSetterValue, TValue, TOwner> {
    return this.invokable;
  }

  public parse(value: TSetterValue): TValue {
    return this.parser(value);
  }

  protected wrap(value: SignalValue<TSetterValue>): SignalValue<TValue> {
    return isReactive(value) ? () => this.parse(value()) : this.parse(value);
  }

  public setInitial(value: SignalValue<TSetterValue>) {
    this.initial = value;
  }

  public setParser(value: (value: TSetterValue) => TValue) {
    this.parser = value;
    if (this.current !== undefined && !isReactive(this.current)) {
      this.last = this.parse(this.current);
    }
    this.markDirty();
  }

  public set(value: SignalValue<TSetterValue> | symbol): TOwner {
    if (value === SignalContext.DEFAULT) {
      value = this.initial!;
    }
    const resolvedValue = value as SignalValue<TSetterValue>;

    if (this.current === value) {
      return this.owner;
    }

    this.current = resolvedValue;
    this.markDirty();

    if (this.dependencies.size > 0) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
    }

    if (!isReactive(resolvedValue)) {
      this.last = this.parse(resolvedValue);
    }

    return this.owner;
  }

  public get(): TValue {

Review comment (suggestion):
Should be:
```suggestion
  public set(value: SignalValue<TSetterValue> | typeof SignalContext.DEFAULT): TOwner {
```

Code after:
  }

  public parse(value: TSetterValue): TValue {
    return this.parser(value);
  }

  protected wrap(value: SignalValue<TSetterValue>): SignalValue<TValue> {
    return isReactive(value) ? () => this.parse(value()) : this.parse(value);
  }

  public setInitial(value: SignalValue<TSetterValue>) {
    this.initial = value;
  }

  public setParser(value: (value: TSetterValue) => TValue) {
    this.parser = value;
    if (this.current !== undefined && !isReactive(this.current)) {
      this.last = this.parse(this.current);
    }
    this.markDirty();
  }

  public set(value: SignalValue<TSetterValue> | typeof DEFAULT): TOwner {
    if (value === DEFAULT) {
      value = this.initial!;
    }

    if (this.current === value) {
      return this.owner;
    }

    this.current = value;
    this.markDirty();

    if (this.dependencies.size > 0) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
    }

    if (!isReactive(value)) {
      this.last = this.parse(value);
    }

    return this.owner;
  }

  public get(): TValue {
    if (this.event.isRaised() && isReactive(this.current)) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
      this.startCollecting();
Repository: motion-canvas/motion-canvas
File: packages/core/src/signals/SignalContext.ts (TypeScript)

Code before:
    if (this.initial !== undefined) {
      this.set(this.initial);
    }
    return this.owner;
  }

  /**
   * Compute the current value of the signal and immediately set it.
   *
   * @remarks
   * This method can be used to stop the signal from updating while keeping its
   * current value.
   *
   * @example
   * ```ts
   * signal.save();
   * // same as:
   * signal(signal());
   * ```
   */
  public save() {
    return this.set(this.get());
  }

  /**
   * Checks if the signal is currently using its initial value.
   *
   * @example
   * ```ts
   *
   * const signal = createSignal(0);
   * signal.isInitial(); // true
   *
   * signal(5);
   * signal.isInitial(); // false
   *
   * signal(SignalContext.DEFAULT);
   * signal.isInitial(); // true
   * ```
   */
  public isInitial() {
    this.collect();
    return this.current === this.initial;
  }

  /**
   * Get the raw value of this signal.
   *
   * @remarks
   * If the signal was provided with a factory function, the function itself
   * will be returned, without invoking it.

Review comment (suggestion):
Docs for functions should use imperative present tense:
```suggestion
   * Check if the signal is currently using its initial value.
```

Code after:

  /**
   * Compute the current value of the signal and immediately set it.
   *
   * @remarks
   * This method can be used to stop the signal from updating while keeping its
   * current value.
   *
   * @example
   * ```ts
   * signal.save();
   * // same as:
   * signal(signal());
   * ```
   */
  public save() {
    return this.set(this.get());
  }

  /**
   * Check if the signal is currently using its initial value.
   *
   * @example
   * ```ts
   *
   * const signal = createSignal(0);
   * signal.isInitial(); // true
   *
   * signal(5);
   * signal.isInitial(); // false
   *
   * signal(DEFAULT);
   * signal.isInitial(); // true
   * ```
   */
  public isInitial() {
    this.collect();
    return this.current === this.initial;
  }

  /**
   * Get the raw value of this signal.
   *
   * @remarks
   * If the signal was provided with a factory function, the function itself
   * will be returned, without invoking it.
   *
   * This method can be used to create copies of signals.
   *
   * @example
   * ```ts
Repository: motion-canvas/motion-canvas
File: packages/core/src/signals/SignalContext.ts (TypeScript)

Code before:
    SignalGetter<TValue>,
    SignalTween<TSetterValue, TValue> {
  /**
   * {@inheritDoc SignalContext.reset}
   */
  reset(): TOwner;

  /**
   * {@inheritDoc SignalContext.save}
   */
  save(): TOwner;

  /**
   * {@inheritDoc SignalContext.isInitial}
   */
  isInitial(): boolean;

  context: TContext;
}

export class SignalContext<
  TSetterValue,
  TValue extends TSetterValue = TSetterValue,
  TOwner = void,
> extends DependencyContext<TOwner> {
  public static readonly DEFAULT = Symbol.for(
    '@motion-canvas/core/signals/default',
  );

  protected current: SignalValue<TSetterValue> | undefined;
  protected last: TValue | undefined;
  protected parser: (value: TSetterValue) => TValue = value => <TValue>value;

  public constructor(
    private initial: SignalValue<TSetterValue> | undefined,
    private readonly interpolation: InterpolationFunction<TValue>,
    owner: TOwner = <TOwner>(<unknown>undefined),
  ) {
    super(owner);

    Object.defineProperty(this.invokable, 'reset', {
      value: this.reset.bind(this),
    });
    Object.defineProperty(this.invokable, 'save', {
      value: this.save.bind(this),
    });
    Object.defineProperty(this.invokable, 'isInitial', {
      value: this.isInitial.bind(this),
    });

    if (this.initial !== undefined) {

Review comment (suggestion):
As you've mentioned in your comment, I think `DEFAULT` should be exported as a separate symbol, not a class member.

Code after:
> extends SignalSetter<TSetterValue, TOwner>,
    SignalGetter<TValue>,
    SignalTween<TSetterValue, TValue> {
  /**
   * {@inheritDoc SignalContext.reset}
   */
  reset(): TOwner;

  /**
   * {@inheritDoc SignalContext.save}
   */
  save(): TOwner;

  /**
   * {@inheritDoc SignalContext.isInitial}
   */
  isInitial(): boolean;

  context: TContext;
}

export class SignalContext<
  TSetterValue,
  TValue extends TSetterValue = TSetterValue,
  TOwner = void,
> extends DependencyContext<TOwner> {
  protected current: SignalValue<TSetterValue> | undefined;
  protected last: TValue | undefined;
  protected parser: (value: TSetterValue) => TValue = value => <TValue>value;

  public constructor(
    private initial: SignalValue<TSetterValue> | undefined,
    private readonly interpolation: InterpolationFunction<TValue>,
    owner: TOwner = <TOwner>(<unknown>undefined),
  ) {
    super(owner);

    Object.defineProperty(this.invokable, 'reset', {
      value: this.reset.bind(this),
    });
    Object.defineProperty(this.invokable, 'save', {
      value: this.save.bind(this),
    });
    Object.defineProperty(this.invokable, 'isInitial', {
      value: this.isInitial.bind(this),
    });

    if (this.initial !== undefined) {
      this.current = this.initial;
      this.markDirty();
Repository: motion-canvas/motion-canvas
File: packages/2d/src/decorators/defaultStyle.ts (TypeScript)

Code before:
import {capitalize} from '@motion-canvas/core/lib/utils';
import {Layout} from 'components';

export function defaultStyle<T>(
  styleName: string,
  parse: (value: string) => T = value => value as T,
): PropertyDecorator {
  return (target: any, key) => {
    target[`getDefault${capitalize(<string>key)}`] = function (this: Layout) {
      this.requestLayoutUpdate();
      const old = (<any>this.element.style)[styleName];
      (<any>this.element.style)[styleName] = '';
      const ret = parse.apply(this, [this.styles.getPropertyValue(styleName)]);
      (<any>this.element.style)[styleName] = old;
      return ret;
    };
  };
}


Review comment (suggestion):
Could be:
```suggestion
      const ret = parse.call(this, this.styles.getPropertyValue(styleName));
```

Code after:
import {capitalize} from '@motion-canvas/core/lib/utils';
import {Layout} from 'components';

export function defaultStyle<T>(
  styleName: string,
  parse: (value: string) => T = value => value as T,
): PropertyDecorator {
  return (target: any, key) => {
    target[`getDefault${capitalize(<string>key)}`] = function (this: Layout) {
      this.requestLayoutUpdate();
      const old = (<any>this.element.style)[styleName];
      (<any>this.element.style)[styleName] = '';
      const ret = parse.call(this, this.styles.getPropertyValue(styleName));
      (<any>this.element.style)[styleName] = old;
      return ret;
    };
  };
}
Repository: motion-canvas/motion-canvas
File: packages/core/src/signals/SignalContext.ts (TypeScript)

Code before:

  public parse(value: TSetterValue): TValue {
    return this.parser(value);
  }

  protected wrap(value: SignalValue<TSetterValue>): SignalValue<TValue> {
    return isReactive(value) ? () => this.parse(value()) : this.parse(value);
  }

  public setInitial(value: SignalValue<TSetterValue>) {
    this.initial = value;
  }

  public setParser(value: (value: TSetterValue) => TValue) {
    this.parser = value;
    if (this.current !== undefined && !isReactive(this.current)) {
      this.last = this.parse(this.current);
    }
    this.markDirty();
  }

  public set(value: SignalValue<TSetterValue> | typeof DEFAULT): TOwner {
    if (value === DEFAULT) {
      value = this.initial!;
    }
    const resolvedValue = value as SignalValue<TSetterValue>;

    if (this.current === value) {
      return this.owner;
    }

    this.current = resolvedValue;
    this.markDirty();

    if (this.dependencies.size > 0) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
    }

    if (!isReactive(resolvedValue)) {
      this.last = this.parse(resolvedValue);
    }

    return this.owner;
  }

  public get(): TValue {
    if (this.event.isRaised() && isReactive(this.current)) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
      this.startCollecting();

Review comment (suggestion):
With `typeof DEFAULT` this is no longer necessary bc typescript will do its magic type narrowing.

Code after:

  public parse(value: TSetterValue): TValue {
    return this.parser(value);
  }

  protected wrap(value: SignalValue<TSetterValue>): SignalValue<TValue> {
    return isReactive(value) ? () => this.parse(value()) : this.parse(value);
  }

  public setInitial(value: SignalValue<TSetterValue>) {
    this.initial = value;
  }

  public setParser(value: (value: TSetterValue) => TValue) {
    this.parser = value;
    if (this.current !== undefined && !isReactive(this.current)) {
      this.last = this.parse(this.current);
    }
    this.markDirty();
  }

  public set(value: SignalValue<TSetterValue> | typeof DEFAULT): TOwner {
    if (value === DEFAULT) {
      value = this.initial!;
    }

    if (this.current === value) {
      return this.owner;
    }

    this.current = value;
    this.markDirty();

    if (this.dependencies.size > 0) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
    }

    if (!isReactive(value)) {
      this.last = this.parse(value);
    }

    return this.owner;
  }

  public get(): TValue {
    if (this.event.isRaised() && isReactive(this.current)) {
      this.dependencies.forEach(dep => dep.unsubscribe(this.markDirty));
      this.dependencies.clear();
      this.startCollecting();
      try {