ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/aspnetcore
File: src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTestUtil.cs (C#)

Code before:
            // Browser session is no longer valid (e.g., browser was closed)
            // Session storage is automatically cleared when browser closes, so cleanup is already done
            // This is expected in some tests, so we silently return
            return;
        }
        finally
        {
            _isSuppressed = false;
        }
    }

    private static void NavigateToOrigin<TServerFixture>(ServerTestBase<TServerFixture> fixture)
        where TServerFixture : ServerFixture
    {
        // Navigate to the test origin to ensure the browser is on the correct state to access sessionStorage
        fixture.Navigate($"{fixture.ServerPathBase}/");
        fixture.Browser.Exists(By.Id("session-storage-marker"));
    }

    public static long GetScrollY(this IWebDriver browser)
        => Convert.ToInt64(((IJavaScriptExecutor)browser).ExecuteScript("return window.scrollY"), CultureInfo.CurrentCulture);

    public static long SetScrollY(this IWebDriver browser, long value)
        => Convert.ToInt64(((IJavaScriptExecutor)browser).ExecuteScript($"window.scrollTo(0, {value})"), CultureInfo.CurrentCulture);

    public static bool IsElementStale(IWebElement element)
    {
        try
        {
            _ = element.Enabled;
            return false;
        }
        catch (StaleElementReferenceException)
        {
            return true;
        }
    }
}


Review comment (bug):
@copilot, can you remove IsElementStale method from [EnhancedNavigationTestUtil.cs‎](https://github.com/dotnet/aspnetcore/pull/64542/files#diff-8bcfad07a145fddd0a36595790550fab49385dbd6cfa4a6240654cfb10ca5810) and change `CanPerformNavigateToFromInteractiveEventHandler` test to use the new staleness check: https://github.com/dotnet/aspnetcore/blob/27fe84d1b17e218c66b53351799687aec7a7598d/src/Components/test/E2ETest/Infrastructure/WebDriverExtensions/WebDriverExtensions.cs#L122

Code after:
            // Browser session is no longer valid (e.g., browser was closed)
            // Session storage is automatically cleared when browser closes, so cleanup is already done
            // This is expected in some tests, so we silently return
            return;
        }
        finally
        {
            _isSuppressed = false;
        }
    }

    private static void NavigateToOrigin<TServerFixture>(ServerTestBase<TServerFixture> fixture)
        where TServerFixture : ServerFixture
    {
        // Navigate to the test origin to ensure the browser is on the correct state to access sessionStorage
        fixture.Navigate($"{fixture.ServerPathBase}/");
        fixture.Browser.Exists(By.Id("session-storage-marker"));
    }

    public static long GetScrollY(this IWebDriver browser)
        => Convert.ToInt64(((IJavaScriptExecutor)browser).ExecuteScript("return window.scrollY"), CultureInfo.CurrentCulture);

    public static long SetScrollY(this IWebDriver browser, long value)
        => Convert.ToInt64(((IJavaScriptExecutor)browser).ExecuteScript($"window.scrollTo(0, {value})"), CultureInfo.CurrentCulture);
}
Repository: dotnet/aspnetcore
File: src/Components/Endpoints/test/Builder/ResourceCollectionUrlEndpointTest.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Components.Endpoints;

public class ResourceCollectionUrlEndpointTest
{
    [Fact]
    public void ComputeFingerprintSuffix_IncludesIntegrityForNonFingerprintedAssets()
    {
        // Arrange - Create a collection with non-fingerprinted assets that have integrity hashes
        var resources = new List<ResourceAsset>
        {
            // Non-fingerprinted asset with integrity (simulates WasmFingerprintAssets=false scenario)
            new ResourceAsset("/_framework/MyApp.dll", new[]
            {
                new ResourceAssetProperty("integrity", "sha256-ABC123")
            }),
            new ResourceAsset("/_framework/System.dll", new[]
            {
                new ResourceAssetProperty("integrity", "sha256-XYZ789")
            })
        };
        var collection = new ResourceAssetCollection(resources);

        // Act
        var fingerprint1 = ResourceCollectionUrlEndpoint.ComputeFingerprintSuffix(collection);

        // Arrange - Change the integrity of one asset (simulates content change between builds)
        resources = new List<ResourceAsset>
        {
            new ResourceAsset("/_framework/MyApp.dll", new[]
            {
                new ResourceAssetProperty("integrity", "sha256-CHANGED")

Review comment (suggestion):
Look at the microsoftdocs and use modern C# (collection initializers, target type, etc.) where possible.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Components.Endpoints;

public class ResourceCollectionUrlEndpointTest
{
    [Fact]
    public void ComputeFingerprintSuffix_IncludesIntegrityForNonFingerprintedAssets()
    {
        // Arrange - Create a collection with non-fingerprinted assets that have integrity hashes
        ResourceAsset[] resources =
        [
            // Non-fingerprinted asset with integrity (simulates WasmFingerprintAssets=false scenario)
            new("/_framework/MyApp.dll",
            [
                new("integrity", "sha256-ABC123")
            ]),
            new("/_framework/System.dll",
            [
                new("integrity", "sha256-XYZ789")
            ])
        ];
        var collection = new ResourceAssetCollection(resources);

        // Act
        var fingerprint1 = ResourceCollectionUrlEndpoint.ComputeFingerprintSuffix(collection);

        // Arrange - Change the integrity of one asset (simulates content change between builds)
        resources =
        [
            new("/_framework/MyApp.dll",
            [
                new("integrity", "sha256-CHANGED")
Repository: dotnet/aspnetcore
File: src/Components/Web/test/Forms/FieldIdGeneratorTest.cs (C#)

Code before:

        Assert.Equal("123Name", result);
    }

    [Fact]
    public void SanitizeHtmlId_ReplacesPeriodAtStart()
    {
        var input = ".Property";

        var result = FieldIdGenerator.SanitizeHtmlId(input);

        Assert.Equal("_Property", result);
    }

    [Fact]
    public void SanitizeHtmlId_AllowsSpecialCharacters()
    {
        var input = "my-field_name:value";

        var result = FieldIdGenerator.SanitizeHtmlId(input);

        Assert.Equal("my-field_name:value", result);
    }

    [Fact]
    public void SanitizeHtmlId_ReplacesWhitespaceWithUnderscores()
    {
        var input = "Field Name";

        var result = FieldIdGenerator.SanitizeHtmlId(input);

        Assert.Equal("Field_Name", result);
    }

    [Fact]
    public void SanitizeHtmlId_ReplacesVariousWhitespaceCharacters()
    {
        var input = "Field\tName\nWith\rVariousWhitespace";

        var result = FieldIdGenerator.SanitizeHtmlId(input);

        Assert.Equal("Field_Name_With_VariousWhitespace", result);
    }
}


Review comment (security):
`SanitizeHtmlId_ReplacesWhitespaceWithUnderscores` and `SanitizeHtmlId_ReplacesVariousWhitespaceCharacters` could be one test.
`SanitizeHtmlId_ReplacesPeriodAtStart` and `SanitizeHtmlId_AllowsStartingWithDigit`, `SanitizeHtmlId_AllowsSpecialCharacters` are redundant, we check similar things in `SanitizeHtmlId_ProducesValidId`, remove it.
Remove inline data with "[InlineData("Model.Property", "Model_Property")]", we have multiple dots in the next one, it's enough to test it.
Remove inline data: "[InlineData("Items[0]", "Items[0]")]", "[InlineData("Items[0].Name", "Items[0]_Name")]", "[InlineData("123Name", "123Name")]", "[InlineData(":Property", ":Property")]" they duplicate the other it test data.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

namespace Microsoft.AspNetCore.Components.Forms;

public class FieldIdGeneratorTest
{
    [Theory]
    [InlineData(null, "")]
    [InlineData("", "")]
    [InlineData("Name", "Name")]
    [InlineData("name", "name")]
    [InlineData("Model.Address.Street", "Model_Address_Street")]
    [InlineData("Model.Items[0].Name", "Model_Items[0]_Name")]
    [InlineData("Field\tName\nWith\rVariousWhitespace", "Field_Name_With_VariousWhitespace")]
    [InlineData("Field\u00A0Name", "Field_Name")] // Non-breaking space
    public void SanitizeHtmlId_ProducesValidId(string? input, string expected)
    {
        var result = FieldIdGenerator.SanitizeHtmlId(input);

        Assert.Equal(expected, result);
    }
}
Repository: dotnet/aspnetcore
File: src/Middleware/ResponseCompression/src/ZstandardCompressionProviderOptions.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.IO.Compression;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.ResponseCompression;

/// <summary>
/// Options for the <see cref="ZstandardCompressionProvider"/>
/// </summary>
public class ZstandardCompressionProviderOptions : IOptions<ZstandardCompressionProviderOptions>
{
    private static readonly ZstandardCompressionOptions DefaultZstandardCompressionOptions = new()
    {
        Quality = 1 // Fastest compression level
    };

    /// <summary>
    /// The compression options to use for the stream. Default quality is 1 (fastest compression level).
    /// </summary>
    public ZstandardCompressionOptions CompressionOptions { get; set; } = DefaultZstandardCompressionOptions;

    /// <inheritdoc />
    ZstandardCompressionProviderOptions IOptions<ZstandardCompressionProviderOptions>.Value => this;
}


Review comment (suggestion):
I think we should just use the defaults from the underlying zstd library.
https://source.dot.net/#System.IO.Compression.Zstandard/System/IO/Compression/ZstandardCompressionOptions.cs,37
> Value 0 indicates the implementation-defined default quality.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.IO.Compression;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.ResponseCompression;

/// <summary>
/// Options for the <see cref="ZstandardCompressionProvider"/>
/// </summary>
public class ZstandardCompressionProviderOptions : IOptions<ZstandardCompressionProviderOptions>
{
    /// <summary>
    /// The compression options to use for the stream.
    /// </summary>
    public ZstandardCompressionOptions CompressionOptions { get; set; } = new();

    /// <inheritdoc />
    ZstandardCompressionProviderOptions IOptions<ZstandardCompressionProviderOptions>.Value => this;
}
Repository: dotnet/efcore
File: src/Microsoft.Data.Sqlite.Core/SqliteDataRecord.cs (C#)

Code before:

    public void Dispose()
    {
        var rc = sqlite3_reset(Handle);
        if (!_alreadyThrown)
        {
            SqliteException.ThrowExceptionForRC(rc, connection.Handle);
        }

        if (!_alreadyAddedChanges)
        {
            AddChanges();
        }
    }

    private void AddChanges()
    {
        if (sqlite3_stmt_readonly(Handle) != 0)
        {
            return;
        }

        var changes = sqlite3_changes(connection.Handle);
        addChanges(changes);
    }

    private byte[] GetCachedBlob(int ordinal)
    {
        if (ordinal < 0
            || ordinal >= FieldCount)
        {
            // NB: Message is provided by the framework
            throw new ArgumentOutOfRangeException(nameof(ordinal), ordinal, message: null);
        }

        var blob = _blobCache?[ordinal];
        if (blob == null)
        {
            blob = GetBlob(ordinal);
            _blobCache ??= new byte[FieldCount][];
            _blobCache[ordinal] = blob;
        }

        return blob;
    }

    internal static int Sqlite3AffinityType(string dataTypeName)
    {
        if (dataTypeName == null)
        {
            // if no type is specified then the column has affinity BLOB

Review comment (performance):
Using Thread.Sleep in a loop can be inefficient and may block the thread unnecessarily. Consider using a more sophisticated backoff strategy or making this configurable, especially since this could impact application responsiveness during busy periods.
```suggestion
            Thread.Sleep(sleepMs);
            // Exponential backoff, up to maxSleepMs
            sleepMs = Math.Min(sleepMs * 2, maxSleepMs);
```

Code after:

        return false;
    }

    public void Dispose()
            => DisposeWithBusyHandling(timeout: -1, totalElapsedTime: default);

    internal void DisposeWithBusyHandling(int timeout, TimeSpan totalElapsedTime)
    {
        int rc;

        var timer = SharedStopwatch.StartNew();

        while (IsBusy(rc = sqlite3_reset(Handle)))
        {
            if (timeout == -1)
            {
                break;
            }
            if (timeout != 0
                && (totalElapsedTime + timer.Elapsed).TotalMilliseconds >= timeout * 1000L)
            {
                break;
            }

            Thread.Sleep(150);
        }

        if (!_alreadyThrown)
        {
            SqliteException.ThrowExceptionForRC(rc, connection.Handle);
        }

        if (!_alreadyAddedChanges)
        {
            AddChanges();
        }
    }

    private void AddChanges()
    {
        if (sqlite3_stmt_readonly(Handle) != 0)
        {
            return;
        }

        var changes = sqlite3_changes(connection.Handle);
        addChanges(changes);
    }

    private byte[] GetCachedBlob(int ordinal)
Repository: dotnet/efcore
File: src/Microsoft.Data.Sqlite.Core/SqliteDataRecord.cs (C#)

Code before:
        AddChanges();
        _alreadyAddedChanges = true;

        return false;
    }

    public void Dispose()
    {
        var rc = sqlite3_reset(Handle);
        if (!_alreadyThrown)
        {
            SqliteException.ThrowExceptionForRC(rc, connection.Handle);
        }

        if (!_alreadyAddedChanges)
        {
            AddChanges();
        }
    }

    private void AddChanges()
    {
        if (sqlite3_stmt_readonly(Handle) != 0)
        {
            return;
        }

        var changes = sqlite3_changes(connection.Handle);
        addChanges(changes);
    }

    private byte[] GetCachedBlob(int ordinal)
    {
        if (ordinal < 0
            || ordinal >= FieldCount)
        {
            // NB: Message is provided by the framework
            throw new ArgumentOutOfRangeException(nameof(ordinal), ordinal, message: null);
        }

        var blob = _blobCache?[ordinal];
        if (blob == null)
        {
            blob = GetBlob(ordinal);
            _blobCache ??= new byte[FieldCount][];
            _blobCache[ordinal] = blob;
        }

        return blob;
    }


Review comment (suggestion):
The timeout calculation multiplies by 1000L on each iteration. Consider pre-calculating `timeout * 1000L` outside the loop to avoid repeated multiplication operations.
```suggestion
                && (totalElapsedTime + timer.Elapsed).TotalMilliseconds >= timeoutMilliseconds)
```

Code after:
            return true;
        }

        AddChanges();
        _alreadyAddedChanges = true;

        return false;
    }

    public void Dispose()
            => DisposeWithBusyHandling(timeout: -1, totalElapsedTime: default);

    internal void DisposeWithBusyHandling(int timeout, TimeSpan totalElapsedTime)
    {
        int rc;

        var timer = SharedStopwatch.StartNew();

        while (IsBusy(rc = sqlite3_reset(Handle)))
        {
            if (timeout == -1)
            {
                break;
            }
            if (timeout != 0
                && (totalElapsedTime + timer.Elapsed).TotalMilliseconds >= timeout * 1000L)
            {
                break;
            }

            Thread.Sleep(150);
        }

        if (!_alreadyThrown)
        {
            SqliteException.ThrowExceptionForRC(rc, connection.Handle);
        }

        if (!_alreadyAddedChanges)
        {
            AddChanges();
        }
    }

    private void AddChanges()
    {
        if (sqlite3_stmt_readonly(Handle) != 0)
        {
            return;
        }
Repository: dotnet/efcore
File: src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs (C#)

Code before:
            if (innerExpression != null
                && _nullSafeAccesses.Contains(innerExpression))
            {
                _nullSafeAccesses.Add(memberExpression);
            }

            return memberExpression;
        }

        protected override Expression VisitUnary(UnaryExpression unaryExpression)
        {
            var operand = Visit(unaryExpression.Operand);
            if (unaryExpression.NodeType is ExpressionType.Convert or ExpressionType.ConvertChecked
                && _nullSafeAccesses.Contains(operand))
            {
                _nullSafeAccesses.Add(unaryExpression);
            }

            return unaryExpression;
        }
    }

    private static bool IsNullConstant(Expression expression)
        => expression is ConstantExpression { Value: null };

    private static bool IsQueryableType(Type type)
    {
        if (type.IsGenericType)
        {
            var genericTypeDefinition = type.GetGenericTypeDefinition();
            return genericTypeDefinition == typeof(IQueryable<>)
                || genericTypeDefinition == typeof(IOrderedQueryable<>)
                || genericTypeDefinition == typeof(DbSet<>);
        }

        return type == typeof(IQueryable);
    }
}


Review comment (refactor):
@copilot This could be shortened to `type.TryGetElementType(typeof(IQueryable)) is not null`. Do that and simplify the method. Do that and possibly inline the result, should be basically one line.

Code after:
        }

        [return: NotNullIfNotNull(nameof(expression))]
        public override Expression? Visit(Expression? expression)
            => expression == null || _nullSafeAccesses.Contains(expression)
                ? expression
                : base.Visit(expression);

        protected override Expression VisitMember(MemberExpression memberExpression)
        {
            var innerExpression = Visit(memberExpression.Expression);
            if (innerExpression != null
                && _nullSafeAccesses.Contains(innerExpression))
            {
                _nullSafeAccesses.Add(memberExpression);
            }

            return memberExpression;
        }

        protected override Expression VisitUnary(UnaryExpression unaryExpression)
        {
            var operand = Visit(unaryExpression.Operand);
            if (unaryExpression.NodeType is ExpressionType.Convert or ExpressionType.ConvertChecked
                && _nullSafeAccesses.Contains(operand))
            {
                _nullSafeAccesses.Add(unaryExpression);
            }

            return unaryExpression;
        }
    }
}
Repository: dotnet/efcore
File: src/EFCore.Relational/Metadata/Internal/RelationalModel.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Data;
using System.Text;

namespace Microsoft.EntityFrameworkCore.Metadata.Internal;

/// <summary>
///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
///     the same compatibility standards as public APIs. It may be changed or removed without notice in
///     any release. You should only use it directly in your code with extreme caution and knowing that
///     doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public class RelationalModel : Annotatable, IRelationalModel
{
    private bool _isReadOnly;

    /// <summary>
    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
    ///     any release. You should only use it directly in your code with extreme caution and knowing that
    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
    /// </summary>
    public RelationalModel(IModel model)
        => Model = model;

    /// <inheritdoc />
    public virtual IModel Model { get; }

Review comment (suggestion):
Unused using directive `System.Data` should be removed as there are no types from this namespace used in the file.
```suggestion

```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text;

namespace Microsoft.EntityFrameworkCore.Metadata.Internal;

/// <summary>
///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
///     the same compatibility standards as public APIs. It may be changed or removed without notice in
///     any release. You should only use it directly in your code with extreme caution and knowing that
///     doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public class RelationalModel : Annotatable, IRelationalModel
{
    private bool _isReadOnly;

    /// <summary>
    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
    ///     any release. You should only use it directly in your code with extreme caution and knowing that
    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
    /// </summary>
    public RelationalModel(IModel model)
        => Model = model;

    /// <inheritdoc />
    public virtual IModel Model { get; }