ronantakizawa/codereview-bench huggingface dataset:
Repository: expo/expo
File: packages/expo-modules-core/ios/Fabric/EXHostWrapper.mm (Objective-C++)

Code before:
// Copyright 2024-present 650 Industries. All rights reserved.

#import <ExpoModulesCore/EXHostWrapper.h>

#import <ReactCommon/RCTHost.h>
#import <React/RCTSurfacePresenter.h>
#import <React/RCTMountingManager.h>
#import <React/RCTComponentViewRegistry.h>

@implementation EXHostWrapper {
  __weak RCTHost *_host;
}

- (instancetype)initWithHost:(RCTHost *)host
{
  if (self = [super init]) {
    _host = host;
  }
  return self;
}

- (id)findModuleWithName:(const char *)name lazilyLoadIfNecessary:(BOOL)lazilyLoadIfNecessary
{
  RCTModuleRegistry *moduleRegistry = _host.moduleRegistry;
  return [moduleRegistry moduleForName:name lazilyLoadIfNecessary:lazilyLoadIfNecessary];
}

- (nullable UIView *)findViewWithTag:(NSInteger)tag
{
  RCTComponentViewRegistry *componentViewRegistry = _host.surfacePresenter.mountingManager.componentViewRegistry;
  return [componentViewRegistry findComponentViewWithTag:tag];
}

@end


Review comment (suggestion):
```suggestion
- (nullable id)findModuleWithName:(nonnull NSString *)name lazilyLoadIfNecessary:(BOOL)lazilyLoadIfNecessary
```

Code after:
// Copyright 2024-present 650 Industries. All rights reserved.

#import <ExpoModulesCore/EXHostWrapper.h>

#import <ReactCommon/RCTHost.h>
#import <React/RCTSurfacePresenter.h>
#import <React/RCTMountingManager.h>
#import <React/RCTComponentViewRegistry.h>

@implementation EXHostWrapper {
  __weak RCTHost *_host;
}

- (instancetype)initWithHost:(RCTHost *)host
{
  if (self = [super init]) {
    _host = host;
  }
  return self;
}

- (nullable id)findModuleWithName:(nonnull NSString *)name lazilyLoadIfNecessary:(BOOL)lazilyLoadIfNecessary
{
  RCTModuleRegistry *moduleRegistry = _host.moduleRegistry;
  return [moduleRegistry moduleForName:[name UTF8String] lazilyLoadIfNecessary:lazilyLoadIfNecessary];
}

- (nullable UIView *)findViewWithTag:(NSInteger)tag
{
  RCTComponentViewRegistry *componentViewRegistry = _host.surfacePresenter.mountingManager.componentViewRegistry;
  return [componentViewRegistry findComponentViewWithTag:tag];
}

@end
Repository: expo/expo
File: packages/expo-server/src/types.ts (TypeScript)

Code before:
 * @see https://docs.expo.dev/router/reference/middleware/
 */
export interface MiddlewareSettings {
  /** Matcher definition that restricts the middleware to run conditionally. */
  matcher?: MiddlewareMatcher;
}

/**
 * Function type for route loaders. Loaders are executed on the server during
 * SSR/SSG to fetch data required by a route.
 *
 * During SSG (Static Site Generation), the `request` parameter will be `undefined`
 * as there is no HTTP request at build time.
 *
 * @param request - An `ImmutableRequest` with read-only headers and no body access. In SSG, this is `undefined`
 * @param params - Route parameters extracted from the URL path
 * @example
 * ```ts
 * import type { LoaderFunction } from 'expo-server';
 *
 * export const loader: LoaderFunction = async (request, params) => {
 *   const data = await fetchData(params.id);
 *   return { data };
 * };
 * ```
 * @see https://docs.expo.dev/router/web/data-loaders
 */
export type LoaderFunction<T = any> = (
  request: ImmutableRequest | undefined,
  params: Record<string, string | string[]>
) => Promise<T> | T;


Review comment (suggestion):
```suggestion
 * @see [Data loaders](/router/web/data-loaders) for more information.
```

Let's add a link with its subject and use internal link.

Code after:
 * @see https://docs.expo.dev/router/reference/middleware/
 */
export interface MiddlewareSettings {
  /** Matcher definition that restricts the middleware to run conditionally. */
  matcher?: MiddlewareMatcher;
}

/**
 * Function type for route loaders. Loaders are executed on the server during
 * SSR/SSG to fetch data required by a route.
 *
 * During SSG (Static Site Generation), the `request` parameter will be `undefined`
 * as there is no HTTP request at build time.
 *
 * @param request - An `ImmutableRequest` with read-only headers and no body access. In SSG, this is `undefined`
 * @param params - Route parameters extracted from the URL path
 * @example
 * ```ts
 * import type { LoaderFunction } from 'expo-server';
 *
 * export const loader: LoaderFunction = async (request, params) => {
 *   const data = await fetchData(params.id);
 *   return { data };
 * };
 * ```
 * @see [Data loaders](/router/web/data-loaders) for more information.
 */
export type LoaderFunction<T = any> = (
  request: ImmutableRequest | undefined,
  params: Record<string, string | string[]>
) => Promise<T> | T;
Repository: expo/expo
File: packages/expo-dev-launcher/ios/SwiftUI/DevLauncherViewModel.swift (Swift)

Code before:
    guard !Task.isCancelled else {
      return
    }

    var discoveredServers: [DevServer] = []
    await withTaskGroup(of: DevServer?.self) { group in
      for result in results {
        group.addTask {
          return await self.pingDevServer(endpoint: result.endpoint)
        }
      }

      for await server in group {
        if let server {
          discoveredServers.append(server)
        }
      }
    }

    await MainActor.run {
      self.devServers = discoveredServers.sorted { $0.url < $1.url }
    }
  }

  private func pingDevServer(endpoint: NWEndpoint) async -> DevServer? {
    do {
      if let host = try await NetworkUtilities.resolveBundlerEndpoint(
        endpoint: endpoint,
        queue: DispatchQueue.main
      ) {
        return DevServer(url: host, description: host, source: "local")
      }
    } catch {
      // Server not running or not reachable
    }

    return nil
  }

  func showError(_ error: EXDevLauncherAppError) {
    currentError = error
  }

  func showErrorAlert(_ message: String) {
    errorAlertMessage = message
    showingErrorAlert = true
  }

  func dismissErrorAlert() {
    errorAlertMessage = ""
    showingErrorAlert = false

Review comment (suggestion):
Does it need to be run on the main thread? If not, let's remove `@MainActor`

```suggestion
      Task { [weak self, results] in
```

Code after:
      while !Task.isCancelled {
        try? await Task.sleep(nanoseconds: 2_000_000_000)
        if Task.isCancelled {
          break
        }
        await scanLocalDevServers()
      }
    }
  }

  private func startDevServerBrowser() {
    pingTask?.cancel()
    browser?.cancel()

    let params = NWParameters()
    params.includePeerToPeer = true
    params.allowLocalEndpointReuse = true

    browser = NWBrowser(
      for: NWBrowser.Descriptor.bonjourWithTXTRecord(type: BONJOUR_TYPE, domain: nil),
      using: params
    )

    browser?.browseResultsChangedHandler = { [weak self] results, _ in
      guard let self = self else { return }
      Task { @MainActor [weak self, results] in
        guard let self else { return }
        self.pingTask?.cancel()
        self.pingTask = Task {
          defer { self.pingTask = nil }
          await self.pingDiscoveryResults(results.map { result in
            DiscoveryResult(
              name: NetworkUtilities.getNWBrowserResultName(result),
              endpoint: result.endpoint
            )
          })
        }
      }
    }

    browser?.start(queue: .main)
  }

  private func scanLocalDevServers() async {
    guard !Task.isCancelled else {
      return
    }

    var discoveredServers: [DevServer] = []
    await withTaskGroup(of: DevServer?.self) { group in
      for result in NetworkUtilities.getLocalEndpointsToScan() {
Repository: expo/expo
File: packages/expo-dev-launcher/ios/SwiftUI/NetworkUtilities.swift (Swift)

Code before:
      return ["localhost"]
    }

    let subnet = components.prefix(3).joined(separator: ".")

    return [
      "localhost",
      "\(subnet).1"
    ]
  }

  static func resolveBundlerEndpoint(
    endpoint: NWEndpoint,
    queue: DispatchQueue,
  ) async throws -> String? {
    let params = NWParameters.tcp
    params.includePeerToPeer = true
    params.allowFastOpen = true
    params.allowLocalEndpointReuse = true
    params.preferNoProxies = true
    params.requiredInterface = endpoint.interface
    params.expiredDNSBehavior = NWParameters.ExpiredDNSBehavior.allow

    let connection = NWConnection(to: endpoint, using: params)

    try await withCheckedThrowingContinuation { (cont: CheckedContinuation<Void, Error>) in
      connection.stateUpdateHandler = { state in
        let handler = connection.stateUpdateHandler
        connection.stateUpdateHandler = nil
        switch state {
          case .ready:
            cont.resume()
          case .failed(let error):
            cont.resume(throwing: error)
          case .cancelled:
            cont.resume(throwing: ConnectionError.failedConnection)
          default:
            connection.stateUpdateHandler = handler
        }
      }
      connection.start(queue: queue)
    }

    guard let host = connection.currentPath
      .flatMap({ $0.remoteEndpoint })
      .flatMap({ buildHttpHost(endpoint: $0) })
    else {
      throw ConnectionError.noPathToHost
    }

    let requestString = """

Review comment (refactor):
This function would look much nicer if these `withCheckedThrowingContinuation` are extracted to separate async functions

Code after:
          receiveLoop()
        }
      }
    }
    receiveLoop()
  }
  guard let responseString = String(data: responseData, encoding: .utf8) else {
    throw ConnectionError.invalidResponse
  }
  return responseString
}

class NetworkUtilities {
  // Same approach as expo-network just without throwing.
  static func getLocalIPAddress() -> String? {
    var address: String?

    var ifaddr: UnsafeMutablePointer<ifaddrs>?
    guard getifaddrs(&ifaddr) == 0 else { return nil }
    guard let firstAddr = ifaddr else { return nil }

    defer { freeifaddrs(ifaddr) }

    for ifptr in sequence(first: firstAddr, next: { $0.pointee.ifa_next }) {
      let interface = ifptr.pointee

      let addrFamily = interface.ifa_addr.pointee.sa_family
      if addrFamily == UInt8(AF_INET) {
        let name = String(cString: interface.ifa_name)

        if name == "en0" || name == "en1" {
          var hostname = [CChar](repeating: 0, count: Int(NI_MAXHOST))
          getnameinfo(
            interface.ifa_addr,
            socklen_t(interface.ifa_addr.pointee.sa_len),
            &hostname,
            socklen_t(hostname.count),
            nil,
            socklen_t(0),
            NI_NUMERICHOST
          )
          address = String(cString: hostname)
          break
        }
      }
    }

    return address
  }

  static func isSimulator() -> Bool {
Repository: expo/expo
File: templates/expo-template-default/src/components/animated-icon.native.tsx (TypeScript)

Code before:
import { Image } from 'expo-image';
import { Dimensions, StyleSheet, View } from 'react-native';
import Animated, { Easing, Keyframe } from 'react-native-reanimated';

const INITIAL_SCALE_FACTOR = Dimensions.get('screen').height / 90;
const DURATION = 600;

const splashKeyframe = new Keyframe({
  0: {
    transform: [{ scale: INITIAL_SCALE_FACTOR }],
    opacity: 1,
  },
  20: {
    opacity: 1,
  },
  70: {
    opacity: 0,
    easing: Easing.elastic(0.7),
  },
  100: {
    opacity: 0,
    transform: [{ scale: 1 }],
    easing: Easing.elastic(0.7),
  },
});

export function AnimatedSplashOverlay() {
  return (
    <View style={styles.container}>
      <Animated.View

Review comment (suggestion):
this isn't ideal, it's pretty complicated for a simple template like this. i wonder if there is a better way to fire a callback when the animation completes? cc  @tjzel / @tomekzaw

Code after:
import { Image } from 'expo-image';
import { useState } from 'react';
import { Dimensions, StyleSheet, View } from 'react-native';
import Animated, { Easing, Keyframe } from 'react-native-reanimated';
import { scheduleOnRN } from 'react-native-worklets';

const INITIAL_SCALE_FACTOR = Dimensions.get('screen').height / 90;
const DURATION = 600;

const splashKeyframe = new Keyframe({
  0: {
    transform: [{ scale: INITIAL_SCALE_FACTOR }],
    opacity: 1,
  },
  20: {
    opacity: 1,
  },
  70: {
    opacity: 0,
    easing: Easing.elastic(0.7),
  },
  100: {
    opacity: 0,
    transform: [{ scale: 1 }],
    easing: Easing.elastic(0.7),
  },
});

export function AnimatedSplashOverlay() {
  const [visible, setVisible] = useState(true);
Repository: expo/expo
File: packages/expo-server/src/vendor/environment/node.ts (TypeScript)

Code before:
    } catch {
      return null;
    }
  }

  async function readJson(request: string) {
    const json = await readText(request);
    return json != null ? JSON.parse(json) : null;
  }

  async function loadModule(request: string) {
    const filePath = path.join(params.build, request);
    if (!fs.existsSync(filePath)) {
      return null;
    } else if (/\.c?js$/.test(filePath)) {
      return require(filePath);
    } else {
      return await import(filePath);
    }
  }

  return createEnvironment({
    readText,
    readJson,
    loadModule,
    isDevelopment: params.environment === 'development',
  });
}

export function createNodeRequestScope(scopeDefinition: ScopeDefinition, params: NodeEnvParams) {
  return createRequestScope(scopeDefinition, (request: Request) => ({
    origin: request.headers.get('Origin') || 'null',
    environment: params.environment ?? process.env.NODE_ENV,
  }));
}


Review comment (suggestion):
I think `params.environment` was currently unset in `@expo/cli`? This also influences `environment()` from `expo-server`, which was meant to be a hosting provider-specific flag. We could reuse it, but probably safer to update `@expo/cli` and pass a flag all the way through

Code after:
      return await fs.promises.readFile(filePath, 'utf-8');
    } catch {
      return null;
    }
  }

  async function readJson(request: string) {
    const json = await readText(request);
    return json != null ? JSON.parse(json) : null;
  }

  async function loadModule(request: string) {
    const filePath = path.join(params.build, request);
    if (!fs.existsSync(filePath)) {
      return null;
    } else if (/\.c?js$/.test(filePath)) {
      return require(filePath);
    } else {
      return await import(filePath);
    }
  }

  return createEnvironment({
    readText,
    readJson,
    loadModule,
    isDevelopment: params.isDevelopment ?? false,
  });
}

export function createNodeRequestScope(scopeDefinition: ScopeDefinition, params: NodeEnvParams) {
  return createRequestScope(scopeDefinition, (request: Request) => ({
    origin: request.headers.get('Origin') || 'null',
    environment: params.environment ?? process.env.NODE_ENV,
  }));
}
Repository: expo/expo
File: packages/@expo/fingerprint/src/hash/__tests__/Hash-ReactImportsPatcher-test.ts (TypeScript)

Code before:
import { vol } from 'memfs';
import pLimit from 'p-limit';
import path from 'path';

import { normalizeOptionsAsync } from '../../Options';
import { createLimiter } from '../../utils/Concurrency';
import { createFileHashResultsAsync } from '../Hash';
import { ReactImportsPatchTransform } from '../ReactImportsPatcher';

jest.mock('fs');
jest.mock('fs/promises');
jest.mock('../../ProjectWorkflow');
jest.mock('../ReactImportsPatcher', () => ({
  ReactImportsPatchTransform: jest
    .fn()
    .mockImplementation(jest.requireActual('stream').PassThrough),
}));

describe(`createFileHashResultsAsync - use ReactImportsPatchTransform`, () => {
  afterEach(() => {
    vol.reset();
    jest.resetAllMocks();
  });

  it('should use ReactImportsPatchTransform when `enableReactImportsPatcher=false`', async () => {
    const limiter = createLimiter(1);
    const options = await normalizeOptionsAsync('/app', {
      debug: true,
      enableReactImportsPatcher: false,
    });

Review comment (suggestion):
[🚨 **critical**] **critical**: Incomplete migration — `p-limit` has been removed from `package.json` but is still imported on line 3 (`import pLimit from 'p-limit'`). Two test cases below still use `pLimit(1)` instead of `createLimiter(1)` (lines 51 and 73: `"should not use ReactImportsPatchTransform if no match platforms"` and `"should not use ReactImportsPatchTransform if no match files"`). These tests will break once a hoisted copy of `p-limit` is no longer available in the monorepo.

The fix is straightforward — replace the remaining `pLimit(1)` calls with `createLimiter(1)` and remove the `p-limit` import.

Code after:
import { vol } from 'memfs';
import path from 'path';

import { normalizeOptionsAsync } from '../../Options';
import { createLimiter } from '../../utils/Concurrency';
import { createFileHashResultsAsync } from '../Hash';
import { ReactImportsPatchTransform } from '../ReactImportsPatcher';

jest.mock('fs');
jest.mock('fs/promises');
jest.mock('../../ProjectWorkflow');
jest.mock('../ReactImportsPatcher', () => ({
  ReactImportsPatchTransform: jest
    .fn()
    .mockImplementation(jest.requireActual('stream').PassThrough),
}));

describe(`createFileHashResultsAsync - use ReactImportsPatchTransform`, () => {
  afterEach(() => {
    vol.reset();
    jest.resetAllMocks();
  });

  it('should use ReactImportsPatchTransform when `enableReactImportsPatcher=false`', async () => {
    const limiter = createLimiter(1);
    const options = await normalizeOptionsAsync('/app', {
      debug: true,
      enableReactImportsPatcher: false,
    });
    const files = {
Repository: expo/expo
File: packages/@expo/cli/src/export/index.ts (TypeScript)

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

import { Command } from '../../bin/cli';
import { assertArgs, getProjectRoot, printHelp } from '../utils/args';
import { logCmdError } from '../utils/errors';

export const expoExport: Command = async (argv) => {
  const args = assertArgs(
    {
      // Types
      '--help': Boolean,
      '--clear': Boolean,
      '--dump-assetmap': Boolean,
      '--dev': Boolean,
      '--source-maps': Boolean,
      '--inline-source-maps': Boolean,
      '--max-workers': Number,
      '--output-dir': String,
      '--platform': [String],
      '--no-minify': Boolean,
      '--no-bytecode': Boolean,
      '--no-ssg': Boolean,
      '--api-only': Boolean,
      '--unstable-hosted-native': Boolean,

      // Hack: This is added because EAS CLI always includes the flag.
      // If supplied, we'll do nothing with the value, but at least the process won't crash.
      // Note that we also don't show this value in the `--help` prompt since we don't want people to use it.
      '--experimental-bundle': Boolean,

      // Aliases
      '-h': '--help',
      '-s': '--source-maps',
      // '-d': '--dump-assetmap',
      '-c': '--clear',
      '-p': '--platform',
      // Interop with Metro docs and RedBox errors.
      '--reset-cache': '--clear',

      // Deprecated
      '--dump-sourcemap': '--source-maps',

Review comment (suggestion):
```suggestion
      '--source-maps': Boolean,
```
I'd prefer `--source-maps inline` since it indicates they must be enabled they should use a particular format

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

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

export const expoExport: Command = async (argv) => {
  const rawArgsMap: arg.Spec = {
    // Types
    '--help': Boolean,
    '--clear': Boolean,
    '--dump-assetmap': Boolean,
    '--dev': Boolean,
    '--max-workers': Number,
    '--output-dir': String,
    '--platform': [String],
    '--no-minify': Boolean,
    '--no-bytecode': Boolean,
    '--no-ssg': Boolean,
    '--api-only': Boolean,
    '--unstable-hosted-native': Boolean,

    // Hack: This is added because EAS CLI always includes the flag.
    // If supplied, we'll do nothing with the value, but at least the process won't crash.
    // Note that we also don't show this value in the `--help` prompt since we don't want people to use it.
    '--experimental-bundle': Boolean,

    // Aliases
    '-h': '--help',
    // '-d': '--dump-assetmap',
    '-c': '--clear',
    '-p': '--platform',
    // Interop with Metro docs and RedBox errors.
    '--reset-cache': '--clear',
  };

  const args = assertWithOptionsArgs(rawArgsMap, {
    argv,
    permissive: true,
Repository: expo/expo
File: packages/@expo/cli/src/export/index.ts (TypeScript)

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

import { Command } from '../../bin/cli';
import { assertArgs, getProjectRoot, printHelp } from '../utils/args';
import { logCmdError } from '../utils/errors';

/**
 * Preprocess argv to handle --source-maps with optional value.
 * If --source-maps or -s is followed by another flag (starts with -) or end of args,
 * insert 'true' as the default value.
 */
function preprocessSourceMapsArg(argv: string[]): string[] {
  const result: string[] = [];
  for (let i = 0; i < argv.length; i++) {
    const arg = argv[i];
    result.push(arg);

    if (arg === '--source-maps' || arg === '-s' || arg === '--dump-sourcemap') {
      const nextArg = argv[i + 1];
      // If no next arg or next arg is a flag, insert 'true' as the value
      if (nextArg === undefined || nextArg.startsWith('-')) {
        result.push('true');
      }
    }
  }
  return result;
}

export const expoExport: Command = async (argv) => {
  // Preprocess argv to handle --source-maps with optional value
  // If --source-maps is followed by another flag or end of args, insert 'true' as the value
  const processedArgv = argv ? preprocessSourceMapsArg(argv) : undefined;

  const args = assertArgs(
    {
      // Types
      '--help': Boolean,

Review comment (suggestion):
Prefer `  const { resolveStringOrBooleanArgsAsync } = await import('../../utils/resolveArgs.js');` like we use in run:ios and run:android for `--scheme` | `--scheme <name>`

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

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

export const expoExport: Command = async (argv) => {
  const rawArgsMap: arg.Spec = {
    // Types
    '--help': Boolean,
    '--clear': Boolean,
    '--dump-assetmap': Boolean,
    '--dev': Boolean,
    '--max-workers': Number,
    '--output-dir': String,
    '--platform': [String],
    '--no-minify': Boolean,
    '--no-bytecode': Boolean,
    '--no-ssg': Boolean,
    '--api-only': Boolean,
    '--unstable-hosted-native': Boolean,

    // Hack: This is added because EAS CLI always includes the flag.
    // If supplied, we'll do nothing with the value, but at least the process won't crash.
    // Note that we also don't show this value in the `--help` prompt since we don't want people to use it.
    '--experimental-bundle': Boolean,

    // Aliases
    '-h': '--help',
    // '-d': '--dump-assetmap',
    '-c': '--clear',
    '-p': '--platform',
    // Interop with Metro docs and RedBox errors.
    '--reset-cache': '--clear',
  };