ronantakizawa/codereview-bench huggingface dataset:
Repository: adobe/react-spectrum
File: packages/@react-aria/utils/src/useEffectEvent.ts (TypeScript)

Code before:
 * governing permissions and limitations under the License.
 */

import React, {useCallback, useRef} from 'react';
import {useLayoutEffect} from './useLayoutEffect';

// Use the earliest effect type possible. useInsertionEffect runs during the mutation phase,
// before all layout effects, but is available only in React 18 and later.
const useEarlyEffect = React['useInsertionEffect'] ?? useLayoutEffect;

// Starting with React 19.2, this hook has been internalized.
const useModernEffectEvent = React['useEffectEvent'] ?? useLegacyEffectEvent;

function useLegacyEffectEvent<T extends Function>(fn?: T): T {
  const ref = useRef<T | null | undefined>(null);
  useEarlyEffect(() => {
    ref.current = fn;
  }, [fn]);
  // @ts-ignore
  return useCallback<T>((...args) => {
    const f = ref.current!;
    return f?.(...args);
  }, []);
}

export function useEffectEvent<T extends Function>(fn?: T): T {
  let noop = useCallback(() => {}, []);
  return useModernEffectEvent(fn ?? noop);
}


Review comment (suggestion):
I'm not really sure why `fn` was optional.... this is an internal hook for us (not documented), so I think we should probably have it be required, then we can get rid of this extra noop function

Code after:
 * governing permissions and limitations under the License.
 */

import React, {useCallback, useRef} from 'react';
import {useLayoutEffect} from './useLayoutEffect';

// Use the earliest effect type possible. useInsertionEffect runs during the mutation phase,
// before all layout effects, but is available only in React 18 and later.
const useEarlyEffect = React['useInsertionEffect'] ?? useLayoutEffect;

// Starting with React 19.2, this hook has been internalized.
const useModernEffectEvent = React['useEffectEvent'] ?? useLegacyEffectEvent;

function useLegacyEffectEvent<T extends Function>(fn?: T): T {
  const ref = useRef<T | null | undefined>(null);
  useEarlyEffect(() => {
    ref.current = fn;
  }, [fn]);
  // @ts-ignore
  return useCallback<T>((...args) => {
    const f = ref.current!;
    return f?.(...args);
  }, []);
}

export function useEffectEvent<T extends Function>(fn: T): T {
  return useModernEffectEvent(fn);
}
Repository: adobe/react-spectrum
File: packages/@react-spectrum/s2/src/Menu.tsx (TypeScript)

Code before:
  return content;
});

export function Divider(props: SeparatorProps): ReactNode {
  return (
    <Separator
      {...props}
      className={mergeStyles(
        divider({
          size: 'M',
          orientation: 'horizontal',
          isStaticColor: false
        }), style({
          display: {
            default: 'grid',
            ':last-child': 'none'
          },
          gridColumnStart: 2,
          gridColumnEnd: -2,
          marginY: size(5) // height of the menu separator is 12px, and the divider is 2px
        })
      )} />
  );
}

export interface MenuSectionProps<T extends object> extends Omit<AriaMenuSectionProps<T>, 'className' | keyof GlobalDOMAttributes> {
  /**
   * The children of the menu section.
   */
  children?: ReactNode
}
export function MenuSection<T extends object>(props: MenuSectionProps<T>): ReactNode {
  // remember, context doesn't work if it's around Section nor inside
  let {size} = useContext(InternalMenuContext);
  return (
    <>
      <AriaMenuSection
        {...props}
        className={section({size})}>
        {props.children}
      </AriaMenuSection>
      <Divider />
    </>
  );
}

export interface MenuItemProps extends Omit<AriaMenuItemProps, 'children' | 'style' | 'className' | 'onClick' | keyof GlobalDOMAttributes>, StyleProps {
  /**
   * The contents of the item.
   */
  children: ReactNode

Review comment (suggestion):
```suggestion
export interface MenuSectionProps<T extends object> extends Omit<AriaMenuSectionProps<T>, 'style' | 'className' | keyof GlobalDOMAttributes> {
```

Code after:
  return content;
});

export function Divider(props: SeparatorProps): ReactNode {
  return (
    <Separator
      {...props}
      className={mergeStyles(
        divider({
          size: 'M',
          orientation: 'horizontal',
          isStaticColor: false
        }), style({
          display: {
            default: 'grid',
            ':last-child': 'none'
          },
          gridColumnStart: 2,
          gridColumnEnd: -2,
          marginY: size(5) // height of the menu separator is 12px, and the divider is 2px
        })
      )} />
  );
}

export interface MenuSectionProps<T extends object> extends Omit<AriaMenuSectionProps<T>, 'style' | 'className' | keyof GlobalDOMAttributes> {
  /**
   * The children of the menu section.
   */
  children?: ReactNode
}
export function MenuSection<T extends object>(props: MenuSectionProps<T>): ReactNode {
  // remember, context doesn't work if it's around Section nor inside
  let {size} = useContext(InternalMenuContext);
  return (
    <>
      <AriaMenuSection
        {...props}
        className={section({size})}>
        {props.children}
      </AriaMenuSection>
      <Divider />
    </>
  );
}

export interface MenuItemProps extends Omit<AriaMenuItemProps, 'children' | 'style' | 'className' | 'onClick' | keyof GlobalDOMAttributes>, StyleProps {
  /**
   * The contents of the item.
   */
  children: ReactNode
Repository: airbnb/lottie-ios
File: lottie-ios/Classes/AnimatableLayers/LOTLayerView.h (C/C++)

Code before:
//
//  LOTLayerView.h
//  LottieAnimator
//
//  Created by Brandon Withrow on 12/14/15.
//  Copyright © 2015 Brandon Withrow. All rights reserved.
//

#import "LOTPlatformCompat.h"
#import "LOTAnimatableLayer.h"

#import "LOTModels.h"

@interface LOTLayerView : LOTAnimatableLayer

- (instancetype)initWithModel:(LOTLayer *)model inLayerGroup:(LOTLayerGroup *)layerGroup inBundle:(NSBundle *)bundle;

- (void)LOT_addChildLayer:(CALayer *)childLayer;

@property (nonatomic, readonly) LOTLayer *layerModel;
@property (nonatomic, assign) BOOL debugModeOn;

@end


Review comment (suggestion):
can you leave `- (instancetype)initWithModel:(LOTLayer *)model inLayerGroup:(LOTLayerGroup *)layerGroup;` so that the header will maintain legacy support? Just add support for passing in nil bundle and have the shorter method call the longer one internally.

Code after:
//
//  LOTLayerView.h
//  LottieAnimator
//
//  Created by Brandon Withrow on 12/14/15.
//  Copyright © 2015 Brandon Withrow. All rights reserved.
//

#import "LOTPlatformCompat.h"
#import "LOTAnimatableLayer.h"

#import "LOTModels.h"

@interface LOTLayerView : LOTAnimatableLayer

- (instancetype)initWithModel:(LOTLayer *)model inLayerGroup:(LOTLayerGroup *)layerGroup;
- (instancetype)initWithModel:(LOTLayer *)model inLayerGroup:(LOTLayerGroup *)layerGroup inBundle:(NSBundle *)bundle;

- (void)LOT_addChildLayer:(CALayer *)childLayer;

@property (nonatomic, readonly) LOTLayer *layerModel;
@property (nonatomic, assign) BOOL debugModeOn;

@end
Repository: airbnb/lottie-ios
File: lottie-swift/src/Private/Utility/Primitives/ColorExtension.swift (Swift)

Code before:
  }
  
  public func encode(to encoder: Encoder) throws {
    var container = encoder.unkeyedContainer()
    try container.encode(r)
    try container.encode(g)
    try container.encode(b)
    try container.encode(a)
  }
  
}

extension Color {
  
  static var clearColor: CGColor {
    return CGColor(colorSpace: CGColorSpaceCreateDeviceRGB(), components: [0, 0, 0, 0])!
  }
  
  var cgColorValue: CGColor {
    var colorspace : CGColorSpace?

    if #available(iOS 9.3, *) {
        // Always use P3 colorspace for now
        colorspace = CGColorSpace.init(name: CGColorSpace.displayP3)
    }

    return CGColor(colorSpace: colorspace ?? CGColorSpaceCreateDeviceRGB(), components: [CGFloat(r), CGFloat(g), CGFloat(b), CGFloat(a)]) ?? Color.clearColor
  }
}


Review comment (suggestion):
For easier to follow code try this:

```
let colorspace : CGColorSpace
if #available(iOS 9.3, *) {
    // Always use P3 colorspace for now
    colorspace = CGColorSpace.init(name: CGColorSpace.displayP3)
} else {
	colorspace = CGColorSpaceCreateDeviceRGB()
}
return CGColor(colorSpace: colorspace, components: [CGFloat(r), CGFloat(g), CGFloat(b), CGFloat(a)]) ?? Color.clearColor
```

Code after:
  }
  
  public func encode(to encoder: Encoder) throws {
    var container = encoder.unkeyedContainer()
    try container.encode(r)
    try container.encode(g)
    try container.encode(b)
    try container.encode(a)
  }
  
}

extension Color {
  
  static var clearColor: CGColor {
    return CGColor(colorSpace: CGColorSpaceCreateDeviceRGB(), components: [0, 0, 0, 0])!
  }
  
  var cgColorValue: CGColor {
    let colorspace : CGColorSpace
    if #available(iOS 9.3, *) {
      // Always use P3 colorspace for now
      colorspace = CGColorSpace.init(name: CGColorSpace.displayP3) ?? CGColorSpaceCreateDeviceRGB()
    } else {
      colorspace = CGColorSpaceCreateDeviceRGB()
    }
    return CGColor(colorSpace: colorspace, components: [CGFloat(r), CGFloat(g), CGFloat(b), CGFloat(a)]) ?? Color.clearColor
  }
}
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/ExperimentalFeatureConfiguration.swift (Swift)

Code before:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

/// Configuration for experimental features that are in development
///
/// Experimental features are not considered stable and are subject
/// to change or be removed at any time.
public struct ExperimentalFeatureConfiguration {

  /// The singleton configuration for experimental features,
  /// which applies to all `AnimationView`s by default.
  public static var shared = ExperimentalFeatureConfiguration()

  /// Whether or not to use the new, experimental, rendering engine
  public var useNewRenderingEngine = false

}


Review comment (suggestion):
Might be useful to use this in the future for not just experimental features but also for risky changes, which is what we do for Epoxy (e.g. enabling UICV prefetching). That way you can have simple "feature flags" for Lottie, especially with other changes to the renderer

Code after:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

/// Configuration for experimental features that are in development
///
/// Experimental features are not considered stable and are subject
/// to change or be removed at any time.
public struct ExperimentalFeatureConfiguration {

  /// The singleton configuration for experimental features,
  /// which applies to all `AnimationView`s by default.
  public static var shared = ExperimentalFeatureConfiguration()

  /// Whether or not to use the new, experimental, rendering engine,
  /// which leverages the Core Animation render server to
  /// animate without executing on the main thread every frame.
  public var useNewRenderingEngine = false

}
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Extensions/CALayer+fillBounds.swift (Swift)

Code before:
// Created by Cal Stephens on 12/15/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

extension CALayer {
  /// Updates the `bounds` of this layer to fill the bounds of its `superlayer`
  /// without setting `frame` (which is not permitted if the layer can rotate)
  func fillBoundsOfSuperlayer() {
    guard let superlayer = superlayer else { return }

    // By default the `anchorPoint` of a layer is `CGPoint(x: 0.5, y: 0.5)`.
    // Setting it to `.zero` makes the layer have the same coordinate space
    // as its superlayer, which lets use use `superlayer.bounds` directly.
    anchorPoint = .zero

    bounds = superlayer.bounds
  }
}


Review comment (suggestion):
nit: specify`@nonobjc` if this is only called from swift?
```suggestion
  @nonobjc
  func fillBoundsOfSuperlayer() {
```

Code after:
// Created by Cal Stephens on 12/15/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

extension CALayer {
  /// Updates the `bounds` of this layer to fill the bounds of its `superlayer`
  /// without setting `frame` (which is not permitted if the layer can rotate)
  @nonobjc
  func fillBoundsOfSuperlayer() {
    guard let superlayer = superlayer else { return }

    // By default the `anchorPoint` of a layer is `CGPoint(x: 0.5, y: 0.5)`.
    // Setting it to `.zero` makes the layer have the same coordinate space
    // as its superlayer, which lets use use `superlayer.bounds` directly.
    anchorPoint = .zero

    bounds = superlayer.bounds
  }
}
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Extensions/KeyframeGroup+CAKeyframeAnimation.swift (Swift)

Code before:
// Created by Cal Stephens on 12/14/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

// MARK: - CAKeyPath

/// A strongly typed value that can be used as the `keyPath` of a CAAnimation
struct CAKeyPath<ValueRepresentation> {
  let name: String
}

// MARK: ExpressibleByStringLiteral

extension CAKeyPath: ExpressibleByStringLiteral {
  init(stringLiteral value: StringLiteralType) {
    name = value
  }
}

// MARK: keyPath definitions

/// Supported key paths and their expected value types are described
/// at https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CoreAnimation_guide/AnimatableProperties/AnimatableProperties.html#//apple_ref/doc/uid/TP40004514-CH11-SW1
/// and https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CoreAnimation_guide/Key-ValueCodingExtensions/Key-ValueCodingExtensions.html
extension CAKeyPath {
  static var position: CAKeyPath<CGPoint> { "transform.translation" }
  static var scale: CAKeyPath<CGFloat> { "transform.scale" }
  static var scaleX: CAKeyPath<CGFloat> { "transform.scale.x" }
  static var scaleY: CAKeyPath<CGFloat> { "transform.scale.y" }

  static var anchorPoint: CAKeyPath<CGPoint> { "anchorPoint" }
  static var opacity: CAKeyPath<CGFloat> { "opacity" }

Review comment (suggestion):
nit:
```suggestion
/// A strongly typed value that can be used as the `keyPath` of a `CAAnimation`
```

Code after:
// Created by Cal Stephens on 12/14/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

// MARK: - CAKeyPath

/// A strongly typed value that can be used as the `keyPath` of a `CAAnimation`
struct CAKeyPath<ValueRepresentation> {
  let name: String

  init(_ name: String) {
    self.name = name
  }
}

// MARK: keyPath definitions

/// Supported key paths and their expected value types are described
/// at https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CoreAnimation_guide/AnimatableProperties/AnimatableProperties.html#//apple_ref/doc/uid/TP40004514-CH11-SW1
/// and https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CoreAnimation_guide/Key-ValueCodingExtensions/Key-ValueCodingExtensions.html
extension CAKeyPath {
  static var position: CAKeyPath<CGPoint> { .init("transform.translation") }
  static var scale: CAKeyPath<CGFloat> { .init("transform.scale") }
  static var scaleX: CAKeyPath<CGFloat> { .init("transform.scale.x") }
  static var scaleY: CAKeyPath<CGFloat> { .init("transform.scale.y") }

  static var anchorPoint: CAKeyPath<CGPoint> { .init(#keyPath(CALayer.anchorPoint)) }
  static var opacity: CAKeyPath<CGFloat> { .init(#keyPath(CALayer.opacity)) }
}

// MARK: - KeyframeGroup + CAKeyframeAnimation
Repository: airbnb/lottie-ios
File: Tests/Snapshotting+presentationLayer.swift (Swift)

Code before:
    precision: Float = 1,
    traits: UITraitCollection = .init())
    -> Snapshotting<UIView, UIImage>
  {
    SimplySnapshotting
      .image(precision: precision, scale: traits.displayScale)
      .pullback { (view: UIView) -> UIImage in
        // Place the view in an on-screen window and then
        // have Core Animation update the screen synchronously
        let window = UIWindow()
        window.isHidden = false
        window.addSubview(view)

        // Calling CATransaction.flush() makes Core Animation synchronously update
        // the content being displayed on-screen, which gives our view a presentation layer.
        CATransaction.flush()

        // Now that the view is on-screen, it has a presentation layer:
        guard let presentationLayer = view.layer.presentation() else {
          fatalError("Presentation layer does not exist and cannot be snapshot")
        }

        let image = UIGraphicsImageRenderer(bounds: view.bounds).image { context in
          presentationLayer.render(in: context.cgContext)
        }

        return image
      }
  }
}


Review comment (bug):
Is it possible to throw here so the test fails rather than crashing? Or does `pullback` not rethrow?

Code after:
  static func imageOfPresentationLayer(
    precision: Float = 1,
    traits: UITraitCollection = .init())
    -> Snapshotting<UIView, UIImage>
  {
    // Use the SnapshotTesting framework's base `SimplySnapshot.image`
    // implementation for creating and diffing image files
    SimplySnapshotting.image(precision: precision, scale: traits.displayScale)
      // Convert the input `UIView` into a `UIImage`
      // by creating and then rendering its `layer.presentation()`
      .pullback { (view: UIView) -> UIImage in
        // Place the view in an on-screen window and then
        // have Core Animation update the screen synchronously
        let window = UIWindow()
        window.bounds = view.bounds
        window.isHidden = false
        window.addSubview(view)

        // Calling CATransaction.flush() makes Core Animation synchronously update
        // the content being displayed on-screen, which gives our view a presentation layer.
        CATransaction.flush()

        // Now that the view is on-screen, it has a presentation layer:
        guard let presentationLayer = view.layer.presentation() else {
          fatalError("Presentation layer does not exist and cannot be snapshot")
        }

        let image = UIGraphicsImageRenderer(bounds: view.bounds).image { context in
          presentationLayer.render(in: context.cgContext)
        }

        return image
      }
  }
}
#endif
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/ExperimentalAnimationLayer.swift (Swift)

Code before:
  }

  func getValue(for _: AnimationKeypath, atFrame _: AnimationFrameTime?) -> Any? {
    fatalError("Currently unsupported")
  }

  func layer(for _: AnimationKeypath) -> CALayer? {
    fatalError("Currently unsupported")
  }

  func animatorNodes(for _: AnimationKeypath) -> [AnimatorNode]? {
    fatalError("Currently unsupported")
  }

}

// MARK: - CALayer + allSublayers

extension CALayer {
  /// All of the layers in the layer tree that are descendants from this later
  @nonobjc
  fileprivate var allSublayers: [CALayer] {
    var allSublayers: [CALayer] = []

    for sublayer in sublayers ?? [] {
      allSublayers += [sublayer] + sublayer.allSublayers
    }

    return allSublayers
  }
}


Review comment (suggestion):
nit: seems like this will create less throwaway arrays
```suggestion
      allSublayers.append(sublayer)
      allSublayers.append(contentsOf: sublayer.allSublayers)
```


Code after:
  }

  func getValue(for _: AnimationKeypath, atFrame _: AnimationFrameTime?) -> Any? {
    fatalError("Currently unsupported")
  }

  func layer(for _: AnimationKeypath) -> CALayer? {
    fatalError("Currently unsupported")
  }

  func animatorNodes(for _: AnimationKeypath) -> [AnimatorNode]? {
    fatalError("Currently unsupported")
  }

}

// MARK: - CALayer + allSublayers

extension CALayer {
  /// All of the layers in the layer tree that are descendants from this later
  @nonobjc
  fileprivate var allSublayers: [CALayer] {
    var allSublayers: [CALayer] = []

    for sublayer in sublayers ?? [] {
      allSublayers.append(sublayer)
      allSublayers.append(contentsOf: sublayer.allSublayers)
    }

    return allSublayers
  }
}
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Animations/VisibilityAnimation.swift (Swift)

Code before:
// Created by Cal Stephens on 12/21/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

extension CALayer {
  /// Adds an animation for the given `inTime` and `outTime` to this `CALayer`
  func addVisibilityAnimation(
    inFrame: AnimationFrameTime,
    outFrame: AnimationFrameTime,
    context: LayerAnimationContext)
  {
    let animation = CAKeyframeAnimation(keyPath: "hidden")
    animation.calculationMode = .discrete

    animation.values = [
      true, // hidden, before `inFrame`
      false, // visible
      true, // hidden, after `outFrame`
    ]

    // From the documentation of `keyTimes`:
    //  - If the calculationMode is set to discrete, the first value in the array
    //    must be 0.0 and the last value must be 1.0. The array should have one more
    //    entry than appears in the values array. For example, if there are two values,
    //    there should be three key times.
    animation.keyTimes = [
      NSNumber(value: 0.0),
      NSNumber(value: Double(context.relativeTime(of: inFrame))),
      NSNumber(value: Double(context.relativeTime(of: outFrame))),
      NSNumber(value: 1.0),
    ]

    animation.configureTiming(with: context)
    add(animation, forKey: "hidden")
  }
}


Review comment (suggestion):
I'm pretty sure that `#keyPath(CALayer.isHidden)` resolves to "hidden" if you want to use it here
```suggestion
    let animation = CAKeyframeAnimation(keyPath: #keyPath(CALayer.isHidden))
```

Code after:
// Created by Cal Stephens on 12/21/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

extension CALayer {
  /// Adds an animation for the given `inTime` and `outTime` to this `CALayer`
  func addVisibilityAnimation(
    inFrame: AnimationFrameTime,
    outFrame: AnimationFrameTime,
    context: LayerAnimationContext)
  {
    let animation = CAKeyframeAnimation(keyPath: #keyPath(isHidden))
    animation.calculationMode = .discrete

    animation.values = [
      true, // hidden, before `inFrame`
      false, // visible
      true, // hidden, after `outFrame`
    ]

    // From the documentation of `keyTimes`:
    //  - If the calculationMode is set to discrete, the first value in the array
    //    must be 0.0 and the last value must be 1.0. The array should have one more
    //    entry than appears in the values array. For example, if there are two values,
    //    there should be three key times.
    animation.keyTimes = [
      NSNumber(value: 0.0),
      NSNumber(value: Double(context.relativeTime(of: inFrame))),
      NSNumber(value: Double(context.relativeTime(of: outFrame))),
      NSNumber(value: 1.0),
    ]

    animation.configureTiming(with: context)
    add(animation, forKey: #keyPath(isHidden))
  }
}