ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: packages/react-dom/src/__tests__/ReactDOMSelect-test.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.
 *
 * @emails react-core
 */

'use strict';

// Fix JSDOM. setAttribute is supposed to throw on things that can't be implicitly toStringed.
const setAttribute = Element.prototype.setAttribute;
Element.prototype.setAttribute = function(name, value) {
  return setAttribute.call(this, name, '' + value);
};

describe('ReactDOMSelect', () => {
  let React;
  let ReactDOM;
  let ReactDOMServer;
  let ReactTestUtils;

  const noop = function () {};

  beforeEach(() => {
    jest.resetModules();
    React = require('react');
    ReactDOM = require('react-dom');
    ReactDOMServer = require('react-dom/server');
    ReactTestUtils = require('react-dom/test-utils');
  });

  it('should allow setting `defaultValue`', () => {
    const stub = (
      <select defaultValue="giraffe">
        <option value="monkey">A monkey!</option>

Review comment (suggestion):
This is a little hacky but it's making the TemporalLike objects throw without us explicitly tostringing in React which we shouldn't need to, since there can be Trusted Types involved. Maybe a newer JSDOM would help.

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.
 *
 * @emails react-core
 */

'use strict';

// Fix JSDOM. setAttribute is supposed to throw on things that can't be implicitly toStringed.
const setAttribute = Element.prototype.setAttribute;
Element.prototype.setAttribute = function (name, value) {
  // eslint-disable-next-line react-internal/safe-string-coercion
  return setAttribute.call(this, name, '' + value);
};

describe('ReactDOMSelect', () => {
  let React;
  let ReactDOM;
  let ReactDOMServer;
  let ReactTestUtils;

  const noop = function () {};

  beforeEach(() => {
    jest.resetModules();
    React = require('react');
    ReactDOM = require('react-dom');
    ReactDOMServer = require('react-dom/server');
    ReactTestUtils = require('react-dom/test-utils');
  });

  it('should allow setting `defaultValue`', () => {
    const stub = (
      <select defaultValue="giraffe">
Repository: facebook/react
File: packages/react-devtools-extensions/src/background.js (JavaScript)

Code before:
                });
              }
            } else {
              backendsToDownload.push(version);
            }
          });
          // Request the necessary backends in the extension DevTools UI
          // TODO: handle this message in main.js
          devtools?.postMessage({
            payload: {
              type: 'react-devtools-additional-backends',
              versions: backendsToDownload,
            },
          });
          break;
      }
    }
  } else if (request.payload?.tabId) {
    const tabId = request.payload?.tabId;
    // This is sent from the devtools page when it is ready for injecting the backend
    if (request.payload.type === 'react-devtools-inject-backend-manager') {
      if (!IS_FIREFOX) {
        // equivalent logic for Firefox is in prepareInjection.js
        chrome.scripting.executeScript({
          target: {tabId},
          files: [`/build/backendManager.js`],
          world: chrome.scripting.ExecutionWorld.MAIN,
        });
      }
    }
  }
});


Review comment (suggestion):
```suggestion
          files: ['/build/backendManager.js'],
```

Code after:
                });
              }
            } else {
              backendsToDownload.push(version);
            }
          });
          // Request the necessary backends in the extension DevTools UI
          // TODO: handle this message in main.js to build the UI
          devtools?.postMessage({
            payload: {
              type: 'react-devtools-additional-backends',
              versions: backendsToDownload,
            },
          });
          break;
      }
    }
  } else if (request.payload?.tabId) {
    const tabId = request.payload?.tabId;
    // This is sent from the devtools page when it is ready for injecting the backend
    if (request.payload.type === 'react-devtools-inject-backend-manager') {
      if (!IS_FIREFOX) {
        // equivalent logic for Firefox is in prepareInjection.js
        chrome.scripting.executeScript({
          target: {tabId},
          files: ['/build/backendManager.js'],
          world: chrome.scripting.ExecutionWorld.MAIN,
        });
      }
    }
  }
});
Repository: facebook/react
File: packages/react-dom-bindings/src/client/escapeSelectorAttributeValueInsideDoubleQuotes.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
 */

// When passing user input into querySelector(All) the embedded string must not alter
// the semantics of the query. This escape function is safe to use when we know the
// provided value is going to be wrapped in double quotes as part of an attribute selector
// Do not use it anywhere else
// we escape double quotes and backslashes
const escapeSelectorAttributeValueInsideDoubleQuotesRegex = /[\n\"\\]/g;
export default function escapeSelectorAttributeValueInsideDoubleQuotes(
  value: string,
): string {
  return value.replace(
    escapeSelectorAttributeValueInsideDoubleQuotesRegex,
    ch => '\\' + ch.charCodeAt(0).toString(16),
  );
}


Review comment (security):
Wait @gnoff I think this needs to add whitespace or else this doesn't work correctly when an escaped character appears before a number or whitespace character:

```suggestion
    ch => '\\' + ch.charCodeAt(0).toString(16) + ' ',
```

https://www.w3.org/TR/css-syntax-3/#escape-diagram

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.
 *
 * @flow
 */

// When passing user input into querySelector(All) the embedded string must not alter
// the semantics of the query. This escape function is safe to use when we know the
// provided value is going to be wrapped in double quotes as part of an attribute selector
// Do not use it anywhere else
// we escape double quotes and backslashes
const escapeSelectorAttributeValueInsideDoubleQuotesRegex = /[\n\"\\]/g;
export default function escapeSelectorAttributeValueInsideDoubleQuotes(
  value: string,
): string {
  return value.replace(
    escapeSelectorAttributeValueInsideDoubleQuotesRegex,
    ch => '\\' + ch.charCodeAt(0).toString(16) + ' ',
  );
}
Repository: facebook/react
File: packages/react-debug-tools/src/ReactDebugHooks.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 {
  MutableSource,
  MutableSourceGetSnapshotFn,
  MutableSourceSubscribeFn,
  ReactContext,
  ReactProviderType,
  StartTransitionOptions,
} from 'shared/ReactTypes';
import {REACT_MEMO_CACHE_SENTINEL} from 'shared/ReactSymbols';
import type {
  Fiber,
  Dispatcher as DispatcherType,
} from 'react-reconciler/src/ReactInternalTypes';

import ErrorStackParser from 'error-stack-parser';
import assign from 'shared/assign';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
  FunctionComponent,
  SimpleMemoComponent,
  ContextProvider,
  ForwardRef,
} from 'react-reconciler/src/ReactWorkTags';

type CurrentDispatcherRef = typeof ReactSharedInternals.ReactCurrentDispatcher;

// Used to track hooks called during a render

type HookLogEntry = {
  primitive: string,
  stackError: Error,
  value: mixed,
  ...
};

Review comment (suggestion):
This might cause compatibility issues in the future. See packages/react-devtools-shared/src/backend/ReactSymbols.js
Also, this is not a type, so should not be in the middle of other `import type` lines

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.
 *
 * @flow
 */

import type {
  MutableSource,
  MutableSourceGetSnapshotFn,
  MutableSourceSubscribeFn,
  ReactContext,
  ReactProviderType,
  StartTransitionOptions,
} from 'shared/ReactTypes';
import type {
  Fiber,
  Dispatcher as DispatcherType,
} from 'react-reconciler/src/ReactInternalTypes';

import ErrorStackParser from 'error-stack-parser';
import assign from 'shared/assign';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
  FunctionComponent,
  SimpleMemoComponent,
  ContextProvider,
  ForwardRef,
} from 'react-reconciler/src/ReactWorkTags';

type CurrentDispatcherRef = typeof ReactSharedInternals.ReactCurrentDispatcher;

// Used to track hooks called during a render

type HookLogEntry = {
  primitive: string,
  stackError: Error,
  value: mixed,
  ...
};
Repository: facebook/react
File: packages/react-dom-bindings/src/client/DOMNamespaces.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
 */

export const HTML_NAMESPACE = 'http://www.w3.org/1999/xhtml';
export const MATH_NAMESPACE = 'http://www.w3.org/1998/Math/MathML';
export const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';


Review comment (bug):
My intuition is that this shouldn't be used anymore because our logic is consistently that we treat HTML as the "catch all" and the only namespaces that are allowed are SVG and MATH.

I think the only place remaining is in `normalizeHTML()` but that should probably check if the namespace is SVG or MATH, otherwise create it without namespace.

That's probably already a bug because it would treat a custom namespace as XML in the comparison which would lead to a mismatch error since everywhere else it's treated as HTML.

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.
 *
 * @flow
 */

export const MATH_NAMESPACE = 'http://www.w3.org/1998/Math/MathML';
export const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';
Repository: facebook/react
File: packages/react-dom/index.classic.fb.js (JavaScript)

Code before:

import {isEnabled} from 'react-dom-bindings/src/events/ReactDOMEventListener';

import Internals from './src/ReactDOMSharedInternals';

// For classic WWW builds, include a few internals that are already in use.
Object.assign((Internals: any), {
  ReactBrowserEventEmitter: {
    isEnabled,
  },
});

export {
  createPortal,
  createRoot,
  hydrateRoot,
  findDOMNode,
  flushSync,
  hydrate,
  render,
  unmountComponentAtNode,
  unstable_batchedUpdates,
  unstable_createEventHandle,
  unstable_renderSubtreeIntoContainer,
  unstable_runWithPriority, // DO NOT USE: Temporarily exposed to migrate off of Scheduler.runWithPriority.
  prefetchDNS,
  preconnect,
  preload,
  preinit,
  version,
} from './src/client/ReactDOM';

export {Internals as __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED};


Review comment (suggestion):
Just noticing that the form flag is on in www but not the async transitions one so it's kind of a useless combination for this export.

Code after:

import {isEnabled} from 'react-dom-bindings/src/events/ReactDOMEventListener';

import Internals from './src/ReactDOMSharedInternals';

// For classic WWW builds, include a few internals that are already in use.
Object.assign((Internals: any), {
  ReactBrowserEventEmitter: {
    isEnabled,
  },
});

export {
  createPortal,
  createRoot,
  hydrateRoot,
  findDOMNode,
  flushSync,
  hydrate,
  render,
  unmountComponentAtNode,
  unstable_batchedUpdates,
  unstable_createEventHandle,
  unstable_renderSubtreeIntoContainer,
  unstable_runWithPriority, // DO NOT USE: Temporarily exposed to migrate off of Scheduler.runWithPriority.
  useFormStatus as experimental_useFormStatus,
  prefetchDNS,
  preconnect,
  preload,
  preinit,
  version,
} from './src/client/ReactDOM';

export {Internals as __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED};
Repository: facebook/react
File: packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js (JavaScript)

Code before:
  return parentType !== 'head' && parentType !== 'body';
}

export function didNotMatchHydratedContainerTextInstance(
  parentContainer: Container,
  textInstance: TextInstance,
  text: string,
  isConcurrentMode: boolean,
  shouldWarnDev: boolean,
) {
  checkForUnmatchedText(
    textInstance.nodeValue,
    text,
    isConcurrentMode,
    shouldWarnDev,
  );
}

export function didNotMatchHydratedTextInstance(
  parentType: string,
  parentProps: Props,
  parentInstance: Instance,
  textInstance: TextInstance,
  text: string,
  isConcurrentMode: boolean,
  shouldWarnDev: boolean,
) {
  if (parentProps[SUPPRESS_HYDRATION_WARNING] !== true) {
    checkForUnmatchedText(
      textInstance.nodeValue,
      text,
      isConcurrentMode,
      shouldWarnDev,
    );
  }
}

export function didNotHydrateInstanceWithinContainer(
  parentContainer: Container,
  instance: HydratableInstance,
) {
  if (__DEV__) {
    if (instance.nodeType === ELEMENT_NODE) {
      warnForDeletedHydratableElement(parentContainer, (instance: any));
    } else if (instance.nodeType === COMMENT_NODE) {
      // TODO: warnForDeletedHydratableSuspenseBoundary
    } else {
      warnForDeletedHydratableText(parentContainer, (instance: any));
    }
  }
}

Review comment (suggestion):
This function is doing double duty as the signal for not clearing forms and as a signal to not clear head and body when singletons flag is turned off. We should make it two separate functions so it is clearer we can delete the legacy one once the singletons flag lands

Code after:
    }
    node = node.previousSibling;
  }
  return null;
}

export function commitHydratedContainer(container: Container): void {
  // Retry if any event replaying was blocked on this.
  retryIfBlockedOn(container);
}

export function commitHydratedSuspenseInstance(
  suspenseInstance: SuspenseInstance,
): void {
  // Retry if any event replaying was blocked on this.
  retryIfBlockedOn(suspenseInstance);
}

export function shouldDeleteUnhydratedTailInstances(
  parentType: string,
): boolean {
  return (
    (enableHostSingletons ||
      (parentType !== 'head' && parentType !== 'body')) &&
    (!enableFormActions || parentType !== 'form')
  );
}

export function didNotMatchHydratedContainerTextInstance(
  parentContainer: Container,
  textInstance: TextInstance,
  text: string,
  isConcurrentMode: boolean,
  shouldWarnDev: boolean,
) {
  checkForUnmatchedText(
    textInstance.nodeValue,
    text,
    isConcurrentMode,
    shouldWarnDev,
  );
}

export function didNotMatchHydratedTextInstance(
  parentType: string,
  parentProps: Props,
  parentInstance: Instance,
  textInstance: TextInstance,
  text: string,
  isConcurrentMode: boolean,
  shouldWarnDev: boolean,
Repository: facebook/react
File: packages/react-dom-bindings/src/events/ReactDOMEventListener.js (JavaScript)

Code before:
    case 'copy':
    case 'cut':
    case 'auxclick':
    case 'dblclick':
    case 'dragend':
    case 'dragstart':
    case 'drop':
    case 'focusin':
    case 'focusout':
    case 'input':
    case 'invalid':
    case 'keydown':
    case 'keypress':
    case 'keyup':
    case 'mousedown':
    case 'mouseup':
    case 'paste':
    case 'pause':
    case 'play':
    case 'pointercancel':
    case 'pointerdown':
    case 'pointerup':
    case 'ratechange':
    case 'reset':
    case 'resize':
    case 'scrollend':
    case 'seeked':
    case 'submit':
    case 'touchcancel':
    case 'touchend':
    case 'touchstart':
    case 'volumechange':
    // Used by polyfills: (fall through)
    case 'change':
    case 'selectionchange':
    case 'textInput':
    case 'compositionstart':
    case 'compositionend':
    case 'compositionupdate':
    // Only enableCreateEventHandleAPI: (fall through)
    case 'beforeblur':
    case 'afterblur':
    // Not used by React but could be by user code: (fall through)
    case 'beforeinput':
    case 'blur':
    case 'fullscreenchange':
    case 'focus':
    case 'hashchange':
    case 'popstate':
    case 'select':
    case 'selectstart':

Review comment (suggestion):
Added this to `DiscreteEventPriority` rather than `ContinuousEventPriority` which would match `onScroll`. I think this is right since the event only fires once at the end of a scroll, but not fully sure about the implications.

Code after:
    case 'copy':
    case 'cut':
    case 'auxclick':
    case 'dblclick':
    case 'dragend':
    case 'dragstart':
    case 'drop':
    case 'focusin':
    case 'focusout':
    case 'input':
    case 'invalid':
    case 'keydown':
    case 'keypress':
    case 'keyup':
    case 'mousedown':
    case 'mouseup':
    case 'paste':
    case 'pause':
    case 'play':
    case 'pointercancel':
    case 'pointerdown':
    case 'pointerup':
    case 'ratechange':
    case 'reset':
    case 'resize':
    case 'seeked':
    case 'submit':
    case 'touchcancel':
    case 'touchend':
    case 'touchstart':
    case 'volumechange':
    // Used by polyfills: (fall through)
    case 'change':
    case 'selectionchange':
    case 'textInput':
    case 'compositionstart':
    case 'compositionend':
    case 'compositionupdate':
    // Only enableCreateEventHandleAPI: (fall through)
    case 'beforeblur':
    case 'afterblur':
    // Not used by React but could be by user code: (fall through)
    case 'beforeinput':
    case 'blur':
    case 'fullscreenchange':
    case 'focus':
    case 'hashchange':
    case 'popstate':
    case 'select':
    case 'selectstart':
      return DiscreteEventPriority;
Repository: facebook/react
File: packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js (JavaScript)

Code before:
  }
  return;
}

function preloadPropsFromPreloadOptions(
  href: string,
  as: string,
  options: PreloadOptions,
): PreloadProps {
  return {
    rel: 'preload',
    as,
    href,
    crossOrigin: as === 'font' ? '' : options.crossOrigin,
    integrity: options.integrity,
    type: options.type,
  };
}

function preloadAsStylePropsFromProps(href: string, props: any): PreloadProps {
  return {
    rel: 'preload',
    as: 'style',
    href: href,
    crossOrigin: props.crossOrigin,
    integrity: props.integrity,
    media: props.media,
    hrefLang: props.hrefLang,
    referrerPolicy: props.referrerPolicy,
  };
}

function preloadAsScriptPropsFromProps(href: string, props: any): PreloadProps {
  return {
    rel: 'preload',
    as: 'script',
    href,
    crossOrigin: props.crossOrigin,
    integrity: props.integrity,
    referrerPolicy: props.referrerPolicy,
  };
}

function stylesheetPropsFromPreinitOptions(
  href: string,
  precedence: string,
  options: PreinitOptions,
): StylesheetProps {
  return {
    rel: 'stylesheet',
    href,

Review comment (suggestion):
I didn't make these changes to `preloadPropsFromPreloadOptions`.  It seems like that's used for `ReactDOM.preload` - please let me know if I should make similar changes there as well

Code after:
  }
  return;
}

function preloadPropsFromPreloadOptions(
  href: string,
  as: string,
  options: PreloadOptions,
): PreloadProps {
  return {
    rel: 'preload',
    as,
    href,
    crossOrigin: as === 'font' ? '' : options.crossOrigin,
    integrity: options.integrity,
    type: options.type,
  };
}

function preloadAsStylePropsFromProps(href: string, props: any): PreloadProps {
  return {
    rel: 'preload',
    as: 'style',
    href: href,
    crossOrigin: props.crossOrigin,
    fetchPriority: props.fetchPriority,
    integrity: props.integrity,
    media: props.media,
    hrefLang: props.hrefLang,
    referrerPolicy: props.referrerPolicy,
  };
}

function preloadAsScriptPropsFromProps(href: string, props: any): PreloadProps {
  return {
    rel: 'preload',
    as: 'script',
    href,
    crossOrigin: props.crossOrigin,
    fetchPriority: props.fetchPriority,
    integrity: props.integrity,
    nonce: props.nonce,
    referrerPolicy: props.referrerPolicy,
  };
}

function stylesheetPropsFromPreinitOptions(
  href: string,
  precedence: string,
  options: PreinitOptions,
): StylesheetProps {
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/views/Highlighter/index.js (JavaScript)

Code before:
  }

  function onMouseEvent(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();
  }

  function onPointerDown(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();

    selectFiberForNode(
      (((event.composed
        ? event.composedPath()[0]
        : event.target): any): HTMLElement),
    );
  }

  let lastHoveredNode: HTMLElement | null = null;
  function onPointerMove(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();

    const target: HTMLElement = ((event.composed
      ? event.composedPath()[0]
      : event.target): any);
    if (lastHoveredNode === target) return;
    lastHoveredNode = target;

    if (target.tagName === 'IFRAME') {
      const iframe: HTMLIFrameElement = (target: any);
      try {
        if (!iframesListeningTo.has(iframe)) {
          const window = iframe.contentWindow;
          registerListenersOnWindow(window);
          iframesListeningTo.add(iframe);
        }
      } catch (error) {
        // This can error when the iframe is on a cross-origin.
      }
    }

    // Don't pass the name explicitly.
    // It will be inferred from DOM tag and Fiber owner.
    showOverlay([target], null, agent, false);

    selectFiberForNode(target);
  }

  function onPointerUp(event: MouseEvent) {
    event.preventDefault();

Review comment (suggestion):
Can you please move it to a separate function? something like this:

```
  function getEventTarget(event: MouseEvent): HTMLElement {
    if (event.composed) {
      return (event.composedPath()[0]: any);
    }

    return (event.target: any);
  }
  ```
  
  This also can be used for lines 155-157

Code after:
  }

  function onMouseEvent(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();
  }

  function onPointerDown(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();

    selectFiberForNode(getEventTarget(event));
  }

  let lastHoveredNode: HTMLElement | null = null;
  function onPointerMove(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();

    const target: HTMLElement = getEventTarget(event);
    if (lastHoveredNode === target) return;
    lastHoveredNode = target;

    if (target.tagName === 'IFRAME') {
      const iframe: HTMLIFrameElement = (target: any);
      try {
        if (!iframesListeningTo.has(iframe)) {
          const window = iframe.contentWindow;
          registerListenersOnWindow(window);
          iframesListeningTo.add(iframe);
        }
      } catch (error) {
        // This can error when the iframe is on a cross-origin.
      }
    }

    // Don't pass the name explicitly.
    // It will be inferred from DOM tag and Fiber owner.
    showOverlay([target], null, agent, false);

    selectFiberForNode(target);
  }

  function onPointerUp(event: MouseEvent) {
    event.preventDefault();
    event.stopPropagation();
  }

  const selectFiberForNode = throttle(
    memoize((node: HTMLElement) => {
      const id = agent.getIDForNode(node);