ronantakizawa/codereview-bench huggingface dataset:
Repository: expo/expo
File: packages/expo-modules-core/src/ts-declarations/SharedObject.ts (TypeScript)

Code before:
import type { EventEmitter, EventsMap } from './EventEmitter';

/**
 * Base class for all shared objects that extends the {@link EventEmitter} class.
 * The implementation is written in C++, installed through JSI and common for mobile platforms.
 */
export declare class SharedObject<TEventsMap extends EventsMap = Record<never, never>>
  extends EventEmitter<TEventsMap>
  implements EventEmitter<TEventsMap>
{
  /**
   * A function that detaches the JS and native objects to let the native object deallocate
   * before the JS object gets deallocated by the JS garbage collector. Any subsequent calls to native
   * functions of the object will throw an error as it is no longer associated with its native counterpart.
   *
   * In most cases, you should never need to use this function, except some specific performance-critical cases when
   * manual memory management makes sense and the native object is known to exclusively retain some native memory
   * (such as binary data or image bitmap). Before calling this function, you should ensure that nothing else will use
   * this object later on. Shared objects created by React hooks are usually automatically released in the effect's cleanup phase,
   * for example `useVideoPlayer` from `expo-video` and `useImage` from `expo-image`.
   */
  release(): void;
}


Review comment (suggestion):
```suggestion
   * for example: `useVideoPlayer()` from `expo-video` and `useImage()` from `expo-image`.
```

Since these are hooks from the respective libraries, let's use parentheses to suffix them. We follow this pattern, in general, when referencing a hook.

Code after:
import type { EventEmitter, EventsMap } from './EventEmitter';

/**
 * Base class for all shared objects that extends the {@link EventEmitter} class.
 * The implementation is written in C++, installed through JSI and common for mobile platforms.
 */
export declare class SharedObject<TEventsMap extends EventsMap = Record<never, never>>
  extends EventEmitter<TEventsMap>
  implements EventEmitter<TEventsMap>
{
  /**
   * A function that detaches the JS and native objects to let the native object deallocate
   * before the JS object gets deallocated by the JS garbage collector. Any subsequent calls to native
   * functions of the object will throw an error as it is no longer associated with its native counterpart.
   *
   * In most cases, you should never need to use this function, except some specific performance-critical cases when
   * manual memory management makes sense and the native object is known to exclusively retain some native memory
   * (such as binary data or image bitmap). Before calling this function, you should ensure that nothing else will use
   * this object later on. Shared objects created by React hooks are usually automatically released in the effect's cleanup phase,
   * for example: `useVideoPlayer()` from `expo-video` and `useImage()` from `expo-image`.
   */
  release(): void;
}
Repository: expo/expo
File: packages/expo-video/src/VideoPlayer.tsx (TypeScript)

Code before:
import { useReleasingSharedObject } from 'expo-modules-core';

import NativeVideoModule from './NativeVideoModule';
import { VideoSource } from './VideoPlayer.types';
import resolveAssetSource from './resolveAssetSource';

// TODO: Temporary solution until we develop a way of overriding prototypes that won't break the lazy loading of the module.
const replace = NativeVideoModule.VideoPlayer.prototype.replace;
NativeVideoModule.VideoPlayer.prototype.replace = function (source: VideoSource) {
  return replace.call(this, parseSource(source));
};

export default class VideoPlayer extends NativeVideoModule.VideoPlayer {
  constructor(source: VideoSource) {
    super(parseSource(source));
  }
}

/**
 * Creates a `VideoPlayer`, which will be automatically cleaned up when the component is unmounted.
 * @param source - A video source that is used to initialize the player.
 * @param setup - A function that allows setting up the player. It will run after the player is created.
 */
export function useVideoPlayer(
  source: VideoSource,
  setup?: (player: VideoPlayer) => void
): VideoPlayer {
  const parsedSource = parseSource(source);


Review comment (suggestion):
```suggestion
import type { VideoSource } from './VideoPlayer.types';
```

Code after:
import { useReleasingSharedObject } from 'expo-modules-core';

import NativeVideoModule from './NativeVideoModule';
import { VideoSource, VideoPlayer } from './VideoPlayer.types';
import resolveAssetSource from './resolveAssetSource';

// TODO: Temporary solution until we develop a way of overriding prototypes that won't break the lazy loading of the module.
const replace = NativeVideoModule.VideoPlayer.prototype.replace;
NativeVideoModule.VideoPlayer.prototype.replace = function (source: VideoSource) {
  return replace.call(this, parseSource(source));
};

/**
 * Creates a direct instance of `VideoPlayer` that doesn't release automatically.
 *
 * > **info** For most use cases you should use the [`useVideoPlayer`](#usevideoplayer) hook instead. See the [Using the VideoPlayer Directly](#using-the-videoplayer-directly) section for more details.
 * @param source
 */
export function createVideoPlayer(source: VideoSource): VideoPlayer {
  return new NativeVideoModule.VideoPlayer(parseSource(source));
}

/**
 * Creates a `VideoPlayer`, which will be automatically cleaned up when the component is unmounted.
 * @param source - A video source that is used to initialize the player.
 * @param setup - A function that allows setting up the player. It will run after the player is created.
 */
export function useVideoPlayer(
  source: VideoSource,
Repository: expo/expo
File: docs/ui/components/CollapsibleTalksGridWrapper/index.tsx (TypeScript)

Code before:
  items: Talk[];
  initialCount?: number;
};

export function CollapsibleTalksGridWrapper({ items, initialCount = 8 }: CollapsibleGridProps) {
  const [showAll, setShowAll] = useState(false);

  return (
    <>
      <TalkGridWrapper>
        {items.slice(0, showAll ? items.length : initialCount).map(item => (
          <TalkGridCell key={item.videoId ?? item.event} {...item} />
        ))}
      </TalkGridWrapper>

      {items.length > initialCount && !showAll && (
        <div className="mt-6 flex justify-center">
          <Button
            size="lg"
            theme="secondary"
            onClick={() => {
              setShowAll(true);
            }}
            rightSlot={<ArrowDownIcon className="icon-sm" />}>
            Show More
          </Button>
        </div>
      )}
    </>
  );
}


Review comment (suggestion):
```suggestion
          <Button
            theme="secondary"
            onClick={() => {
              setShowAll(true);
            }}>
            Show More
          </Button>
```

The down pointing arrow feels like the interaction would scroll the view, maybe let's stick to the label only. 

Also the button size seems a bit too big for an in-content us, usually those larger ones we use on marketing side. Would retain the default "md" sizing or even try to use `sm`.

Code after:
  initialCount?: number;
};

export function CollapsibleTalksGridWrapper({ items, initialCount = 9 }: CollapsibleGridProps) {
  const [showAll, setShowAll] = useState(false);

  return (
    <>
      <TalkGridWrapper>
        {items.slice(0, showAll ? items.length : initialCount).map(item => (
          <TalkGridCell key={item.videoId ?? item.event} {...item} />
        ))}
      </TalkGridWrapper>

      {items.length > initialCount && (
        <div className="mt-6 flex justify-center">
          <Button
            theme="secondary"
            onClick={() => {
              setShowAll(!showAll);
            }}>
            {showAll ? 'Show Less' : 'Show More'}
          </Button>
        </div>
      )}
    </>
  );
}
Repository: expo/expo
File: docs/ui/components/PageHeader/PageTitleButtons.tsx (TypeScript)

Code before:
        ...(sourceCodeUrl?.startsWith('https://github.com/expo/expo')
          ? [
              {
                icon: <Tag03Icon className="mt-0.5 text-icon-secondary" />,
                label: 'Changelog',
                tooltip: (
                  <>
                    View the changelog of <MONOSPACE>{packageName}</MONOSPACE> on GitHub
                  </>
                ),
                href: `${sourceCodeUrl}/CHANGELOG.md`,
              },
            ]
          : []),
        ...(packageName
          ? [
              {
                icon: <BuildIcon className="mt-0.5 text-icon-secondary" />,
                label: 'npm',
                tooltip: 'View package in npm registry',
                href: `https://www.npmjs.com/package/${packageName}`,
              },
            ]
          : []),
      ].map(({ label, icon, tooltip, href }) => (
        <Tooltip.Root key={label} delayDuration={500}>
          <Tooltip.Trigger asChild>
            <Button
              theme="quaternary"
              className="min-h-[48px] min-w-[60px] justify-center px-2 max-xl-gutters:min-h-[unset]"
              openInNewTab
              href={href}>
              <div
                className={mergeClasses(
                  'flex flex-col items-center',
                  'max-xl-gutters:flex-row max-xl-gutters:gap-1.5'
                )}>
                {icon}
                <FOOTNOTE crawlable={false} theme="secondary">
                  {label}
                </FOOTNOTE>
              </div>
            </Button>
          </Tooltip.Trigger>
          <Tooltip.Content className="max-w-[300px]">
            <FOOTNOTE>{tooltip}</FOOTNOTE>
          </Tooltip.Content>
        </Tooltip.Root>
      ))}
    </>
  );

Review comment (refactor):
Also, might be nice to extract the button to a separate component, and pass the pre-constructed objects as props.

Code after:
          : []),
        ...(sourceCodeUrl?.startsWith('https://github.com/expo/expo')
          ? [
              {
                icon: <Tag03Icon className="mt-0.5 text-icon-secondary" />,
                label: 'Changelog',
                tooltip: (
                  <>
                    View the changelog of <MONOSPACE>{packageName}</MONOSPACE> on GitHub
                  </>
                ),
                href: `${sourceCodeUrl}/CHANGELOG.md`,
              },
            ]
          : []),
        ...(packageName
          ? [
              {
                icon: <BuildIcon className="mt-0.5 text-icon-secondary" />,
                label: 'npm',
                tooltip: 'View package in npm registry',
                href: `https://www.npmjs.com/package/${packageName}`,
              },
            ]
          : []),
      ].map(props => (
        <SdkPackageButton key={props.label} {...props} />
      ))}
    </>
  );
}
Repository: expo/expo
File: docs/ui/components/PageHeader/PageTitleButtons.tsx (TypeScript)

Code before:

export function PageTitleButtons({ packageName, sourceCodeUrl }: Props) {
  const router = useRouter();
  return (
    <>
      {!sourceCodeUrl && !packageName && router?.pathname && (
        <Button
          theme="quaternary"
          className="justify-center pl-2.5 pr-2"
          openInNewTab
          href={githubUrl(router.pathname)}
          aria-label="Edit content of this page on GitHub">
          <div className="flex flex-row items-center gap-2">
            <Edit05Icon className="icon-sm text-icon-secondary" />
            <FOOTNOTE crawlable={false} theme="secondary">
              Edit this page
            </FOOTNOTE>
          </div>
        </Button>
      )}
      {sourceCodeUrl && (
        <SdkPackageButton
          label="GitHub"
          Icon={GithubIcon}
          href={sourceCodeUrl}
          tooltip="View source code of package on GitHub"
        />
      )}
      {sourceCodeUrl?.startsWith('https://github.com/expo/expo') && (
        <SdkPackageButton
          label="Changelog"
          Icon={Tag03Icon}
          href={`${sourceCodeUrl}/CHANGELOG.md`}
          tooltip="View package changelog on GitHub"
        />
      )}
      {packageName && (
        <SdkPackageButton
          label="npm"
          Icon={BuildIcon}
          href={`https://www.npmjs.com/package/${packageName}`}
          tooltip="View package in npm registry"
        />
      )}
    </>
  );
}


Review comment (suggestion):
```suggestion
          tooltip="View source code on GitHub"
```

Code after:

export function PageTitleButtons({ packageName, sourceCodeUrl }: Props) {
  const router = useRouter();
  return (
    <>
      {!sourceCodeUrl && !packageName && router?.pathname && (
        <Button
          theme="quaternary"
          className="justify-center pl-2.5 pr-2"
          openInNewTab
          href={githubUrl(router.pathname)}
          aria-label="Edit content of this page on GitHub">
          <div className="flex flex-row items-center gap-2">
            <Edit05Icon className="icon-sm text-icon-secondary" />
            <FOOTNOTE crawlable={false} theme="secondary">
              Edit this page
            </FOOTNOTE>
          </div>
        </Button>
      )}
      {sourceCodeUrl && (
        <SdkPackageButton
          label="GitHub"
          Icon={GithubIcon}
          href={sourceCodeUrl}
          tooltip="View source code on GitHub"
        />
      )}
      {sourceCodeUrl?.startsWith('https://github.com/expo/expo') && (
        <SdkPackageButton
          label="Changelog"
          Icon={Tag03Icon}
          href={`${sourceCodeUrl}/CHANGELOG.md`}
          tooltip="View package changelog on GitHub"
        />
      )}
      {packageName && (
        <SdkPackageButton
          label="npm"
          Icon={BuildIcon}
          href={`https://www.npmjs.com/package/${packageName}`}
          tooltip="View package in npm registry"
        />
      )}
    </>
  );
}
Repository: expo/expo
File: packages/expo-router/src/link/elements.tsx (TypeScript)

Code before:
/**
 * A component used to render and customize the link preview.
 *
 * If `Link.Preview` is used without any props, it will render a preview of the `href` passed to the `Link`.
 *
 * If multiple `Link.Preview` components are used within a single `Link`, only the first one will be rendered.
 *
 * To customize the preview, you can pass custom content as children.
 *
 * @example
 * ```tsx
 * <Link href="/about">
 *   <Link.Preview>
 *     <Text>Custom Preview Content</Text>
 *   </Link.Trigger>
 * </Link>
 * ```
 *
 * @example
 * ```tsx
 * <Link href="/about">
 *   <Link.Preview />
 * </Link>
 * ```
 *
 * **Note:** You can use the alias `Link.Preview` for this component.
 *
 * @platform ios
 */
export function LinkPreview(props: LinkPreviewProps) {
  const { width, height, children } = props;
  const internalPreviewContext = use(InternalLinkPreviewContext);
  if (useIsPreview() || process.env.EXPO_OS !== 'ios' || !internalPreviewContext) {
    return null;
  }
  const { isVisible, href } = internalPreviewContext;
  const contentSize = {
    width: width ?? 0,
    height: height ?? 0,
  };
  let content: React.ReactNode;
  if (children) {
    content = isVisible ? children : null;
  } else {
    content = isVisible ? <HrefPreview href={href} /> : null;
  }

  return (
    <NativeLinkPreviewContent
      style={{
        /* Setting default background here, so that the preview is not transparent */

Review comment (suggestion):
```suggestion
 * > **Note**: You can use the alias `Link.Preview` for this component.
```

Code after:
/**
 * A component used to render and customize the link preview.
 *
 * If `Link.Preview` is used without any props, it will render a preview of the `href` passed to the `Link`.
 *
 * If multiple `Link.Preview` components are used within a single `Link`, only the first one will be rendered.
 *
 * To customize the preview, you can pass custom content as children.
 *
 * @example
 * ```tsx
 * <Link href="/about">
 *   <Link.Preview>
 *     <Text>Custom Preview Content</Text>
 *   </Link.Trigger>
 * </Link>
 * ```
 *
 * @example
 * ```tsx
 * <Link href="/about">
 *   <Link.Preview />
 * </Link>
 * ```
 *
 * > **Note**: You can use the alias `Link.Preview` for this component.
 *
 * @platform ios
 */
export function LinkPreview(props: LinkPreviewProps) {
  const { width, height, children } = props;
  const internalPreviewContext = use(InternalLinkPreviewContext);
  if (useIsPreview() || process.env.EXPO_OS !== 'ios' || !internalPreviewContext) {
    return null;
  }
  const { isVisible, href } = internalPreviewContext;
  const contentSize = {
    width: width ?? 0,
    height: height ?? 0,
  };
  let content: React.ReactNode;
  if (children) {
    content = isVisible ? children : null;
  } else {
    content = isVisible ? <HrefPreview href={href} /> : null;
  }

  return (
    <NativeLinkPreviewContent
      style={{
        /* Setting default background here, so that the preview is not transparent */
Repository: expo/expo
File: packages/expo-router/src/link/elements.tsx (TypeScript)

Code before:
        backgroundColor: '#fff',
      }}
      preferredContentSize={contentSize}>
      {content}
    </NativeLinkPreviewContent>
  );
}

export type LinkTriggerProps = PropsWithChildren;

/**
 * Serves as the trigger for a link.
 * The content inside this component will be rendered as part of the base link.
 *
 * If multiple `Link.Trigger` components are used within a single `Link`, only the first will be rendered.
 *
 * @example
 * ```tsx
 * <Link href="/about">
 *   <Link.Trigger>
 *     Trigger
 *   </Link.Trigger>
 * </Link>
 * ```
 *
 * **Note:** You can use the alias `Link.Trigger` for this component.
 *
 * @platform ios
 */
export function LinkTrigger(props: LinkTriggerProps) {
  if (React.Children.count(props.children) > 1 || !isValidElement(props.children)) {
    // If onPress is passed, this means that Link passed props to this component.
    // We can assume that asChild is used, so we throw an error, because link will not work in this case.
    if (props && typeof props === 'object' && 'onPress' in props) {
      throw new Error(
        'When using Link.Trigger in an asChild Link, you must pass a single child element that will emit onPress event.'
      );
    }
    return props.children;
  }
  return <Slot {...props} />;
}


Review comment (suggestion):
```suggestion
 * > **Note**: You can use the alias `Link.Trigger` for this component.
```

Code after:
        backgroundColor: '#fff',
      }}
      preferredContentSize={contentSize}>
      {content}
    </NativeLinkPreviewContent>
  );
}

export type LinkTriggerProps = PropsWithChildren;

/**
 * Serves as the trigger for a link.
 * The content inside this component will be rendered as part of the base link.
 *
 * If multiple `Link.Trigger` components are used within a single `Link`, only the first will be rendered.
 *
 * @example
 * ```tsx
 * <Link href="/about">
 *   <Link.Trigger>
 *     Trigger
 *   </Link.Trigger>
 * </Link>
 * ```
 *
 * > **Note**: You can use the alias `Link.Trigger` for this component.
 *
 * @platform ios
 */
export function LinkTrigger(props: LinkTriggerProps) {
  if (React.Children.count(props.children) > 1 || !isValidElement(props.children)) {
    // If onPress is passed, this means that Link passed props to this component.
    // We can assume that asChild is used, so we throw an error, because link will not work in this case.
    if (props && typeof props === 'object' && 'onPress' in props) {
      throw new Error(
        'When using Link.Trigger in an asChild Link, you must pass a single child element that will emit onPress event.'
      );
    }
    return props.children;
  }
  return <Slot {...props} />;
}
Repository: expo/expo
File: packages/expo-localization/src/ExpoLocalization.ts (TypeScript)

Code before:
      };
    });
  },
  getCalendars(): Calendar[] {
    const locale = ((typeof Intl !== 'undefined'
      ? Intl.DateTimeFormat().resolvedOptions()
      : null) ?? null) as unknown as null | ExtendedLocale;
    return [
      {
        calendar: ((locale?.calendar || locale?.calendars?.[0]) as CalendarIdentifier) || null,
        timeZone: locale?.timeZone || locale?.timeZones?.[0] || null,
        uses24hourClock: (locale?.hourCycle || locale?.hourCycles?.[0])?.startsWith('h2') ?? null, //https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/hourCycle
        firstWeekday: locale?.weekInfo?.firstDay || null,
      },
    ];
  },
};

function regionToTemperatureUnit(region: string) {
  return USES_FAHRENHEIT.includes(region) ? 'fahrenheit' : 'celsius';
}

function languageTextDirection(locale: ExtendedLocale) {
  // getTextInfo fallback
  return USES_RTL.includes(locale.language) ? 'rtl' : 'ltr';
}


Review comment (suggestion):
```suggestion
function languageTextDirection(language: string) {
  return USES_RTL.includes(language) ? 'rtl' : 'ltr';
}
```

Code after:
        measurementSystem: null,
        currencyCode: null,
        currencySymbol: null,
        languageCurrencyCode: null,
        languageCurrencySymbol: null,
        // On web, we don't have a way to get the region code, except from the language tag. `regionCode` and `languageRegionCode` are the same.
        regionCode: region || null,
        languageRegionCode: region || null,
        temperatureUnit,
      };
    }) as [Locale, ...Locale[]];
  },
  getCalendars(): [Calendar, ...Calendar[]] {
    const locale = ((typeof Intl !== 'undefined'
      ? Intl.DateTimeFormat().resolvedOptions()
      : null) ?? null) as unknown as null | ExtendedLocale;
    return [
      {
        calendar: ((locale?.calendar || locale?.calendars?.[0]) as CalendarIdentifier) || null,
        timeZone: locale?.timeZone || locale?.timeZones?.[0] || null,
        uses24hourClock: (locale?.hourCycle || locale?.hourCycles?.[0])?.startsWith('h2') ?? null, //https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/hourCycle
        firstWeekday: locale?.weekInfo?.firstDay || null,
      },
    ];
  },
};

function regionToTemperatureUnit(region: string) {
  return USES_FAHRENHEIT.includes(region) ? 'fahrenheit' : 'celsius';
}

function languageTextDirection(language: string) {
  return USES_RTL.includes(language) ? 'rtl' : 'ltr';
}