ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/playwright
File: packages/playwright/src/reporters/reporterV2.ts (TypeScript)

Code before:
        this.onStdErr(item.stderr.chunk, item.stderr.test, item.stderr.result);
    }
  }

  onTestBegin(test: TestCase, result: TestResult) {
    this._reporter.onTestBegin?.(test, result);
  }

  onStdOut(chunk: string | Buffer, test?: TestCase, result?: TestResult) {
    if (this._deferred) {
      this._deferred.push({ stdout: { chunk, test, result } });
      return;
    }
    this._reporter.onStdOut?.(chunk, test, result);
  }

  onStdErr(chunk: string | Buffer, test?: TestCase, result?: TestResult) {
    if (this._deferred) {
      this._deferred.push({ stderr: { chunk, test, result } });
      return;
    }
    this._reporter.onStdErr?.(chunk, test, result);
  }

  async onTestPaused(test: TestCase, result: TestResult, step?: TestStep) {
    return await (this._reporter as ReporterV2).onTestPaused?.(test, result, step);
  }

  onTestEnd(test: TestCase, result: TestResult) {
    this._reporter.onTestEnd?.(test, result);
  }

  async onEnd(result: FullResult) {
    return await this._reporter.onEnd?.(result);
  }

  async onExit() {
    await this._reporter.onExit?.();
  }

  onError(error: TestError) {
    if (this._deferred) {
      this._deferred.push({ error });
      return;
    }
    this._reporter.onError?.(error);
  }

  onStepBegin(test: TestCase, result: TestResult, step: TestStep) {
    this._reporter.onStepBegin?.(test, result, step);
  }

Review comment (suggestion):
`reporter` v1 does not have `onTestPaused()`, so there is no reason to call it.

Code after:
        this.onStdErr(item.stderr.chunk, item.stderr.test, item.stderr.result);
    }
  }

  onTestBegin(test: TestCase, result: TestResult) {
    this._reporter.onTestBegin?.(test, result);
  }

  onStdOut(chunk: string | Buffer, test?: TestCase, result?: TestResult) {
    if (this._deferred) {
      this._deferred.push({ stdout: { chunk, test, result } });
      return;
    }
    this._reporter.onStdOut?.(chunk, test, result);
  }

  onStdErr(chunk: string | Buffer, test?: TestCase, result?: TestResult) {
    if (this._deferred) {
      this._deferred.push({ stderr: { chunk, test, result } });
      return;
    }
    this._reporter.onStdErr?.(chunk, test, result);
  }

  onTestEnd(test: TestCase, result: TestResult) {
    this._reporter.onTestEnd?.(test, result);
  }

  async onEnd(result: FullResult) {
    return await this._reporter.onEnd?.(result);
  }

  async onExit() {
    await this._reporter.onExit?.();
  }

  onError(error: TestError) {
    if (this._deferred) {
      this._deferred.push({ error });
      return;
    }
    this._reporter.onError?.(error);
  }

  onStepBegin(test: TestCase, result: TestResult, step: TestStep) {
    this._reporter.onStepBegin?.(test, result, step);
  }

  onStepEnd(test: TestCase, result: TestResult, step: TestStep) {
    this._reporter.onStepEnd?.(test, result, step);
  }
Repository: microsoft/playwright
File: tests/playwright-test/timeout.spec.ts (TypeScript)

Code before:
/**
 * Copyright (c) Microsoft Corporation.
 *
 * 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 { ReporterV2 } from 'packages/playwright/src/reporters/reporterV2';
import { test, expect } from './playwright-test-fixtures';

test('should run fixture teardown on timeout', async ({ runInlineTest }) => {
  const result = await runInlineTest({
    'helper.ts': `
      import { test as base, expect } from '@playwright/test';
      export const test = base.extend({
        foo: async ({}, run, testInfo) => {
          await run();
          console.log('STATUS:' + testInfo.status);
        }
      });
    `,
    'c.spec.ts': `
      import { test } from './helper';
      test('works', async ({ foo }) => {
        await new Promise(f => setTimeout(f, 100000));
      });
    `
  }, { timeout: 1000 });
  expect(result.exitCode).toBe(1);
  expect(result.failed).toBe(1);
  expect(result.output).toContain('STATUS:timedOut');
});


Review comment (suggestion):
```suggestion
import type { ReporterV2 } from 'packages/playwright/src/reporters/reporterV2';
```

Code after:
/**
 * Copyright (c) Microsoft Corporation.
 *
 * 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 type { ReporterV2 } from 'packages/playwright/src/reporters/reporterV2';
import { test, expect } from './playwright-test-fixtures';

test('should run fixture teardown on timeout', async ({ runInlineTest }) => {
  const result = await runInlineTest({
    'helper.ts': `
      import { test as base, expect } from '@playwright/test';
      export const test = base.extend({
        foo: async ({}, run, testInfo) => {
          await run();
          console.log('STATUS:' + testInfo.status);
        }
      });
    `,
    'c.spec.ts': `
      import { test } from './helper';
      test('works', async ({ foo }) => {
        await new Promise(f => setTimeout(f, 100000));
      });
    `
  }, { timeout: 1000 });
  expect(result.exitCode).toBe(1);
  expect(result.failed).toBe(1);
  expect(result.output).toContain('STATUS:timedOut');
});
Repository: microsoft/playwright
File: tests/mcp/video.spec.ts (TypeScript)

Code before:
      arguments: { url: server.HELLO_WORLD },
    })).toHaveResponse({
      code: expect.stringContaining(`page.goto('http://localhost`),
    });

    await client.callTool({
      name: 'browser_evaluate',
      arguments: {
        function: `async () => {
          document.body.style.backgroundColor = "red";
          await new Promise(resolve => setTimeout(resolve, 100));
          document.body.style.backgroundColor = "green";
          await new Promise(resolve => setTimeout(resolve, 100));
          document.body.style.backgroundColor = "blue";
          await new Promise(resolve => setTimeout(resolve, 100));
        }`,
      },
    });

    expect(await client.callTool({
      name: 'browser_close',
    })).toHaveResponse({
      code: expect.stringContaining(`page.close()`),
    });

    const [file] = await fs.promises.readdir(videosDir);
    expect(file).toMatch(/.*.\webm/);
  });
}


Review comment (suggestion):
Make a fixture for `test.info().outputPath('tmp', 'playwright-mcp-output')` path, otherwise it's hard to track where it comes from.

Code after:
  })).toHaveResponse({
    code: expect.stringContaining(`page.close()`),
  });
}

async function produceFrames(client: Client) {
  expect(await client.callTool({
    name: 'browser_evaluate',
    arguments: {
      function: `async () => {
        document.body.style.backgroundColor = "red";
        await new Promise(resolve => setTimeout(resolve, 100));
        document.body.style.backgroundColor = "green";
        await new Promise(resolve => setTimeout(resolve, 100));
        document.body.style.backgroundColor = "blue";
        await new Promise(resolve => setTimeout(resolve, 100));
        return 'ok';
      }`,
    },
  })).toHaveResponse({
    result: '"ok"',
  });
}

async function checkIntermediateVideoFileExists(videosDir?: string) {
  const files = await fs.promises.readdir(videosDir ?? test.info().outputPath('tmp', 'playwright-mcp-output'));
  expect(files[0]).toMatch(/\.webm/);
}
Repository: microsoft/playwright
File: packages/html-reporter/src/speedboard.tsx (TypeScript)

Code before:
    return null;

  let clash = false;
  const bots: Record<string, number[]> = {};
  for (const shard of shards) {
    let botName = shard.botName || '';
    if (botName.startsWith('@'))
      botName = botName.slice(1);
    bots[botName] ??= [];
    const shardIndex = Math.max(shard.shardIndex - 1, 0);
    if (bots[botName][shardIndex] !== undefined)
      clash = true;
    bots[botName][shardIndex] = shard.duration;
  }

  const maxSeries = Math.max(...Object.values(bots).map(shardDurations => shardDurations.length));

  return <AutoChip header='Shard Duration'>
    <GroupedBarChart
      data={Object.values(bots)}
      groups={Object.keys(bots)}
      series={Array.from({ length: maxSeries }).map((_, i) => `Shard ${i + 1}`)}
    />
    {clash && <div style={{ marginTop: 8 }}>
      <icons.warning />
      Some shards could not be differentiated because of missing bot names.
      Please refer to <a href='https://playwright.dev/docs/test-sharding#merging-reports-from-multiple-environments' target='_blank' rel='noopener noreferrer'>
        the docs
      </a> on how to fix this.
    </div>}
  </AutoChip>;
}


Review comment (suggestion):
```suggestion
      Some shards could not be differentiated because of missing global tags.
```

Code after:
    return null;

  let clash = false;
  const bots: Record<string, number[]> = {};
  for (const shard of shards) {
    const botName = shard.tag.join(' ');
    bots[botName] ??= [];
    const shardIndex = Math.max((shard.shardIndex ?? 1) - 1, 0);
    if (bots[botName][shardIndex] !== undefined)
      clash = true;
    bots[botName][shardIndex] = shard.duration;
  }

  const maxSeries = Math.max(...Object.values(bots).map(shardDurations => shardDurations.length));

  return <AutoChip header='Shard Duration'>
    <GroupedBarChart
      data={Object.values(bots)}
      groups={Object.keys(bots)}
      series={Array.from({ length: maxSeries }).map((_, i) => `Shard ${i + 1}`)}
    />
    {clash && <div style={{ marginTop: 8 }}>
      <icons.warning />
      Some shards could not be differentiated because of missing global tags.
      Please refer to <a href='https://playwright.dev/docs/test-sharding#merging-reports-from-multiple-environments' target='_blank' rel='noopener noreferrer'>
        the docs
      </a> on how to fix this.
    </div>}
  </AutoChip>;
}
Repository: microsoft/playwright
File: utils/generate_types/overrides-testReporter.d.ts (TypeScript)

Code before:
   *   - 'timedout' - the global time has been reached.
   *   - 'interrupted' - interrupted by the user.
   */
  status: 'passed' | 'failed' | 'timedout' | 'interrupted';

  /**
   * Test start wall time.
   */
  startTime: Date;

  /**
   * Test duration in milliseconds.
   */
  duration: number;

  /**
   * Only present on merged reports.
   */
  shards?: {
    /**
     * The index of the shard, one-based.
     */
    shardIndex?: number;

    /**
     * Bot tag that differentiates CI environments.
     */
    tag: string[];

    /**
     * Start wall time of shard.
     */
    startTime: Date;

    /**
     * Shard run duration in milliseconds.
     */
    duration: number;
  }[];
}

export interface Reporter {
  onEnd?(result: FullResult): Promise<{ status?: FullResult['status'] } | undefined | void> | void;
}

export interface JSONReport {
  config: Omit<FullConfig, 'projects'> & {
    projects: {
      outputDir: string,
      repeatEach: number,
      retries: number,

Review comment (suggestion):
```suggestion
     * Global tag that differentiates CI environments.
```

Code after:
   *   - 'timedout' - the global time has been reached.
   *   - 'interrupted' - interrupted by the user.
   */
  status: 'passed' | 'failed' | 'timedout' | 'interrupted';

  /**
   * Test start wall time.
   */
  startTime: Date;

  /**
   * Test duration in milliseconds.
   */
  duration: number;

  /**
   * Only present on merged reports.
   */
  shards?: {
    /**
     * The index of the shard, one-based.
     */
    shardIndex?: number;

    /**
     * Global tag that differentiates CI environments.
     */
    tag: string[];

    /**
     * Start wall time of shard.
     */
    startTime: Date;

    /**
     * Shard run duration in milliseconds.
     */
    duration: number;
  }[];
}

export interface Reporter {
  onEnd?(result: FullResult): Promise<{ status?: FullResult['status'] } | undefined | void> | void;
}

export interface JSONReport {
  config: Omit<FullConfig, 'projects'> & {
    projects: {
      outputDir: string,
      repeatEach: number,
      retries: number,
Repository: microsoft/playwright
File: packages/playwright-core/src/server/agent/pageAgent.ts (TypeScript)

Code before:
  cache.newActions[cacheKey] = newEntry;

  if (cacheOutFile) {
    const entries = Object.entries(cache.newActions);
    entries.sort((e1, e2) => e1[0].localeCompare(e2[0]));
    await fs.promises.writeFile(cacheOutFile, JSON.stringify(Object.fromEntries(entries), undefined, 2));
  } else if (cacheFile) {
    const entries = Object.entries(cache.actions);
    entries.sort((e1, e2) => e1[0].localeCompare(e2[0]));
    await fs.promises.writeFile(cacheFile, JSON.stringify(Object.fromEntries(entries), undefined, 2));
  }
}

type Cache = {
  actions: actions.CachedActions;
  newActions: actions.CachedActions;
};

const allCaches = new Map<string, Cache>();

async function cachedActions(cacheFile: string): Promise<Cache> {
  let cache = allCaches.get(cacheFile);
  if (!cache) {
    let json: any;
    try {
      json = await fs.promises.readFile(cacheFile, 'utf-8').then(text => JSON.parse(text));
    } catch (error) {
      throw new Error(`Failed to parse cache file ${cacheFile}:\n${error.message}`);
    }
    const parsed = actions.cachedActionsSchema.safeParse(json);
    if (parsed.error)
      throw new Error(`Failed to parse cache file ${cacheFile}:\n${zod.prettifyError(parsed.error)}`);
    cache = { actions: parsed.data, newActions: {} };
    allCaches.set(cacheFile, cache);
  }
  return cache;
}


Review comment (performance):
```suggestion
      json = await fs.promises.readFile(cacheFile, 'utf-8').then(text => JSON.parse(text.trim() || '{}'));
```

Code after:

  if (cacheOutFile) {
    const entries = Object.entries(cache.newActions);
    entries.sort((e1, e2) => e1[0].localeCompare(e2[0]));
    await fs.promises.writeFile(cacheOutFile, JSON.stringify(Object.fromEntries(entries), undefined, 2));
  } else if (cacheFile) {
    const entries = Object.entries(cache.actions);
    entries.sort((e1, e2) => e1[0].localeCompare(e2[0]));
    await fs.promises.writeFile(cacheFile, JSON.stringify(Object.fromEntries(entries), undefined, 2));
  }
}

type Cache = {
  actions: actions.CachedActions;
  newActions: actions.CachedActions;
};

const allCaches = new Map<string, Cache>();

async function cachedActions(cacheFile: string): Promise<Cache> {
  let cache = allCaches.get(cacheFile);
  if (!cache) {
    const content = await fs.promises.readFile(cacheFile, 'utf-8').catch(() => '');
    let json: any;
    try {
      json = JSON.parse(content.trim() || '{}');
    } catch (error) {
      throw new Error(`Failed to parse cache file ${cacheFile}:\n${error.message}`);
    }
    const parsed = actions.cachedActionsSchema.safeParse(json);
    if (parsed.error)
      throw new Error(`Failed to parse cache file ${cacheFile}:\n${zod.prettifyError(parsed.error)}`);
    cache = { actions: parsed.data, newActions: {} };
    allCaches.set(cacheFile, cache);
  }
  return cache;
}
Repository: microsoft/playwright
File: packages/playwright/src/mcp/terminal/cli.ts (TypeScript)

Code before:
    process.exit(0);
  }

  // Handle 'session' command specially - it doesn't need daemon connection
  if (commandName === 'session') {
    await handleSessionCommand(args);
    return;
  }

  const command = help.commands[commandName];
  if (args.help || args.h) {
    if (command) {
      console.log(command);
    } else {
      console.log('playwright-cli - run playwright mcp commands from terminal\n');
      console.log(help.global);
    }
    process.exit(0);
  }
  if (!command) {
    console.error(`Unknown command: ${commandName}\n`);
    console.log(help.global);
    process.exit(1);
  }

  // Resolve session name: --session flag > PLAYWRIGHT_CLI_SESSION env > current session > 'default'
  let sessionName = resolveSessionName(args);
  if (sessionName === 'default' && !args.session && !process.env.PLAYWRIGHT_CLI_SESSION)
    sessionName = await getCurrentSession();

  await runCliCommand(sessionName, args);
}

main().catch(e => {
  console.error(e.message);
  process.exit(1);
});


Review comment (suggestion):
```suggestion
  if (sessionName === 'default')
```

Code after:

  // Handle 'session' command specially - it doesn't need daemon connection
  if (commandName === 'session') {
    await handleSessionCommand(args);
    return;
  }

  const command = help.commands[commandName];
  if (args.help || args.h) {
    if (command) {
      console.log(command);
    } else {
      console.log('playwright-cli - run playwright mcp commands from terminal\n');
      console.log(help.global);
    }
    process.exit(0);
  }
  if (!command) {
    console.error(`Unknown command: ${commandName}\n`);
    console.log(help.global);
    process.exit(1);
  }

  // Resolve session name: --session flag > PLAYWRIGHT_CLI_SESSION env > current session > 'default'
  let sessionName = resolveSessionName(args);
  if (sessionName === 'default' && !args.session && !process.env.PLAYWRIGHT_CLI_SESSION)
    sessionName = await getCurrentSession();

  runCliCommand(sessionName, args).catch(e => {
    console.error(e.message);
    process.exit(1);
  });
}

main().catch(e => {
  console.error(e.message);
  process.exit(1);
});
Repository: microsoft/playwright
File: packages/playwright/src/mcp/browser/response.ts (TypeScript)

Code before:
 */

import fs from 'fs';
import path from 'path';

import { debug } from 'playwright-core/lib/utilsBundle';
import { renderModalStates, shouldIncludeMessage } from './tab';
import { dateAsFileName } from './tools/utils';
import { scaleImageToFitMessage } from './tools/screenshot';

import type { TabHeader } from './tab';
import type { CallToolResult, ImageContent, TextContent } from '@modelcontextprotocol/sdk/types.js';
import type { Context } from './context';

export const requestDebug = debug('pw:mcp:request');

type Result = {
  data: Buffer | string;
  title: string;
  file?:  {
    prefix: string;
    ext: string;
    suggestedFilename?: string;
    contentType?: string;
  };
};

type Section = {
  title: string;
  content: Result[];
  isError?: boolean;
};

export class Response {
  private _results: Result[] = [];
  private _errors: string[] = [];
  private _code: string[] = [];
  private _context: Context;
  private _includeSnapshot: 'none' | 'full' | 'incremental' = 'none';
  private _includeSnapshotFileName: string | undefined;

  readonly toolName: string;
  readonly toolArgs: Record<string, any>;

  private constructor(ordinal: number, context: Context, toolName: string, toolArgs: Record<string, any>) {
    this._context = context;
    this.toolName = toolName;
    this.toolArgs = toolArgs;
  }

  static _ordinal = 0;

Review comment (suggestion):
Perhaps call this `imageContentType`? Up until `serializeResponse` it was not clear this is not suitable for, say, yaml snapshots.

Code after:
 */

import fs from 'fs';
import path from 'path';

import { debug } from 'playwright-core/lib/utilsBundle';
import { renderModalStates, shouldIncludeMessage } from './tab';
import { dateAsFileName } from './tools/utils';
import { scaleImageToFitMessage } from './tools/screenshot';

import type { TabHeader } from './tab';
import type { CallToolResult, ImageContent, TextContent } from '@modelcontextprotocol/sdk/types.js';
import type { Context } from './context';

export const requestDebug = debug('pw:mcp:request');

type Result = {
  text?: string;
  data?: Buffer;
  isBase64?: boolean;
  title: string;
  file?:  {
    prefix: string;
    ext: string;
    suggestedFilename?: string;
    contentType?: string;
  };
};

export type Section = {
  title: string;
  content: Result[];
  isError?: boolean;
};

export class Response {
  private _results: Result[] = [];
  private _errors: string[] = [];
  private _code: string[] = [];
  private _context: Context;
  private _includeSnapshot: 'none' | 'full' | 'incremental' = 'none';
  private _includeSnapshotFileName: string | undefined;

  readonly toolName: string;
  readonly toolArgs: Record<string, any>;

  private constructor(ordinal: number, context: Context, toolName: string, toolArgs: Record<string, any>) {
    this._context = context;
    this.toolName = toolName;
    this.toolArgs = toolArgs;
  }
Repository: microsoft/playwright
File: packages/playwright/src/mcp/browser/context.ts (TypeScript)

Code before:
  for (const root of clientInfo.roots) {
    const url = new URL(root.uri);
    let rootPath;
    try {
      rootPath = fileURLToPath(url);
    } catch (e) {
      // Support WSL paths on Windows.
      if (e.code === 'ERR_INVALID_FILE_URL_PATH' && os.platform() === 'win32')
        rootPath = decodeURIComponent(url.pathname);
    }
    if (!rootPath)
      continue;
    paths.push(rootPath);
  }
  if (paths.length === 0)
    paths.push(process.cwd());
  return paths;
}


function originOrHostGlob(originOrHost: string) {
  // Support wildcard port patterns like "http://localhost:*" or "https://example.com:*"
  const wildcardPortMatch = originOrHost.match(/^(https?):\/\/([^/:]+):\*$/);
  if (wildcardPortMatch) {
    const [, protocol, hostname] = wildcardPortMatch;
    return `${protocol}://${hostname}:*/**`;
  }

  try {
    const url = new URL(originOrHost);
    // localhost:1234 will parse as protocol 'localhost:' and 'null' origin.
    if (url.origin !== 'null')
      return `${url.origin}/**`;
  } catch {
  }
  // Support for legacy host-only mode.
  return `*://${originOrHost}/**`;
}


Review comment (suggestion):
No need to capture separate groups:

```js
  const wildcardPortMatch = originOrHost.match(/^(https?:\/\/[^/:]+):\*$/);
  if (wildcardPortMatch)
    return `${wildcardPortMatch[1]}:*/**`;
```

Code after:
  for (const root of clientInfo.roots) {
    const url = new URL(root.uri);
    let rootPath;
    try {
      rootPath = fileURLToPath(url);
    } catch (e) {
      // Support WSL paths on Windows.
      if (e.code === 'ERR_INVALID_FILE_URL_PATH' && os.platform() === 'win32')
        rootPath = decodeURIComponent(url.pathname);
    }
    if (!rootPath)
      continue;
    paths.push(rootPath);
  }
  if (paths.length === 0)
    paths.push(process.cwd());
  return paths;
}


function originOrHostGlob(originOrHost: string) {
  // Support wildcard port patterns like "http://localhost:*" or "https://example.com:*"
  const wildcardPortMatch = originOrHost.match(/^(https?:\/\/[^/:]+):\*$/);
  if (wildcardPortMatch)
    return `${wildcardPortMatch[1]}:*/**`;

  try {
    const url = new URL(originOrHost);
    // localhost:1234 will parse as protocol 'localhost:' and 'null' origin.
    if (url.origin !== 'null')
      return `${url.origin}/**`;
  } catch {
  }
  // Support for legacy host-only mode.
  return `*://${originOrHost}/**`;
}