ronantakizawa/codereview-bench huggingface dataset:
Repository: vapor/vapor
File: Sources/VaporTesting/withApp.swift (Swift)

Code before:
import Vapor

/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: The method where you should register services like routes, databases, providers, and more.
///   - test: The method where you can perform your tests with the configured application.
@discardableResult public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        if let configure {
            try await configure(app)
        }
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


Review comment (suggestion):
```suggestion
///   - test: A closure which performs your actual test with the configured application.
```

Code after:
import Vapor

/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: A closure where you can register routes, databases, providers, and more.
///   - test: A closure which performs your actual test with the configured application.
@discardableResult
public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        try await configure?(app)
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
Repository: vapor/vapor
File: Sources/Vapor/HTTP/EndpointCache.swift (Swift)

Code before:
import Foundation
import NIOConcurrencyHelpers
import NIOCore
import Logging
import HTTPTypes

public enum EndpointCacheError: Swift.Error {
    case unexpctedResponseStatus(HTTPStatus, uri: URI)
    case contentDecodeFailure(any Error)
}

/// Handles the complexities of HTTP caching.
public actor EndpointCache<T>: Sendable where T: Decodable & Sendable {
    private var cached: (cachedData: T?, cacheDate: Date?)
    private var request: Task<T, any Error>?
    private var headers: HTTPFields?
    private let uri: URI

    /// The designated initializer.
    /// - Parameters:
    ///   - uri: The `URI` of the resource to be downloaded.
    public init(uri: URI) {
        self.uri = uri
        self.request = nil
        self.headers = nil
        self.cached = (nil, nil)
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - request: The `Request` which is initiating the download.
    ///   - logger: An optional logger
    public func get(on request: Request, logger: Logger? = nil) async throws -> T {
        try await self.download(using: request.client, logger: logger ?? request.logger)
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - client: The `Client` which will perform the download.

Review comment (suggestion):
we no longer need this in Vapor 5 (we can do breaking changes there), so you can just get rid of it.

Code after:
import Foundation
import NIOConcurrencyHelpers
import NIOCore
import Logging
import HTTPTypes

public enum EndpointCacheError: Swift.Error {
    case unexpectedResponseStatus(HTTPStatus, uri: URI)
    case contentDecodeFailure(any Error)

    @available(*, deprecated, renamed: "unexpectedResponseStatus")
    public static func unexpctedResponseStatus(_ status: HTTPStatus, uri: URI) -> Self {
        .unexpectedResponseStatus(status, uri: uri)
    }
}

/// Handles the complexities of HTTP caching.
public actor EndpointCache<T>: Sendable where T: Decodable & Sendable {
    private var cached: (cachedData: T?, cacheDate: Date?)
    private var request: Task<T, any Error>?
    private var headers: HTTPFields?
    private let uri: URI

    /// The designated initializer.
    /// - Parameters:
    ///   - uri: The `URI` of the resource to be downloaded.
    public init(uri: URI) {
        self.uri = uri
        self.request = nil
        self.headers = nil
        self.cached = (nil, nil)
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - request: The `Request` which is initiating the download.
    ///   - logger: An optional logger
    public func get(on request: Request, logger: Logger? = nil) async throws -> T {
        try await self.download(using: request.client, logger: logger ?? request.logger)
Repository: vapor/vapor
File: Tests/VaporTests/AsyncRouteTests.swift (Swift)

Code before:
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "//foop//barp/buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "//foop//barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "/foop//barp/buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "/foop//barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "/foop/barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "//foop/barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }
    }
}

#if compiler(>=6.1)
extension WebSocket: @retroactive Equatable {}
extension Vapor.WebSocket: Swift.Hashable {}
#else
extension WebSocket: Equatable {}
extension Vapor.WebSocket: Swift.Hashable {}
#endif

extension Vapor.WebSocket {
    public static func == (lhs: WebSocket, rhs: WebSocket) -> Bool {
        lhs === rhs
    }

    public func hash(into hasher: inout Hasher) {
        ObjectIdentifier(self).hash(into: &hasher)
    }
}


Review comment (suggestion):
Use `@retroactive` exclusively if compiler is >=6.1, in 6.2 the "fully-qualified names" workaround no longer silences the warnings.

Code after:
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "//foop//barp/buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "//foop//barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "/foop//barp/buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "/foop//barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "/foop/barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }.test(.GET, "//foop/barp//buz") { res in
            XCTAssertEqual(res.status, .ok)
            XCTAssertEqual(res.body.string, "foopbarp")
        }
    }
}

#if compiler(>=6.1)
extension WebSocket: @retroactive Equatable {}
extension Vapor.WebSocket: @retroactive Swift.Hashable {}
#else
extension WebSocket: Equatable {}
extension Vapor.WebSocket: Swift.Hashable {}
#endif

extension Vapor.WebSocket {
    public static func == (lhs: WebSocket, rhs: WebSocket) -> Bool {
        lhs === rhs
    }

    public func hash(into hasher: inout Hasher) {
        ObjectIdentifier(self).hash(into: &hasher)
    }
}
Repository: vercel/next.js
File: test/e2e/app-dir/static-generation-status/index.test.ts (TypeScript)

Code before:
import { nextTestSetup } from 'e2e-utils'

const isPPREnabled = process.env.__NEXT_EXPERIMENTAL_PPR === 'true'

describe('app-dir static-generation-status', () => {
  const { next } = nextTestSetup({
    files: __dirname,
  })

  it('should render the page using notFound with status 404', async () => {
    const { status } = await next.fetch('/not-found-page')
    expect(status).toBe(404)
  })

  it('should render the page using redirect with status 307', async () => {
    const { status } = await next.fetch('/redirect-page', {
      redirect: 'manual',
    })
    expect(status).toBe(307)
  })

  it('should render the client page using redirect with status 307', async () => {
    const { status } = await next.fetch('/redirect-client-page', {
      redirect: 'manual',
    })
    expect(status).toBe(isPPREnabled ? 200 : 307)
  })

  it('should respond with 308 status code if permanent flag is set', async () => {
    const { status } = await next.fetch('/redirect-permanent', {
      redirect: 'manual',
    })
    expect(status).toBe(308)
  })

  it('should render the non existed route redirect with status 404', async () => {
    expect((await next.fetch('/does-not-exist')).status).toBe(404)
  })
})


Review comment (performance):
Doesn't seem right that PPR would return a 200 but that mode will be removed anyway in favor of Cache Components where we do return a 307

Code after:
import { nextTestSetup } from 'e2e-utils'

describe('app-dir static-generation-status', () => {
  const { next } = nextTestSetup({
    files: __dirname,
  })

  it('should render the page using notFound with status 404', async () => {
    const { status } = await next.fetch('/not-found-page')
    expect(status).toBe(404)
  })

  it('should render the page using redirect with status 307', async () => {
    const { status } = await next.fetch('/redirect-page', {
      redirect: 'manual',
    })
    expect(status).toBe(307)
  })

  it('should render the client page using redirect with status 307', async () => {
    const { status } = await next.fetch('/redirect-client-page', {
      redirect: 'manual',
    })
    expect(status).toBe(307)
  })

  it('should respond with 308 status code if permanent flag is set', async () => {
    const { status } = await next.fetch('/redirect-permanent', {
      redirect: 'manual',
    })
    expect(status).toBe(308)
  })

  it('should render the non existed route redirect with status 404', async () => {
    expect((await next.fetch('/does-not-exist')).status).toBe(404)
  })
})
Repository: vercel/next.js
File: test/lib/next-test-utils.ts (TypeScript)

Code before:
  }
}

export async function retry<T>(
  fn: () => T | Promise<T>,
  duration: number = 3000,
  interval: number = 500,
  description: string = fn.name
): Promise<T> {
  if (duration % interval !== 0) {
    throw new Error(
      `invalid duration ${duration} and interval ${interval} mix, duration must be evenly divisible by interval`
    )
  }

  for (let i = duration; i >= 0; i -= interval) {
    try {
      return await fn()
    } catch (err) {
      if (i === 0) {
        console.error(
          `Failed to retry${
            description ? ` ${description}` : ''
          } within ${duration}ms`
        )
        throw err
      }
      console.log(
        `Retrying${description ? ` ${description}` : ''} in ${interval}ms`
      )
      await waitFor(interval)
    }
  }

  throw new Error('Duration cannot be less than 0.')
}

export async function assertHasRedbox(browser: Playwright) {
  const redbox = browser.locateRedbox()
  try {
    await redbox.waitFor({ timeout: 5000 })
  } catch (errorCause) {
    const error = new Error('Expected Redbox but found no visible one.')
    Error.captureStackTrace(error, assertHasRedbox)
    throw error
  }

  try {
    await redbox
      .locator('[data-nextjs-error-suspended]')
      .waitFor({ state: 'detached', timeout: 10000 })

Review comment (suggestion):
Nicer to just pass a named function to `retry` IMO which also helps in callstacks.

Code after:
      throw new Error(`Unknown replacement attempt type: ${pattern}`)
    }

    const newContent = currentContent.replace(pattern, newValue)
    this.write(newContent)
  }

  prepend(str: string) {
    const content = readFileSync(this.path, 'utf8')
    this.write(str + content)
  }

  delete() {
    unlinkSync(this.path)
  }

  restore() {
    this.write(this.originalContent!)
  }
}

export async function retry<T>(
  fn: () => T | Promise<T>,
  duration: number = 3000,
  interval: number = 500,
  description: string = fn.name
): Promise<T> {
  if (duration % interval !== 0) {
    throw new Error(
      `invalid duration ${duration} and interval ${interval} mix, duration must be evenly divisible by interval`
    )
  }

  for (let i = duration; i >= 0; i -= interval) {
    try {
      return await fn()
    } catch (err) {
      if (i === 0) {
        console.error(
          `Failed to retry${
            description ? ` ${description}` : ''
          } within ${duration}ms`
        )
        throw err
      }
      debugPrint(
        `Retrying${description ? ` ${description}` : ''} in ${interval}ms`
      )
      await waitFor(interval)
    }
  }
Repository: vercel/next.js
File: examples/with-edgedb/components/Post.tsx (TypeScript)

Code before:
import React from "react";
import Streamdown from "streamdown";
import Link from "next/link";
import { PostProps } from "../pages/blog/[id]";

const Post: React.FC<{ post: PostProps }> = ({ post }) => {
  return (
    <Link href={`/blog/${post.id}`}>
      <div>
        <h2>{post.title}</h2>
        <small>By {post.authorName}</small>
        <br />
        <br />
        <Streamdown mode="static" className="streamdown">
          {post.content || ""}
        </Streamdown>
        <style jsx>{`
          div {
            color: inherit;
            padding: 2rem;
            cursor: pointer;
          }
          h2 {
            margin: 0px;
            padding-bottom: 4px;
          }
          small {

Review comment (suggestion):
```
## Error Type
Build Error

## Error Message
Export default doesn't exist in target module

## Build Output
./examples/with-edgedb/components/Post.tsx:2:1
Export default doesn't exist in target module
  1 | import React from "react";
> 2 | import Streamdown from "streamdown";
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  3 | import Link from "next/link";
  4 | import { PostProps } from "../pages/blog/[id]";
  5 |
```

Code after:
import React from "react";
import { Streamdown } from "streamdown";
import Link from "next/link";
import { PostProps } from "../pages/blog/[id]";

const Post: React.FC<{ post: PostProps }> = ({ post }) => {
  return (
    <Link href={`/blog/${post.id}`}>
      <div>
        <h2>{post.title}</h2>
        <small>By {post.authorName}</small>
        <br />
        <br />
        <Streamdown mode="static" className="streamdown">
          {post.content || ""}
        </Streamdown>
        <style jsx>{`
          div {
            color: inherit;
            padding: 2rem;
            cursor: pointer;
          }
          h2 {
            margin: 0px;
            padding-bottom: 4px;
          }
          small {
Repository: vercel/next.js
File: packages/create-next-app/helpers/get-pkg-manager.ts (TypeScript)

Code before:
  return 'npm'
}

/**
 * Get the major version of pnpm being used.
 * Returns null if unable to determine the version.
 *
 * First tries to parse from npm_config_user_agent (e.g., "pnpm/9.13.2 npm/? ..."),
 * then falls back to spawning `pnpm --version`.
 */
export function getPnpmMajorVersion(): number | null {
  // Try to get version from user agent first (e.g., "pnpm/9.13.2 npm/? node/v20.x linux x64")
  const userAgent = process.env.npm_config_user_agent || ''
  const pnpmVersionMatch = userAgent.match(/pnpm\/(\d+)/)
  if (pnpmVersionMatch) {
    return parseInt(pnpmVersionMatch[1], 10)
  }

  // Fall back to spawning pnpm --version
  try {
    const version = execSync('pnpm --version --quiet', {
      encoding: 'utf8',
      stdio: ['pipe', 'pipe', 'ignore'],
    }).trim()
    const majorVersion = parseInt(version.split('.')[0], 10)
    if (!isNaN(majorVersion)) {
      return majorVersion
    }
  } catch {
    // pnpm not available or failed to run
  }

  return null
}


Review comment (suggestion):
@cursor `majorVersion` is already a number so we should use `Number.isNaN`

Code after:
  return 'npm'
}

/**
 * Get the major version of pnpm being used.
 * Returns null if unable to determine the version.
 *
 * First tries to parse from npm_config_user_agent (e.g., "pnpm/9.13.2 npm/? ..."),
 * then falls back to spawning `pnpm --version --silent`.
 */
export function getPnpmMajorVersion(): number | null {
  // Try to get version from user agent first (e.g., "pnpm/9.13.2 npm/? node/v20.x linux x64")
  const userAgent = process.env.npm_config_user_agent || ''
  const pnpmVersionMatch = userAgent.match(/pnpm\/(\d+)/)
  if (pnpmVersionMatch) {
    return parseInt(pnpmVersionMatch[1], 10)
  }

  // Fall back to spawning pnpm --version
  try {
    const version = execSync('pnpm --version --silent', {
      encoding: 'utf8',
      stdio: ['pipe', 'pipe', 'ignore'],
    }).trim()
    const majorVersion = parseInt(version.split('.')[0], 10)
    if (!Number.isNaN(majorVersion)) {
      return majorVersion
    }
  } catch {
    // pnpm not available or failed to run
  }

  return null
}
Repository: vercel/next.js
File: packages/next/src/shared/lib/deployment-id.ts (TypeScript)

Code before:
let deploymentId: string | undefined

if (typeof window !== 'undefined') {
  deploymentId = document.documentElement.dataset.dplId
  // Immediately remove the attribute to prevent hydration errors (the dplId was inserted into the
  // HTML only), React isn't aware of it at all.
  delete document.documentElement.dataset.dplId
} else {
  // build/define-env.ts might replace this with "false"
  deploymentId = process.env.NEXT_DEPLOYMENT_ID || undefined
}

export function getDeploymentId(): string | undefined {
  return deploymentId
}

export function getDeploymentIdQueryOrEmptyString(): string {
  if (deploymentId) {
    return `?dpl=${deploymentId}`
  }
  return ''
}


Review comment (suggestion):
Maybe add a comment that this could be `globalThis.NEXT_DEPLOYMENT_ID` in a WebWorker

Code after:
let deploymentId: string | undefined

if (typeof window !== 'undefined') {
  deploymentId = document.documentElement.dataset.dplId
  // Immediately remove the attribute to prevent hydration errors (the dplId was inserted into the
  // HTML only), React isn't aware of it at all.
  delete document.documentElement.dataset.dplId
} else {
  // Client side: replaced with globalThis.NEXT_DEPLOYMENT_ID
  // Server side: left as is or replaced with a string or replaced with false
  deploymentId = process.env.NEXT_DEPLOYMENT_ID || undefined
}

export function getDeploymentId(): string | undefined {
  return deploymentId
}

export function getDeploymentIdQueryOrEmptyString(): string {
  if (deploymentId) {
    return `?dpl=${deploymentId}`
  }
  return ''
}
Repository: vercel/next.js
File: packages/next/src/build/templates/app-page.ts (TypeScript)

Code before:
          onAfterTaskError: () => {},

          onInstrumentationRequestError: (
            error,
            _request,
            errorContext,
            silenceLog
          ) =>
            routeModule.onRequestError(
              req,
              error,
              errorContext,
              silenceLog,
              routerServerContext
            ),
          err: getRequestMeta(req, 'invokeError'),
          dev: routeModule.isDev,
        },
      }

      if (isDebugStaticShell || isDebugDynamicAccesses) {
        context.renderOpts.nextExport = true
        context.renderOpts.supportsDynamicResponse = false
        context.renderOpts.isDebugDynamicAccesses = isDebugDynamicAccesses
      }

      // When we're revalidating in the background, we should not allow dynamic
      // responses.
      if (forceStaticRender) {
        context.renderOpts.supportsDynamicResponse = false
      }

      const result = await invokeRouteModule(span, context)

      const { metadata } = result

      const {
        cacheControl,
        headers = {},
        // Add any fetch tags that were on the page to the response headers.
        fetchTags: cacheTags,
        fetchMetrics,
      } = metadata

      if (cacheTags) {
        headers[NEXT_CACHE_TAGS_HEADER] = cacheTags
      }

      // Pull any fetch metrics from the render onto the request.
      ;(req as any).fetchMetrics = fetchMetrics


Review comment (suggestion):
Just above where `context` is constructed, it already does exactly this. So this block is always a noop

Code after:
          onAfterTaskError: () => {},

          onInstrumentationRequestError: (
            error,
            _request,
            errorContext,
            silenceLog
          ) =>
            routeModule.onRequestError(
              req,
              error,
              errorContext,
              silenceLog,
              routerServerContext
            ),
          err: getRequestMeta(req, 'invokeError'),
          dev: routeModule.isDev,
        },
      }

      // When we're revalidating in the background, we should not allow dynamic
      // responses.
      if (forceStaticRender) {
        context.renderOpts.supportsDynamicResponse = false
      }

      const result = await invokeRouteModule(span, context)

      const { metadata } = result

      const {
        cacheControl,
        headers = {},
        // Add any fetch tags that were on the page to the response headers.
        fetchTags: cacheTags,
        fetchMetrics,
      } = metadata

      if (cacheTags) {
        headers[NEXT_CACHE_TAGS_HEADER] = cacheTags
      }

      // Pull any fetch metrics from the render onto the request.
      ;(req as any).fetchMetrics = fetchMetrics

      // we don't throw static to dynamic errors in dev as isSSG
      // is a best guess in dev since we don't have the prerender pass
      // to know whether the path is actually static or not
      if (
        isSSG &&
        cacheControl?.revalidate === 0 &&