Conversation
- 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
pp-2741 Next.js support updates: appId/base path handling, config cleanup, dependency upgrades, and security fixes.
# [0.14.0-beta.1](v0.13.2...v0.14.0-beta.1) (2026-02-20) ### Bug Fixes * ejs v4 default import for ESM compatibility ([e64f4c8](e64f4c8)) * improve dev server restart reliability and config change detection ([3edd020](3edd020)) ### Features * **cli:** appId support, base path handling ([63ec031](63ec031)) * **cli:** appId support, base path handling, API routes passthrough ([2c16dae](2c16dae))
sergak01
requested review from
MaksymovVolodymyr,
Sadilenko and
michailozdemir
February 20, 2026 17:26
📝 WalkthroughWalkthroughVersion bumped to 0.14.0-beta.1; dependency upgrades and an overrides entry added. Major internal changes to Next.js integration and basePath/assetPrefix handling, updated CLI runtime loading and appId resolution, proxy rewrite regex tweaked, EJS import style changed, and test apps extended with dataset fetches and pp-dev config tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser)
participant Proxy as Dev Proxy/Server
participant Next as Next.js handler
participant Config as PP-Dev Config Loader
participant Internal as Internal Route Handler
Note over Proxy,Config: Startup
Proxy->>Config: safeNextImport() / load pp-dev config (PHASE_DEVELOPMENT_SERVER)
Config-->>Proxy: mergedConfig (appId, basePath, assetPrefix)
Note over Client,Proxy: Request flow
Client->>Proxy: GET /p/test-nextjs/some/path
Proxy->>Proxy: match basePath? (using mergedConfig.basePath)
alt basePath matches
Proxy->>Next: forward full URL (do NOT strip basePath)
Next-->>Proxy: response (may redirect to add trailing slash)
Proxy-->>Client: response
else internal/api?
Proxy->>Internal: handle internal route (static api, proxy-ignore)
Internal-->>Proxy: internal response
Proxy-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Feb 20, 2026
Docstrings generation was requested by @sergak01. * #44 (comment) The following files were modified: * `src/cli.ts` * `src/index.ts` * `src/lib/proxy-pass.middleware.ts` * `src/plugins/client-injection-plugin.ts` * `tests/test-nextjs-cjs/src/api/dataset-data.ts` * `tests/test-nextjs-cjs/src/api/user.ts` * `tests/test-nextjs-cjs/src/pages/_document.tsx` * `tests/test-nextjs-cjs/src/pages/index.tsx` * `tests/test-nextjs/src/api/dataset-data.ts` * `tests/test-nextjs/src/api/user.ts` * `tests/test-nextjs/src/pages/_document.tsx` * `tests/test-nextjs/src/pages/index.tsx`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/client-injection-plugin.ts (1)
316-316:⚠️ Potential issue | 🟡 Minor
console.logincloseBundle()is a production artifactThis fires on every build and every hot reload cycle in watch mode. Use the existing structured
loggeror remove it.- console.log(`[pp-dev:client] Performance metrics:`, performanceMetrics); + // logger.info(`[pp-dev:client] Performance metrics: ${JSON.stringify(performanceMetrics)}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/client-injection-plugin.ts` at line 316, The console.log in closeBundle() that prints performanceMetrics is a production artifact; replace it with the plugin's structured logger (e.g., logger.debug or logger.info) so metrics use the existing logging system: locate the closeBundle() function and change the console.log(`[pp-dev:client] Performance metrics:`, performanceMetrics) call to logger.debug (or logger.info) with a clear message and the performanceMetrics object; if no logger is available in that scope, remove the console.log and either pass the existing logger into the plugin or omit the metric output entirely.
🧹 Nitpick comments (11)
tests/test-nextjs/src/api/dataset-data.ts (1)
2-4: Mixingawait+.then()is an anti-patternThe outer function is
async, so chaining.then()on the awaitedfetchresult adds unnecessary ceremony. The proposed fix above already resolves this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/src/api/dataset-data.ts` around lines 2 - 4, The code mixes await and .then on the fetch call: replace the fetch(...).then(...) chain with direct awaits — await the fetch response, then await res.json(), and return json.data; specifically, remove the .then(async (res) => (await res.json()).data) pattern and instead use: const res = await fetch(...); const json = await res.json(); return json.data; so the fetch call and res.json() are both awaited directly.tests/test-nextjs-cjs/next.config.js (1)
3-17: Remove the stale commented-out config block.This old config block is fully superseded by the active
withPPDev(...)call below. Keeping it adds noise. Consider removing it to keep the test config clean.🤖 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 3 - 17, Remove the stale commented-out Next.js config block that duplicates the active withPPDev(...) configuration; specifically delete the entire commented section containing the old nextConfig declaration and its properties so only the active withPPDev(...) call remains (look for the commented block starting with /** `@type` {import('next').NextConfig} */ and the const nextConfig = withPPDev(...) snippet).src/cli.ts (3)
830-837: Middleware arrays are declared after thecreateServercallback that references them.
fullMiddlewareChainandessentialMiddlewareChainare declared at lines 832-837, after thecreateServercallback (line 727) that reads them at line 735. This works becausehttpServer.listen()runs after setup, but the ordering makes the code harder to follow. Consider hoisting these declarations before thecreateServercall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 830 - 837, The middleware array declarations (fullMiddlewareChain and essentialMiddlewareChain) are defined after the createServer callback that references them, which hurts readability; move the declarations for fullMiddlewareChain and essentialMiddlewareChain (and mi if desired) above the createServer(...) callback so the callback body that reads them sees their definitions earlier, ensuring the httpServer.listen() sequence remains unchanged and no runtime behavior is altered.
553-558: FragileloadConfigresolution chain — consider adding a guard.The triple-fallback through
importConfig.default.default,importConfig['module.exports'].default, andimportConfig.defaultsilently picks the first truthy value. If all areundefined,loadConfigwill beundefinedand the call on line 583 will throw an unhelpful error. A guard after this resolution would improve debuggability.♻️ Suggested improvement
const loadConfig: typeof import('next/dist/server/config.js').default = (importConfig as any).default.default || (importConfig as any)['module.exports'].default || (importConfig as any).default; + + if (typeof loadConfig !== 'function') { + throw new Error( + `Failed to resolve Next.js loadConfig. Got: ${typeof loadConfig}. ` + + `Ensure your Next.js version is compatible.` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 553 - 558, The current resolution chain for loadConfig (using importConfig.default.default, importConfig['module.exports'].default, importConfig.default) can produce undefined and later cause an unhelpful runtime error; after resolving loadConfig, add a guard that verifies loadConfig is defined and throw a clear, descriptive error if not (include diagnostic info from importConfig such as Object.keys(importConfig) or the raw importConfig value) so callers of loadConfig (the subsequent call that currently fails) get an actionable message; optionally wrap the dynamic import/assignment in a try/catch to surface import errors with the same diagnostic detail.
649-661: Dead guard:templateNameis alwaysnullhere.
templateNameis initialized tonullon line 649, so theif (!templateName)check on line 651 is always true. If this was intended to allow a config-sourced template name to take priority, that logic is missing.♻️ Suggested simplification
- let templateName = null; - - if (!templateName) { - try { - const { getPkg } = await import('./config.js'); - const pkg = getPkg(); - - templateName = pkg.name; - } catch (error) { - // Fallback to project directory name - templateName = basename(projectRoot); - } + let templateName: string; + try { + const { getPkg } = await import('./config.js'); + const pkg = getPkg(); + templateName = pkg.name; + } catch (error) { + // Fallback to project directory name + templateName = basename(projectRoot); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 649 - 661, The guard around templateName is dead because templateName is set to null and then immediately tested; change the flow so you first attempt to read the config value and only fall back to the project directory name if that config read fails or returns a falsy name: remove the pre-initialization to null (or initialize to undefined), attempt to import and call getPkg() and, if pkg.name is a non-empty string, assign it to templateName, otherwise set templateName = basename(projectRoot); update references to templateName accordingly (symbols: templateName, getPkg, pkg.name, basename, projectRoot).tests/test-nextjs-cjs/src/pages/index.tsx (1)
21-27: Potential infinite fetch loop ifgetDatasetDataresolves to a falsy value.If the API returns
null,undefined, or any falsy value fordata, the!datasetDataguard stays true and the effect re-fires on every render. The existingusereffect (line 13-19) has the same pattern, but it's worth noting. Consider using a separateloadedflag or initializing state to a sentinel other thannull.♻️ Suggested fix
- const [datasetData, setDatasetData] = useState<any>(null); + const [datasetData, setDatasetData] = useState<{ data: any; loaded: boolean }>({ data: null, loaded: false }); useEffect(() => { - if (!datasetData) { + if (!datasetData.loaded) { getDatasetData(1).then((datasetData) => { - setDatasetData(datasetData); + setDatasetData({ data: datasetData, loaded: true }); }); } - }, [datasetData]); + }, [datasetData.loaded]);🤖 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 effect that fetches datasetData (useEffect observing datasetData which calls getDatasetData and setDatasetData) can loop if getDatasetData resolves to a falsy value; change the logic to use an explicit loaded flag or sentinel state instead of relying on the truthiness of datasetData—for example add a datasetLoaded boolean state (e.g., datasetLoaded, setDatasetLoaded) or initialize datasetData to a unique sentinel and update setDatasetData and setDatasetLoaded after the fetch; then change the useEffect to check the loaded flag (or sentinel equality) before fetching and set the loaded flag true after a fetch completes so the effect won’t re-run when the fetch result is falsy.src/index.ts (3)
256-264: Dev and production branches are identical — the conditional is dead code.Both the
isDevelopmentand the fallthrough branch callmergeConfigs(baseConfig, nextConfiguration)with the same arguments. Either collapse them or add the intended differentiation (e.g.,trailingSlashis already handled inbaseConfig).♻️ Suggested 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); + // 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);🤖 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 (isDevelopment) branch is redundant because both branches return mergeConfigs(baseConfig, nextConfiguration); remove the dead conditional and simply return mergeConfigs(baseConfig, nextConfiguration) once, or if different behavior was intended, implement the intended divergence inside the isDevelopment branch (e.g., modify baseConfig or nextConfiguration before calling mergeConfigs). Update the code around the isDevelopment check and the return using mergeConfigs(baseConfig, nextConfiguration) so only one path calls mergeConfigs or so the dev branch alters configs before merging.
180-186:mergeConfigsgivesnextConfigurationprecedence overbaseConfig— user config can silently override PP-Dev's computedbasePath/assetPrefix.
Object.assign({}, baseConfig, nextConfiguration, ...)means anybasePathorassetPrefixin the user's Next.js config will override the values computed bywithPPDev. If this is intentional (allowing user overrides), consider documenting it. If PP-Dev values should always win, reverse the merge order or strip those keys fromnextConfigurationbefore merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 180 - 186, mergeConfigs currently does Object.assign({}, baseConfig, nextConfiguration, ...) so user nextConfiguration can overwrite PP-Dev computed keys like basePath and assetPrefix; update mergeConfigs to ensure PP-Dev values win by removing basePath and assetPrefix from nextConfiguration (or alternatively reverse the merge order to apply baseConfig last) before calling Object.assign. Locate mergeConfigs and the nextConfiguration parameter and either strip nextConfiguration.basePath and nextConfiguration.assetPrefix (or clone and delete them) prior to the merge, or change the merge order so baseConfig is applied after nextConfiguration (ensuring withPPDev's computed values take precedence).
148-171: SimplifycreateBasePath—templateLess === truebranches are identical regardless ofv7Features.Both
v7Featuresbranches produce the same result whentemplateLessistrue(/p/{name}). The nested conditionals can be flattened.♻️ Suggested simplification
function createBasePath( templateName: string, templateLess: boolean, isDevelopment: boolean, v7Features: boolean, ): string { if (isDevelopment) { - if (v7Features) { - if (templateLess) { - return `${pathPagePrefix}/${templateName}`; - } else { - return `${pathTemplateLocalPrefix}/${templateName}`; - } - } else { - if (templateLess) { - return `${pathPagePrefix}/${templateName}`; - } else { - return `${pathTemplatePrefix}/${templateName}`; - } - } + if (templateLess) { + return `${pathPagePrefix}/${templateName}`; + } + return v7Features + ? `${pathTemplateLocalPrefix}/${templateName}` + : `${pathTemplatePrefix}/${templateName}`; } return `/p/${templateName}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 148 - 171, The createBasePath function duplicates the templateLess === true branch for both v7Features cases; simplify by handling templateLess early: if isDevelopment && templateLess return `${pathPagePrefix}/${templateName}`; then for isDevelopment handle the remaining v7Features case returning `${pathTemplateLocalPrefix}/${templateName}` when v7Features is true or `${pathTemplatePrefix}/${templateName}` when false; otherwise return `/p/${templateName}`. Update the function createBasePath and remove the nested duplicate templateLess branches (referencing pathPagePrefix, pathTemplateLocalPrefix, pathTemplatePrefix).tests/test-nextjs/package.json (1)
59-61: Same open-endedminimatchoverride as rootpackage.json.Consider using
"^10.2.1"here as well for consistency and safety (see comment on rootpackage.json).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/package.json` around lines 59 - 61, The overrides entry in tests/test-nextjs/package.json currently pins "minimatch" to ">=10.2.1"; update the overrides value for "minimatch" to use a caret range "^10.2.1" instead, mirroring the root package.json approach for consistency and safer semver updates—edit the "overrides" -> "minimatch" entry in that package.json to "^10.2.1".package.json (1)
68-71:minimatchoverride uses an open-ended lower bound — consider pinning to a major range.
">=10.2.1"will accept any future major version of minimatch (11.x, 12.x, …), which could introduce breaking changes in transitive consumers. A range like"^10.2.1"would be safer while still resolving the intended minimum.♻️ Suggested fix
"overrides": { "chokidar": "^4.0.3", - "minimatch": ">=10.2.1" + "minimatch": "^10.2.1" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 68 - 71, The package.json overrides object currently pins "minimatch" with an open-ended lower bound ">=10.2.1" which allows future major versions and may introduce breaking changes; update the overrides entry for minimatch to use a stable semver range such as "^10.2.1" (or another chosen major range) so transitive consumers are restricted to compatible releases, i.e., modify the "overrides" -> "minimatch" value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 1-13: The version header currently uses an h1 ("#
[0.14.0-beta.1]...") while its subsections use h3 ("### Bug Fixes" / "###
Features"), violating the heading hierarchy; fix by changing the version header
to h2 ("## [0.14.0-beta.1]...") so it matches the rest of the CHANGELOG
structure (or alternatively insert an h2-level wrapper above the existing
subsections), ensuring the "Bug Fixes" and "Features" remain as "###" under that
h2.
In `@src/cli.ts`:
- Around line 637-646: The current appId expression uses parseInt without a
radix and doesn't guard against NaN, so non-numeric env values can produce NaN
which defeats the nullish-coalescing fallback; update the parsing for MI_APP_ID
and MI_PORTAL_PAGE_ID to use parseInt with radix 10 (or Number.parseInt(...,
10)) and wrap the result with a NaN check (e.g., Number.isFinite or
!Number.isNaN) so that if parsing yields NaN you return undefined and let the ??
chain continue; apply this change where appId is computed referencing
originalAppId and portalPageId so invalid env values fall through to the next
source or the default 1.
- Around line 664-675: The basePath calculation ignores the v7Features flag
causing a mismatch; update the logic in the block that uses pathPagePrefix,
pathTemplatePrefix and configBasePath (and uses templateLess and v7Features) so
it picks '/pt' when templateLess is false and v7Features is false (or reuse the
existing createBasePath function from src/index.ts). Concretely, when
configBasePath is absent set base using templateLess and v7Features (mirror
createBasePath) rather than only templateLess so base becomes
'/p/{templateName}', '/pl/{templateName}' or '/pt/{templateName}' as
appropriate.
In `@tests/test-nextjs-cjs/package.json`:
- Line 25: The project uses "eslint-config-next": "13.5.11" which is
incompatible with the installed "next" (^16.1.6); update the eslint config to a
Next-major-compatible version by changing the "eslint-config-next" dependency in
package.json to a 16.x release (e.g., "^16.1.0" or the same exact major/minor as
"next"), then reinstall dependencies (npm/yarn install) and re-run lint to
ensure rules align with Next 16 features; reference the package.json entries for
"eslint-config-next" and "next" when making this change.
In `@tests/test-nextjs/next.config.mjs`:
- Around line 22-23: Remove the explicit assetPrefix and basePath properties
from the withPPDev() call so they don't override computed values; specifically,
delete the hardcoded assetPrefix and basePath entries in the next.config.mjs
test file to allow createBasePath() and the mergeConfigs flow (Object.assign({},
baseConfig, nextConfiguration, ...)) to supply the correct basePath/assetPrefix
for test-nextjs (see createBasePath in src/index.ts and withPPDev/mergeConfigs
behavior).
In `@tests/test-nextjs/src/api/dataset-data.ts`:
- Around line 1-5: The getDatasetData helper swallows HTTP errors because it
never checks res.ok; update the getDatasetData function to inspect the fetch
response (res.ok) and throw a descriptive error when false (including status and
response body or statusText) instead of blindly returning (await
res.json()).data; ensure callers get a rejected promise on non-2xx responses so
failures surface in tests.
In `@tests/test-nextjs/src/pages/index.tsx`:
- Around line 21-27: Change the effect checks from loose falsy checks to
explicit null checks (e.g., use datasetData === null and similarly for user) so
the effect only runs when data is actually absent; then handle promise
rejections from getDatasetData(1) by adding a .catch() (or convert the effect to
an async IIFE with try/catch) to log or set an error instead of leaving an
unhandled rejection; also ensure you defensively handle getDatasetData responses
(res.ok) where that function is implemented so failed HTTP responses are
surfaced to the caller.
---
Outside diff comments:
In `@src/plugins/client-injection-plugin.ts`:
- Line 316: The console.log in closeBundle() that prints performanceMetrics is a
production artifact; replace it with the plugin's structured logger (e.g.,
logger.debug or logger.info) so metrics use the existing logging system: locate
the closeBundle() function and change the console.log(`[pp-dev:client]
Performance metrics:`, performanceMetrics) call to logger.debug (or logger.info)
with a clear message and the performanceMetrics object; if no logger is
available in that scope, remove the console.log and either pass the existing
logger into the plugin or omit the metric output entirely.
---
Duplicate comments:
In `@tests/test-nextjs-cjs/src/api/dataset-data.ts`:
- Around line 1-5: The getDatasetData function is missing an HTTP error guard
and uses the await + .then() anti-pattern; replace the current fetch chain in
getDatasetData with a straightforward async/await flow: await the fetch, check
res.ok and throw a descriptive Error including status/statusText when not ok,
then await res.json() and return json.data; also consider moving getDatasetData
into a shared test utility so both test suites reuse the same function to
prevent duplication and drift.
---
Nitpick comments:
In `@package.json`:
- Around line 68-71: The package.json overrides object currently pins
"minimatch" with an open-ended lower bound ">=10.2.1" which allows future major
versions and may introduce breaking changes; update the overrides entry for
minimatch to use a stable semver range such as "^10.2.1" (or another chosen
major range) so transitive consumers are restricted to compatible releases,
i.e., modify the "overrides" -> "minimatch" value accordingly.
In `@src/cli.ts`:
- Around line 830-837: The middleware array declarations (fullMiddlewareChain
and essentialMiddlewareChain) are defined after the createServer callback that
references them, which hurts readability; move the declarations for
fullMiddlewareChain and essentialMiddlewareChain (and mi if desired) above the
createServer(...) callback so the callback body that reads them sees their
definitions earlier, ensuring the httpServer.listen() sequence remains unchanged
and no runtime behavior is altered.
- Around line 553-558: The current resolution chain for loadConfig (using
importConfig.default.default, importConfig['module.exports'].default,
importConfig.default) can produce undefined and later cause an unhelpful runtime
error; after resolving loadConfig, add a guard that verifies loadConfig is
defined and throw a clear, descriptive error if not (include diagnostic info
from importConfig such as Object.keys(importConfig) or the raw importConfig
value) so callers of loadConfig (the subsequent call that currently fails) get
an actionable message; optionally wrap the dynamic import/assignment in a
try/catch to surface import errors with the same diagnostic detail.
- Around line 649-661: The guard around templateName is dead because
templateName is set to null and then immediately tested; change the flow so you
first attempt to read the config value and only fall back to the project
directory name if that config read fails or returns a falsy name: remove the
pre-initialization to null (or initialize to undefined), attempt to import and
call getPkg() and, if pkg.name is a non-empty string, assign it to templateName,
otherwise set templateName = basename(projectRoot); update references to
templateName accordingly (symbols: templateName, getPkg, pkg.name, basename,
projectRoot).
In `@src/index.ts`:
- Around line 256-264: The if (isDevelopment) branch is redundant because both
branches return mergeConfigs(baseConfig, nextConfiguration); remove the dead
conditional and simply return mergeConfigs(baseConfig, nextConfiguration) once,
or if different behavior was intended, implement the intended divergence inside
the isDevelopment branch (e.g., modify baseConfig or nextConfiguration before
calling mergeConfigs). Update the code around the isDevelopment check and the
return using mergeConfigs(baseConfig, nextConfiguration) so only one path calls
mergeConfigs or so the dev branch alters configs before merging.
- Around line 180-186: mergeConfigs currently does Object.assign({}, baseConfig,
nextConfiguration, ...) so user nextConfiguration can overwrite PP-Dev computed
keys like basePath and assetPrefix; update mergeConfigs to ensure PP-Dev values
win by removing basePath and assetPrefix from nextConfiguration (or
alternatively reverse the merge order to apply baseConfig last) before calling
Object.assign. Locate mergeConfigs and the nextConfiguration parameter and
either strip nextConfiguration.basePath and nextConfiguration.assetPrefix (or
clone and delete them) prior to the merge, or change the merge order so
baseConfig is applied after nextConfiguration (ensuring withPPDev's computed
values take precedence).
- Around line 148-171: The createBasePath function duplicates the templateLess
=== true branch for both v7Features cases; simplify by handling templateLess
early: if isDevelopment && templateLess return
`${pathPagePrefix}/${templateName}`; then for isDevelopment handle the remaining
v7Features case returning `${pathTemplateLocalPrefix}/${templateName}` when
v7Features is true or `${pathTemplatePrefix}/${templateName}` when false;
otherwise return `/p/${templateName}`. Update the function createBasePath and
remove the nested duplicate templateLess branches (referencing pathPagePrefix,
pathTemplateLocalPrefix, pathTemplatePrefix).
In `@tests/test-nextjs-cjs/next.config.js`:
- Around line 3-17: Remove the stale commented-out Next.js config block that
duplicates the active withPPDev(...) configuration; specifically delete the
entire commented section containing the old nextConfig declaration and its
properties so only the active withPPDev(...) call remains (look for the
commented block starting with /** `@type` {import('next').NextConfig} */ and the
const nextConfig = withPPDev(...) snippet).
In `@tests/test-nextjs-cjs/src/pages/index.tsx`:
- Around line 21-27: The effect that fetches datasetData (useEffect observing
datasetData which calls getDatasetData and setDatasetData) can loop if
getDatasetData resolves to a falsy value; change the logic to use an explicit
loaded flag or sentinel state instead of relying on the truthiness of
datasetData—for example add a datasetLoaded boolean state (e.g., datasetLoaded,
setDatasetLoaded) or initialize datasetData to a unique sentinel and update
setDatasetData and setDatasetLoaded after the fetch; then change the useEffect
to check the loaded flag (or sentinel equality) before fetching and set the
loaded flag true after a fetch completes so the effect won’t re-run when the
fetch result is falsy.
In `@tests/test-nextjs/package.json`:
- Around line 59-61: The overrides entry in tests/test-nextjs/package.json
currently pins "minimatch" to ">=10.2.1"; update the overrides value for
"minimatch" to use a caret range "^10.2.1" instead, mirroring the root
package.json approach for consistency and safer semver updates—edit the
"overrides" -> "minimatch" entry in that package.json to "^10.2.1".
In `@tests/test-nextjs/src/api/dataset-data.ts`:
- Around line 2-4: The code mixes await and .then on the fetch call: replace the
fetch(...).then(...) chain with direct awaits — await the fetch response, then
await res.json(), and return json.data; specifically, remove the .then(async
(res) => (await res.json()).data) pattern and instead use: const res = await
fetch(...); const json = await res.json(); return json.data; so the fetch call
and res.json() are both awaited directly.
- Update cli.ts (restart/config changes from previous work) - Update test-nextjs-cjs package.json with overrides - Update package-lock.json files
chore: update test fixtures and dependencies
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli.ts (2)
1105-1141:⚠️ Potential issue | 🟠 MajorSignal handlers accumulate on every restart — listener leak and cross-restart closure hazard.
processObj.on('SIGINT/SIGTERM/...')inside thehttpServer.listen()callback (Lines 1125–1141) is re-registered on every call tostartNextServer. Old registrations are never removed, so after N restarts there are N inner handler registrations plus the single outer registration at Lines 1239–1251. Each stale inner handler still holds a closure over the outerhttpServervariable; after a restart that variable points to the new server, meaning old handlers can race to close the current server.There are also two independent
gracefulShutdownimplementations (Lines 1051 and 1203) registered for the same signals — a design inconsistency on top of the listener leak.Fix: Remove the inner signal-handler block from the
listen()callback entirely. The outer handlers at Lines 1239–1251 already cover process signals; they just need access tohttpServerandnextApp(which they get via closure). If restart-specific cleanup is needed, remove old listeners withprocess.removeListenerbefore adding new ones.🐛 Proposed approach
- if (typeof processObj.on === 'function') { - try { - processObj.on('SIGINT', () => gracefulShutdown('SIGINT')); - processObj.on('SIGTERM', () => gracefulShutdown('SIGTERM')); - processObj.on('uncaughtException', (error) => { ... }); - processObj.on('unhandledRejection', (reason, promise) => { ... }); - ... - } catch (error) { ... } - } else { ... } + // Signal handling is centralized in the outer handlers below (lines 1239-1251). + // No per-restart registration needed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1105 - 1141, The signal-handler registrations inside the startNextServer/httpServer.listen callback (the block that uses processObj.on for 'SIGINT','SIGTERM','uncaughtException','unhandledRejection') must be removed to prevent accumulating listeners and stale closures over httpServer/nextApp; instead rely on the existing outer/global handlers that call gracefulShutdown (the outer handlers around Lines 1239–1251) or, if you must re-register per-restart, first remove old listeners via process.removeListener for the specific callbacks before adding new ones. Locate the inner block by the use of processObj.on and gracefulShutdown within the listen callback and delete that block (or replace with a no-op), ensuring only the single canonical gracefulShutdown implementation (not duplicate ones) is kept and that the outer handlers have access to the current httpServer and nextApp via their closure.
1183-1196:⚠️ Potential issue | 🟡 MinorInconsistent peer-dependency version in error message (
^16vs^15insafeNextImport).The error recovery message instructs users to
npm install next@^16, butsrc/lib/next-import.ts(thesafeNextImportfunction) states"npm install next@^15", and the project itself is pinned to Next.js 15.5.12 inlibrary_context. Next.js 16 was released in October 2025 and introduces breaking changes, so the peer-dependency range should be deliberate and consistent across both locations.- logger.error(colors.white(' npm install next@^16')); + logger.error(colors.white(' npm install next@^15'));Apply the same correction to the
yarn addandpnpm addlines below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1183 - 1196, The error message in the Next.js peer-dependency recovery block should use the same peer range as safeNextImport and the pinned project version (Next.js 15.x); update the three occurrences of "^16" in the CLI's logger.error lines to "^15" so the npm, yarn and pnpm install suggestions match the safeNextImport guidance (see safeNextImport in src/lib/next-import.ts and the pinned 15.5.12 in library_context) to ensure consistent, correct instructions to users.
🧹 Nitpick comments (3)
src/cli.ts (3)
1106-1121: Dead code:process.onavailability check is neverfalsein Node.js.
typeof process.on !== 'function'is alwaysfalsein a Node.js CLI process, making theglobalThis.processfallback and the surroundingif/elseunreachable. The entire block can be removed.- // Handle process signals - try to use process.on if available - let processObj = process; - - // If local process.on is not available, try global process - if (typeof process.on !== 'function') { - const globalProcess = - (globalThis as any).process || (global as any).process; - - if (globalProcess && typeof globalProcess.on === 'function') { - processObj = globalProcess; - logger.info( - colors.green( - '✅ Using global process object for event handlers', - ), - ); - } - } - - if (typeof processObj.on === 'function') { - try { + try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1106 - 1121, The fallback branch checking typeof process.on !== 'function' is unreachable in a Node.js CLI; remove the surrounding conditional and fallback logic and use the native process object directly. Specifically, eliminate the code that declares processObj and the entire block that references process.on, globalThis.process / global.process, and the logger.info/colors.green message, and update any remaining references to use process (or an existing processObj assigned to process) so event handlers attach to the built-in process without the dead-code fallback.
580-580: Unlabelled debug log.
logger.info(projectRoot)emits the raw path with no context label, making it noisy and indistinguishable from other output. Either remove it or add a descriptive prefix.- logger.info(projectRoot); + logger.info(colors.blue(`🔧 Project root: ${projectRoot}`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` at line 580, The log call logger.info(projectRoot) emits an unlabeled raw path; update the call in src/cli.ts (the logger.info usage) to either remove this debug-only line or replace it with a descriptive message such as logger.info('projectRoot: %s', projectRoot) (or equivalent in your logger) so the path is emitted with context; ensure you only keep it if it's intended for runtime info and not transient debug output.
649-661: Dead conditional ontemplateNameand missing fallback forundefinedpkg.name.
templateNameis alwaysnullat Line 649, so theif (!templateName)guard on Line 651 always evaluates totrueand can be removed. Additionally, ifgetPkg()succeeds but thepackage.jsonhas nonamefield,templateNameis silently set toundefined, which later produces abaseof/p/undefined.♻️ Proposed fix
-let templateName = null; - -if (!templateName) { - try { - const { getPkg } = await import('./config.js'); - const pkg = getPkg(); - - templateName = pkg.name; - } catch (error) { - // Fallback to project directory name - templateName = basename(projectRoot); - } -} +let templateName: string; +try { + const { getPkg } = await import('./config.js'); + const pkg = getPkg(); + templateName = pkg?.name || basename(projectRoot); +} catch (error) { + templateName = basename(projectRoot); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 649 - 661, Remove the dead conditional around templateName (it's initialized to null so if (!templateName) is always true) and instead directly try to load the package info via getPkg(), then ensure you handle a missing or falsy pkg.name by falling back to basename(projectRoot); specifically, in the block using getPkg()/pkg, set templateName = pkg && pkg.name ? pkg.name : basename(projectRoot) (or equivalent) so you never assign undefined and avoid generating paths like /p/undefined; keep the basename(projectRoot) fallback inside the existing try/catch logic and only use the catch branch for import/load errors.
🤖 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 553-558: The fallback chain assigning loadConfig from
import('next/dist/server/config.js') is unsafe because (importConfig as
any)['module.exports'] can be undefined and accessing .default will throw;
update the expression that computes loadConfig to use optional chaining when
accessing module.exports (i.e., check (importConfig as
any)['module.exports']?.default) so the third fallback (importConfig.default)
can run, and add a short comment near the importConfig/loadConfig code noting
this relies on an internal Next.js path and may break across Next.js releases so
it should be revisited if you upgrade Next.js (referencing the importConfig
variable and the loadConfig constant to locate the change).
- Line 689: The assetPrefix assignment is out of sync with basePath for the
v7Features/templateLess/configBasePath cases; update assetPrefix to mirror the
same logic as basePath: if configBasePath is provided use that, otherwise if
templateLess use pathPagePrefix, else when v7Features is true use
pathLayoutPrefix, else use pathPagePrefix, ensuring assetPrefix uses
templateName just like basePath and referencing the same variables (assetPrefix,
basePath, templateLess, v7Features, configBasePath, templateName,
pathPagePrefix, pathLayoutPrefix).
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 1105-1141: The signal-handler registrations inside the
startNextServer/httpServer.listen callback (the block that uses processObj.on
for 'SIGINT','SIGTERM','uncaughtException','unhandledRejection') must be removed
to prevent accumulating listeners and stale closures over httpServer/nextApp;
instead rely on the existing outer/global handlers that call gracefulShutdown
(the outer handlers around Lines 1239–1251) or, if you must re-register
per-restart, first remove old listeners via process.removeListener for the
specific callbacks before adding new ones. Locate the inner block by the use of
processObj.on and gracefulShutdown within the listen callback and delete that
block (or replace with a no-op), ensuring only the single canonical
gracefulShutdown implementation (not duplicate ones) is kept and that the outer
handlers have access to the current httpServer and nextApp via their closure.
- Around line 1183-1196: The error message in the Next.js peer-dependency
recovery block should use the same peer range as safeNextImport and the pinned
project version (Next.js 15.x); update the three occurrences of "^16" in the
CLI's logger.error lines to "^15" so the npm, yarn and pnpm install suggestions
match the safeNextImport guidance (see safeNextImport in src/lib/next-import.ts
and the pinned 15.5.12 in library_context) to ensure consistent, correct
instructions to users.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 1106-1121: The fallback branch checking typeof process.on !==
'function' is unreachable in a Node.js CLI; remove the surrounding conditional
and fallback logic and use the native process object directly. Specifically,
eliminate the code that declares processObj and the entire block that references
process.on, globalThis.process / global.process, and the
logger.info/colors.green message, and update any remaining references to use
process (or an existing processObj assigned to process) so event handlers attach
to the built-in process without the dead-code fallback.
- Line 580: The log call logger.info(projectRoot) emits an unlabeled raw path;
update the call in src/cli.ts (the logger.info usage) to either remove this
debug-only line or replace it with a descriptive message such as
logger.info('projectRoot: %s', projectRoot) (or equivalent in your logger) so
the path is emitted with context; ensure you only keep it if it's intended for
runtime info and not transient debug output.
- Around line 649-661: Remove the dead conditional around templateName (it's
initialized to null so if (!templateName) is always true) and instead directly
try to load the package info via getPkg(), then ensure you handle a missing or
falsy pkg.name by falling back to basename(projectRoot); specifically, in the
block using getPkg()/pkg, set templateName = pkg && pkg.name ? pkg.name :
basename(projectRoot) (or equivalent) so you never assign undefined and avoid
generating paths like /p/undefined; keep the basename(projectRoot) fallback
inside the existing try/catch logic and only use the catch branch for
import/load errors.
MaksymovVolodymyr
approved these changes
Feb 23, 2026
This was referenced Feb 27, 2026
This was referenced Jun 30, 2026
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
3 participants
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.
Features & Improvements
CLI & Dev Server
isRestartingis always reset (Next.js and Vite)clearProjectConfigCache()to clear Node module cache fornext.config,vite.config, andpp-dev.configon restarthttpServererror handler for listen failures (e.g. EADDRINUSE)Configuration
rewritePathbehaviorBuild & Dependencies
externalLiveBindingsTest Fixtures
withPPDev, addbasePath/assetPrefix, add dataset-data API,suppressHydrationWarning