ronantakizawa/codereview-bench huggingface dataset:
Repository: adobe/react-spectrum
File: packages/@react-aria/utils/test/runAfterTransition.test.ts (TypeScript)

Code before:
class MockTransitionEvent extends Event {
  propertyName: string;

  constructor(type: string, eventInitDict?: TransitionEventInit) {
    super(type, eventInitDict);
    this.propertyName = eventInitDict?.propertyName || '';
  }
}

describe('runAfterTransition', () => {
  const originalTransitionEvent = global.TransitionEvent;
  const nodes = new Set<Node>();

  beforeAll(() => {
    global.TransitionEvent = MockTransitionEvent as any;
  });

  afterAll(() => {
    global.TransitionEvent = originalTransitionEvent;
  });

  beforeEach(() => {
    jest
      .spyOn(window, 'requestAnimationFrame')
      // @ts-expect-error -- mock
      .mockImplementation((cb) => cb());
  });

  afterEach(() => {
    jest.restoreAllMocks();
    cleanupElements();
  });

  function appendElement(element: HTMLElement) {
    nodes.add(element);
    document.body.appendChild(element);
    return element;
  }

  function cleanupElements() {
    for (const node of nodes) {
      document.body.removeChild(node);
      nodes.delete(node);
    }
  }

  it('calls callback immediately when no transition is running', () => {
    const callback = jest.fn();
    runAfterTransition(callback);
    expect(callback).toHaveBeenCalled();
  });

Review comment (suggestion):
better to make use of `jest.useFakeTimers`
you'll need to add some
```
act(() => {jest.runOnlyPendingTimers();});
```
in order to trigger the mocked raf

Code after:

class MockTransitionEvent extends Event {
  propertyName: string;

  constructor(type: string, eventInitDict?: TransitionEventInit) {
    super(type, eventInitDict);
    this.propertyName = eventInitDict?.propertyName || '';
  }
}

describe('runAfterTransition', () => {
  const originalTransitionEvent = global.TransitionEvent;
  const nodes = new Set<Node>();

  beforeAll(() => {
    global.TransitionEvent = MockTransitionEvent as any;
  });

  afterAll(() => {
    global.TransitionEvent = originalTransitionEvent;
  });
  
  beforeEach(() => {
    jest.useFakeTimers();
  });

  afterEach(() => {
    jest.useRealTimers();
    jest.restoreAllMocks();
    cleanupElements();
  });

  function appendElement(element: HTMLElement) {
    nodes.add(element);
    document.body.appendChild(element);
    return element;
  }

  function cleanupElements() {
    for (const node of nodes) {
      document.body.removeChild(node);
      nodes.delete(node);
    }
  }

  it('calls callback immediately when no transition is running', () => {
    const callback = jest.fn();
    runAfterTransition(callback);
    act(() => {jest.runOnlyPendingTimers();});
    expect(callback).toHaveBeenCalled();
  });
Repository: adobe/react-spectrum
File: packages/@react-aria/utils/src/runAfterTransition.ts (TypeScript)

Code before:
      }

      transitionCallbacks.clear();
    }
  };

  document.body.addEventListener('transitionrun', onTransitionStart);
  document.body.addEventListener('transitionend', onTransitionEnd);
}

if (typeof document !== 'undefined') {
  if (document.readyState !== 'loading') {
    setupGlobalEvents();
  } else {
    document.addEventListener('DOMContentLoaded', setupGlobalEvents);
  }
}

/**
 * Cleans up any elements that are no longer in the document.
 * This is necessary because we can't rely on transitionend events to fire
 * for elements that are removed from the document while transitioning.
 */
function cleanupDetachedElements() {
  for (const [element] of transitionsByElement) {
    if (element instanceof HTMLElement && !element.isConnected) {
      transitionsByElement.delete(element);
    }
  }
}

export function runAfterTransition(fn: () => void): void {
  // Wait one frame to see if an animation starts, e.g. a transition on mount.
  requestAnimationFrame(() => {
    cleanupDetachedElements();
    // If no transitions are running, call the function immediately.
    // Otherwise, add it to a list of callbacks to run at the end of the animation.
    if (transitionsByElement.size === 0) {
      fn();
    } else {
      transitionCallbacks.add(fn);
    }
  });
}


Review comment (suggestion):
One last edgecase to avoid, if we're running code across iframes then we need to check in this way

```suggestion
    let windowObject = getOwnerWindow(element);
    if (element instanceof windowObject.HTMLElement && !element.isConnected) {
```

Code after:
      }

      transitionCallbacks.clear();
    }
  };

  document.body.addEventListener('transitionrun', onTransitionStart);
  document.body.addEventListener('transitionend', onTransitionEnd);
}

if (typeof document !== 'undefined') {
  if (document.readyState !== 'loading') {
    setupGlobalEvents();
  } else {
    document.addEventListener('DOMContentLoaded', setupGlobalEvents);
  }
}

/**
 * Cleans up any elements that are no longer in the document.
 * This is necessary because we can't rely on transitionend events to fire
 * for elements that are removed from the document while transitioning.
 */
function cleanupDetachedElements() {
  for (const [eventTarget] of transitionsByElement) {
    // Similar to `eventTarget instanceof Element && !eventTarget.isConnected`, but avoids
    // the explicit instanceof check, since it may be different in different contexts.
    if ('isConnected' in eventTarget && !eventTarget.isConnected) {
      transitionsByElement.delete(eventTarget);
    }
  }
}

export function runAfterTransition(fn: () => void): void {
  // Wait one frame to see if an animation starts, e.g. a transition on mount.
  requestAnimationFrame(() => {
    cleanupDetachedElements();
    // If no transitions are running, call the function immediately.
    // Otherwise, add it to a list of callbacks to run at the end of the animation.
    if (transitionsByElement.size === 0) {
      fn();
    } else {
      transitionCallbacks.add(fn);
    }
  });
}
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/NotificationBadge.tsx (TypeScript)

Code before:
    forcedColors: 'ButtonFace'
  },
  height: {
    size: {
      S: {
        default: 12,
        isIndicatorOnly: 8
      },
      M: {
        default: fontRelative(18), // sort of arbitrary? tried to get as close to the figma designs as possible
        isIndicatorOnly: 8
      },
      L: {
        default: 16,
        isIndicatorOnly: fontRelative(12)
      },
      XL: {
        default: 18,
        isIndicatorOnly: fontRelative(12)
      }
    }
  },
  aspectRatio: {
    isIndicatorOnly: 'square',
    isSingleDigit: 'square'
  },
  width: 'max',
  paddingX: {
    isDoubleDigit: 'edge-to-text'
  },
  borderRadius: 'pill'
}, getAllowedOverrides());

/**
 * Notification badges are used to indicate new or pending activity .
 */
export const NotificationBadge = forwardRef(function Badge(props: NotificationBadgeProps, ref: DOMRef<HTMLDivElement>) {
  let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-spectrum/s2');
  [props, ref] = useSpectrumContextProps(props, ref, NotificationBadgeContext);
  let {
    size = 'S',
    value,
    isDisabled = false,
    staticColor,
    ...otherProps
  } = props as NotificationBadgeContextProps;
  let domRef = useDOMRef(ref);
  let {locale} = useLocale();
  let formattedValue = '';

  let isIndicatorOnly = false;

Review comment (suggestion):
noticed that having width set to `fit` caused the text to wrap in locales like `'zh-CN-u-nu-hanidec'` so i changed it 

Code after:
  },
  height: {
    size: {
      S: {
        default: 12,
        isIndicatorOnly: 8
      },
      M: {
        default: fontRelative(18), // sort of arbitrary? tried to get as close to the figma designs as possible
        isIndicatorOnly: 8
      },
      L: {
        default: 16,
        isIndicatorOnly: fontRelative(12)
      },
      XL: {
        default: 18,
        isIndicatorOnly: fontRelative(12)
      }
    }
  },
  aspectRatio: {
    isIndicatorOnly: 'square',
    isSingleDigit: 'square'
  },
  width: 'max',
  paddingX: {
    isDoubleDigit: 'edge-to-text'
  },
  borderRadius: 'pill'
}, getAllowedOverrides());

/**
 * Notification badges are used to indicate new or pending activity .
 */
export const NotificationBadge = forwardRef(function Badge(props: NotificationBadgeProps, ref: DOMRef<HTMLDivElement>) {
  let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-spectrum/s2');
  [props, ref] = useSpectrumContextProps(props, ref, NotificationBadgeContext);
  let {
    size = 'S',
    value,
    isDisabled = false,
    staticColor,
    ...otherProps
  } = props as NotificationBadgeContextProps;
  let domRef = useDOMRef(ref);
  let {locale} = useLocale();
  let formattedValue = '';

  let isIndicatorOnly = false;
  let isSingleDigit = false;
Repository: adobe/react-spectrum
File: packages/@react-aria/focus/src/FocusRing.tsx (TypeScript)

Code before:
export interface FocusRingProps {
  /** Child element to apply CSS classes to. */
  children: ReactElement,
  /** CSS class to apply when the element is focused. */
  focusClass?: string,
  /** CSS class to apply when the element has keyboard focus. */
  focusRingClass?: string,
  /**
   * Whether to show the focus ring when something
   * inside the container element has focus (true), or
   * only if the container itself has focus (false).
   * @default false
   */
  within?: boolean,
  /** Whether the element is a text input. */
  isTextInput?: boolean,
  /** Whether the element will be auto focused. */
  autoFocus?: boolean
}

/**
 * A utility component that applies a CSS class when an element has keyboard focus.
 * Focus rings are visible only when the user is interacting with a keyboard,
 * not with a mouse, touch, or other input methods.
 */
export function FocusRing(props: FocusRingProps): JSX.Element {
  let {children, focusClass, focusRingClass} = props;
  let {isFocused, isFocusVisible, focusProps} = useFocusRing(props);
  let child = React.Children.only(children);

  return React.cloneElement(child, mergeProps(child.props as any, {
    ...focusProps,
    className: clsx({
      [focusClass || '']: isFocused,
      [focusRingClass || '']: isFocusVisible
    })
  }));
}


Review comment (suggestion):
I just checked the unpkg for this one, apparently it was ReactElement, I'll update a few of the others
https://app.unpkg.com/@react-aria/focus@3.20.1/files/dist/types.d.ts

Code after:
export interface FocusRingProps {
  /** Child element to apply CSS classes to. */
  children: ReactElement,
  /** CSS class to apply when the element is focused. */
  focusClass?: string,
  /** CSS class to apply when the element has keyboard focus. */
  focusRingClass?: string,
  /**
   * Whether to show the focus ring when something
   * inside the container element has focus (true), or
   * only if the container itself has focus (false).
   * @default false
   */
  within?: boolean,
  /** Whether the element is a text input. */
  isTextInput?: boolean,
  /** Whether the element will be auto focused. */
  autoFocus?: boolean
}

/**
 * A utility component that applies a CSS class when an element has keyboard focus.
 * Focus rings are visible only when the user is interacting with a keyboard,
 * not with a mouse, touch, or other input methods.
 */
export function FocusRing(props: FocusRingProps): React.ReactElement<unknown, string | React.JSXElementConstructor<any>> {
  let {children, focusClass, focusRingClass} = props;
  let {isFocused, isFocusVisible, focusProps} = useFocusRing(props);
  let child = React.Children.only(children);

  return React.cloneElement(child, mergeProps(child.props as any, {
    ...focusProps,
    className: clsx({
      [focusClass || '']: isFocused,
      [focusRingClass || '']: isFocusVisible
    })
  }));
}
Repository: adobe/react-spectrum
File: packages/react-aria-components/src/Table.tsx (TypeScript)

Code before:
          default:
            throw new Error('Unsupported node type in TableHeader: ' + item.type);
        }
      }, [])
    });

    let THead = useElementType('thead');
    let {rowGroupProps} = useTableRowGroup();
    let {hoverProps, isHovered} = useHover({
      onHoverStart: props.onHoverStart,
      onHoverChange: props.onHoverChange,
      onHoverEnd: props.onHoverEnd
    });

    let renderProps = useRenderProps({
      className: props.className,
      style: props.style,
      defaultClassName: 'react-aria-TableHeader',
      values: {
        isHovered
      }
    });

    return (
      <THead
        {...mergeProps(filterDOMProps(props as any, {propNames: tableHeaderPropNames}), rowGroupProps, hoverProps)}
        {...renderProps}
        ref={ref}
        data-hovered={isHovered || undefined}>
        {headerRows}
      </THead>
    );
  },
  props => (
    <Collection dependencies={props.dependencies} items={props.columns}>
      {props.children}
    </Collection>
  )
);

function TableHeaderRow({item}: {item: GridNode<any>}) {
  let ref = useRef<HTMLTableRowElement>(null);
  let state = useContext(TableStateContext)!;
  let {isVirtualized, CollectionBranch} = useContext(CollectionRendererContext);
  let {rowProps} = useTableHeaderRow({node: item, isVirtualized}, state, ref);
  let {checkboxProps} = useTableSelectAllCheckbox(state);
  let TR = useElementType('tr');

  return (
    <TR {...rowProps} ref={ref}>
      <Provider

Review comment (performance):
could you remove the prop from the filterDOMProps (it's slow and nothing else returns that)
you can just pass it directly to the element
`onScroll={props.onScroll}`

Code after:
        }
      }, [])
    });

    let THead = useElementType('thead');
    let {rowGroupProps} = useTableRowGroup();
    let {hoverProps, isHovered} = useHover({
      onHoverStart: props.onHoverStart,
      onHoverChange: props.onHoverChange,
      onHoverEnd: props.onHoverEnd
    });

    let renderProps = useRenderProps({
      className: props.className,
      style: props.style,
      defaultClassName: 'react-aria-TableHeader',
      values: {
        isHovered
      }
    });

    return (
      <THead
        {...mergeProps(filterDOMProps(props as any), rowGroupProps, hoverProps)}
        {...renderProps}
        onScroll={props.onScroll}
        ref={ref}
        data-hovered={isHovered || undefined}>
        {headerRows}
      </THead>
    );
  },
  props => (
    <Collection dependencies={props.dependencies} items={props.columns}>
      {props.children}
    </Collection>
  )
);

function TableHeaderRow({item}: {item: GridNode<any>}) {
  let ref = useRef<HTMLTableRowElement>(null);
  let state = useContext(TableStateContext)!;
  let {isVirtualized, CollectionBranch} = useContext(CollectionRendererContext);
  let {rowProps} = useTableHeaderRow({node: item, isVirtualized}, state, ref);
  let {checkboxProps} = useTableSelectAllCheckbox(state);
  let TR = useElementType('tr');

  return (
    <TR {...rowProps} ref={ref}>
      <Provider
        values={[
Repository: adobe/react-spectrum
File: packages/@react-aria/interactions/src/usePress.ts (TypeScript)

Code before:
    preventFocusOnPress,
    removeAllGlobalListeners,
    allowTextSelectionOnPress,
    cancel,
    cancelOnPointerExit,
    triggerPressEnd,
    triggerPressStart,
    triggerPressUp,
    triggerClick,
    triggerSyntheticClick
  ]);

  // Avoid onClick delay for double tap to zoom by default.
  useEffect(() => {
    if (process.env.NODE_ENV === 'test') {
      return;
    }

    const ownerDocument = getOwnerDocument(domRef?.current);
    const styleId = 'react-aria-pressable-style';
    if (ownerDocument && ownerDocument.head && !ownerDocument.getElementById(styleId)) {
      const style = ownerDocument.createElement('style');
      style.id = styleId;
      style.textContent = `
@layer {
  :where([data-react-aria-pressable]) {
    touch-action: pan-x pan-y pinch-zoom;
  }
}
      `.trim();
      ownerDocument.head.prepend(style);
    }
  }, [domRef]);

  // Remove user-select: none in case component unmounts immediately after pressStart
  useEffect(() => {
    let state = ref.current;
    return () => {
      if (!allowTextSelectionOnPress) {
        restoreTextSelection(state.target ?? undefined);
      }
      for (let dispose of state.disposables) {
        dispose();
      }
      state.disposables = [];
    };
  }, [allowTextSelectionOnPress]);

  return {
    isPressed: isPressedProp || isPressed,
    pressProps: mergeProps(domProps, pressProps, {'data-react-aria-pressable': true})

Review comment (suggestion):
Actually the `:where` may be unnecessary. Specificity shouldn't matter since this is the lowest `@layer` and nothing else is in the same layer.

Code after:
    addGlobalListener,
    isDisabled,
    preventFocusOnPress,
    removeAllGlobalListeners,
    allowTextSelectionOnPress,
    cancel,
    cancelOnPointerExit,
    triggerPressEnd,
    triggerPressStart,
    triggerPressUp,
    triggerClick,
    triggerSyntheticClick
  ]);

  // Avoid onClick delay for double tap to zoom by default.
  useEffect(() => {
    if (!domRef || process.env.NODE_ENV === 'test') {
      return;
    }

    const ownerDocument = getOwnerDocument(domRef.current);
    if (!ownerDocument || !ownerDocument.head || ownerDocument.getElementById(STYLE_ID)) {
      return;
    }

    const style = ownerDocument.createElement('style');
    style.id = STYLE_ID;
    // touchAction: 'manipulation' is supposed to be equivalent, but in
    // Safari it causes onPointerCancel not to fire on scroll.
    // https://bugs.webkit.org/show_bug.cgi?id=240917
    style.textContent = `
@layer {
  [${PRESSABLE_ATTRIBUTE}] {
    touch-action: pan-x pan-y pinch-zoom;
  }
}
    `.trim();
    ownerDocument.head.prepend(style);
  }, [domRef]);

  // Remove user-select: none in case component unmounts immediately after pressStart
  useEffect(() => {
    let state = ref.current;
    return () => {
      if (!allowTextSelectionOnPress) {
        restoreTextSelection(state.target ?? undefined);
      }
      for (let dispose of state.disposables) {
        dispose();
      }
      state.disposables = [];
Repository: adobe/react-spectrum
File: packages/@react-aria/interactions/src/usePress.ts (TypeScript)

Code before:
        x = rect.width / 2;
        y = rect.height / 2;
      }
    }
    this.type = type;
    this.pointerType = pointerType;
    this.target = originalEvent.currentTarget as Element;
    this.shiftKey = originalEvent.shiftKey;
    this.metaKey = originalEvent.metaKey;
    this.ctrlKey = originalEvent.ctrlKey;
    this.altKey = originalEvent.altKey;
    this.x = x;
    this.y = y;
  }

  continuePropagation() {
    this.#shouldStopPropagation = false;
  }

  get shouldStopPropagation() {
    return this.#shouldStopPropagation;
  }
}

const LINK_CLICKED = Symbol('linkClicked');
const styleId = 'react-aria-pressable-style';

/**
 * Handles press interactions across mouse, touch, keyboard, and screen readers.
 * It normalizes behavior across browsers and platforms, and handles many nuances
 * of dealing with pointer and keyboard events.
 */
export function usePress(props: PressHookProps): PressResult {
  let {
    onPress,
    onPressChange,
    onPressStart,
    onPressEnd,
    onPressUp,
    onClick,
    isDisabled,
    isPressed: isPressedProp,
    preventFocusOnPress,
    shouldCancelOnPointerExit,
    allowTextSelectionOnPress,
    ref: domRef,
    ...domProps
  } = usePressResponderContext(props);

  let [isPressed, setPressed] = useState(false);
  let ref = useRef<PressState>({

Review comment (suggestion):
```suggestion
const STYLE_ID = 'react-aria-pressable-style';
```

Code after:
        x = rect.width / 2;
        y = rect.height / 2;
      }
    }
    this.type = type;
    this.pointerType = pointerType;
    this.target = originalEvent.currentTarget as Element;
    this.shiftKey = originalEvent.shiftKey;
    this.metaKey = originalEvent.metaKey;
    this.ctrlKey = originalEvent.ctrlKey;
    this.altKey = originalEvent.altKey;
    this.x = x;
    this.y = y;
  }

  continuePropagation() {
    this.#shouldStopPropagation = false;
  }

  get shouldStopPropagation() {
    return this.#shouldStopPropagation;
  }
}

const LINK_CLICKED = Symbol('linkClicked');
const STYLE_ID = 'react-aria-pressable-style';
const PRESSABLE_ATTRIBUTE = 'data-react-aria-pressable';

/**
 * Handles press interactions across mouse, touch, keyboard, and screen readers.
 * It normalizes behavior across browsers and platforms, and handles many nuances
 * of dealing with pointer and keyboard events.
 */
export function usePress(props: PressHookProps): PressResult {
  let {
    onPress,
    onPressChange,
    onPressStart,
    onPressEnd,
    onPressUp,
    onClick,
    isDisabled,
    isPressed: isPressedProp,
    preventFocusOnPress,
    shouldCancelOnPointerExit,
    allowTextSelectionOnPress,
    ref: domRef,
    ...domProps
  } = usePressResponderContext(props);

  let [isPressed, setPressed] = useState(false);
Repository: adobe/react-spectrum
File: packages/@react-aria/select/src/HiddenSelect.tsx (TypeScript)

Code before:
      autoComplete: selectProps.autoComplete,
      name,
      disabled: isDisabled,
      value: state.selectedKey ?? ''
    };

    if (validationBehavior === 'native') {
      // Use a visually hidden <input type="text"> rather than <input type="hidden">
      // so that an empty value blocks HTML form submission when the field is required.
      return (
        <input
          {...containerProps}
          {...inputProps}
          type="text"
          required={selectProps.required}
          onChange={() => {/** Ignore react warning. */}}
          onInvalid={(e) => {
            // Prevent native browser error popup from appearing.
            e.preventDefault();
            triggerRef.current?.focus();
          }} />
      );
    }

    return (
      <input {...inputProps} />
    );
  }

  return null;
}


Review comment (bug):
Noticed in the story that keyboard tab navigation is broken, we'll want to put a `tabIndex={-1}` on this element I think so it can't be tabbed to

Code after:
                );
              }
            })}
          </select>
        </label>
      </div>
    );
  } else if (name) {
    let data = selectData.get(state) || {};
    let {validationBehavior} = data;

    let inputProps: InputHTMLAttributes<HTMLInputElement> = {
      type: 'hidden',
      autoComplete: selectProps.autoComplete,
      name,
      form,
      disabled: isDisabled,
      value: state.selectedKey ?? ''
    };

    if (validationBehavior === 'native') {
      // Use a hidden <input type="text"> rather than <input type="hidden">
      // so that an empty value blocks HTML form submission when the field is required.
      return (
        <input
          {...inputProps}
          ref={inputRef}
          style={{display: 'none'}}
          type="text"
          required={selectProps.required}
          onChange={() => {/** Ignore react warning. */}} />
      );
    }

    return (
      <input {...inputProps} ref={inputRef} />
    );
  }

  return null;
}