ronantakizawa/codereview-bench huggingface dataset:
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/cdp/CdpSession.ts (TypeScript)

Code before:
  }

  /**
   * Sets the {@link CdpTarget} associated with the session instance.
   *
   * @internal
   */
  setTarget(target: CdpTarget): void {
    this.#target = target;
  }

  /**
   * Gets the {@link CdpTarget} associated with the session instance.
   *
   * @internal
   */
  target(): CdpTarget {
    assert(this.#target, 'Target must exist');
    return this.#target;
  }

  override connection(): Connection | undefined {
    return this.#connection;
  }

  override get detached(): boolean {
    return this.#closed;
  }

  get #closed(): boolean {
    return this.#connection._closed || this.#detached;
  }

  override parentSession(): CDPSession | undefined {
    if (!this.#parentSessionId) {
      // In some cases, e.g., DevTools pages there is no parent session. In this
      // case, we treat the current session as the parent session.
      return this;
    }
    const parent = this.#connection?.session(this.#parentSessionId);
    return parent ?? undefined;
  }

  override send<T extends keyof ProtocolMapping.Commands>(
    method: T,
    params?: ProtocolMapping.Commands[T]['paramsType'][0],
    options?: CommandOptions,
  ): Promise<ProtocolMapping.Commands[T]['returnType']> {
    if (this.#closed) {
      return Promise.reject(
        new TargetCloseError(

Review comment (suggestion):
Nit: you can remove the private  `#closed` getter as getter value can't be modified, so no need to have two thing that are the same.

Code after:
  }

  /**
   * Sets the {@link CdpTarget} associated with the session instance.
   *
   * @internal
   */
  setTarget(target: CdpTarget): void {
    this.#target = target;
  }

  /**
   * Gets the {@link CdpTarget} associated with the session instance.
   *
   * @internal
   */
  target(): CdpTarget {
    assert(this.#target, 'Target must exist');
    return this.#target;
  }

  override connection(): Connection | undefined {
    return this.#connection;
  }

  override get detached(): boolean {
    return this.#connection._closed || this.#detached;
  }

  override parentSession(): CDPSession | undefined {
    if (!this.#parentSessionId) {
      // In some cases, e.g., DevTools pages there is no parent session. In this
      // case, we treat the current session as the parent session.
      return this;
    }
    const parent = this.#connection?.session(this.#parentSessionId);
    return parent ?? undefined;
  }

  override send<T extends keyof ProtocolMapping.Commands>(
    method: T,
    params?: ProtocolMapping.Commands[T]['paramsType'][0],
    options?: CommandOptions,
  ): Promise<ProtocolMapping.Commands[T]['returnType']> {
    if (this.detached) {
      return Promise.reject(
        new TargetCloseError(
          `Protocol error (${method}): Session closed. Most likely the ${this.#targetType} has been closed.`,
        ),
      );
    }
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/cdp/Target.ts (TypeScript)

Code before:

  _addChildTarget(target: CdpTarget): void {
    this.#childTargets.add(target);
  }

  _removeChildTarget(target: CdpTarget): void {
    this.#childTargets.delete(target);
  }

  _childTargets(): ReadonlySet<CdpTarget> {
    return this.#childTargets;
  }

  protected _sessionFactory(): (
    isAutoAttachEmulated: boolean,
  ) => Promise<CdpCDPSession> {
    if (!this.#sessionFactory) {
      throw new Error('sessionFactory is not initialized');
    }
    return this.#sessionFactory;
  }

  /**
   * @internal
   */
  _createCDPSession(): Promise<CdpCDPSession> {
    if (!this.#sessionFactory) {
      throw new Error('sessionFactory is not initialized');
    }
    return this.#sessionFactory(false).then(session => {
      session.setTarget(this);
      return session;
    });
  }

  override createCDPSession(): Promise<CDPSession> {
    return this._createCDPSession();
  }

  override url(): string {
    return this.#targetInfo.url;
  }

  override type(): TargetType {
    const type = this.#targetInfo.type;
    switch (type) {
      case 'page':
        return TargetType.PAGE;
      case 'background_page':
        return TargetType.BACKGROUND_PAGE;
      case 'service_worker':

Review comment (suggestion):
We don't need to wrap this into a internal function, as CdpCDPSession extends CDPSession, so TypeScript won't complain.

Code after:
  _addChildTarget(target: CdpTarget): void {
    this.#childTargets.add(target);
  }

  _removeChildTarget(target: CdpTarget): void {
    this.#childTargets.delete(target);
  }

  _childTargets(): ReadonlySet<CdpTarget> {
    return this.#childTargets;
  }

  protected _sessionFactory(): (
    isAutoAttachEmulated: boolean,
  ) => Promise<CdpCDPSession> {
    if (!this.#sessionFactory) {
      throw new Error('sessionFactory is not initialized');
    }
    return this.#sessionFactory;
  }

  override createCDPSession(): Promise<CdpCDPSession> {
    if (!this.#sessionFactory) {
      throw new Error('sessionFactory is not initialized');
    }
    return this.#sessionFactory(false).then(session => {
      session.setTarget(this);
      return session;
    });
  }

  override url(): string {
    return this.#targetInfo.url;
  }

  override type(): TargetType {
    const type = this.#targetInfo.type;
    switch (type) {
      case 'page':
        return TargetType.PAGE;
      case 'background_page':
        return TargetType.BACKGROUND_PAGE;
      case 'service_worker':
        return TargetType.SERVICE_WORKER;
      case 'shared_worker':
        return TargetType.SHARED_WORKER;
      case 'browser':
        return TargetType.BROWSER;
      case 'webview':
        return TargetType.WEBVIEW;
      case 'tab':
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/cdp/Target.ts (TypeScript)

Code before:
   */
  constructor(
    targetInfo: Protocol.Target.TargetInfo,
    session: CdpCDPSession | undefined,
    browserContext: BrowserContext | undefined,
    targetManager: TargetManager | undefined,
    sessionFactory:
      | ((isAutoAttachEmulated: boolean) => Promise<CdpCDPSession>)
      | undefined,
  ) {
    super();
    this.#session = session;
    this.#targetManager = targetManager;
    this.#targetInfo = targetInfo;
    this.#browserContext = browserContext;
    this._targetId = targetInfo.targetId;
    this.#sessionFactory = sessionFactory;
    if (this.#session) {
      this.#session.setTarget(this);
    }
  }

  override async asPage(): Promise<Page> {
    const session = this._session();
    if (!session) {
      return await this._createCDPSession().then(session => {
        return CdpPage._create(session, this, null);
      });
    }
    return await CdpPage._create(session, this, null);
  }

  _subtype(): string | undefined {
    return this.#targetInfo.subtype;
  }

  _session(): CdpCDPSession | undefined {
    return this.#session;
  }

  _addChildTarget(target: CdpTarget): void {
    this.#childTargets.add(target);
  }

  _removeChildTarget(target: CdpTarget): void {
    this.#childTargets.delete(target);
  }

  _childTargets(): ReadonlySet<CdpTarget> {
    return this.#childTargets;
  }

Review comment (suggestion):
Don't rename the handle in the promise it's consing to have to variable in the same scope with the same name, But its a pattern we call Session clients as well. 

Code after:
  constructor(
    targetInfo: Protocol.Target.TargetInfo,
    session: CdpCDPSession | undefined,
    browserContext: BrowserContext | undefined,
    targetManager: TargetManager | undefined,
    sessionFactory:
      | ((isAutoAttachEmulated: boolean) => Promise<CdpCDPSession>)
      | undefined,
  ) {
    super();
    this.#session = session;
    this.#targetManager = targetManager;
    this.#targetInfo = targetInfo;
    this.#browserContext = browserContext;
    this._targetId = targetInfo.targetId;
    this.#sessionFactory = sessionFactory;
    if (this.#session) {
      this.#session.setTarget(this);
    }
  }

  override async asPage(): Promise<Page> {
    const session = this._session();
    if (!session) {
      return await this.createCDPSession().then(client => {
        return CdpPage._create(client, this, null);
      });
    }
    return await CdpPage._create(session, this, null);
  }

  _subtype(): string | undefined {
    return this.#targetInfo.subtype;
  }

  _session(): CdpCDPSession | undefined {
    return this.#session;
  }

  _addChildTarget(target: CdpTarget): void {
    this.#childTargets.add(target);
  }

  _removeChildTarget(target: CdpTarget): void {
    this.#childTargets.delete(target);
  }

  _childTargets(): ReadonlySet<CdpTarget> {
    return this.#childTargets;
  }
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/api/Page.ts (TypeScript)

Code before:
   *
   * For example, `0.5` will shrink the width and height of the output video by
   * half. `2` will double the width and height of the output video.
   *
   * @defaultValue `1`
   */
  scale?: number;
  /**
   * Specifies the speed to record at.
   *
   * For example, `0.5` will slowdown the output video by 50%. `2` will double the
   * speed of the output video.
   *
   * @defaultValue `1`
   */
  speed?: number;
  /**
   * Specifies the frame rate in frames per second.
   *
   * @defaultValue `30` (`20` for GIF)
   */
  fps?: number;
  /**
   * Specifies the number of times to loop playback, from `0` to `Infinity`.
   *
   * @defaultValue `0`
   */
  loop?: number;
  /**
   * Specifies the delay between iterations of a loop, in ms.
   * `-1` is a special value to re-use the previous delay.
   *
   * @defaultValue `-1`
   */
  delay?: number;
  /**
   * Specifies the recording
   * {@link https://trac.ffmpeg.org/wiki/Encode/VP9#constantq | quality}
   * Constant Rate Factor between `0`–`63`. Lower values mean better quality.
   *
   * @defaultValue `30`
   */
  quality?: number;
  /**
   * Specifies the maximum number of
   * {@link https://ffmpeg.org/ffmpeg-filters.html#palettegen | palette}
   * colors to quantize, with GIF limited to `256`.
   * Restrict the palette to only necessary colors to reduce output file size.
   *
   * @defaultValue `256`
   */

Review comment (suggestion):
I think we want a little better explanation here, 
As if undefined is provided no loop;
If 0 or Infinity is provided - loop Infinity
And default is undefined.

Code after:
  crop?: BoundingBox;
  /**
   * Scales the output video.
   *
   * For example, `0.5` will shrink the width and height of the output video by
   * half. `2` will double the width and height of the output video.
   *
   * @defaultValue `1`
   */
  scale?: number;
  /**
   * Specifies the speed to record at.
   *
   * For example, `0.5` will slowdown the output video by 50%. `2` will double the
   * speed of the output video.
   *
   * @defaultValue `1`
   */
  speed?: number;
  /**
   * Specifies the frame rate in frames per second.
   *
   * @defaultValue `30` (`20` for GIF)
   */
  fps?: number;
  /**
   * Specifies the number of times to loop playback, from `0` to `Infinity`.
   * A value of `0` or `undefined` will disable looping.
   *
   * @defaultValue `undefined`
   */
  loop?: number;
  /**
   * Specifies the delay between iterations of a loop, in ms.
   * `-1` is a special value to re-use the previous delay.
   *
   * @defaultValue `-1`
   */
  delay?: number;
  /**
   * Specifies the recording
   * {@link https://trac.ffmpeg.org/wiki/Encode/VP9#constantq | quality}
   * Constant Rate Factor between `0`–`63`. Lower values mean better quality.
   *
   * @defaultValue `30`
   */
  quality?: number;
  /**
   * Specifies the maximum number of
   * {@link https://ffmpeg.org/ffmpeg-filters.html#palettegen | palette}
   * colors to quantize, with GIF limited to `256`.
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/node/ScreenRecorder.ts (TypeScript)

Code before:
        }),
        map(buffer => {
          void this.#writeFrame(buffer);
          return [buffer, performance.now()] as const;
        }),
        takeUntil(fromEvent(this.#controller.signal, 'abort')),
      ),
      {defaultValue: [Buffer.from([]), performance.now()] as const},
    );
  }

  #getFormatArgs(
    format: FileFormat,
    fps: number | 'source_fps',
    loop: number,
    delay: number,
    quality: number,
    colors: number,
  ) {
    const libvpx = [
      // Sets the codec to use.
      ['-c:v', 'vp9'],
      // Sets the quality. Lower the better.
      ['-crf', `${quality}`],
      // Sets the quality and how efficient the compression will be.
      ['-deadline', 'realtime', '-cpu-used', `${os.cpus().length / 2}`],
    ];
    switch (format) {
      case 'webm':
        return [
          ...libvpx,
          // Sets the format
          ['-f', 'webm'],
        ].flat();
      case 'gif':
        fps = DEFAULT_FPS === fps ? 20 : 'source_fps';
        if (loop === Infinity) {
          loop = 0;
        }
        if (delay !== -1) {
          // ms to cs
          delay /= 10;
        }
        return [
          // Sets the frame rate and uses a custom palette generated from the
          // input.
          [
            '-vf',
            `fps=${fps},split[s0][s1];[s0]palettegen=stats_mode=diff:max_colors=${colors}[p];[s1][p]paletteuse=dither=bayer`,
          ],
          // Sets the number of times to loop playback.

Review comment (bug):
Funny bug its max value here is 8 - https://trac.ffmpeg.org/wiki/Encode/VP9#CPUUtilizationSpeed
```suggestion
      ['-deadline', 'realtime', '-cpu-used', `${Math.min(os.cpus().length / 2, 8)}`],
```

Code after:
        }),
        map(buffer => {
          void this.#writeFrame(buffer);
          return [buffer, performance.now()] as const;
        }),
        takeUntil(fromEvent(this.#controller.signal, 'abort')),
      ),
      {defaultValue: [Buffer.from([]), performance.now()] as const},
    );
  }

  #getFormatArgs(
    format: FileFormat,
    fps: number | 'source_fps',
    loop: number,
    delay: number,
    quality: number,
    colors: number,
  ) {
    const libvpx = [
      // Sets the codec to use.
      ['-c:v', 'vp9'],
      // Sets the quality. Lower the better.
      ['-crf', `${quality}`],
      // Sets the quality and how efficient the compression will be.
      [
        '-deadline',
        'realtime',
        '-cpu-used',
        `${Math.min(os.cpus().length / 2, 8)}`,
      ],
    ];
    switch (format) {
      case 'webm':
        return [
          ...libvpx,
          // Sets the format
          ['-f', 'webm'],
        ].flat();
      case 'gif':
        fps = DEFAULT_FPS === fps ? 20 : 'source_fps';
        if (loop === Infinity) {
          loop = 0;
        }
        if (delay !== -1) {
          // ms to cs
          delay /= 10;
        }
        return [
          // Sets the frame rate and uses a custom palette generated from the
          // input.
Repository: puppeteer/puppeteer
File: packages/browsers/src/detectPlatform.ts (TypeScript)

Code before:
/**
 * @license
 * Copyright 2023 Google Inc.
 * SPDX-License-Identifier: Apache-2.0
 */

import os from 'node:os';

import {BrowserPlatform} from './browser-data/browser-data.js';

/**
 * @public
 */
export function detectBrowserPlatform(): BrowserPlatform | undefined {
  const platform = os.platform();
  switch (platform) {
    case 'darwin':
      return os.arch() === 'arm64'
        ? BrowserPlatform.MAC_ARM
        : BrowserPlatform.MAC;
    case 'linux':
      return os.arch() === 'arm64'
        ? BrowserPlatform.LINUX_ARM
        : BrowserPlatform.LINUX;
    case 'win32':
      return os.arch() === 'x64' ||
        // Windows 11 for ARM supports x64 emulation
        (os.arch() === 'arm64' && isWindows11(os.release()))
        ? BrowserPlatform.WIN64
        : BrowserPlatform.WIN32;
    default:
      return undefined;
  }
}

/**
 * Windows 11 is identified by the version 10.0.22000 or greater
 * @internal
 */
function isWindows11(version: string): boolean {
  const parts = version.split('.');
  if (parts.length > 2) {
    const major = parseInt(parts[0] as string, 10);
    const minor = parseInt(parts[1] as string, 10);
    const patch = parseInt(parts[2] as string, 10);
    return (
      major > 10 ||

Review comment (suggestion):
Nit: It would be better to have this in a variable like the platform as we reference it 4 times `os.arch()`

Code after:
/**
 * @license
 * Copyright 2023 Google Inc.
 * SPDX-License-Identifier: Apache-2.0
 */

import os from 'node:os';

import {BrowserPlatform} from './browser-data/browser-data.js';

/**
 * @public
 */
export function detectBrowserPlatform(): BrowserPlatform | undefined {
  const platform = os.platform();
  const arch = os.arch();
  switch (platform) {
    case 'darwin':
      return arch === 'arm64' ? BrowserPlatform.MAC_ARM : BrowserPlatform.MAC;
    case 'linux':
      return arch === 'arm64'
        ? BrowserPlatform.LINUX_ARM
        : BrowserPlatform.LINUX;
    case 'win32':
      return arch === 'x64' ||
        // Windows 11 for ARM supports x64 emulation
        (arch === 'arm64' && isWindows11(os.release()))
        ? BrowserPlatform.WIN64
        : BrowserPlatform.WIN32;
    default:
      return undefined;
  }
}

/**
 * Windows 11 is identified by the version 10.0.22000 or greater
 * @internal
 */
function isWindows11(version: string): boolean {
  const parts = version.split('.');
  if (parts.length > 2) {
    const major = parseInt(parts[0] as string, 10);
    const minor = parseInt(parts[1] as string, 10);
    const patch = parseInt(parts[2] as string, 10);
    return (
      major > 10 ||
      (major === 10 && minor > 0) ||
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/util/disposable.ts (TypeScript)

Code before:
   *   constructor() {
   *     // stack will be disposed when exiting constructor for any reason
   *     using stack = new DisposableStack();
   *
   *     // get first resource
   *     this.#res1 = stack.use(getResource1());
   *
   *     // get second resource. If this fails, both `stack` and `#res1` will be disposed.
   *     this.#res2 = stack.use(getResource2());
   *
   *     // all operations succeeded, move resources out of `stack` so that
   *     // they aren't disposed when constructor exits
   *     this.#disposables = stack.move();
   *   }
   *
   *   [disposeSymbol]() {
   *     this.#disposables.dispose();
   *   }
   * }
   * ```
   */
  move(): DisposableStack {
    if (this.#disposed) {
      throw new ReferenceError('A disposed stack can not use anything new');
    }
    const stack = new DisposableStack();
    stack.#stack = this.#stack;
    this.#disposed = true;
    return stack;
  }

  /**
   * Disposes each resource in the stack in last-in-first-out (LIFO) manner
   * @returns {void}
   */
  [disposeSymbol](): void {
    if (this.#disposed) {
      return;
    }
    this.#disposed = true;
    const errors: unknown[] = [];
    for (const resource of this.#stack) {
      try {
        resource[disposeSymbol]();
      } catch (e) {
        errors.push(e);
      }
    }
    if (errors.length === 1) {
      throw errors[0];
    } else if (errors.length > 1) {

Review comment (suggestion):
We need to nullify the stack here after copying it.
```suggestion
    stack.#stack = this.#stack;
    this.#stack = [];
```

Code after:
   *     // stack will be disposed when exiting constructor for any reason
   *     using stack = new DisposableStack();
   *
   *     // get first resource
   *     this.#res1 = stack.use(getResource1());
   *
   *     // get second resource. If this fails, both `stack` and `#res1` will be disposed.
   *     this.#res2 = stack.use(getResource2());
   *
   *     // all operations succeeded, move resources out of `stack` so that
   *     // they aren't disposed when constructor exits
   *     this.#disposables = stack.move();
   *   }
   *
   *   [disposeSymbol]() {
   *     this.#disposables.dispose();
   *   }
   * }
   * ```
   */
  move(): DisposableStack {
    if (this.#disposed) {
      throw new ReferenceError('A disposed stack can not use anything new');
    }
    const stack = new DisposableStack();
    stack.#stack = this.#stack;
    this.#stack = [];
    this.#disposed = true;
    return stack;
  }

  /**
   * Disposes each resource in the stack in last-in-first-out (LIFO) manner.
   */
  [disposeSymbol](): void {
    if (this.#disposed) {
      return;
    }
    this.#disposed = true;
    const errors: unknown[] = [];
    for (const resource of this.#stack.reverse()) {
      try {
        resource[disposeSymbol]();
      } catch (e) {
        errors.push(e);
      }
    }
    if (errors.length === 1) {
      throw errors[0];
    } else if (errors.length > 1) {
      let suppressed = null;
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/util/disposable.ts (TypeScript)

Code before:
    if (this.#disposed) {
      throw new ReferenceError('A disposed stack can not use anything new');
    }
    const stack = new DisposableStack();
    stack.#stack = this.#stack;
    this.#disposed = true;
    return stack;
  }

  /**
   * Disposes each resource in the stack in last-in-first-out (LIFO) manner
   * @returns {void}
   */
  [disposeSymbol](): void {
    if (this.#disposed) {
      return;
    }
    this.#disposed = true;
    const errors: unknown[] = [];
    for (const resource of this.#stack) {
      try {
        resource[disposeSymbol]();
      } catch (e) {
        errors.push(e);
      }
    }
    if (errors.length === 1) {
      throw errors[0];
    } else if (errors.length > 1) {
      throw new AggregateError(
        errors,
        'Multiple errors occurred during disposal',
      );
    }
  }

  readonly [Symbol.toStringTag] = 'DisposableStack';
}

/**
 * @internal
 */
export class AsyncDisposableStack {
  #disposed = false;
  #stack: AsyncDisposable[] = [];

  /**
   * Gets a value indicating whether the stack has been disposed.
   * @returns {boolean}
   */
  get disposed(): boolean {

Review comment (suggestion):
From my understanding this is not suppose to throw an error, it should suppress the errors that are produced by resouce[disposeSymbol].

Code after:
      throw new ReferenceError('A disposed stack can not use anything new');
    }
    const stack = new DisposableStack();
    stack.#stack = this.#stack;
    this.#stack = [];
    this.#disposed = true;
    return stack;
  }

  /**
   * Disposes each resource in the stack in last-in-first-out (LIFO) manner.
   */
  [disposeSymbol](): void {
    if (this.#disposed) {
      return;
    }
    this.#disposed = true;
    const errors: unknown[] = [];
    for (const resource of this.#stack.reverse()) {
      try {
        resource[disposeSymbol]();
      } catch (e) {
        errors.push(e);
      }
    }
    if (errors.length === 1) {
      throw errors[0];
    } else if (errors.length > 1) {
      let suppressed = null;
      for (const error of errors.reverse()) {
        if (suppressed === null) {
          suppressed = error;
        } else {
          suppressed = new SuppressedError(error, suppressed);
        }
      }
      throw suppressed;
    }
  }

  readonly [Symbol.toStringTag] = 'DisposableStack';
}

/**
 * @internal
 */
export class AsyncDisposableStack {
  #disposed = false;
  #stack: AsyncDisposable[] = [];

  /**