ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/jest
File: packages/jest-console/src/getConsoleOutput.ts (TypeScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import chalk from 'chalk';
import type {Config} from '@jest/types';
import {
  type StackTraceConfig,
  type StackTraceOptions,
  formatStackTrace,
} from 'jest-message-util';
import type {ConsoleBuffer} from './types';

export default function getConsoleOutput(
  buffer: ConsoleBuffer,
  config: StackTraceConfig,
  globalConfig: Config.GlobalConfig,
): string {
  const TITLE_INDENT =
    globalConfig?.verbose === true ? ' '.repeat(2) : ' '.repeat(4);
  const CONSOLE_INDENT = TITLE_INDENT + ' '.repeat(2);

  const logEntries = buffer.reduce((output, {type, message, origin}) => {
    message = message
      .split(/\n/)
      .map(line => CONSOLE_INDENT + line)
      .join('\n');

    let typeMessage = `console.${type}`;
    let noStackTrace = true;
    let noCodeFrame = true;

    if (type === 'warn') {
      message = chalk.yellow(message);
      typeMessage = chalk.yellow(typeMessage);
      noStackTrace = globalConfig?.noStackTrace ?? false;
      noCodeFrame = false;
    } else if (type === 'error') {
      message = chalk.red(message);
      typeMessage = chalk.red(typeMessage);
      noStackTrace = globalConfig?.noStackTrace ?? false;
      noCodeFrame = false;
    }

    const options: StackTraceOptions = {

Review comment (suggestion):
I have to add `verbose` nullish check for double assurance

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import chalk from 'chalk';
import type {Config} from '@jest/types';
import {
  type StackTraceConfig,
  type StackTraceOptions,
  formatStackTrace,
} from 'jest-message-util';
import type {ConsoleBuffer} from './types';

export default function getConsoleOutput(
  buffer: ConsoleBuffer,
  config: StackTraceConfig,
  globalConfig: Config.GlobalConfig,
): string {
  const TITLE_INDENT =
    globalConfig.verbose === true ? ' '.repeat(2) : ' '.repeat(4);
  const CONSOLE_INDENT = TITLE_INDENT + ' '.repeat(2);

  const logEntries = buffer.reduce((output, {type, message, origin}) => {
    message = message
      .split(/\n/)
      .map(line => CONSOLE_INDENT + line)
      .join('\n');

    let typeMessage = `console.${type}`;
    let noStackTrace = true;
    let noCodeFrame = true;

    if (type === 'warn') {
      message = chalk.yellow(message);
      typeMessage = chalk.yellow(typeMessage);
      noStackTrace = globalConfig?.noStackTrace ?? false;
      noCodeFrame = false;
    } else if (type === 'error') {
      message = chalk.red(message);
      typeMessage = chalk.red(typeMessage);
      noStackTrace = globalConfig?.noStackTrace ?? false;
      noCodeFrame = false;
    }

    const options: StackTraceOptions = {
Repository: facebook/jest
File: packages/jest-reporters/src/GitHubActionsReporter.ts (TypeScript)

Code before:
};

type ResultTreeNode = {
  name: string;
  passed: boolean;
  children: Array<ResultTreeNode | ResultTreeLeaf>;
};

type ResultTree = {
  children: Array<ResultTreeLeaf | ResultTreeNode>;
  name: string;
  passed: boolean;
  performanceInfo: PerformanceInfo;
};

export default class GitHubActionsReporter extends BaseReporter {
  static readonly filename = __filename;
  private readonly options: {silent: boolean};
  protected globalConfig: Config.GlobalConfig;

  constructor(
    _globalConfig: Config.GlobalConfig,
    reporterOptions: {silent?: boolean} = {},
  ) {
    super();
    this.globalConfig = _globalConfig;
    this.options = {
      silent:
        typeof reporterOptions.silent === 'boolean'
          ? reporterOptions.silent
          : true,
    };
  }

  override onTestResult(
    test: Test,
    testResult: TestResult,
    aggregatedResults: AggregatedResult,
  ): void {
    this.generateAnnotations(test, testResult);
    if (!this.options.silent) {
      this.printFullResult(test.context, testResult);
    }
    if (this.isLastTestSuite(aggregatedResults)) {
      this.printFailedTestLogs(test, testResult.console, aggregatedResults);
    }
  }

  private generateAnnotations(
    {context}: Test,
    {testResults}: TestResult,

Review comment (suggestion):
```suggestion
globalConfig: Config.GlobalConfig,
reporterOptions: {silent?: boolean} = {},
) {
super();
    this.globalConfig = globalConfig;
```

Code after:
};

type ResultTreeNode = {
  name: string;
  passed: boolean;
  children: Array<ResultTreeNode | ResultTreeLeaf>;
};

type ResultTree = {
  children: Array<ResultTreeLeaf | ResultTreeNode>;
  name: string;
  passed: boolean;
  performanceInfo: PerformanceInfo;
};

export default class GitHubActionsReporter extends BaseReporter {
  static readonly filename = __filename;
  private readonly options: {silent: boolean};
  private readonly globalConfig: Config.GlobalConfig;

  constructor(
    globalConfig: Config.GlobalConfig,
    reporterOptions: {silent?: boolean} = {},
  ) {
    super();
    this.globalConfig = globalConfig;
    this.options = {
      silent:
        typeof reporterOptions.silent === 'boolean'
          ? reporterOptions.silent
          : true,
    };
  }

  override onTestResult(
    test: Test,
    testResult: TestResult,
    aggregatedResults: AggregatedResult,
  ): void {
    this.generateAnnotations(test, testResult);
    if (!this.options.silent) {
      this.printFullResult(test.context, testResult);
    }
    if (this.isLastTestSuite(aggregatedResults)) {
      this.printFailedTestLogs(test, testResult.console, aggregatedResults);
    }
  }

  private generateAnnotations(
    {context}: Test,
    {testResults}: TestResult,
Repository: facebook/jest
File: packages/jest-reporters/src/GitHubActionsReporter.ts (TypeScript)

Code before:
};

type ResultTreeLeaf = {
  name: string;
  status: Status;
  duration: number;
  children: Array<never>;
};

type ResultTreeNode = {
  name: string;
  passed: boolean;
  children: Array<ResultTreeNode | ResultTreeLeaf>;
};

type ResultTree = {
  children: Array<ResultTreeLeaf | ResultTreeNode>;
  name: string;
  passed: boolean;
  performanceInfo: PerformanceInfo;
};

export default class GitHubActionsReporter extends BaseReporter {
  static readonly filename = __filename;
  private readonly options: {silent: boolean};
  protected globalConfig: Config.GlobalConfig;

  constructor(
    _globalConfig: Config.GlobalConfig,
    reporterOptions: {silent?: boolean} = {},
  ) {
    super();
    this.globalConfig = _globalConfig;
    this.options = {
      silent:
        typeof reporterOptions.silent === 'boolean'
          ? reporterOptions.silent
          : true,
    };
  }

  override onTestResult(
    test: Test,
    testResult: TestResult,
    aggregatedResults: AggregatedResult,
  ): void {
    this.generateAnnotations(test, testResult);
    if (!this.options.silent) {
      this.printFullResult(test.context, testResult);
    }
    if (this.isLastTestSuite(aggregatedResults)) {

Review comment (suggestion):
```suggestion
  private readonly globalConfig: Config.GlobalConfig;
```

Code after:
};

type ResultTreeLeaf = {
  name: string;
  status: Status;
  duration: number;
  children: Array<never>;
};

type ResultTreeNode = {
  name: string;
  passed: boolean;
  children: Array<ResultTreeNode | ResultTreeLeaf>;
};

type ResultTree = {
  children: Array<ResultTreeLeaf | ResultTreeNode>;
  name: string;
  passed: boolean;
  performanceInfo: PerformanceInfo;
};

export default class GitHubActionsReporter extends BaseReporter {
  static readonly filename = __filename;
  private readonly options: {silent: boolean};
  private readonly globalConfig: Config.GlobalConfig;

  constructor(
    globalConfig: Config.GlobalConfig,
    reporterOptions: {silent?: boolean} = {},
  ) {
    super();
    this.globalConfig = globalConfig;
    this.options = {
      silent:
        typeof reporterOptions.silent === 'boolean'
          ? reporterOptions.silent
          : true,
    };
  }

  override onTestResult(
    test: Test,
    testResult: TestResult,
    aggregatedResults: AggregatedResult,
  ): void {
    this.generateAnnotations(test, testResult);
    if (!this.options.silent) {
      this.printFullResult(test.context, testResult);
    }
    if (this.isLastTestSuite(aggregatedResults)) {
Repository: facebook/react
File: src/test/all.js (JavaScript)

Code before:
// This file exists both to give a single entry point for all the utility
// modules in src/test and to specify an ordering on those modules, since
// some still have implicit dependencies on others.

require("./phantom");
require("./console");
require("ReactTestUtils");
require("reactComponentExpect");
require("./diff");
require("./PrintReporter");
require("./HtmlReporter");
require("./ReporterView");
require("./SpecView");
require("./SuiteView");
require("./jasmine-support");
require("mocks");
require("mock-modules");
require("./mock-timers");


Review comment (suggestion):
ternary across this many lines is _groosssss_. Can you just use `if`?

What about just using the shim @ https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind - this here is not quite the same.


Code after:
// This file exists both to give a single entry point for all the utility
// modules in src/test and to specify an ordering on those modules, since
// some still have implicit dependencies on others.

var Ap = Array.prototype;
var slice = Ap.slice;
var Fp = Function.prototype;

if (!Fp.bind) {
  // PhantomJS doesn't support Function.prototype.bind natively, so
  // polyfill it whenever this module is required.
  Fp.bind = function(context) {
    var func = this;
    var args = slice.call(arguments, 1);
    var bound;

    if (func.prototype) {
      if (args.length > 0) {
        bound = function() {
          return func.apply(
            !(this instanceof func) && context || this,
            args.concat(slice.call(arguments))
          );
        };
      } else {
        bound = function() {
          return func.apply(
            !(this instanceof func) && context || this,
            arguments
          );
        };
      }

      bound.prototype = Object.create(func.prototype);

    } else if (args.length > 0) {
      bound = function() {
        return func.apply(
          context || this,
          args.concat(slice.call(arguments))
        );
      };
    } else {
      bound = function() {
        return func.apply(context || this, arguments);
      };
    }

    return bound;
  };
}

require("ReactTestUtils");
require("reactComponentExpect");
require("mocks");
require("mock-modules");
require("./mock-timers");
Repository: facebook/react
File: src/test/all.js (JavaScript)

Code before:
// This file exists both to give a single entry point for all the utility
// modules in src/test and to specify an ordering on those modules, since
// some still have implicit dependencies on others.

require("./phantom");
require("./console");
require("ReactTestUtils");
require("reactComponentExpect");
require("./diff");
require("./PrintReporter");
require("./HtmlReporter");
require("./ReporterView");
require("./SpecView");
require("./SuiteView");
require("./jasmine-support");
require("mocks");
require("mock-modules");
require("./mock-timers");


Review comment (suggestion):
Sure, sure.

For what it's worth, the `static_upstream` polyfill doesn't bother with constructor function binding, either. Seems like a nontrivial cost to pay for a minority use case.


Code after:
// This file exists both to give a single entry point for all the utility
// modules in src/test and to specify an ordering on those modules, since
// some still have implicit dependencies on others.

var Ap = Array.prototype;
var slice = Ap.slice;
var Fp = Function.prototype;

if (!Fp.bind) {
  // PhantomJS doesn't support Function.prototype.bind natively, so
  // polyfill it whenever this module is required.
  Fp.bind = function(context) {
    var func = this;
    var args = slice.call(arguments, 1);
    var bound;

    if (func.prototype) {
      if (args.length > 0) {
        bound = function() {
          return func.apply(
            !(this instanceof func) && context || this,
            args.concat(slice.call(arguments))
          );
        };
      } else {
        bound = function() {
          return func.apply(
            !(this instanceof func) && context || this,
            arguments
          );
        };
      }

      bound.prototype = Object.create(func.prototype);

    } else if (args.length > 0) {
      bound = function() {
        return func.apply(
          context || this,
          args.concat(slice.call(arguments))
        );
      };
    } else {
      bound = function() {
        return func.apply(context || this, arguments);
      };
    }

    return bound;
  };
}

require("ReactTestUtils");
require("reactComponentExpect");
require("mocks");
require("mock-modules");
require("./mock-timers");
Repository: facebook/react
File: packages/react-devtools-shared/src/hook.js (JavaScript)

Code before:
    const isUnmounting =
      current.memoizedState == null || current.memoizedState.element == null;

    // Keep track of mounted roots so we can hydrate when DevTools connect.
    if (!isKnownRoot && !isUnmounting) {
      mountedRoots.add(root);
    } else if (isKnownRoot && isUnmounting) {
      mountedRoots.delete(root);
    }
    const rendererInterface = rendererInterfaces.get(rendererID);
    if (rendererInterface != null) {
      rendererInterface.handleCommitFiberRoot(root, priorityLevel);
    }
  }

  // TODO: More meaningful names for "rendererInterfaces" and "renderers".
  const fiberRoots = {};
  const rendererInterfaces = new Map();
  const listeners = {};
  const renderers = new Map();

  let hook: DevToolsHook;
  if (canAccessParentWindow(target) && !isMainWindow(target)) {
    hook = getMainWindow(target).__REACT_DEVTOOLS_GLOBAL_HOOK__;
  }
  if (!hook) {
    hook = {
      rendererInterfaces,
      listeners,

      // Fast Refresh for web relies on this.
      renderers,

      emit,
      getFiberRoots,
      inject,
      on,
      off,
      sub,

      // This is a legacy flag.
      // React v16 checks the hook for this to ensure DevTools is new enough.
      supportsFiber: true,

      // React calls these methods.
      checkDCE,
      onCommitFiberUnmount,
      onCommitFiberRoot,
    };
  }
  Object.defineProperty(

Review comment (suggestion):
I handle now  the case if we don't have `__REACT_DEVTOOLS_GLOBAL_HOOK__` in the parent window

Code after:
    const isUnmounting =
      current.memoizedState == null || current.memoizedState.element == null;

    // Keep track of mounted roots so we can hydrate when DevTools connect.
    if (!isKnownRoot && !isUnmounting) {
      mountedRoots.add(root);
    } else if (isKnownRoot && isUnmounting) {
      mountedRoots.delete(root);
    }
    const rendererInterface = rendererInterfaces.get(rendererID);
    if (rendererInterface != null) {
      rendererInterface.handleCommitFiberRoot(root, priorityLevel);
    }
  }

  // TODO: More meaningful names for "rendererInterfaces" and "renderers".
  const fiberRoots = {};
  const rendererInterfaces = new Map();
  const listeners = {};
  const renderers = new Map();

  let hook: DevToolsHook = getMainWindow(target).__REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (!hook) {
    hook = {
      rendererInterfaces,
      listeners,

      // Fast Refresh for web relies on this.
      renderers,

      emit,
      getFiberRoots,
      inject,
      on,
      off,
      sub,

      // This is a legacy flag.
      // React v16 checks the hook for this to ensure DevTools is new enough.
      supportsFiber: true,

      // React calls these methods.
      checkDCE,
      onCommitFiberUnmount,
      onCommitFiberRoot,
    };
  }
  Object.defineProperty(
    target,
    '__REACT_DEVTOOLS_GLOBAL_HOOK__',
    ({
Repository: facebook/react
File: packages/react-server/src/ReactFlightServer.js (JavaScript)

Code before:

type ReactModelObject = {+[key: string]: ReactModel};

type Segment = {
  id: number,
  model: ReactModel,
  ping: () => void,
  context: ContextSnapshot,
};

export type Request = {
  status: 0 | 1 | 2,
  fatalError: mixed,
  destination: null | Destination,
  bundlerConfig: BundlerConfig,
  cache: Map<Function, mixed>,
  nextChunkId: number,
  pendingChunks: number,
  pingedSegments: Array<Segment>,
  completedModuleChunks: Array<Chunk>,
  completedJSONChunks: Array<Chunk>,
  completedErrorChunks: Array<Chunk>,
  writtenSymbols: Map<Symbol, number>,
  writtenModules: Map<ModuleKey, number>,
  writtenProviders: Map<string, number>,
  identifierPrefix?: string,
  identifierCount: number,
  onError: (error: mixed) => void,
  toJSON: (key: string, value: ReactModel) => ReactJSONValue,
};

const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher;

function defaultErrorHandler(error: mixed) {
  console['error'](error);
  // Don't transform to our wrapper
}

const OPEN = 0;
const CLOSING = 1;
const CLOSED = 2;

export function createRequest(
  model: ReactModel,
  bundlerConfig: BundlerConfig,
  onError: void | ((error: mixed) => void),
  context?: Array<[string, ServerContextJSONValue]>,
  identifierPrefix?: string,
): Request {
  const pingedSegments = [];
  const request = {

Review comment (suggestion):
We should never have optional properties in internal data structures since we want consistent hidden classes. Only short lived and rare config objects and such should have those.

This should just be `string` always and set to empty string when you initialize it if no option is passed.

Code after:

type ReactModelObject = {+[key: string]: ReactModel};

type Segment = {
  id: number,
  model: ReactModel,
  ping: () => void,
  context: ContextSnapshot,
};

export type Request = {
  status: 0 | 1 | 2,
  fatalError: mixed,
  destination: null | Destination,
  bundlerConfig: BundlerConfig,
  cache: Map<Function, mixed>,
  nextChunkId: number,
  pendingChunks: number,
  pingedSegments: Array<Segment>,
  completedModuleChunks: Array<Chunk>,
  completedJSONChunks: Array<Chunk>,
  completedErrorChunks: Array<Chunk>,
  writtenSymbols: Map<Symbol, number>,
  writtenModules: Map<ModuleKey, number>,
  writtenProviders: Map<string, number>,
  identifierPrefix: string,
  identifierCount: number,
  onError: (error: mixed) => void,
  toJSON: (key: string, value: ReactModel) => ReactJSONValue,
};

const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher;

function defaultErrorHandler(error: mixed) {
  console['error'](error);
  // Don't transform to our wrapper
}

const OPEN = 0;
const CLOSING = 1;
const CLOSED = 2;

export function createRequest(
  model: ReactModel,
  bundlerConfig: BundlerConfig,
  onError: void | ((error: mixed) => void),
  context?: Array<[string, ServerContextJSONValue]>,
  identifierPrefix?: string,
): Request {
  const pingedSegments = [];
  const request = {
Repository: facebook/react
File: packages/react-reconciler/src/ReactCapturedValue.js (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import type {Fiber} from './ReactInternalTypes';

import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack';

export type CapturedValue<T> = {
  value: T,
  source: Fiber | null,
  stack: string | null,
  digest: string | null,
};

export function createCapturedValueAtFiber<T>(
  value: T,
  source: Fiber,
): CapturedValue<T> {
  // If the value is an error, call this function immediately after it is thrown
  // so the stack is accurate.
  return {
    value,
    source,
    stack: getStackByFiberInDevAndProd(source),
    digest: null,
  };
}

export function createCapturedValue<T>(
  value: T,
  digest: ?string,
  stack: ?string,
): CapturedValue<T> {
  return {
    value,
    source: null,
    stack: stack != null ? stack : null,
    digest: digest != null ? digest : null,
  };
}


Review comment (suggestion):
this was already being used on to construct add hoc captured values for the recoverable error pathway. It doesn't need value generality and this helps us avoid type checking the value before deciding if the stack needs to go in the map

Code after:
): CapturedValue<T> {
  // If the value is an error, call this function immediately after it is thrown
  // so the stack is accurate.
  let stack;
  if (typeof value === 'object' && value !== null) {
    const capturedStack = CapturedStacks.get(value);
    if (typeof capturedStack === 'string') {
      stack = capturedStack;
    } else {
      stack = getStackByFiberInDevAndProd(source);
      CapturedStacks.set(value, stack);
    }
  } else {
    stack = getStackByFiberInDevAndProd(source);
  }

  return {
    value,
    source,
    stack,
    digest: null,
  };
}

export function createCapturedValueFromError(
  value: Error,
  digest: ?string,
  stack: ?string,
): CapturedValue<Error> {
  if (typeof stack === 'string') {
    CapturedStacks.set(value, stack);
  }
  return {
    value,
    source: null,
    stack: stack != null ? stack : null,
    digest: digest != null ? digest : null,
  };
}
Repository: facebook/react
File: packages/react-reconciler/src/ReactCapturedValue.js (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import type {Fiber} from './ReactInternalTypes';

import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack';

export type CapturedValue<T> = {
  value: T,
  source: Fiber | null,
  stack: string | null,
  digest: string | null,
};

export function createCapturedValueAtFiber<T>(
  value: T,
  source: Fiber,
): CapturedValue<T> {
  // If the value is an error, call this function immediately after it is thrown
  // so the stack is accurate.
  return {
    value,
    source,
    stack: getStackByFiberInDevAndProd(source),
    digest: null,
  };
}

export function createCapturedValue<T>(
  value: T,
  digest: ?string,
  stack: ?string,
): CapturedValue<T> {
  return {
    value,
    source: null,
    stack: stack != null ? stack : null,
    digest: digest != null ? digest : null,
  };
}


Review comment (suggestion):
The way this is used today we don't really need to capture the stack in the map because these errors aren't bubbling up during a render. However it could be used in other ways in the future so to keep it correct I'm stashing the stack here if it is provided.

Code after:
    const capturedStack = CapturedStacks.get(value);
    if (typeof capturedStack === 'string') {
      stack = capturedStack;
    } else {
      stack = getStackByFiberInDevAndProd(source);
      CapturedStacks.set(value, stack);
    }
  } else {
    stack = getStackByFiberInDevAndProd(source);
  }

  return {
    value,
    source,
    stack,
    digest: null,
  };
}

export function createCapturedValueFromError(
  value: Error,
  digest: ?string,
  stack: ?string,
): CapturedValue<Error> {
  if (typeof stack === 'string') {
    CapturedStacks.set(value, stack);
  }
  return {
    value,
    source: null,
    stack: stack != null ? stack : null,
    digest: digest != null ? digest : null,
  };
}