ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/playwright
File: packages/playwright/src/mcp/browser/tab.ts (TypeScript)

Code before:
  download: Download;
};

type RequestLogEntry = {
  type: 'request';
  wallTime: number;
  request: playwright.Request;
};

type EventEntry = ConsoleLogEntry | DownloadStartLogEntry | DownloadFinishLogEntry | RequestLogEntry;


export type TabHeader = {
  title: string;
  url: string;
  current: boolean;
};

export type TabSnapshot = {
  ariaSnapshot: string;
  ariaSnapshotDiff?: string;
  modalStates: ModalState[];
  events: EventEntry[];
  consoleLog?: {
    file: string;
    firstNewEntryTimestamp: number;
    newEntryCount: number;
  }
};

export class Tab extends EventEmitter<TabEventsInterface> {
  readonly context: Context;
  readonly page: Page;
  private _lastHeader: TabHeader = { title: 'about:blank', url: 'about:blank', current: false };
  private _consoleMessages: ConsoleMessage[] = [];
  private _downloads: Download[] = [];
  private _requests: Set<playwright.Request> = new Set();
  private _onPageClose: (tab: Tab) => void;
  private _modalStates: ModalState[] = [];
  private _initializedPromise: Promise<void>;
  private _needsFullSnapshot = false;
  private _recentEventEntries: EventEntry[] = [];
  private _consoleLogFile: string | undefined;

  constructor(context: Context, page: playwright.Page, onPageClose: (tab: Tab) => void) {
    super();
    this.context = context;
    this.page = page as Page;
    this._onPageClose = onPageClose;
    page.on('console', event => this._handleConsoleMessage(messageToConsoleMessage(event)));
    page.on('pageerror', error => this._handleConsoleMessage(pageErrorToConsoleMessage(error)));

Review comment (suggestion):
The new entry timestamp has no meaning, should be event timestamps relative to the log creation instead.

Code after:
  download: Download;
};

type RequestLogEntry = {
  type: 'request';
  wallTime: number;
  request: playwright.Request;
};

type EventEntry = ConsoleLogEntry | DownloadStartLogEntry | DownloadFinishLogEntry | RequestLogEntry;


export type TabHeader = {
  title: string;
  url: string;
  current: boolean;
};

export type LogChunk = {
  file: string;
  fromLine: number;
  toLine: number;
  entryCount: number;
};

export type TabSnapshot = {
  ariaSnapshot: string;
  ariaSnapshotDiff?: string;
  modalStates: ModalState[];
  events: EventEntry[];
  logChunk?: LogChunk;
};

class LogFile {
  private _startTime: number = Date.now();
  private _context: Context;
  private _filePrefix: string;
  private _title: string;

  private _file: string | undefined;
  private _stopped: boolean = false;

  private _line: number = 0;
  private _entries: number = 0;
  private _lastLine: number = 0;
  private _lastEntries: number = 0;

  private _writeChain: Promise<void> = Promise.resolve();

  constructor(context: Context, filePrefix: string, title: string) {
    this._context = context;
Repository: microsoft/playwright
File: packages/playwright/src/mcp/terminal/program.ts (TypeScript)

Code before:
    return;
  }

  if (commandName === 'close') {
    await handleSessionCommand(sessionManager, 'stop', args);
    return;
  }

  await sessionManager.run(args);
}

const outputDir = path.join(process.cwd(), '.playwright-cli');

function daemonSocketPath(sessionName: string): string {
  const socketName = `${sessionName}.sock`;
  if (os.platform() === 'win32')
    return `\\\\.\\pipe\\${installationDirHash}-${socketName}`;
  const socketsDir = process.env.PLAYWRIGHT_DAEMON_SOCKETS_DIR || path.join(os.tmpdir(), 'playwright-cli');
  return path.join(socketsDir, installationDirHash, socketName);
}

function sessionConfigFromArgs(version: string, sessionName: string, args: MinimistArgs): SessionConfig {
  let config = args.config;
  try {
    if (!config && fs.existsSync('playwright-cli.json'))
      config = path.resolve(process.cwd(), 'playwright-cli.json');
  } catch {
  }
  return {
    version,
    socketPath: daemonSocketPath(sessionName),
    cli: {
      headed: args.headed,
      extension: args.extension,
      browser: args.browser,
      isolated: args.isolated,
      config,
    },
    userDataDirPrefix: path.resolve(daemonProfilesDir, `ud-${sessionName}`),
  };
}


Review comment (suggestion):
```suggestion
      config = path.resolve('playwright-cli.json');
```

Code after:
      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);
  }

  const sessionManager = await SessionManager.create(version, args);
  if (commandName.startsWith('session')) {
    const subcommand = args._[0].split('-').slice(1).join('-');
    await handleSessionCommand(sessionManager, subcommand, args);
    return;
  }

  if (commandName === 'config') {
    await handleSessionCommand(sessionManager, 'config', args);
    return;
  }

  if (commandName === 'close') {
    await handleSessionCommand(sessionManager, 'stop', args);
    return;
  }

  await sessionManager.run(args);
}

const outputDir = path.join(process.cwd(), '.playwright-cli');

function daemonSocketPath(sessionName: string): string {
  const socketName = `${sessionName}.sock`;
  if (os.platform() === 'win32')
    return `\\\\.\\pipe\\${installationDirHash}-${socketName}`;
  const socketsDir = process.env.PLAYWRIGHT_DAEMON_SOCKETS_DIR || path.join(os.tmpdir(), 'playwright-cli');
  return path.join(socketsDir, installationDirHash, socketName);
}

function sessionConfigFromArgs(version: string, sessionName: string, args: MinimistArgs): SessionConfig {
  let config = args.config;
  try {
    if (!config && fs.existsSync('playwright-cli.json'))
      config = path.resolve('playwright-cli.json');
  } catch {
  }
Repository: microsoft/playwright
File: packages/playwright-ct-core/src/vitePlugin.ts (TypeScript)

Code before:
          // Silent if can't read the file.
        }
      }
      return transformIndexFile(id, content, templateDir, registerSource, importInfos);
    },

    async writeBundle(this: PluginContext) {
      for (const importInfo of importInfos.values()) {
        const importPath = resolveHook(importInfo.filename, importInfo.importSource);
        if (!importPath)
          continue;
        const deps = new Set<string>();
        const id = await moduleResolver(importPath);
        if (!id)
          continue;
        collectViteModuleDependencies(this, id, deps);
        depsCollector.set(importPath, [...deps]);
      }
    },
  };
}

function collectViteModuleDependencies(context: PluginContext, id: string, deps: Set<string>) {
  if (!path.isAbsolute(id))
    return;
  const normalizedId = path.normalize(id).split(/\?|#/)[0];
  if (deps.has(normalizedId))
    return;
  deps.add(normalizedId);
  const module = context.getModuleInfo(id);
  for (const importedId of module?.importedIds || [])
    collectViteModuleDependencies(context, importedId, deps);
  for (const importedId of module?.dynamicallyImportedIds || [])
    collectViteModuleDependencies(context, importedId, deps);
}


Review comment (suggestion):
- I think we should strip `?` and `#` before the line 292, just in case there was something like `/..` in the query params or hash.
- It would be great to add a comment explaining/having an example of an import with a query param.

Code after:
          // Silent if can't read the file.
        }
      }
      return transformIndexFile(id, content, templateDir, registerSource, importInfos);
    },

    async writeBundle(this: PluginContext) {
      for (const importInfo of importInfos.values()) {
        const importPath = resolveHook(importInfo.filename, importInfo.importSource);
        if (!importPath)
          continue;
        const deps = new Set<string>();
        const id = await moduleResolver(importPath);
        if (!id)
          continue;
        collectViteModuleDependencies(this, id, deps);
        depsCollector.set(importPath, [...deps]);
      }
    },
  };
}

function collectViteModuleDependencies(context: PluginContext, id: string, deps: Set<string>) {
  // Example: "src/Component.tsx?raw" or "src/Component.tsx?import" should resolve to the file path.
  const cleanedId = id.split(/[?#]/)[0];
  const normalizedId = path.normalize(cleanedId);
  if (!path.isAbsolute(normalizedId))
    return;
  if (deps.has(normalizedId))
    return;
  deps.add(normalizedId);
  const module = context.getModuleInfo(id);
  for (const importedId of module?.importedIds || [])
    collectViteModuleDependencies(context, importedId, deps);
  for (const importedId of module?.dynamicallyImportedIds || [])
    collectViteModuleDependencies(context, importedId, deps);
}
Repository: microsoft/playwright
File: packages/playwright/src/mcp/browser/tools/config.ts (TypeScript)

Code before:
 *
 * 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 { z } from 'playwright-core/lib/mcpBundle';
import { defineTool } from './tool';

const configShow = defineTool({
  capability: 'config',

  schema: {
    name: 'browser_config_print',
    title: 'Print config',
    description: 'Print the final resolved config after merging CLI options, environment variables and config file.',
    inputSchema: z.object({}),
    type: 'readOnly',
  },

  handle: async (context, params, response) => {
    response.addTextResult(JSON.stringify(context.config, null, 2));
  },
});

export default [
  configShow,
];


Review comment (suggestion):
This is not a user config, this is our full config with internal properties (skill mode), potentially different in many ways.

Code after:
 *
 * 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 { z } from 'playwright-core/lib/mcpBundle';
import { defineTool } from './tool';

const configShow = defineTool({
  capability: 'config',

  schema: {
    name: 'browser_get_config',
    title: 'Get config',
    description: 'Get the final resolved config after merging CLI options, environment variables and config file.',
    inputSchema: z.object({}),
    type: 'readOnly',
  },

  handle: async (context, params, response) => {
    response.addTextResult(JSON.stringify(context.config, null, 2));
  },
});

export default [
  configShow,
];
Repository: microsoft/playwright
File: packages/playwright-core/src/server/trace/viewer/traceViewer.ts (TypeScript)

Code before:
      page.on('close', () => context.close({ reason: 'Trace viewer closed' }).catch(() => {}));

    await page.mainFrame().goto(progress, url);
  });
  return page;
}

export async function openTraceInBrowser(url: string) {
  // eslint-disable-next-line no-console
  console.log('\nListening on ' + url);
  if (!isUnderTest())
    await open(url.replace('0.0.0.0', 'localhost')).catch(() => {});
}

class StdinServer implements Transport {
  private _pollTimer: NodeJS.Timeout | undefined;
  private _traceUrl: string | undefined;

  constructor(stdin: Readable) {
    stdin.on('data', data => {
      const url = data.toString().trim();
      if (url === this._traceUrl)
        return;
      if (url.endsWith('.json'))
        this._pollLoadTrace(url);
      else
        this._loadTrace(url);
    });
    stdin.on('close', () => gracefullyProcessExitDoNotHang(0));
  }

  onconnect() {
  }

  async dispatch(method: string, params: any) {
    if (method === 'initialize') {
      if (this._traceUrl)
        this._loadTrace(this._traceUrl);
    }
  }

  onclose() {
  }

  sendEvent?: (method: string, params: any) => void;
  close?: () => void;

  private _loadTrace(traceUrl: string) {
    this._traceUrl = traceUrl;
    clearTimeout(this._pollTimer);
    this.sendEvent?.('loadTraceRequested', { traceUrl: validateTraceUrl(traceUrl) });

Review comment (suggestion):
- Make sure that when something enters our code it goes via validateTraceUrl immediately. Rename it to validateTraceUrl to validateTraceUrlOrPath

Code after:

    await page.mainFrame().goto(progress, url);
  });
  return page;
}

export async function openTraceInBrowser(url: string) {
  // eslint-disable-next-line no-console
  console.log('\nListening on ' + url);
  if (!isUnderTest())
    await open(url.replace('0.0.0.0', 'localhost')).catch(() => {});
}

class StdinServer implements Transport {
  private _pollTimer: NodeJS.Timeout | undefined;
  private _traceUrl: string | undefined;

  constructor() {
    process.stdin.on('data', data => {
      const url = validateTraceUrlOrPath(data.toString().trim());
      if (!url || url === this._traceUrl)
        return;
      if (url.endsWith('.json'))
        this._pollLoadTrace(url);
      else
        this._loadTrace(url);
    });
    process.stdin.on('close', () => gracefullyProcessExitDoNotHang(0));
  }

  onconnect() {
  }

  async dispatch(method: string, params: any) {
    if (method === 'initialize') {
      if (this._traceUrl)
        this._loadTrace(this._traceUrl);
    }
  }

  onclose() {
  }

  sendEvent?: (method: string, params: any) => void;
  close?: () => void;

  private _loadTrace(traceUrl: string) {
    this._traceUrl = traceUrl;
    clearTimeout(this._pollTimer);
    this.sendEvent?.('loadTraceRequested', { traceUrl });
  }
Repository: microsoft/playwright
File: packages/playwright/src/mcp/terminal/program.ts (TypeScript)

Code before:
  browser?: string;
  config?: string;
  extension?: boolean;
  headed?: boolean;
  persistent?: boolean;
  profile?: string;
};

const globalOptions: (keyof (GlobalOptions & OpenOptions))[] = [
  'browser',
  'config',
  'extension',
  'headed',
  'help',
  'persistent',
  'profile',
  'session',
  'version',
];

export async function program(packageLocation: string) {
  const clientInfo = createClientInfo(packageLocation);
  const help = require('./help.json');

  const argv = process.argv.slice(2);
  const args: MinimistArgs = require('minimist')(argv, { boolean: [...help.booleanOptions, 'help', 'version'], string: [...help.stringOptions, '_'] });
  for (const option of help.booleanOptions) {
    if (!argv.includes(`--${option}`) && !argv.includes(`--no-${option}`))
      delete args[option];
  }
  // Normalize -s alias to --session
  if (args.s) {
    args.session = args.s;
    delete args.s;
  }

  const commandName = args._?.[0];

  if (args.version || args.v) {
    console.log(clientInfo.version);
    process.exit(0);
  }

  const command = commandName && 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);
    }

Review comment (suggestion):
I think we should still include `booleanOptions` and `globalOptions` that are cli-only, instead of relying upon them being listed in help.

Alternatively, we can expose those from help, but I'd prefer that to be a separate PR.

Code after:
  browser?: string;
  config?: string;
  extension?: boolean;
  headed?: boolean;
  persistent?: boolean;
  profile?: string;
};

const globalOptions: (keyof (GlobalOptions & OpenOptions))[] = [
  'browser',
  'config',
  'extension',
  'headed',
  'help',
  'persistent',
  'profile',
  'session',
  'version',
];

const booleanOptions: (keyof (GlobalOptions & OpenOptions & { all?: boolean }))[] = [
  'all',
  'help',
  'version',
  'extension',
  'headed',
  'persistent'
];

export async function program(packageLocation: string) {
  const clientInfo = createClientInfo(packageLocation);
  const help = require('./help.json');

  const argv = process.argv.slice(2);
  const boolean = [...help.booleanOptions, ...booleanOptions];
  const args: MinimistArgs = require('minimist')(argv, { boolean, string: [...help.stringOptions, '_'] });
  for (const option of boolean) {
    if (!argv.includes(`--${option}`) && !argv.includes(`--no-${option}`))
      delete args[option];
    if (argv.some(arg => arg.startsWith(`--${option}=`) || arg.startsWith(`--no-${option}=`))) {
      console.error(`boolean option '--${option}' should not be passed with '=value', use '--${option}' or '--no-${option}' instead`);
      process.exit(1);
    }
  }
  // Normalize -s alias to --session
  if (args.s) {
    args.session = args.s;
    delete args.s;
  }

  const commandName = args._?.[0];
Repository: microsoft/playwright
File: packages/injected/src/recorder/pollingRecorder.ts (TypeScript)

Code before:

    this._pollRecorderModeTimer = this._recorder.injectedScript.utils.builtins.setTimeout(() => this._pollRecorderMode(), pollPeriod);
  }

  async performAction(action: actions.PerformOnRecordAction) {
    await this._embedder.__pw_recorderPerformAction(action);
  }

  async recordAction(action: actions.Action): Promise<void> {
    await this._embedder.__pw_recorderRecordAction(action);
  }

  async elementPicked(elementInfo: ElementInfo): Promise<void> {
    await this._embedder.__pw_recorderElementPicked(elementInfo);
  }

  async setMode(mode: Mode): Promise<void> {
    await this._embedder.__pw_recorderSetMode(mode);
  }

  async setOverlayState(state: OverlayState): Promise<void> {
    await this._embedder.__pw_recorderSetOverlayState(state);
  }

  async log(...args: any[]) {
    await this._embedder.__pw_recorderLog(args);
  }
}

export default PollingRecorder;


Review comment (suggestion):
Let's check `this._recorder.injectedScript.isUnderTest` before sending to the embedder.

Code after:

    this._pollRecorderModeTimer = this._recorder.injectedScript.utils.builtins.setTimeout(() => this._pollRecorderMode(), pollPeriod);
  }

  async performAction(action: actions.PerformOnRecordAction) {
    await this._embedder.__pw_recorderPerformAction(action);
  }

  async recordAction(action: actions.Action): Promise<void> {
    await this._embedder.__pw_recorderRecordAction(action);
  }

  async elementPicked(elementInfo: ElementInfo): Promise<void> {
    await this._embedder.__pw_recorderElementPicked(elementInfo);
  }

  async setMode(mode: Mode): Promise<void> {
    await this._embedder.__pw_recorderSetMode(mode);
  }

  async setOverlayState(state: OverlayState): Promise<void> {
    await this._embedder.__pw_recorderSetOverlayState(state);
  }

  async log(...args: any[]) {
    if (this._recorder.injectedScript.isUnderTest)
      await this._embedder.__pw_recorderLog(args);
  }
}

export default PollingRecorder;
Repository: microsoft/playwright
File: tests/page/page-network-response.spec.ts (TypeScript)

Code before:
    // Don't end the response yet
  });

  const [request] = await Promise.all([
    page.waitForEvent('request'),
    server.waitForRequest('/get'),
    page.evaluate(() => { void fetch('./get', { method: 'GET' }); }),
  ]);

  // Response hasn't been received yet
  expect(request.existingResponse()).toBe(null);

  // Now send the response
  serverResponse.setHeader('Content-Type', 'text/plain; charset=utf-8');
  serverResponse.end('done');
  await page.waitForEvent('response');

  // After response is received, existingResponse should return the response
  const existingResponse = request.existingResponse();
  expect(existingResponse).not.toBe(null);
  expect(existingResponse.status()).toBe(200);
});

it('request.existingResponse should return the response after it is received', async ({ page, server }) => {
  const response = await page.goto(server.EMPTY_PAGE);
  const request = response.request();
  expect(request.existingResponse()).toBe(response);
});

it('should return http version', async ({ page, server }) => {
  const response = await page.goto(server.EMPTY_PAGE);
  expect(await response.httpVersion()).toBe('HTTP/1.1');
});


Review comment (bug):
Let's add a test for a failing request - `httpVersion()` should not hang in this case. Consult `Page.Events.RequestFailed` from `page-event-network.spec.ts` to see how to trigger a failing request.

Code after:
    page.waitForEvent('request'),
    server.waitForRequest('/get'),
    page.evaluate(() => { void fetch('./get', { method: 'GET' }); }),
  ]);

  // Response hasn't been received yet
  expect(request.existingResponse()).toBe(null);

  // Now send the response
  serverResponse.setHeader('Content-Type', 'text/plain; charset=utf-8');
  serverResponse.end('done');
  await page.waitForEvent('response');

  // After response is received, existingResponse should return the response
  const existingResponse = request.existingResponse();
  expect(existingResponse).not.toBe(null);
  expect(existingResponse.status()).toBe(200);
});

it('request.existingResponse should return the response after it is received', async ({ page, server }) => {
  const response = await page.goto(server.EMPTY_PAGE);
  const request = response.request();
  expect(request.existingResponse()).toBe(response);
});

it('should return http version', async ({ page, server }) => {
  const response = await page.goto(server.EMPTY_PAGE);
  expect(await response.httpVersion()).toBe('HTTP/1.1');
});