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

Code before:
   */
  ignoreHTTPSErrors?: boolean;
  /**
   * Sets the viewport for each page.
   *
   * @defaultValue '\{width: 800, height: 600\}'
   */
  defaultViewport?: Viewport | null;
  /**
   * Slows down Puppeteer operations by the specified amount of milliseconds to
   * aid debugging.
   */
  slowMo?: number;
  /**
   * Callback to decide if Puppeteer should connect to a given target or not.
   */
  targetFilter?: TargetFilterCallback;
  /**
   * @internal
   */
  _isPageTarget?: IsPageTargetCallback;

  /**
   * @defaultValue 'cdp' when launching Chrome and 'webDriverBiDi' when
   * launching Firefox. When connecting to a running browser, defaults
   * to 'cdp'.
   * @public
   */
  protocol?: ProtocolType;
  /**
   * Timeout setting for individual protocol (CDP) calls.
   *
   * @defaultValue `180_000`
   */
  protocolTimeout?: number;
}

/**
 * @public
 */
export interface ConnectOptions extends BrowserConnectOptions {
  browserWSEndpoint?: string;
  browserURL?: string;
  transport?: ConnectionTransport;
  /**
   * Headers to use for the web socket connection.
   * @remarks
   * Only works in the Node.js environment.
   */
  headers?: Record<string, string>;
}

Review comment (refactor):
Let's simplify it something like:
```suggestion
   * @defaultValue Determined at run time:
   * Launching Chrome - 'cdp'.
   * Launching Firefox - 'webDriverBiDi'.
   * Connecting to a browser - 'cdp'.
```

Code after:
   */
  ignoreHTTPSErrors?: boolean;
  /**
   * Sets the viewport for each page.
   *
   * @defaultValue '\{width: 800, height: 600\}'
   */
  defaultViewport?: Viewport | null;
  /**
   * Slows down Puppeteer operations by the specified amount of milliseconds to
   * aid debugging.
   */
  slowMo?: number;
  /**
   * Callback to decide if Puppeteer should connect to a given target or not.
   */
  targetFilter?: TargetFilterCallback;
  /**
   * @internal
   */
  _isPageTarget?: IsPageTargetCallback;

  /**
   * @defaultValue Determined at run time:
   *
   * - Launching Chrome - 'cdp'.
   *
   * - Launching Firefox - 'webDriverBiDi'.
   *
   * - Connecting to a browser - 'cdp'.
   *
   * @public
   */
  protocol?: ProtocolType;
  /**
   * Timeout setting for individual protocol (CDP) calls.
   *
   * @defaultValue `180_000`
   */
  protocolTimeout?: number;
}

/**
 * @public
 */
export interface ConnectOptions extends BrowserConnectOptions {
  browserWSEndpoint?: string;
  browserURL?: string;
  transport?: ConnectionTransport;
  /**
   * Headers to use for the web socket connection.
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/core/Request.ts (TypeScript)

Code before:
    while (redirect) {
      if (redirect && !redirect.#redirect) {
        return redirect;
      }
      redirect = redirect.#redirect;
    }
    return redirect;
  }
  get response(): Bidi.Network.ResponseData | undefined {
    return this.#response;
  }
  get url(): string {
    return this.#event.request.url;
  }
  get isBlocked(): boolean {
    return this.#event.isBlocked;
  }

  async continueRequest({
    url,
    method,
    headers,
    cookies,
    body,
  }: Omit<Bidi.Network.ContinueRequestParameters, 'request'>): Promise<void> {
    await this.#session.send('network.continueRequest', {
      request: this.id,
      url,
      method,
      headers,
      body,
      cookies,
    });
  }

  async failRequest(): Promise<void> {
    await this.#session.send('network.failRequest', {
      request: this.id,
    });
  }

  async provideResponse({
    statusCode,
    reasonPhrase,
    headers,
    body,
  }: Omit<Bidi.Network.ProvideResponseParameters, 'request'>): Promise<void> {
    await this.#session.send('network.provideResponse', {
      request: this.id,
      statusCode,
      reasonPhrase,

Review comment (suggestion):
I think the Puppeteer API expects undefined if not present. 
```suggestion
    return this.#event['goog:postData'] ?? undefined;
```

Code after:
    while (redirect) {
      if (redirect && !redirect.#redirect) {
        return redirect;
      }
      redirect = redirect.#redirect;
    }
    return redirect;
  }
  get response(): Bidi.Network.ResponseData | undefined {
    return this.#response;
  }
  get url(): string {
    return this.#event.request.url;
  }
  get isBlocked(): boolean {
    return this.#event.isBlocked;
  }

  get resourceType(): string | undefined {
    // @ts-expect-error non-standard attribute.
    return this.#event['goog:resourceType'] ?? undefined;
  }

  get postData(): string {
    // @ts-expect-error non-standard attribute.
    return this.#event['goog:postData'] ?? '';
  }

  get hasPostData(): boolean {
    // @ts-expect-error non-standard attribute.
    return this.#event['goog:hasPostData'] ?? false;
  }

  async continueRequest({
    url,
    method,
    headers,
    cookies,
    body,
  }: Omit<Bidi.Network.ContinueRequestParameters, 'request'>): Promise<void> {
    await this.#session.send('network.continueRequest', {
      request: this.id,
      url,
      method,
      headers,
      body,
      cookies,
    });
  }

  async failRequest(): Promise<void> {
Repository: puppeteer/puppeteer
File: test/src/cookies.spec.ts (TypeScript)

Code before:
        value: '123456',
      });
      const cookies = await page.cookies();
      await expectCookieEquals(
        cookies.sort((a, b) => {
          return a.name.localeCompare(b.name);
        }),
        [
          {
            name: 'password',
            value: '123456',
            domain: 'localhost',
            path: '/',
            sameParty: false,
            expires: -1,
            size: 14,
            httpOnly: false,
            secure: false,
            session: true,
            sourceScheme: 'NonSecure',
          },
        ]
      );
    });
    it('should set cookie with all available properties', async () => {
      const {page, server} = await getTestState();
      await page.goto(server.EMPTY_PAGE);
      await page.setCookie({
        name: 'password',
        value: '123456',
        domain: 'localhost',
        path: '/',
        sameParty: false,
        expires: -1,
        httpOnly: false,
        secure: false,
        sourceScheme: 'Unset',
      });
      const cookies = await page.cookies();
      await expectCookieEquals(
        cookies
          .filter(cookie => {
            return cookie.name === 'password';
          })
          .sort((a, b) => {
            return a.name.localeCompare(b.name);
          }),
        [
          {
            name: 'password',
            value: '123456',

Review comment (suggestion):
```suggestion
      const {page, server} = await getTestState();

```

Code after:
        value: '123456',
      });
      const cookies = await page.cookies();
      await expectCookieEquals(
        cookies.sort((a, b) => {
          return a.name.localeCompare(b.name);
        }),
        [
          {
            name: 'password',
            value: '123456',
            domain: 'localhost',
            path: '/',
            sameParty: false,
            expires: -1,
            size: 14,
            httpOnly: false,
            secure: false,
            session: true,
            sourceScheme: 'NonSecure',
          },
        ]
      );
    });
    it('should set cookie with all available properties', async () => {
      const {page, server} = await getTestState();

      await page.goto(server.EMPTY_PAGE);
      await page.setCookie({
        name: 'password',
        value: '123456',
        domain: 'localhost',
        path: '/',
        sameParty: false,
        expires: -1,
        httpOnly: false,
        secure: false,
        sourceScheme: 'Unset',
      });
      const cookies = await page.cookies();
      await expectCookieEquals(
        cookies.sort((a, b) => {
          return a.name.localeCompare(b.name);
        }),
        [
          {
            name: 'password',
            value: '123456',
            domain: 'localhost',
            path: '/',
            sameParty: false,
Repository: puppeteer/puppeteer
File: tools/clean.mjs (JavaScript)

Code before:
#!/usr/bin/env node

/**
 * @license
 * Copyright 2022 Google Inc.
 * SPDX-License-Identifier: Apache-2.0
 */

import {exec} from 'child_process';
import {readdirSync} from 'fs';
import readline from 'node:readline';

let resolver;
let rejector;
const confirmation = new Promise((res, rej) => {
  resolver = res;
  rejector = rej;
});

if (process.env.npm_command !== 'run-script') {
  console.log('Running command directly may result in data loss.');
  console.log('Ref: https://github.com/puppeteer/puppeteer/issues/12917');
  console.log('Please use the provided npm scripts!');
  const rl = readline.createInterface({
    input: process.stdin,
    output: process.stdout,
  });

  rl.question('Do you want to proceed? (y/N) ', answer => {
    rl.close();
    if (
      answer.localeCompare('y', undefined, {
        sensitivity: 'base',
      }) === 'y' ||
      answer === 'yes'
    ) {
      resolver();
    } else {
      rejector('Canceled');
    }
  });
} else {
  resolver();
}


Review comment (bug):
why is it important if it is run via npm or not? let's just check if `git rev-parse --show-toplevel` is the process.cwd() and abort cleaning if git root is wrong

Code after:
#!/usr/bin/env node

/**
 * @license
 * Copyright 2022 Google Inc.
 * SPDX-License-Identifier: Apache-2.0
 */

import {exec} from 'child_process';
import {readdirSync} from 'fs';
import path from 'path';
import url from 'url';
import {promisify} from 'util';

const execAsync = promisify(exec);

const __dirname = url.fileURLToPath(new URL('.', import.meta.url));
const puppeteerRoot = path.join(__dirname, '../');

const {stdout} = await execAsync('git rev-parse --show-toplevel');
const gitTopLevel = stdout.replaceAll('\n', '').trim();

// https://github.com/puppeteer/puppeteer/issues/12917
if (path.relative(gitTopLevel, puppeteerRoot) !== '') {
  console.log('Top level .git is not Puppeteer');
  process.exit(1);
}

// Prevents cleaning config files such as in `.vscode`
if (path.relative(puppeteerRoot, process.cwd()) === '') {
  console.log('Trying to execute from Puppeteer root');
  process.exit(1);
}

await execAsync(
  `git clean -Xf ${readdirSync(process.cwd())
    .filter(file => {
      return file !== 'node_modules';
    })
    .join(' ')}`
);
Repository: puppeteer/puppeteer
File: packages/browsers/src/browser-data/firefox.ts (TypeScript)

Code before:
      break;
  }
  switch (channel) {
    case FirefoxChannel.NIGHTLY:
      return `${baseUrl}/${resolveDownloadPath(platform, resolvedBuildId).join('/')}`;
    case FirefoxChannel.DEVEDITION:
    case FirefoxChannel.BETA:
    case FirefoxChannel.STABLE:
    case FirefoxChannel.ESR:
      return `${baseUrl}/${resolvedBuildId}/${platformName(platform)}/en-US/${archive(platform, resolvedBuildId)}`;
  }
}

export function resolveDownloadPath(
  platform: BrowserPlatform,
  buildId: string
): string[] {
  const [channel] = parseBuildId(buildId);
  switch (channel) {
    case FirefoxChannel.NIGHTLY:
      return [archiveNightly(platform, buildId)];
    case FirefoxChannel.DEVEDITION:
    case FirefoxChannel.BETA:
    case FirefoxChannel.STABLE:
    case FirefoxChannel.ESR:
      return [archive(platform, buildId)];
  }
}

export function relativeExecutablePath(
  platform: BrowserPlatform,
  buildId: string
): string {
  const [channel] = parseBuildId(buildId);
  switch (channel) {
    case FirefoxChannel.NIGHTLY:
      switch (platform) {
        case BrowserPlatform.MAC_ARM:
        case BrowserPlatform.MAC:
          return path.join(
            'Firefox Nightly.app',
            'Contents',
            'MacOS',
            'firefox'
          );
        case BrowserPlatform.LINUX:
          return path.join('firefox', 'firefox');
        case BrowserPlatform.WIN32:
        case BrowserPlatform.WIN64:
          return path.join('firefox', 'firefox.exe');
      }

Review comment (suggestion):
```suggestion
      return ['en-US', archive(platform, buildId)];
```

Code after:
      break;
  }
  return `${baseUrl}/${resolveDownloadPath(platform, buildId).join('/')}`;
}

export function resolveDownloadPath(
  platform: BrowserPlatform,
  buildId: string
): string[] {
  const [channel, resolvedBuildId] = parseBuildId(buildId);
  switch (channel) {
    case FirefoxChannel.NIGHTLY:
      return [archiveNightly(platform, resolvedBuildId)];
    case FirefoxChannel.DEVEDITION:
    case FirefoxChannel.BETA:
    case FirefoxChannel.STABLE:
    case FirefoxChannel.ESR:
      return [
        resolvedBuildId,
        platformName(platform),
        'en-US',
        archive(platform, resolvedBuildId),
      ];
  }
}

export function relativeExecutablePath(
  platform: BrowserPlatform,
  buildId: string
): string {
  const [channel] = parseBuildId(buildId);
  switch (channel) {
    case FirefoxChannel.NIGHTLY:
      switch (platform) {
        case BrowserPlatform.MAC_ARM:
        case BrowserPlatform.MAC:
          return path.join(
            'Firefox Nightly.app',
            'Contents',
            'MacOS',
            'firefox'
          );
        case BrowserPlatform.LINUX:
          return path.join('firefox', 'firefox');
        case BrowserPlatform.WIN32:
        case BrowserPlatform.WIN64:
          return path.join('firefox', 'firefox.exe');
      }
    case FirefoxChannel.BETA:
    case FirefoxChannel.DEVEDITION:
    case FirefoxChannel.ESR:
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/DownloadBehavior.ts (TypeScript)

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

export type DownloadPolicy = 'deny' | 'allow' | 'allowAndName' | 'default';

/**
 * @public
 */
export interface DownloadBehavior {
  /**
   * Whether to allow all or deny all download requests, or use default behavior if
   * available.
   *
   * @remarks
   * Setting this to `allowAndName` will name all files according to their download guids.
   */
  policy: DownloadPolicy;
  /**
   * The default path to save downloaded files to.
   *
   * @remarks
   * Setting this is required if behavior is set to `allow` or `allowAndName`.
   */
  downloadPath?: string;
}


Review comment (suggestion):
I think this one also needs a `@public` annotation and a re-export in packages/puppeteer-core/src/common/common.ts so that docs and types are fully generated.

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

/**
 * @public
 */
export type DownloadPolicy = 'deny' | 'allow' | 'allowAndName' | 'default';

/**
 * @public
 */
export interface DownloadBehavior {
  /**
   * Whether to allow all or deny all download requests, or use default behavior if
   * available.
   *
   * @remarks
   * Setting this to `allowAndName` will name all files according to their download guids.
   */
  policy: DownloadPolicy;
  /**
   * The default path to save downloaded files to.
   *
   * @remarks
   * Setting this is required if behavior is set to `allow` or `allowAndName`.
   */
  downloadPath?: string;
}
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/node/PuppeteerNode.ts (TypeScript)

Code before:
      default:
        throw new Error(`Unknown product: ${this.#lastLaunchedBrowser}`);
    }
    return this.#_launcher;
  }

  /**
   * The default executable path.
   */
  executablePath(channel?: ChromeReleaseChannel): string {
    return this.#launcher.executablePath(channel);
  }

  /**
   * @internal
   */
  get browserVersion(): string {
    return (
      this.configuration?.[this.lastLaunchedBrowser]?.version ??
      this.defaultBrowserRevision!
    );
  }

  /**
   * The default download path for puppeteer. For puppeteer-core, this
   * code should never be called as it is never defined.
   *
   * @internal
   */
  get defaultDownloadPath(): string | undefined {
    return this.configuration.cacheDirectory;
  }

  /**
   * The name of the browser that was last launched.
   */
  get lastLaunchedBrowser(): SupportedBrowser {
    return this.#lastLaunchedBrowser ?? this.defaultBrowser;
  }

  /**
   * The name of the browser that will be launched by default. For
   * `puppeteer`, this is influenced by your configuration. Otherwise, it's
   * `chrome`.
   */
  get defaultBrowser(): SupportedBrowser {
    return this.configuration.defaultBrowser ?? 'chrome';
  }

  /**
   * @deprecated Do not use as this field as it does not take into account

Review comment (suggestion):
am I right to say that the previous behaviour was to always validate the path in `executablePath`, so this could be a breaking change? since it no longer checks if the path exists from what I can see.

is there any way to still make it validate the path or would that have to be manually done now

Code after:
      case 'chrome':
        return new ChromeLauncher(this);
      case 'firefox':
        return new FirefoxLauncher(this);
      default:
        throw new Error(`Unknown product: ${browser}`);
    }
  }

  /**
   * The default executable path for a given ChromeReleaseChannel.
   */
  executablePath(channel: ChromeReleaseChannel): string;
  /**
   * The default executable path given LaunchOptions.
   */
  executablePath(options: LaunchOptions): string;
  /**
   * The default executable path.
   */
  executablePath(): string;
  executablePath(optsOrChannel?: ChromeReleaseChannel | LaunchOptions): string {
    if (optsOrChannel === undefined) {
      return this.#getLauncher(this.lastLaunchedBrowser).executablePath(
        undefined,
        /* validatePath= */ false,
      );
    }
    if (typeof optsOrChannel === 'string') {
      return this.#getLauncher('chrome').executablePath(
        optsOrChannel,
        /* validatePath= */ false,
      );
    }
    return this.#getLauncher(
      optsOrChannel.browser ?? this.lastLaunchedBrowser,
    ).resolveExecutablePath(optsOrChannel.headless, /* validatePath= */ false);
  }

  /**
   * @internal
   */
  get browserVersion(): string {
    return (
      this.configuration?.[this.lastLaunchedBrowser]?.version ??
      this.defaultBrowserRevision!
    );
  }

  /**
   * The default download path for puppeteer. For puppeteer-core, this
Repository: puppeteer/puppeteer
File: test/src/mocha-utils.ts (TypeScript)

Code before:
}

const defaultBrowserOptions = Object.assign(
  {
    handleSIGINT: true,
    executablePath: process.env['BINARY'],
    headless: headless === 'shell' ? ('shell' as const) : isHeadless,
    dumpio: !!process.env['DUMPIO'],
    protocol,
    args: [
      `--host-resolver-rules=MAP domain1.test 127.0.0.1,MAP domain2.test 127.0.0.1,MAP domain3.test 127.0.0.1`,
    ],
    extraPrefsFirefox: {
      'network.dns.localDomains': `domain1.test,domain2.test,domain3.test`,
    },
  } satisfies LaunchOptions,
  extraLaunchOptions,
);

if (defaultBrowserOptions.executablePath) {
  console.warn(
    `WARN: running ${product} tests with ${defaultBrowserOptions.executablePath}`,
  );
  if (!fs.existsSync(defaultBrowserOptions.executablePath)) {
    throw new Error(
      `Browser not found at ${defaultBrowserOptions.executablePath}`,
    );
  }
} else {
  const executablePath = puppeteer.executablePath();
  if (!fs.existsSync(executablePath)) {
    throw new Error(
      `Browser is not downloaded at ${executablePath}. Run 'npm install' and try to re-run tests`,
    );
  }
}

const processVariables: {
  product: string;
  headless: 'true' | 'false' | 'shell';
  isHeadless: boolean;
  isFirefox: boolean;
  isChrome: boolean;
  protocol: 'cdp' | 'webDriverBiDi';
  defaultBrowserOptions: LaunchOptions;
} = {
  product,
  headless,
  isHeadless,
  isFirefox,
  isChrome,

Review comment (suggestion):
```suggestion
      `Browser executable not found at ${defaultBrowserOptions.executablePath}`,
```

Code after:
}

const defaultBrowserOptions = Object.assign(
  {
    handleSIGINT: true,
    executablePath: process.env['BINARY'],
    headless: headless === 'shell' ? ('shell' as const) : isHeadless,
    dumpio: !!process.env['DUMPIO'],
    protocol,
    args: [
      `--host-resolver-rules=MAP domain1.test 127.0.0.1,MAP domain2.test 127.0.0.1,MAP domain3.test 127.0.0.1`,
    ],
    extraPrefsFirefox: {
      'network.dns.localDomains': `domain1.test,domain2.test,domain3.test`,
    },
  } satisfies LaunchOptions,
  extraLaunchOptions,
);

if (defaultBrowserOptions.executablePath) {
  console.warn(
    `WARN: running ${product} tests with ${defaultBrowserOptions.executablePath}`,
  );
  if (!fs.existsSync(defaultBrowserOptions.executablePath)) {
    throw new Error(
      `Browser executable not found at ${defaultBrowserOptions.executablePath}`,
    );
  }
} else {
  const executablePath = puppeteer.executablePath();
  if (!fs.existsSync(executablePath)) {
    throw new Error(
      `Browser is not downloaded at ${executablePath}. Run 'npm install' and try to re-run tests`,
    );
  }
}

const processVariables: {
  product: string;
  headless: 'true' | 'false' | 'shell';
  isHeadless: boolean;
  isFirefox: boolean;
  isChrome: boolean;
  protocol: 'cdp' | 'webDriverBiDi';
  defaultBrowserOptions: LaunchOptions;
} = {
  product,
  headless,
  isHeadless,
  isFirefox,
  isChrome,
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/PDFOptions.ts (TypeScript)

Code before:
    cm: {width: 118.9, height: 84.1},
    in: {width: 46.811, height: 33.1102},
  },
  a1: {
    cm: {width: 84.1, height: 59.4},
    in: {width: 33.1102, height: 23.3858},
  },
  a2: {
    cm: {width: 59.4, height: 42},
    in: {width: 23.3858, height: 16.5354},
  },
  a3: {
    cm: {width: 42, height: 29.7},
    in: {width: 16.5354, height: 11.6929},
  },
  a4: {
    cm: {width: 29.7, height: 21},
    in: {width: 8.27, height: 11.7},
  },
  a5: {
    cm: {width: 21, height: 14.8},
    in: {width: 8.2677, height: 5.8268},
  },
  a6: {
    cm: {width: 14.8, height: 10.5},
    in: {width: 5.8268, height: 4.1339},
  },
} as const;


Review comment (suggestion):
Some of these values seemed flipped, other seem to be changed for the `in`.
Can you please revert all changes to the `in` parts, I believe all of them should be correct.
The width should always (except at `ledger`) be the smaller of the two, and people can use the `landscape` prop if they want another view.

Ref for the `in` values - https://github.com/puppeteer/puppeteer/pull/13087

Code after:
    cm: {width: 84.1, height: 118.9},
    in: {width: 33.1102, height: 46.811},
  },
  a1: {
    cm: {width: 59.4, height: 84.1},
    in: {width: 23.3858, height: 33.1102},
  },
  a2: {
    cm: {width: 42, height: 59.4},
    in: {width: 16.5354, height: 23.3858},
  },
  a3: {
    cm: {width: 29.7, height: 42},
    in: {width: 11.6929, height: 16.5354},
  },
  a4: {
    cm: {width: 21, height: 29.7},
    in: {width: 8.2677, height: 11.6929},
  },
  a5: {
    cm: {width: 14.8, height: 21},
    in: {width: 5.8268, height: 8.2677},
  },
  a6: {
    cm: {width: 10.5, height: 14.8},
    in: {width: 4.1339, height: 5.8268},
  },
} as const;