ronantakizawa/codereview-bench huggingface dataset:
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/Tabs.tsx (TypeScript)

Code before:
}

export const TabsContext = createContext<ContextValue<Partial<TabsProps>, DOMRefValue<HTMLDivElement>>>(null);
const InternalTabsContext = createContext<Partial<TabsProps> & {
  tablistRef?: RefObject<HTMLDivElement | null>,
  prevRef?: RefObject<DOMRect | null>,
  selectedKey?: Key | null
}>({});

interface CollapseContextType {
  showTabs: boolean,
  menuId: string,
  valueId: string,
  ariaLabel?: string | undefined,
  ariaDescribedBy?: string | undefined,
  tabs: Array<Node<any>>,
  listRef: RefObject<HTMLDivElement | null>,
  onSelectionChange?: (key: Key) => void
}

const CollapseContext = createContext<CollapseContextType>({
  showTabs: true,
  menuId: '',
  valueId: '',
  tabs: [],
  listRef: {current: null}
});

const tabs = style({
  position: 'relative',
  display: 'flex',
  flexShrink: 0,
  font: 'ui',
  flexDirection: {
    orientation: {
      horizontal: 'column'
    }
  }
}, getAllowedOverrides({height: true}));

/**
 * Tabs organize content into multiple sections and allow users to navigate between them. The content under the set of tabs should be related and form a coherent unit.
 */
export const Tabs = forwardRef(function Tabs(props: TabsProps, ref: DOMRef<HTMLDivElement>) {
  [props, ref] = useSpectrumContextProps(props, ref, TabsContext);
  let {
    density = 'regular',
    isDisabled,
    disabledKeys,
    orientation = 'horizontal',
    labelBehavior = 'show'

Review comment (suggestion):
```suggestion
  listRef: Ref
```

maybe RefObject, i forget exactly

Code after:
}

export const TabsContext = createContext<ContextValue<Partial<TabsProps>, DOMRefValue<HTMLDivElement>>>(null);
const InternalTabsContext = createContext<Partial<TabsProps> & {
  tablistRef?: RefObject<HTMLDivElement | null>,
  prevRef?: RefObject<DOMRect | null>,
  selectedKey?: Key | null
}>({});

interface CollapseContextType {
  showTabs: boolean,
  menuId: string,
  valueId: string,
  ariaLabel?: string | undefined,
  ariaDescribedBy?: string | undefined,
  tabs: Array<Node<any>>,
  listRef?: RefObject<HTMLDivElement | null>,
  onSelectionChange?: (key: Key) => void
}

const CollapseContext = createContext<CollapseContextType>({
  showTabs: true,
  menuId: '',
  valueId: '',
  tabs: []
});

const tabs = style({
  position: 'relative',
  display: 'flex',
  flexShrink: 0,
  font: 'ui',
  flexDirection: {
    orientation: {
      horizontal: 'column'
    }
  }
}, getAllowedOverrides({height: true}));

/**
 * Tabs organize content into multiple sections and allow users to navigate between them. The content under the set of tabs should be related and form a coherent unit.
 */
export const Tabs = forwardRef(function Tabs(props: TabsProps, ref: DOMRef<HTMLDivElement>) {
  [props, ref] = useSpectrumContextProps(props, ref, TabsContext);
  let {
    density = 'regular',
    isDisabled,
    disabledKeys,
    orientation = 'horizontal',
    labelBehavior = 'show'
  } = props;
Repository: adobe/react-spectrum
File: packages/react-aria-components/src/GridList.tsx (TypeScript)

Code before:
  });

  return (
    <section
      {...filterDOMProps(props as any)}
      {...rowGroupProps}
      {...renderProps}
      ref={ref}>
      <Provider
        values={[
          [HeaderContext, {...headingProps, ref: headingRef}],
          [GridListHeaderContext, {...rowProps}]
        ]}>
        <CollectionBranch
          collection={state.collection}
          parent={section}
          renderDropIndicator={useRenderDropIndicator(dragAndDropHooks, dropState)} />
      </Provider>
    </section>
  );
}

const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null);

export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) {
  [props, ref] = useContextProps(props, ref, HeaderContext);
  let rowProps = useContext(GridListHeaderContext);

  return (
    <div {...rowProps} >
      <header className="react-aria-Header" {...props} ref={ref}>
        {props.children}
      </header>
    </div>
  );
});


/**
 * A GridListSection represents a section within a GridList.
 */
export const GridListSection = /*#__PURE__*/ createBranchComponent('section', GridListSectionInner);


Review comment (suggestion):
with ListBox, you can just use a normal `Header`, but with GridList, in order to follow correct aria-pattern, the header must be inside a div with a `role=row`, hence why i've created a new component called GridListHeader. are we okay with that?

from WAI-ARIA: 

Each cell is either a DOM descendant of or owned by a row element and has one of the following roles:

- [columnheader](https://w3c.github.io/aria/#columnheader) if the cell contains a title or header information for the column.
- [rowheader](https://w3c.github.io/aria/#rowheader) if the cell contains title or header information for the row.
- [gridcell](https://w3c.github.io/aria/#gridcell) if the cell does not contain column or row header information.


Code after:
    values: {}
  });

  let DOMProps = filterDOMProps(props as any, {global: true});
  delete DOMProps.id;

  return (
    <section
      {...mergeProps(DOMProps, renderProps, rowGroupProps)}
      ref={ref}>
      <Provider
        values={[
          [HeaderContext, {...rowProps, ref: headingRef}],
          [GridListHeaderContext, {...rowHeaderProps}]
        ]}>
        <CollectionBranch
          collection={state.collection}
          parent={item} />
      </Provider>
    </section>
  );
});

const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null);

export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) {
  [props, ref] = useContextProps(props, ref, HeaderContext);
  let rowHeaderProps = useContext(GridListHeaderContext);

  return (
    <header {...props} ref={ref}>
      <div {...rowHeaderProps} style={{display: 'contents'}}>
        {props.children}
      </div>
    </header>
  );
});
Repository: adobe/react-spectrum
File: packages/@react-aria/gridlist/src/useGridListSection.ts (TypeScript)

Code before:

  /** Props for the grid's row group element. */
  rowGroupProps: DOMAttributes
}

/**
 * Provides the behavior and accessibility implementation for a section in a grid list.
 * See `useGridList` for more details about grid list.
 * @param props - Props for the section.
 */
export function useGridListSection(props: AriaGridListSectionProps): GridListSectionAria {
  let {heading, 'aria-label': ariaLabel} = props;
  let headingId = useId();
  
  return {
    rowProps: {
      role: 'row'
    },
    headingProps: heading ? {
      id: headingId,
      role: 'rowheader'
    } : {},
    rowGroupProps: {
      role: 'rowgroup',
      'aria-label': ariaLabel,
      'aria-labelledby': heading ? headingId : undefined
    }
  };
}


Review comment (suggestion):
I think you need to use useLabels to handle merging the aria-label and aria-labelledby in case both exist

Code after:
  rowGroupProps: DOMAttributes
}

/**
 * Provides the behavior and accessibility implementation for a section in a grid list.
 * See `useGridList` for more details about grid list.
 * @param props - Props for the section.
 */
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function useGridListSection<T>(props: AriaGridListSectionProps, state: ListState<T>, ref: RefObject<HTMLElement | null>): GridListSectionAria {
  let {'aria-label': ariaLabel} = props;
  let headingId = useSlotId();
  let labelProps = useLabels({
    'aria-label': ariaLabel,
    'aria-labelledby': headingId
  });

  return {
    rowProps: {
      role: 'row'
    },
    rowHeaderProps: {
      id: headingId,
      role: 'rowheader'
    },
    rowGroupProps: {
      role: 'rowgroup',
      ...labelProps
    }
  };
}
Repository: adobe/react-spectrum
File: packages/@react-aria/gridlist/src/useGridListSection.ts (TypeScript)

Code before:
export interface AriaGridListSectionProps {
  /** The heading for the section. */
  heading?: ReactNode,
  /** An accessibility label for the section. Required if `heading` is not present. */
  'aria-label'?: string
}

export interface GridListSectionAria {
  /** Props for the wrapper list item. */
  rowProps: DOMAttributes,

  /** Props for the heading element, if any. */
  headingProps: DOMAttributes,

  /** Props for the grid's row group element. */
  rowGroupProps: DOMAttributes
}

/**
 * Provides the behavior and accessibility implementation for a section in a grid list.
 * See `useGridList` for more details about grid list.
 * @param props - Props for the section.
 */
export function useGridListSection(props: AriaGridListSectionProps): GridListSectionAria {
  let {heading, 'aria-label': ariaLabel} = props;
  let headingId = useId();
  let labelProps = useLabels({
    'aria-label': ariaLabel,
    'aria-labelledby': headingId
  });
  
  return {
    rowProps: {
      role: 'row'
    },
    headingProps: heading ? {
      id: headingId,
      role: 'rowheader'
    } : {},
    rowGroupProps: {
      role: 'rowgroup',
      ...labelProps
    }
  };
}


Review comment (suggestion):
nit, but I wonder if we can omit passing the heading to the hook and instead leverage `useSlotId` to generate this `headingId`? That way either the user spreads `headingProps` onto an element and thus the `id` + `aria-labelledby` stick around or they don't and the `id` doesn't get used

Code after:
export interface AriaGridListSectionProps {
  /** An accessibility label for the section. Required if `heading` is not present. */
  'aria-label'?: string
}

export interface GridListSectionAria {
  /** Props for the wrapper list item. */
  rowProps: DOMAttributes,

  /** Props for the heading element, if any. */
  rowHeaderProps: DOMAttributes,

  /** Props for the grid's row group element. */
  rowGroupProps: DOMAttributes
}

/**
 * Provides the behavior and accessibility implementation for a section in a grid list.
 * See `useGridList` for more details about grid list.
 * @param props - Props for the section.
 */
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function useGridListSection<T>(props: AriaGridListSectionProps, state: ListState<T>, ref: RefObject<HTMLElement | null>): GridListSectionAria {
  let {'aria-label': ariaLabel} = props;
  let headingId = useSlotId();
  let labelProps = useLabels({
    'aria-label': ariaLabel,
    'aria-labelledby': headingId
  });

  return {
    rowProps: {
      role: 'row'
    },
    rowHeaderProps: {
      id: headingId,
      role: 'rowheader'
    },
    rowGroupProps: {
      role: 'rowgroup',
      ...labelProps
    }
  };
}
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/NumberField.tsx (TypeScript)

Code before:
import {FormContext} from './Form';
import {GlobalDOMAttributes, HelpTextProps, SpectrumLabelableProps} from '@react-types/shared';
import {pressScale} from './pressScale';
import {TextFieldRef} from '@react-types/textfield';
import {useButton, useFocusRing, useHover} from 'react-aria';
import {useSpectrumContextProps} from './useSpectrumContextProps';


export interface NumberFieldProps extends
  Omit<AriaNumberFieldProps, 'children' | 'className' | 'style' | keyof GlobalDOMAttributes>,
  StyleProps,
  SpectrumLabelableProps,
  HelpTextProps {
  /**
   * Whether to hide the increment and decrement buttons.
   * @default false
   */
  hideStepper?: boolean,
  /**
   * The size of the NumberField.
   *
   * @default 'M'
   */
  size?: 'S' | 'M' | 'L' | 'XL',
  /**
   * Temporary text that occupies the text input when it is empty.
   */
  placeholder?: string
}

export const NumberFieldContext = createContext<ContextValue<Partial<NumberFieldProps>, TextFieldRef>>(null);

const inputButton = style<ButtonRenderProps & {size: 'S' | 'M' | 'L' | 'XL', type: 'decrement' | 'increment'}>({
  ...controlBorderRadius('sm'),
  display: 'flex',
  outlineStyle: 'none',
  textAlign: 'center',
  borderStyle: 'none',
  alignItems: 'center',
  justifyContent: 'center',
  width: {
    size: {
      S: 16,
      M: 20,
      L: 24,
      XL: 32
    }
  },
  height: 'auto',
  marginStart: {
    default: 'text-to-control',

Review comment (suggestion):
inherit (extend) from `InputProps`? hopefully then it comes withe the MDN description and link?

and if it doesn't, it can be overridden there to provide that

Code after:
import {filterDOMProps, mergeProps, mergeRefs} from '@react-aria/utils';
import {FormContext} from './Form';
import {GlobalDOMAttributes, HelpTextProps, SpectrumLabelableProps} from '@react-types/shared';
import {pressScale} from './pressScale';
import {TextFieldRef} from '@react-types/textfield';
import {useButton, useFocusRing, useHover} from 'react-aria';
import {useSpectrumContextProps} from './useSpectrumContextProps';


export interface NumberFieldProps extends
  Omit<AriaNumberFieldProps, 'children' | 'className' | 'style' | keyof GlobalDOMAttributes>,
  StyleProps,
  SpectrumLabelableProps,
  HelpTextProps,
  Pick<InputProps, 'placeholder'> {
  /**
   * Whether to hide the increment and decrement buttons.
   * @default false
   */
  hideStepper?: boolean,
  /**
   * The size of the NumberField.
   *
   * @default 'M'
   */
  size?: 'S' | 'M' | 'L' | 'XL'
}

export const NumberFieldContext = createContext<ContextValue<Partial<NumberFieldProps>, TextFieldRef>>(null);

const inputButton = style<ButtonRenderProps & {size: 'S' | 'M' | 'L' | 'XL', type: 'decrement' | 'increment'}>({
  ...controlBorderRadius('sm'),
  display: 'flex',
  outlineStyle: 'none',
  textAlign: 'center',
  borderStyle: 'none',
  alignItems: 'center',
  justifyContent: 'center',
  width: {
    size: {
      S: 16,
      M: 20,
      L: 24,
      XL: 32
    }
  },
  height: 'auto',
  marginStart: {
    default: 'text-to-control',
    type: {
      increment: 0
Repository: adobe/react-spectrum
File: packages/@react-aria/overlays/src/useOverlayPosition.ts (TypeScript)

Code before:

  let close = useCallback(() => {
    if (!isResizing.current) {
      onClose?.();
    }
  }, [onClose, isResizing]);

  // When scrolling a parent scrollable region of the trigger (other than the body),
  // we hide the popover. Otherwise, its position would be incorrect.
  useCloseOnScroll({
    triggerRef: targetRef,
    isOpen,
    onClose: onClose && close
  });

  return {
    overlayProps: {
      style: {
        position: 'absolute',
        zIndex: 100000, // should match the z-index in ModalTrigger
        ...position?.position,
        maxHeight: position?.maxHeight ?? '100vh'
      }
    },
    placement: position?.placement ?? null,
    triggerOrigin: position?.triggerOrigin ?? null,
    arrowProps: {
      'aria-hidden': 'true',
      role: 'presentation',
      style: {
        left: position?.arrowOffsetLeft,
        top: position?.arrowOffsetTop
      }
    },
    updatePosition
  };
}

function useResize(onResize) {
  useLayoutEffect(() => {
    window.addEventListener('resize', onResize, false);
    return () => {
      window.removeEventListener('resize', onResize, false);
    };
  }, [onResize]);
}

function translateRTL(position, direction) {
  if (direction === 'rtl') {
    return position.replace('start', 'right').replace('end', 'left');
  }

Review comment (suggestion):
```suggestion
        insetInlineStart: !position ? 0 : undefined,
```

Code after:
    return () => {
      visualViewport?.removeEventListener('resize', onResize);
      visualViewport?.removeEventListener('scroll', onScroll);
    };
  }, [updatePosition]);

  let close = useCallback(() => {
    if (!isResizing.current) {
      onClose?.();
    }
  }, [onClose, isResizing]);

  // When scrolling a parent scrollable region of the trigger (other than the body),
  // we hide the popover. Otherwise, its position would be incorrect.
  useCloseOnScroll({
    triggerRef: targetRef,
    isOpen,
    onClose: onClose && close
  });

  return {
    overlayProps: {
      style: {
        position: position ? 'absolute' : 'fixed',
        top: !position ? 0 : undefined,
        left: !position ? 0 : undefined,
        zIndex: 100000, // should match the z-index in ModalTrigger
        ...position?.position,
        maxHeight: position?.maxHeight ?? '100vh'
      }
    },
    placement: position?.placement ?? null,
    triggerOrigin: position?.triggerOrigin ?? null,
    arrowProps: {
      'aria-hidden': 'true',
      role: 'presentation',
      style: {
        left: position?.arrowOffsetLeft,
        top: position?.arrowOffsetTop
      }
    },
    updatePosition
  };
}

function useResize(onResize) {
  useLayoutEffect(() => {
    window.addEventListener('resize', onResize, false);
    return () => {
      window.removeEventListener('resize', onResize, false);
    };
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/TableView.tsx (TypeScript)

Code before:
  }

  return (
    <RACTableBody
      // @ts-ignore
      ref={domRef}
      className={style({height: 'full'})}
      {...props}
      renderEmptyState={emptyRender}
      dependencies={[loadingState]}>
      {renderer}
    </RACTableBody>
  );
});

const cellFocus = {
  outlineStyle: {
    default: 'none',
    isFocusVisible: 'solid'
  },
  outlineOffset: -2,
  outlineWidth: 2,
  outlineColor: 'focus-ring',
  borderRadius: '[6px]',
  pointerEvents: 'none'
} as const;

function CellFocusRing() {
  return <div role="presentation" className={style({...cellFocus, position: 'absolute', inset: 0})({isFocusVisible: true})} />;
}

const columnStyles = style({
  height: 'inherit',
  boxSizing: 'border-box',
  color: {
    default: baseColor('neutral'),
    forcedColors: 'ButtonText'
  },
  paddingX: {
    default: 16,
    isMenu: 0
  },
  textAlign: {
    align: {
      start: 'start',
      center: 'center',
      end: 'end'
    }
  },
  outlineStyle: 'none',
  position: 'relative',

Review comment (bug):
what what the reason for the `pointerEvents: 'none'` here? I'm sure its fine, but curious what the original bug was, just a press on the focus ring itself?

Code after:
    );
  }

  return (
    <RACTableBody
      // @ts-ignore
      ref={domRef}
      className={style({height: 'full'})}
      {...props}
      renderEmptyState={emptyRender}
      dependencies={[loadingState]}>
      {renderer}
    </RACTableBody>
  );
});

const cellFocus = {
  outlineStyle: {
    default: 'none',
    isFocusVisible: 'solid'
  },
  outlineOffset: -2,
  outlineWidth: 2,
  outlineColor: 'focus-ring',
  borderRadius: '[6px]',
  pointerEvents: 'none'
} as const;

function CellFocusRing() {
  return <div role="presentation" className={style({...cellFocus, position: 'absolute', inset: 0})({isFocusVisible: true})} />;
}

const columnStyles = style({
  height: 'inherit',
  boxSizing: 'border-box',
  color: {
    default: baseColor('neutral'),
    forcedColors: 'ButtonText'
  },
  paddingX: {
    default: 16,
    isMenu: 0
  },
  textAlign: {
    align: {
      start: 'start',
      center: 'center',
      end: 'end'
    }
  },
  outlineStyle: 'none',
Repository: adobe/react-spectrum
File: packages/@react-aria/tooltip/src/useTooltipTrigger.ts (TypeScript)

Code before:
import {mergeProps, useId} from '@react-aria/utils';
import {TooltipTriggerProps} from '@react-types/tooltip';
import {TooltipTriggerState} from '@react-stately/tooltip';
import {useEffect, useRef} from 'react';

export interface TooltipTriggerAria {
  /**
   * Props for the trigger element.
   */
  triggerProps: DOMAttributes,

  /**
   * Props for the overlay container element.
   */
  tooltipProps: DOMAttributes
}

/**
 * Provides the behavior and accessibility implementation for a tooltip trigger, e.g. a button
 * that shows a description when focused or hovered.
 */
export function useTooltipTrigger(props: TooltipTriggerProps, state: TooltipTriggerState, ref: RefObject<FocusableElement | null>) : TooltipTriggerAria {
  let {
    isDisabled,
    trigger,
    closeOnPress
  } = props;

  let tooltipId = useId();

  let isHovered = useRef(false);
  let isFocused = useRef(false);

  let handleShow = () => {
    if (isHovered.current || isFocused.current) {
      state.open(isFocused.current);
    }
  };

  let handleHide = (immediate?: boolean) => {
    if (!isHovered.current && !isFocused.current) {
      state.close(immediate);
    }
  };

  useEffect(() => {
    let onKeyDown = (e) => {
      if (ref && ref.current) {
        // Escape after clicking something can give it keyboard focus
        // dismiss tooltip on esc key press
        if (e.key === 'Escape') {

Review comment (suggestion):
Don't want to change the default behaviour for people who are using the hooks.
```suggestion
    closeOnPress = true
```

Code after:
import {mergeProps, useId} from '@react-aria/utils';
import {TooltipTriggerProps} from '@react-types/tooltip';
import {TooltipTriggerState} from '@react-stately/tooltip';
import {useEffect, useRef} from 'react';

export interface TooltipTriggerAria {
  /**
   * Props for the trigger element.
   */
  triggerProps: DOMAttributes,

  /**
   * Props for the overlay container element.
   */
  tooltipProps: DOMAttributes
}

/**
 * Provides the behavior and accessibility implementation for a tooltip trigger, e.g. a button
 * that shows a description when focused or hovered.
 */
export function useTooltipTrigger(props: TooltipTriggerProps, state: TooltipTriggerState, ref: RefObject<FocusableElement | null>) : TooltipTriggerAria {
  let {
    isDisabled,
    trigger,
    closeOnPress = true
  } = props;

  let tooltipId = useId();

  let isHovered = useRef(false);
  let isFocused = useRef(false);

  let handleShow = () => {
    if (isHovered.current || isFocused.current) {
      state.open(isFocused.current);
    }
  };

  let handleHide = (immediate?: boolean) => {
    if (!isHovered.current && !isFocused.current) {
      state.close(immediate);
    }
  };

  useEffect(() => {
    let onKeyDown = (e) => {
      if (ref && ref.current) {
        // Escape after clicking something can give it keyboard focus
        // dismiss tooltip on esc key press
        if (e.key === 'Escape') {