ronantakizawa/codereview-bench huggingface dataset:
Repository: vercel/next.js
File: test/development/typescript-native-preview/index.test.ts (TypeScript)

Code before:
import { createNext } from 'e2e-utils'
import { NextInstance } from 'e2e-utils'
import { retry } from 'next-test-utils'

describe('typescript-native-preview', () => {
  let next: NextInstance

  beforeAll(async () => {
    next = await createNext({
      files: {
        'app/layout.tsx': `
          import { ReactNode } from 'react'
          export default function Root({ children }: { children: ReactNode }) {
            return (
              <html>
                <body>{children}</body>
              </html>
            )
          }
        `,
        'app/page.tsx': `
          export default function Page() {
            return <p>hello world</p>
          }
        `,
        'tsconfig.json': JSON.stringify({
          compilerOptions: {
            target: 'ES2017',
            lib: ['dom', 'dom.iterable', 'esnext'],
            allowJs: true,
            skipLibCheck: true,
            strict: true,
            noEmit: true,
            esModuleInterop: true,

Review comment (suggestion):
Most of the more modern tests use `nextTestSetup`, which is a bit of a hack that calls `beforeAll` and `afterAll` for you and returns a `NextInstance` that's guaranteed to be valid by the time any tests run.

https://github.com/vercel/next.js/blob/6adedb5e39027f4f95e5648c84c67ca4e4a930cb/test/lib/e2e-utils/index.ts#L243-L280

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

describe('typescript-native-preview', () => {
  const { next } = nextTestSetup({
    files: {
      'app/layout.tsx': `
        import { ReactNode } from 'react'
        export default function Root({ children }: { children: ReactNode }) {
          return (
            <html>
              <body>{children}</body>
            </html>
          )
        }
      `,
      'app/page.tsx': `
        export default function Page() {
          return <p>hello world</p>
        }
      `,
      'tsconfig.json': JSON.stringify({
        compilerOptions: {
          target: 'ES2017',
          lib: ['dom', 'dom.iterable', 'esnext'],
          allowJs: true,
          skipLibCheck: true,
          strict: true,
          noEmit: true,
          esModuleInterop: true,
          module: 'esnext',
          moduleResolution: 'bundler',
          resolveJsonModule: true,
          isolatedModules: true,
Repository: vercel/next.js
File: test/e2e/app-dir/static-rsc-cache-components/static-rsc-cache-components.test.ts (TypeScript)

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

describe('static-rsc-cache-components', () => {
  const { next, isNextDev, skipped } = nextTestSetup({
    files: __dirname,
    // TODO: Enable deploy test once builder changes have landed
    skipDeployment: true,
  })

  if (isNextDev || skipped) {
    it('is skipped', () => {})
    return
  }

  it('navigates to prerendered route without waiting for dynamic render', async () => {
    const browser = await next.browser('/')

    // Track elapsed time across the client navigation.
    await browser.eval(`window.__navStart = performance.now()`)

    const link = await browser.elementByCss('a[href="/alpha"]')
    await link.click()

    const slug = await browser.elementById('slug')
    expect(await slug.innerText()).toBe('Hi alpha')

    const elapsed = await browser.eval(`performance.now() - window.__navStart`)

    // The page has an intentional 2s delay in the Server Component.
    // If this were a dynamic render, navigation would take ~2s+.
    expect(elapsed).toBeLessThan(1500)
  })
})


Review comment (suggestion):
might be good to also check on disk the `.rsc` files are populated in `isNextStart` mode before a request/the browser is loaded. 

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

describe('static-rsc-cache-components', () => {
  const { next, isNextDev } = nextTestSetup({
    files: __dirname,
  })

  if (isNextDev) {
    it('is skipped', () => {})
    return
  }

  it('navigates to prerendered route without waiting for dynamic render', async () => {
    const browser = await next.browser('/')

    // Track elapsed time across the client navigation.
    await browser.eval(`window.__navStart = performance.now()`)

    const link = await browser.elementByCss('a[href="/alpha"]')
    await link.click()

    const slug = await browser.elementById('slug')
    expect(await slug.innerText()).toBe('Hi alpha')

    const elapsed = await browser.eval(`performance.now() - window.__navStart`)

    // The page has an intentional 2s delay in the Server Component.
    // If this were a dynamic render, navigation would take ~2s+.
    expect(elapsed).toBeLessThan(1500)
  })
})
Repository: vercel/next.js
File: packages/next/src/export/index.ts (TypeScript)

Code before:
          // rewrites `/` to `/index`).
          const segmentsDirDest = join(outDir, unnormalizedRoute)
          const segmentPaths = await collectSegmentPaths(segmentsDir)
          await Promise.all(
            segmentPaths.map(async (segmentFileSrc) => {
              const segmentPath =
                '/' + segmentFileSrc.slice(0, -RSC_SEGMENT_SUFFIX.length)
              const segmentFilename =
                convertSegmentPathToStaticExportFilename(segmentPath)
              const segmentFileDest = join(segmentsDirDest, segmentFilename)
              await fs.mkdir(dirname(segmentFileDest), { recursive: true })
              await fs.copyFile(
                join(segmentsDir, segmentFileSrc),
                segmentFileDest
              )
            })
          )
        }
      })
    )
  }

  if (failedExportAttemptsByPage.size > 0) {
    const failedPages = Array.from(failedExportAttemptsByPage.keys())
    throw new ExportError(
      `Export encountered errors on following ${failedPages.length} ${failedPages.length === 1 ? 'path' : 'paths'}:\n\t${failedPages
        .sort()
        .join('\n\t')}`
    )
  }

  await fs.writeFile(
    join(distDir, EXPORT_DETAIL),
    formatManifest({
      version: 1,
      outDirectory: outDir,
      success: true,
    }),
    'utf8'
  )

  if (telemetry) {
    await telemetry.flush()
  }

  // Clean up activity listeners for progress.
  if (staticWorker) {
    staticWorker.setOnActivity(undefined)
    staticWorker.setOnActivityAbort(undefined)
  }


Review comment (bug):
while we're here

```suggestion
      `Export encountered errors on ${failedPages.length} ${failedPages.length === 1 ? 'path' : 'paths'}:\n\t${failedPages
```

"following" feels redundant when we already have a colon at the end of the sentence.

Code after:
              const segmentFileDest = join(segmentsDirDest, segmentFilename)
              await fs.mkdir(dirname(segmentFileDest), { recursive: true })
              await fs.copyFile(
                join(segmentsDir, segmentFileSrc),
                segmentFileDest
              )
            })
          )
        }
      })
    )
  }

  if (failedExportAttemptsByPage.size > 0) {
    const failedPages = Array.from(failedExportAttemptsByPage.keys())
    throw new ExportError(
      `Export encountered errors on ${failedPages.length} ${failedPages.length === 1 ? 'path' : 'paths'}:\n\t${failedPages
        .sort()
        .join('\n\t')}`
    )
  }

  await fs.writeFile(
    join(distDir, EXPORT_DETAIL),
    formatManifest({
      version: 1,
      outDirectory: outDir,
      success: true,
    }),
    'utf8'
  )

  if (telemetry) {
    await telemetry.flush()
  }

  // Clean up activity listeners for progress.
  if (staticWorker) {
    staticWorker.setOnActivity(undefined)
    staticWorker.setOnActivityAbort(undefined)
  }

  if (!staticWorker && worker) {
    await worker.end()
  }

  return collector
}

async function collectSegmentPaths(segmentsDirectory: string) {
  const results: Array<string> = []
Repository: vercel/next.js
File: packages/next/src/build/swc/options.ts (TypeScript)

Code before:
    development: false,
    hasReactRefresh: false,
    globalWindow: !isServer,
    modularizeImports,
    swcPlugins,
    compilerOptions,
    jsConfig,
    resolvedBaseUrl,
    supportedBrowsers: undefined,
    esm,
    // Don't apply server layer transformations for Jest
    // Disable server / client graph assertions for Jest
    bundleLayer: undefined,
    serverComponents: false,
    serverReferenceHashSalt,
  })

  // In production, webpack DefinePlugin replaces process.env.__NEXT_IMAGE_OPTS
  // with an object literal at compile time. Emulate that here by enabling
  // SWC's optimizer globals.envs so the same compile-time replacement happens
  // during Jest transforms.
  if (imageConfig) {
    baseOptions.jsc.transform.optimizer.globals = {
      envs: {
        __NEXT_IMAGE_OPTS: JSON.stringify(imageConfig),
      },
    } as any
  }

  const useCjsModules = shouldOutputCommonJs(filename)
  return {
    ...baseOptions,
    env: {
      targets: {
        // Targets the current version of Node.js
        node: process.versions.node,
      },
    },
    module: {
      type: esm && !useCjsModules ? 'es6' : 'commonjs',
    },
    disableNextSsg: true,
    pagesDir,
  }
}

export function getLoaderSWCOptions({
  // This is not passed yet as "paths" resolving is handled by webpack currently.
  // resolvedBaseUrl,
  filename,
  development,

Review comment (performance):
```suggestion
        ...baseOptions.jsc.transform.optimizer.globals?.env,
        __NEXT_IMAGE_OPTS: JSON.stringify(imageConfig),
```

Do we need to merge with existing env object if any?

Code after:
    development: false,
    hasReactRefresh: false,
    globalWindow: !isServer,
    modularizeImports,
    swcPlugins,
    compilerOptions,
    jsConfig,
    resolvedBaseUrl,
    supportedBrowsers: undefined,
    esm,
    // Don't apply server layer transformations for Jest
    // Disable server / client graph assertions for Jest
    bundleLayer: undefined,
    serverComponents: false,
    serverReferenceHashSalt,
  })

  // In production, webpack DefinePlugin replaces process.env.__NEXT_IMAGE_OPTS
  // with an object literal at compile time. Emulate that here by enabling
  // SWC's optimizer globals.envs so the same compile-time replacement happens
  // during Jest transforms.
  if (imageConfig) {
    baseOptions.jsc.transform.optimizer.globals = {
      envs: {
        ...baseOptions.jsc.transform.optimizer.globals?.envs,
        __NEXT_IMAGE_OPTS: JSON.stringify(imageConfig),
      },
    } as any
  }

  const useCjsModules = shouldOutputCommonJs(filename)
  return {
    ...baseOptions,
    env: {
      targets: {
        // Targets the current version of Node.js
        node: process.versions.node,
      },
    },
    module: {
      type: esm && !useCjsModules ? 'es6' : 'commonjs',
    },
    disableNextSsg: true,
    pagesDir,
  }
}

export function getLoaderSWCOptions({
  // This is not passed yet as "paths" resolving is handled by webpack currently.
  // resolvedBaseUrl,
  filename,
Repository: vercel/next.js
File: packages/next/src/build/swc/options.ts (TypeScript)

Code before:
}

export function getJestSWCOptions({
  isServer,
  filename,
  esm,
  modularizeImports,
  swcPlugins,
  compilerOptions,
  jsConfig,
  resolvedBaseUrl,
  pagesDir,
  imageConfig,
  serverReferenceHashSalt,
}: {
  isServer: boolean
  filename: string
  esm: boolean
  modularizeImports?: NextConfig['modularizeImports']
  swcPlugins: ExperimentalConfig['swcPlugins']
  compilerOptions: NextConfig['compiler']
  jsConfig: any
  resolvedBaseUrl?: ResolvedBaseUrl
  pagesDir?: string
  serverComponents?: boolean
  imageConfig?: Record<string, unknown>
  serverReferenceHashSalt: string
}) {
  let baseOptions = getBaseSWCOptions({
    filename,
    jest: true,
    development: false,
    hasReactRefresh: false,
    globalWindow: !isServer,
    modularizeImports,
    swcPlugins,
    compilerOptions,
    jsConfig,
    resolvedBaseUrl,
    supportedBrowsers: undefined,
    esm,
    // Don't apply server layer transformations for Jest
    // Disable server / client graph assertions for Jest
    bundleLayer: undefined,
    serverComponents: false,
    serverReferenceHashSalt,
  })

  // In production, webpack DefinePlugin replaces process.env.__NEXT_IMAGE_OPTS
  // with an object literal at compile time. Emulate that here by enabling
  // SWC's optimizer globals.envs so the same compile-time replacement happens

Review comment (suggestion):
```suggestion
  imageConfig?: Partial<NextConfig['images']>
```

Code after:
}

export function getJestSWCOptions({
  isServer,
  filename,
  esm,
  modularizeImports,
  swcPlugins,
  compilerOptions,
  jsConfig,
  resolvedBaseUrl,
  pagesDir,
  imageConfig,
  serverReferenceHashSalt,
}: {
  isServer: boolean
  filename: string
  esm: boolean
  modularizeImports?: NextConfig['modularizeImports']
  swcPlugins: ExperimentalConfig['swcPlugins']
  compilerOptions: NextConfig['compiler']
  jsConfig: any
  resolvedBaseUrl?: ResolvedBaseUrl
  pagesDir?: string
  serverComponents?: boolean
  imageConfig?: Partial<NextConfig['images']>
  serverReferenceHashSalt: string
}) {
  let baseOptions = getBaseSWCOptions({
    filename,
    jest: true,
    development: false,
    hasReactRefresh: false,
    globalWindow: !isServer,
    modularizeImports,
    swcPlugins,
    compilerOptions,
    jsConfig,
    resolvedBaseUrl,
    supportedBrowsers: undefined,
    esm,
    // Don't apply server layer transformations for Jest
    // Disable server / client graph assertions for Jest
    bundleLayer: undefined,
    serverComponents: false,
    serverReferenceHashSalt,
  })

  // In production, webpack DefinePlugin replaces process.env.__NEXT_IMAGE_OPTS
  // with an object literal at compile time. Emulate that here by enabling
  // SWC's optimizer globals.envs so the same compile-time replacement happens
Repository: vercel/next.js
File: test/e2e/app-dir/not-found-with-pages-i18n/not-found-with-pages.test.ts (TypeScript)

Code before:
  if (isNextStart) {
    it('should write all locales to the pages manifest', async () => {
      const pagesManifest = JSON.parse(
        await next.readFile('.next/server/pages-manifest.json')
      )

      expect(pagesManifest['/404']).toBe('pages/404.html')
      expect(pagesManifest['/en/404']).toBe('pages/404.html')
      expect(pagesManifest['/en-GB/404']).toBe('pages/404.html')
    })
  }

  it('should prefer the app router 404 over the pages router 404 when both are present - app', async () => {
    const browser = await next.browser('/app-dir/foo')
    expect(await browser.elementByCss('h1').text()).toBe(
      'APP ROUTER - 404 PAGE'
    )

    await browser.loadPage(next.url)
    expect(await browser.elementByCss('h1').text()).toBe(
      'APP ROUTER - 404 PAGE'
    )
  })

  it('should prefer the app router 404 over the pages router 404 when both are present - pages', async () => {
    const browser = await next.browser('/foo')
    expect(await browser.elementByCss('h1').text()).toBe(
      'APP ROUTER - 404 PAGE'
    )

    await browser.loadPage(next.url)
    expect(await browser.elementByCss('h1').text()).toBe(
      'APP ROUTER - 404 PAGE'
    )
  })
})


Review comment (suggestion):
Might be good for these to also do fetch requests themselves to check for `status` code itself and server rendered HTML with hydration.

Code after:
  })

  if (isNextStart) {
    it('should write all locales to the pages manifest', async () => {
      const pagesManifest = JSON.parse(
        await next.readFile('.next/server/pages-manifest.json')
      )

      expect(pagesManifest['/404']).toBe('pages/404.html')
      expect(pagesManifest['/en/404']).toBe('pages/404.html')
      expect(pagesManifest['/en-GB/404']).toBe('pages/404.html')
    })
  }

  // When using the builder, pages router is incorrectly preferred. This is already fixed when using the adapter
  const SKIP_TEST_FOR_BUILDER = isNextDeploy ? !isAdapterTest : false

  if (!SKIP_TEST_FOR_BUILDER) {
    it.each(['/app-dir/foo', '/foo'])(
      'should prefer the app router 404 over the pages router 404 when both are present - browser %s',
      async (s) => {
        const browser = await next.browser(s)
        expect(await browser.elementByCss('h1').text()).toBe(
          'APP ROUTER - 404 PAGE'
        )

        await browser.loadPage(next.url)
        expect(await browser.elementByCss('h1').text()).toBe(
          'APP ROUTER - 404 PAGE'
        )
      }
    )
  }

  it.each(SKIP_TEST_FOR_BUILDER ? ['/app-dir/foo'] : ['/app-dir/foo', '/foo'])(
    'should prefer the app router 404 over the pages router 404 when both are present - SSR %s',
    async (s) => {
      const ssr = await next.fetch(s)
      expect(ssr.status).toEqual(404)
      expect(await ssr.text()).toContain('APP ROUTER - 404 PAGE')
    }
  )
})
Repository: vercel/next.js
File: scripts/pull-turbo-cache.js (JavaScript)

Code before:
      stdio: captureOutput ? 'pipe' : stdio,
    })

    if (captureOutput) {
      child.stdout?.on('data', (data) => {
        process.stdout.write(data)
        output += data.toString()
      })
      child.stderr?.on('data', (data) => {
        process.stderr.write(data)
      })
    }

    child.on('exit', (code, signal) => {
      resolve({ code, signal, output })
    })
  })
}

/**
 * @param {string} command
 * @param {number} maxRetries
 * @returns {Promise<boolean>}
 */
async function runWithRetry(command, maxRetries) {
  for (let attempt = 1; attempt <= maxRetries; attempt++) {
    console.log(`Attempt ${attempt}/${maxRetries}...`)
    const { code, signal } = await runCommand(command)

    if (!code && !signal) {
      return true // success
    }

    console.warn(
      `Attempt ${attempt} failed (exit code ${code}, signal ${signal})`
    )

    if (attempt < maxRetries) {
      console.log(`Retrying in ${RETRY_DELAY_MS / 1000}s...`)
      await sleep(RETRY_DELAY_MS)
    }
  }
  return false // all retries failed
}

;(async function () {
  const target = process.argv[process.argv.length - 1]
  const turboCommand = `pnpm dlx turbo@${process.env.TURBO_VERSION || 'latest'}`

  // First, do a dry run to check cache status
  const { code, signal, output } = await runCommand(

Review comment (suggestion):
"maxAttempts" you meant here? If you set `maxRetries` to 1, we only attempt once and never retry.

Code after:
      stdio: captureOutput ? 'pipe' : stdio,
    })

    if (captureOutput) {
      child.stdout?.on('data', (data) => {
        process.stdout.write(data)
        output += data.toString()
      })
      child.stderr?.on('data', (data) => {
        process.stderr.write(data)
      })
    }

    child.on('exit', (code, signal) => {
      resolve({ code, signal, output })
    })
  })
}

/**
 * @param {string} command
 * @param {number} maxAttempts
 * @returns {Promise<boolean>}
 */
async function runWithRetry(command, maxAttempts) {
  for (let attempt = 1; attempt <= maxAttempts; attempt++) {
    console.log(`Attempt ${attempt}/${maxAttempts}...`)
    const { code, signal } = await runCommand(command)

    if (!code && !signal) {
      return true // success
    }

    console.warn(
      `Attempt ${attempt} failed (exit code ${code}, signal ${signal})`
    )

    if (attempt < maxAttempts) {
      console.log(`Retrying in ${RETRY_DELAY_MS / 1000}s...`)
      await sleep(RETRY_DELAY_MS)
    }
  }
  return false // all attempts failed
}

;(async function () {
  const target = process.argv[process.argv.length - 1]
  const turboCommand = `pnpm dlx turbo@${process.env.TURBO_VERSION || 'latest'}`

  // First, do a dry run to check cache status
  const { code, signal, output } = await runCommand(
Repository: vercel/next.js
File: test/unit/image-optimizer/find-closest-quality.test.ts (TypeScript)

Code before:
      output: 75,
    },
    {
      input: [undefined, { qualities: [25, 60, 100] }],
      output: 60, // use closest to 75 when 75 is not in the config
    },
    {
      input: [undefined, { qualities: [25, 50, 75] }],
      output: 75, // use 75 when 75 is in the config
    },
    {
      input: [undefined, { qualities: [100, 10, 75, 15] }],
      output: 75, // use 75 when 75 is in the config, even out of order
    },
    {
      input: [10, { qualities: [100, 10, 75, 15] }],
      output: 10, // use input even when config is out of order
    },
    {
      input: [14, { qualities: [100, 10, 75, 15] }],
      output: 15, // use closet input even when config is out of order
    },
    {
      input: [1, { qualities: [75] }],
      output: 75, // low quality should not return 0
    },
  ])('for quality $input expected $output', ({ input, output }) => {
    expect(findClosestQuality(...input)).toEqual(output)
  })
})


Review comment (suggestion):
```suggestion
    },
    {
      input: [4, { qualities: [5, 25] }],
      output: 5, // ensure low quality still rounds up
    },
    {
      input: [6, { qualities: [5, 25] }],
      output: 5, // ensure low quality still rounds down
    },
```

Code after:
      output: 75,
    },
    {
      input: [undefined, { qualities: [25, 60, 100] }],
      output: 60, // use closest to 75 when 75 is not in the config
    },
    {
      input: [undefined, { qualities: [25, 50, 75] }],
      output: 75, // use 75 when 75 is in the config
    },
    {
      input: [undefined, { qualities: [100, 10, 75, 15] }],
      output: 75, // use 75 when 75 is in the config, even out of order
    },
    {
      input: [10, { qualities: [100, 10, 75, 15] }],
      output: 10, // use input even when config is out of order
    },
    {
      input: [14, { qualities: [100, 10, 75, 15] }],
      output: 15, // use closet input even when config is out of order
    },
    {
      input: [1, { qualities: [75] }],
      output: 75, // low quality should not return 0
    },
    {
      input: [4, { qualities: [5, 25] }],
      output: 5, // ensure low quality still rounds up
    },
    {
      input: [6, { qualities: [5, 25] }],
      output: 5, // ensure low quality still rounds down
    },
  ])('for quality $input expected $output', ({ input, output }) => {
    expect(findClosestQuality(...input)).toEqual(output)
  })
})
Repository: vercel/next.js
File: test/e2e/app-dir/errors/app/server-component/recover/page.js (JavaScript)

Code before:
import { Suspense } from 'react'
import { connection } from 'next/server'

export default function Page() {
  return (
    <Suspense>
      <PageImpl />
    </Suspense>
  )
}

let hasThrown = false

async function PageImpl() {
  // Throw only during the runtime
  await connection()

  if (!hasThrown) {
    hasThrown = true
    throw new Error('this is a test')
  }

  return <p id="recover">Recovered</p>
}


Review comment (suggestion):
It should probably set a globalThis to false and then in the test itself you set it to true to switch from erroring to recovered. by using a render side effect the test could change subtly  in the future based on multiple renders that might make it no longer assert what we expect

Code after:
import { Suspense } from 'react'
import { connection } from 'next/server'

export default function Page() {
  return (
    <Suspense>
      <Container>
        <Content />
      </Container>
    </Suspense>
  )
}

function Container({ children }) {
  return <div>{children}</div>
}

async function Content() {
  await connection()

  if (!globalThis.__nextTestRecover) {
    throw new Error('this is a test')
  }

  return <p id="recover">Recovered</p>
}