Doc Improvements #3
Conversation
Merge into canary
…files with 86+ new test cases
CodeRabbit Generated Unit Tests: Expand test suites for four Next.js files with 86+ new test cases
Removed the buildPassed job from the workflow.
Updated dependencies to fix Next.js and React CVE vulnerabilities. The fix-react2shell-next tool automatically updated the following packages to their secure versions: - next - react-server-dom-webpack - react-server-dom-parcel - react-server-dom-turbopack All package.json files have been scanned and vulnerable versions have been patched to the correct fixed versions based on the official React advisory. Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
…nents-cve-vu-9xhypu
Summary of ChangesHello @JaclynCodes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's robustness and maintainability by expanding test coverage, standardizing core dependencies, and refining CI/CD pipelines. The changes aim to improve code quality, catch regressions earlier, and ensure a more consistent development environment across various modules and examples. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves test coverage for several utility modules and updates dependencies across the repository. The new tests are comprehensive and cover many edge cases. I've left one comment regarding test isolation for the logging utilities to improve test robustness. The addition of TEST_ENHANCEMENTS.md is a great way to document these improvements. The dependency updates seem to be a good housekeeping measure to ensure stability.
| it('should cache based on complete message combination', () => { | ||
| warnOnce('part1', 'part2') | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('part1', 'part2') | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('part1', 'part3') | ||
| expect(messages.length).toBe(2) | ||
| }) |
There was a problem hiding this comment.
The tests for warnOnce are not properly isolated. warnOnce uses a module-level cache that is not reset between tests. This can lead to flaky tests that depend on execution order.
To fix this, you need to reset Jest's module cache and get a fresh instance of warnOnce for each test. This should be applied to all tests for warnOnce in this suite.
The same issue applies to the errorOnce tests. While they use require inside each test, they are missing jest.resetModules(), so they are also not correctly isolated.
it('should cache based on complete message combination', () => {
jest.resetModules()
const { warnOnce } = require('next/dist/build/output/log')
warnOnce('part1', 'part2')
expect(messages.length).toBe(1)
warnOnce('part1', 'part2')
expect(messages.length).toBe(1)
warnOnce('part1', 'part3')
expect(messages.length).toBe(2)
})
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 261 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="examples/reproduction-template/package.json">
<violation number="1" location="examples/reproduction-template/package.json:9">
P2: The reproduction template is documented to use `next@canary` for verification, but this change pins it to a stable release. That makes new reproductions inconsistent with the README guidance and can cause bugs already fixed in canary to be missed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| "dependencies": { | ||
| "next": "canary", | ||
| "next": "15.5.8", |
There was a problem hiding this comment.
P2: The reproduction template is documented to use next@canary for verification, but this change pins it to a stable release. That makes new reproductions inconsistent with the README guidance and can cause bugs already fixed in canary to be missed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/reproduction-template/package.json, line 9:
<comment>The reproduction template is documented to use `next@canary` for verification, but this change pins it to a stable release. That makes new reproductions inconsistent with the README guidance and can cause bugs already fixed in canary to be missed.</comment>
<file context>
@@ -6,7 +6,7 @@
},
"dependencies": {
- "next": "canary",
+ "next": "15.5.8",
"react": "19.2.0",
"react-dom": "19.2.0"
</file context>
| "next": "15.5.8", | |
| "next": "canary", |
There was a problem hiding this comment.
Pull request overview
This PR attempts to (1) expand unit test coverage for several low-level utilities, (2) standardize/pin next dependency versions across the repo (including examples/fixtures), and (3) adjust CI by adding CodeQL scanning and simplifying an existing workflow.
Changes:
- Expanded unit tests for
addBasePath, build logging helpers,getProjectDir/detectTypo, and exported constants. - Pinned/updated
nextversions across many workspaces/examples/fixtures and updatedpnpm-lock.yamlaccordingly. - Added a new CodeQL workflow and removed the
buildPassedjob frombuild_and_deploy.yml; added aTEST_ENHANCEMENTS.mdsummary doc.
Reviewed changes
Copilot reviewed 248 out of 261 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/router-add-base-path.test.ts | Adds extensive coverage for addBasePath behavior under different env configurations. |
| test/unit/build-output-log.test.ts | Adds coverage for warnOnce/errorOnce caching and the various log helpers. |
| test/unit/get-project-dir.test.ts | Adds tests for getProjectDir and typo detection behavior. |
| test/unit/phaseConstants.test.ts | Adds broader assertions for exported constants/enums and conventions. |
| TEST_ENHANCEMENTS.md | Documents the intended test coverage improvements. |
| .github/workflows/codeql.yml | Introduces CodeQL scanning workflow (currently invalid YAML per review comments). |
| .github/workflows/build_and_deploy.yml | Removes the buildPassed job. |
| package.json | Pins root next dependency (currently to a version flagged as vulnerable in lockfile). |
| pnpm-lock.yaml | Updates lockfile for new Next versions; includes a deprecation/security notice for [email protected]. |
| turbopack/packages/turbo-tracing-next-plugin/package.json | Pins next in devDependencies for the tracing plugin. |
| turbopack/packages/turbo-tracing-next-plugin/test/with-mongodb-mongoose/package.json | Pins next in a turbopack test fixture. |
| crates/next-core/js/package.json | Pins next dependency for the next-core JS workspace. |
| apps/docs/package.json | Updates next version used by docs app. |
| apps/bundle-analyzer/package.json | Updates next version used by bundle-analyzer app. |
| packages/third-parties/package.json | Updates canary next version used by the third-parties package. |
| packages/next-codemod/bin/testfixtures/*/package.json | Updates next versions used in codemod CLI fixtures. |
| test/.stats-app/package.json | Pins next in stats app fixture. |
| test/integration/styled-jsx-plugin/app/package.json | Pins next in an integration test fixture. |
| test/e2e/next-test/first-time-setup-{js,ts}/package.json | Pins next in first-time setup e2e fixtures. |
| bench/*/package.json | Pins next versions in benchmark workspaces. |
| examples/**/package.json | Pins next versions across many examples. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('with basePath set', () => { | ||
| const originalBasePath = process.env.__NEXT_ROUTER_BASEPATH | ||
| const originalManualBasePath = process.env.__NEXT_MANUAL_CLIENT_BASE_PATH | ||
|
|
||
| beforeEach(() => { | ||
| process.env.__NEXT_ROUTER_BASEPATH = '/docs' | ||
| delete process.env.__NEXT_MANUAL_CLIENT_BASE_PATH | ||
| }) |
There was a problem hiding this comment.
addBasePath captures __NEXT_ROUTER_BASEPATH at module initialization time (see packages/next/src/client/add-base-path.ts), but this test sets process.env.__NEXT_ROUTER_BASEPATH in beforeEach after importing addBasePath. As a result, the "with basePath set" assertions won't reflect the updated env and will fail. Consider setting env vars before importing the module (e.g., jest.resetModules() + require(...) after setting env) or using jest.isolateModules per suite.
| it('should resolve provided directory path', () => { | ||
| const { getProjectDir } = require('next/dist/lib/get-project-dir') | ||
| const result = getProjectDir('.', false) | ||
| expect(result).toBeTruthy() | ||
| expect(result).toContain('test') | ||
| }) |
There was a problem hiding this comment.
getProjectDir('.', false) resolves to the current working directory (repo root in CI), which typically will not contain the substring 'test'. This expectation is brittle and likely to fail depending on where tests are run. Prefer asserting equality with realpathSync(process.cwd()), or at least that the path exists / is absolute.
| describe('constant immutability', () => { | ||
| it('should not allow reassignment of phase constants', () => { | ||
| expect(() => { | ||
| // @ts-expect-error - testing immutability | ||
| PHASE_EXPORT = 'something-else' | ||
| }).toThrow() | ||
| }) |
There was a problem hiding this comment.
This "immutability" test is only verifying that the local imported binding is read-only/const, not that the exported value is immutable. It will throw regardless of the implementation of next/constants, so it doesn't add meaningful coverage. Consider removing it or replacing it with a behavior-based assertion that can actually regress.
| matrix: | ||
| include: | ||
| - language: actions | ||
| build-mode: none | ||
| - language: javascript-typescript | ||
| build-mode: none | ||
| - language: python | ||
| build-mode: none | ||
| - language: rust | ||
| build-mode: none |
There was a problem hiding this comment.
The YAML indentation for the matrix.include list is invalid: the - language: ... entries need to be indented under include:. As written, this workflow won't parse and won't run.
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
There was a problem hiding this comment.
The steps: section is not properly indented: step entries (- name: ...) must be nested under steps:. As written, the workflow file is invalid YAML and GitHub Actions will reject it.
| "devDependencies": { | ||
| "next": "workspace:*" | ||
| "next": "15.5.8" |
There was a problem hiding this comment.
Changing this dev dependency from workspace:* to a pinned next version means the plugin's tests/dev setup no longer exercise the in-repo packages/next build. In this monorepo that tends to defeat the purpose of plugin integration testing, and 15.5.8 is also flagged as vulnerable in the lockfile. Consider keeping workspace:* (or otherwise documenting why this package must test against a published Next) and avoid the vulnerable version.
| }, | ||
| "dependencies": { | ||
| "next": "workspace:*" | ||
| "next": "15.5.8" |
There was a problem hiding this comment.
Switching next from workspace:* to a pinned published version makes this workspace stop testing against the in-repo packages/next code and can lead to duplicated/mismatched Next installs. Given the repo uses workspaces heavily, consider keeping workspace:* here (and avoid pinning to 15.5.8, which is marked vulnerable in the lockfile).
| "next": "15.5.8" | |
| "next": "workspace:*" |
| describe('warnOnce caching behavior', () => { | ||
| const original = console.warn | ||
| let messages: any[] | ||
|
|
||
| beforeEach(() => { | ||
| messages = [] | ||
| console.warn = (m: any) => messages.push(m) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| console.warn = original | ||
| }) | ||
|
|
||
| it('should cache based on complete message combination', () => { | ||
| warnOnce('part1', 'part2') | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('part1', 'part2') | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('part1', 'part3') | ||
| expect(messages.length).toBe(2) | ||
| }) | ||
|
|
||
| it('should handle numeric arguments', () => { | ||
| warnOnce('count', 42) | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('count', 42) | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('count', 43) | ||
| expect(messages.length).toBe(2) | ||
| }) | ||
|
|
||
| it('should handle boolean arguments', () => { | ||
| warnOnce('flag', true) | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('flag', false) | ||
| expect(messages.length).toBe(2) | ||
| }) | ||
|
|
||
| it('should handle object arguments', () => { | ||
| warnOnce('object', { key: 'value' }) | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('object', { key: 'value' }) | ||
| expect(messages.length).toBe(1) | ||
| }) | ||
|
|
||
| it('should handle empty string arguments', () => { | ||
| warnOnce('') | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('') | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce('', '') | ||
| expect(messages.length).toBe(2) | ||
| }) | ||
|
|
||
| it('should handle undefined arguments', () => { | ||
| warnOnce(undefined) | ||
| expect(messages.length).toBe(1) | ||
|
|
||
| warnOnce(undefined) | ||
| expect(messages.length).toBe(1) | ||
| }) |
There was a problem hiding this comment.
warnOnce keeps a module-level LRU cache keyed by message.join(' '). This cache persists across tests, and values like warnOnce(''), warnOnce(undefined), and warnOnce(null) all produce the same key (''), so later tests can be skipped unexpectedly depending on execution order. To make these tests reliable, isolate/reset the module between tests (e.g., jest.resetModules() in beforeEach and require(...) inside each test) or otherwise clear the cache.
| beforeAll(() => { | ||
| originalCwd = process.cwd() | ||
| originalProcessEnv = { ...process.env } | ||
| }) | ||
|
|
||
| afterAll(() => { | ||
| process.env = originalProcessEnv | ||
| }) |
There was a problem hiding this comment.
Reassigning process.env (process.env = originalProcessEnv) replaces Node’s special process.env object with a plain object and can break other tests that rely on environment behavior. Instead, restore only the keys you mutate (or snapshot/restore via Object.assign(process.env, ...) plus deleting extra keys).
| "nanoid": "3.1.30", | ||
| "next": "workspace:*", | ||
| "next": "15.5.8", | ||
| "node-fetch": "2.6.7", |
There was a problem hiding this comment.
Pinning the repo root next dependency to 15.5.8 is risky here: (1) the lockfile marks [email protected] as deprecated due to a security vulnerability, and (2) this repo already uses workspace:* for internal packages (e.g. create-next-app, eslint-config-next), so switching next away from the workspace version can break local development/testing against packages/next. Prefer using workspace:* (or at least a non-vulnerable patched version) consistently.
1 participant
For Contributors
Improving Documentation
pnpm prettier-fixto fix formatting issues before opening the PR.Fixing a bug
This pull request introduces several important changes across the repository, focusing on three main areas: enhanced test coverage, dependency updates (notably for
next), and CI workflow improvements. The most significant update is the addition of a comprehensive test enhancement document summarizing major improvements in test coverage and quality. Additionally, thenextdependency is updated to specific versions across multiple packages and examples for consistency and stability. Finally, a new advanced CodeQL workflow is added for improved code scanning, and a redundant build workflow job is removed.Test Coverage Enhancements:
TEST_ENHANCEMENTS.md, detailing comprehensive improvements to test coverage and quality for key unit tests, including expanded test suites, edge case handling, integration testing, and improved test organization and documentation.Dependency Updates:
nextdependency to specific stable versions (primarily15.5.8or later) in multiplepackage.jsonfiles across apps, benchmarks, crates, and examples to ensure consistency and avoid issues with workspace references or outdated versions. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]CI/CD and Workflow Improvements:
.github/workflows/codeql.ymlfor advanced CodeQL static analysis, enabling scheduled and PR-based security/code scanning for multiple languages.buildPassedjob from.github/workflows/build_and_deploy.ymlto streamline the CI workflow and eliminate unnecessary steps.issues linked usingfixes #numberAdding a feature
fixes #numberFor Maintainers
What?
Why?
How?
Closes NEXT-
Fixes #
-->