ronantakizawa/codereview-bench huggingface dataset:
Repository: puppeteer/puppeteer
File: packages/browsers/src/install.ts (TypeScript)

Code before:
    }
    debugInstall(`Downloading binary from ${url}`);
    try {
      debugTime('download');
      await downloadFile(url, archivePath, options.downloadProgressCallback);
    } finally {
      debugTimeEnd('download');
    }

    debugInstall(`Installing ${archivePath} to ${outputPath}`);
    try {
      debugTime('extract');
      await unpackArchive(archivePath, outputPath);
    } finally {
      debugTimeEnd('extract');
    }
    const installedBrowser = new InstalledBrowser(
      cache,
      options.browser,
      options.buildId,
      options.platform
    );
    if (options.buildIdAlias) {
      const metadata = installedBrowser.readMetadata();
      metadata.aliases[options.buildIdAlias] = options.buildId;
      metadata.aliases['latest'] = options.buildId;
      installedBrowser.writeMetadata(metadata);
    }
    return installedBrowser;
  } finally {
    if (existsSync(archivePath)) {
      await unlink(archivePath);
    }
  }
}

/**
 * @public
 */
export interface UninstallOptions {
  /**
   * Determines the platform for the browser binary.
   *
   * @defaultValue **Auto-detected.**
   */
  platform?: BrowserPlatform;
  /**
   * The path to the root of the cache directory.
   */
  cacheDir: string;
  /**

Review comment (bug):
Why do we always update the `latest` ? This feels wrong, as if I install `latest` then install `stable`. The last install will be latest will be wrong. 

Code after:
    }
    debugInstall(`Downloading binary from ${url}`);
    try {
      debugTime('download');
      await downloadFile(url, archivePath, options.downloadProgressCallback);
    } finally {
      debugTimeEnd('download');
    }

    debugInstall(`Installing ${archivePath} to ${outputPath}`);
    try {
      debugTime('extract');
      await unpackArchive(archivePath, outputPath);
    } finally {
      debugTimeEnd('extract');
    }
    const installedBrowser = new InstalledBrowser(
      cache,
      options.browser,
      options.buildId,
      options.platform
    );
    if (options.buildIdAlias) {
      const metadata = installedBrowser.readMetadata();
      metadata.aliases[options.buildIdAlias] = options.buildId;
      installedBrowser.writeMetadata(metadata);
    }
    return installedBrowser;
  } finally {
    if (existsSync(archivePath)) {
      await unlink(archivePath);
    }
  }
}

/**
 * @public
 */
export interface UninstallOptions {
  /**
   * Determines the platform for the browser binary.
   *
   * @defaultValue **Auto-detected.**
   */
  platform?: BrowserPlatform;
  /**
   * The path to the root of the cache directory.
   */
  cacheDir: string;
  /**
   * Determines which browser to uninstall.
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/HTTPRequest.ts (TypeScript)

Code before:
    this.interception.enabled = request.isBlocked;

    this.#request = request;
    this.#frame = frame;
    this.#redirectBy = redirectBy;
    this.id = request.id;
  }

  override get client(): CDPSession {
    return this.#frame.client;
  }

  #initialize() {
    this.#request.on('redirect', request => {
      const httpRequest = BidiHTTPRequest.from(request, this.#frame, this);
      void httpRequest.finalizeInterceptions();
    });
    this.#request.once('success', data => {
      this.#response = BidiHTTPResponse.from(data, this);
    });
    this.#request.on('authenticate', this.#handleAuthentication);

    this.#frame.page().trustedEmitter.emit(PageEvent.Request, this);

    if (Object.keys(this.#extraHTTPHeaders).length) {
      console.log('added header', this.#extraHTTPHeaders);
      this.interception.handlers.push(async () => {
        await this.continue(
          {
            headers: this.headers(),
          },
          0
        );
      });
    }
  }

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

  override resourceType(): ResourceType {
    throw new UnsupportedOperation();
  }

  override method(): string {
    return this.#request.method;
  }

  override postData(): string | undefined {
    throw new UnsupportedOperation();

Review comment (suggestion):
```suggestion
      console.log('added header', this.#extraHTTPHeaders);
```

remove

Code after:
    this.interception.enabled = request.isBlocked;

    this.#request = request;
    this.#frame = frame;
    this.#redirectBy = redirectBy;
    this.id = request.id;
  }

  override get client(): CDPSession {
    return this.#frame.client;
  }

  #initialize() {
    this.#request.on('redirect', request => {
      const httpRequest = BidiHTTPRequest.from(request, this.#frame, this);
      void httpRequest.finalizeInterceptions();
    });
    this.#request.once('success', data => {
      this.#response = BidiHTTPResponse.from(data, this);
    });
    this.#request.on('authenticate', this.#handleAuthentication);

    this.#frame.page().trustedEmitter.emit(PageEvent.Request, this);

    if (Object.keys(this.#extraHTTPHeaders).length) {
      this.interception.handlers.push(async () => {
        await this.continue(
          {
            headers: this.headers(),
          },
          0
        );
      });
    }
  }

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

  override resourceType(): ResourceType {
    throw new UnsupportedOperation();
  }

  override method(): string {
    return this.#request.method;
  }

  override postData(): string | undefined {
    throw new UnsupportedOperation();
  }
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/cdp/ElementHandle.ts (TypeScript)

Code before:

  @throwIfDisposed()
  override async autofill(data: AutofillData): Promise<void> {
    const nodeInfo = await this.client.send('DOM.describeNode', {
      objectId: this.handle.id,
    });
    const fieldId = nodeInfo.node.backendNodeId;
    const frameId = this.frame._id;
    await this.client.send('Autofill.trigger', {
      fieldId,
      frameId,
      card: data.creditCard,
    });
  }

  override async *queryAXTree(
    name?: string | undefined,
    role?: string | undefined
  ): AwaitableIterable<ElementHandle<Node>> {
    const {nodes} = await this.client.send('Accessibility.queryAXTree', {
      objectId: this.id,
      accessibleName: name,
      role,
    });

    const results = nodes.filter((node: Protocol.Accessibility.AXNode) => {
      if (node.ignored) {
        return false;
      }
      if (!node.role) {
        return false;
      }
      if (NON_ELEMENT_NODE_ROLES.has(node.role.value)) {
        return false;
      }
      return true;
    });

    return yield* AsyncIterableUtil.map(results, node => {
      return this.realm.adoptBackendNode(node.backendDOMNodeId) as Promise<
        ElementHandle<Node>
      >;
    });
  }
}


Review comment (suggestion):
Nit:
```suggestion
    const results = nodes.filter(node => {
```

Code after:

  @throwIfDisposed()
  override async autofill(data: AutofillData): Promise<void> {
    const nodeInfo = await this.client.send('DOM.describeNode', {
      objectId: this.handle.id,
    });
    const fieldId = nodeInfo.node.backendNodeId;
    const frameId = this.frame._id;
    await this.client.send('Autofill.trigger', {
      fieldId,
      frameId,
      card: data.creditCard,
    });
  }

  override async *queryAXTree(
    name?: string | undefined,
    role?: string | undefined
  ): AwaitableIterable<ElementHandle<Node>> {
    const {nodes} = await this.client.send('Accessibility.queryAXTree', {
      objectId: this.id,
      accessibleName: name,
      role,
    });

    const results = nodes.filter(node => {
      if (node.ignored) {
        return false;
      }
      if (!node.role) {
        return false;
      }
      if (NON_ELEMENT_NODE_ROLES.has(node.role.value)) {
        return false;
      }
      return true;
    });

    return yield* AsyncIterableUtil.map(results, node => {
      return this.realm.adoptBackendNode(node.backendDOMNodeId) as Promise<
        ElementHandle<Node>
      >;
    });
  }
}
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/HTTPRequest.ts (TypeScript)

Code before:
import {UnsupportedOperation} from '../common/Errors.js';

import type {Request} from './core/Request.js';
import type {BidiFrame} from './Frame.js';
import {BidiHTTPResponse} from './HTTPResponse.js';

export const requests = new WeakMap<Request, BidiHTTPRequest>();

/**
 * @internal
 */
export class BidiHTTPRequest extends HTTPRequest {
  static from(
    bidiRequest: Request,
    frame: BidiFrame,
    redirectChain: BidiHTTPRequest[] = []
  ): BidiHTTPRequest {
    const request = new BidiHTTPRequest(bidiRequest, frame, redirectChain);
    request.#initialize();
    return request;
  }
  #response: BidiHTTPResponse | null = null;
  override readonly id: string;
  readonly #frame: BidiFrame;
  readonly #request: Request;
  declare _redirectChain: BidiHTTPRequest[];

  private constructor(
    request: Request,
    frame: BidiFrame,
    redirectChain: BidiHTTPRequest[] = []
  ) {
    super();
    requests.set(request, this);

    this.interception.enabled = request.isBlocked;

    this.#request = request;
    this.#frame = frame;
    this._redirectChain = redirectChain;
    this.id = request.id;
  }

  override get client(): CDPSession {
    return this.#frame.client;
  }

  #initialize() {
    this.#request.on('redirect', request => {
      this._redirectChain.push(this);


Review comment (suggestion):
perhaps we could keep it private and instantiate it in the constructor:

1) if !redirectBy, set this.#redirectChain to []
2) if redirectBy, set this.#redirectChain to redirectBy.redirectChain() (without slicing, using the same instance of the chain)

Code after:
import {UnsupportedOperation} from '../common/Errors.js';

import type {Request} from './core/Request.js';
import type {BidiFrame} from './Frame.js';
import {BidiHTTPResponse} from './HTTPResponse.js';

export const requests = new WeakMap<Request, BidiHTTPRequest>();

/**
 * @internal
 */
export class BidiHTTPRequest extends HTTPRequest {
  static from(
    bidiRequest: Request,
    frame: BidiFrame,
    redirect?: BidiHTTPRequest
  ): BidiHTTPRequest {
    const request = new BidiHTTPRequest(bidiRequest, frame, redirect);
    request.#initialize();
    return request;
  }

  #redirectChain: BidiHTTPRequest[];
  #response: BidiHTTPResponse | null = null;
  override readonly id: string;
  readonly #frame: BidiFrame;
  readonly #request: Request;

  private constructor(
    request: Request,
    frame: BidiFrame,
    redirect?: BidiHTTPRequest
  ) {
    super();
    requests.set(request, this);

    this.interception.enabled = request.isBlocked;

    this.#request = request;
    this.#frame = frame;
    this.#redirectChain = redirect ? redirect.#redirectChain : [];
    this.id = request.id;
  }

  override get client(): CDPSession {
    return this.#frame.client;
  }

  #initialize() {
    this.#request.on('redirect', request => {
      const httpRequest = BidiHTTPRequest.from(request, this.#frame, this);
Repository: puppeteer/puppeteer
File: packages/browsers/src/install.ts (TypeScript)

Code before:
   * Determines the host that will be used for downloading.
   *
   * @defaultValue Either
   *
   * - https://storage.googleapis.com/chrome-for-testing-public or
   * - https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central
   *
   */
  baseUrl?: string;
  /**
   * Whether to unpack and install browser archives.
   *
   * @defaultValue `true`
   */
  unpack?: boolean;
  /**
   * @internal
   * @defaultValue `false`
   */
  forceFallbackForTesting?: boolean;

  /**
   * Whether to attempt to install system-level dependencies required
   * for the browser.
   *
   * Currently, only supported for Debian and Ubuntu and only for Chrome.
   * Requires root privileges.
   *
   * @defaultValue `false`
   */
  installDeps?: boolean;
}

/**
 * Downloads and unpacks the browser archive according to the
 * {@link InstallOptions}.
 *
 * @returns a {@link InstalledBrowser} instance.
 *
 * @public
 */
export function install(
  options: InstallOptions & {unpack?: true}
): Promise<InstalledBrowser>;
/**
 * Downloads the browser archive according to the {@link InstallOptions} without
 * unpacking.
 *
 * @returns the absolute path to the archive.
 *
 * @public

Review comment (suggestion):
I think we should try to "say" the Browser first as that is more important than the Platform.
```suggestion
   * Only supported for Chrome on Debian or Ubuntu.
```

Code after:
   * Determines the host that will be used for downloading.
   *
   * @defaultValue Either
   *
   * - https://storage.googleapis.com/chrome-for-testing-public or
   * - https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central
   *
   */
  baseUrl?: string;
  /**
   * Whether to unpack and install browser archives.
   *
   * @defaultValue `true`
   */
  unpack?: boolean;
  /**
   * @internal
   * @defaultValue `false`
   */
  forceFallbackForTesting?: boolean;

  /**
   * Whether to attempt to install system-level dependencies required
   * for the browser.
   *
   * Only supported for Chrome on Debian or Ubuntu.
   * Requires system-level privileges to run `apt-get`.
   *
   * @defaultValue `false`
   */
  installDeps?: boolean;
}

/**
 * Downloads and unpacks the browser archive according to the
 * {@link InstallOptions}.
 *
 * @returns a {@link InstalledBrowser} instance.
 *
 * @public
 */
export function install(
  options: InstallOptions & {unpack?: true}
): Promise<InstalledBrowser>;
/**
 * Downloads the browser archive according to the {@link InstallOptions} without
 * unpacking.
 *
 * @returns the absolute path to the archive.
 *
 * @public
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/util.ts (TypeScript)

Code before:
/**
 * @internal
 */
export function fromEmitterEvent<
  Events extends Record<EventType, unknown>,
  Event extends keyof Events,
>(emitter: EventEmitter<Events>, eventName: Event): Observable<Events[Event]> {
  return new Observable(subscriber => {
    const listener = (event: Events[Event]) => {
      subscriber.next(event);
    };
    emitter.on(eventName, listener);
    return () => {
      emitter.off(eventName, listener);
    };
  });
}

/**
 * @internal
 */
export function fromAbortSignal(signal?: AbortSignal): Observable<never> {
  return signal
    ? fromEvent(signal, 'abort').pipe(
        map(() => {
          throw new Error(`Aborted reason: ${signal.reason}`);
        })
      )
    : NEVER;
}

/**
 * @internal
 */
export function filterAsync<T>(
  predicate: (value: T) => boolean | PromiseLike<boolean>
): OperatorFunction<T, T> {
  return mergeMap<T, Observable<T>>((value): Observable<T> => {
    return from(Promise.resolve(predicate(value))).pipe(
      filter(isMatch => {
        return isMatch;
      }),
      map(() => {
        return value;
      })
    );
  });
}


Review comment (suggestion):
I forgot how we do it in other places but can we use singal.throwIfAborted() here. I think in other places we do not construct custom errors

Code after:
/**
 * @internal
 */
export function fromEmitterEvent<
  Events extends Record<EventType, unknown>,
  Event extends keyof Events,
>(emitter: EventEmitter<Events>, eventName: Event): Observable<Events[Event]> {
  return new Observable(subscriber => {
    const listener = (event: Events[Event]) => {
      subscriber.next(event);
    };
    emitter.on(eventName, listener);
    return () => {
      emitter.off(eventName, listener);
    };
  });
}

/**
 * @internal
 */
export function fromAbortSignal(
  signal?: AbortSignal,
  cause?: Error
): Observable<never> {
  return signal
    ? fromEvent(signal, 'abort').pipe(
        map(() => {
          if (signal.reason instanceof Error) {
            signal.reason.cause = cause;
            throw signal.reason;
          }

          throw new Error(signal.reason, {cause});
        })
      )
    : NEVER;
}

/**
 * @internal
 */
export function filterAsync<T>(
  predicate: (value: T) => boolean | PromiseLike<boolean>
): OperatorFunction<T, T> {
  return mergeMap<T, Observable<T>>((value): Observable<T> => {
    return from(Promise.resolve(predicate(value))).pipe(
      filter(isMatch => {
        return isMatch;
      }),
      map(() => {
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/core/BrowsingContext.ts (TypeScript)

Code before:
  ): Promise<void> {
    await this.#session.send('browsingContext.navigate', {
      context: this.id,
      url,
      wait,
    });
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async reload(options: ReloadOptions = {}): Promise<void> {
    await this.#session.send('browsingContext.reload', {
      context: this.id,
      ...options,
    });
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async setCacheBypass(enabled: boolean): Promise<void> {
    // @ts-expect-error not in BiDi types yet.
    await this.#session.send('browsingContext.setCacheBypass', {
      contexts: [this.id],
      bypass: enabled,
    });
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async print(options: PrintOptions = {}): Promise<string> {
    const {
      result: {data},
    } = await this.#session.send('browsingContext.print', {
      context: this.id,
      ...options,
    });
    return data;
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async handleUserPrompt(options: HandleUserPromptOptions = {}): Promise<void> {
    await this.#session.send('browsingContext.handleUserPrompt', {

Review comment (performance):
Our command is in the network module
```suggestion
    await this.#session.send('network.setCacheBypass', {
```

Code after:
  ): Promise<void> {
    await this.#session.send('browsingContext.navigate', {
      context: this.id,
      url,
      wait,
    });
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async reload(options: ReloadOptions = {}): Promise<void> {
    await this.#session.send('browsingContext.reload', {
      context: this.id,
      ...options,
    });
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async setCacheBehavior(cacheBehavior: 'default' | 'bypass'): Promise<void> {
    // @ts-expect-error not in BiDi types yet.
    await this.#session.send('network.setCacheBehavior', {
      contexts: [this.id],
      cacheBehavior,
    });
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async print(options: PrintOptions = {}): Promise<string> {
    const {
      result: {data},
    } = await this.#session.send('browsingContext.print', {
      context: this.id,
      ...options,
    });
    return data;
  }

  @throwIfDisposed<BrowsingContext>(context => {
    // SAFETY: Disposal implies this exists.
    return context.#reason!;
  })
  async handleUserPrompt(options: HandleUserPromptOptions = {}): Promise<void> {
    await this.#session.send('browsingContext.handleUserPrompt', {
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/Page.ts (TypeScript)

Code before:

  override async removeScriptToEvaluateOnNewDocument(
    id: string
  ): Promise<void> {
    await this.#frame.browsingContext.removePreloadScript(id);
  }

  override async exposeFunction<Args extends unknown[], Ret>(
    name: string,
    pptrFunction:
      | ((...args: Args) => Awaitable<Ret>)
      | {default: (...args: Args) => Awaitable<Ret>}
  ): Promise<void> {
    return await this.mainFrame().exposeFunction(
      name,
      'default' in pptrFunction ? pptrFunction.default : pptrFunction
    );
  }

  override isDragInterceptionEnabled(): boolean {
    return false;
  }

  override async setCacheEnabled(enabled?: boolean): Promise<void> {
    if (!this.#browserContext.browser().cdpSupported) {
      await this.#frame.browsingContext.setCacheBypass(enabled ?? false);
      return;
    }
    // TODO: handle CDP-specific cases such as mprach.
    await this._client().send('Network.setCacheDisabled', {
      cacheDisabled: !enabled,
    });
  }

  override async cookies(...urls: string[]): Promise<Cookie[]> {
    const normalizedUrls = (urls.length ? urls : [this.url()]).map(url => {
      return new URL(url);
    });

    const cookies = await this.#frame.browsingContext.getCookies();
    return cookies
      .map(cookie => {
        return bidiToPuppeteerCookie(cookie);
      })
      .filter(cookie => {
        return normalizedUrls.some(url => {
          return testUrlMatchCookie(cookie, url);
        });
      });
  }


Review comment (performance):
```suggestion
      await this.#frame.browsingContext.setCacheBypass(!enabled ?? true);
```


Code after:

  override async removeScriptToEvaluateOnNewDocument(
    id: string
  ): Promise<void> {
    await this.#frame.browsingContext.removePreloadScript(id);
  }

  override async exposeFunction<Args extends unknown[], Ret>(
    name: string,
    pptrFunction:
      | ((...args: Args) => Awaitable<Ret>)
      | {default: (...args: Args) => Awaitable<Ret>}
  ): Promise<void> {
    return await this.mainFrame().exposeFunction(
      name,
      'default' in pptrFunction ? pptrFunction.default : pptrFunction
    );
  }

  override isDragInterceptionEnabled(): boolean {
    return false;
  }

  override async setCacheEnabled(enabled?: boolean): Promise<void> {
    if (!this.#browserContext.browser().cdpSupported) {
      await this.#frame.browsingContext.setCacheBehavior(
        enabled ? 'default' : 'bypass'
      );
      return;
    }
    // TODO: handle CDP-specific cases such as mprach.
    await this._client().send('Network.setCacheDisabled', {
      cacheDisabled: !enabled,
    });
  }

  override async cookies(...urls: string[]): Promise<Cookie[]> {
    const normalizedUrls = (urls.length ? urls : [this.url()]).map(url => {
      return new URL(url);
    });

    const cookies = await this.#frame.browsingContext.getCookies();
    return cookies
      .map(cookie => {
        return bidiToPuppeteerCookie(cookie);
      })
      .filter(cookie => {
        return normalizedUrls.some(url => {
          return testUrlMatchCookie(cookie, url);
        });
      });