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

Code before:
 *
 * Example output:
 * ["NetworkService", "NetworkServiceInProcess", "Foo"]
 *
 * @internal
 */
export function getFeatures(flag: string, options: string[] = []): string[] {
  return options
    .filter(s => {
      return s.startsWith(flag.endsWith('=') ? flag : `${flag}=`);
    })
    .map(s => {
      return s.split(new RegExp(`${flag}` + '=\\s*'))[1]?.trim();
    })
    .filter(s => {
      return s;
    }) as string[];
}

/**
 * Removes all elements in-place from the given array that match the given
 * regular expression.
 *
 * @internal
 */
export function removeMatching(array: string[], regex: RegExp): string[] {
  let i = 0;
  while (i < array.length) {
    if (regex.test(array[i]!)) {
      array.splice(i, 1);
    } else {
      i++;
    }
  }
  return array;
}


Review comment (refactor):
Can we create the reg-ex in this function it would be nice to have a consistent API for extracting and removing. 
Than we a better name would be `removeMatchingFlags`.

Code after:
 *
 * Example output:
 * ["NetworkService", "NetworkServiceInProcess", "Foo"]
 *
 * @internal
 */
export function getFeatures(flag: string, options: string[] = []): string[] {
  return options
    .filter(s => {
      return s.startsWith(flag.endsWith('=') ? flag : `${flag}=`);
    })
    .map(s => {
      return s.split(new RegExp(`${flag}` + '=\\s*'))[1]?.trim();
    })
    .filter(s => {
      return s;
    }) as string[];
}

/**
 * Removes all elements in-place from the given string array
 * that match the given command-line flag.
 *
 * @internal
 */
export function removeMatchingFlags(array: string[], flag: string): string[] {
  const regex = new RegExp(`^${flag}=.*`);
  let i = 0;
  while (i < array.length) {
    if (regex.test(array[i]!)) {
      array.splice(i, 1);
    } else {
      i++;
    }
  }
  return array;
}
Repository: puppeteer/puppeteer
File: test/installation/src/puppeteer-cli.spec.ts (TypeScript)

Code before:
import {configureSandbox} from './sandbox.js';

describe('Puppeteer CLI', () => {
  configureSandbox({
    dependencies: ['@puppeteer/browsers', 'puppeteer-core', 'puppeteer'],
    env: cwd => {
      return {
        PUPPETEER_CACHE_DIR: join(cwd, '.cache', 'puppeteer'),
        PUPPETEER_SKIP_DOWNLOAD: 'true',
      };
    },
  });

  it('can launch', async function () {
    const result = spawnSync('npx', ['puppeteer', '--help'], {
      // npx is not found without the shell flag on Windows.
      shell: process.platform === 'win32',
      cwd: this.sandbox,
    });
    assert.strictEqual(result.status, 0);
    assert.ok(
      result.stdout.toString('utf-8').startsWith('puppeteer <command>')
    );
  });

  it('can dowload a browser', async function () {
    assert.ok(!existsSync(join(this.sandbox, '.cache', 'puppeteer')));
    const result = spawnSync(
      'npx',
      ['puppeteer', 'browsers', 'install', 'chrome'],
      {
        // npx is not found without the shell flag on Windows.
        shell: process.platform === 'win32',
        cwd: this.sandbox,
        env: {
          ...process.env,
          PUPPETEER_CACHE_DIR: join(this.sandbox, '.cache', 'puppeteer'),
        },
      }
    );
    assert.strictEqual(result.status, 0);
    const files = await readdir(join(this.sandbox, '.cache', 'puppeteer'));
    assert.equal(files.length, 1);
    assert.equal(files[0], 'chrome');
  });
});


Review comment (suggestion):
```suggestion
  it('can download a browser', async function () {
```

Code after:
import {configureSandbox} from './sandbox.js';

describe('Puppeteer CLI', () => {
  configureSandbox({
    dependencies: ['@puppeteer/browsers', 'puppeteer-core', 'puppeteer'],
    env: cwd => {
      return {
        PUPPETEER_CACHE_DIR: join(cwd, '.cache', 'puppeteer'),
        PUPPETEER_SKIP_DOWNLOAD: 'true',
      };
    },
  });

  it('can launch', async function () {
    const result = spawnSync('npx', ['puppeteer', '--help'], {
      // npx is not found without the shell flag on Windows.
      shell: process.platform === 'win32',
      cwd: this.sandbox,
    });
    assert.strictEqual(result.status, 0);
    assert.ok(
      result.stdout.toString('utf-8').startsWith('puppeteer <command>')
    );
  });

  it('can download a browser', async function () {
    assert.ok(!existsSync(join(this.sandbox, '.cache', 'puppeteer')));
    const result = spawnSync(
      'npx',
      ['puppeteer', 'browsers', 'install', 'chrome'],
      {
        // npx is not found without the shell flag on Windows.
        shell: process.platform === 'win32',
        cwd: this.sandbox,
        env: {
          ...process.env,
          PUPPETEER_CACHE_DIR: join(this.sandbox, '.cache', 'puppeteer'),
        },
      }
    );
    assert.strictEqual(result.status, 0);
    const files = await readdir(join(this.sandbox, '.cache', 'puppeteer'));
    assert.equal(files.length, 1);
    assert.equal(files[0], 'chrome');
  });
});
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/Page.ts (TypeScript)

Code before:
            window.visualViewport.pageTop,
          ] as const;
        });
        box = {
          ...clip,
          x: clip.x - pageLeft,
          y: clip.y - pageTop,
        };
      }
    }

    if (clip !== undefined && clip.scale !== undefined && clip.scale !== 1) {
      throw new UnsupportedOperation(
        `BiDi does not support 'scale' in 'clip'.`
      );
    }

    const {
      result: {data},
    } = await this.#connection.send('browsingContext.captureScreenshot', {
      context: this.mainFrame()._id,
      origin: captureBeyondViewport ? 'document' : 'viewport',
      format: {
        type: `image/${type}`,
        ...(quality !== undefined ? {quality: quality / 100} : {}),
      },
      ...(box ? {clip: {type: 'box', ...box}} : {}),
    });
    return data;
  }

  override async waitForRequest(
    urlOrPredicate:
      | string
      | ((req: BidiHTTPRequest) => boolean | Promise<boolean>),
    options: {timeout?: number} = {}
  ): Promise<BidiHTTPRequest> {
    const {timeout = this._timeoutSettings.timeout()} = options;
    return await waitForHTTP(
      this.#networkManager,
      NetworkManagerEvent.Request,
      urlOrPredicate,
      timeout,
      this.#closedDeferred
    );
  }

  override async waitForResponse(
    urlOrPredicate:
      | string
      | ((res: BidiHTTPResponse) => boolean | Promise<boolean>),

Review comment (suggestion):
The `quality` and `box` property are hard to read, please consider either the original or an alternative. 

Code after:
            window.visualViewport.pageLeft,
            window.visualViewport.pageTop,
          ] as const;
        });
        box = {
          ...clip,
          x: clip.x - pageLeft,
          y: clip.y - pageTop,
        };
      }
    }

    if (clip !== undefined && clip.scale !== undefined && clip.scale !== 1) {
      throw new UnsupportedOperation(
        `BiDi does not support 'scale' in 'clip'.`
      );
    }

    const {
      result: {data},
    } = await this.#connection.send('browsingContext.captureScreenshot', {
      context: this.mainFrame()._id,
      origin: captureBeyondViewport ? 'document' : 'viewport',
      format: {
        type: `image/${type}`,
        ...(quality !== undefined ? {quality: quality / 100} : {}),
      },
      ...(box ? {clip: {type: 'box', ...box}} : {}),
    });
    return data;
  }

  override async waitForRequest(
    urlOrPredicate:
      | string
      | ((req: BidiHTTPRequest) => boolean | Promise<boolean>),
    options: {timeout?: number} = {}
  ): Promise<BidiHTTPRequest> {
    const {timeout = this._timeoutSettings.timeout()} = options;
    return await waitForHTTP(
      this.#networkManager,
      NetworkManagerEvent.Request,
      urlOrPredicate,
      timeout,
      this.#closedDeferred
    );
  }

  override async waitForResponse(
    urlOrPredicate:
      | string
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/Browser.ts (TypeScript)

Code before:
  override newPage(): Promise<Page> {
    return this.#defaultContext.newPage();
  }

  override targets(): Target[] {
    return [this.#browserTarget, ...Array.from(this.#targets.values())];
  }

  _getTargetById(id: string): BidiTarget {
    const target = this.#targets.get(id);
    if (!target) {
      throw new Error('Target not found');
    }
    return target;
  }

  override target(): Target {
    return this.#browserTarget;
  }

  override async disconnect(): Promise<void> {
    try {
      await this.#connection.send('session.end', {});
      this.#connection.dispose();
    } catch (e) {
      console.log(e);
    }
  }
}


Review comment (bug):
I assume we don't want to show error here so we should use `debugError` to print only when explicitly enabled.

Code after:
  }

  override newPage(): Promise<Page> {
    return this.#defaultContext.newPage();
  }

  override targets(): Target[] {
    return [this.#browserTarget, ...Array.from(this.#targets.values())];
  }

  _getTargetById(id: string): BidiTarget {
    const target = this.#targets.get(id);
    if (!target) {
      throw new Error('Target not found');
    }
    return target;
  }

  override target(): Target {
    return this.#browserTarget;
  }

  override async disconnect(): Promise<void> {
    try {
      // Fail silently if the session cannot be ended.
      await this.#connection.send('session.end', {});
    } catch (e) {
      debugError(e);
    }
    this.#connection.dispose();
  }
}
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/Browser.ts (TypeScript)

Code before:
  override newPage(): Promise<Page> {
    return this.#defaultContext.newPage();
  }

  override targets(): Target[] {
    return [this.#browserTarget, ...Array.from(this.#targets.values())];
  }

  _getTargetById(id: string): BidiTarget {
    const target = this.#targets.get(id);
    if (!target) {
      throw new Error('Target not found');
    }
    return target;
  }

  override target(): Target {
    return this.#browserTarget;
  }

  override async disconnect(): Promise<void> {
    try {
      await this.#connection.send('session.end', {});
      this.#connection.dispose();
    } catch (e) {
      console.log(e);
    }
  }
}


Review comment (bug):
Better use `debugError` instead of console.log. Also, we probably should call this.#connection.dispose(); even if session.end fails.

Code after:
  }

  override newPage(): Promise<Page> {
    return this.#defaultContext.newPage();
  }

  override targets(): Target[] {
    return [this.#browserTarget, ...Array.from(this.#targets.values())];
  }

  _getTargetById(id: string): BidiTarget {
    const target = this.#targets.get(id);
    if (!target) {
      throw new Error('Target not found');
    }
    return target;
  }

  override target(): Target {
    return this.#browserTarget;
  }

  override async disconnect(): Promise<void> {
    try {
      // Fail silently if the session cannot be ended.
      await this.#connection.send('session.end', {});
    } catch (e) {
      debugError(e);
    }
    this.#connection.dispose();
  }
}
Repository: puppeteer/puppeteer
File: test/src/worker.spec.ts (TypeScript)

Code before:
/**
 * Copyright 2020 Google Inc. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import expect from 'expect';
import type {CdpWebWorker} from 'puppeteer-core/internal/cdp/WebWorker.js';
import type {ConsoleMessage} from 'puppeteer-core/internal/common/ConsoleMessage.js';

import {getTestState, setupTestBrowserHooks} from './mocha-utils.js';
import {waitEvent} from './utils.js';

describe('Workers', function () {
  setupTestBrowserHooks();

  it('Page.workers', async () => {
    const {page, server} = await getTestState();

    await Promise.all([
      waitEvent(page, 'workercreated'),
      page.goto(server.PREFIX + '/worker/worker.html'),
    ]);
    const worker = page.workers()[0]!;
    expect(worker?.url()).toContain('worker.js');

    expect(
      await worker?.evaluate(() => {
        return (globalThis as any).workerFunction();
      })
    ).toBe('worker function result');

    await page.goto(server.EMPTY_PAGE);

Review comment (bug):
This looks incorrect, we should use the API one when referring to the type here  

Code after:
/**
 * Copyright 2020 Google Inc. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import expect from 'expect';
import type {WebWorker} from 'puppeteer-core/internal/api/WebWorker.js';
import type {ConsoleMessage} from 'puppeteer-core/internal/common/ConsoleMessage.js';

import {getTestState, setupTestBrowserHooks} from './mocha-utils.js';
import {waitEvent} from './utils.js';

describe('Workers', function () {
  setupTestBrowserHooks();

  it('Page.workers', async () => {
    const {page, server} = await getTestState();

    await Promise.all([
      waitEvent(page, 'workercreated'),
      page.goto(server.PREFIX + '/worker/worker.html'),
    ]);
    const worker = page.workers()[0]!;
    expect(worker?.url()).toContain('worker.js');

    expect(
      await worker?.evaluate(() => {
        return (globalThis as any).workerFunction();
      })
    ).toBe('worker function result');

    await page.goto(server.EMPTY_PAGE);
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/api/WebWorker.ts (TypeScript)

Code before:
  >(func: Func | string, ...args: Params): Promise<Awaited<ReturnType<Func>>> {
    func = withSourcePuppeteerURLIfNone(this.evaluate.name, func);
    return await this.mainRealm().evaluate(func, ...args);
  }

  /**
   * Evaluates a given function in the {@link WebWorker | worker}.
   *
   * @remarks If the given function returns a promise,
   * {@link WebWorker.evaluate | evaluate} will wait for the promise to resolve.
   *
   * In general, you should use
   * {@link WebWorker.evaluateHandle | evaluateHandle} if
   * {@link WebWorker.evaluate | evaluate} cannot serialize the return value
   * properly or you need a mutable {@link JSHandle | handle} to the return
   * object.
   *
   * @param func - Function to be evaluated.
   * @param args - Arguments to pass into `func`.
   * @returns A {@link JSHandle | handle} to the return value of `func`.
   */
  async evaluateHandle<
    Params extends unknown[],
    Func extends EvaluateFunc<Params> = EvaluateFunc<Params>,
  >(
    pageFunction: Func | string,
    ...args: Params
  ): Promise<HandleFor<Awaited<ReturnType<Func>>>> {
    pageFunction = withSourcePuppeteerURLIfNone(
      this.evaluateHandle.name,
      pageFunction
    );
    return await this.mainRealm().evaluateHandle(pageFunction, ...args);
  }
}


Review comment (bug):
Else documentation is broken
```suggestion
    func: Func | string,
```

Code after:
    Func extends EvaluateFunc<Params> = EvaluateFunc<Params>,
  >(func: Func | string, ...args: Params): Promise<Awaited<ReturnType<Func>>> {
    func = withSourcePuppeteerURLIfNone(this.evaluate.name, func);
    return await this.mainRealm().evaluate(func, ...args);
  }

  /**
   * Evaluates a given function in the {@link WebWorker | worker}.
   *
   * @remarks If the given function returns a promise,
   * {@link WebWorker.evaluate | evaluate} will wait for the promise to resolve.
   *
   * In general, you should use
   * {@link WebWorker.evaluateHandle | evaluateHandle} if
   * {@link WebWorker.evaluate | evaluate} cannot serialize the return value
   * properly or you need a mutable {@link JSHandle | handle} to the return
   * object.
   *
   * @param func - Function to be evaluated.
   * @param args - Arguments to pass into `func`.
   * @returns A {@link JSHandle | handle} to the return value of `func`.
   */
  async evaluateHandle<
    Params extends unknown[],
    Func extends EvaluateFunc<Params> = EvaluateFunc<Params>,
  >(
    func: Func | string,
    ...args: Params
  ): Promise<HandleFor<Awaited<ReturnType<Func>>>> {
    func = withSourcePuppeteerURLIfNone(this.evaluateHandle.name, func);
    return await this.mainRealm().evaluateHandle(func, ...args);
  }
}
Repository: puppeteer/puppeteer
File: test/src/browser.spec.ts (TypeScript)

Code before:
      const target = browser.target();
      expect(target.type()).toBe('browser');
    });
  });

  describe('Browser.process', function () {
    it('should return child_process instance', async () => {
      const {browser} = await getTestState();

      const process = await browser.process();
      expect(process!.pid).toBeGreaterThan(0);
    });
    it('should not return child_process for remote browser', async () => {
      const {browser, puppeteer} = await getTestState();

      const browserWSEndpoint = browser.wsEndpoint();
      const remoteBrowser = await puppeteer.connect({
        browserWSEndpoint,
        protocol: browser.protocol,
      });
      expect(remoteBrowser.process()).toBe(null);
      await remoteBrowser.disconnect();
    });
  });

  describe('Browser.connected', () => {
    it('should set the browser connected state', async () => {
      const {browser, puppeteer} = await getTestState();

      const browserWSEndpoint = browser.wsEndpoint();
      const newBrowser = await puppeteer.connect({
        browserWSEndpoint,
        protocol: browser.protocol,
      });
      expect(newBrowser.connected).toBe(true);
      await newBrowser.disconnect();
      expect(newBrowser.connected).toBe(false);
    });
  });
});


Review comment (suggestion):
Let's revert unrelated changes. Also https://pptr.dev/api/puppeteer.browser.isconnected there is some issue with the documentation for Browser.connected

Code after:
      const target = browser.target();
      expect(target.type()).toBe('browser');
    });
  });

  describe('Browser.process', function () {
    it('should return child_process instance', async () => {
      const {browser} = await getTestState();

      const process = await browser.process();
      expect(process!.pid).toBeGreaterThan(0);
    });
    it('should not return child_process for remote browser', async () => {
      const {browser, puppeteer} = await getTestState();

      const browserWSEndpoint = browser.wsEndpoint();
      const remoteBrowser = await puppeteer.connect({
        browserWSEndpoint,
        protocol: browser.protocol,
      });
      expect(remoteBrowser.process()).toBe(null);
      await remoteBrowser.disconnect();
    });
  });

  describe('Browser.isConnected', () => {
    it('should set the browser connected state', async () => {
      const {browser, puppeteer} = await getTestState();

      const browserWSEndpoint = browser.wsEndpoint();
      const newBrowser = await puppeteer.connect({
        browserWSEndpoint,
        protocol: browser.protocol,
      });
      expect(newBrowser.isConnected()).toBe(true);
      await newBrowser.disconnect();
      expect(newBrowser.isConnected()).toBe(false);
    });
  });
});
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/bidi/Browser.ts (TypeScript)

Code before:
      target.browserContext().emit(BrowserContextEvent.TargetDestroyed, target);
    }
  }

  get connection(): BidiConnection {
    // SAFETY: We only have one implementation.
    return this.#browserCore.session.connection as BidiConnection;
  }

  override wsEndpoint(): string {
    return this.connection.url;
  }

  override async close(): Promise<void> {
    for (const [eventName, handler] of this.#connectionEventHandlers) {
      this.connection.off(eventName, handler);
    }
    if (this.connection.closed) {
      return;
    }

    try {
      await this.#browserCore.close();
      await this.#closeCallback?.call(null);
    } catch (error) {
      // Fail silently.
      debugError(error);
    }
  }

  override get connected(): boolean {
    return !this.#browserCore.disposed;
  }

  override process(): ChildProcess | null {
    return this.#process ?? null;
  }

  override async createIncognitoBrowserContext(
    _options?: BrowserContextOptions
  ): Promise<BidiBrowserContext> {
    // TODO: implement incognito context https://github.com/w3c/webdriver-bidi/issues/289.
    const context = new BidiBrowserContext(this, {
      defaultViewport: this.#defaultViewport,
      isDefault: false,
    });
    this.#contexts.push(context);
    return context;
  }

  override async version(): Promise<string> {

Review comment (bug):
I think we should not fail silently for closeCallback and also we should run it even if #browserCore.close() failed.  Perhaps move it to finally?

Code after:
      target.browserContext().emit(BrowserContextEvent.TargetDestroyed, target);
    }
  }

  get connection(): BidiConnection {
    // SAFETY: We only have one implementation.
    return this.#browserCore.session.connection as BidiConnection;
  }

  override wsEndpoint(): string {
    return this.connection.url;
  }

  override async close(): Promise<void> {
    for (const [eventName, handler] of this.#connectionEventHandlers) {
      this.connection.off(eventName, handler);
    }
    if (this.connection.closed) {
      return;
    }

    try {
      await this.#browserCore.close();
      await this.#closeCallback?.call(null);
    } catch (error) {
      // Fail silently.
      debugError(error);
    } finally {
      this.connection.dispose();
    }
  }

  override get connected(): boolean {
    return !this.#browserCore.disposed;
  }

  override process(): ChildProcess | null {
    return this.#process ?? null;
  }

  override async createIncognitoBrowserContext(
    _options?: BrowserContextOptions
  ): Promise<BidiBrowserContext> {
    // TODO: implement incognito context https://github.com/w3c/webdriver-bidi/issues/289.
    const context = new BidiBrowserContext(this, {
      defaultViewport: this.#defaultViewport,
      isDefault: false,
    });
    this.#contexts.push(context);
    return context;
  }