Conversation
Update root and test lockfiles to pull non-vulnerable axios, follow-redirects, and next versions so repo-wide audits pass.
Improve stdin shortcut lifecycle cleanup to avoid stale bindings and preserve terminal state across server restarts. Add trace hooks to the dts rollup build to pinpoint where type generation can stall.
Update the CommonJS test fixture lockfile to the latest local pp-dev package and aligned dependency metadata so fixture installs stay reproducible.
PP-3311 pp-dev: dist/client/proxy refactor, tooling, and test alignment
## [0.18.1-beta.1](v0.18.0...v0.18.1-beta.1) (2026-04-21) ### Bug Fixes * **cli:** harden shortcut cleanup and add dts trace logging ([4de05ed](4de05ed))
📝 WalkthroughWalkthroughThis PR adds a browser confirmation modal and bidirectional sync-action flow, introduces ZIP backup analysis and BUILD-MANIFEST fingerprinting, centralizes non-JSON API error handling, extends version/manifest metadata, and applies many formatting/structural edits across build, config, client, service, middleware, and test files. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ClientApp
participant DevServer
participant DistService
Browser->>ClientApp: websocket 'template:sync:action-required' (requestId, payload)
ClientApp->>Browser: open confirmModal() (user prompt)
alt user approves
Browser->>ClientApp: modal resolves approved=true
ClientApp->>DevServer: websocket 'template:sync:action-response' {requestId, approved:true}
DevServer->>DistService: start sync/build with provided assets
DistService-->>DevServer: send sync result (success/failure)
DevServer-->>ClientApp: websocket 'template:sync:response' (status)
else user cancels
Browser->>ClientApp: modal resolves approved=false
ClientApp->>DevServer: websocket 'template:sync:action-response' {requestId, approved:false}
DevServer-->>ClientApp: websocket 'template:sync:response' {cancelled:true, message}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
MaksymovVolodymyr
previously approved these changes
Apr 21, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
BUILD_IMPROVEMENTS.md (1)
91-104:⚠️ Potential issue | 🟡 MinorRemove duplicate "Rollup (Primary)" section.
Lines 91-97 and 98-104 contain identical headings and content. This appears to be an unintended duplication that should be removed to avoid confusion.
🧹 Proposed fix to remove the duplicate
### Rollup (Primary) - Advanced tree-shaking - Plugin ecosystem - Source map generation - Bundle analysis support - -### Rollup (Primary) - -- Advanced tree-shaking -- Plugin ecosystem -- Source map generation -- Bundle analysis support🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BUILD_IMPROVEMENTS.md` around lines 91 - 104, Remove the duplicated "Rollup (Primary)" section by keeping a single heading and its bullet list and deleting the second identical block; locate the duplicate heading "Rollup (Primary)" and the repeated bullets under it and remove the redundant copy so the document contains only one Rollup (Primary) section.src/lib/helpers/token.helper.ts (1)
26-42:⚠️ Potential issue | 🟡 MinorWrap the switch case in braces to create proper block scope.
Static analysis correctly flags that
const lowerMessageat line 27 is declared without block-level scope. While the code functions correctly (each path returns), wrapping the case in braces prevents potential issues if the code is modified later and aligns with best practices.🔧 Proposed fix to add block scope
switch (status) { - case 412: + case 412: { const lowerMessage = message.toLowerCase(); if (lowerMessage.includes('session expired') || lowerMessage.includes('session has expired')) { return { status, message, code: 'SESSION_EXPIRED', userFriendlyMessage: 'Your session has expired', suggestions: [ 'Refresh your token in the portal', 'Re-authenticate with the portal', 'Check if your token has been revoked', 'Ensure your token has the correct permissions', ], }; } return { status, message, code: 'AUTH_FAILED', userFriendlyMessage: 'Authentication failed', suggestions: [ 'Verify your token is correct', 'Check token permissions', "Ensure the token hasn't expired", 'Try generating a new token', ], }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/token.helper.ts` around lines 26 - 42, Wrap the case 412 switch branch in its own block to provide proper block-level scoping for the const lowerMessage: add braces surrounding the branch that starts with case 412 and ends with the return object so that lowerMessage (and any future temp vars) are scoped to that case; update the case 412 branch near token.helper.ts where lowerMessage is declared to use this new block scope.scripts/run-tests.js (1)
119-121:⚠️ Potential issue | 🟡 MinorFix callback signature in error handler.
The error handler passes
nullas the second argument, but the callback signature expects{ exitCode: number }. This is inconsistent with the TypeScript version ine2e/config.spec.ts(line 44), which correctly passes{ exitCode: 1 }.🐛 Proposed fix to match callback contract
child.on('error', (error) => { - callback(error, null); + callback(error, { exitCode: 1 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-tests.js` around lines 119 - 121, The error handler currently calls callback(error, null) which violates the callback contract expecting a second arg shaped like { exitCode: number }; update the handler on the child process (child.on('error', ...)) to pass an object with an exitCode (e.g., { exitCode: 1 }) as the second argument so the signature matches the TypeScript test helper (see e2e/config.spec.ts usage). Ensure only the second argument is changed and the first remains the error object.tests/test-nextjs/src/pages/_document.tsx (1)
3-8:⚠️ Potential issue | 🟡 MinorRemove the
metadataexport—it's not supported in Pages Router.The
metadataexport (lines 5-8) is exclusive to the App Router and does not function in Pages Router's_document.tsx. For Pages Router, use thenext/headcomponent in individual pages to handle metadata instead, or define static site-wide head content directly within this file's<Head>element if needed.🤖 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 3 - 8, Remove the unsupported Pages Router export: delete the exported metadata constant (export const metadata: Metadata = {...}) and its import of Metadata from 'next' in _document.tsx; instead, move any page-specific title/description into individual pages using next/head or place site-wide head elements inside the existing <Head> element in the Document component so metadata is handled via the Pages Router mechanism.src/lib/helpers/url.helper.ts (1)
18-32:⚠️ Potential issue | 🟠 MajorEscape dynamic values before building replacement regexes.
originalHostandurlPathare treated as regex source, so values containing metacharacters like.,+,?, or[can over-match or throw. Escape the literal inputs before interpolating them.🛡️ Proposed fix
+const escapeRegExp = (value: string) => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + export const urlReplacer = (originalHost: string, destinationHost: string, content: string) => { - const urlReplaceRegExp = new RegExp(`(!!)?(https?(:(\\\\)?/(\\\\)?/)${originalHost})`, 'gi'); + const escapedOriginalHost = escapeRegExp(originalHost); + const urlReplaceRegExp = new RegExp(`(!!)?(https?(:(\\\\)?/(\\\\)?/)${escapedOriginalHost})`, 'gi'); return content.replace(urlReplaceRegExp, (substring, ...args) => { if (substring.startsWith('!!')) { return args[1]; @@ }; export const urlPathReplacer = (urlPath: string, destinationPath: string, content: string) => { - const urlReplaceRegExp = new RegExp(`${urlPath.replace(/\\*\//gi, '\\\\/')}`, 'gi'); - const unescapedUrlReplaceRegExp = new RegExp(`${urlPath}`, 'gi'); + const escapedUrlPath = escapeRegExp(urlPath); + const urlReplaceRegExp = new RegExp(escapedUrlPath.replace(/\\*\//gi, '\\\\/'), 'gi'); + const unescapedUrlReplaceRegExp = new RegExp(escapedUrlPath, 'gi');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/url.helper.ts` around lines 18 - 32, The regexp constructors in urlReplacer and urlPathReplacer use unescaped user inputs (originalHost and urlPath), which can cause over-matching or runtime errors; fix by escaping those inputs before interpolating into new RegExp (create/inline an escapeRegExp function that replaces special characters like .+*?^${}()|[]\ with escaped versions) and use the escapedOriginalHost when building urlReplaceRegExp in urlReplacer and escapedUrlPath when building both urlReplaceRegExp and unescapedUrlReplaceRegExp in urlPathReplacer so the patterns match the literal strings.src/lib/proxy-pass.middleware.ts (1)
32-39:⚠️ Potential issue | 🟠 MajorApply the streaming interceptor callback or remove it.
The callback created at Lines 183-185 is never invoked because
streamResponseInterceptorignores itsinterceptorparameter. Streaming responses are currently piped through without the intended URL rewriting.Do you want me to generate a safe streaming transform implementation for this path?
Also applies to: 181-187
🤖 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 32 - 39, streamResponseInterceptor currently ignores its interceptor parameter so proxied streams are unmodified; implement a streaming Transform that, when interceptor is provided, reads chunks from proxyRes, converts to Buffer (respecting encoding), calls interceptor(chunk, encoding) and pushes the interceptor result, then pipe proxyRes -> transform -> res; if interceptor is undefined, keep the direct proxyRes.pipe(res) behavior. Update the function (streamResponseInterceptor) to construct and use a stream.Transform (from 'stream') named e.g. responseTransform, ensure you handle Buffer/Uint8Array and encoding types correctly and forward errors from proxyRes/transform to res, and keep the existing res.setHeader/ res.setHeaders logic unchanged.src/lib/proxy-cache.middleware.ts (1)
221-240:⚠️ Potential issue | 🟠 MajorPreserve
ServerResponse.write/endcallback overloads while buffering.Node.js
ServerResponse.write()andend()support overloads where callbacks can be passed as the second argument without encoding (e.g.,res.write(chunk, callback)orres.end(chunk, callback)). The current signatures treat the second parameter as encoding only, so passing a callback function causesBuffer.from(data, callbackFn)to fail. Additionally, thewritemethod ignores and never invokes captured callbacks during buffering, breaking code that relies on write completion.The suggestion to normalize encoding and callback parameters, queue write callbacks, and forward them during final
originalEndcall addresses both issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-cache.middleware.ts` around lines 221 - 240, The buffered override of (res as any).write incorrectly treats the second argument as encoding only and drops any callback; update (res as any).write to normalize parameters by checking if the second arg is a function (treat as callback) or a string (encoding), default encoding to 'utf8' when needed, capture/queue any provided callback into a write-callbacks array (e.g., writeCallbacks) alongside pushing Buffer data into chunks, and ensure those queued callbacks are invoked when the buffered body is flushed by originalEnd (or originalWrite) so callers receive their completion callbacks; reference the overridden (res as any).write, originalWrite, originalEnd, chunks, and the new write-callbacks queue when making changes.tests/unit/plugin/version-plugin.spec.ts (1)
222-239:⚠️ Potential issue | 🟡 MinorCase-sensitive substring check can miss
BUILD-MANIFEST.jsonin the "disabled" negative test.
files.find((f) => f.includes('VERSION') || f.includes('manifest'))is case-sensitive —'manifest'won't match'BUILD-MANIFEST.json'. If the plugin regressed and wrote the build manifest whileenabled: false, this test would still pass and miss the regression. Consider matching'MANIFEST'(or using a case-insensitive regex) so both artifacts are covered.💡 Proposed fix
- const versionFile = files.find((f: string) => f.includes('VERSION') || f.includes('manifest')); + const versionFile = files.find((f: string) => /VERSION|MANIFEST|manifest/.test(f));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/plugin/version-plugin.spec.ts` around lines 222 - 239, The negative test uses a case-sensitive substring check that can miss BUILD-MANIFEST.json; update the assertion in the test that inspects files (the files.find(...) call) to use a case-insensitive match (e.g., a case-insensitive RegExp) or explicitly check for 'MANIFEST' in addition to 'manifest' so that any manifest filename like BUILD-MANIFEST.json or VERSION is detected when versionPlugin(...) is called with enabled: false and invokeCloseBundle(plugin) is run.
🟡 Minor comments (5)
CHANGELOG.md-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorInconsistent heading hierarchy in changelog.
The changelog mixes
#(h1) and##(h2) for version headers. Line 8 uses# [0.18.0-beta.1](h1), immediately followed by### Features(h3) on line 10, which violates markdown heading increment rules. Version headers should consistently use##(h2) throughout the file, as done for the new 0.18.1-beta.1 entry (line 1) and older entries below.📝 Proposed fix to standardize heading levels
-# [0.18.0-beta.1](https://github.com/mi-examples/pp-dev/compare/v0.17.0...v0.18.0-beta.1) (2026-04-06) +## [0.18.0-beta.1](https://github.com/mi-examples/pp-dev/compare/v0.17.0...v0.18.0-beta.1) (2026-04-06) ### FeaturesApply this same fix to all other version headers that incorrectly use
#instead of##(lines 14, 24, 40, 46, 64, 76, 82, 93, 103, 109, 115, 121, 127, 134, 144, 154, 160).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 8 - 10, Replace top-level single-hash version headers that start with "# [" (e.g., "# [0.18.0-beta.1]") with second-level "## [" headings so all release/version entries use a consistent h2 level; update every occurrence matching that pattern (the comment lists examples such as the header on the 0.18.0-beta.1 entry and the other entries that currently use "#" instead of "##") and leave subheadings like "### Features" unchanged.rollup.config.ts-67-106 (1)
67-106:⚠️ Potential issue | 🟡 MinorValidate DTS monitor interval values before scheduling timers.
Number(process.env...)can produceNaN,0, or negative values. That can make the timeout fire immediately or the heartbeat spam logs during release builds.🛡️ Proposed validation
function typeDefsMonitorPlugin() { - const timeoutMs = Number(process.env.PP_DEV_DTS_TIMEOUT_MS ?? 120_000); - const heartbeatMs = Number(process.env.PP_DEV_DTS_HEARTBEAT_MS ?? 10_000); + const positiveNumberFromEnv = (name: string, fallback: number) => { + const value = Number(process.env[name]); + + return Number.isFinite(value) && value > 0 ? value : fallback; + }; + + const timeoutMs = positiveNumberFromEnv('PP_DEV_DTS_TIMEOUT_MS', 120_000); + const heartbeatMs = positiveNumberFromEnv('PP_DEV_DTS_HEARTBEAT_MS', 10_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rollup.config.ts` around lines 67 - 106, Validate and sanitize timeoutMs and heartbeatMs in typeDefsMonitorPlugin before scheduling timers: ensure Number(process.env.PP_DEV_DTS_TIMEOUT_MS) and Number(process.env.PP_DEV_DTS_HEARTBEAT_MS) are finite positive integers (fallback to 120_000 and 10_000 respectively if they are NaN, <=0, or Infinity), then only call setInterval and setTimeout in buildStart when the sanitized heartbeatMs/timeoutMs are valid; also guard heartbeat.unref and timeout.unref behind existence checks and clearTimers should still work with potentially unset timers (heartbeat/timeout possibly null).src/cli.ts-1300-1302 (1)
1300-1302:⚠️ Potential issue | 🟡 MinorAwait profiler shutdown in the build finalizer.
stopProfilercan return a promise; without awaiting it, the build command may finish before the CPU profile is written.🧪 Proposed fix
} finally { - stopProfiler((message) => createLogger(options.logLevel).info(message)); + await stopProfiler((message) => createLogger(options.logLevel).info(message)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1300 - 1302, The finalizer calls stopProfiler(...) but doesn't await its possible Promise, so ensure you await stopProfiler in the finally block (i.e., change the call to await stopProfiler((message) => createLogger(options.logLevel).info(message))) and make the enclosing function/handler async or return the resulting promise so the build command waits for the CPU profile write to complete; update any callers/returns as needed to propagate the async change.src/client/index.html-14-18 (1)
14-18:⚠️ Potential issue | 🟡 MinorAdd
relto_blanklinks.These links open a new tab; add
rel="noopener noreferrer"to prevent the opened page from accessingwindow.opener.Suggested fix
<a href="https://www.npmjs.com/package/{%= PACKAGE_NAME %}/v/{%= VERSION %}" target="_blank" + rel="noopener noreferrer" class="pp-dev-info__link" ><a href="!!{%= backendBaseURL %}/admin/page/edit/id/{%= portalPageId %}" target="_blank" + rel="noopener noreferrer" class="pp-dev-info__link" >{%= portalPageId %}</a >Also applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.html` around lines 14 - 18, The anchor elements using target="_blank" (the <a> elements with class "pp-dev-info__link" and href like "https://www.npmjs.com/package/{%= PACKAGE_NAME %}/v/{%= VERSION %}") must include rel="noopener noreferrer"; update those anchors (both the occurrence around lines with the npm badge and the other similar anchor at lines ~50-55) to add rel="noopener noreferrer" to prevent the opened page from accessing window.opener.src/client/index.ts-232-287 (1)
232-287:⚠️ Potential issue | 🟡 MinorConfirm modal is missing basic accessibility affordances.
The dialog has no ARIA semantics or focus management, which makes the confirm flow hard to use with a keyboard or screen reader:
- No
role="dialog"/aria-modal="true"/aria-labelledbyon the overlay or dialog element.- Focus is not moved into the modal on open, nor trapped inside it, nor restored to the previously-focused element on close.
- Tabbing can escape to the underlying page while the backdrop visually blocks it.
This is a dev-tool UI so the impact is limited, but it's cheap to fix — set ARIA attributes on
$confirm, call$confirmButton.focus()after append, rememberdocument.activeElementbefore opening and restore it on close, and add a small Tab-cycling trap between cancel/confirm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.ts` around lines 232 - 287, In confirmModal, add ARIA and focus management: set role="dialog", aria-modal="true" on $confirm and add aria-labelledby pointing to an id you assign to $title; before appending, save document.activeElement to a variable (previousActiveElement) and after appending call $confirmButton.focus(); implement a simple tab-trap on the modal (keydown handler that intercepts Tab and cycles between $cancelButton and $confirmButton); ensure all event listeners are removed and previousActiveElement.focus() is restored when closing by updating closeConfirmModalByResult to accept/restore the saved previousActiveElement and clean up the tab-trap listener.
🧹 Nitpick comments (5)
CONTRIBUTING.md (1)
78-78: Optional: tighten wording for the CI-skip instruction.Consider a shorter, more direct phrasing to reduce ambiguity (e.g., “To amend a release commit, include
[skip ci]in the commit message.”).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 78, Tighten the CI-skip instruction sentence by replacing the current wording with a shorter, direct phrase; update the line that currently reads "If you need to make changes to the release commit (like updating the changelog), you can skip the CI by including `[skip ci]` in your commit message." to a concise variant such as "To amend a release commit, include `[skip ci]` in the commit message." so the guidance is clearer and less ambiguous.src/lib/helpers/login.helper.ts (3)
377-384: Scope helper control lookups to the injected form.Because this script is injected into host HTML, global
document.getElementById(...)can bind to an unrelated/stale element with the same ID. Query withinformContentinstead.Proposed scoped lookup
-const tokenTypeSwitcher = document.getElementById('token-type-switcher'); -const tokenInput = document.getElementById('helper-token') as HTMLInputElement; -const tokenCaption = document.getElementById('token-caption') as HTMLSpanElement; -const submitButton = document.getElementById('helper-token-submit') as HTMLButtonElement; -const errorElement = document.getElementById('error-message') as HTMLDivElement; +const tokenTypeSwitcher = formContent.querySelector<HTMLElement>('#token-type-switcher'); +const tokenInput = formContent.querySelector<HTMLInputElement>('#helper-token'); +const tokenCaption = formContent.querySelector<HTMLSpanElement>('#token-caption'); +const submitButton = formContent.querySelector<HTMLButtonElement>('#helper-token-submit'); +const errorElement = formContent.querySelector<HTMLDivElement>('#error-message'); const formWrapper = formContent.querySelector('.helper-login-wrapper') as HTMLElement;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/login.helper.ts` around lines 377 - 384, The DOM lookups use global document.getElementById and can pick up unrelated elements; change all lookups to query within the injected form by replacing document.getElementById(...) with scoped queries off formContent (e.g., formContent.querySelector(...) or formContent.getElementById-equivalent) for the symbols tokenTypeSwitcher, tokenInput, tokenCaption, submitButton, errorElement and keep formWrapper found via formContent.querySelector('.helper-login-wrapper'); ensure the typed casts (HTMLInputElement/HTMLSpanElement/HTMLButtonElement/HTMLDivElement/HTMLElement) remain correct and update the null-check that verifies those scoped variables.
398-423: Avoid:has()in runtime selectors — use filtering for broader browser compatibility.The
document.querySelector()call with:has()syntax throws aSyntaxErrorin browsers without support, which would prevent the fallback logic from executing. While the project targets modern environments (Next.js 15+, Node 22+), the injected helper lacks explicit browser compatibility documentation and would fail silently in older browsers without even reaching the fallback check at line 423.Replace the selector-level
:has()with a broader form selector and JavaScript filtering:Recommended change
-const formSelector = '#mi-react-root form:not([id]):has(input[name="login"]):has(input[name="password"])'; +const formSelector = '#mi-react-root form:not([id])'; +const findLoginForm = () => + Array.from(document.querySelectorAll<HTMLFormElement>(formSelector)).find( + (form) => form.querySelector('input[name="login"]') && form.querySelector('input[name="password"]'), + ) ?? null; // Main logic - use MutationObserver instead of setInterval for better performance const observer = new MutationObserver((mutations) => { for (const mutation of mutations) { if (mutation.type === 'childList') { - const loginForm = document.querySelector<HTMLFormElement>(formSelector); + const loginForm = findLoginForm(); if (loginForm) { observer.disconnect(); initializeLoginForm(loginForm); @@ // Fallback: check if form already exists -const existingForm = document.querySelector<HTMLFormElement>(formSelector); +const existingForm = findLoginForm();This approach works in all browsers with
querySelectorsupport and is consistent with the fallback pattern already in use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/login.helper.ts` around lines 398 - 423, The current selector uses :has() which can throw a SyntaxError in some browsers; update by replacing formSelector with a broader selector like '#mi-react-root form:not([id])' and then, wherever you call document.querySelector<HTMLFormElement>(formSelector) (both inside the MutationObserver callback and for existingForm), instead query for the broad selector and filter the resulting element(s) in JS to ensure they contain input[name="login"] and input[name="password"] before calling observer.disconnect() and initializeLoginForm(loginForm); keep the observer.observe logic unchanged but use the filtered loginForm variable so the fallback check for existingForm likewise verifies inputs via form.querySelector('input[name="login"]') and form.querySelector('input[name="password"]').
338-346: Use element-only traversal for DOM node selection ininitializeLoginForm.
firstChild/lastChildcan return text or comment nodes, not just elements. The subsequent code calls element-only APIs (innerHTML,setAttribute,appendChild) on these nodes, which would fail at runtime if a non-element node is selected. UsefirstElementChild/lastElementChildinstead, which only return Element nodes.Proposed hardening
function initializeLoginForm(loginForm: HTMLFormElement) { - const contentWrapper = loginForm.firstChild as HTMLDivElement; + const contentWrapper = loginForm.firstElementChild as HTMLDivElement | null; if (!contentWrapper) { return; } // Create form elements - const rowTemplate = (contentWrapper.lastChild as HTMLDivElement)?.cloneNode(true) as HTMLDivElement; + const rowTemplate = contentWrapper.lastElementChild?.cloneNode(true) as HTMLDivElement | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/login.helper.ts` around lines 338 - 346, The function initializeLoginForm uses node-only accessors (loginForm.firstChild and contentWrapper.lastChild) which can return non-Element nodes; change those to element-only accessors (use loginForm.firstElementChild and (contentWrapper as Element).lastElementChild) and update the casts to HTMLDivElement to ensure the variables contentWrapper and rowTemplate are Elements, add null checks after using firstElementChild/lastElementChild before using element APIs (innerHTML, setAttribute, appendChild), and adjust any subsequent references that assume Node to use Element/HTMLDivElement types (e.g., contentWrapper, rowTemplate) so DOM methods are safe at runtime.src/index.ts (1)
89-100: Bound theexecSyncgit fallback with a timeout.
execSync('git config --get remote.origin.url', …)has no timeout, so if the git process hangs (e.g., a misconfigured system git, slow NFS, or an unexpected credential helper prompt),getViteConfig()— and therefore dev server/build startup — will block indefinitely. The stdio config already prevents interactive prompts in the common case, but a small safety bound is cheap insurance.♻️ Proposed change
const gitRemoteOriginUrl = execSync('git config --get remote.origin.url', { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], + timeout: 2000, }).trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 89 - 100, The execSync call that reads git origin URL can hang; modify the call (the execSync in getViteConfig / the snippet that calls execSync('git config --get remote.origin.url', ...)) to include a timeout option (e.g., timeout: 5000) so the child will be killed after a short bound; preserve existing options (encoding, stdio) and keep the try/catch behavior so failures/timeouts are ignored as before.
🤖 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/api/page.ts`:
- Around line 45-50: The code currently returns data.page without validating it;
update the function that checks response data (the block using the variable data
and calling unavailablePageDataError()) to verify that data.page is present and
valid (e.g., typeof data.page === 'object' && data.page !== null or otherwise
meets your page shape) and if not, throw unavailablePageDataError() instead of
returning undefined; replace the final return data.page with a guarded return
after this validation so downstream callers never receive undefined.
In `@src/cli.ts`:
- Around line 1249-1268: The reducer seeds with backups[0] which may be a
directory or non-ZIP; first filter the directory entries to a ZIP-only array
(e.g., create zipBackups = backups.filter(b => b.isFile() &&
b.name.endsWith('.zip'))), check if zipBackups.length is zero and log/return as
you already do, then run the reduce over zipBackups (using zipBackups[0] as the
seed) to compute latestBackup; update references to backups in this block to use
the ZIP-only array and keep backupsDirPath, latestBackup, and the existing
filter/reduce logic names to locate the change.
In `@src/client/index.ts`:
- Around line 228-287: The current closeAllConfirmModals() only removes DOM
nodes and thus leaks document keydown listeners and leaves the Promise returned
by confirmModal() unresolved; update the implementation so that confirmModal()
registers each active modal (overlay, resolve, and onKeyDown) in a Map/Set and
exposes a teardown function that removes the overlay, removes the keydown
listener (onKeyDown), and calls the stored resolve(false) (or resolve(true) for
confirm path) before deleting the entry; then change closeAllConfirmModals() to
iterate active modals and call that teardown for each (ensuring
closeConfirmModalByResult or the new teardown is used for
cancel/escape/overlay-click and the confirm path removes the entry before
resolving true).
In `@src/lib/client.service.ts`:
- Around line 83-94: The promise returned by requestSyncAction can hang forever
if no response arrives; modify requestSyncAction to register a timeout (e.g.,
configurable ms) after creating requestId and setting this.syncActionResolvers
(same map used to resolve on 'template:sync:action-response') that will cleanup
the resolver (this.syncActionResolvers.delete(requestId)) and resolve(false) or
reject after timeout, and ensure the timeout is cleared when the resolver is
invoked normally; also add a cleanup path that removes any outstanding resolvers
when the WS disconnects (hook into server.ws close/error handlers) so stale
entries are removed.
In `@src/lib/dist.service.ts`:
- Around line 575-583: The current early return in DistService (the if using
resolvedAnalysis.allFilesInMeta, resolvedAnalysis.mainHash and lastMainHash) can
skip creating a backup when the manifest hash matches but the on-disk files or
the backupFingerprint changed; modify the condition so we only skip when both
the mainHash is equal AND the backupFingerprint (or whichever computed
fingerprint you use to detect content changes) is unchanged. Concretely, in the
block that checks resolvedAnalysis.allFilesInMeta and resolvedAnalysis.mainHash
and lastMainHash === resolvedAnalysis.mainHash, add a comparison against the
existing backupFingerprint (or compute and compare currentFingerprint) and only
return when fingerprint equality is true; otherwise fall through and create/save
the new backup so the user override prompt is preserved.
- Around line 410-426: The manifest loop currently reads files using
path.join(contentRootDir, relativePath) without validating that relativePath is
safe; ensure you reject unsafe/escaping paths before reading by normalizing and
resolving each path and confirming it is contained within contentRootDir (e.g.,
for entries in normalizedManifestFiles inside the same function in
dist.service.ts that uses pathToPosix and buildManifestChecksum). For each
relativePath, convert to a posix-normalized form, compute const absolutePath =
path.resolve(contentRootDir, relativePath) and if
(!absolutePath.startsWith(path.resolve(contentRootDir) + path.sep)) throw a
descriptive Error (e.g., "VERSION manifest contains unsafe path: ..."); only
then read the file with fs.readFile. Ensure this validation happens before any
file IO and applies to both the manifest parsing loop and the subsequent
file-reading loop.
- Around line 590-604: Currently the code updates this.currentMeta
(lastBackupName, lastBackupHash, lastBackupDate, lastMainHash) and calls
this.syncMeta() before calling fs.writeFile, and uses finally() to log success
even on failure; change the flow so you first await
fs.writeFile(path.resolve(this.backupFolder, filename), backupFile) and only
after that resolves successfully update this.currentMeta (set lastBackupName =
filename, lastBackupHash = resolvedBackupFileHash, lastBackupDate =
backupDate.toISOString(), set or delete lastMainHash based on
resolvedAnalysis.mainHash), then call await this.syncMeta(), and finally call
this.logger.info(`Backup saved to ${filename}`); ensure any error from
fs.writeFile is allowed to propagate (do not update meta or log success on
failure).
- Around line 340-372: The code currently leaves versionManifestPath null when
the BUILD-MANIFEST references a VERSION file that isn't present, allowing
corrupt backups to pass; after resolving candidates (the block that sets
versionManifestPath from resolvedCandidates using this.pathToPosix and
relativePaths), add a check: if buildManifest.compat?.versionFileRequired (or
the manifest flag used by version-plugin, e.g.,
buildManifest.compat.versionFileRequired === true) and versionManifestPath ===
null, throw a descriptive Error stating the required version file (include
buildManifest.versionFile) is missing so the process fails rather than skipping
fingerprint comparison.
In `@src/lib/helpers/server.ts`:
- Around line 36-38: The server.listen override (server.listen) currently
assigns the result of originalListen to the local listener variable but doesn't
return it; update the override so it returns that listener (the Server instance)
— i.e., after listener = originalListen.apply(this, args); add a return of
listener so server.listen conforms to express.Express['listen']'s return type
and preserves chaining for callers expecting the Server from originalListen.
- Around line 55-56: The URL is constructed with brackets around address.address
unconditionally, which breaks for IPv4/hostnames; update the URL creation so it
only brackets IPv6 literals (e.g., use net.isIP(address.address) === 6 or
address.family === 'IPv6' to detect IPv6) and otherwise build the origin as
`http://${address.address}:${address.port}`; adjust the const url assignment
that currently uses `new URL(base || '',
\`http://[${address.address}]:${address.port}\`)` to conditionally include
brackets only for IPv6 addresses.
In `@src/lib/pp.middleware.ts`:
- Around line 246-247: The code treats Page.template_id === null as present
(using typeof page.template_id !== 'undefined'), causing this.#isV710OrHigher
and subsequent non-null assertions to try to use a null template ID; change
those checks to explicitly exclude null (e.g., check page.template_id !==
undefined && page.template_id !== null or use page.template_id != null) wherever
you currently use typeof page.template_id !== 'undefined' (including the
occurrences setting this.#isV710OrHigher and the later blocks that access
template assets), so template_id === null is treated as absent and no null ID is
requested.
- Around line 476-478: Escape regex metacharacters in each page-variable name
before building the RegExp and avoid special `$` replacement interpretation by
using a replacer function: when iterating this.#pageVars (the loop variable v)
compute an escapedName (escape characters like . * + ? ^ $ { } ( ) | [ ] \ /)
then use new RegExp(escapedName, 'g') and call result.replace(theRegex, () =>
v.value) so replacement values are inserted verbatim; add a small helper like
escapeRegExp used by the loop to centralize the escaping.
- Around line 86-111: Remove the process-wide TLS and Axios default mutations:
delete the assignment to process.env.NODE_TLS_REJECT_UNAUTHORIZED and the
assignment to axios.defaults.httpsAgent so certificate validation is only
relaxed for this instance; keep the per-instance httpsAgent creation (the
https.Agent with rejectUnauthorized: false) and pass it into this.#axios =
axios.create(...) as before (retain httpAgent and keepAlive:false). Also verify
no other code paths rely on process-wide NODE_TLS_REJECT_UNAUTHORIZED or
axios.defaults.httpsAgent.
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 127-129: The referer header replacement uses the client-controlled
host directly in a RegExp which can be abused; update the logic in the proxy
middleware where proxyReq.setHeader('referer', ...) is set (look for variables
host, referer, baseURL and the call to proxyReq.setHeader) to first escape regex
metacharacters in host (or, better, parse the referer to get its origin) and
then build a safe anchored pattern (e.g. ^https?://<escapedHost>) so only the
referer origin is replaced with baseURL; ensure you handle non-string referer
and preserve existing behavior when host or referer are absent.
- Around line 159-165: The current logic attaches a new per-request handler with
req.socket.once('close', ...) which accumulates on keep-alive sockets; replace
this by attaching cleanup to the request/response lifecycle (e.g., use
req.on('close', cleanup) or proxyReq.on('close', cleanup) instead of
req.socket.once) or deduplicate at the socket level (e.g., store a
WeakSet/WeakMap keyed by req.socket and only add one shared close handler).
Update the code around req.socket.setMaxListeners(0) and
req.socket.once('close', ...) to remove the per-request socket.once usage and
implement either req.on('close', ...) or proxyReq.on('close', ...) with proper
removal, or add deduplication using a WeakMap keyed by req.socket so duplicate
handlers are not added.
In `@src/plugins/version-plugin.ts`:
- Around line 89-119: normalizeRepositoryUrl currently returns HTTP(S) URLs
unchanged, which can leak userinfo (credentials/tokens) present in CI-provided
repository URLs; update normalizeRepositoryUrl to strip any userinfo from URLs
(e.g., remove "user" or "user:pass@" from "http(s)://...") before returning. In
practice, after computing normalizedUrl (and also for branches where you
construct https://host/path for scp/ssh/git protocols), parse or regex-strip an
optional userinfo segment (username[:password]@) so the final returned string
never contains credentials; update the function normalizeRepositoryUrl to
perform this userinfo removal for all code paths.
In `@src/shortcuts.ts`:
- Around line 98-115: The cleanupClosure must be made idempotent and must
unregister its own close listener from the specific httpServer it was attached
to: capture the http server reference when creating cleanupBinding (e.g. const
boundServer = server.httpServer), attach with boundServer.on('close',
cleanupBinding), and at the start of cleanupBinding return early if
cleanupActiveShortcutBinding !== cleanupBinding; then remove the listener with
boundServer.off('close', cleanupBinding) (or removeListener) and finally clear
cleanupActiveShortcutBinding and revert stdin state; this ensures the old
cleanup won't run later and that each binding unregisters itself from its
original server.
---
Outside diff comments:
In `@BUILD_IMPROVEMENTS.md`:
- Around line 91-104: Remove the duplicated "Rollup (Primary)" section by
keeping a single heading and its bullet list and deleting the second identical
block; locate the duplicate heading "Rollup (Primary)" and the repeated bullets
under it and remove the redundant copy so the document contains only one Rollup
(Primary) section.
In `@scripts/run-tests.js`:
- Around line 119-121: The error handler currently calls callback(error, null)
which violates the callback contract expecting a second arg shaped like {
exitCode: number }; update the handler on the child process (child.on('error',
...)) to pass an object with an exitCode (e.g., { exitCode: 1 }) as the second
argument so the signature matches the TypeScript test helper (see
e2e/config.spec.ts usage). Ensure only the second argument is changed and the
first remains the error object.
In `@src/lib/helpers/token.helper.ts`:
- Around line 26-42: Wrap the case 412 switch branch in its own block to provide
proper block-level scoping for the const lowerMessage: add braces surrounding
the branch that starts with case 412 and ends with the return object so that
lowerMessage (and any future temp vars) are scoped to that case; update the case
412 branch near token.helper.ts where lowerMessage is declared to use this new
block scope.
In `@src/lib/helpers/url.helper.ts`:
- Around line 18-32: The regexp constructors in urlReplacer and urlPathReplacer
use unescaped user inputs (originalHost and urlPath), which can cause
over-matching or runtime errors; fix by escaping those inputs before
interpolating into new RegExp (create/inline an escapeRegExp function that
replaces special characters like .+*?^${}()|[]\ with escaped versions) and use
the escapedOriginalHost when building urlReplaceRegExp in urlReplacer and
escapedUrlPath when building both urlReplaceRegExp and unescapedUrlReplaceRegExp
in urlPathReplacer so the patterns match the literal strings.
In `@src/lib/proxy-cache.middleware.ts`:
- Around line 221-240: The buffered override of (res as any).write incorrectly
treats the second argument as encoding only and drops any callback; update (res
as any).write to normalize parameters by checking if the second arg is a
function (treat as callback) or a string (encoding), default encoding to 'utf8'
when needed, capture/queue any provided callback into a write-callbacks array
(e.g., writeCallbacks) alongside pushing Buffer data into chunks, and ensure
those queued callbacks are invoked when the buffered body is flushed by
originalEnd (or originalWrite) so callers receive their completion callbacks;
reference the overridden (res as any).write, originalWrite, originalEnd, chunks,
and the new write-callbacks queue when making changes.
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 32-39: streamResponseInterceptor currently ignores its interceptor
parameter so proxied streams are unmodified; implement a streaming Transform
that, when interceptor is provided, reads chunks from proxyRes, converts to
Buffer (respecting encoding), calls interceptor(chunk, encoding) and pushes the
interceptor result, then pipe proxyRes -> transform -> res; if interceptor is
undefined, keep the direct proxyRes.pipe(res) behavior. Update the function
(streamResponseInterceptor) to construct and use a stream.Transform (from
'stream') named e.g. responseTransform, ensure you handle Buffer/Uint8Array and
encoding types correctly and forward errors from proxyRes/transform to res, and
keep the existing res.setHeader/ res.setHeaders logic unchanged.
In `@tests/test-nextjs/src/pages/_document.tsx`:
- Around line 3-8: Remove the unsupported Pages Router export: delete the
exported metadata constant (export const metadata: Metadata = {...}) and its
import of Metadata from 'next' in _document.tsx; instead, move any page-specific
title/description into individual pages using next/head or place site-wide head
elements inside the existing <Head> element in the Document component so
metadata is handled via the Pages Router mechanism.
In `@tests/unit/plugin/version-plugin.spec.ts`:
- Around line 222-239: The negative test uses a case-sensitive substring check
that can miss BUILD-MANIFEST.json; update the assertion in the test that
inspects files (the files.find(...) call) to use a case-insensitive match (e.g.,
a case-insensitive RegExp) or explicitly check for 'MANIFEST' in addition to
'manifest' so that any manifest filename like BUILD-MANIFEST.json or VERSION is
detected when versionPlugin(...) is called with enabled: false and
invokeCloseBundle(plugin) is run.
---
Minor comments:
In `@CHANGELOG.md`:
- Around line 8-10: Replace top-level single-hash version headers that start
with "# [" (e.g., "# [0.18.0-beta.1]") with second-level "## [" headings so all
release/version entries use a consistent h2 level; update every occurrence
matching that pattern (the comment lists examples such as the header on the
0.18.0-beta.1 entry and the other entries that currently use "#" instead of
"##") and leave subheadings like "### Features" unchanged.
In `@rollup.config.ts`:
- Around line 67-106: Validate and sanitize timeoutMs and heartbeatMs in
typeDefsMonitorPlugin before scheduling timers: ensure
Number(process.env.PP_DEV_DTS_TIMEOUT_MS) and
Number(process.env.PP_DEV_DTS_HEARTBEAT_MS) are finite positive integers
(fallback to 120_000 and 10_000 respectively if they are NaN, <=0, or Infinity),
then only call setInterval and setTimeout in buildStart when the sanitized
heartbeatMs/timeoutMs are valid; also guard heartbeat.unref and timeout.unref
behind existence checks and clearTimers should still work with potentially unset
timers (heartbeat/timeout possibly null).
In `@src/cli.ts`:
- Around line 1300-1302: The finalizer calls stopProfiler(...) but doesn't await
its possible Promise, so ensure you await stopProfiler in the finally block
(i.e., change the call to await stopProfiler((message) =>
createLogger(options.logLevel).info(message))) and make the enclosing
function/handler async or return the resulting promise so the build command
waits for the CPU profile write to complete; update any callers/returns as
needed to propagate the async change.
In `@src/client/index.html`:
- Around line 14-18: The anchor elements using target="_blank" (the <a> elements
with class "pp-dev-info__link" and href like "https://www.npmjs.com/package/{%=
PACKAGE_NAME %}/v/{%= VERSION %}") must include rel="noopener noreferrer";
update those anchors (both the occurrence around lines with the npm badge and
the other similar anchor at lines ~50-55) to add rel="noopener noreferrer" to
prevent the opened page from accessing window.opener.
In `@src/client/index.ts`:
- Around line 232-287: In confirmModal, add ARIA and focus management: set
role="dialog", aria-modal="true" on $confirm and add aria-labelledby pointing to
an id you assign to $title; before appending, save document.activeElement to a
variable (previousActiveElement) and after appending call
$confirmButton.focus(); implement a simple tab-trap on the modal (keydown
handler that intercepts Tab and cycles between $cancelButton and
$confirmButton); ensure all event listeners are removed and
previousActiveElement.focus() is restored when closing by updating
closeConfirmModalByResult to accept/restore the saved previousActiveElement and
clean up the tab-trap listener.
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 78: Tighten the CI-skip instruction sentence by replacing the current
wording with a shorter, direct phrase; update the line that currently reads "If
you need to make changes to the release commit (like updating the changelog),
you can skip the CI by including `[skip ci]` in your commit message." to a
concise variant such as "To amend a release commit, include `[skip ci]` in the
commit message." so the guidance is clearer and less ambiguous.
In `@src/index.ts`:
- Around line 89-100: The execSync call that reads git origin URL can hang;
modify the call (the execSync in getViteConfig / the snippet that calls
execSync('git config --get remote.origin.url', ...)) to include a timeout option
(e.g., timeout: 5000) so the child will be killed after a short bound; preserve
existing options (encoding, stdio) and keep the try/catch behavior so
failures/timeouts are ignored as before.
In `@src/lib/helpers/login.helper.ts`:
- Around line 377-384: The DOM lookups use global document.getElementById and
can pick up unrelated elements; change all lookups to query within the injected
form by replacing document.getElementById(...) with scoped queries off
formContent (e.g., formContent.querySelector(...) or
formContent.getElementById-equivalent) for the symbols tokenTypeSwitcher,
tokenInput, tokenCaption, submitButton, errorElement and keep formWrapper found
via formContent.querySelector('.helper-login-wrapper'); ensure the typed casts
(HTMLInputElement/HTMLSpanElement/HTMLButtonElement/HTMLDivElement/HTMLElement)
remain correct and update the null-check that verifies those scoped variables.
- Around line 398-423: The current selector uses :has() which can throw a
SyntaxError in some browsers; update by replacing formSelector with a broader
selector like '#mi-react-root form:not([id])' and then, wherever you call
document.querySelector<HTMLFormElement>(formSelector) (both inside the
MutationObserver callback and for existingForm), instead query for the broad
selector and filter the resulting element(s) in JS to ensure they contain
input[name="login"] and input[name="password"] before calling
observer.disconnect() and initializeLoginForm(loginForm); keep the
observer.observe logic unchanged but use the filtered loginForm variable so the
fallback check for existingForm likewise verifies inputs via
form.querySelector('input[name="login"]') and
form.querySelector('input[name="password"]').
- Around line 338-346: The function initializeLoginForm uses node-only accessors
(loginForm.firstChild and contentWrapper.lastChild) which can return non-Element
nodes; change those to element-only accessors (use loginForm.firstElementChild
and (contentWrapper as Element).lastElementChild) and update the casts to
HTMLDivElement to ensure the variables contentWrapper and rowTemplate are
Elements, add null checks after using firstElementChild/lastElementChild before
using element APIs (innerHTML, setAttribute, appendChild), and adjust any
subsequent references that assume Node to use Element/HTMLDivElement types
(e.g., contentWrapper, rowTemplate) so DOM methods are safe at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 805fed51-0075-41e2-9a6f-a0161fb3418c
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsontests/test-commonjs/package-lock.jsonis excluded by!**/package-lock.jsontests/test-nextjs-cjs/package-lock.jsonis excluded by!**/package-lock.jsontests/test-nextjs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (101)
.github/dependabot.yml.prettierignore.prettierrc.cjsBUILD_IMPROVEMENTS.mdCHANGELOG.mdCONTRIBUTING.mdREADME.mde2e/config.spec.tse2e/server.spec.tse2e/tsconfig.jsonesbuild.config.tspackage.jsonplaywright.config.tspp-dev.d.tsrollup.config.tsscripts/build-optimizer.jsscripts/build-parallel.jsscripts/patch-npm-bundled-vulnerabilities.mjsscripts/postbuild.jsscripts/run-tests.jsscripts/startup-optimizer.jssrc/api/assets.tssrc/api/index.tssrc/api/page-template.tssrc/api/page.tssrc/api/unavailable-json-api.tssrc/cli.tssrc/client/assets/css/client.scsssrc/client/index.htmlsrc/client/index.tssrc/client/rollup.config.tssrc/client/tsconfig.jsonsrc/client/types/assets.d.tssrc/config.tssrc/constants.tssrc/index.tssrc/lib/auth.provider.tssrc/lib/changelog-generator.tssrc/lib/client.service.tssrc/lib/dist.service.tssrc/lib/helpers/content-encoding.helper.tssrc/lib/helpers/login.helper.tssrc/lib/helpers/server.tssrc/lib/helpers/token.helper.tssrc/lib/helpers/url.helper.tssrc/lib/internal.middleware.tssrc/lib/load-pp-data.middleware.tssrc/lib/logger.tssrc/lib/next-import.tssrc/lib/pp.middleware.tssrc/lib/proxy-cache.middleware.tssrc/lib/proxy-pass.middleware.tssrc/lib/rewrite-response.middleware.tssrc/plugin.tssrc/plugins/client-injection-plugin.tssrc/plugins/mi-topbar-plugin.tssrc/plugins/version-plugin.tssrc/shortcuts.tstests/README.mdtests/TESTING.mdtests/integration/middleware/pp-redirect.spec.tstests/integration/middleware/proxy-cache.spec.tstests/integration/middleware/rewrite-response.spec.tstests/setup.tstests/test-commonjs/index.htmltests/test-commonjs/pp-dev.config.tstests/test-commonjs/public/__template_variables.jsontests/test-commonjs/src/api/dataset-data.tstests/test-commonjs/src/api/entity.tstests/test-commonjs/src/api/user.tstests/test-commonjs/src/constants.tstests/test-commonjs/src/main.tstests/test-nextjs-cjs/src/api/entity.tstests/test-nextjs-cjs/src/api/user.tstests/test-nextjs-cjs/src/constants.tstests/test-nextjs-cjs/src/pages/index.tsxtests/test-nextjs-cjs/src/styles/global.scsstests/test-nextjs-cjs/src/styles/tailwind.csstests/test-nextjs-cjs/tsconfig.jsontests/test-nextjs/custom-server.tstests/test-nextjs/src/api/entity.tstests/test-nextjs/src/api/user.tstests/test-nextjs/src/constants.tstests/test-nextjs/src/pages/_document.tsxtests/test-nextjs/src/pages/index.tsxtests/test-nextjs/src/styles/global.scsstests/test-nextjs/src/styles/tailwind.csstests/test-nextjs/tsconfig.jsontests/tsconfig.jsontests/unit/config/config.loader.spec.tstests/unit/helpers/formatting.spec.tstests/unit/lib/content-encoding.helper.spec.tstests/unit/lib/token.helper.spec.tstests/unit/lib/url.helper.spec.tstests/unit/plugin/plugin.normalize.spec.tstests/unit/plugin/version-plugin.spec.tstsconfig.cjs.jsontsconfig.esm.jsontsconfig.jsonvitest.config.tsvitest.integration.config.ts
💤 Files with no reviewable changes (1)
- src/api/assets.ts
Prevent unresolved sync actions and stale listeners while enforcing required VERSION manifest presence and stripping sensitive URL/replacement edge cases.
PP-3311 Fix modal/sync lifecycle and safety checks
## [0.18.1-beta.2](v0.18.1-beta.1...v0.18.1-beta.2) (2026-04-22) ### Bug Fixes * harden sync prompt lifecycle and metadata safety ([3aaaeb6](3aaaeb6))
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/lib/pp.middleware.ts (2)
250-252:⚠️ Potential issue | 🟠 Major
nulltemplate IDs still treated as present.
typeof page.template_id !== 'undefined'returnstruefornull, which flips#isV710OrHigherand then drivespageTemplateApi.get(pageInfo.template_id!, ...)calls (lines 539, 559) with a null ID. Same pattern at line 426. Usepage.template_id != nullto treatnullas absent.🐛 Proposed fix
- if (typeof page.template_id !== 'undefined') { + if (page.template_id != null) { this.#isV710OrHigher = true; }Apply the same change at line 426, and optionally add
&& pageInfo.template_id != nullto the#isV710OrHigherbranches at 538 and 558.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/pp.middleware.ts` around lines 250 - 252, The check treating template IDs as present uses typeof page.template_id !== 'undefined', which is true for null; update the presence checks to use page.template_id != null (and the equivalent pageInfo.template_id != null) so null is considered absent, update the assignment to this.#isV710OrHigher to use that null-safe check (occurences that set `#isV710OrHigher` and the earlier check around the same logic), and also add explicit pageInfo.template_id != null guards before calling pageTemplateApi.get(...) to avoid passing null into those calls.
86-111:⚠️ Potential issue | 🔴 CriticalProcess-wide TLS disable +
axios.defaultsmutation still present.
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'(line 87) andaxios.defaults.httpsAgent = httpsAgent(line 110) were flagged in the prior review and remain in the code. These mutate global state: onceMiAPIis constructed withdisableSSLValidation, every HTTPS request in the process — including unrelated libraries that use bareaxiosor plainhttps— bypasses certificate validation for the lifetime of the process. The per-instancehttpsAgentwithrejectUnauthorized: falsealready handles TLS relaxation for this axios client; the globals are unnecessary.🛡️ Proposed fix
- if (disableSSLValidation) { - process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; - } - const httpsAgent = new https.Agent({ keepAlive: false, ...(disableSSLValidation ? { rejectUnauthorized: false } : {}), }); this.#axios = axios.create({ baseURL, headers, timeout: 30000, maxRedirects: 5, httpAgent: new http.Agent({ keepAlive: false }), httpsAgent, }); - - if (disableSSLValidation) { - axios.defaults.httpsAgent = httpsAgent; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/pp.middleware.ts` around lines 86 - 111, Remove the process-wide TLS and axios-default mutations: delete the assignment to process.env.NODE_TLS_REJECT_UNAUTHORIZED and the assignment to axios.defaults.httpsAgent so TLS relaxation is not applied globally; keep the per-instance httpsAgent creation (with rejectUnauthorized:false when disableSSLValidation is true) and pass that agent into this.#axios so only the MiAPI instance uses the relaxed TLS behavior (refer to disableSSLValidation, httpsAgent, and this.#axios in the constructor).src/lib/dist.service.ts (2)
581-610:⚠️ Potential issue | 🟠 MajorSave-skip and metadata persistence ordering: two unresolved concerns from the previous review.
Skip condition (lines 581-589): still triggers when
mainHashmatches, regardless ofversionManifestHashMismatches.lengthorbuildManifestMismatch. If on-disk files diverge from VERSION (the exact case the new prompt flow exists to handle), the new server backup that produced the override prompt is discarded and never written tobackups/, so users can't inspect what the server sent. Gate the skip on a clean analysis.Metadata order (lines 596-610):
this.currentMetais updated andsyncMeta()is awaited beforefs.writeFile, and the.finally()logs "Backup saved" even on write failure. A disk/permission error leavessync-service.meta.jsonpointing at a backup file that doesn't exist. Persist metadata only after the write resolves.🛡️ Proposed combined fix
- if (resolvedAnalysis.allFilesInMeta && resolvedAnalysis.mainHash && lastMainHash === resolvedAnalysis.mainHash) { + if ( + resolvedAnalysis.allFilesInMeta && + resolvedAnalysis.mainHash && + lastMainHash === resolvedAnalysis.mainHash && + resolvedAnalysis.versionManifestHashMismatches.length === 0 && + !resolvedAnalysis.buildManifestMismatch + ) { @@ - this.currentMeta!.lastBackupName = filename; - this.currentMeta!.lastBackupHash = resolvedBackupFileHash; - this.currentMeta!.lastBackupDate = backupDate.toISOString(); - - if (resolvedAnalysis.mainHash) { - this.currentMeta!.lastMainHash = resolvedAnalysis.mainHash; - } else { - delete this.currentMeta!.lastMainHash; - } - - await this.syncMeta(); - - return await fs.writeFile(path.resolve(this.backupFolder, filename), backupFile).finally(() => { - this.logger.info(`Backup saved to ${filename}`); - }); + await fs.writeFile(path.resolve(this.backupFolder, filename), backupFile); + + this.currentMeta!.lastBackupName = filename; + this.currentMeta!.lastBackupHash = resolvedBackupFileHash; + this.currentMeta!.lastBackupDate = backupDate.toISOString(); + + if (resolvedAnalysis.mainHash) { + this.currentMeta!.lastMainHash = resolvedAnalysis.mainHash; + } else { + delete this.currentMeta!.lastMainHash; + } + + await this.syncMeta(); + this.logger.info(`Backup saved to ${filename}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dist.service.ts` around lines 581 - 610, The skip logic in the resolvedAnalysis check currently only compares mainHash and can discard server-produced backups even when versionManifestHashMismatches or buildManifestMismatch indicate on-disk divergence; update the early-return condition in the block that references resolvedAnalysis (e.g., check resolvedAnalysis.versionManifestHashMismatches.length === 0 && !resolvedAnalysis.buildManifestMismatch in addition to mainHash equality) so the skip only occurs for a clean analysis. Also change the save flow so fs.writeFile(path.resolve(this.backupFolder, filename), backupFile) is awaited first and only on successful resolution update this.currentMeta (lastBackupName/lastBackupHash/lastBackupDate/lastMainHash), call await this.syncMeta(), and only then call logger.info(`Backup saved to ${filename}`); ensure the .finally() no longer unconditionally logs success and that errors from fs.writeFile propagate or are handled before metadata is persisted.
416-454:⚠️ Potential issue | 🟠 MajorPath traversal via VERSION manifest entries still unguarded.
relativePathkeys here come from the untrusted backup'sVERSION.filesmap, not from the extracted file list. A crafted manifest with an entry like../../etc/passwdwill causepath.join(contentRootDir, relativePath)at line 431 to read outside the extracted directory. Validate that each normalized path is relative and stays withincontentRootDirbefore recording it or reading the file.🛡️ Proposed fix
for (const [relativePath, expectedHash] of Object.entries(manifest.files)) { if (typeof expectedHash !== 'string' || !expectedHash.trim()) { throw new Error(`VERSION manifest contains invalid hash for file: ${relativePath}`); } - normalizedManifestFiles[this.pathToPosix(relativePath)] = expectedHash; + const normalizedRelativePath = this.pathToPosix(relativePath); + + if ( + !normalizedRelativePath || + path.isAbsolute(relativePath) || + path.win32.isAbsolute(relativePath) || + normalizedRelativePath.split('/').includes('..') + ) { + throw new Error(`VERSION manifest contains unsafe file path: ${relativePath}`); + } + + normalizedManifestFiles[normalizedRelativePath] = expectedHash; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dist.service.ts` around lines 416 - 454, The manifest-relativePath entries are untrusted and can escape contentRootDir; update the loop that processes manifest.files/normalizedManifestFiles (symbols: manifest.files, normalizedManifestFiles, pathToPosix, buildManifestChecksum, isTemplateVariablesBasename) to validate each normalized path before saving or reading: compute resolved = path.resolve(contentRootDir, normalizedPath) and compare against contentRootRootResolved = path.resolve(contentRootDir) (use a trailing path.sep when comparing) to ensure resolved startsWith(contentRootRootResolved + path.sep) (and reject absolute paths or any path that would navigate above the root); if validation fails, throw or skip the entry so you never call fs.readFile on an escaped path. Also ensure pathToPosix is applied consistently when checking/recording entries.
🧹 Nitpick comments (3)
src/lib/pp.middleware.ts (1)
155-157: Minor: escaping/is unnecessary.
/has no special meaning in a JS regex pattern (only as the literal delimiter in regex literals, not insidenew RegExp(...)); including it in the character class is harmless but slightly misleading. This matches the helper incli.ts(line 115) anddist.service.ts(line 250) which correctly omit/.♻️ Proposed change
`#escapeRegExp`(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\\/]/g, '\\$&'); + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/pp.middleware.ts` around lines 155 - 157, The character class in the private method `#escapeRegExp` currently includes an unnecessary '/' character; update the regex in `#escapeRegExp`(value: string): string to remove the '/' from the character class (so it matches other special regex chars but not '/'), keeping the same replace logic and return value to maintain behavior consistent with cli.ts and dist.service.ts helpers.src/lib/client.service.ts (1)
268-395: Mismatch prompting ordering is intricate — worth a brief test.The interplay between
onlyTemplateVariablesMismatch,mixedTemplateAndOthers,pathsForVersionModal, and the BUILD-MANIFEST-mismatch-only prompt (gated onversionManifestHashMismatches.length === 0) is correct but non-obvious. Because user-facing behavior differs across four cases (only-template-vars, mixed, other-only, BUILD-MANIFEST-only), this is a good candidate for a unit test ononTemplateSyncthat exercises eachBackupAnalysisshape and asserts the expectedrequestSyncAction/template:sync:responsesequence.Want me to draft a test matrix for these four mismatch scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client.service.ts` around lines 268 - 395, Add unit tests for onTemplateSync that cover the four user-facing scenarios: onlyTemplateVariablesMismatch, mixedTemplateAndOthers, other-only (pathsForVersionModal non-empty), and BUILD-MANIFEST-only (backupAnalysis.buildManifestMismatch true with versionManifestHashMismatches.length === 0). For each test, construct a BackupAnalysis shape exercising the specific booleans/arrays (use backupAnalysis.versionManifestHashMismatches, backupAnalysis.unknownFiles, backupAnalysis.templateVariables, and backupAnalysis.buildManifestMismatch), stub requestSyncAction to capture prompts and server.ws.send to capture responses, then assert the exact sequence of calls: whether promptReplaceTemplateVariables (indirectly via requestSyncAction) runs before/after version prompts, whether requestSyncAction was invoked with the VERSION list (pathsForVersionModal), and whether BUILD-MANIFEST prompt occurs only when expected; reuse promptReplaceTemplateVariables, onlyTemplateVariablesMismatch, mixedTemplateAndOthers, pathsForVersionModal, and buildManifestMismatch as identifiers to target behavior.src/cli.ts (1)
842-848: Use per-emittersetMaxListeners(20)on thehttpsAgentinstead of bumping the process-wide default.The httpsAgent that triggers the listener warnings is created inside MiAPI's constructor (src/lib/pp.middleware.ts). Rather than bumping
EventEmitter.defaultMaxListenersglobally in cli.ts, apply the limit directly to the agent instance:const httpsAgent = new https.Agent({ keepAlive: false, ...(disableSSLValidation ? { rejectUnauthorized: false } : {}), }); httpsAgent.setMaxListeners(20);This approach is more targeted, clearer in intent, and avoids masking potential listener leaks elsewhere in the application. Remove the process-wide bump from cli.ts:846 once the per-emitter approach is in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 842 - 848, The code currently raises the process-wide EventEmitter.defaultMaxListeners in cli.ts (EventEmitter.defaultMaxListeners) which is too broad; instead update the https agent created in MiAPI's constructor (the httpsAgent inside the MiAPI constructor in src/lib/pp.middleware.ts) to call httpsAgent.setMaxListeners(20) after instantiation (and include the disableSSLValidation option as needed), then remove the global EventEmitter.defaultMaxListeners bump in cli.ts so only the specific httpsAgent emitter has its listener limit increased.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/dist.service.ts`:
- Around line 581-610: The skip logic in the resolvedAnalysis check currently
only compares mainHash and can discard server-produced backups even when
versionManifestHashMismatches or buildManifestMismatch indicate on-disk
divergence; update the early-return condition in the block that references
resolvedAnalysis (e.g., check
resolvedAnalysis.versionManifestHashMismatches.length === 0 &&
!resolvedAnalysis.buildManifestMismatch in addition to mainHash equality) so the
skip only occurs for a clean analysis. Also change the save flow so
fs.writeFile(path.resolve(this.backupFolder, filename), backupFile) is awaited
first and only on successful resolution update this.currentMeta
(lastBackupName/lastBackupHash/lastBackupDate/lastMainHash), call await
this.syncMeta(), and only then call logger.info(`Backup saved to ${filename}`);
ensure the .finally() no longer unconditionally logs success and that errors
from fs.writeFile propagate or are handled before metadata is persisted.
- Around line 416-454: The manifest-relativePath entries are untrusted and can
escape contentRootDir; update the loop that processes
manifest.files/normalizedManifestFiles (symbols: manifest.files,
normalizedManifestFiles, pathToPosix, buildManifestChecksum,
isTemplateVariablesBasename) to validate each normalized path before saving or
reading: compute resolved = path.resolve(contentRootDir, normalizedPath) and
compare against contentRootRootResolved = path.resolve(contentRootDir) (use a
trailing path.sep when comparing) to ensure resolved
startsWith(contentRootRootResolved + path.sep) (and reject absolute paths or any
path that would navigate above the root); if validation fails, throw or skip the
entry so you never call fs.readFile on an escaped path. Also ensure pathToPosix
is applied consistently when checking/recording entries.
In `@src/lib/pp.middleware.ts`:
- Around line 250-252: The check treating template IDs as present uses typeof
page.template_id !== 'undefined', which is true for null; update the presence
checks to use page.template_id != null (and the equivalent pageInfo.template_id
!= null) so null is considered absent, update the assignment to
this.#isV710OrHigher to use that null-safe check (occurences that set
`#isV710OrHigher` and the earlier check around the same logic), and also add
explicit pageInfo.template_id != null guards before calling
pageTemplateApi.get(...) to avoid passing null into those calls.
- Around line 86-111: Remove the process-wide TLS and axios-default mutations:
delete the assignment to process.env.NODE_TLS_REJECT_UNAUTHORIZED and the
assignment to axios.defaults.httpsAgent so TLS relaxation is not applied
globally; keep the per-instance httpsAgent creation (with
rejectUnauthorized:false when disableSSLValidation is true) and pass that agent
into this.#axios so only the MiAPI instance uses the relaxed TLS behavior (refer
to disableSSLValidation, httpsAgent, and this.#axios in the constructor).
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 842-848: The code currently raises the process-wide
EventEmitter.defaultMaxListeners in cli.ts (EventEmitter.defaultMaxListeners)
which is too broad; instead update the https agent created in MiAPI's
constructor (the httpsAgent inside the MiAPI constructor in
src/lib/pp.middleware.ts) to call httpsAgent.setMaxListeners(20) after
instantiation (and include the disableSSLValidation option as needed), then
remove the global EventEmitter.defaultMaxListeners bump in cli.ts so only the
specific httpsAgent emitter has its listener limit increased.
In `@src/lib/client.service.ts`:
- Around line 268-395: Add unit tests for onTemplateSync that cover the four
user-facing scenarios: onlyTemplateVariablesMismatch, mixedTemplateAndOthers,
other-only (pathsForVersionModal non-empty), and BUILD-MANIFEST-only
(backupAnalysis.buildManifestMismatch true with
versionManifestHashMismatches.length === 0). For each test, construct a
BackupAnalysis shape exercising the specific booleans/arrays (use
backupAnalysis.versionManifestHashMismatches, backupAnalysis.unknownFiles,
backupAnalysis.templateVariables, and backupAnalysis.buildManifestMismatch),
stub requestSyncAction to capture prompts and server.ws.send to capture
responses, then assert the exact sequence of calls: whether
promptReplaceTemplateVariables (indirectly via requestSyncAction) runs
before/after version prompts, whether requestSyncAction was invoked with the
VERSION list (pathsForVersionModal), and whether BUILD-MANIFEST prompt occurs
only when expected; reuse promptReplaceTemplateVariables,
onlyTemplateVariablesMismatch, mixedTemplateAndOthers, pathsForVersionModal, and
buildManifestMismatch as identifiers to target behavior.
In `@src/lib/pp.middleware.ts`:
- Around line 155-157: The character class in the private method `#escapeRegExp`
currently includes an unnecessary '/' character; update the regex in
`#escapeRegExp`(value: string): string to remove the '/' from the character class
(so it matches other special regex chars but not '/'), keeping the same replace
logic and return value to maintain behavior consistent with cli.ts and
dist.service.ts helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af960c61-d286-4f53-a4e7-6ffb4559a62a
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontests/test-nextjs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
CHANGELOG.mdpackage.jsonsrc/cli.tssrc/client/index.tssrc/lib/client.service.tssrc/lib/dist.service.tssrc/lib/pp.middleware.tssrc/plugins/version-plugin.tssrc/shortcuts.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
MaksymovVolodymyr
approved these changes
Apr 22, 2026
2 tasks
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.
Release: merge develop into main (0.18.1-beta.1 and accumulated work)
Summary
This merge brings
origin/developintoorigin/main, promoting the current beta line and all integrated work: core refactors (dist/client services, proxy stack, CLI), tooling (Prettier, Dependabot), build and dependency updates, documentation, and test alignment. The branch includes the release commitchore(release): 0.18.1-beta.1 [skip ci]and 9 preceding feature/maintenance commits. Diff vsmain: 105 files, roughly +2.9k / -2.7k lines.Key changes
unavailable-json-apiand client asset typings.package.json/ lockfile, and semantic-release-style 0.18.1-beta.1 bump ondevelop.Stats
Included commits (
origin/main..origin/develop, no merges)035ee09— chore(deps): patch npm audit vulnerabilities4de05ed— fix(cli): harden shortcut cleanup and add dts trace logging8edd465— chore(test): refresh commonjs fixture lockfile42d52b3— chore(tooling): add Prettier configurationd4780d9— chore(ci): update Dependabot configurationcaf2b98— chore(build): update dependencies, bundler configs, scripts, and test runners212c555— docs: refresh README, changelog, and testing documentation277ed89— refactor(core): improve dist and client services, proxy stack, CLI, and pluginsb2be1bd— test: align e2e, integration, unit suites, and sandbox fixtures01e73a7— chore(release): 0.18.1-beta.1 [skip ci]Testing / release checklist
developbefore mergenpm test/ project test scripts on a clean checkouttests/test-nextjs*,tests/test-commonjs) if part of release processBreaking changes / compatibility
mainfor downstream installs.Summary by CodeRabbit
New Features
Bug Fixes
Chores