ronantakizawa/codereview-bench huggingface dataset:
Repository: paritytech/substrate
File: primitives/api/proc-macro/src/impl_runtime_apis.rs (Rust)

Code before:
			.segments
			.last()
			.ok_or_else(|| Error::new(impl_trait_path.span(), "Empty trait path not possible!"))?
			.ident;

		for item in &impl_.items {
			if let ImplItem::Fn(method) = item {
				let impl_call = generate_impl_call(
					&method.sig,
					&impl_.self_ty,
					input,
					&impl_trait,
					&trait_api_ver,
				)?;
				let mut attrs = filter_cfg_attrs(&impl_.attrs);

				// If the method has got a `#[cfg(feature = X)]` attribute match it against the impl
				// block `cfg_attr` add a feature flag for this impl call. Note that the feature
				// guard is added to `Vec<Attribute>` which is returned as a result and not to the
				// method itself.
				if let Some((feature_name, _)) = &trait_api_ver.feature_gated {
					if method_feature_flag_matches_trait_feature_gate(&method.attrs, &feature_name)?
					{
						add_feature_guard(&mut attrs, &feature_name, true)
					}
				}

				impl_calls.push((
					impl_trait_ident.clone(),
					method.sig.ident.clone(),
					impl_call,
					attrs,
				));
			}
		}
	}

	Ok(impl_calls)
}

/// Generate the dispatch function that is used in native to call into the runtime.
fn generate_dispatch_function(impls: &[ItemImpl]) -> Result<TokenStream> {
	let data = Ident::new("_sp_api_input_data_", Span::call_site());
	let c = generate_crate_access();
	let impl_calls =
		generate_impl_calls(impls, &data)?
			.into_iter()
			.map(|(trait_, fn_name, impl_, attrs)| {
				let name = prefix_function_with_trait(&trait_, &fn_name);
				quote!(
					#( #attrs )*

Review comment (suggestion):
We could probably just do:
```
attrs.extend(filter_cfg_attrs(&method.attrs));
```

This is much easier :P 

Code after:
			.segments
			.last()
			.ok_or_else(|| Error::new(impl_trait_path.span(), "Empty trait path not possible!"))?
			.ident;

		for item in &impl_.items {
			if let ImplItem::Fn(method) = item {
				let impl_call = generate_impl_call(
					&method.sig,
					&impl_.self_ty,
					input,
					&impl_trait,
					&trait_api_ver,
				)?;
				let mut attrs = filter_cfg_attrs(&impl_.attrs);

				// Add any `#[cfg(feature = X)]` attributes of the method to result
				attrs.extend(filter_cfg_attrs(&method.attrs));

				impl_calls.push((
					impl_trait_ident.clone(),
					method.sig.ident.clone(),
					impl_call,
					attrs,
				));
			}
		}
	}

	Ok(impl_calls)
}

/// Generate the dispatch function that is used in native to call into the runtime.
fn generate_dispatch_function(impls: &[ItemImpl]) -> Result<TokenStream> {
	let data = Ident::new("_sp_api_input_data_", Span::call_site());
	let c = generate_crate_access();
	let impl_calls =
		generate_impl_calls(impls, &data)?
			.into_iter()
			.map(|(trait_, fn_name, impl_, attrs)| {
				let name = prefix_function_with_trait(&trait_, &fn_name);
				quote!(
					#( #attrs )*
					#name => Some(#c::Encode::encode(&{ #impl_ })),
				)
			});

	Ok(quote!(
		#c::std_enabled! {
			pub fn dispatch(method: &str, mut #data: &[u8]) -> Option<Vec<u8>> {
				match method {
Repository: paritytech/substrate
File: frame/scheduler/src/tests.rs (Rust)

Code before:

use super::*;
use crate::mock::{
	logger, new_test_ext, root, run_to_block, LoggerCall, RuntimeCall, Scheduler, Test, *,
};
use frame_support::{
	assert_err, assert_noop, assert_ok,
	traits::{Contains, GetStorageVersion, OnInitialize, QueryPreimage, StorePreimage},
	Hashable,
};
use sp_runtime::traits::Hash;
use substrate_test_utils::assert_eq_uvec;

#[test]
#[docify::export]
fn basic_scheduling_works() {
	new_test_ext().execute_with(|| {
		// Call to schedule
		let call =
			RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_parts(10, 0) });

		// BaseCallFilter should be implemented to accept `Logger::log` runtime call which is
		// implemented for `BaseFilter` in the mock runtime
		assert!(!<Test as frame_system::Config>::BaseCallFilter::contains(&call));

		// Schedule call to be executed at the the 4th block
		assert_ok!(Scheduler::do_schedule(
			DispatchTime::At(4),
			None,
			127,
			root(),
			Preimage::bound(call).unwrap()
		));

		// `log` runtime call should not have executed yet
		run_to_block(3);
		assert!(logger::log().is_empty());

		run_to_block(4);
		// `log` runtime call should have executed at block 4
		assert_eq!(logger::log(), vec![(root(), 42u32)]);

		run_to_block(100);
		assert_eq!(logger::log(), vec![(root(), 42u32)]);
	});
}

#[test]
#[docify::export]
fn scheduling_with_preimages_works() {
	new_test_ext().execute_with(|| {

Review comment (suggestion):
```suggestion
		// Schedule call to be executed at the 4th block
```

Code after:

use super::*;
use crate::mock::{
	logger, new_test_ext, root, run_to_block, LoggerCall, RuntimeCall, Scheduler, Test, *,
};
use frame_support::{
	assert_err, assert_noop, assert_ok,
	traits::{Contains, GetStorageVersion, OnInitialize, QueryPreimage, StorePreimage},
	Hashable,
};
use sp_runtime::traits::Hash;
use substrate_test_utils::assert_eq_uvec;

#[test]
#[docify::export]
fn basic_scheduling_works() {
	new_test_ext().execute_with(|| {
		// Call to schedule
		let call =
			RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_parts(10, 0) });

		// BaseCallFilter should be implemented to accept `Logger::log` runtime call which is
		// implemented for `BaseFilter` in the mock runtime
		assert!(!<Test as frame_system::Config>::BaseCallFilter::contains(&call));

		// Schedule call to be executed at the 4th block
		assert_ok!(Scheduler::do_schedule(
			DispatchTime::At(4),
			None,
			127,
			root(),
			Preimage::bound(call).unwrap()
		));

		// `log` runtime call should not have executed yet
		run_to_block(3);
		assert!(logger::log().is_empty());

		run_to_block(4);
		// `log` runtime call should have executed at block 4
		assert_eq!(logger::log(), vec![(root(), 42u32)]);

		run_to_block(100);
		assert_eq!(logger::log(), vec![(root(), 42u32)]);
	});
}

#[test]
#[docify::export]
fn scheduling_with_preimages_works() {
	new_test_ext().execute_with(|| {
Repository: paritytech/substrate
File: client/consensus/beefy/src/lib.rs (Rust)

Code before:
	error!(target: LOG_TARGET, "{}", err_msg);
	Err(ClientError::Backend(err_msg))
}

fn expect_validator_set<B, BE, R>(
	runtime: &R,
	backend: &BE,
	at_header: &B::Header,
) -> ClientResult<ValidatorSet<AuthorityId>>
where
	B: Block,
	BE: Backend<B>,
	R: ProvideRuntimeApi<B>,
	R::Api: BeefyApi<B, AuthorityId>,
{
	runtime
		.runtime_api()
		.validator_set(at_header.hash())
		.ok()
		.flatten()
		.or_else(|| {
			// if state unavailable, fallback to walking up the chain looking for the header
			// Digest emitted when validator set active 'at_header' was enacted.
			let blockchain = backend.blockchain();
			let mut header = at_header.clone();
			loop {
				match worker::find_authorities_change::<B>(&header) {
					Some(active) => return Some(active),
					// Move up the chain.
					None => header = blockchain.expect_header(*header.parent_hash()).ok()?,
				}
			}
		})
		.ok_or_else(|| ClientError::Backend("Could not find initial validator set".into()))
}


Review comment (security):
As a defensive manner I'd pass the BEEFY genesis block number to this function so that we can early-exit if we never find the authorities (otherwise we'll loop until block 0). This should never happen but might as well be safe.

Code after:
	error!(target: LOG_TARGET, "{}", err_msg);
	Err(ClientError::Backend(err_msg))
}

fn expect_validator_set<B, BE, R>(
	runtime: &R,
	backend: &BE,
	at_header: &B::Header,
	beefy_genesis: NumberFor<B>,
) -> ClientResult<ValidatorSet<AuthorityId>>
where
	B: Block,
	BE: Backend<B>,
	R: ProvideRuntimeApi<B>,
	R::Api: BeefyApi<B, AuthorityId>,
{
	runtime
		.runtime_api()
		.validator_set(at_header.hash())
		.ok()
		.flatten()
		.or_else(|| {
			// if state unavailable, fallback to walking up the chain looking for the header
			// Digest emitted when validator set active 'at_header' was enacted.
			let blockchain = backend.blockchain();
			let mut header = at_header.clone();
			while *header.number() >= beefy_genesis {
				match worker::find_authorities_change::<B>(&header) {
					Some(active) => return Some(active),
					// Move up the chain.
					None => header = blockchain.expect_header(*header.parent_hash()).ok()?,
				}
			}
			None
		})
		.ok_or_else(|| ClientError::Backend("Could not find initial validator set".into()))
}
Repository: paritytech/substrate
File: frame/contracts/src/storage/meter.rs (Rust)

Code before:
		self.total_deposit = self
			.total_deposit
			.saturating_add(&absorbed.total_deposit)
			.saturating_add(&own_deposit);
		self.charges.extend_from_slice(&absorbed.charges);
		if !own_deposit.is_zero() {
			self.charges.push(Charge {
				contract: contract.clone(),
				amount: own_deposit,
				status: absorbed.is_terminated(),
			});
		}
	}

	/// The amount of balance that is still available from the original `limit`.
	fn available(&self) -> BalanceOf<T> {
		self.total_deposit.available(&self.limit)
	}

	/// True if the contract is alive.
	fn is_alive(&self) -> bool {
		matches!(self.own_contribution, Contribution::Alive(_))
	}

	/// True if the contract is terminated.
	fn is_terminated(&self) -> ContractStatus<T> {
		match &self.own_contribution {
			Contribution::Terminated { deposit: _, beneficiary } =>
				ContractStatus::Terminated { beneficiary: beneficiary.clone() },
			_ => ContractStatus::Alive,
		}
	}
}

/// Functions that only apply to the root state.
impl<T, E> RawMeter<T, E, Root>
where
	T: Config,
	E: Ext<T>,
{
	/// Create new storage meter for the specified `origin` and `limit`.
	///
	/// This tries to [`Ext::check_limit`] on `origin` and fails if this is not possible.
	pub fn new(
		origin: &Origin<T>,
		limit: Option<BalanceOf<T>>,
		min_leftover: BalanceOf<T>,
	) -> Result<Self, DispatchError> {
		// Check the limit only if the origin is not root.
		return match origin {
			Origin::Root => Ok(Self {

Review comment (suggestion):
```suggestion
	fn contract_status(&self) -> ContractStatus<T> {
```

Code after:
		self.total_deposit = self
			.total_deposit
			.saturating_add(&absorbed.total_deposit)
			.saturating_add(&own_deposit);
		self.charges.extend_from_slice(&absorbed.charges);
		if !own_deposit.is_zero() {
			self.charges.push(Charge {
				contract: contract.clone(),
				amount: own_deposit,
				state: absorbed.contract_state(),
			});
		}
	}

	/// The amount of balance that is still available from the original `limit`.
	fn available(&self) -> BalanceOf<T> {
		self.total_deposit.available(&self.limit)
	}

	/// Returns the state of the currently executed contract.
	fn contract_state(&self) -> ContractState<T> {
		match &self.own_contribution {
			Contribution::Terminated { deposit: _, beneficiary } =>
				ContractState::Terminated { beneficiary: beneficiary.clone() },
			_ => ContractState::Alive,
		}
	}
}

/// Functions that only apply to the root state.
impl<T, E> RawMeter<T, E, Root>
where
	T: Config,
	E: Ext<T>,
{
	/// Create new storage meter for the specified `origin` and `limit`.
	///
	/// This tries to [`Ext::check_limit`] on `origin` and fails if this is not possible.
	pub fn new(
		origin: &Origin<T>,
		limit: Option<BalanceOf<T>>,
		min_leftover: BalanceOf<T>,
	) -> Result<Self, DispatchError> {
		// Check the limit only if the origin is not root.
		return match origin {
			Origin::Root => Ok(Self {
				limit: limit.unwrap_or(T::DefaultDepositLimit::get()),
				..Default::default()
			}),
			Origin::Signed(o) => {
				let limit = E::check_limit(o, limit, min_leftover)?;
Repository: paritytech/substrate
File: frame/contracts/src/debug.rs (Rust)

Code before:
pub use crate::exec::ExportedFunction;
use crate::{CodeHash, Config, LOG_TARGET};
use pallet_contracts_primitives::ExecReturnValue;

/// Umbrella trait for all interfaces that serves for debugging.
pub trait Debugging<T: Config>: Tracing<T> {}

impl<T: Config, V> Debugging<T> for V where V: Tracing<T> {}

/// Defines methods to capture contract calls, enabling external observers to
/// measure, trace, and react to contract interactions.
pub trait Tracing<T: Config> {
	type CallSpan: CallSpan;

	/// Creates a new call span to encompass the upcoming contract execution.
	///
	/// This method should be invoked just before the execution of a contract and
	/// marks the beginning of a traceable span of execution.
	///
	/// # Arguments
	///
	/// * `code_hash` - The code hash of the contract being called.
	/// * `entry_point` - Describes whether the call is the constructor or a regular call.
	/// * `input_data` - The raw input data of the call.
	fn new_call_span(
		code_hash: &CodeHash<T>,
		entry_point: ExportedFunction,
		input_data: &[u8],
	) -> Self::CallSpan;
}


Review comment (bug):
Debug would be nicer but a bit annoying as it clash with `std:fmt:Debug`, `ConractDebug` would be another good name, any pref?

Code after:
pub use crate::exec::ExportedFunction;
use crate::{CodeHash, Config, LOG_TARGET};
use pallet_contracts_primitives::ExecReturnValue;

/// Umbrella trait for all interfaces that serves for debugging.
pub trait Debugger<T: Config>: Tracing<T> {}

impl<T: Config, V> Debugger<T> for V where V: Tracing<T> {}

/// Defines methods to capture contract calls, enabling external observers to
/// measure, trace, and react to contract interactions.
pub trait Tracing<T: Config> {
	/// The type of [`CallSpan`] that is created by this trait.
	type CallSpan: CallSpan;

	/// Creates a new call span to encompass the upcoming contract execution.
	///
	/// This method should be invoked just before the execution of a contract and
	/// marks the beginning of a traceable span of execution.
	///
	/// # Arguments
	///
	/// * `code_hash` - The code hash of the contract being called.
	/// * `entry_point` - Describes whether the call is the constructor or a regular call.
	/// * `input_data` - The raw input data of the call.
	fn new_call_span(
		code_hash: &CodeHash<T>,
		entry_point: ExportedFunction,
		input_data: &[u8],
	) -> Self::CallSpan;
}
Repository: payloadcms/payload
File: packages/ui/src/utilities/stringifyTruncated.ts (TypeScript)

Code before:
/**
 * Safely stringify a value and truncate it to a maximum length.
 *
 * Converts any value to a JSON string representation and truncates the resulting
 * string if it exceeds the maximum length. If truncation occurs, an ellipsis (…)
 * is appended to indicate incomplete output.
 *
 * @param value - The value to stringify (can be any type including objects, arrays, primitives)
 * @param maxLength - The maximum character length of the output string
 * @returns A JSON string representation, truncated with "…" if it exceeds maxLength
 */
export function stringifyTruncated(value: unknown, maxLength: number): string {
  const stringifiedJSON = JSON.stringify(value)
  return stringifiedJSON.length > maxLength
    ? `${stringifiedJSON.substring(0, maxLength)}\u2026`
    : stringifiedJSON
}


Review comment (bug):
Because we're hardcoding the ellipse into the truncated value here, it's possible for the rendered output to display double ellipse (`......`) if CSS also sets `text-overflow: ellipsis`. I believe this is the current bahavior on mobile.

Code after:
/**
 * Safely stringify a value and truncate it to a maximum length.
 *
 * Converts any value to a JSON string representation and truncates the resulting
 * string if it exceeds the maximum length. If truncation occurs, an ellipsis (…)
 * is appended to indicate incomplete output.
 *
 * @param value - The value to stringify (can be any type including objects, arrays, primitives)
 * @param maxLength - The maximum character length of the output string
 * @returns A JSON string representation, truncated with "…" if it exceeds maxLength
 */
export function stringifyTruncated(value: unknown, maxLength: number): string {
  const stringifiedJSON = JSON.stringify(value)
  const totalChars = stringifiedJSON.length

  // Only truncate if the string is significantly longer (>1.5x the max length)
  if (totalChars / maxLength > 1.5) {
    return `${stringifiedJSON.substring(0, maxLength)}\u2026`
  }

  return stringifiedJSON
}
Repository: payloadcms/payload
File: test/fields/collections/Array/e2e.spec.ts (TypeScript)

Code before:
        '#field-items__0__subArray > div.array-field__draggable-rows > div',
      )
      const subArrayContainer2 = field.locator(
        '#field-items__1__subArray > div.array-field__draggable-rows > div',
      )
      await expect(subArrayContainer).toHaveCount(2)
      await expect(subArrayContainer2).toHaveCount(1)

      await copyPasteField({
        page,
        fieldName: 'items',
        rowIndex: 1,
      })

      await copyPasteField({
        page,
        fieldName: 'items',
        rowIndex: 0,
        action: 'paste',
      })

      await expect(subArrayContainer).toHaveCount(1)
      await expect(subArrayContainer2).toHaveCount(1)
    })
  })
  test('should return empty array for empty array fields with getDataByPath', async () => {
    await page.goto(url.create)

    // Wait for the test component to render
    await page.waitForSelector('[data-testid="getDataByPath-test"]')

    // Check that getDataByPath returned an empty array, not 0
    await expect(page.getByTestId('empty-array-result')).toHaveText('ARRAY')
    await expect(page.getByTestId('empty-array-length')).toHaveText('0')
  })
})


Review comment (suggestion):
```suggestion
  test('should return empty array from getDataByPath for array fields without rows', async () => {
```

Code after:
        '#field-items__0__subArray > div.array-field__draggable-rows > div',
      )
      const subArrayContainer2 = field.locator(
        '#field-items__1__subArray > div.array-field__draggable-rows > div',
      )
      await expect(subArrayContainer).toHaveCount(2)
      await expect(subArrayContainer2).toHaveCount(1)

      await copyPasteField({
        page,
        fieldName: 'items',
        rowIndex: 1,
      })

      await copyPasteField({
        page,
        fieldName: 'items',
        rowIndex: 0,
        action: 'paste',
      })

      await expect(subArrayContainer).toHaveCount(1)
      await expect(subArrayContainer2).toHaveCount(1)
    })
  })
  test('should return empty array from getDataByPath for array fields without rows', async () => {
    await page.goto(url.create)

    // Wait for the test component to render
    await page.waitForSelector('#getDataByPath-test')

    // Check that getDataByPath returned an empty array, not 0
    await expect(page.locator('#empty-array-result')).toHaveText('ARRAY')
    await expect(page.locator('#empty-array-length')).toHaveText('0')
  })
})
Repository: payloadcms/payload
File: packages/payload/src/versions/buildCollectionFields.ts (TypeScript)

Code before:
        }),
      })

      if (config.localization.enableStatusLocalization) {
        const localeStatusFields: Field[] = config.localization.locales.map((locale) => {
          const code = typeof locale === 'string' ? locale : locale.code

          return {
            name: code,
            type: 'select',
            options: [
              { label: 'Draft', value: 'draft' },
              { label: 'Published', value: 'published' },
            ],
          }
        })

        fields.push({
          name: 'localeStatus',
          type: 'group',
          admin: {
            disableBulkEdit: true,
            disabled: true,
          },
          fields: localeStatusFields,
          index: true,
          ...(flatten && {
            flattenedFields: localeStatusFields as FlattenedField[],
          })!,
        })
      }
    }

    fields.push({
      name: 'latest',
      type: 'checkbox',
      admin: {
        disabled: true,
      },
      index: true,
    })

    if (collection?.versions?.drafts?.autosave) {
      fields.push({
        name: 'autosave',
        type: 'checkbox',
        index: true,
      })
    }
  }


Review comment (suggestion):
indexing on the group itself won't do much. We likely want the subfields to be `index: true` instead.

Code after:
        }),
      })

      if (config.experimental?.localizeStatus) {
        const localeStatusFields = buildLocaleStatusField(config)

        fields.push({
          name: 'localeStatus',
          type: 'group',
          admin: {
            disableBulkEdit: true,
            disabled: true,
          },
          fields: localeStatusFields,
          ...(flatten && {
            flattenedFields: localeStatusFields as FlattenedField[],
          })!,
        })
      }
    }

    fields.push({
      name: 'latest',
      type: 'checkbox',
      admin: {
        disabled: true,
      },
      index: true,
    })

    if (collection?.versions?.drafts?.autosave) {
      fields.push({
        name: 'autosave',
        type: 'checkbox',
        index: true,
      })
    }
  }

  return fields as true extends T ? FlattenedField[] : Field[]
}
Repository: payloadcms/payload
File: packages/payload/src/utilities/sanitizeFallbackLocale.ts (TypeScript)

Code before:
          : undefined

      if (localeSpecificFallback) {
        fallbackLocale = localeSpecificFallback
      } else {
        // Use defaultLocale as fallback otherwise
        if (localization && 'fallback' in localization && localization.fallback) {
          fallbackLocale = localization.defaultLocale
        }
      }
    }
  } else {
    fallbackLocale = null
  }

  if (
    typeof fallbackLocale === 'string' &&
    fallbackLocale.startsWith('[') &&
    fallbackLocale.endsWith(']')
  ) {
    fallbackLocale = fallbackLocale
      .slice(1, -1)
      .split(',')
      .map((l) => l.trim())
  }

  return fallbackLocale as null | string
}


Review comment (suggestion):
This feels out of place. Seems like any logic like this should be done outside of this function and the fallbackLocale should then be passed in. Not sure about the string manipulation here. Feels like we should be able to get the fallbackLocale off of the parsed params and pass it in.

Code after:
import type { SanitizedLocalizationConfig } from '../config/types.js'
import type { TypedLocale } from '../index.js'

interface Args {
  fallbackLocale: false | TypedLocale | TypedLocale[]
  locale: string
  localization: SanitizedLocalizationConfig
}

/**
 * Sanitizes fallbackLocale based on a provided fallbackLocale, locale and localization config
 *
 * Handles the following scenarios:
 * - determines if a fallback locale should be used
 * - determines if a locale specific fallback should be used in place of the default locale
 * - sets the fallbackLocale to 'null' if no fallback locale should be used
 */
export const sanitizeFallbackLocale = ({
  fallbackLocale,
  locale,
  localization,
}: Args): null | string => {
  let hasFallbackLocale = false

  if (fallbackLocale === undefined || fallbackLocale === null) {
    hasFallbackLocale = Boolean(localization && localization.fallback)
  }

  if (
    fallbackLocale &&
    (Array.isArray(fallbackLocale) || !['false', 'none', 'null'].includes(fallbackLocale))
  ) {
    hasFallbackLocale = true
  }

  if (hasFallbackLocale) {
    if (!fallbackLocale) {
      // Check for locale specific fallback
      const localeSpecificFallback =
        localization && localization?.locales?.length
          ? localization.locales.find((localeConfig) => localeConfig.code === locale)
              ?.fallbackLocale
          : undefined

      if (localeSpecificFallback) {
        fallbackLocale = localeSpecificFallback
      } else {
        // Use defaultLocale as fallback otherwise
        if (localization && 'fallback' in localization && localization.fallback) {
          fallbackLocale = localization.defaultLocale
        }
      }
    }
  } else {
    fallbackLocale = null
  }

  return fallbackLocale as null | string
}