ronantakizawa/codereview-bench huggingface dataset:
Repository: mui/material-ui
File: scripts/build.js (JavaScript)

Code before:
    babelConfigPath,
    '--extensions',
    '".js,.ts"',
    srcDir,
    '--out-dir',
    outDir,
    '--ignore',
    '"**/*.test.js","**/*.spec.ts","**/*.d.ts"',
  ].join(' ');

  if (verbose) {
    // eslint-disable-next-line no-console
    console.log(`running '${command}' with ${JSON.stringify(env)}`);
  }

  return exec(command, { env });
}

yargs
  .command({
    command: '$0 <bundle>',
    description: 'build package',
    builder: (command) => {
      return command
        .positional('bundle', {
          description: '"cjs" | "esm" | "esnext"',
          type: 'string',
        })
        .option('out-dir', { default: './build', type: 'string' })
        .option('verbose', { type: 'boolean' });
    },
    handler: run,
  })
  .help()
  .strict(true)
  .version(false)
  .parse();


Review comment (suggestion):
```suggestion
          description: '"cjs" | "esm" | "es"',
```

Code after:
  };
  const babelConfigPath = path.resolve(__dirname, '../babel.config.js');
  const srcDir = path.resolve('./src');
  const outDir = path.resolve(
    relativeOutDir,
    {
      cjs: '.',
      esm: './esm',
      es: './es',
    }[bundle],
  );

  const command = [
    'yarn babel',
    '--config-file',
    babelConfigPath,
    '--extensions',
    '".js,.ts"',
    srcDir,
    '--out-dir',
    outDir,
    '--ignore',
    '"**/*.test.js","**/*.spec.ts","**/*.d.ts"',
  ].join(' ');

  if (verbose) {
    // eslint-disable-next-line no-console
    console.log(`running '${command}' with ${JSON.stringify(env)}`);
  }

  return exec(command, { env });
}

yargs
  .command({
    command: '$0 <bundle>',
    description: 'build package',
    builder: (command) => {
      return command
        .positional('bundle', {
          description: `Valid bundles: "${validBundles.join('" | "')}"`,
          type: 'string',
        })
        .option('out-dir', { default: './build', type: 'string' })
        .option('verbose', { type: 'boolean' });
    },
    handler: run,
  })
  .help()
  .strict(true)
  .version(false)
Repository: mui/material-ui
File: packages/material-ui/src/ButtonBase/ButtonBase.js (JavaScript)

Code before:
        rippleRef.current[rippleAction](event);
      }

      return true;
    });
  }

  const handleMouseDown = useRippleHandler('start', onMouseDown);
  const handleDragLeave = useRippleHandler('stop', onDragLeave);
  const handleMouseUp = useRippleHandler('stop', onMouseUp);
  const handleMouseLeave = useRippleHandler('stop', (event) => {
    if (focusVisible) {
      event.preventDefault();
    }
    if (onMouseLeave) {
      onMouseLeave(event);
    }
  });
  const handleTouchStart = useRippleHandler('start', onTouchStart);
  const handleTouchEnd = useRippleHandler('stop', onTouchEnd);
  const handleTouchMove = useRippleHandler('stop', onTouchMove);
  const handleBlur = useRippleHandler(
    'stop',
    (event) => {
      if (focusVisible) {
        onBlurVisible();
        setFocusVisible(false);
      }
      if (onBlur) {
        onBlur(event);
      }
    },
    false,
  );

  const handleFocus = useEventCallback((event) => {
    // Fix for https://github.com/facebook/react/issues/7769
    if (!buttonRef.current) {
      buttonRef.current = event.currentTarget;
    }

    if (isFocusVisible(event)) {
      setFocusVisible(true);

      if (onFocusVisible) {
        onFocusVisible(event);
      }
    }

    if (onFocus) {
      onFocus(event);

Review comment (suggestion):
This is currently not needed but will be in the future with React 17. `onBlurVisible` already has a notice so this shouldn't be changed.

Code after:

      const ignore = skipRippleAction;
      if (!ignore && rippleRef.current) {
        rippleRef.current[rippleAction](event);
      }

      return true;
    });
  }

  const handleMouseDown = useRippleHandler('start', onMouseDown);
  const handleDragLeave = useRippleHandler('stop', onDragLeave);
  const handleMouseUp = useRippleHandler('stop', onMouseUp);
  const handleMouseLeave = useRippleHandler('stop', (event) => {
    if (focusVisible) {
      event.preventDefault();
    }
    if (onMouseLeave) {
      onMouseLeave(event);
    }
  });
  const handleTouchStart = useRippleHandler('start', onTouchStart);
  const handleTouchEnd = useRippleHandler('stop', onTouchEnd);
  const handleTouchMove = useRippleHandler('stop', onTouchMove);

  const handleBlur = useRippleHandler(
    'stop',
    (event) => {
      handleBlurVisible(event);
      if (isFocusVisibleRef.current === false) {
        setFocusVisible(false);
      }
      if (onBlur) {
        onBlur(event);
      }
    },
    false,
  );

  const handleFocus = useEventCallback((event) => {
    // Fix for https://github.com/facebook/react/issues/7769
    if (!buttonRef.current) {
      buttonRef.current = event.currentTarget;
    }

    handleFocusVisible(event);
    if (isFocusVisibleRef.current === true) {
      setFocusVisible(true);

      if (onFocusVisible) {
        onFocusVisible(event);
Repository: mui/material-ui
File: packages/material-ui/src/ListItem/ListItem.spec.tsx (TypeScript)

Code before:
function BooleanButtonTest() {
  // https://github.com/mui-org/material-ui/issues/14971

  function EditableItemFail(props: { editable: boolean }) {
    const { editable } = props;
    // @ts-expect-error 'boolean' is not assignable to type 'true'
    return <ListItem button={editable}>Editable? {editable}</ListItem>;
  }

  function EditableItemValid(props: { editable: boolean }) {
    const { editable } = props;
    if (editable) {
      <ListItem button>Editable? Yes</ListItem>;
    }
    return <ListItem>Editable? No</ListItem>;
  }
}

// verify that https://github.com/mui-org/material-ui/issues/19756 already worked.
function MouseEnterTest() {
  function handleMouseEnter(event: React.MouseEvent<HTMLLIElement>) {}
  <ListItem onMouseEnter={handleMouseEnter} />;

  function handleMouseEnterButton(event: React.MouseEvent<HTMLDivElement>) {}
  // @ts-expect-error
  <ListItem onMouseEnter={handleMouseEnterButton} />; // desired: missing prop button
  <ListItem button onMouseEnter={handleMouseEnterButton} />;
}


Review comment (suggestion):
This is a TypeScript error which does name it `property`. It should only change if the underlying TS error message changed.

Code after:
function BooleanButtonTest() {
  // https://github.com/mui-org/material-ui/issues/14971

  function EditableItemFail(props: { editable: boolean }) {
    const { editable } = props;
    // @ts-expect-error 'boolean' is not assignable to type 'true'
    return <ListItem button={editable}>Editable? {editable}</ListItem>;
  }

  function EditableItemValid(props: { editable: boolean }) {
    const { editable } = props;
    if (editable) {
      <ListItem button>Editable? Yes</ListItem>;
    }
    return <ListItem>Editable? No</ListItem>;
  }
}

// verify that https://github.com/mui-org/material-ui/issues/19756 already worked.
function MouseEnterTest() {
  function handleMouseEnter(event: React.MouseEvent<HTMLLIElement>) {}
  <ListItem onMouseEnter={handleMouseEnter} />;

  function handleMouseEnterButton(event: React.MouseEvent<HTMLDivElement>) {}
  // @ts-expect-error
  <ListItem onMouseEnter={handleMouseEnterButton} />; // desired: missing property button
  <ListItem button onMouseEnter={handleMouseEnterButton} />;
}
Repository: mui/material-ui
File: packages/material-ui-utils/src/HTMLElementType.ts (TypeScript)

Code before:
export default function HTMLElementType(
  props: { [key: string]: any },
  propName: string,
  componentName: string,
  location: string,
  propFullName: string,
) {
  if (process.env.NODE_ENV === 'production') {
    return null;
  }

  const propValue = props[propName];
  const safePropName = propFullName || propName;

  if (propValue == null) {
    return null;
  }

  if (propValue && propValue.nodeType !== 1) {
    return new Error(
      `Invalid ${location} \`${safePropName}\` supplied to \`${componentName}\`. ` +
        `Expected an HTMLElement.`,
    );
  }

  return null;
}

Review comment (suggestion):
Could this work with the safer `unknown`?
```suggestion
  props: { [key: string]: unknown },
```

Code after:
export default function HTMLElementType(
  props: { [key: string]: unknown },
  propName: string,
  componentName: string,
  location: string,
  propFullName: string,
): Error | null {
  if (process.env.NODE_ENV === 'production') {
    return null;
  }

  const propValue = props[propName];
  const safePropName = propFullName || propName;

  if (propValue == null) {
    return null;
  }

  if (propValue && (propValue as any).nodeType !== 1) {
    return new Error(
      `Invalid ${location} \`${safePropName}\` supplied to \`${componentName}\`. ` +
        `Expected an HTMLElement.`,
    );
  }

  return null;
}
Repository: mui/material-ui
File: packages/material-ui-utils/src/chainPropTypes.ts (TypeScript)

Code before:
import * as PropTypes from 'prop-types';

export default function chainPropTypes(
  propType1: PropTypes.Validator<unknown>,
  propType2: PropTypes.Validator<unknown>,
): PropTypes.Requireable<unknown> {
  if (process.env.NODE_ENV === 'production') {
    // @ts-ignore
    return () => null;
  }

  // @ts-ignore
  return function validate(...args) {
    return propType1(...args) || propType2(...args);
  };
}


Review comment (bug):
This should not fail type-checking. Otherwise the production build is unsound which is far more important than the development build being sound.

Code after:
import * as PropTypes from 'prop-types';

export default function chainPropTypes(
  propType1: PropTypes.Validator<unknown>,
  propType2: PropTypes.Validator<unknown>,
): PropTypes.Validator<unknown> {
  if (process.env.NODE_ENV === 'production') {
    return () => null;
  }

  return function validate(...args) {
    return propType1(...args) || propType2(...args);
  };
}
Repository: mui/material-ui
File: packages/material-ui-utils/src/deepmerge.ts (TypeScript)

Code before:
export function isPlainObject(item: any) {
  return item && typeof item === 'object' && item.constructor === Object;
}

export interface DeepmergeOptions {
  clone?: boolean;
}

export default function deepmerge<T>(
  target: Partial<T>,
  source: Partial<T>,
  options: DeepmergeOptions = { clone: true },
) {
  const output = options.clone ? { ...target } : target;

  if (isPlainObject(target) && isPlainObject(source)) {
    Object.keys(source).forEach((key) => {
      // Avoid prototype pollution
      if (key === '__proto__') {
        return;
      }

      // @ts-ignore
      if (isPlainObject(source[key]) && key in target) {
        // @ts-ignore
        output[key] = deepmerge(target[key], source[key], options);

Review comment (suggestion):
```suggestion
export function isPlainObject(item: unknown) {
```

Code after:
export function isPlainObject(item: unknown): item is Record<keyof any, unknown> {
  return item && typeof item === 'object' && item.constructor === Object;
}

export interface DeepmergeOptions {
  clone?: boolean;
}

export default function deepmerge<T>(
  target: T,
  source: unknown,
  options: DeepmergeOptions = { clone: true },
): T {
  const output = options.clone ? { ...target } : target;

  if (isPlainObject(target) && isPlainObject(source)) {
    Object.keys(source).forEach((key) => {
      // Avoid prototype pollution
      if (key === '__proto__') {
        return;
      }

      if (isPlainObject(source[key]) && key in target) {
        // Since `output` is a clone of `target` and we have narrowed `target` in this block we can cast to the same type.
        (output as Record<keyof any, unknown>)[key] = deepmerge(target[key], source[key], options);
      } else {
Repository: mui/material-ui
File: packages/material-ui-utils/src/deepmerge.ts (TypeScript)

Code before:

export interface DeepmergeOptions {
  clone?: boolean;
}

export default function deepmerge<T>(
  target: Partial<T>,
  source: Partial<T>,
  options: DeepmergeOptions = { clone: true },
) {
  const output = options.clone ? { ...target } : target;

  if (isPlainObject(target) && isPlainObject(source)) {
    Object.keys(source).forEach((key) => {
      // Avoid prototype pollution
      if (key === '__proto__') {
        return;
      }

      // @ts-ignore
      if (isPlainObject(source[key]) && key in target) {
        // @ts-ignore
        output[key] = deepmerge(target[key], source[key], options);
      } else {
        // @ts-ignore
        output[key] = source[key];
      }
    });
  }

  return output;
}


Review comment (suggestion):
Try to avoid `@ts-ignore` since we can't see what the issue is. Rather do some unsafe type casting so that at least human can verify the assumptions visually. 

Code after:

export interface DeepmergeOptions {
  clone?: boolean;
}

export default function deepmerge<T>(
  target: T,
  source: unknown,
  options: DeepmergeOptions = { clone: true },
): T {
  const output = options.clone ? { ...target } : target;

  if (isPlainObject(target) && isPlainObject(source)) {
    Object.keys(source).forEach((key) => {
      // Avoid prototype pollution
      if (key === '__proto__') {
        return;
      }

      if (isPlainObject(source[key]) && key in target) {
        // Since `output` is a clone of `target` and we have narrowed `target` in this block we can cast to the same type.
        (output as Record<keyof any, unknown>)[key] = deepmerge(target[key], source[key], options);
      } else {
        (output as Record<keyof any, unknown>)[key] = source[key];
      }
    });
  }

  return output;
}
Repository: mui/material-ui
File: packages/material-ui-utils/src/elementAcceptingRef.ts (TypeScript)

Code before:
import PropTypes from 'prop-types';
import chainPropTypes from './chainPropTypes';

function isClassComponent(elementType: any) {
  // elementType.prototype?.isReactComponent
  const { prototype = {} } = elementType;

  return Boolean(prototype.isReactComponent);
}

function acceptingRef(
  props: { [key: string]: any },
  propName: string,
  componentName: string,
  location: string,
  propFullName: string,
) {
  const element = props[propName];
  const safePropName = propFullName || propName;

  if (element == null) {
    return null;
  }

  let warningHint;

  const elementType = element.type;
  /**
   * Blacklisting instead of whitelisting
   *
   * Blacklisting will miss some components, such as React.Fragment. Those will at least
   * trigger a warning in React.
   * We can't whitelist because there is no safe way to detect React.forwardRef
   * or class components. "Safe" means there's no public API.
   *
   */
  if (typeof elementType === 'function' && !isClassComponent(elementType)) {

Review comment (suggestion):
```suggestion
  props: { [key: string]: unknown },
```

Code after:
import PropTypes from 'prop-types';
import chainPropTypes from './chainPropTypes';

function isClassComponent(elementType: Function) {
  // elementType.prototype?.isReactComponent
  const { prototype = {} } = elementType;

  return Boolean(prototype.isReactComponent);
}

function acceptingRef(
  props: { [key: string]: unknown },
  propName: string,
  componentName: string,
  location: string,
  propFullName: string,
) {
  const element = props[propName];
  const safePropName = propFullName || propName;

  if (element == null) {
    return null;
  }

  let warningHint;

  const elementType: unknown = (element as any).type;
  /**
   * Blacklisting instead of whitelisting
   *
   * Blacklisting will miss some components, such as React.Fragment. Those will at least
   * trigger a warning in React.
   * We can't whitelist because there is no safe way to detect React.forwardRef
   * or class components. "Safe" means there's no public API.
   *
   */
  if (typeof elementType === 'function' && !isClassComponent(elementType)) {
Repository: mui/material-ui
File: packages/material-ui-utils/src/elementTypeAcceptingRef.test.tsx (TypeScript)

Code before:
          return null;
        }
      }

      assertPass(Component);
    });

    it('accepts pure class components', () => {
      class Component extends React.PureComponent {
        render() {
          return null;
        }
      }

      assertPass(Component);
    });

    it('accepts forwardRef', () => {
      const Component = React.forwardRef(() => null);

      assertPass(Component);
    });

    it('accepts memo', () => {
      // @ts-ignore
      const Component = React.memo('div');

      assertPass(Component);
    });

    it('accepts lazy', () => {
      const Component = React.lazy(() =>
        // @ts-ignore
        Promise.resolve({ default: (props) => <div {...props} /> }),
      );

      // should actually fail when mounting since the ref is forwarded to a function component
      // but since this happens in a promise our consoleErrorMock doesn't catch it properly
      assertPass(Component);
    });

    it('technically allows other exotics like strict mode', () => {
      assertPass(React.StrictMode);
    });

    // undesired behavior
    it('accepts Fragment', () => {
      assertPass(React.Fragment, { failsOnMount: true });
    });
  });


Review comment (suggestion):
```suggestion
      const Component = React.memo(() => null);
```

memoizing a host component doesn't make sense in the first place.

Code after:
          return null;
        }
      }

      assertPass(Component);
    });

    it('accepts pure class components', () => {
      class Component extends React.PureComponent {
        render() {
          return null;
        }
      }

      assertPass(Component);
    });

    it('accepts forwardRef', () => {
      const Component = React.forwardRef(() => null);

      assertPass(Component);
    });

    it('accepts memo', () => {
      const Component = React.memo(React.forwardRef(() => null));

      assertPass(Component);
    });

    it('accepts lazy', () => {
      const Component = React.lazy(() =>
        Promise.resolve({ default: (props: any) => <div {...props} /> }),
      );

      // should actually fail when mounting since the ref is forwarded to a function component
      // but since this happens in a promise our consoleErrorMock doesn't catch it properly
      assertPass(Component);
    });

    it('technically allows other exotics like strict mode', () => {
      assertPass(React.StrictMode);
    });

    // undesired behavior
    it('accepts Fragment', () => {
      assertPass(React.Fragment, { failsOnMount: true });
    });
  });

  describe('rejections', () => {
    function assertFail(Component: any, hint: string) {
Repository: mui/material-ui
File: packages/material-ui-utils/src/elementTypeAcceptingRef.ts (TypeScript)

Code before:
import PropTypes from 'prop-types';
import chainPropTypes from './chainPropTypes';

function isClassComponent(elementType: any) {
  // elementType.prototype?.isReactComponent
  const { prototype = {} } = elementType;

  return Boolean(prototype.isReactComponent);
}

function elementTypeAcceptingRef(
  props: { [key: string]: any },
  propName: string,
  componentName: string,
  location: string,
  propFullName: string,
) {
  const propValue = props[propName];
  const safePropName = propFullName || propName;

  if (propValue == null) {
    return null;
  }

  let warningHint;

  /**
   * Blacklisting instead of whitelisting
   *

Review comment (suggestion):
```suggestion
function isClassComponent(elementType: { prototype?: Record<keyof any, any> }): boolean {
```

Code after:
import PropTypes from 'prop-types';
import chainPropTypes from './chainPropTypes';

function isClassComponent(elementType: Function) {
  // elementType.prototype?.isReactComponent
  const { prototype } = elementType;

  return Boolean(prototype.isReactComponent);
}

function elementTypeAcceptingRef(
  props: { [key: string]: unknown },
  propName: string,
  componentName: string,
  location: string,
  propFullName: string,
) {
  const propValue = props[propName];
  const safePropName = propFullName || propName;

  if (propValue == null) {
    return null;
  }

  let warningHint;

  /**
   * Blacklisting instead of whitelisting
   *
Repository: mui/material-ui
File: docs/src/modules/redux/optionsReducer.js (JavaScript)

Code before:
import { ACTION_TYPES, CODE_VARIANTS } from 'docs/src/modules/constants';
import mapTranslations from 'docs/src/modules/utils/mapTranslations';

const req = require.context('docs/translations', false, /translations.*\.json$/);
const translations = mapTranslations(req, 'json');

function getPath(obj, path) {
  if (!path || typeof path !== 'string') {
    return null;
  }

  return path.split('.').reduce((acc, item) => (acc && acc[item] ? acc[item] : null), obj);
}

const warnOnce = {};

const getT = (userLanguage) => (key, options = {}) => {
  const { ignoreWarning = false } = options;
  const wordings = translations[userLanguage];

  if (!wordings) {
    console.error(`Missing language: ${userLanguage}.`);
    return '…';
  }

  const translation = getPath(wordings, key);

  if (!translation) {
    const fullKey = `${userLanguage}:${key}`;
    // No warnings in CI env
    if (!ignoreWarning && !warnOnce[fullKey] && typeof window !== 'undefined') {
      console.error(`Missing translation for ${fullKey}.`);
      warnOnce[fullKey] = true;
    }
    return getPath(translations.en, key);
  }

  return translation;
};

const mapping = {
  [ACTION_TYPES.OPTIONS_CHANGE]: (state, action) => {
    const newState = {

Review comment (refactor):
This only memoized creation of a function not the actual `options.t` utility. We could also inline the function in the reducer and rely on closures. Then it's more obvious that this can't be a problematic pattern unless you also consider function components declaring functions in their render method a problem.

Code after:
import { ACTION_TYPES, CODE_VARIANTS } from 'docs/src/modules/constants';

const mapping = {
  [ACTION_TYPES.OPTIONS_CHANGE]: (state, action) => {
    const newState = {
      codeVariant: action.payload.codeVariant || state.codeVariant,
      userLanguage: action.payload.userLanguage || state.userLanguage,
    };
    return newState;
  },
};

export default function optionsReducer(state = {}, action) {
  let newState = { ...state };

  if (!newState.codeVariant) {
    newState.codeVariant = CODE_VARIANTS.JS;
  }
  if (!newState.userLanguage) {
    newState.userLanguage = 'en';
  }

  if (mapping[action.type]) {
    newState = mapping[action.type](state, action);
  }

  return newState;
}