ronantakizawa/codereview-bench huggingface dataset:
Repository: actix/actix-web
File: actix-web/src/test/mod.rs (Rust)

Code before:
//! Various helpers for Actix applications to use during testing.
//!
//! # Creating A Test Service
//! - [`init_service`]
//!
//! # Off-The-Shelf Test Services
//! - [`ok_service`]
//! - [`status_service`]
//!
//! # Calling Test Service
//! - [`TestRequest`]
//! - [`call_service`]
//! - [`try_call_service`]
//! - [`call_and_read_body`]
//! - [`try_call_and_read_body`]
//! - [`call_and_read_body_json`]
//! - [`try_call_and_read_body_json`]
//!
//! # Reading Response Payloads
//! - [`read_body`]
//! - [`try_read_body`]
//! - [`read_body_json`]
//! - [`try_read_body_json`]

// TODO: more docs on generally how testing works with these parts

pub use actix_http::test::TestBuffer;

mod test_request;
mod test_services;
mod test_utils;

pub use self::test_request::TestRequest;
#[allow(deprecated)]
pub use self::test_services::{default_service, ok_service, simple_service, status_service};
#[allow(deprecated)]
pub use self::test_utils::{
    call_and_read_body, call_and_read_body_json, call_service, init_service, read_body,
    read_body_json, read_response, read_response_json,
};

#[cfg(test)]
pub(crate) use self::test_utils::try_init_service;

/// Reduces boilerplate code when testing expected response payloads.
///
/// Must be used inside an async test. Works for both `ServiceRequest` and `HttpRequest`.

Review comment (refactor):
Looks like I didn't re-export these with `pub use` below according to doc lint. 

- [x] re-export new test helpers in mod.rs

Code after:
//! Various helpers for Actix applications to use during testing.
//!
//! # Creating A Test Service
//! - [`init_service`]
//!
//! # Off-The-Shelf Test Services
//! - [`ok_service`]
//! - [`status_service`]
//!
//! # Calling Test Service
//! - [`TestRequest`]
//! - [`call_service`]
//! - [`try_call_service`]
//! - [`call_and_read_body`]
//! - [`call_and_read_body_json`]
//! - [`try_call_and_read_body_json`]
//!
//! # Reading Response Payloads
//! - [`read_body`]
//! - [`try_read_body`]
//! - [`read_body_json`]
//! - [`try_read_body_json`]

// TODO: more docs on generally how testing works with these parts

pub use actix_http::test::TestBuffer;

mod test_request;
mod test_services;
mod test_utils;

pub use self::test_request::TestRequest;
#[allow(deprecated)]
pub use self::test_services::{default_service, ok_service, simple_service, status_service};
#[allow(deprecated)]
pub use self::test_utils::{
    call_and_read_body, call_and_read_body_json, call_service, init_service, read_body,
    read_body_json, read_response, read_response_json, try_call_and_read_body_json,
    try_call_service, try_read_body, try_read_body_json,
};

#[cfg(test)]
pub(crate) use self::test_utils::try_init_service;

/// Reduces boilerplate code when testing expected response payloads.
///
/// Must be used inside an async test. Works for both `ServiceRequest` and `HttpRequest`.
Repository: actix/actix-web
File: awc/src/builder.rs (Rust)

Code before:
use std::{convert::TryFrom, fmt, net::IpAddr, rc::Rc, time::Duration};

use base64::{prelude::BASE64_STANDARD, Engine};

use actix_http::{
    error::HttpError,
    header::{self, HeaderMap, HeaderName, TryIntoHeaderPair},
    Uri,
};
use actix_rt::net::{ActixStream, TcpStream};
use actix_service::{boxed, Service};

use crate::{
    client::{
        ClientConfig, ConnectInfo, Connector, ConnectorService, TcpConnectError, TcpConnection,
    },
    connect::DefaultConnector,
    error::SendRequestError,
    middleware::{NestTransform, Redirect, Transform},
    Client, ConnectRequest, ConnectResponse,
};

/// An HTTP Client builder
///
/// This type can be used to construct an instance of `Client` through a
/// builder-like pattern.
pub struct ClientBuilder<S = (), M = ()> {
    max_http_version: Option<http::Version>,

Review comment (suggestion):
if prelude is being used, then lets just use it

```suggestion
use base64::prelude::*;
```

Code after:
use std::{convert::TryFrom, fmt, net::IpAddr, rc::Rc, time::Duration};

use base64::prelude::*;

use actix_http::{
    error::HttpError,
    header::{self, HeaderMap, HeaderName, TryIntoHeaderPair},
    Uri,
};
use actix_rt::net::{ActixStream, TcpStream};
use actix_service::{boxed, Service};

use crate::{
    client::{
        ClientConfig, ConnectInfo, Connector, ConnectorService, TcpConnectError, TcpConnection,
    },
    connect::DefaultConnector,
    error::SendRequestError,
    middleware::{NestTransform, Redirect, Transform},
    Client, ConnectRequest, ConnectResponse,
};

/// An HTTP Client builder
///
/// This type can be used to construct an instance of `Client` through a
/// builder-like pattern.
pub struct ClientBuilder<S = (), M = ()> {
    max_http_version: Option<http::Version>,
Repository: actix/actix-web
File: actix-http/src/body/utils.rs (Rust)

Code before:
use std::task::Poll;

use actix_rt::pin;
use actix_utils::future::poll_fn;
use bytes::{Bytes, BytesMut};
use futures_core::ready;

use super::{BodySize, MessageBody};

/// Collects all the bytes produced by `body`.
///
/// Any errors produced by the body stream are returned immediately.
///
/// Consider using [`to_bytes_limit`] instead to protect against memory exhaustion.
///
/// # Examples
///
/// ```
/// use actix_http::body::{self, to_bytes};
/// use bytes::Bytes;
///
/// # actix_rt::System::new().block_on(async {
/// let body = body::None::new();
/// let bytes = to_bytes(body).await.unwrap();
/// assert!(bytes.is_empty());
///
/// let body = Bytes::from_static(b"123");
/// let bytes = to_bytes(body).await.unwrap();
/// assert_eq!(bytes, "123");
/// # });
/// ```
pub async fn to_bytes<B: MessageBody>(body: B) -> Result<Bytes, B::Error> {
    to_bytes_limit(body, usize::MAX)
        .await
        .expect("body should never overflow usize::MAX")
}

/// Error type returned from [`to_bytes_limit`] when body produced exceeds limit.
#[derive(Debug)]

Review comment (suggestion):
I wonder if marking it as `deprecated` would be a stronger/better way to deliver the message, especially to those who are _today_ using `to_bytes` and exposed to memory exhaustion.

Code after:
use std::task::Poll;

use actix_rt::pin;
use actix_utils::future::poll_fn;
use bytes::{Bytes, BytesMut};
use futures_core::ready;

use super::{BodySize, MessageBody};

/// Collects all the bytes produced by `body`.
///
/// Any errors produced by the body stream are returned immediately.
///
/// Consider using [`to_bytes_limited`] instead to protect against memory exhaustion.
///
/// # Examples
///
/// ```
/// use actix_http::body::{self, to_bytes};
/// use bytes::Bytes;
///
/// # actix_rt::System::new().block_on(async {
/// let body = body::None::new();
/// let bytes = to_bytes(body).await.unwrap();
/// assert!(bytes.is_empty());
///
/// let body = Bytes::from_static(b"123");
/// let bytes = to_bytes(body).await.unwrap();
/// assert_eq!(bytes, "123");
/// # });
/// ```
pub async fn to_bytes<B: MessageBody>(body: B) -> Result<Bytes, B::Error> {
    to_bytes_limited(body, usize::MAX)
        .await
        .expect("body should never overflow usize::MAX")
}

/// Error type returned from [`to_bytes_limited`] when body produced exceeds limit.
#[derive(Debug)]
Repository: actix/actix-web
File: actix-files/src/files.rs (Rust)

Code before:
            read_mode_threshold: 0,
        }
    }

    /// Show files listing for directories.
    ///
    /// By default show files listing is disabled.
    ///
    /// When used with [`Files::index_file()`], files listing is shown as a fallback
    /// when the index file is not found.
    pub fn show_files_listing(mut self) -> Self {
        self.show_index = true;
        self
    }

    /// Redirects to a slash-ended path when browsing a directory.
    ///
    /// By default never redirect.
    pub fn redirect_to_slash_directory(mut self) -> Self {
        self.redirect_to_slash = true;
        self
    }

    /// Redirect with permanent redirect status code
    ///
    /// By default redirect with temporary redirect status code
    pub fn with_permanent_redirect(mut self) -> Self {
        self.with_permanent_redirect = true;
        self
    }

    /// Set custom directory renderer.
    pub fn files_listing_renderer<F>(mut self, f: F) -> Self
    where
        for<'r, 's> F:
            Fn(&'r Directory, &'s HttpRequest) -> Result<ServiceResponse, io::Error> + 'static,
    {
        self.renderer = Rc::new(f);
        self
    }

    /// Specifies MIME override callback.
    pub fn mime_override<F>(mut self, f: F) -> Self
    where
        F: Fn(&mime::Name<'_>) -> DispositionType + 'static,
    {
        self.mime_override = Some(Rc::new(f));
        self
    }

    /// Sets path filtering closure.

Review comment (suggestion):
```suggestion
    /// Redirect with permanent redirect status code (308).
    ///
    /// By default redirect with temporary redirect status code (307).
```

Code after:
            read_mode_threshold: 0,
        }
    }

    /// Show files listing for directories.
    ///
    /// By default show files listing is disabled.
    ///
    /// When used with [`Files::index_file()`], files listing is shown as a fallback
    /// when the index file is not found.
    pub fn show_files_listing(mut self) -> Self {
        self.show_index = true;
        self
    }

    /// Redirects to a slash-ended path when browsing a directory.
    ///
    /// By default never redirect.
    pub fn redirect_to_slash_directory(mut self) -> Self {
        self.redirect_to_slash = true;
        self
    }

    /// Redirect with permanent redirect status code (308).
    ///
    /// By default redirect with temporary redirect status code (307).
    pub fn with_permanent_redirect(mut self) -> Self {
        self.with_permanent_redirect = true;
        self
    }

    /// Set custom directory renderer.
    pub fn files_listing_renderer<F>(mut self, f: F) -> Self
    where
        for<'r, 's> F:
            Fn(&'r Directory, &'s HttpRequest) -> Result<ServiceResponse, io::Error> + 'static,
    {
        self.renderer = Rc::new(f);
        self
    }

    /// Specifies MIME override callback.
    pub fn mime_override<F>(mut self, f: F) -> Self
    where
        F: Fn(&mime::Name<'_>) -> DispositionType + 'static,
    {
        self.mime_override = Some(Rc::new(f));
        self
    }

    /// Sets path filtering closure.
Repository: adobe/react-spectrum
File: packages/@react-aria/utils/src/useObjectRef.ts (TypeScript)

Code before:

import {MutableRefObject, useMemo, useRef} from 'react';

/**
 * Offers an object ref for a given callback ref or an object ref. Especially
 * helfpul when passing forwarded refs (created using `React.forwardRef`) to
 * React Aria hooks.
 *
 * @param forwardedRef The original ref intended to be used.
 * @returns An object ref that updates the given ref.
 * @see https://reactjs.org/docs/forwarding-refs.html
 */
export function useObjectRef<T>(forwardedRef?: ((instance: T | null) => void) | MutableRefObject<T | null> | null): MutableRefObject<T | null> {
  const objRef: MutableRefObject<T | null> = useRef<T>(null);
  return useMemo(() => ({
    get current() {
      return objRef.current;
    },
    set current(value) {
      objRef.current = value;
      if (typeof forwardedRef === 'function') {
        forwardedRef(value);
      } else if (forwardedRef) {
        forwardedRef.current = value;
      }
    }
  }), [forwardedRef]);
}


Review comment (suggestion):
Probably not important, but there's a slight behavior change here:

```js
let ref = useObjectRef(value => {});
ref.current = {};
ref.current = undefined
```

Previously, the function would be called twice with {} and undefined. Now it will be called with {} and null.

Same below in the object ref case. You'd set `ref.current = undefined` but the underlying ref would be set to null.

Code after:

import {MutableRefObject, useCallback, useMemo, useRef} from 'react';

/**
 * Offers an object ref for a given callback ref or an object ref. Especially
 * helfpul when passing forwarded refs (created using `React.forwardRef`) to
 * React Aria hooks.
 *
 * @param ref The original ref intended to be used.
 * @returns An object ref that updates the given ref.
 * @see https://react.dev/reference/react/forwardRef
 */
export function useObjectRef<T>(ref?: ((instance: T | null) => (() => void) | void) | MutableRefObject<T | null> | null): MutableRefObject<T | null> {
  const objRef: MutableRefObject<T | null> = useRef<T>(null);
  const cleanupRef: MutableRefObject<(() => void) | void> = useRef(undefined);

  const refEffect = useCallback(
    (instance: T | null) => {
      if (typeof ref === 'function') {
        const refCallback = ref;
        const refCleanup = refCallback(instance);
        return () => {
          if (typeof refCleanup === 'function') {
            refCleanup();
          } else {
            refCallback(null);
          }
        };
      } else if (ref) {
        ref.current = instance;
        return () => {
          ref.current = null;
        };
      }
    },
    [ref]
  );

  return useMemo(
    () => ({
      get current() {
        return objRef.current;
      },
      set current(value) {
        objRef.current = value;
        if (cleanupRef.current) {
          cleanupRef.current();
          cleanupRef.current = undefined;
        }

        if (value != null) {
Repository: adobe/react-spectrum
File: packages/@react-aria/utils/src/useObjectRef.ts (TypeScript)

Code before:
/*
 * Copyright 2021 Adobe. All rights reserved.
 * This file is licensed to you under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License. You may obtain a copy
 * of the License at http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software distributed under
 * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
 * OF ANY KIND, either express or implied. See the License for the specific language
 * governing permissions and limitations under the License.
 */

import {MutableRefObject, useMemo, useRef} from 'react';

/**
 * Offers an object ref for a given callback ref or an object ref. Especially
 * helfpul when passing forwarded refs (created using `React.forwardRef`) to
 * React Aria hooks.
 *
 * @param forwardedRef The original ref intended to be used.
 * @returns An object ref that updates the given ref.
 * @see https://reactjs.org/docs/forwarding-refs.html
 */
export function useObjectRef<T>(forwardedRef?: ((instance: T | null) => void) | MutableRefObject<T | null> | null): MutableRefObject<T | null> {
  const objRef: MutableRefObject<T | null> = useRef<T>(null);
  return useMemo(() => ({
    get current() {
      return objRef.current;
    },
    set current(value) {
      objRef.current = value;
      if (typeof forwardedRef === 'function') {
        forwardedRef(value);
      } else if (forwardedRef) {
        forwardedRef.current = value;
      }
    }
  }), [forwardedRef]);
}


Review comment (suggestion):
Also this means there's no way to distinguish between `null` to trigger a cleanup vs `null` being a valid value of the ref. But not sure there's a way around that.

Code after:
            refCallback(null);
          }
        };
      } else if (ref) {
        ref.current = instance;
        return () => {
          ref.current = null;
        };
      }
    },
    [ref]
  );

  return useMemo(
    () => ({
      get current() {
        return objRef.current;
      },
      set current(value) {
        objRef.current = value;
        if (cleanupRef.current) {
          cleanupRef.current();
          cleanupRef.current = undefined;
        }

        if (value != null) {
          cleanupRef.current = refEffect(value);
        }
      }
    }),
    [refEffect]
  );
}
Repository: adobe/react-spectrum
File: packages/react-aria-components/stories/GridList.stories.tsx (TypeScript)

Code before:
      className={styles.menu}
      aria-label="Grid list with tag group"
      keyboardNavigationBehavior="tab"
      style={{
        width: 300,
        height: 300
      }}>
      <MyGridListItem textValue="Tags">
        1,1
        <TagGroup aria-label="Tag group" onRemove={action('onRemove')}>
          <TagList style={{display: 'flex', gap: 10}}>
            <Tag key="1">Tag 1<Button slot="remove">X</Button></Tag>
            <Tag key="2">Tag 2<Button slot="remove">X</Button></Tag>
            <Tag key="3">Tag 3<Button slot="remove">X</Button></Tag>
          </TagList>
        </TagGroup>
      </MyGridListItem>
      <MyGridListItem>
        1,2 <Button>Actions</Button>
      </MyGridListItem>
      <MyGridListItem>
        1,3
        <TagGroup aria-label="Tag group">
          <TagList style={{display: 'flex', gap: 10}}>
            <Tag key="1">Tag 1</Tag>
            <Tag key="2">Tag 2</Tag>
            <Tag key="3">Tag 3</Tag>
          </TagList>
        </TagGroup>
      </MyGridListItem>
    </GridList>
  );
}


Review comment (suggestion):
Is it weird that a user tabbing into the Gridlist's TagGroup needs to tab through each of the "X" buttons before being able to tab out of the GridList entirely? I think ideally hitting Tab when focused on the "X" button would move you out of the GridList then

Code after:
}

export function TagGroupInsideGridList() {
  return (
    <GridList
      className={styles.menu}
      aria-label="Grid list with tag group"
      keyboardNavigationBehavior="tab"
      style={{
        width: 300,
        height: 300
      }}>
      <MyGridListItem textValue="Tags">
        1,1
        <TagGroup aria-label="Tag group 1" onRemove={action('onRemove')}>
          <TagList style={{display: 'flex', gap: 10}}>
            <Tag key="1">Tag 1<Button slot="remove">X</Button></Tag>
            <Tag key="2">Tag 2<Button slot="remove">X</Button></Tag>
            <Tag key="3">Tag 3<Button slot="remove">X</Button></Tag>
          </TagList>
        </TagGroup>
        <TagGroup aria-label="Tag group 2" onRemove={action('onRemove')}>
          <TagList style={{display: 'flex', gap: 10}}>
            <Tag key="1">Tag 1<Button slot="remove">X</Button></Tag>
            <Tag key="2">Tag 2<Button slot="remove">X</Button></Tag>
            <Tag key="3">Tag 3<Button slot="remove">X</Button></Tag>
          </TagList>
        </TagGroup>
      </MyGridListItem>
      <MyGridListItem>
        1,2 <Button>Actions</Button>
      </MyGridListItem>
      <MyGridListItem>
        1,3
        <TagGroup aria-label="Tag group">
          <TagList style={{display: 'flex', gap: 10}}>
            <Tag key="1">Tag 1</Tag>
            <Tag key="2">Tag 2</Tag>
            <Tag key="3">Tag 3</Tag>
          </TagList>
        </TagGroup>
      </MyGridListItem>
    </GridList>
  );
}
Repository: adobe/react-spectrum
File: packages/@react-aria/utils/src/runAfterTransition.ts (TypeScript)

Code before:
      }

      transitionCallbacks.clear();
    }
  };

  document.body.addEventListener('transitionrun', onTransitionStart);
  document.body.addEventListener('transitionend', onTransitionEnd);
}

if (typeof document !== 'undefined') {
  if (document.readyState !== 'loading') {
    setupGlobalEvents();
  } else {
    document.addEventListener('DOMContentLoaded', setupGlobalEvents);
  }
}

/**
 * Cleans up any elements that are no longer in the document.
 * This is necessary because we can't rely on transitionend events to fire
 * for elements that are removed from the document while transitioning.
 */
function cleanupDetachedElements() {
  for (const [element] of transitionsByElement) {
    if (element instanceof HTMLElement && !document.contains(element)) {
      transitionsByElement.delete(element);
    }
  }
}

export function runAfterTransition(fn: () => void): void {
  // Wait one frame to see if an animation starts, e.g. a transition on mount.
  requestAnimationFrame(() => {
    cleanupDetachedElements();
    // If no transitions are running, call the function immediately.
    // Otherwise, add it to a list of callbacks to run at the end of the animation.
    if (transitionsByElement.size === 0) {
      fn();
    } else {
      transitionCallbacks.add(fn);
    }
  });
}


Review comment (suggestion):
using isConnected can help us avoid some odd edgecases and should theoretically be faster as well https://developer.mozilla.org/en-US/docs/Web/API/Node/isConnected
```suggestion
    if (element instanceof HTMLElement && !element.isConnected) {
```

Code after:
      }

      transitionCallbacks.clear();
    }
  };

  document.body.addEventListener('transitionrun', onTransitionStart);
  document.body.addEventListener('transitionend', onTransitionEnd);
}

if (typeof document !== 'undefined') {
  if (document.readyState !== 'loading') {
    setupGlobalEvents();
  } else {
    document.addEventListener('DOMContentLoaded', setupGlobalEvents);
  }
}

/**
 * Cleans up any elements that are no longer in the document.
 * This is necessary because we can't rely on transitionend events to fire
 * for elements that are removed from the document while transitioning.
 */
function cleanupDetachedElements() {
  for (const [eventTarget] of transitionsByElement) {
    // Similar to `eventTarget instanceof Element && !eventTarget.isConnected`, but avoids
    // the explicit instanceof check, since it may be different in different contexts.
    if ('isConnected' in eventTarget && !eventTarget.isConnected) {
      transitionsByElement.delete(eventTarget);
    }
  }
}

export function runAfterTransition(fn: () => void): void {
  // Wait one frame to see if an animation starts, e.g. a transition on mount.
  requestAnimationFrame(() => {
    cleanupDetachedElements();
    // If no transitions are running, call the function immediately.
    // Otherwise, add it to a list of callbacks to run at the end of the animation.
    if (transitionsByElement.size === 0) {
      fn();
    } else {
      transitionCallbacks.add(fn);
    }
  });
}