ronantakizawa/codereview-bench huggingface dataset:
Repository: expo/expo
File: docs/ui/components/CommandMenu/Items/CommandItemBase.tsx (TypeScript)

Code before:
export const CommandItemBase = ({
  children,
  url,
  isExternalLink,
  isNested,
  onSelect,
  className,
  value,
}: Props) => {
  const [copyDone, setCopyDone] = useState(false);

  const copyUrl = () => {
    navigator.clipboard?.writeText(url);
    setCopyDone(true);
    setTimeout(() => setCopyDone(false), 1500);
  };

  return (
    <Command.Item
      className={mergeClasses(className, 'relative')}
      value={value}
      data-nested={isNested ? true : undefined}
      onMouseUp={event => {
        // middle click (typical *nix copy shortcut) and
        // right click (works with Mac trackpads)
        // NOTE: onAuxClick is not supported in Safari
        if (event.button === 1 || event.button === 2) {
          copyUrl();
        }
      }}
      onSelect={() => {
        openLink(url, isExternalLink);
        onSelect && onSelect();
      }}
      onContextMenu={event => {
        event.preventDefault();
      }}>
      {children}
      {copyDone && (
        <Tag
          name="Copied!"
          className="absolute right-2.5 top-[calc(50%-13px)] !m-0 !border-secondary !bg-default"
        />
      )}
    </Command.Item>
  );
};


Review comment (security):
```suggestion
        // note(Keith): middle click (typical *nix copy shortcut) 
        // right click (works with Mac trackpads)
        // onAuxClick is not supported in Safari
```

Nittiest nit, but it's nice to have the comment attribution in code, since blame will not show the correct author of the comment after a while. 😉 

Code after:
  children,
  url,
  isExternalLink,
  isNested,
  onSelect,
  className,
  value,
}: Props) => {
  const [copyDone, setCopyDone] = useState(false);

  const copyUrl = () => {
    navigator.clipboard?.writeText(url);
    setCopyDone(true);
    setTimeout(() => setCopyDone(false), 1500);
  };

  return (
    <Command.Item
      className={mergeClasses('relative', className)}
      value={value}
      data-nested={isNested ? true : undefined}
      onMouseUp={event => {
        // note(Keith): middle click (typical *nix copy shortcut)
        // right click (works with Mac trackpads)
        // onAuxClick is not supported in Safari
        if (event.button === 1 || event.button === 2) {
          copyUrl();
        }
      }}
      onSelect={() => {
        openLink(url, isExternalLink);
        onSelect && onSelect();
      }}
      onContextMenu={event => {
        event.preventDefault();
      }}>
      {children}
      {copyDone && (
        <Tag
          name="Copied!"
          className="absolute right-2.5 top-[calc(50%-13px)] !m-0 !border-secondary !bg-default"
        />
      )}
    </Command.Item>
  );
};
Repository: expo/expo
File: packages/expo-dev-menu/src/DevMenu.ts (TypeScript)

Code before:
 */
export function closeMenu(): void {
  ExpoDevMenu.closeMenu();
}

let hasRegisteredCallbackListener = false;

function registerCallbackListener() {
  if (!hasRegisteredCallbackListener) {
    DeviceEventEmitter.addListener('registeredCallbackFired', (name: string) => {
      hasRegisteredCallbackListener = true;
      const handler = handlers.get(name);

      if (handler != null) {
        handler();
      }
    });
  }
}

registerCallbackListener();

let handlers = new Map<string, () => void>();

/**
 * A method allowing to specify custom entries in the development client menu.
 * @param items
 */
export async function registerDevMenuItems(items: ExpoDevMenuItem[]): Promise<void> {
  if (!__DEV__) {
    // resolve undefined
    return;
  }

  handlers = new Map();
  const callbackNames: { name: string; shouldCollapse?: boolean }[] = [];

  items.forEach((item) => {
    handlers.set(item.name, item.callback);
    callbackNames.push({ name: item.name, shouldCollapse: item.shouldCollapse });
  });

  return await ExpoDevMenu.addDevMenuCallbacks(callbackNames);
}


Review comment (suggestion):
```suggestion
 * A method that allows to specify custom entries in the development client menu.
```

Code after:
 */
export function closeMenu(): void {
  ExpoDevMenu.closeMenu();
}

let hasRegisteredCallbackListener = false;

function registerCallbackListener() {
  if (!hasRegisteredCallbackListener) {
    DeviceEventEmitter.addListener('registeredCallbackFired', (name: string) => {
      hasRegisteredCallbackListener = true;
      const handler = handlers.get(name);

      if (handler != null) {
        handler();
      }
    });
  }
}

registerCallbackListener();

let handlers = new Map<string, () => void>();

/**
 * A method that allows to specify custom entries in the development client menu.
 * @param items
 */
export async function registerDevMenuItems(items: ExpoDevMenuItem[]): Promise<void> {
  if (!__DEV__) {
    // resolve undefined
    return;
  }

  handlers = new Map();
  const callbackNames: { name: string; shouldCollapse?: boolean }[] = [];

  items.forEach((item) => {
    handlers.set(item.name, item.callback);
    callbackNames.push({ name: item.name, shouldCollapse: item.shouldCollapse });
  });

  return await ExpoDevMenu.addDevMenuCallbacks(callbackNames);
}
Repository: expo/expo
File: packages/expo-dev-launcher/src/DevLauncher.ts (TypeScript)

Code before:
import { ExpoUpdatesManifest } from 'expo-manifests';
import { NativeModules } from 'react-native';
import './setUpErrorHandler.fx';

/**
 * @hidden Dev launcher manifests are only ones served by servers (not embedded bare manifests)
 */
export type Manifest = ExpoUpdatesManifest;

export { disableErrorHandling } from './DevLauncherErrorManager';

/**
 * @hidden
 */
export function registerErrorHandlers() {
  console.warn(
    'DevLauncher.registerErrorHandlers has been deprecated. To enable error handlers you need to import "expo-dev-launcher" at the top of your index.js.'
  );
}

/**
 * A method returning a boolean indicating if the current application is a development build.
 */
export function isDevelopmentBuild(): boolean {
  return !!NativeModules.EXDevLauncher;
}

/**
 * @hidden
 */
export type DevLauncherExtension = {
  navigateToLauncherAsync: () => Promise<void>;
};


Review comment (suggestion):
```suggestion
 * A method that returns a boolean to indicate if the current application is a development build.
```

Let's update the verbiage to keep the language easier to read.

Code after:
import { ExpoUpdatesManifest } from 'expo-manifests';
import { NativeModules } from 'react-native';
import './setUpErrorHandler.fx';

/**
 * @hidden Dev launcher manifests are only ones served by servers (not embedded bare manifests)
 */
export type Manifest = ExpoUpdatesManifest;

export { disableErrorHandling } from './DevLauncherErrorManager';

/**
 * @hidden
 */
export function registerErrorHandlers() {
  console.warn(
    'DevLauncher.registerErrorHandlers has been deprecated. To enable error handlers you need to import "expo-dev-launcher" at the top of your index.js.'
  );
}

/**
 * A method that returns a boolean to indicate if the current application is a development build.
 */
export function isDevelopmentBuild(): boolean {
  return !!NativeModules.EXDevLauncher;
}

/**
 * @hidden
 */
export type DevLauncherExtension = {
  navigateToLauncherAsync: () => Promise<void>;
};
Repository: expo/expo
File: packages/expo-video/src/VideoView.tsx (TypeScript)

Code before:
import { SharedObject } from 'expo-modules-core';
import {
  ReactNode,
  PureComponent,
  DependencyList,
  createRef,
  useRef,
  useMemo,
  useEffect,
} from 'react';

import NativeVideoModule from './NativeVideoModule';
import NativeVideoView from './NativeVideoView';
import { VideoPlayer, VideoSource, VideoViewProps } from './VideoView.types';

/**
 * Creates a `VideoPlayer`, which will be automatically cleaned up when the component is unmounted.
 * @param source - A video source to initialize the player with
 * @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 = typeof source === 'string' ? { uri: source } : source;

  return useReleasingSharedObject(() => {
    const player = new NativeVideoModule.VideoPlayer(parsedSource);
    setup?.(player);
    return player;
  }, [JSON.stringify(parsedSource)]);
}

/**
 * Returns whether the current device supports Picture in Picture (PiP) mode.
 * @returns A `boolean` which is `true` if the device supports PiP mode, and `false` otherwise.
 * @platform android
 * @platform ios
 */
export function isPictureInPictureSupported(): Promise<boolean> {
  return NativeVideoModule.isPictureInPictureSupported();
}


Review comment (suggestion):
```suggestion
 * @param source - A video source that is used to initialize the player.
```

Let's rephrase this to make it more clear and also add the missing period.

Code after:
import { SharedObject } from 'expo-modules-core';
import {
  ReactNode,
  PureComponent,
  DependencyList,
  createRef,
  useRef,
  useMemo,
  useEffect,
} from 'react';

import NativeVideoModule from './NativeVideoModule';
import NativeVideoView from './NativeVideoView';
import { VideoPlayer, VideoSource, VideoViewProps } from './VideoView.types';

/**
 * 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 = typeof source === 'string' ? { uri: source } : source;

  return useReleasingSharedObject(() => {
    const player = new NativeVideoModule.VideoPlayer(parsedSource);
    setup?.(player);
    return player;
  }, [JSON.stringify(parsedSource)]);
}

/**
 * Returns whether the current device supports Picture in Picture (PiP) mode.
 * @returns A `boolean` which is `true` if the device supports PiP mode, and `false` otherwise.
 * @platform android
 * @platform ios
 */
export function isPictureInPictureSupported(): Promise<boolean> {
  return NativeVideoModule.isPictureInPictureSupported();
}
Repository: expo/expo
File: docs/constants/navigation.js (JavaScript)

Code before:
    makePage('archive/publishing-websites-webpack.mdx'),
    makePage('archive/customizing-webpack.mdx'),
    makePage('archive/glossary.mdx'),
  ]),
];

const featurePreview = [];

const versionsReference = VERSIONS.reduce(
  (all, version) => ({
    ...all,
    [version]: [
      makeSection('Configuration files', pagesFromDir(`versions/${version}/config`), {
        expanded: true,
      }),
      makeSection('Expo SDK', pagesFromDir(`versions/${version}/sdk`), { expanded: true }),
      makeSection('Technical specs', [
        makePage('technical-specs/expo-updates-1.mdx'),
        makePage('technical-specs/expo-sfv-0.mdx'),
      ]),
      makeSection(
        'More',
        [
          makePage('more/expo-cli.mdx'),
          makePage('more/glossary-of-terms.mdx'),
          makePage('more/qr-codes.mdx'),
        ],
        {
          expanded: true,
        }
      ),
      makeSection(
        'React Native',
        [
          make('page', {
            href: 'https://reactnative.dev/docs/components-and-apis',
            sidebarTitle: 'Visit documentation',
          }),
        ],
        { expanded: true }
      ),
    ],
  }),
  {}
);

const reference = { ...versionsReference, latest: versionsReference['latest'] };

export default {
  home,
  general,

Review comment (suggestion):
```suggestion
          makePage('more/qr-codes.mdx'),
          makePage('more/glossary-of-terms.mdx'),
```

(Non-blocking) I'd suggest adding this page before Glossary of terms doc. Organizing the sidebar, it seems pages with info like Glossary and/or Troubleshooting/Error codes make more sense as a mental model from a reader's perspective.

Code after:
    makePage('archive/publishing-websites-webpack.mdx'),
    makePage('archive/customizing-webpack.mdx'),
    makePage('archive/glossary.mdx'),
  ]),
];

const featurePreview = [];

const versionsReference = VERSIONS.reduce(
  (all, version) => ({
    ...all,
    [version]: [
      makeSection('Configuration files', pagesFromDir(`versions/${version}/config`), {
        expanded: true,
      }),
      makeSection('Expo SDK', pagesFromDir(`versions/${version}/sdk`), { expanded: true }),
      makeSection('Technical specs', [
        makePage('technical-specs/expo-updates-1.mdx'),
        makePage('technical-specs/expo-sfv-0.mdx'),
      ]),
      makeSection(
        'More',
        [
          makePage('more/expo-cli.mdx'),
          makePage('more/qr-codes.mdx'),
          makePage('more/glossary-of-terms.mdx'),
        ],
        {
          expanded: true,
        }
      ),
      makeSection(
        'React Native',
        [
          make('page', {
            href: 'https://reactnative.dev/docs/components-and-apis',
            sidebarTitle: 'Visit documentation',
          }),
        ],
        { expanded: true }
      ),
    ],
  }),
  {}
);

const reference = { ...versionsReference, latest: versionsReference['latest'] };

export default {
  home,
  general,
Repository: expo/expo
File: packages/expo-symbols/src/SymbolModule.types.ts (TypeScript)

Code before:
   */
  resizeMode?: ContentMode;
  /**
   * The animation configuration to apply to the symbol.
   */
  animationSpec?: AnimationSpec;
} & ViewProps;

/**
 * It narrows down some props to types expected by the native/web side.
 * @hidden
 */
export interface NativeSymbolViewProps extends ViewProps {
  name: string;
  type: SymbolType;
  scale?: SymbolScale;
  weight?: SymbolWeight;
  animated: boolean;
  colors?: string | string[];
  tint?: string;
  resizeMode?: ContentMode;
  animationSpec?: AnimationSpec;
}

/**
 * @hidden Described in the {@link SymbolViewProps['weight']}
 */
export type SymbolWeight =
  | 'unspecified'
  | 'ultraLight'
  | 'thin'
  | 'light'
  | 'regular'
  | 'medium'
  | 'semibold'
  | 'bold'
  | 'heavy'
  | 'black';

/**
 * @hidden Described in the {@link SymbolViewProps['scale']}
 */
export type SymbolScale = 'default' | 'unspecified' | 'small' | 'medium' | 'large';

/**
 * @hidden Described in the {@link SymbolViewProps['resizeMode']}
 */
export type ContentMode =
  | 'scaleToFill'
  | 'scaleAspectFit'
  | 'scaleAspectFill'

Review comment (suggestion):
```suggestion
 * Type description in here.
```

We should not hide the types like that. There will be still has link generated to those types It's recommended to leave those most of the API visible in general, unless there is something that we really have a reason to hide. This suggestion also applies for the following types.

Also, we do not support `@link` syntax like JSDoc does, we usually expect Markdown in the code description.

Code after:
 * It narrows down some props to types expected by the native/web side.
 * @hidden
 */
export interface NativeSymbolViewProps extends ViewProps {
  name: string;
  type: SymbolType;
  scale?: SymbolScale;
  weight?: SymbolWeight;
  animated: boolean;
  colors?: string | string[];
  tint?: string;
  resizeMode?: ContentMode;
  animationSpec?: AnimationSpec;
}

/**
 * The weight of the symbol to render.
 */
export type SymbolWeight =
  | 'unspecified'
  | 'ultraLight'
  | 'thin'
  | 'light'
  | 'regular'
  | 'medium'
  | 'semibold'
  | 'bold'
  | 'heavy'
  | 'black';

/**
 * The scale of the symbol to render.
 */
export type SymbolScale = 'default' | 'unspecified' | 'small' | 'medium' | 'large';

/**
 * Determines how the image should be resized to fit its container.
 */
export type ContentMode =
  | 'scaleToFill'
  | 'scaleAspectFit'
  | 'scaleAspectFill'
  | 'redraw'
  | 'center'
  | 'top'
  | 'bottom'
  | 'left'
  | 'right'
  | 'topLeft'
  | 'topRight'
  | 'bottomLeft'
Repository: expo/expo
File: packages/expo-modules-core/src/reload.ts (TypeScript)

Code before:
/**
 * Reloads the app. This function works for both release and debug builds.
 *
 * Unlike [`Updates.reloadAsync()`](/versions/latest/sdk/updates/#updatesreloadasync),
 * this function does not use a new update even if one is available. It only reloads the app using the same JavaScript bundle that is currently running.
 *
 * @param reason The reason for reloading the app. This is used only for some platforms.
 */
export async function reloadAppAsync(reason: string = 'Reloaded from JS call'): Promise<void> {
  await globalThis.expo?.reloadAppAsync(reason);
}


Review comment (bug):
```suggestion
 * Reloads the app. This method works for both release and debug builds.
```

Code after:
/**
 * Reloads the app. This method works for both release and debug builds.
 *
 * Unlike [`Updates.reloadAsync()`](/versions/latest/sdk/updates/#updatesreloadasync),
 * this function does not use a new update even if one is available. It only reloads the app using the same JavaScript bundle that is currently running.
 *
 * @param reason The reason for reloading the app. This is used only for some platforms.
 */
export async function reloadAppAsync(reason: string = 'Reloaded from JS call'): Promise<void> {
  await globalThis.expo?.reloadAppAsync(reason);
}
Repository: expo/expo
File: packages/expo/src/errors/ExpoErrorManager.ts (TypeScript)

Code before:
export function createErrorHandler(originalHandler) {
  return (error, isFatal) => originalHandler(error, isFatal);
}

/**
 * @hidden
 * @deprecated Remove in the future.
 */
export function disableErrorHandling() {}


Review comment (suggestion):
```suggestion
 * @deprecated Will be removed in the future.
```

Code after:
export function createErrorHandler(originalHandler) {
  return (error, isFatal) => originalHandler(error, isFatal);
}

/**
 * @hidden
 * @deprecated Will be removed in the future.
 */
export function disableErrorHandling() {}
Repository: expo/expo
File: docs/common/snack.ts (TypeScript)

Code before:
export const SNACK_URL = 'https://snack.expo.dev';
// export const SNACK_URL = 'http://snack.expo.test';

type Config = {
  baseURL: string;
  templateId?: string;
  code?: string | null;
  files?: Record<string, string>;
  codeLanguage: string | undefined;
};

type File = {
  type: 'CODE' | 'ASSET';
  contents?: string;
  url?: string;
};

export function getSnackFiles(config: Config) {
  const { templateId, code, files, baseURL, codeLanguage } = config;

  const result: Record<string, File> = {};
  if (files) {
    Object.keys(files).forEach(path => {
      const url = files[path];
      const isCode = /\.(jsx?|tsx?|json|md)$/i.test(path);
      if (isCode) {
        result[path] = {
          type: 'CODE',
          url: url.match(/^https?:\/\//) ? url : `${baseURL}/${url}`,
        };
      } else {
        result[path] = {
          type: 'ASSET',
          contents: url, // Should be a snack-code-uploads S3 url

Review comment (suggestion):
```suggestion
  codeLanguage?: string;
```

Typing nit.

Code after:
export const SNACK_URL = 'https://snack.expo.dev';
// export const SNACK_URL = 'http://snack.expo.test';

type Config = {
  baseURL: string;
  templateId?: string;
  code?: string | null;
  files?: Record<string, string>;
  codeLanguage?: string;
};

type File = {
  type: 'CODE' | 'ASSET';
  contents?: string;
  url?: string;
};

export function getSnackFiles(config: Config) {
  const { templateId, code, files, baseURL, codeLanguage } = config;

  const result: Record<string, File> = {};
  if (files) {
    Object.keys(files).forEach(path => {
      const url = files[path];
      const isCode = /\.(jsx?|tsx?|json|md)$/i.test(path);
      if (isCode) {
        result[path] = {
          type: 'CODE',
          url: url.match(/^https?:\/\//) ? url : `${baseURL}/${url}`,
        };
      } else {
        result[path] = {
          type: 'ASSET',
          contents: url, // Should be a snack-code-uploads S3 url
Repository: expo/expo
File: packages/expo-modules-core/src/ts-declarations/SharedRef.ts (TypeScript)

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

/**
 * A {@link SharedObject} that holds a reference to any native object. Allows passing references
 * to native instances among different independent libraries.
 *
 * For instance, `ImageRef` from `expo-image` references a [Drawable](https://developer.android.com/reference/android/graphics/drawable/Drawable)
 * on Android and an [UIImage](https://developer.apple.com/documentation/uikit/uiimage) on iOS. Since both types are common on these platforms,
 * different native modules can use them without depending on each other. In particular, this enables `expo-image-manipulator` to pass the resulted image
 * directly to the image view from `expo-image` without any additional writes and reads from the file system.
 */
export declare class SharedRef<TEventsMap extends EventsMap = Record<never, never>>
  extends SharedObject<TEventsMap>
  implements SharedObject<TEventsMap> {}


Review comment (suggestion):
```suggestion
 * For instance, `ImageRef` from `expo-image` references a [Drawable](https://developer.android.com/reference/android/graphics/drawable/Drawable)
 * on Android and an [UIImage](https://developer.apple.com/documentation/uikit/uiimage) on iOS. Since both types are common on these platforms,
 * different native modules can use them without depending on each other. In particular, this enables the `expo-image-manipulator` to pass the resulted image
 * directly to the image view from `expo-image` without any additional writes and reads from the file system.
```

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

/**
 * A {@link SharedObject} that holds a reference to any native object. Allows passing references
 * to native instances among different independent libraries.
 *
 * For instance, `ImageRef` from `expo-image` references a [Drawable](https://developer.android.com/reference/android/graphics/drawable/Drawable)
 * on Android and an [UIImage](https://developer.apple.com/documentation/uikit/uiimage) on iOS. Since both types are common on these platforms,
 * different native modules can use them without depending on each other. In particular, this enables the `expo-image-manipulator` to pass the resulted image
 * directly to the image view from `expo-image` without any additional writes and reads from the file system.
 */
export declare class SharedRef<TEventsMap extends EventsMap = Record<never, never>>
  extends SharedObject<TEventsMap>
  implements SharedObject<TEventsMap> {}