Merged
Conversation
|
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 Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test-nextjs/src/pages/_document.tsx (1)
12-15: Avoid applyingsuppressHydrationWarningto<body>; target the narrowest element with an actual mismatch.Applying it at
<body>suppresses warnings for the entire body subtree, masking hydration issues elsewhere on the page. Per Next.js best practices, reserve this escape hatch for specific elements where a mismatch is unavoidable. If the issue is the afterInteractive script injectingwindow.PP_VARIABLES, either fix the mismatch at its source (move logic that depends on it into a client-side effect) or suppress only the element directly affected by it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/src/pages/_document.tsx` around lines 12 - 15, Remove suppressHydrationWarning from the <body> element in the custom Document and either move the code that depends on window.PP_VARIABLES into a client-side effect (so it no longer causes SSR/client markup mismatch) or apply suppressHydrationWarning to the smallest affected element (the specific component that reads/writes window.PP_VARIABLES) instead; update the _document.tsx markup where Html, Head, Main and body are defined to no longer set suppressHydrationWarning on body and locate the mismatch to fix or narrowly wrap that specific component.tests/test-nextjs-cjs/src/pages/_document.tsx (1)
12-15: Consider narrowing suppressHydrationWarning to the smallest affected subtree.Placing it on
<body>suppresses warnings from its direct children and prevents debugging of real hydration mismatches deeper in the page. Reserve it for the specific element that actually has an unavoidable mismatch (e.g., a timestamp, locale-dependent content, or theme class/attribute intentionally set on the client), or add a comment explaining what the suppression is for. This keeps other hydration problems visible and follows React's guidance to use suppressHydrationWarning as a narrow escape hatch, not a broad toggle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs-cjs/src/pages/_document.tsx` around lines 12 - 15, The suppressHydrationWarning attribute is applied to <body>, which is too broad; move it from the <body> element to the smallest specific element that actually has the unavoidable hydration mismatch (for example inside the component rendered by <Main /> or a specific DOM node that renders timestamps/theme/class differences), or if the mismatch truly spans the whole body, add an inline comment next to suppressHydrationWarning explaining the exact reason and which dynamic content causes it; locate the attribute on the <body> in _document.tsx and either remove it and add it to the precise subtree that mismatches (the element rendered by Main or a named component) or keep it but document the justification in a comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 506-516: The await of safeNextImport currently runs before the
logger exists and outside any try/catch, so missing Next.js causes an unhandled
rejection and skips your custom messaging; move the safeNextImport call to occur
after createLogger() (or perform it inside startNextServer) and wrap it in a
try/catch that uses the created logger to call the same error/usage guidance
from the next-import helper; specifically update the block that calls
safeNextImport and/or the startNextServer flow so that safeNextImport is invoked
under the logger context and failures are caught and logged (referencing
safeNextImport, startNextServer, and createLogger to locate the changes).
---
Nitpick comments:
In `@tests/test-nextjs-cjs/src/pages/_document.tsx`:
- Around line 12-15: The suppressHydrationWarning attribute is applied to
<body>, which is too broad; move it from the <body> element to the smallest
specific element that actually has the unavoidable hydration mismatch (for
example inside the component rendered by <Main /> or a specific DOM node that
renders timestamps/theme/class differences), or if the mismatch truly spans the
whole body, add an inline comment next to suppressHydrationWarning explaining
the exact reason and which dynamic content causes it; locate the attribute on
the <body> in _document.tsx and either remove it and add it to the precise
subtree that mismatches (the element rendered by Main or a named component) or
keep it but document the justification in a comment.
In `@tests/test-nextjs/src/pages/_document.tsx`:
- Around line 12-15: Remove suppressHydrationWarning from the <body> element in
the custom Document and either move the code that depends on window.PP_VARIABLES
into a client-side effect (so it no longer causes SSR/client markup mismatch) or
apply suppressHydrationWarning to the smallest affected element (the specific
component that reads/writes window.PP_VARIABLES) instead; update the
_document.tsx markup where Html, Head, Main and body are defined to no longer
set suppressHydrationWarning on body and locate the mismatch to fix or narrowly
wrap that specific component.
- Next.js: add try/finally to ensure isRestarting is always reset in listen callback - Next.js: add httpServer error handler for listen failures (e.g. EADDRINUSE) - Vite: add finally block to always reset isRestarting on success or failure - Add clearProjectConfigCache() to clear Node module cache for config files on restart - Ensures next.config, vite.config, and pp-dev.config changes are detected on restart
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
1183-1187:⚠️ Potential issue | 🟡 MinorIncorrect Next.js version in installation instructions.
The error message suggests installing
next@^16, but Next.js 16 does not exist. This should match the peer dependency version specified in package.json (>= 13 < 17).🐛 Proposed fix
- logger.error(colors.white(' npm install next@^16')); + logger.error(colors.white(' npm install next@^15')); logger.error(colors.blue(' 2. Or use yarn:')); - logger.error(colors.white(' yarn add next@^16')); + logger.error(colors.white(' yarn add next@^15')); logger.error(colors.blue(' 3. Or use pnpm:')); - logger.error(colors.white(' pnpm add next@^16')); + logger.error(colors.white(' pnpm add next@^15'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1183 - 1187, The installation instructions print the incorrect Next.js version (next@^16); update the three logger.error messages that show 'next@^16' in src/cli.ts to reflect the supported peer range—use a correct suggestion such as 'next@^13' (or explicitly 'next@>=13 <17') for npm/yarn/pnpm so the displayed install commands match the package.json peer dependency; locate the logger.error calls that output the install commands and change their version text accordingly.
🧹 Nitpick comments (8)
tests/test-nextjs-cjs/src/pages/_document.tsx (1)
10-26: Consider scopingsuppressHydrationWarningto the known mismatching subtree.Setting it on
<body>hides all hydration mismatches; if possible, limit to the element that intentionally diverges to keep warnings actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs-cjs/src/pages/_document.tsx` around lines 10 - 26, The suppressHydrationWarning currently applied to the <body> in the Document component is too broad; narrow it to the specific subtree that intentionally mismatches (the Script/pp-variables output). Update the Document function to remove suppressHydrationWarning from <body> and instead add it to the smallest enclosing element around the mismatched content (e.g., the element wrapping the inline Script or the Script container you render with id 'pp-variables'), so only that element suppresses hydration warnings while the rest of the page still surfaces mismatches.src/lib/proxy-pass.middleware.ts (1)
39-41: Add a quick regression test for the new default rewritePath.This guards the intended exclusions for
/pt*and/pl*(and ensures other paths still proxy).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-pass.middleware.ts` around lines 39 - 41, Add a small regression test that asserts the default rewritePath regex in initProxy (the rewritePath default in the initProxy function using ProxyOpts) excludes paths starting with /pt and /pl while allowing other paths to match; specifically, create test cases for e.g. "/pt123" and "/pl/abc" which should not match, and for "/foo", "/pother" or "/ptx" variants that should match or not according to intended behavior, using the same regex extracted from initProxy (rewritePath) to ensure future changes don't regress the exclusion semantics.tests/test-nextjs-cjs/next.config.js (1)
18-28: Document why basePath/assetPrefix are disabled in the CJS variant, or restore parity with the ESM config.The ESM config (
tests/test-nextjs/next.config.mjs:22-23) hasassetPrefixandbasePathenabled, but the CJS variant has them commented out. Either uncomment them to maintain test coverage parity, or add a note explaining why the CJS variant intentionally skips these properties.📝 Optional clarification
const nextConfig = withPPDev({ output: 'export', cleanDistDir: true, reactStrictMode: true, distDir: 'dist', images: { unoptimized: true, }, + // NOTE: basePath/assetPrefix intentionally disabled in CJS variant (root-based tests). // assetPrefix: '/pt/test-nextjs-cjs', // basePath: '/pl/test-nextjs-cjs', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs-cjs/next.config.js` around lines 18 - 28, The CJS nextConfig created via withPPDev currently has assetPrefix and basePath commented out (see nextConfig, assetPrefix, basePath), causing divergence from the ESM config; either restore parity by uncommenting and matching the ESM values for assetPrefix and basePath in nextConfig (so tests cover both variants), or add a concise inline comment above the commented lines explaining why the CJS variant intentionally omits them (e.g., known CJS-specific limitation or test constraint) and reference withPPDev and nextConfig so reviewers can verify the rationale.tests/test-nextjs/src/pages/index.tsx (1)
21-27: Add optional cleanup and error handling for the async effect.This prevents setState on unmounted components and avoids unhandled rejections.
♻️ Suggested improvement
useEffect(() => { + let cancelled = false; if (!datasetData) { - getDatasetData(1).then((datasetData) => { - setDatasetData(datasetData); - }); + getDatasetData(1) + .then((data) => { + if (!cancelled) setDatasetData(data); + }) + .catch((err) => { + console.error('Failed to load dataset data', err); + }); } -}, [datasetData]); + return () => { + cancelled = true; + }; +}, [datasetData]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/src/pages/index.tsx` around lines 21 - 27, The effect using getDatasetData in useEffect can cause setDatasetData on an unmounted component and leave unhandled promise rejections; modify the effect in pages/index.tsx to track mounted state (e.g., let isMounted = true) or use an AbortController, call getDatasetData(1).then(...) and in the then handler only call setDatasetData if isMounted, add a .catch to handle and log errors, and return a cleanup function that flips isMounted = false (or aborts the request) to prevent state updates after unmount; reference the useEffect block, getDatasetData, and setDatasetData when making the change.tests/test-nextjs-cjs/src/pages/index.tsx (1)
21-27: Add cleanup and error handling to the async effect.This prevents setState on unmounted components and avoids unhandled rejections.
♻️ Suggested implementation
useEffect(() => { + let cancelled = false; if (!datasetData) { - getDatasetData(1).then((datasetData) => { - setDatasetData(datasetData); - }); + getDatasetData(1) + .then((data) => { + if (!cancelled) setDatasetData(data); + }) + .catch((err) => { + console.error('Failed to load dataset data', err); + }); } -}, [datasetData]); + return () => { + cancelled = true; + }; +}, [datasetData]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs-cjs/src/pages/index.tsx` around lines 21 - 27, The useEffect that calls getDatasetData(1) should add error handling and a cleanup to avoid setting state on an unmounted component and unhandled promise rejections: wrap the async call in a try/catch (or chain .catch) to log/handle errors, track mounted state with a boolean flag (e.g., isMounted) or use an AbortController to cancel the request, and only call setDatasetData when still mounted; ensure the effect returns a cleanup function that flips the flag or aborts the controller to prevent setState after unmount. Reference: useEffect, getDatasetData, setDatasetData.tests/test-nextjs-cjs/src/api/dataset-data.ts (1)
1-4: Add error handling and prepare for potential basePath usage (optional refactor for robustness).Both the CJS and ESM variants currently lack basic error handling and basePath-awareness. While basePath is currently disabled, the config shows it's a known option that could be enabled later. Adding these improvements makes the helper more robust.
✅ Suggested fix
-export async function getDatasetData(datasetId: number) { - return await fetch(`/api/dataset_data?dataset=${datasetId}`, { - headers: { accept: 'application/json' }, - }).then(async (res) => (await res.json()).data); -} +export async function getDatasetData(datasetId: number, basePath = '') { + const prefix = basePath.replace(/\/$/, ''); + const res = await fetch( + `${prefix}/api/dataset_data?dataset=${encodeURIComponent(datasetId)}`, + { headers: { accept: 'application/json' } }, + ); + if (!res.ok) { + throw new Error(`Failed to fetch dataset data: ${res.status}`); + } + return (await res.json()).data; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs-cjs/src/api/dataset-data.ts` around lines 1 - 4, Update getDatasetData to handle fetch/network/JSON errors and to respect an optional basePath config; validate the response status before parsing and throw or return a controlled error when fetch fails, response.ok is false, or JSON parsing doesn't yield expected .data. Locate the getDatasetData function and add try/catch around the fetch and res.json calls, include status-based error handling for non-OK responses, and build the request URL using a configurable basePath (fallback to '' when not set) so the helper works if basePath is later enabled.src/index.ts (1)
256-264: Redundant production branch.The development and production branches return identical results (both call
mergeConfigs(baseConfig, nextConfiguration)). The conditional structure can be simplified.♻️ Proposed simplification
if (isDevelopment) { // Merge base config with user's Next.js config. // PP-Dev config is NOT added to Next.js config (avoids "Unrecognized key" warnings). // CLI and app get config from getConfig() / pp-dev.config.js instead. - return mergeConfigs(baseConfig, nextConfiguration); } - // Production configuration return mergeConfigs(baseConfig, nextConfiguration);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 256 - 264, The if/else produces identical results; simplify by removing the redundant production branch and always returning mergeConfigs(baseConfig, nextConfiguration). Update the logic around the isDevelopment check so any development-only comments or side-effects remain but the function simply calls and returns mergeConfigs(baseConfig, nextConfiguration) (refer to symbols isDevelopment, mergeConfigs, baseConfig, nextConfiguration) to avoid duplicate return paths.tests/test-nextjs/package.json (1)
33-33: Duplicaterollupdependency.
rollupis listed in bothdependencies(line 33) anddevDependencies(line 54) with the same version^4.58.0. This is redundant and can cause confusion. For a test project, it should likely only be indevDependencies.♻️ Proposed fix - remove from dependencies
"react-dom": "^18.3.1", - "rollup": "^4.58.0", "sass": "^1.97.3",Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/package.json` at line 33, The package.json contains a duplicate "rollup" entry in both "dependencies" and "devDependencies"; remove the "rollup": "^4.58.0" line from the dependencies section and keep the single entry in devDependencies so tooling is only declared as a dev-time dependency (update package.json by editing the dependencies object to delete the "rollup" key).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 555-558: The current dynamic resolution for loadConfig from
importConfig can yield undefined and crash later; after attempting the fallback
chain on importConfig (referencing importConfig and loadConfig), validate that
loadConfig is a function/value you expect and throw a clear, early Error if none
of the export shapes matched; alternatively adjust the fallback order to check
(importConfig as any).default, (importConfig as any)['module.exports'],
(importConfig as any).default?.default, and finally importConfig itself, then
assert typeof loadConfig !== 'undefined' (or typeof loadConfig === 'function' if
it should be callable) and throw an informative error that includes the
importConfig export keys to aid debugging.
In `@src/index.ts`:
- Around line 154-171: The production branch currently returns a hardcoded
`/p/${templateName}` which ignores the templateLess and v7Features flags; update
the basePath logic in the same function in src/index.ts to mirror the
development branch: compute and return the path using templateLess, v7Features
and the same prefixes (pathPagePrefix, pathTemplateLocalPrefix,
pathTemplatePrefix) instead of hardcoding `/p/...`. Also ensure assetPrefix is
computed consistently with that same logic (so when v7Features && !templateLess
you use pathTemplateLocalPrefix or the correct prefix instead of always using
pathTemplatePrefix) so basePath and assetPrefix remain aligned.
In `@tests/test-nextjs/src/api/dataset-data.ts`:
- Around line 1-4: The fetch helpers getDatasetData(datasetId) and
getCurrentUser currently assume successful responses; update both to validate
responses by checking res.ok after fetch and throw a descriptive error if not OK
(include status and statusText), and ensure callers handle network errors by
adding .catch or try/catch around calls; specifically modify getDatasetData and
getCurrentUser to await fetch, inspect response.ok, parse JSON only on success,
and propagate a clear Error when res.ok is false so call sites can handle it
cleanly.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 1183-1187: The installation instructions print the incorrect
Next.js version (next@^16); update the three logger.error messages that show
'next@^16' in src/cli.ts to reflect the supported peer range—use a correct
suggestion such as 'next@^13' (or explicitly 'next@>=13 <17') for npm/yarn/pnpm
so the displayed install commands match the package.json peer dependency; locate
the logger.error calls that output the install commands and change their version
text accordingly.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 256-264: The if/else produces identical results; simplify by
removing the redundant production branch and always returning
mergeConfigs(baseConfig, nextConfiguration). Update the logic around the
isDevelopment check so any development-only comments or side-effects remain but
the function simply calls and returns mergeConfigs(baseConfig,
nextConfiguration) (refer to symbols isDevelopment, mergeConfigs, baseConfig,
nextConfiguration) to avoid duplicate return paths.
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 39-41: Add a small regression test that asserts the default
rewritePath regex in initProxy (the rewritePath default in the initProxy
function using ProxyOpts) excludes paths starting with /pt and /pl while
allowing other paths to match; specifically, create test cases for e.g. "/pt123"
and "/pl/abc" which should not match, and for "/foo", "/pother" or "/ptx"
variants that should match or not according to intended behavior, using the same
regex extracted from initProxy (rewritePath) to ensure future changes don't
regress the exclusion semantics.
In `@tests/test-nextjs-cjs/next.config.js`:
- Around line 18-28: The CJS nextConfig created via withPPDev currently has
assetPrefix and basePath commented out (see nextConfig, assetPrefix, basePath),
causing divergence from the ESM config; either restore parity by uncommenting
and matching the ESM values for assetPrefix and basePath in nextConfig (so tests
cover both variants), or add a concise inline comment above the commented lines
explaining why the CJS variant intentionally omits them (e.g., known
CJS-specific limitation or test constraint) and reference withPPDev and
nextConfig so reviewers can verify the rationale.
In `@tests/test-nextjs-cjs/src/api/dataset-data.ts`:
- Around line 1-4: Update getDatasetData to handle fetch/network/JSON errors and
to respect an optional basePath config; validate the response status before
parsing and throw or return a controlled error when fetch fails, response.ok is
false, or JSON parsing doesn't yield expected .data. Locate the getDatasetData
function and add try/catch around the fetch and res.json calls, include
status-based error handling for non-OK responses, and build the request URL
using a configurable basePath (fallback to '' when not set) so the helper works
if basePath is later enabled.
In `@tests/test-nextjs-cjs/src/pages/_document.tsx`:
- Around line 10-26: The suppressHydrationWarning currently applied to the
<body> in the Document component is too broad; narrow it to the specific subtree
that intentionally mismatches (the Script/pp-variables output). Update the
Document function to remove suppressHydrationWarning from <body> and instead add
it to the smallest enclosing element around the mismatched content (e.g., the
element wrapping the inline Script or the Script container you render with id
'pp-variables'), so only that element suppresses hydration warnings while the
rest of the page still surfaces mismatches.
In `@tests/test-nextjs-cjs/src/pages/index.tsx`:
- Around line 21-27: The useEffect that calls getDatasetData(1) should add error
handling and a cleanup to avoid setting state on an unmounted component and
unhandled promise rejections: wrap the async call in a try/catch (or chain
.catch) to log/handle errors, track mounted state with a boolean flag (e.g.,
isMounted) or use an AbortController to cancel the request, and only call
setDatasetData when still mounted; ensure the effect returns a cleanup function
that flips the flag or aborts the controller to prevent setState after unmount.
Reference: useEffect, getDatasetData, setDatasetData.
In `@tests/test-nextjs/package.json`:
- Line 33: The package.json contains a duplicate "rollup" entry in both
"dependencies" and "devDependencies"; remove the "rollup": "^4.58.0" line from
the dependencies section and keep the single entry in devDependencies so tooling
is only declared as a dev-time dependency (update package.json by editing the
dependencies object to delete the "rollup" key).
In `@tests/test-nextjs/src/pages/index.tsx`:
- Around line 21-27: The effect using getDatasetData in useEffect can cause
setDatasetData on an unmounted component and leave unhandled promise rejections;
modify the effect in pages/index.tsx to track mounted state (e.g., let isMounted
= true) or use an AbortController, call getDatasetData(1).then(...) and in the
then handler only call setDatasetData if isMounted, add a .catch to handle and
log errors, and return a cleanup function that flips isMounted = false (or
aborts the request) to prevent state updates after unmount; reference the
useEffect block, getDatasetData, and setDatasetData when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Core
appIdconfig option with fallback fromportalPageIdand env vars (MI_APP_ID,MI_PORTAL_PAGE_ID)basePathin Next.js configppDevonly (noexperimental.ppDev) to avoid Next.js validation warningsrewritePathfor/ptand/plprefixes; API routes passthrough for Next.jsimport ejs from 'ejs')Build
minimatchoverride (>=10.2.1) to fix ReDoS vulnerabilitiesexternalLiveBindings: falsefor Node built-insTests
withPPDev, addedbasePath/assetPrefix, dataset-data API,suppressHydrationWarningFiles Changed
25 files changed, ~4.9k insertions, ~5.8k deletions.
Testing
npm run devworks in test-nextjs-cjsnpm auditreports 0 vulnerabilities in rootSummary by CodeRabbit
New Features
Bug Fixes
Chores