ronantakizawa/codereview-bench huggingface dataset:
Repository: expo/expo
File: apps/bare-expo/modules/benchmarking/android/src/main/java/expo/modules/benchmark/BenchmarkingBridgeModule.kt (Kotlin)

Code before:
package expo.modules.benchmark

import com.facebook.react.bridge.BaseJavaModule
import com.facebook.react.bridge.ReactMethod
import com.facebook.react.bridge.ReadableArray
import expo.modules.kotlin.iterator

class BenchmarkingBridgeModule : BaseJavaModule() {
  override fun getName(): String = "BenchmarkingBridgeModule"

  @ReactMethod
  fun nothing() {
    // Do nothing
  }

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun addNumbers(a: Double, b: Double): Double {
    return a + b
  }

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun addStrings(a: String, b: String): String {
    return a + b
  }

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun foldArray(array: ReadableArray): Double {
    var sum = 0.0
    var iterator = array.iterator()
    while (iterator.hasNext()) {
      sum += iterator.next().asDouble()
    }

    return sum
  }
}


Review comment (suggestion):
This is weird:
- It started throwing `ParsingException` because of https://github.com/facebook/react-native/commit/c5132f485f6c5af51c6e8133eb5f029a389762dc
  - I understand that async methods should be void and accept a `Promise` arg
  - But totally don't understand why sync methods cannot be `void` :/

I made it [match the TurboModule impl](https://github.com/expo/expo/blob/0075c66a5fd5a92c4ef6f03a92dacd34b2212beb/apps/bare-expo/modules/benchmarking/android/src/main/java/expo/modules/benchmark/NativeBenchmarkingTurboModuleSpec.java#L23-L25) but IMO it would make more sense to do it the other way:
- Turbo/Bridge  `async nothing()` comparison to Expo sync method is pointless
  - Now benchmarking these takes 700ms async vs 100ms sync expo
- I'd change it to sync `nothing(): Int  { return 0 }`
  - We get rid of async overhead at a small cost of returning a constant
  - Benchmark time gets down to ~80ms, which is more reliable time for doing 100,000x nothing

Code after:
package expo.modules.benchmark

import com.facebook.react.bridge.BaseJavaModule
import com.facebook.react.bridge.ReactMethod
import com.facebook.react.bridge.ReadableArray
import expo.modules.kotlin.iterator

class BenchmarkingBridgeModule : BaseJavaModule() {
  override fun getName(): String = "BenchmarkingBridgeModule"

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun nothing(): Double {
    // Do nothing

    // For some reason, isBlockingSynchronousMethod doesn't let functions be Void/Unit
    // so returning a dummy number
    return 0.0
  }

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun addNumbers(a: Double, b: Double): Double {
    return a + b
  }

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun addStrings(a: String, b: String): String {
    return a + b
  }

  @ReactMethod(isBlockingSynchronousMethod = true)
  fun foldArray(array: ReadableArray): Double {
    var sum = 0.0
    var iterator = array.iterator()
    while (iterator.hasNext()) {
      sum += iterator.next().asDouble()
    }

    return sum
  }
}
Repository: expo/expo
File: packages/expo-maps/src/apple/AppleMaps.types.ts (TypeScript)

Code before:
import type { SharedRefType } from 'expo';
import type { Ref } from 'react';
import type { ProcessedColorValue, StyleProp, ViewStyle } from 'react-native';

import { CameraPosition, Coordinates } from '../shared.types';

/**
 * @platform ios
 */
export type AppleMapsMarker = {
  /**
   * The unique identifier for the marker. This can be used to identify the clicked marker in the `onMarkerClick` event.
   */
  id?: string;

  /**
   * The SF Symbol to display for the marker.
   * This is mutually exclusive with `monogram`. If both are provided, `systemImage` takes precedence.
   */
  systemImage?: string;

  /**
   * A short text (typically initials or 1-2 characters) to display on the marker balloon.
   * This is mutually exclusive with `systemImage`. If both are provided, `systemImage` takes precedence.
   */
  monogram?: string;

  /**
   * The coordinates of the marker.
   */
  coordinates?: Coordinates;

  /**
   * The title of the marker, displayed in the callout when the marker is clicked.
   */
  title?: string;

  /**
   * The tint color of the marker.
   */
  tintColor?: string;
};

/**
 * @platform ios
 */
export type AppleMapsUISettings = {
  /**
   * Whether the compass is enabled on the map.

Review comment (suggestion):
```suggestion
   * This is mutually exclusive with `systemImage`. If both are provided, `systemImage` takes precedence.
   * @platform ios 17.0+
```

Let's also add the platform to indiciate that this only works for a certain OS level and above as per https://developer.apple.com/documentation/mapkit/marker/init(_:monogram:coordinate:)-2ojcy

Code after:
import type { SharedRefType } from 'expo';
import type { Ref } from 'react';
import type { ProcessedColorValue, StyleProp, ViewStyle } from 'react-native';

import { CameraPosition, Coordinates } from '../shared.types';

/**
 * @platform ios
 */
export type AppleMapsMarker = {
  /**
   * The unique identifier for the marker. This can be used to identify the clicked marker in the `onMarkerClick` event.
   */
  id?: string;

  /**
   * The SF Symbol to display for the marker.
   * This is mutually exclusive with `monogram`. If both are provided, `systemImage` takes precedence.
   */
  systemImage?: string;

  /**
   * A short text (typically initials or 1-2 characters) to display on the marker balloon.
   * This is mutually exclusive with `systemImage`. If both are provided, `systemImage` takes precedence.
   * @platform ios 17.0+
   */
  monogram?: string;

  /**
   * The coordinates of the marker.
   */
  coordinates?: Coordinates;

  /**
   * The title of the marker, displayed in the callout when the marker is clicked.
   */
  title?: string;

  /**
   * The tint color of the marker.
   */
  tintColor?: string;
};

/**
 * @platform ios
 */
export type AppleMapsUISettings = {
  /**
Repository: expo/expo
File: packages/expo-screen-capture/ios/ScreenCaptureModule.swift (Swift)

Code before:
    }
  }

  @objc
  func preventScreenRecording() {
    guard let keyWindow = keyWindow,
      let visibleView = keyWindow.subviews.first else { return }
    let blockView = getOrCreateBlockView()

    let isCaptured = UIScreen.main.isCaptured
    if isCaptured {
      visibleView.addSubview(blockView)
    } else {
      blockView.removeFromSuperview()
    }
  }

  @objc
  func listenForScreenCapture() {
    sendEvent(onScreenshotEventName, [
      "body": nil
    ])
  }

  private func getOrCreateBlockView() -> UIView {
    guard let blockView = blockView else {
      let view = UIView()
      let boundLength = max(UIScreen.main.bounds.size.width, UIScreen.main.bounds.size.height)
      view.frame = CGRect(x: 0, y: 0, width: boundLength, height: boundLength)
      view.backgroundColor = .black

      self.blockView = view
      return view
    }
    return blockView
  }

  private func preventScreenshots() {
    guard let keyWindow = keyWindow else {
      return
    }

    let textField = UITextField()
    textField.isSecureTextEntry = true
    textField.isUserInteractionEnabled = false
    textField.backgroundColor = UIColor.clear
    textField.frame = UIScreen.main.bounds

    originalParent = keyWindow.layer.superlayer

    keyWindow.layer.superlayer?.addSublayer(textField.layer)

Review comment (suggestion):
```suggestion
    guard let blockView else {
```

Code after:
    }
  }

  @objc
  func preventScreenRecording() {
    guard let keyWindow = keyWindow,
      let visibleView = keyWindow.subviews.first else { return }
    let blockView = getOrCreateBlockView()

    let isCaptured = UIScreen.main.isCaptured
    if isCaptured {
      visibleView.addSubview(blockView)
    } else {
      blockView.removeFromSuperview()
    }
  }

  @objc
  func listenForScreenCapture() {
    sendEvent(onScreenshotEventName, [
      "body": nil
    ])
  }

  private func getOrCreateBlockView() -> UIView {
    guard let blockView else {
      let view = UIView()
      let boundLength = max(UIScreen.main.bounds.size.width, UIScreen.main.bounds.size.height)
      view.frame = CGRect(x: 0, y: 0, width: boundLength, height: boundLength)
      view.backgroundColor = .black

      self.blockView = view
      return view
    }
    return blockView
  }

  private func preventScreenshots() {
    guard let keyWindow = keyWindow else {
      return
    }

    let textField = UITextField()
    textField.isSecureTextEntry = true
    textField.isUserInteractionEnabled = false
    textField.backgroundColor = UIColor.clear
    textField.frame = UIScreen.main.bounds

    originalParent = keyWindow.layer.superlayer

    keyWindow.layer.superlayer?.addSublayer(textField.layer)
Repository: expo/expo
File: packages/expo-screen-capture/ios/ScreenCaptureModule.swift (Swift)

Code before:
import ExpoModulesCore

let onScreenshotEventName = "onScreenshot"

public final class ScreenCaptureModule: Module {
  private var isBeingObserved = false
  private var isListening = false
  private var blockView: UIView? = nil
  private var protectionTextField: UITextField?
  private var originalParent: CALayer?
  private var blurEffectView: AnimatedBlurEffectView?
  private var blurIntensity: CGFloat = 0.5
  private var keyWindow: UIWindow? {
    return UIApplication.shared.connectedScenes
      .flatMap { ($0 as? UIWindowScene)?.windows ?? [] }
      .last { $0.isKeyWindow }
  }

  public func definition() -> ModuleDefinition {
    Name("ExpoScreenCapture")

    Events(onScreenshotEventName)

    OnDestroy {
      allowScreenshots()
      disableAppSwitcherProtection()
    }

    OnStartObserving {
      self.setIsBeing(observed: true)
    }

    OnStopObserving {

Review comment (suggestion):
```suggestion
  private var blockView: UIView?
```

Code after:
import ExpoModulesCore

let onScreenshotEventName = "onScreenshot"

public final class ScreenCaptureModule: Module {
  private var isBeingObserved = false
  private var isListening = false
  private var blockView: UIView?
  private var protectionTextField: UITextField?
  private var originalParent: CALayer?
  private var blurEffectView: AnimatedBlurEffectView?
  private var blurIntensity: CGFloat = 0.5
  private var keyWindow: UIWindow? {
    return UIApplication.shared.connectedScenes
      .flatMap { ($0 as? UIWindowScene)?.windows ?? [] }
      .last { $0.isKeyWindow }
  }

  public func definition() -> ModuleDefinition {
    Name("ExpoScreenCapture")

    Events(onScreenshotEventName)

    OnDestroy {
      allowScreenshots()
      disableAppSwitcherProtection()
    }

    OnStartObserving {
      self.setIsBeing(observed: true)
    }

    OnStopObserving {
Repository: expo/expo
File: apps/expo-go/android/expoview/src/main/java/host/exp/exponent/home/DevSessionRow.kt (Kotlin)

Code before:
import androidx.compose.ui.unit.dp
import coil3.compose.AsyncImage
import host.exp.expoview.R

@Composable
fun DevSessionRow(session: DevSession) {
  val uriHandler = LocalUriHandler.current

  ClickableItemRow(
    onClick = { uriHandler.openUri(session.url) },
    icon = {
      if (session.iconUrl != null) {
        AsyncImage(
          session.iconUrl,
          contentDescription = "Icon",
          modifier = Modifier
            .size(24.dp)
            .clip(shape = RoundedCornerShape(4.dp))
        )
      } else {
        Image(
          painter = if (session.source == DevSessionSource.Desktop) {
            painterResource(id = R.drawable.cli)
          } else {
            painterResource(id = R.drawable.snack)
          },
          contentDescription = "Icon",
          modifier = Modifier
            .size(24.dp)
            .clip(shape = RoundedCornerShape(4.dp))
        )
      }
    }
  ) {
    Column {
      // TODO: Add platform icon
      Text(
        text = session.description
      )
      Text(
        text = session.url,
        style = MaterialTheme.typography.bodySmall
      )
    }
  }
}


Review comment (suggestion):
```suggestion
          painter = painterResource(
            id = if (session.source == DevSessionSource.Desktop) {
              R.drawable.cli
            } else {
            	R.drawable.snack
            }
          ),
```

Code after:
import androidx.compose.ui.unit.dp
import coil3.compose.AsyncImage
import host.exp.expoview.R

@Composable
fun DevSessionRow(session: DevSession) {
  val uriHandler = LocalUriHandler.current

  ClickableItemRow(
    onClick = { uriHandler.openUri(session.url) },
    icon = {
      if (session.iconUrl != null) {
        AsyncImage(
          session.iconUrl,
          contentDescription = "Icon",
          modifier = Modifier
            .size(24.dp)
            .clip(shape = RoundedCornerShape(4.dp))
        )
      } else {
        Image(
          painter = painterResource(
            id = if (session.source == DevSessionSource.Desktop) {
              R.drawable.cli
            } else {
              R.drawable.snack
            }
          ),
          contentDescription = "Icon",
          modifier = Modifier
            .size(24.dp)
            .clip(shape = RoundedCornerShape(4.dp))
        )
      }
    }
  ) {
    Column {
      // TODO: Add platform icon
      Text(
        text = session.description
      )
      Text(
        text = session.url,
        style = MaterialTheme.typography.bodySmall
      )
    }
  }
}
Repository: expo/expo
File: apps/expo-go/android/expoview/src/main/java/host/exp/exponent/home/ProjectDetailsScreen.kt (Kotlin)

Code before:
  onShowAllBranchesClick: () -> Unit,
  bottomBar: @Composable () -> Unit = { },
  appFlow: Flow<ProjectsQuery.ById?>
) {
  val app by appFlow.collectAsStateWithLifecycle(initialValue = null)
  // Should we use the same flow pattern as in ProjectsScreen? Only case is probably account deletion.
  val branches by viewModel
    .branches(app?.id, 5)
    .collectAsStateWithLifecycle(initialValue = null)

  val context = LocalContext.current

  // Filter the branches to only include those that have updates.
  val branchesToRender = branches?.filter { it.updates.isNotEmpty() }

  Scaffold(
    topBar = {
      TopAppBarWithBackIcon(
        app?.name ?: "Project",
        onGoBack = onGoBack,
        actions = {
          IconButton(onClick = {
            val app = app
            if (app != null) {
              shareProjectUrl(context, app.fullName, app.name)
            }
          }) {
            Icon(
              painter = painterResource(id = R.drawable.share),
              contentDescription = "Share Project"
            )
          }
        }
      )
    },
    bottomBar = bottomBar
  ) { padding ->

    if (app == null) {
      return@Scaffold Box(
        modifier = Modifier
          .fillMaxWidth()
          .padding(16.dp),
        contentAlignment = Alignment.Center
      ) {
        CircularProgressIndicator()
      }
    }

    // Use a simple Box for the layout, as pull-to-refresh is not needed
    Column(

Review comment (suggestion):
```suggestion
						app?.let {
							shareProjectUrl(context, it.fullName, it.name)
						}
```

Code after:
  onShowAllBranchesClick: () -> Unit,
  bottomBar: @Composable () -> Unit = { },
  appFlow: Flow<ProjectsQuery.ById?>
) {
  val app by appFlow.collectAsStateWithLifecycle(initialValue = null)
  // Should we use the same flow pattern as in ProjectsScreen? Only case is probably account deletion.
  val branches by viewModel
    .branches(app?.id, 5)
    .collectAsStateWithLifecycle(initialValue = null)

  val context = LocalContext.current

  // Filter the branches to only include those that have updates.
  val branchesToRender = branches?.filter { it.updates.isNotEmpty() }

  Scaffold(
    topBar = {
      TopAppBarWithBackIcon(
        app?.name ?: "Project",
        onGoBack = onGoBack,
        actions = {
          IconButton(onClick = {
            app?.let {
              shareProjectUrl(context, it.fullName, it.name)
            }
          }) {
            Icon(
              painter = painterResource(id = R.drawable.share),
              contentDescription = "Share Project"
            )
          }
        }
      )
    },
    bottomBar = bottomBar
  ) { padding ->

    if (app == null) {
      return@Scaffold Box(
        modifier = Modifier
          .fillMaxWidth()
          .padding(16.dp),
        contentAlignment = Alignment.Center
      ) {
        CircularProgressIndicator()
      }
    }

    // Use a simple Box for the layout, as pull-to-refresh is not needed
    Column(
      modifier = Modifier
Repository: expo/expo
File: packages/expo-sharing/ios/SharingExceptions.swift (Swift)

Code before:
import ExpoModulesCore

internal final class FailedToResolveContentTypeException: GenericException<String>, @unchecked Sendable {
  override var reason: String {
    "Expo-sharing has failed to resolve the content type for \(param)"
  }
}

internal final class FailedToFetchURLContentDetailsException: GenericException<(URL, String)>, @unchecked Sendable {
  override var reason: String {
    "Expo-sharing has failed to fetch content type for \(param.0.absoluteString): \(param.1)"
  }
}

internal final class FailedToResolveAppGroupIdException: Exception, @unchecked Sendable {
  override var reason: String {
    "Expo-sharing has failed to fetch the app group id"
  }
}


Review comment (bug):
```suggestion
internal final class FailedToFetchURLContentDetailsException: GenericException<(url: URL, reason: String)>, @unchecked Sendable {
  override var reason: String {
    "Expo-sharing has failed to fetch content type for \(param.url.absoluteString): \(param.reason)"
```

Code after:
import ExpoModulesCore

internal final class FilePermissionException: Exception, @unchecked Sendable {
  override var reason: String {
    "You don't have access to the provided file"
  }
}

internal final class MissingCurrentViewControllerException: Exception, @unchecked Sendable {
  override var reason: String {
    "Cannot determine currently presented view controller"
  }
}

internal final class UnsupportedTypeException: Exception, @unchecked Sendable {
  override var reason: String {
    "Could not share file since there were no apps registered for its type"
  }
}

internal final class FilePermissionModuleException: Exception, @unchecked Sendable {
  override var reason: String {
    "File permission module not found"
  }
}

internal final class FailedToResolveContentTypeException: GenericException<String>, @unchecked Sendable {
  override var reason: String {
    "Expo-sharing has failed to resolve the content type for \(param)"
  }
}

internal final class FailedToFetchURLContentDetailsException: GenericException<(url: URL, reason: String)>, @unchecked Sendable {
  override var reason: String {
    "Expo-sharing has failed to fetch content type for \(param.url.absoluteString): \(param.reason)"
  }
Repository: expo/expo
File: packages/expo-router/src/native-tabs/types.ts (TypeScript)

Code before:
import type {
  DefaultRouterOptions,
  EventMapBase,
  NavigationState,
  ParamListBase,
  RouteProp,
  ScreenListeners,
  TabNavigationState,
} from '@react-navigation/native';
import type { PropsWithChildren } from 'react';
import type {
  ColorValue,
  ImageSourcePropType,
  StyleProp,
  TextStyle,
  ViewStyle,
} from 'react-native';
import type { BottomTabsScreenProps } from 'react-native-screens';
import type { SFSymbol } from 'sf-symbols-typescript';

/**
 * Event map for NativeTabs navigation events.
 * Only `tabPress` is currently supported.
 */
export type NativeTabNavigationEventMap = {
  tabPress: { data: { __internalTabsType: 'native' }; canPreventDefault: false };
};

export type NativeScreenProps = Partial<Omit<BottomTabsScreenProps, 'tabKey' | 'isFocused'>>;

export interface NativeTabOptions extends DefaultRouterOptions {
  icon?: SymbolOrImageSource;
  selectedIcon?: SymbolOrImageSource;
  title?: string;
  badgeValue?: string;
  selectedLabelStyle?: NativeTabsLabelStyle;
  labelStyle?: NativeTabsLabelStyle;
  role?: NativeTabsTabBarItemRole;
  selectedIconColor?: ColorValue;
  selectedBadgeBackgroundColor?: ColorValue;
  badgeBackgroundColor?: ColorValue;
  badgeTextColor?: ColorValue;
  backgroundColor?: ColorValue;
  blurEffect?: NativeTabsBlurEffect;
  shadowColor?: ColorValue;
  iconColor?: ColorValue;
  disableTransparentOnScrollEdge?: boolean;

Review comment (suggestion):
```suggestion
 * Event map for `NativeTabs` navigation events.
```

Missing syntax highlight

Code after:
import type {
  DefaultRouterOptions,
  EventMapBase,
  NavigationState,
  ParamListBase,
  RouteProp,
  ScreenListeners,
  TabNavigationState,
} from '@react-navigation/native';
import type { PropsWithChildren } from 'react';
import type {
  ColorValue,
  ImageSourcePropType,
  StyleProp,
  TextStyle,
  ViewStyle,
} from 'react-native';
import type { TabsScreenProps } from 'react-native-screens';
import type { SFSymbol } from 'sf-symbols-typescript';

/**
 * Event map for `NativeTabs` navigation events.
 * Only `tabPress` is currently supported.
 */
export type NativeTabNavigationEventMap = {
  tabPress: { data: { __internalTabsType: 'native' }; canPreventDefault: false };
};

export type NativeScreenProps = Partial<Omit<TabsScreenProps, 'tabKey' | 'isFocused'>>;

export interface NativeTabOptions extends DefaultRouterOptions {
  icon?: SymbolOrImageSource;
  selectedIcon?: SymbolOrImageSource;
  title?: string;
  badgeValue?: string;
  selectedLabelStyle?: NativeTabsLabelStyle;
  labelStyle?: NativeTabsLabelStyle;
  role?: NativeTabsTabBarItemRole;
  selectedIconColor?: ColorValue;
  selectedBadgeBackgroundColor?: ColorValue;
  badgeBackgroundColor?: ColorValue;
  badgeTextColor?: ColorValue;
  backgroundColor?: ColorValue;
  blurEffect?: NativeTabsBlurEffect;
  shadowColor?: ColorValue;
  iconColor?: ColorValue;
  disableTransparentOnScrollEdge?: boolean;
Repository: expo/expo
File: packages/expo-modules-core/ios/Fabric/EXHostWrapper.h (C/C++)

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

#import <ExpoModulesCore/Platform.h>

#ifdef __cplusplus
#import <ReactCommon/RCTHost.h>
#endif

/**
 A wrapper around RCTHost. RCTHost isn't directly available in Swift.
 */
NS_SWIFT_NAME(ExpoHostWrapper)
@interface EXHostWrapper : NSObject

#ifdef __cplusplus
- (instancetype _Nonnull)initWithHost:(RCTHost * _Nonnull)host;
#endif

- (nullable UIView *)findViewWithTag:(NSInteger)tag;

- (id)findModuleWithName:(const char *)name lazilyLoadIfNecessary:(BOOL)lazilyLoadIfNecessary;

@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/Platform.h>

#ifdef __cplusplus
#import <ReactCommon/RCTHost.h>
#endif

/**
 A wrapper around RCTHost. RCTHost isn't directly available in Swift.
 */
NS_SWIFT_NAME(ExpoHostWrapper)
@interface EXHostWrapper : NSObject

#ifdef __cplusplus
- (instancetype _Nonnull)initWithHost:(RCTHost * _Nonnull)host;
#endif

- (nullable UIView *)findViewWithTag:(NSInteger)tag;

- (nullable id)findModuleWithName:(nonnull NSString *)name lazilyLoadIfNecessary:(BOOL)lazilyLoadIfNecessary;

@end