ronantakizawa/codereview-bench huggingface dataset:
Repository: expo/expo
File: packages/expo-notifications/src/topicSubscription.ts (TypeScript)

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

import TopicSubscriptionModule from './TopicSubscriptionModule';
import { warnOfExpoGoPushUsage } from './warnOfExpoGoPushUsage';

/**
 * Subscribes the device to a push notification topic. This allows the device to receive notifications sent to that topic.
 * @param topic The topic name to subscribe to.
 * @return A Promise which resolves to `null` once the device is subscribed to the topic.
 * @platform android
 */
export async function subscribeToTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.subscribeToTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'subscribeToTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.subscribeToTopicAsync(topic);
}

/**
 * Unsubscribes the device from a push notification topic. The device will no longer receive notifications sent to that topic.
 * @param topic The topic name to unsubscribe from.
 * @return A Promise which resolves to `null` once the device is unsubscribed from the topic.
 * @platform android
 */
export async function unsubscribeFromTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.unsubscribeFromTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'unsubscribeFromTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.unsubscribeFromTopicAsync(topic);
}

Review comment (suggestion):
```suggestion
 * @return a Promise which resolves to `null` once the device is subscribed to the topic.
```

This is prefixed as "Returns A ...". So lets update it so that it becomes "Returns a ..."

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

import TopicSubscriptionModule from './TopicSubscriptionModule';
import { warnOfExpoGoPushUsage } from './warnOfExpoGoPushUsage';

/**
 * Subscribes the device to a push notification topic. This allows the device to receive notifications sent to that topic.
 * @param topic The topic name to subscribe to.
 * @return a Promise which resolves to `null` once the device is subscribed to the topic.
 * @platform android
 */
export async function subscribeToTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.subscribeToTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'subscribeToTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.subscribeToTopicAsync(topic);
}

/**
 * Unsubscribes the device from a push notification topic. The device will no longer receive notifications sent to that topic.
 * @param topic The topic name to unsubscribe from.
 * @return a Promise which resolves to `null` once the device is unsubscribed from the topic.
 * @platform android
 */
export async function unsubscribeFromTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.unsubscribeFromTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'unsubscribeFromTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.unsubscribeFromTopicAsync(topic);
}
Repository: expo/expo
File: packages/expo-notifications/src/topicSubscription.ts (TypeScript)

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

import TopicSubscriptionModule from './TopicSubscriptionModule';
import { warnOfExpoGoPushUsage } from './warnOfExpoGoPushUsage';

/**
 * Subscribes the device to a push notification topic. This allows the device to receive notifications sent to that topic.
 * @param topic The topic name to subscribe to.
 * @return A Promise which resolves to `null` once the device is subscribed to the topic.
 * @platform android
 */
export async function subscribeToTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.subscribeToTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'subscribeToTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.subscribeToTopicAsync(topic);
}

/**
 * Unsubscribes the device from a push notification topic. The device will no longer receive notifications sent to that topic.
 * @param topic The topic name to unsubscribe from.
 * @return A Promise which resolves to `null` once the device is unsubscribed from the topic.
 * @platform android
 */
export async function unsubscribeFromTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.unsubscribeFromTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'unsubscribeFromTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.unsubscribeFromTopicAsync(topic);
}


Review comment (suggestion):
```suggestion
 * @return a Promise which resolves to `null` once the device is unsubscribed from the topic.
```

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

import TopicSubscriptionModule from './TopicSubscriptionModule';
import { warnOfExpoGoPushUsage } from './warnOfExpoGoPushUsage';

/**
 * Subscribes the device to a push notification topic. This allows the device to receive notifications sent to that topic.
 * @param topic The topic name to subscribe to.
 * @return a Promise which resolves to `null` once the device is subscribed to the topic.
 * @platform android
 */
export async function subscribeToTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.subscribeToTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'subscribeToTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.subscribeToTopicAsync(topic);
}

/**
 * Unsubscribes the device from a push notification topic. The device will no longer receive notifications sent to that topic.
 * @param topic The topic name to unsubscribe from.
 * @return a Promise which resolves to `null` once the device is unsubscribed from the topic.
 * @platform android
 */
export async function unsubscribeFromTopicAsync(topic: string): Promise<null> {
  if (!TopicSubscriptionModule.unsubscribeFromTopicAsync) {
    throw new UnavailabilityError('ExpoNotifications', 'unsubscribeFromTopicAsync');
  }
  warnOfExpoGoPushUsage();

  return TopicSubscriptionModule.unsubscribeFromTopicAsync(topic);
}
Repository: expo/expo
File: packages/@expo/cli/src/run/index.ts (TypeScript)

Code before:

  {bold Usage}
    $ npx expo run:android <dir>
    $ npx expo run:ios <dir>

  {bold Options}
    --no-build-cache                 Clear the native build cache
    --no-install                     Skip installing dependencies
    --no-bundler                     Skip starting the bundler
    --configuration <configuration>  {underline iOS} Xcode configuration to use. Debug or Release. {dim Default: Debug}
    --variant <name>                 {underline Android} build variant to use. {dim Default: debug}
    -d, --device [device]            Device name to run the app on
    -p, --port <port>                Port to start the dev server on. {dim Default: 8081}
    -h, --help                       Output usage information
`,
      0
    );
  }

  const platform = await selectAsync('Select the platform to run', [
    { title: 'Android', value: 'android' },
    { title: 'iOS', value: 'ios' },
  ]);

  switch (platform) {
    case 'android': {
      const { expoRunAndroid } = await import('./android');
      return expoRunAndroid(argv);
    }

    case 'ios': {
      const { expoRunIos } = await import('./ios');
      return expoRunIos(argv);
    }

    default:
      throw new CommandError('UNSUPPORTED_PLATFORM', `Unsupported platform: ${platform}`);
  }
};


Review comment (suggestion):
After this step, it should print the `expo run:<platform>` command with the same flags. So the user knows what command to run next time. cc @EvanBacon 

Code after:
    }

    // Remove the platform from raw arguments, when provided
    const argsWithoutPlatform = !platform ? argv : argv?.splice(1);

    // Do not capture `--help` when platform is provided
    if (!platform && args['--help']) {
      printHelp(
        'Run the native app locally',
        `npx expo run <android|ios>`,
        chalk`{dim $} npx expo run <android|ios> --help  Output usage information`
      );
    }

    if (!platform) {
      const { selectAsync } = await import('../utils/prompts.js');
      platform = await selectAsync('Select the platform to run', [
        { title: 'Android', value: 'android' },
        { title: 'iOS', value: 'ios' },
      ]);
    }

    logPlatformRunCommand(platform, argsWithoutPlatform);

    switch (platform) {
      case 'android': {
        const { expoRunAndroid } = await import('./android/index.js');
        return expoRunAndroid(argsWithoutPlatform);
      }

      case 'ios': {
        const { expoRunIos } = await import('./ios/index.js');
        return expoRunIos(argsWithoutPlatform);
      }

      default:
        throw new CommandError('UNSUPPORTED_PLATFORM', `Unsupported platform: ${platform}`);
    }
  } catch (error: any) {
    logCmdError(error);
  }
};
Repository: expo/expo
File: packages/@expo/cli/src/run/index.ts (TypeScript)

Code before:
      // Aliases
      '-h': '--help',
      // All other flags are handled by `expoRunAndroid` or `expoRunIos`
    },
    {
      argv,
      permissive: true,
    }
  );

  if (args['--help']) {
    Log.exit(
      chalk`
  {bold Description}
    Run the native app locally

  {bold Usage}
    $ npx expo run:android <dir>
    $ npx expo run:ios <dir>

  {bold Options}
    --no-build-cache                 Clear the native build cache
    --no-install                     Skip installing dependencies
    --no-bundler                     Skip starting the bundler
    --configuration <configuration>  {underline iOS} Xcode configuration to use. Debug or Release. {dim Default: Debug}
    --variant <name>                 {underline Android} build variant to use. {dim Default: debug}
    -d, --device [device]            Device name to run the app on
    -p, --port <port>                Port to start the dev server on. {dim Default: 8081}
    -h, --help                       Output usage information
`,
      0
    );
  }

  const platform = await selectAsync('Select the platform to run', [
    { title: 'Android', value: 'android' },
    { title: 'iOS', value: 'ios' },
  ]);

  switch (platform) {
    case 'android': {
      const { expoRunAndroid } = await import('./android');
      return expoRunAndroid(argv);
    }

    case 'ios': {
      const { expoRunIos } = await import('./ios');
      return expoRunIos(argv);
    }

    default:

Review comment (suggestion):
Seems like we should drop these and just add a tip at the end which tells people to use `run:ios` and `run:android` for more options.

Code after:
      '-h': '--help',
    },
    {
      argv,
      // Allow additional flags for both android and ios commands
      permissive: true,
    }
  );

  try {
    let [platform] = args._ ?? [];

    // Workaround, filter `--flag` as platform
    if (platform?.startsWith('-')) {
      platform = '';
    }

    // Remove the platform from raw arguments, when provided
    const argsWithoutPlatform = !platform ? argv : argv?.splice(1);

    // Do not capture `--help` when platform is provided
    if (!platform && args['--help']) {
      printHelp(
        'Run the native app locally',
        `npx expo run <android|ios>`,
        chalk`{dim $} npx expo run <android|ios> --help  Output usage information`
      );
    }

    if (!platform) {
      const { selectAsync } = await import('../utils/prompts.js');
      platform = await selectAsync('Select the platform to run', [
        { title: 'Android', value: 'android' },
        { title: 'iOS', value: 'ios' },
      ]);
    }

    logPlatformRunCommand(platform, argsWithoutPlatform);

    switch (platform) {
      case 'android': {
        const { expoRunAndroid } = await import('./android/index.js');
        return expoRunAndroid(argsWithoutPlatform);
      }

      case 'ios': {
        const { expoRunIos } = await import('./ios/index.js');
        return expoRunIos(argsWithoutPlatform);
      }

      default:
Repository: expo/expo
File: packages/@expo/cli/src/run/index.ts (TypeScript)

Code before:
#!/usr/bin/env node
import chalk from 'chalk';

import { Command } from '../../bin/cli';
import { Log } from '../log';
import { assertWithOptionsArgs } from '../utils/args';
import { CommandError } from '../utils/errors';
import { selectAsync } from '../utils/prompts';

export const expoRun: Command = async (argv) => {
  const args = assertWithOptionsArgs(
    {
      // Types
      '--help': Boolean,
      // Aliases
      '-h': '--help',
      // All other flags are handled by `expoRunAndroid` or `expoRunIos`
    },
    {
      argv,
      permissive: true,
    }
  );

  if (args['--help']) {
    Log.exit(
      chalk`
  {bold Description}
    Run the native app locally

  {bold Usage}
    $ npx expo run:android <dir>
    $ npx expo run:ios <dir>

  {bold Options}
    --no-build-cache                 Clear the native build cache
    --no-install                     Skip installing dependencies
    --no-bundler                     Skip starting the bundler
    --configuration <configuration>  {underline iOS} Xcode configuration to use. Debug or Release. {dim Default: Debug}
    --variant <name>                 {underline Android} build variant to use. {dim Default: debug}
    -d, --device [device]            Device name to run the app on
    -p, --port <port>                Port to start the dev server on. {dim Default: 8081}
    -h, --help                       Output usage information
`,
      0

Review comment (suggestion):
If we're doing permissive, then maybe it makes sense to allow a `-p, --platform` flag which can skip the prompt.

Code after:
#!/usr/bin/env node
import chalk from 'chalk';

import { logPlatformRunCommand } from './hints';
import { Command } from '../../bin/cli';
import { assertWithOptionsArgs, printHelp } from '../utils/args';
import { CommandError, logCmdError } from '../utils/errors';

export const expoRun: Command = async (argv) => {
  const args = assertWithOptionsArgs(
    {
      // Types
      '--help': Boolean,
      // Aliases
      '-h': '--help',
    },
    {
      argv,
      // Allow additional flags for both android and ios commands
      permissive: true,
    }
  );

  try {
    let [platform] = args._ ?? [];

    // Workaround, filter `--flag` as platform
    if (platform?.startsWith('-')) {
      platform = '';
    }

    // Remove the platform from raw arguments, when provided
    const argsWithoutPlatform = !platform ? argv : argv?.splice(1);

    // Do not capture `--help` when platform is provided
    if (!platform && args['--help']) {
      printHelp(
        'Run the native app locally',
        `npx expo run <android|ios>`,
        chalk`{dim $} npx expo run <android|ios> --help  Output usage information`
      );
    }

    if (!platform) {
      const { selectAsync } = await import('../utils/prompts.js');
Repository: expo/expo
File: packages/@expo/cli/src/run/index.ts (TypeScript)

Code before:
      chalk`
  {bold Info}
    Run the native app locally

  {bold Usage}
    {dim $} npx expo run:android <dir>
    {dim $} npx expo run:ios <dir>

  {bold Options}
    -p, --platform <android|ios>     Run the native app on this platform
    -h, --help                       Output usage information

    {dim $} npx expo run:android --help    Output Android usage information
    {dim $} npx expo run:ios --help        Output iOS usage information
`,
      0
    );
  }

  try {
    const platform =
      args['--platform'] ??
      (await selectAsync('Select the platform to run', [
        { title: 'Android', value: 'android' },
        { title: 'iOS', value: 'ios' },
      ]));

    // Filter `--platform=android|ios` or `--platform android|ios` from the args for `run:android|ios`
    const argsWithoutPlatform = Object.values(args._).filter(
      (flag) => !flag.startsWith('--platform') || ['android', 'ios'].includes(flag)
    );

    logPlatformRunCommand(platform, argsWithoutPlatform);

    switch (platform) {
      case 'android': {
        const { expoRunAndroid } = await import('./android');
        return expoRunAndroid(argsWithoutPlatform);
      }

      case 'ios': {
        const { expoRunIos } = await import('./ios');
        return expoRunIos(argsWithoutPlatform);
      }

      default:
        throw new CommandError('UNSUPPORTED_PLATFORM', `Unsupported platform: ${platform}`);
    }
  } catch (error: any) {
    logCmdError(error);
  }

Review comment (refactor):
The reason we extracted the argument logic in the other commands was so that we could setup unit tests. Given this command is tied to a native build, it isn’t a spectacular candidate for e2e testing. 

Code after:
      platform = '';
    }

    // Remove the platform from raw arguments, when provided
    const argsWithoutPlatform = !platform ? argv : argv?.splice(1);

    // Do not capture `--help` when platform is provided
    if (!platform && args['--help']) {
      printHelp(
        'Run the native app locally',
        `npx expo run <android|ios>`,
        chalk`{dim $} npx expo run <android|ios> --help  Output usage information`
      );
    }

    if (!platform) {
      const { selectAsync } = await import('../utils/prompts.js');
      platform = await selectAsync('Select the platform to run', [
        { title: 'Android', value: 'android' },
        { title: 'iOS', value: 'ios' },
      ]);
    }

    logPlatformRunCommand(platform, argsWithoutPlatform);

    switch (platform) {
      case 'android': {
        const { expoRunAndroid } = await import('./android/index.js');
        return expoRunAndroid(argsWithoutPlatform);
      }

      case 'ios': {
        const { expoRunIos } = await import('./ios/index.js');
        return expoRunIos(argsWithoutPlatform);
      }

      default:
        throw new CommandError('UNSUPPORTED_PLATFORM', `Unsupported platform: ${platform}`);
    }
  } catch (error: any) {
    logCmdError(error);
  }
};
Repository: expo/expo
File: docs/ui/components/CommandMenu/Items/CommandItemBase.tsx (TypeScript)

Code before:
import { mergeClasses } from '@expo/styleguide';
import { Command } from 'cmdk';
import { useState } from 'react';

import { openLink } from '../utils';

import { Tag } from '~/ui/components/Tag';

type Props = {
  children: React.ReactNode;
  url: string;
  onSelect?: () => void;
  isExternalLink?: boolean;
  isNested?: boolean;
  // Props forwarded to Command.Item
  className?: string;
  value?: string;
};

/**
 * Wrapper for Command.Item that adds copy link on right/ middle click + visual copy indicator.
 */
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}

Review comment (suggestion):
```suggestion
type Props = PropsWithChildren<{
  url: string;
  onSelect?: () => void;
  isExternalLink?: boolean;
  isNested?: boolean;
  // Props forwarded to Command.Item
  className?: string;
  value?: string;
}>;
```

Typing nit.

Code after:
import { mergeClasses } from '@expo/styleguide';
import { Command } from 'cmdk';
import { PropsWithChildren, useState } from 'react';

import { openLink } from '../utils';

import { Tag } from '~/ui/components/Tag';

type Props = PropsWithChildren<{
  url: string;
  onSelect?: () => void;
  isExternalLink?: boolean;
  isNested?: boolean;
  // Props forwarded to Command.Item
  className?: string;
  value?: string;
}>;

/**
 * Wrapper for Command.Item that adds copy link on right/ middle click + visual copy indicator.
 */
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('relative', className)}
      value={value}
      data-nested={isNested ? true : undefined}
Repository: expo/expo
File: docs/ui/components/CommandMenu/Items/CommandItemBase.tsx (TypeScript)

Code before:
  value?: string;
};

/**
 * Wrapper for Command.Item that adds copy link on right/ middle click + visual copy indicator.
 */
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 (suggestion):
```suggestion
      className={mergeClasses('relative', className)}
```

Styling nit, this order change will make sure that user provided classes will overwrite those already defined inside of the component.

Code after:
}>;

/**
 * Wrapper for Command.Item that adds copy link on right/ middle click + visual copy indicator.
 */
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('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>
  );