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

Code before:

/**
 * A node for drawing a two-dimensional grid.
 *
 * @preview
 * ```tsx editor
 * import {Grid, makeScene2D} from '@motion-canvas/2d';
 * import {all, createRef} from '@motion-canvas/core';
 *
 * export default makeScene2D(function* (view) {
 *   const grid = createRef<Grid>();
 *
 *   view.add(
 *     <Grid
 *       ref={grid}
 *       width={'100%'}
 *       height={'100%'}
 *       stroke={'#666'}
 *       start={0.5}
 *       end={0.5}
 *     />,
 *   );
 *
 *   yield* all(
 *     grid().end(1, 1).wait(1).to(0.5, 1),
 *     grid().start(0, 1).wait(1).to(0.5, 1),
 *   );
 * });
 * ```
 */
export class Grid extends Shape {
  /**
   * The spacing between the grid lines.
   */
  @initial(80)
  @vector2Signal('spacing')
  public declare readonly spacing: Vector2Signal<this>;

  /**
   * The percentage that should be clipped from the beginning of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes before the given percentage will
   * be made invisible.
   *
   * This property is usefully for animating the grid appearing on screen.
   */
  @initial(0)
  @signal()
  public declare readonly start: SimpleSignal<number, this>;


Review comment (suggestion):
When providing examples for Nodes, they should be initially visible and then hide/reappear
So that even if the reader doesn't press play, the node will be immediately visible.
```suggestion
 *       start={0}
 *       end={1}
 *     />,
 *   );
 *
 *   yield* all(
 *     grid().end(0.5, 1).to(1, 1).wait(1),
 *     grid().start(0.5, 1).to(0, 1).wait(1),
```

Code after:

/**
 * A node for drawing a two-dimensional grid.
 *
 * @preview
 * ```tsx editor
 * import {Grid, makeScene2D} from '@motion-canvas/2d';
 * import {all, createRef} from '@motion-canvas/core';
 *
 * export default makeScene2D(function* (view) {
 *   const grid = createRef<Grid>();
 *
 *   view.add(
 *     <Grid
 *       ref={grid}
 *       width={'100%'}
 *       height={'100%'}
 *       stroke={'#666'}
 *       start={0}
 *       end={1}
 *     />,
 *   );
 *
 *   yield* all(
 *     grid().end(0.5, 1).to(1, 1).wait(1),
 *     grid().start(0.5, 1).to(0, 1).wait(1),
 *   );
 * });
 * ```
 */
export class Grid extends Shape {
  /**
   * The spacing between the grid lines.
   */
  @initial(80)
  @vector2Signal('spacing')
  public declare readonly spacing: Vector2Signal<this>;

  /**
   * The percentage that should be clipped from the beginning of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes before the given percentage will
   * be made invisible.
   *
   * This property is useful for animating the grid appearing on-screen.
   */
  @initial(0)
  @signal()
  public declare readonly start: SimpleSignal<number, this>;
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Grid.ts (TypeScript)

Code before:
 *     />,
 *   );
 *
 *   yield* all(
 *     grid().end(1, 1).wait(1).to(0.5, 1),
 *     grid().start(0, 1).wait(1).to(0.5, 1),
 *   );
 * });
 * ```
 */
export class Grid extends Shape {
  /**
   * The spacing between the grid lines.
   */
  @initial(80)
  @vector2Signal('spacing')
  public declare readonly spacing: Vector2Signal<this>;

  /**
   * The percentage that should be clipped from the beginning of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes before the given percentage will
   * be made invisible.
   *
   * This property is usefully for animating the grid appearing on screen.
   */
  @initial(0)
  @signal()
  public declare readonly start: SimpleSignal<number, this>;

  /**
   * The percentage that should be clipped from the end of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes after the given percentage will
   * be made invisible.
   *
   * This property is usefully for animating the grid appearing on screen.
   */
  @initial(1)
  @signal()
  public declare readonly end: SimpleSignal<number, this>;

  public constructor(props: GridProps) {
    super(props);
  }

  protected override drawShape(context: CanvasRenderingContext2D) {
    context.save();
    this.applyStyle(context);

Review comment (suggestion):
```suggestion
   * This property is useful for animating the grid appearing on the screen.
```

Code after:
 *     />,
 *   );
 *
 *   yield* all(
 *     grid().end(0.5, 1).to(1, 1).wait(1),
 *     grid().start(0.5, 1).to(0, 1).wait(1),
 *   );
 * });
 * ```
 */
export class Grid extends Shape {
  /**
   * The spacing between the grid lines.
   */
  @initial(80)
  @vector2Signal('spacing')
  public declare readonly spacing: Vector2Signal<this>;

  /**
   * The percentage that should be clipped from the beginning of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes before the given percentage will
   * be made invisible.
   *
   * This property is useful for animating the grid appearing on-screen.
   */
  @initial(0)
  @signal()
  public declare readonly start: SimpleSignal<number, this>;

  /**
   * The percentage that should be clipped from the end of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes after the given percentage will
   * be made invisible.
   *
   * This property is useful for animating the grid appearing on-screen.
   */
  @initial(1)
  @signal()
  public declare readonly end: SimpleSignal<number, this>;

  public constructor(props: GridProps) {
    super(props);
  }

  protected override drawShape(context: CanvasRenderingContext2D) {
    context.save();
    this.applyStyle(context);
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Grid.ts (TypeScript)

Code before:
   */
  @initial(80)
  @vector2Signal('spacing')
  public declare readonly spacing: Vector2Signal<this>;

  /**
   * The percentage that should be clipped from the beginning of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes before the given percentage will
   * be made invisible.
   *
   * This property is usefully for animating the grid appearing on screen.
   */
  @initial(0)
  @signal()
  public declare readonly start: SimpleSignal<number, this>;

  /**
   * The percentage that should be clipped from the end of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes after the given percentage will
   * be made invisible.
   *
   * This property is usefully for animating the grid appearing on screen.
   */
  @initial(1)
  @signal()
  public declare readonly end: SimpleSignal<number, this>;

  public constructor(props: GridProps) {
    super(props);
  }

  protected override drawShape(context: CanvasRenderingContext2D) {
    context.save();
    this.applyStyle(context);
    this.drawRipple(context);

    const spacing = this.spacing();
    const size = this.computedSize().scale(0.5);
    const steps = size.div(spacing).floored;

    for (let x = -steps.x; x <= steps.x; x++) {
      const [from, to] = this.mapPoints(-size.height, size.height);

      context.beginPath();
      context.moveTo(spacing.x * x, from);
      context.lineTo(spacing.x * x, to);
      context.stroke();

Review comment (suggestion):
```suggestion
   * This property is useful for animating the grid appearing on the screen.
```

Code after:
   */
  @initial(80)
  @vector2Signal('spacing')
  public declare readonly spacing: Vector2Signal<this>;

  /**
   * The percentage that should be clipped from the beginning of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes before the given percentage will
   * be made invisible.
   *
   * This property is useful for animating the grid appearing on-screen.
   */
  @initial(0)
  @signal()
  public declare readonly start: SimpleSignal<number, this>;

  /**
   * The percentage that should be clipped from the end of each grid line.
   *
   * @remarks
   * The portion of each grid line that comes after the given percentage will
   * be made invisible.
   *
   * This property is useful for animating the grid appearing on-screen.
   */
  @initial(1)
  @signal()
  public declare readonly end: SimpleSignal<number, this>;

  public constructor(props: GridProps) {
    super(props);
  }

  protected override drawShape(context: CanvasRenderingContext2D) {
    context.save();
    this.applyStyle(context);
    this.drawRipple(context);

    const spacing = this.spacing();
    const size = this.computedSize().scale(0.5);
    const steps = size.div(spacing).floored;

    for (let x = -steps.x; x <= steps.x; x++) {
      const [from, to] = this.mapPoints(-size.height, size.height);

      context.beginPath();
      context.moveTo(spacing.x * x, from);
      context.lineTo(spacing.x * x, to);
      context.stroke();
Repository: motion-canvas/motion-canvas
File: packages/vite-plugin/src/openInExplorer.ts (TypeScript)

Code before:
import {execSync, spawn} from 'child_process';
import {platform} from 'os';
import {existsSync, mkdirSync} from 'fs';

export function openInExplorer(file: string) {
  let explorer: string | null = null;
  const os = platform();

  if (!existsSync(file)) {
    mkdirSync(file, {recursive: true});
  }

  switch (os) {
    case 'win32':
      explorer = 'explorer';
      break;
    case 'linux':
      if (isRunningOnWSL()) {
        explorer = 'bash';
        file = `cd ${file} && explorer.exe .`;
      } else {
        explorer = 'xdg-open';
      }
      break;
    case 'darwin':
      explorer = 'open';
      break;
  }

  if (explorer === 'bash') {
    spawn(explorer, ['-c', file], {detached: true}).unref();
  } else if (explorer) {
    spawn(explorer, [file], {detached: true}).unref();
  } else {
    console.warn(`Unsupported OS: ${os}`);
  }

Review comment (refactor):
I think this should be done wherever the `openInExplorer` function is called.
Since the job of this helper method is to open the given directory in an explorer, I wouldn't expect it to create said directory as a side-effect.

Code after:
import {execSync, spawn} from 'child_process';
import {platform} from 'os';

export function openInExplorer(file: string) {
  let command: string | null = null;
  let args = [file];
  const os = platform();

  switch (os) {
    case 'win32':
      command = 'explorer';
      break;
    case 'linux':
      if (isRunningOnWSL()) {
        command = 'bash';
        args = ['-c', `cd ${file} && explorer.exe .`];
      } else {
        command = 'xdg-open';
      }
      break;
    case 'darwin':
      command = 'open';
      break;
  }

  if (command) {
    spawn(command, args, {detached: true}).unref();
  } else {
    console.warn(`Unsupported OS: ${os}`);
  }
}

function isRunningOnWSL(): boolean {
  try {
    const uname = execSync('uname -a').toString().toLowerCase();
    return uname.includes('microsoft');
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/CodeBlock.ts (TypeScript)

Code before:
type CodePoint = [number, number];
type CodeRange = [CodePoint, CodePoint];

export interface CodeProps extends ShapeProps {
  language?: string;
  children?: Code;
  code?: SignalValue<Code>;
  selection?: CodeRange[];
  theme?: CodeStyle;
}

export interface CodeModification {
  from: Code;
  to: Code;
}

export class CodeBlock extends Shape {
  private static initialized = createComputedAsync(
    () => ready().then(() => true),
    false,
  );

  @initial(0)
  @signal()
  public declare readonly lineCount: SimpleSignal<number, this>;

  @initial('tsx')
  @signal()
  public declare readonly language: SimpleSignal<string, this>;

  @initial('')
  @parser(function (this: CodeBlock, value: Code): CodeTree {
    return typeof value === 'string'
      ? {
          language: this.language(),
          spans: [value],
          nodes: [],
        }
      : value;
  })
  @signal()
  public declare readonly code: Signal<Code, CodeTree, this>;

  @initial(undefined)
  @signal()
  public declare readonly theme: Signal<CodeStyle | null, CodeStyle, this>;

  @initial(lines(0, Infinity))
  @signal()
  public declare readonly selection: SimpleSignal<CodeRange[], this>;


Review comment (suggestion):
It doesn't really make sense for `lineCount` to be a signal since it's not possible to assign it a value in a meaningful way.
Instead, it should be a method marked as `computed`:
```suggestion
  @computed()
  public lineCount(): number {
    // calculate the count
  }
```
`Computed` can be thought of as a read-only signal and can be used in most places a signal can.

Code after:
type CodePoint = [number, number];
type CodeRange = [CodePoint, CodePoint];

export interface CodeProps extends ShapeProps {
  language?: string;
  children?: Code;
  code?: SignalValue<Code>;
  selection?: CodeRange[];
  theme?: CodeStyle;
}

export interface CodeModification {
  from: Code;
  to: Code;
}

export class CodeBlock extends Shape {
  private static initialized = createComputedAsync(
    () => ready().then(() => true),
    false,
  );

  @initial('tsx')
  @signal()
  public declare readonly language: SimpleSignal<string, this>;

  @initial('')
  @parser(function (this: CodeBlock, value: Code): CodeTree {
    return typeof value === 'string'
      ? {
          language: this.language(),
          spans: [value],
          nodes: [],
        }
      : value;
  })
  @signal()
  public declare readonly code: Signal<Code, CodeTree, this>;

  @initial(undefined)
  @signal()
  public declare readonly theme: Signal<CodeStyle | null, CodeStyle, this>;

  @initial(lines(0, Infinity))
  @signal()
  public declare readonly selection: SimpleSignal<CodeRange[], this>;

  protected *tweenSelection(
    value: CodeRange[],
    duration: number,
    timingFunction: TimingFunction,
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/CodeBlock.ts (TypeScript)

Code before:

  protected getLineCountOfTokenArray(tokens: Token[]): number {
    let count = 0;

    for (const token of tokens) {
      for (let i = 0; i < token.code.length; i++) {
        if (token.code[i] === '\n') {
          count++;
        }
      }
    }

    if (tokens.length > 0) {
      count++;
    }

    return count;
  }

  @computed()
  public getLineCount(): number {
    const progress = this.codeProgress();
    if (progress !== null) {
      // Interpolate between the two line counts based on progress (lerping)
      return Math.round(
        this.currentLineCount +
          progress * (this.newLineCount - this.currentLineCount),
      );
    }

    return this.getLineCountOfTokenArray(this.parsed());
  }

  @computed()
  protected parsed() {
    if (!CodeBlock.initialized()) {
      return [];
    }

    return parse(this.code(), {codeStyle: this.theme()});
  }

  public constructor({children, ...rest}: CodeProps) {
    super({
      fontFamily: 'monospace',
      ...rest,
    });
    if (children) {
      this.code(children);
    }
  }

Review comment (suggestion):
We have a function for linear interpolation:
```suggestion
      return Math.round(
        map(this.currentLineCount, this.newLineCount, progress)
      );
```

Code after:

  protected getLineCountOfTokenArray(tokens: Token[]): number {
    let count = 0;

    for (const token of tokens) {
      for (let i = 0; i < token.code.length; i++) {
        if (token.code[i] === '\n') {
          count++;
        }
      }
    }

    if (tokens.length > 0) {
      count++;
    }

    return count;
  }

  @computed()
  public lineCount(): number {
    const progress = this.codeProgress();
    if (progress !== null) {
      return Math.round(
        map(this.currentLineCount, this.newLineCount, progress),
      );
    }

    return this.getLineCountOfTokenArray(this.parsed());
  }

  @computed()
  protected parsed() {
    if (!CodeBlock.initialized()) {
      return [];
    }

    return parse(this.code(), {codeStyle: this.theme()});
  }

  public constructor({children, ...rest}: CodeProps) {
    super({
      fontFamily: 'monospace',
      ...rest,
    });
    if (children) {
      this.code(children);
    }
  }

  @computed()
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/CodeBlock.ts (TypeScript)

Code before:
  private selectionProgress = createSignal<number | null>(null);
  private oldSelection: CodeRange[] | null = null;
  private diffed: MorphToken[] | null = null;
  private currentLineCount = 0;
  private newLineCount = 0;

  protected getLineCountOfTokenArray(tokens: Token[]): number {
    let count = 0;

    for (const token of tokens) {
      for (let i = 0; i < token.code.length; i++) {
        if (token.code[i] === '\n') {
          count++;
        }
      }
    }

    if (tokens.length > 0) {
      count++;
    }

    return count;
  }

  @computed()
  public getLineCount(): number {
    const progress = this.codeProgress();
    if (progress !== null) {
      // Interpolate between the two line counts based on progress (lerping)
      return Math.round(
        this.currentLineCount +
          progress * (this.newLineCount - this.currentLineCount),
      );
    }

    return this.getLineCountOfTokenArray(this.parsed());
  }

  @computed()
  protected parsed() {
    if (!CodeBlock.initialized()) {
      return [];
    }

    return parse(this.code(), {codeStyle: this.theme()});
  }

  public constructor({children, ...rest}: CodeProps) {
    super({
      fontFamily: 'monospace',
      ...rest,

Review comment (suggestion):
By convention, computed methods are called like properties:
```suggestion
  public lineCount(): number {
```

Code after:
  private selectionProgress = createSignal<number | null>(null);
  private oldSelection: CodeRange[] | null = null;
  private diffed: MorphToken[] | null = null;
  private currentLineCount = 0;
  private newLineCount = 0;

  protected getLineCountOfTokenArray(tokens: Token[]): number {
    let count = 0;

    for (const token of tokens) {
      for (let i = 0; i < token.code.length; i++) {
        if (token.code[i] === '\n') {
          count++;
        }
      }
    }

    if (tokens.length > 0) {
      count++;
    }

    return count;
  }

  @computed()
  public lineCount(): number {
    const progress = this.codeProgress();
    if (progress !== null) {
      return Math.round(
        map(this.currentLineCount, this.newLineCount, progress),
      );
    }

    return this.getLineCountOfTokenArray(this.parsed());
  }

  @computed()
  protected parsed() {
    if (!CodeBlock.initialized()) {
      return [];
    }

    return parse(this.code(), {codeStyle: this.theme()});
  }

  public constructor({children, ...rest}: CodeProps) {
    super({
      fontFamily: 'monospace',
      ...rest,
    });
    if (children) {
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Video.ts (TypeScript)

Code before:
  /**
   * Whether the video should be smoothed.
   *
   * @remarks
   * When disabled, the video will be scaled using the nearest neighbor
   * interpolation with no smoothing. The resulting video will appear pixelated.
   *
   * @defaultValue true
   */
  @initial(true)
  @signal()
  public declare readonly smoothing: SimpleSignal<boolean, this>;

  /**
   * Whether this video should loop upon reaching the end.
   */
  @initial(false)
  @signal()
  public declare readonly loop: SimpleSignal<boolean, this>;

  /**
   * The rate at which the video plays, as multiples of the normal speed.
   *
   * @defaultValue 1.0
   */
  @initial(1.0)
  @signal()
  public declare readonly playbackRate: SimpleSignal<number, this>;

  @initial(0)
  @signal()
  protected declare readonly time: SimpleSignal<number, this>;

  @initial(false)
  @signal()
  protected declare readonly playing: SimpleSignal<boolean, this>;

  private lastTime = -1;

  public constructor({play, ...props}: VideoProps) {
    super(props);
    if (play) {
      this.play();
    }
  }

  public isPlaying(): boolean {
    return this.playing();
  }

  public getCurrentTime(): number {

Review comment (suggestion):
```suggestion
   * @defaultValue 1
   */
  @initial(1)
```

Code after:
  public declare readonly alpha: SimpleSignal<number, this>;

  /**
   * Whether the video should be smoothed.
   *
   * @remarks
   * When disabled, the video will be scaled using the nearest neighbor
   * interpolation with no smoothing. The resulting video will appear pixelated.
   *
   * @defaultValue true
   */
  @initial(true)
  @signal()
  public declare readonly smoothing: SimpleSignal<boolean, this>;

  /**
   * Whether this video should loop upon reaching the end.
   */
  @initial(false)
  @signal()
  public declare readonly loop: SimpleSignal<boolean, this>;

  /**
   * The rate at which the video plays, as multiples of the normal speed.
   *
   * @defaultValue 1
   */
  @initial(1)
  @signal()
  public declare readonly playbackRate: SimpleSignal<number, this>;

  @initial(0)
  @signal()
  protected declare readonly time: SimpleSignal<number, this>;

  @initial(false)
  @signal()
  protected declare readonly playing: SimpleSignal<boolean, this>;

  private lastTime = -1;

  public constructor({play, ...props}: VideoProps) {
    super(props);
    if (play) {
      this.play();
    }
  }

  public isPlaying(): boolean {
    return this.playing();
  }
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Video.ts (TypeScript)

Code before:
      this.ratio() ?? video.videoWidth / video.videoHeight
    ).toString();
  }

  protected setCurrentTime(value: number) {
    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 getPlaybackRate() {
    return this.playbackRate.context.getter();
  }

  protected setPlaybackRate(playbackRate: number) {
    this.playbackRate.context.setter(playbackRate);

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

  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):
I mentioned this to give you a complete picture but it's already the default behavior
Conceptually, it's similar to calling the `super` method:
```ts
protected getPlaybackRate() {
  return super.getPlaybackRate();
}
```
So the entire thing can be omitted.

Code after:
    super.applyFlex();
    const video = this.video();
    this.element.style.aspectRatio = (
      this.ratio() ?? video.videoWidth / video.videoHeight
    ).toString();
  }

  protected setCurrentTime(value: number) {
    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));
      }
    }