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

Code before:
  } else if (name) {
    let data = selectData.get(state) || {};
    let {validationBehavior} = data;

    let inputProps: InputHTMLAttributes<HTMLInputElement> = {
      type: 'hidden',
      autoComplete: selectProps.autoComplete,
      name,
      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}
          style={{display: 'none'}}
          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 (suggestion):
I think useFormValidation should handle this. The trigger shouldn't necessarily be focused on invalid - the first invalid field in the form should be focused, which might not be the select.

We may need to attach the `selectRef` to the input for this to work.

Code after:
                    {item.textValue}
                  </option>
                );
              }
            })}
          </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;
}
Repository: adobe/react-spectrum
File: packages/@react-stately/datepicker/src/useDateFieldState.ts (TypeScript)

Code before:
  /** Gets a formatter based on state's props. */
  getDateFormatter(locale: string, formatOptions: FormatterOptions): DateFormatter
}

const EDITABLE_SEGMENTS = {
  year: true,
  month: true,
  day: true,
  hour: true,
  minute: true,
  second: true,
  dayPeriod: true,
  era: true
};

const PAGE_STEP = {
  year: 5,
  month: 2,
  day: 7,
  hour: 2,
  minute: 15,
  second: 15
};

// Node seems to convert everything to lowercase...
const TYPE_MAPPING = {
  dayperiod: 'dayPeriod'
};

export interface DateFieldStateOptions<T extends DateValue = DateValue> extends DatePickerProps<T> {
  /**
   * The maximum unit to display in the date field.
   * @default 'year'
   */
  maxGranularity?: 'year' | 'month' | Granularity,
  /** The locale to display and edit the value according to. */
  locale: string,
  /**
   * A function that creates a [Calendar](../internationalized/date/Calendar.html)
   * object for a given calendar identifier. Such a function may be imported from the
   * `@internationalized/date` package, or manually implemented to include support for
   * only certain calendars.
   */
  createCalendar: (name: CalendarIdentifier) => Calendar
}

/**
 * Provides state management for a date field component.
 * A date field allows users to enter and edit date and time values using a keyboard.
 * Each part of a date value is displayed in an individually editable segment.
 */

Review comment (suggestion):
Are you trying on node 22? I'm not seeing the issue in my local node environment
![Screenshot 2025-05-22 at 5 40 14 pm](https://github.com/user-attachments/assets/d98f0405-6c50-4014-b4cf-00ba5500e994)


Code after:
  /** Gets a formatter based on state's props. */
  getDateFormatter(locale: string, formatOptions: FormatterOptions): DateFormatter
}

const EDITABLE_SEGMENTS = {
  year: true,
  month: true,
  day: true,
  hour: true,
  minute: true,
  second: true,
  dayPeriod: true,
  era: true
};

const PAGE_STEP = {
  year: 5,
  month: 2,
  day: 7,
  hour: 2,
  minute: 15,
  second: 15
};

const TYPE_MAPPING = {
  // Node seems to convert everything to lowercase...
  dayperiod: 'dayPeriod',
  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatToParts#named_years
  relatedYear: 'year',
  yearName: 'literal', // not editable
  unknown: 'literal'
};

export interface DateFieldStateOptions<T extends DateValue = DateValue> extends DatePickerProps<T> {
  /**
   * The maximum unit to display in the date field.
   * @default 'year'
   */
  maxGranularity?: 'year' | 'month' | Granularity,
  /** The locale to display and edit the value according to. */
  locale: string,
  /**
   * A function that creates a [Calendar](../internationalized/date/Calendar.html)
   * object for a given calendar identifier. Such a function may be imported from the
   * `@internationalized/date` package, or manually implemented to include support for
   * only certain calendars.
   */
  createCalendar: (name: CalendarIdentifier) => Calendar
}

/**
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/Tabs.tsx (TypeScript)

Code before:

function TabListInner<T extends object>(props: TabListProps<T>) {
  let {
    tablistRef,
    tablineRef,
    density,
    isDisabled,
    disabledKeys,
    orientation,
    labelBehavior,
    selectedKey,
    'aria-label': ariaLabel,
    'aria-labelledby': ariaLabelledBy
  } = useContext(InternalTabsContext) ?? {};

  return (
    <div
      style={props.UNSAFE_style}
      className={(props.UNSAFE_className || '') + style({position: 'relative'}, getAllowedOverrides())(null, props.styles)}>
      <RACTabList
        {...props}
        aria-label={ariaLabel}
        aria-labelledby={ariaLabelledBy}
        ref={tablistRef}
        className={renderProps => tablist({...renderProps, labelBehavior, density})} />
      <TabLine
        tablistRef={tablistRef}
        tablineRef={tablineRef}
        disabledKeys={disabledKeys}
        isDisabled={isDisabled}
        selectedKey={selectedKey}
        orientation={orientation}
        density={density} />
    </div>
  );
}

interface TabLineProps {
  disabledKeys: Iterable<Key> | undefined,
  isDisabled: boolean | undefined,
  selectedKey?: Key | null,
  orientation?: Orientation,
  density?: 'compact' | 'regular',
  tablistRef?: RefObject<HTMLDivElement | null>,
  tablineRef?: RefObject<HTMLDivElement | null>
}

const selectedIndicator = style<{isDisabled: boolean, orientation?: Orientation}>({
  position: 'absolute',
  backgroundColor: {
    default: 'neutral',

Review comment (suggestion):
I think the idea would be to move TabLine to be _inside_ the Tab rather than a sibling of the TabList. This way you can remove all the resize observers and measurements and just set it to have `width: 100%`. Then the browser will handle all the layout for you, and you only need to measure when an animation is about to happen. This will make it work with server rendering as well because we won't need JS to measure it on the initial render. See SegmentedControl for how this is implemented.

Code after:
    orientation: {
      horizontal: '[2px]'
    }
  },
  width: {
    default: 'full',
    orientation: {
      vertical: '[2px]'
    }
  },
  bottom: {
    default: 0
  },
  top: {
    orientation: {
      vertical: 0
    }
  },
  left: {
    orientation: {
      horizontal: 0
    }
  },
  insetStart: {
    orientation: {
      vertical: -12
    }
  },
  borderStyle: 'none',
  borderRadius: 'full'
});

const tab = style<TabRenderProps & {density?: 'compact' | 'regular', labelBehavior?: 'show' | 'hide'}>({
  ...focusRing(),
  display: 'flex',
  color: {
    default: baseColor('neutral-subdued'),
    isSelected: baseColor('neutral'),
    isDisabled: 'disabled',
    forcedColors: {
      isSelected: 'Highlight',
      isDisabled: 'GrayText'
    }
  },
  borderRadius: 'sm',
  gap: 'text-to-visual',
  height: {
    density: {
      compact: 32,
      regular: 48
    }
Repository: adobe/react-spectrum
File: packages/@react-aria/datepicker/stories/useDatePicker.stories.tsx (TypeScript)

Code before:
 */

import {createCalendar, parseDate} from '@internationalized/date';
import React, {useRef} from 'react';
import {useDateField} from '../src';
import {useDateFieldState} from '@react-stately/datepicker';
import {useLocale} from '@react-aria/i18n';

export function ProgrammaticSetValueExample() {
  let {locale} = useLocale();
  let state = useDateFieldState({locale, createCalendar});
  let ref = useRef<HTMLDivElement>(null);
  let {fieldProps} = useDateField({'aria-label': 'Date'}, state, ref);
  return (
    <div>
      <div {...fieldProps} ref={ref} data-testid="field">
        {state.segments.map((seg, i) => <span key={i}>{seg.text}</span>)}
      </div>
      <button onClick={() => state.setValue(parseDate('2020-01-01'))} data-testid="set">Set</button>
    </div>
  );
}

export default {
  title: 'Date and Time/useDatePicker'
};


Review comment (suggestion):
```suggestion
function ProgrammaticSetValueExampleRender(): ReactElement {
  let {locale} = useLocale();
  let state = useDateFieldState({locale, createCalendar});
  let ref = useRef<HTMLDivElement>(null);
  let {fieldProps} = useDateField({'aria-label': 'Date'}, state, ref);
  return (
    <div>
      <div {...fieldProps} ref={ref} data-testid="field">
        {state.segments.map((seg, i) => <span key={i}>{seg.text}</span>)}
      </div>
      <button onClick={() => state.setValue(parseDate('2020-01-01'))} data-testid="set">Set</button>
    </div>
  );
}

export default {
  title: 'Date and Time/useDatePicker'
} as Meta<typeof ProgrammaticSetValueExampleRender>;

type Story = StoryObj<typeof ProgrammaticSetValueExampleRender>;

export const ProgrammaticSetValueExample: StoryObj = {
  render: () => <ProgrammaticSetValueExampleRender />
};
```

so this doesn't create conflict with my prs

Code after:
 */

import {createCalendar, parseDate} from '@internationalized/date';
import {Meta, StoryObj} from '@storybook/react';
import React, {ReactElement, useRef} from 'react';
import {useDateField} from '../src';
import {useDateFieldState} from '@react-stately/datepicker';
import {useLocale} from '@react-aria/i18n';

export function ProgrammaticSetValueExampleRender(): ReactElement {
  let {locale} = useLocale();
  let state = useDateFieldState({locale, createCalendar});
  let ref = useRef<HTMLDivElement>(null);
  let {fieldProps} = useDateField({'aria-label': 'Date'}, state, ref);
  return (
    <div>
      <div {...fieldProps} ref={ref} data-testid="field">
        {state.segments.map((seg, i) => <span key={i}>{seg.text}</span>)}
      </div>
      <button onClick={() => state.setValue(parseDate('2020-01-01'))} data-testid="set">Set</button>
    </div>
  );
}

export default {
  title: 'Date and Time/useDatePicker',
  excludeStories: ['ProgrammaticSetValueExampleRender']
} as Meta<typeof ProgrammaticSetValueExampleRender>;

type Story = StoryObj<typeof ProgrammaticSetValueExampleRender>;

export const ProgrammaticSetValueExample: Story = {
  render: () => <ProgrammaticSetValueExampleRender />
};
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/ProgressCircle.tsx (TypeScript)

Code before:
export const ProgressCircleContext = createContext<ContextValue<Partial<ProgressCircleProps>, DOMRefValue<HTMLDivElement>>>(null);

// Double check the types passed to each style, may not need all for each
const wrapper = style<ProgressCircleStyleProps>({
  ...staticColor(),
  size: {
    default: 32,
    size: {
      S: 16,
      L: 64
    }
  },
  aspectRatio: 'square'
}, getAllowedOverrides({height: true}));

const track = style({
  stroke: {
    default: 'gray-300',
    isStaticColor: 'transparent-overlay-300',
    forcedColors: 'Background'
  },
  strokeWidth: {
    default: `[${pxToRem(3)}]`,
    size: {
      S: 2,
      L: 4
    },
    forcedColors: {
      default: 2,
      size: {
        S: 1,
        L: `[${pxToRem(3)}]`
      }
    }
  }
});

const fill = style({
  stroke: {
    default: 'blue-900',
    isStaticColor: 'transparent-overlay-900',
    forcedColors: 'ButtonText'
  },
  rotate: -90,
  transformOrigin: 'center',
  strokeWidth: {
    default: `[${pxToRem(3)}]`,
    size: {
      S: 2,
      L: 4
    }

Review comment (suggestion):
I don't think these other values are converted to rems in the theme. Borders and strokes use pixels by default, but we chose to use rems for progress circle. So I think you will need to manually convert these too.

Code after:
export const ProgressCircleContext = createContext<ContextValue<Partial<ProgressCircleProps>, DOMRefValue<HTMLDivElement>>>(null);

// Double check the types passed to each style, may not need all for each
const wrapper = style<ProgressCircleStyleProps>({
  ...staticColor(),
  size: {
    default: 32,
    size: {
      S: 16,
      L: 64
    }
  },
  aspectRatio: 'square'
}, getAllowedOverrides({height: true}));

const track = style({
  stroke: {
    default: 'gray-300',
    isStaticColor: 'transparent-overlay-300',
    forcedColors: 'Background'
  },
  strokeWidth: {
    default: `[${pxToRem(3)}]`,
    size: {
      S: `[${pxToRem(2)}]`,
      L: `[${pxToRem(4)}]`
    },
    forcedColors: {
      default: `[${pxToRem(2)}]`,
      size: {
        S: `[${pxToRem(1)}]`,
        L: `[${pxToRem(3)}]`
      }
    }
  }
});

const fill = style({
  stroke: {
    default: 'blue-900',
    isStaticColor: 'transparent-overlay-900',
    forcedColors: 'ButtonText'
  },
  rotate: -90,
  transformOrigin: 'center',
  strokeWidth: {
    default: `[${pxToRem(3)}]`,
    size: {
      S: `[${pxToRem(2)}]`,
      L: `[${pxToRem(4)}]`
    }
Repository: adobe/react-spectrum
File: packages/@react-aria/i18n/src/useDefaultLocale.ts (TypeScript)

Code before:
}

/**
 * Returns the current browser/system language, and updates when it changes.
 */
export function useDefaultLocale(): Locale {
  let [defaultLocale, setDefaultLocale] = useState(currentLocale);

  useEffect(() => {
    if (listeners.size === 0) {
      window.addEventListener('languagechange', updateLocale);
    }

    listeners.add(setDefaultLocale);

    return () => {
      listeners.delete(setDefaultLocale);
      if (listeners.size === 0) {
        window.removeEventListener('languagechange', updateLocale);
      }
    };
  }, []);

  return defaultLocale;
}


Review comment (refactor):
So I don't think we want to completely remove this. Instead, we want to split the I18nProvider's internals into two components, one which calls the useDefaultLocale and one which doesn't. This is how to conditionally call hooks since they can't be in an if statement.

Which component we call inside the I18nProvider should be determined by if the `locale` prop is passed to the I18nProvider.

see proposal here: https://github.com/adobe/react-spectrum/issues/7344#issuecomment-2864308424

Side note, if you've done it this way instead intentionally, can you explain your decision?

Code after:
  }
}

/**
 * Returns the current browser/system language, and updates when it changes.
 */
export function useDefaultLocale(): Locale {
  let isSSR = useIsSSR();
  let [defaultLocale, setDefaultLocale] = useState(currentLocale);

  useEffect(() => {
    if (listeners.size === 0) {
      window.addEventListener('languagechange', updateLocale);
    }

    listeners.add(setDefaultLocale);

    return () => {
      listeners.delete(setDefaultLocale);
      if (listeners.size === 0) {
        window.removeEventListener('languagechange', updateLocale);
      }
    };
  }, []);

  // We cannot determine the browser's language on the server, so default to
  // en-US. This will be updated after hydration on the client to the correct value.
  if (isSSR) {
    return {
      locale: 'en-US',
      direction: 'ltr'
    };
  }

  return defaultLocale;
}
Repository: adobe/react-spectrum
File: packages/@react-aria/i18n/src/context.tsx (TypeScript)

Code before:
    <I18nContext.Provider value={defaultLocale}>
      {children}
    </I18nContext.Provider>
  );
}

/**
 * Provides the locale for the application to all child components.
 */
export function I18nProvider(props: I18nProviderProps): JSX.Element {
  let {locale, children} = props;

  // Conditionally render different components to avoid calling useDefaultLocale.
  // This is necessary because useDefaultLocale triggers a re-render.
  if (locale) {
    return <I18nProviderWithLocale locale={locale} children={children} />;
  }

  return <I18nProviderWithDefaultLocale children={children} />;
}

/**
 * Returns the current locale and layout direction.
 */
export function useLocale(): Locale {
  let defaultLocale = getDefaultLocale();
  let context = useContext(I18nContext);
  return context || defaultLocale;
}


Review comment (suggestion):
I think this one has to stay so the locale doesn't become stale?

Maybe it'd be a good idea to add some tests making use of `window.addEventListener('languagechange'` it doesn't look like we have any right now.

Code after:
    <I18nContext.Provider value={defaultLocale}>
      {children}
    </I18nContext.Provider>
  );
}

/**
 * Provides the locale for the application to all child components.
 */
export function I18nProvider(props: I18nProviderProps): JSX.Element {
  let {locale, children} = props;

  // Conditionally render different components to avoid calling useDefaultLocale.
  // This is necessary because useDefaultLocale triggers a re-render.
  if (locale) {
    return <I18nProviderWithLocale locale={locale} children={children} />;
  }

  return <I18nProviderWithDefaultLocale children={children} />;
}

/**
 * Returns the current locale and layout direction.
 */
export function useLocale(): Locale {
  let defaultLocale = useDefaultLocale();
  let context = useContext(I18nContext);
  return context || defaultLocale;
}
Repository: adobe/react-spectrum
File: packages/@react-stately/data/src/useListData.ts (TypeScript)

Code before:
        ...state,
        selectedKeys
      }));
    },
    addSelectedKeys(selectedKeys: Selection) {
      dispatch(state => {
        if (state.selectedKeys === 'all') {
          return state;
        }
        if (selectedKeys === 'all') {
          return {
            ...state,
            selectedKeys: 'all'
          };
        }
        let validSelectedKeys = new Set(selectedKeys);
        const allValidKeys = new Set(state.items.map(getKey!));
        for (let key of validSelectedKeys) {
          if (!allValidKeys.has(key)) {
            validSelectedKeys.delete(key);
          }
        }
        let selection: Selection = new Set([...state.selectedKeys, ...validSelectedKeys]);
        if (selection.size === state.items.length) {
          selection = 'all';
        }

        return {
          ...state,
          selectedKeys: selection
        };
      });
    },
    removeSelectedKeys(selectedKeys: Selection) {
      dispatch(state => {
        if (selectedKeys === 'all') {
          return {
            ...state,
            selectedKeys: new Set()
          };
        }

        let selection: Selection = state.selectedKeys  === 'all' ? new Set(state.items.map(getKey!)) : new Set(state.selectedKeys);
        for (let key of selectedKeys) {
          selection.delete(key);
        }
        if (selection.size === state.items.length) {
          selection = 'all';
        }
        return {
          ...state,

Review comment (suggestion):
This isn't quite the same. `'all'` is a special case as it means also, all potential items, even those that aren't added on the client yet. I think it's better to keep it as the set for now.

We'll see if others on the team agree since this isn't `useAsyncList`.

Code after:
        ...state,
        selectedKeys
      }));
    },
    addKeysToSelection(selectedKeys: Selection) {
      dispatch(state => {
        if (state.selectedKeys === 'all') {
          return state;
        }
        if (selectedKeys === 'all') {
          return {
            ...state,
            selectedKeys: 'all'
          };
        }

        return {
          ...state,
          selectedKeys: new Set([...state.selectedKeys, ...selectedKeys])
        };
      });
    },
    removeKeysFromSelection(selectedKeys: Selection) {
      dispatch(state => {
        if (selectedKeys === 'all') {
          return {
            ...state,
            selectedKeys: new Set()
          };
        }

        let selection: Selection = state.selectedKeys  === 'all' ? new Set(state.items.map(getKey!)) : new Set(state.selectedKeys);
        for (let key of selectedKeys) {
          selection.delete(key);
        }
        return {
          ...state,
          selectedKeys: selection
        };
      });
    },
    setFilterText(filterText: string) {
      dispatch(state => ({
        ...state,
        filterText
      }));
    },
    insert(index: number, ...values: T[]) {
      dispatch(state => insert(state, index, ...values));
    },
    insertBefore(key: Key, ...values: T[]) {