ronantakizawa/codereview-bench huggingface dataset:
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?: Lang;
  children?: CodeTree | string;
  code?: SignalValue<CodeTree | string>;
  selection?: CodeRange[];
  theme?: CodeStyle;
  stockTheme?: Theme;
}

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

export class CodeBlock extends Shape {
  @initial('tsx')
  @signal()
  public declare readonly language: SimpleSignal<Lang, this>;

  @initial(undefined)
  @signal()
  public declare readonly stockTheme: SimpleSignal<Theme | undefined, this>;

  protected static loadedLanguages = new Map<Lang, boolean>();
  protected static loadedThemes = new Map<Theme, boolean>();

  @computed()
  protected isReady() {
    const language = this.language();
    const theme = this.stockTheme();
    if (
      !CodeBlock.loadedLanguages.has(language) ||
      (theme != null && !CodeBlock.loadedThemes.has(theme))
    ) {
      DependencyContext.collectPromise(
        ready({
          languages: [language],
          themes: theme != null ? [theme] : [],
        }).then(() => {
          CodeBlock.loadedLanguages.set(language, true);
          if (theme != null) {
            CodeBlock.loadedThemes.set(theme, true);
          }
        }),
      );
      return false;
    }

Review comment (suggestion):
The default value should be `null` to be consistent with other Node properties:
```suggestion
  @initial(null)
  @signal()
  public declare readonly stockTheme: SimpleSignal<Theme | null, this>;
```

Code after:

type CodePoint = [number, number];
type CodeRange = [CodePoint, CodePoint];

export interface CodeProps extends ShapeProps {
  language?: Lang;
  children?: CodeTree | string;
  code?: SignalValue<CodeTree | string>;
  selection?: CodeRange[];
  theme?: CodeStyle;
  stockTheme?: Theme;
}

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

export class CodeBlock extends Shape {
  @initial('tsx')
  @signal()
  public declare readonly language: SimpleSignal<Lang, this>;

  @initial(null)
  @signal()
  public declare readonly stockTheme: SimpleSignal<Theme | undefined, this>;

  protected static loadedLanguages = new Map<Lang, boolean>();
  protected static loadedThemes = new Map<Theme, boolean>();

  @computed()
  protected isReady() {
    const language = this.language();
    if (!CodeBlock.loadedLanguages.has(language)) {
      CodeBlock.loadedLanguages.set(language, false);
      DependencyContext.collectPromise(
        ready({languages: [language]}).then(() => {
          CodeBlock.loadedLanguages.set(language, true);
        }),
      );
      return false;
    }

    const theme = this.stockTheme();
    if (theme != null && !CodeBlock.loadedThemes.has(theme)) {
      CodeBlock.loadedThemes.set(theme, false);
      DependencyContext.collectPromise(
        ready({themes: [theme]}).then(() => {
          CodeBlock.loadedThemes.set(theme, true);
        }),
      );
Repository: motion-canvas/motion-canvas
File: packages/ui/src/contexts/shortcuts.tsx (TypeScript)

Code before:
import {ComponentChildren, createContext} from 'preact';
import {useContext, useState} from 'preact/hooks';

export type Shortcut = {
  key: string;
  action: string;
  isGlobal?: boolean;
};

export type ShortcutModules = 'global' | 'timeline' | 'none';
type ShortcutsByModule = Record<ShortcutModules, Shortcut[]>;
type ShortcutsState = {
  currentModule: ShortcutModules;
  shortcuts: ShortcutsByModule;
  setCurrentModule?: (module: ShortcutModules) => void;
};

const initialshortcuts: ShortcutsByModule = {
  global: [
    {key: 'Space', action: 'Toggle playback', isGlobal: true},
    {key: '←', action: 'Previous frame', isGlobal: true},
    {key: '→', action: 'Next frame', isGlobal: true},
    {key: 'Shift + ←', action: 'Reset to first frame', isGlobal: true},
    {key: 'Shift + →', action: 'Seek to last frame', isGlobal: true},
    {key: 'm', action: 'Toggle audio', isGlobal: true},
    {key: 'l', action: 'Toggle loop', isGlobal: true},
  ],
  timeline: [{key: 'f', action: 'Focus Playhead'}],
  none: [],
};

const ShortcutsContext = createContext<ShortcutsState>({
  currentModule: 'none',
  shortcuts: initialshortcuts,
});

export function ShortcutsProvider({children}: {children: ComponentChildren}) {
  const [currentModule, setCurrentModule] = useState<ShortcutModules>('none');

  return (
    <ShortcutsContext.Provider
      value={{currentModule, setCurrentModule, shortcuts: initialshortcuts}}
    >
      {children}
    </ShortcutsContext.Provider>

Review comment (suggestion):
Is `isGlobal` necessary?
Since all global keys by definition are in the `global` module, we could just assume that it will never be set as the current module.

Code after:
import {ComponentChildren, createContext} from 'preact';
import {useContext, useState} from 'preact/hooks';

export type Shortcut = {
  key: string;
  action: string;
};

export type ShortcutModules = 'global' | 'timeline' | 'viewport' | 'none';
type ShortcutsByModule = Record<ShortcutModules, Shortcut[]>;
type ShortcutsState = {
  currentModule: ShortcutModules;
  shortcuts: ShortcutsByModule;
  setCurrentModule?: (module: ShortcutModules) => void;
};

const InitialShortcuts: ShortcutsByModule = {
  global: [
    {key: 'Space', action: 'Toggle playback'},
    {key: '<-', action: 'Previous frame'},
    {key: '->', action: 'Next frame'},
    {key: 'Shift + <-', action: 'Reset to first frame'},
    {key: 'Shift + ->', action: 'Seek to last frame'},
    {key: 'M', action: 'Toggle audio'},
    {key: 'L', action: 'Toggle loop'},
  ],
  viewport: [
    {key: '0', action: 'Reset zoom'},
    {key: '=', action: 'Zoom in'},
    {key: '-', action: 'Zoom out'},
    {key: "'", action: 'Toggle grid'},
  ],
  timeline: [{key: 'F', action: 'Focus playhead'}],
  none: [],
};

const ShortcutsContext = createContext<ShortcutsState>({
  currentModule: 'none',
  shortcuts: InitialShortcuts,
});

export function ShortcutsProvider({children}: {children: ComponentChildren}) {
  const [currentModule, setCurrentModule] = useState<ShortcutModules>('none');

  return (
Repository: motion-canvas/motion-canvas
File: packages/ui/src/Editor.tsx (TypeScript)

Code before:
import {Sidebar} from './components/sidebar';
import {Timeline} from './components/timeline';
import {Footer} from './components/footer';
import {Viewport} from './components/viewport';
import {ResizeableLayout} from './components/layout';
import {useState} from 'preact/hooks';

export function Editor() {
  const [sidebarOpen, setSidebarOpen] = useState(false);

  return (
    <ResizeableLayout
      id={'main-timeline'}
      size={0.7}
      vertical
      start={
        <ResizeableLayout
          resizeable={sidebarOpen}
          id={'sidebar-vieport'}
          start={<Sidebar setOpen={setSidebarOpen} />}
          end={<Viewport />}
        />
      }
      end={<Timeline />}
      footer={<Footer />}
    />
  );
}


Review comment (suggestion):
`ResizeableLayout` is meant to have only two panes.
We should put the status bar under the timeline instead:
```suggestion
      end={
        <div className="add-necessary-css">
          <Timeline />
          <Footer />
        </div>
      }
```

Code after:
import {Sidebar} from './components/sidebar';
import {Timeline} from './components/timeline';
import {Footer} from './components/footer';
import {Viewport} from './components/viewport';
import {ResizeableLayout} from './components/layout';
import {useState} from 'preact/hooks';
import {usePresenterState} from './hooks';
import {PresenterState} from '@motion-canvas/core';
import {PresentationMode} from './components/presentation';

export function Editor() {
  const state = usePresenterState();
  const [sidebarOpen, setSidebarOpen] = useState(false);

  return state === PresenterState.Initial ? (
    <ResizeableLayout
      id={'main-timeline'}
      size={0.7}
      vertical
      start={
        <ResizeableLayout
          resizeable={sidebarOpen}
          id={'sidebar-vieport'}
          start={<Sidebar setOpen={setSidebarOpen} />}
          end={<Viewport />}
        />
      }
      end={
        <>
          <Timeline />
          <Footer />
        </>
      }
    />
  ) : (
    <PresentationMode />
  );
}
Repository: motion-canvas/motion-canvas
File: packages/ui/src/Editor.tsx (TypeScript)

Code before:
import {Sidebar} from './components/sidebar';
import {Timeline} from './components/timeline';
import {Footer} from './components/footer';
import {Viewport} from './components/viewport';
import {ResizeableLayout} from './components/layout';
import {useState} from 'preact/hooks';

export function Editor() {
  const [sidebarOpen, setSidebarOpen] = useState(false);

  return (
    <ResizeableLayout
      id={'main-timeline'}
      size={0.7}
      vertical
      start={
        <ResizeableLayout
          resizeable={sidebarOpen}
          id={'sidebar-vieport'}
          start={<Sidebar setOpen={setSidebarOpen} />}
          end={<Viewport />}
        />
      }
      end={
        <div>
          <Timeline />
          <Footer />
        </div>
      }
    />
  );
}


Review comment (suggestion):
I think some CSS will be needed here because right now the timeline doesn't expand correctly:
![image](https://user-images.githubusercontent.com/64662184/223293036-ae195e74-54fc-4039-8aa2-6d6792de8121.png)


Code after:
import {Sidebar} from './components/sidebar';
import {Timeline} from './components/timeline';
import {Footer} from './components/footer';
import {Viewport} from './components/viewport';
import {ResizeableLayout} from './components/layout';
import {useState} from 'preact/hooks';
import {usePresenterState} from './hooks';
import {PresenterState} from '@motion-canvas/core';
import {PresentationMode} from './components/presentation';

export function Editor() {
  const state = usePresenterState();
  const [sidebarOpen, setSidebarOpen] = useState(false);

  return state === PresenterState.Initial ? (
    <ResizeableLayout
      id={'main-timeline'}
      size={0.7}
      vertical
      start={
        <ResizeableLayout
          resizeable={sidebarOpen}
          id={'sidebar-vieport'}
          start={<Sidebar setOpen={setSidebarOpen} />}
          end={<Viewport />}
        />
      }
      end={
        <>
          <Timeline />
          <Footer />
        </>
      }
    />
  ) : (
    <PresentationMode />
  );
}
Repository: motion-canvas/motion-canvas
File: packages/2d/src/components/Polygon.ts (TypeScript)

Code before:
  /**
   * Changes the starting angle (in degrees) of the polygon.
   *
   * @remarks
   * Use this to change the apparent rotation of the polygon.
   *
   * @example
   * ```tsx
   * <Polygon
   *   x={0}
   *   y={0}
   *   width={320}
   *   height={320}
   *   nSides={6}
   *   angle={30}
   *   stroke={'#fff'}
   *   lineWidth={8}
   *   fill={'lightseagreen'}
   * />
   * ```
   */
  @initial(0)
  @signal()
  public declare readonly angle: SimpleSignal<number, this>;

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

  protected override getPath(): Path2D {
    const path = new Path2D();
    const angle = (this.angle() / 360) * 2 * Math.PI;
    const width = this.width() / 2;
    const height = this.height() / 2;
    const nSides = this.nSides();
    let x0 = 0;
    let y0 = 0;

    for (let i = 0; i <= nSides; i += 1) {
      const theta = (i * 2 * Math.PI) / nSides - angle;
      const x = width * Math.sin(theta);
      const y = -height * Math.cos(theta);
      if (i == 0) {
        path.moveTo(x, y);
        x0 = x;
        y0 = y;
      } else {
        path.lineTo(x, y);
      }
    }
    path.lineTo(x0, y0);

Review comment (suggestion):
`Shape`s should also implement a `getRipplePath()` method that generates the same path but expanded by `this.rippleSize()`. This is used for ripple effects:
```ts
yield* shape().ripple();
```

Code after:
   * Sets the number of sides of the polygon.
   *
   * @remarks
   * For example, a value of 6 creates a hexagon.
   *
   *
   * @example
   * ```tsx
   * <Polygon
   *   size={320}
   *   sides={7}
   *   stroke={'#fff'}
   *   lineWidth={8}
   *   fill={'lightseagreen'}
   * />
   * ```
   */
  @initial(6)
  @signal()
  public declare readonly sides: SimpleSignal<number, this>;

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

  protected override getPath(): Path2D {
    const path = new Path2D();
    const sides = this.sides();

    const box = BBox.fromSizeCentered(this.size());
    drawPolygon(path, box, sides);

    return path;
  }
  protected override getRipplePath(): Path2D {
    const path = new Path2D();
    const sides = this.sides();
    const rippleSize = this.rippleSize();

    const box = BBox.fromSizeCentered(this.size()).expand(rippleSize);
    drawPolygon(path, box, sides);

    return path;
  }
}
Repository: motion-canvas/motion-canvas
File: packages/ui/src/components/settings/RenderingSettings.tsx (TypeScript)

Code before:
        <Select
          options={FileTypes}
          value={state.fileType}
          onChange={value =>
            meta.rendering.fileType.set(value as CanvasOutputMimeType)
          }
        />
      </Group>
      {state.fileType !== 'image/png' && (
        <Group>
          <Label>quality (%)</Label>
          <Input
            type="number"
            min={0}
            max={100}
            value={state.quality * 100}
            onChange={event => {
              meta.rendering.quality.set(
                parseFloat((event.target as HTMLInputElement).value) / 100,
              );
            }}
          />
        </Group>
      )}
      <Group>
        <Label>{meta.rendering.groupByScene.name}</Label>
        <Input
          type="checkbox"
          checked={Boolean(state.groupByScene)}
          onChange={() => {
            meta.rendering.groupByScene.set(!state.groupByScene);
          }}
        />
      </Group>
    </>
  );
}


Review comment (suggestion):
i'm not sure if this meant to be pulled from the `meta` to be honest. `MetaField` documentation mentions something like this: https://github.com/motion-canvas/motion-canvas/blob/1c259d0d574bb56dbc8bc448300d9b94ee4d0bc4/packages/core/src/meta/MetaField.ts#L25-L31
while no other fields use it. depending on your answer i can either change it all to read from `MetaField` value, or use string literal here instead.

Code after:
        <Select
          options={FileTypes}
          value={state.fileType}
          onChange={value =>
            meta.rendering.fileType.set(value as CanvasOutputMimeType)
          }
        />
      </Group>
      {state.fileType !== 'image/png' && (
        <Group>
          <Label>quality (%)</Label>
          <Input
            type="number"
            min={0}
            max={100}
            value={state.quality * 100}
            onChange={event => {
              meta.rendering.quality.set(
                parseFloat((event.target as HTMLInputElement).value) / 100,
              );
            }}
          />
        </Group>
      )}
      <Group>
        <Label>group by scene</Label>
        <Input
          type="checkbox"
          checked={state.groupByScene}
          onChange={() => {
            meta.rendering.groupByScene.set(!state.groupByScene);
          }}
        />
      </Group>
    </>
  );
}
Repository: motion-canvas/motion-canvas
File: packages/ui/src/components/settings/RenderingSettings.tsx (TypeScript)

Code before:
          onChange={value =>
            meta.rendering.fileType.set(value as CanvasOutputMimeType)
          }
        />
      </Group>
      {state.fileType !== 'image/png' && (
        <Group>
          <Label>quality (%)</Label>
          <Input
            type="number"
            min={0}
            max={100}
            value={state.quality * 100}
            onChange={event => {
              meta.rendering.quality.set(
                parseFloat((event.target as HTMLInputElement).value) / 100,
              );
            }}
          />
        </Group>
      )}
      <Group>
        <Label>{meta.rendering.groupByScene.name}</Label>
        <Input
          type="checkbox"
          checked={Boolean(state.groupByScene)}
          onChange={() => {
            meta.rendering.groupByScene.set(!state.groupByScene);
          }}
        />
      </Group>
    </>
  );
}


Review comment (suggestion):
The cast is not necessary:
```suggestion
          checked={state.groupByScene}
```
The type system prevents anything other than a boolean to be set as the value of this field.
If you want to make sure that the value read from the meta file is correct, you should create a new `BoolMetaField` class and override the `parse` method.

Code after:
          onChange={value =>
            meta.rendering.fileType.set(value as CanvasOutputMimeType)
          }
        />
      </Group>
      {state.fileType !== 'image/png' && (
        <Group>
          <Label>quality (%)</Label>
          <Input
            type="number"
            min={0}
            max={100}
            value={state.quality * 100}
            onChange={event => {
              meta.rendering.quality.set(
                parseFloat((event.target as HTMLInputElement).value) / 100,
              );
            }}
          />
        </Group>
      )}
      <Group>
        <Label>group by scene</Label>
        <Input
          type="checkbox"
          checked={state.groupByScene}
          onChange={() => {
            meta.rendering.groupByScene.set(!state.groupByScene);
          }}
        />
      </Group>
    </>
  );
}
Repository: motion-canvas/motion-canvas
File: packages/2d/src/curves/CurvePoint.ts (TypeScript)

Code before:
import type {Vector2} from '@motion-canvas/core/lib/types';

export interface CurvePoint {
  position: Vector2;
  /**
   * @deprecated The tangent is currently inconsistent for different types of
   * curves and may sometimes return the normal of the point, instead. This will
   * be fixed in the next major version but is kept as is for now for backwards
   * compatibility reasons. To always get the real tangent of the point, you can
   * use `normal.flipped.perpendicular`, instead.
   */
  tangent: Vector2;
  normal: Vector2;
}


Review comment (suggestion):
When used as a block tag, `@deprecated` should be formatted with a new line:
```suggestion
   * @deprecated 
   * The tangent is currently inconsistent for different types of
```

Code after:
import type {Vector2} from '@motion-canvas/core/lib/types';

export interface CurvePoint {
  position: Vector2;
  /**
   * @deprecated
   * The tangent is currently inconsistent for different types of curves and may
   * sometimes return the normal of the point, instead. This will be fixed in
   * the next major version but is kept as is for now for backwards
   * compatibility reasons. To always get the real tangent of the point, you can
   * use `normal.flipped.perpendicular`, instead.
   */
  tangent: Vector2;
  normal: Vector2;
}
Repository: motion-canvas/motion-canvas
File: packages/core/src/flow/loopUntil.ts (TypeScript)

Code before:
import {useDuration, usePlayback, useThread} from '../utils';
import {decorate, threadable} from '../decorators';
import {ThreadGenerator} from '../threading';
import {LoopCallback} from './loop';

decorate(loopUntil, threadable());
/**
 * Run the given generator until an event occurs.
 *
 * @example
 * ```ts
 * yield* loopUntil(
 *   'Stop Looping',
 *   () => circle().position.x(-10, 0.1).to(10, 0.1)
 * );
 * ```
 *
 * @param event - The event.
 * @param tasks - A list of tasks to run.
 */
export function* loopUntil(
  event: string,
  factory: LoopCallback,
): ThreadGenerator {
  const thread = useThread();
  const step = usePlayback().framesToSeconds(1);
  const targetTime = thread.time() + useDuration(event);

  let iteration = 0;
  while (targetTime - step > thread.fixed) {
    const generator = factory(iteration);
    if (generator) {
      yield* generator;
    } else {
      yield;
    }
    iteration += 1;
  }
  thread.time(targetTime);
}


Review comment (bug):
This parameter description is incorrect. The parameter should be called `factory` instead of `tasks`. It's also not a list of tasks to run but a function creating the generator to run. This was probably incorrectly copy&pasted from the doc block of `all`.

I'd suggest using the description of `factory` from the `loop` function.

Code after:
import {useDuration, usePlayback, useThread} from '../utils';
import {decorate, threadable} from '../decorators';
import {ThreadGenerator} from '../threading';
import {LoopCallback} from './loop';

decorate(loopUntil, threadable());
/**
 * Run the given generator until an event occurs.
 *
 * @example
 * ```ts
 * yield* loopUntil(
 *   'Stop Looping',
 *   () => circle().position.x(-10, 0.1).to(10, 0.1)
 * );
 * ```
 *
 * @param event - The event.
 * @param factory - A function creating the generator to run. Because generators
 *                can't be reset, a new generator is created on each iteration.
 */
export function* loopUntil(
  event: string,
  factory: LoopCallback,
): ThreadGenerator {
  const thread = useThread();
  const step = usePlayback().framesToSeconds(1);
  const targetTime = thread.time() + useDuration(event);

  let iteration = 0;
  while (targetTime - step > thread.fixed) {
    const generator = factory(iteration);
    if (generator) {
      yield* generator;
    } else {
      yield;
    }
    iteration += 1;
  }
  thread.time(targetTime);
}