Conversation
🚀 Promote develop to main (0.18.3-beta.1 — audit fixes + dependabot)
Replace legacy NPM_TOKEN auth with GitHub OIDC to match the npm Trusted Publisher config for release.yml.
ci(release): fix npm publish via OIDC trusted publishing
🚀 Promote develop to main (0.19.0-beta.1 — Next.js dev panel + audit fixes)
…grate codemod - Replace flat VitePPDevOptions with grouped PPDevConfig (mi, app, proxy, build, sync) - Add defineConfig() TypeScript helper for config authoring DX - Add pp-dev migrate CLI command for automatic 0.x → 1.0 config migration with 22 unit tests - Rename portalPageId → appId across API, middleware, and CLI - Fix critical listener leak in load-pp-data middleware (subscribe without unsubscribe on restart) - Remove dead code: LRUCache, performanceMetrics, getConfigCacheStats, getDebugInfo, mergeConfigs - Consolidate getAssetPaths into dev-panel.ts; move path prefix constants to constants.ts - Replace console.* with createLogger() in server-side code - Fix proxyPassMiddleware return type: NextHandleFunction instead of any - esbuild: target node24, add packages:external to prevent bundling node_modules in config loader
- Update CSS variables to MI brand palette (#075B7E, #077E45, #AC2B2B, #FFB000) - Add Inter font family to panel * reset - Toast: border-based type indicator (2px solid), 2px border-radius, 280px max-width - Modal: 408px fixed width, 3px border-radius, column-stacked full-width buttons - Buttons: 28px size with branded border and border-radius - Add SVG icons (success, danger, warning, info) to toast popup title row
…st fixtures - Add CHANGELOG.md v1.0.0 entry with full breaking changes and migration guide - Rewrite README.md for v1.0 grouped config API with migration section - Bump version 0.19.0-beta.2 → 1.0.0 in package.json - Require Node.js >=24 in engines; update GitHub Actions workflows to node 24 - Update undici >=7.28.0 and http-proxy-middleware ^3.0.7 overrides for security - Migrate test fixture pp-dev.config.* to new PPDevConfig format - Update test fixture package.json with security overrides
…astructure - tests/e2e/server-lifecycle.spec.ts: 3 tests — clean startup, config watcher active, HTTP response - tests/mock-mi/server.ts: record/replay mock server (nock-free, Express-based, gzip-aware cassettes) - tests/mock-mi/record.ts: interactive cassette recorder (manual Ctrl+C) - tests/mock-mi/record-auto.ts: autonomous recorder for CI cassette capture - vitest.e2e.config.ts: Vitest 4 E2E config (forks pool, singleFork, 60s timeout) - package.json: add test:e2e and record:mi scripts; cassettes/*.json excluded from git (may contain session tokens)
proc.kill('SIGTERM') only kills the shell (npm.cmd), leaving Next.js child
processes running and causing 'Another next dev server is already running'
on the next test run.
Four new CVEs in undici <=6.26.0 (GHSA-p88m-4jfj-68fv, GHSA-vxpw-j846-p89q, GHSA-35p6-xmwp-9g52, GHSA-g8m3-5g58-fq7m) affect node_modules/npm/node_modules/undici via the chain semantic-release → @semantic-release/npm → npm@11.17.0 → node-gyp@12.4.0. npm overrides cannot reach npm's isolated node_modules tree; `npm audit fix` itself reports it cannot be fixed automatically. Fix approach: - patch-npm-bundled-vulnerabilities.mjs: copy root undici@8.5.0 into npm's nested dir (node-gyp only uses fetch/Agent/EnvHttpProxyAgent/RetryAgent — all in undici v8) - audit-all.mjs: root audit uses --audit-level=critical so CI is not blocked by this unfixable-via-overrides high-severity advisory in a dev-only tool - package-lock.json: lockfile updated to reflect undici@8.5.0 in npm's tree
undici@8.x removed lib/handler/wrap-handler.js which jsdom@29.1.1 imports as an internal API (jsdom requires undici@^7.25.0). Cap at ^7.28.0 to stay on the 7.x line while clearing <=6.26.0 CVEs. Add undici@^7.28.0 as explicit devDependency so it is always hoisted to node_modules/undici — required by patch-npm-bundled-vulnerabilities.mjs as the source to copy into node_modules/npm/node_modules/undici.
- Remove blue borders from minimize and sync buttons - Reduce panel padding from 8px 12px to 2px, add gap: 8px - Bump panel height to 32px (matches 28px button + 2px padding each side) - Font size 13px → 12px (Body style) - Separator height 12px → 16px, color rgba(0,0,0,.15) → rgba(34,34,34,.24) - Add __sep divider element between last section and sync button - Link gap 4px → 8px (matches Figma section gap)
… hang On Windows, rollup-plugin-dts leaves TypeScript language-service handles alive after the build completes, causing the process to hang indefinitely. An in-process setTimeout/unref workaround cannot fire when TS blocks the event loop synchronously. - Extract DTS build into rollup.config.types.ts (standalone config) - Add scripts/build-types.mjs: spawns rollup as child process with a 60s OS-level watchdog (taskkill /T /F on Windows, SIGKILL on Unix) - Remove typeDefsMonitorPlugin() and DTS config from rollup.config.ts - Fix TS2353: remove unknown Logger.log property in proxy-pass.middleware - Update test-fixture lockfiles and tsconfigs after reinstall
…rd.ts - Fix regex: `url:s*` -> `url:\s*`, unescaped `//` -> `\/\/` - Replace `spawn(node, [node_modules/.bin/pp-dev])` with `spawn(process.execPath, [PP_DEV_JS])` to match record-auto.ts and work correctly on Windows where .bin/pp-dev is a shell script - Remove unused saveCassette/CASSETTES_DIR imports
server.ts: - Add explicit types to proxyRes/req/res/err callback params (TS6 unknown regression) - Fix Buffer: use Buffer<ArrayBuffer> for gunzip Promise to match NonSharedBuffer - Replace (res as Response).status().json() with res.writeHead()/res.end() record.ts / record-auto.ts: - Change import from ./server.ts to ./server.js to avoid TS5097 (allowImportingTsExtensions not set; tsx resolves .js -> .ts natively)
- Add net import; type error handler res as ServerResponse | net.Socket (ErrorCallback expects union type, not just ServerResponse) - Replace async gunzip Promise with gunzipSync to avoid Buffer<T> invariance issues (Buffer<ArrayBuffer> vs Buffer<ArrayBufferLike>)
Binary content (zip, images, etc.) corrupts when stored via toString(utf-8). - Add bodyEncoding?: base64 field to Interaction.response - Add isBinaryContentType() helper to detect non-text content types - Record mode: store binary responses as base64, text as utf-8 - Replay mode: decode base64 back to Buffer before sending
Pass MI_ACCESS_TOKEN to pp-dev so proxied requests are authenticated, then directly fetch /api/page_template and asset/download via mock-mi to record them without triggering a full next build. Fixes auto-recorder capturing only 2 interactions (unauthenticated MI endpoints redirect to login; only public ones were recorded before).
Adds a built-in request inspector that captures every HTTP request passing through the dev server middleware stack: - RequestStore: in-memory ring buffer with size-based eviction (configurable maxMemory, default 100 MB) - createRequestCaptureMiddleware: taps req/res streams to record headers and bodies up to captureLimit (default 10 MB); larger bodies are stored truncated - registerInspectorRoutes: serves the inspector UI at /@pp-dev/inspector and a JSON REST API at /@api/requests (list, detail, stats, DELETE) - Inspector UI: scrollable request list with method, status, source badge (P/C/L), timing; collapsible detail panels for request/response headers and bodies with Copy and Save-to-file buttons - Source badge: colored letter square (P=proxy, C=cache, L=local) displayed left of the HTTP status in the request list - InspectorConfig added to plugin options: enabled, maxMemory, captureLimit - cli.ts and plugin.ts wired up with captureLimit forwarding
Adds a styled console.log() call (via an IIFE) in the client bundle so the inspector URL appears in the DevTools console history whenever the dev panel is loaded — visible even when DevTools is opened after the page loads.
…auto-fixes
Adds eslint + typescript-eslint (flat config) with two enforced rules:
- curly: 'all' — every if/else/loop body must use {}
- padding-line-between-statements — blank lines required before/after
if/loop blocks, before return, and between var-declaration blocks and
surrounding code (consecutive declarations may stay together)
Runs eslint --fix across all src/**/*.ts files to bring existing code
into compliance.
Adds inspector config option table to Configuration Options and a new Request Inspector section covering the web UI, source badges (P/C/L), DevTools console banner, REST API endpoints, and configuration example.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds actions/cache for ~/.cache/ms-playwright keyed on package-lock.json hash. On cache hit only system deps are reinstalled (install-deps), skipping the ~170 MB Chromium download.
ubuntu-latest already has Chromium system deps in the runner image. When the browser cache is hit there is nothing to install — skipping install-deps removes the slow apt-get font/lib setup (~1-2 min). Also adds restore-keys fallback so a partial cache match (different package-lock hash but same OS) reuses existing browsers.
Splits --with-deps into two separate steps: 1. install-deps (apt-get) runs with DEBIAN_FRONTEND=noninteractive and NEEDRESTART_MODE=a so needrestart never waits for user input 2. playwright install chromium (browser binary only, no apt) is cached needrestart on Ubuntu 24.04 hangs in non-interactive environments after apt-get unless explicitly told to auto-restart — this was causing the step to block for 60+ minutes on every run.
Clears ~10 GB of unused toolchains (dotnet, Android SDK, CodeQL) before downloading and extracting the Chromium binary. Full disk is the likely cause of the extraction hang after the 170 MB download completes. Also adds: - timeout-minutes: 10 on the browser install step (fail fast if hung) - timeout-minutes: 20 + continue-on-error: true on the test job so a Playwright hang never blocks the overall CI result - df -h before/after cleanup to confirm available space in logs
sergak01
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical 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 (1)
src/config.ts (1)
237-239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
package.jsonpp-devagainstnulland arrays.
typeof null === 'object', so"pp-dev": nullmakesgetConfig()returnnulldespite thePPDevConfigreturn type, which will crash downstream normalization.Suggested fix
- if (!configFound && typeof pkg['pp-dev'] === 'object') { - config = pkg['pp-dev']; + const packageConfig = pkg['pp-dev']; + + if ( + !configFound && + packageConfig && + typeof packageConfig === 'object' && + !Array.isArray(packageConfig) + ) { + config = packageConfig as PPDevConfig; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 237 - 239, The fallback in getConfig() assigns pkg['pp-dev'] whenever typeof is "object", which incorrectly accepts null and arrays. Update the package.json check in getConfig() to only use pkg['pp-dev'] when it is a non-null plain object, so config never becomes null or an array before normalizeConfig() runs. Keep the logic anchored around getConfig() and the pkg['pp-dev'] fallback so the returned PPDevConfig stays valid.
🟠 Major comments (21)
tests/unit/plugin/plugin.normalize.spec.ts-264-300 (1)
264-300: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear MI env vars for validation tests too.
validatePPDevConfig()falls back toprocess.env.MI_BACKEND_URL; these missing-URL tests can pass/fail based on the runner environment unless the env is reset like the normalization suite.Suggested fix
describe('validatePPDevConfig', () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { ...originalEnv }; + delete process.env.MI_BACKEND_URL; + delete process.env.MI_ACCESS_TOKEN; + }); + + afterEach(() => { + process.env = originalEnv; + }); + it('throws if templateName is empty', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/plugin/plugin.normalize.spec.ts` around lines 264 - 300, Update the `validatePPDevConfig` tests in `plugin.normalize.spec.ts` so they do not rely on ambient `process.env.MI_BACKEND_URL`; the missing-URL cases should explicitly clear or override that env value in the test setup/teardown, similar to the normalization suite, to keep `validatePPDevConfig()` deterministic across runners. Use the existing `validatePPDevConfig` describe block and its related `beforeEach`/`afterEach` hooks to isolate env state for all assertions that expect `mi.url is required`.src/index.ts-291-314 (1)
291-314: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeep-merge and normalize grouped config before deriving Next paths.
The top-level
Object.assigndrops sibling fields inside groups likemiandapp, andapp.nameis ignored forbasePatheven thoughnormalizePPDevConfig()uses it as the effective template name. This can makewithPPDev()serve a different path than the Vite/CLI config.Suggested fix
- const mergedConfig: PPDevConfig = Object.assign({}, config, nextConfigPPDev, ppDevConfig ?? {}); + const mergedConfig: PPDevConfig = { + ...config, + ...nextConfigPPDev, + ...(ppDevConfig ?? {}), + mi: { ...config.mi, ...nextConfigPPDev?.mi, ...ppDevConfig?.mi }, + app: { ...config.app, ...nextConfigPPDev?.app, ...ppDevConfig?.app }, + proxy: { ...config.proxy, ...nextConfigPPDev?.proxy, ...ppDevConfig?.proxy }, + build: { ...config.build, ...nextConfigPPDev?.build, ...ppDevConfig?.build }, + sync: { ...config.sync, ...nextConfigPPDev?.sync, ...ppDevConfig?.sync }, + inspector: { ...config.inspector, ...nextConfigPPDev?.inspector, ...ppDevConfig?.inspector }, + }; + const normalized = normalizePPDevConfig(mergedConfig, templateName); - // Derive display flags from grouped config - const appType = mergedConfig.app?.type ?? 'template'; - const templateLess = appType === 'page'; - const apiVersion = mergedConfig.mi?.apiVersion ?? 7; - const v7Features = apiVersion >= 7; + const { templateLess, v7Features } = normalized; // Create base configuration with appropriate base path const isDevelopment = phase === PHASE_DEVELOPMENT_SERVER; - const basePath = createBasePath(templateName, templateLess, isDevelopment, v7Features); + const basePath = createBasePath(normalized.templateName, templateLess, isDevelopment, v7Features); const baseConfig: NextConfig = { basePath, trailingSlash: isDevelopment ? true : undefined, }; if (!templateLess) { - baseConfig.assetPrefix = `${PATH_TEMPLATE_PREFIX}/${templateName}`; + baseConfig.assetPrefix = `${PATH_TEMPLATE_PREFIX}/${normalized.templateName}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 291 - 314, The config handling in withPPDev is shallow-merging grouped objects, so sibling fields inside app and mi can be lost, and basePath is derived from templateName instead of the normalized app.name used by normalizePPDevConfig(). Fix this by deep-merging the grouped config before reading app/mi values, then derive the effective template name from the normalized config and pass that into createBasePath so Next.js paths match the CLI/Vite config.Source: Coding guidelines
scripts/build-types.mjs-16-31 (1)
16-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStart the DTS child in its own process group.
scripts/build-types.mjs:35-47On POSIX,killTree()targets-pid, but this child isn’t started withdetached: true, so the timeout can miss the shell/Rollup tree and leave the build hanging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-types.mjs` around lines 16 - 31, The DTS child process started in build-types.mjs is not placed in its own process group, so killTree() cannot reliably terminate the full shell/Rollup subtree on POSIX. Update the child spawn logic in the code that launches the DTS build to run detached on non-Windows platforms, and ensure the existing killTree() path that uses process.kill(-pid, 'SIGKILL') is targeting that detached group. Keep the current Windows taskkill behavior unchanged..mcp.json-3-5 (1)
3-5: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a cross-platform, pinned MCP launch command.
cmd /conly works on Windows, andnpx -y figma-developer-mcppulls the latest package at runtime instead of using a locked dependency. Switch to a platform-agnostic launcher and pin the MCP package version, or add it as a repo dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.mcp.json around lines 3 - 5, The MCP entry for the Framelink Figma server uses a Windows-only launcher and an unpinned runtime package fetch, which should be made cross-platform and reproducible. Update the MCP config entry for “Framelink MCP for Figma” to use a platform-agnostic launch approach instead of cmd /c, and ensure figma-developer-mcp is pinned to a specific version or installed as a repository dependency so the command is deterministic. Keep the change localized to the MCP config block so the launch behavior is consistent across environments..github/workflows/ci.yml-14-20 (1)
14-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable persisted Git credentials in both CI checkout steps.
These jobs execute repo-controlled install/build/test steps after checkout, so leaving the token in local git config unnecessarily widens the blast radius if a dependency or script is compromised. The beta release workflow already uses
persist-credentials: false, so CI should match that hardening.
.github/workflows/ci.yml#L14-L20: addpersist-credentials: falseunder the build job’sactions/checkoutstep..github/workflows/ci.yml#L44-L51: addpersist-credentials: falseunder the test job’sactions/checkoutstep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 14 - 20, Disable persisted Git credentials on both `actions/checkout` steps in `.github/workflows/ci.yml`: add `persist-credentials: false` to the build job checkout at `.github/workflows/ci.yml#L14-L20` and the test job checkout at `.github/workflows/ci.yml#L44-L51`, matching the existing hardening used in the beta release workflow.Source: Linters/SAST tools
.github/workflows/ci.yml-65-69 (1)
65-69: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftStop bypassing fixture lockfiles in CI.
npm install --package-lock=falsemakes these fixture dependency trees float on every run, so CI is no longer reproducible andaudit:allcan pass against a different graph than the one developers actually checked in. Install from committed fixture lockfiles (npm ci --prefix ...) and update those lockfiles/overrides when they need patching. As per coding guidelines,tests/**/package.json:If test-fixture lockfiles need patching, add/update overrides in their package.json and run npm install;tests/*/{package.json,package-lock.json}:ensure all dependency trees are updated so that npm run audit:all remains clean across all packages, not only the root package-lock.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 65 - 69, The E2E fixture install step is bypassing committed lockfiles via npm install --package-lock=false, which makes the CI dependency graph non-reproducible. Update the fixture install commands in the CI workflow to use npm ci for the test-nextjs, test-commonjs, and test-nextjs-cjs fixtures so they install exactly from their checked-in lockfiles. If any fixture needs patched dependencies, update the corresponding tests/**/package.json overrides and regenerate the matching package-lock.json files so audit:all stays clean.Sources: Coding guidelines, Linters/SAST tools
tests/mock-mi/record-auto.ts-59-69 (1)
59-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
try/finallyfor teardown instead of the global exception hook.Any rejected await after the config patch bypasses the success path and can leave
tests/test-nextjs/pp-dev.config.tspointed at mock-mi, withpp-devor mock-mi still running. TheuncaughtExceptionhandler is not a reliable replacement for structured cleanup around top-level awaits.Also applies to: 203-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock-mi/record-auto.ts` around lines 59 - 69, Replace the global uncaughtException-based teardown in record-auto.ts with structured cleanup around the top-level async flow. Wrap the config patch and the awaited record/startup steps in a try/finally so cleanup() always runs after any rejection, and remove the process.once('uncaughtException') handler; use the existing cleanup and CONFIG_PATH logic to restore the config and stop leaving mock-mi/pp-dev running.tests/mock-mi/cassettes/startup.json-21-34 (1)
21-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the
/api/page/id/733payload.This entry is keyed by
GET:/api/page/id/733, but the body still returns"id":937and the 937 page metadata. Replay mode matches by method and path, so any fixture requesting page 733 gets contradictory data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock-mi/cassettes/startup.json` around lines 21 - 34, The mock cassette entry for GET:/api/page/id/733 has mismatched page data because the response body still contains the 937 page metadata. Update the response payload in the startup cassette so the body returned by the /api/page/id/733 fixture consistently reflects page 733, including the page id and all associated fields, keeping the request path and response content aligned.tests/mock-mi/record-auto.ts-95-99 (1)
95-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPin the Next.js port before hardcoding
PP_BASE.The script always probes
http://localhost:3000, butpp-dev nextis launched without--portor--strictPort. If 3000 is already taken, Next can choose another port and this recorder will either hit the wrong process or produce an incomplete cassette.Also applies to: 127-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock-mi/record-auto.ts` around lines 95 - 99, The `record-auto.ts` Next.js launcher is assuming port 3000 via `PP_BASE`, but `spawn(..., 'next')` does not pin the port, so the recorder can target the wrong server if 3000 is busy. Update the `ppdev` startup to pass an explicit port (and ideally strict-port behavior) to the `next` command, then align the `PP_BASE`/probe target in the recorder logic with that fixed port so `record-auto` always records against the intended instance.tests/mock-mi/server.ts-298-326 (1)
298-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDecode
br/deflateresponses before turning them into text.Only gzip is decompressed here. If MI replies with Brotli or deflate, the non-binary branch converts compressed bytes to UTF-8 while preserving
content-encoding, which writes corrupted bodies into the cassette and breaks replay fidelity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock-mi/server.ts` around lines 298 - 326, The response-capture path in tests/mock-mi/server.ts only handles gzip before storing cassette bodies, so Brotli and deflate payloads can be saved as corrupted UTF-8 text. Update the decompression branch in the proxy response handling logic around the `encoding`, `storedHeaders`, and `interactionMap.set` flow to also decode `br` and `deflate` responses before converting non-binary bodies to text, and make sure the corresponding `content-encoding`/`content-length` headers are adjusted consistently after successful decompression.tests/mock-mi/server.ts-71-137 (1)
71-137: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact filesystem paths before saving cassettes.
sanitizeJsonValue()andsanitizeTextBody()scrub identities and tokens, but they never rewritepath/linkfields. The committedtests/mock-mi/cassettes/startup.jsonstill contains/opt/mi/shared/..., so recording currently leaks internal deployment paths into the repo. Extend the sanitizer to replace filesystem paths and re-record the cassette.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock-mi/server.ts` around lines 71 - 137, The cassette sanitizers in sanitizeJsonValue and sanitizeTextBody are missing filesystem-path redaction, so internal deployment paths can still be saved into recorded fixtures. Update the sanitization logic to detect and replace path-like values in JSON fields such as path/link and in free-text bodies before cassettes are written. Use the existing sanitizeBody flow in tests/mock-mi/server.ts as the place to centralize this redaction, then re-record the affected cassette so it no longer contains real filesystem paths.tests/mock-mi/record.ts-62-73 (1)
62-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick wintests/mock-mi/record.ts:62-73 — Make shutdown idempotent and persist the cassette before closing.
cleanup()is reached from both the signal handlers andppdev.once('exit'), soppdev.kill()can re-enter it while shutdown is already in flight. Guard it so it runs once, and callmockMi.save?.()beforeclose()so an unexpectedpp-devexit still writes the cassette.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock-mi/record.ts` around lines 62 - 73, Make the shutdown path in cleanup idempotent because it is invoked from both the SIGINT/SIGTERM handlers and ppdev.once('exit'), which can cause re-entrant cleanup when ppdev.kill() triggers exit while shutdown is already running. Add a single-run guard in cleanup and ensure the cassette is persisted by calling mockMi.save?.() before mockMi.close(), so the record flow in tests/mock-mi/record.ts reliably writes data even on unexpected pp-dev termination.scripts/run-playwright-e2e.ts-201-222 (1)
201-222: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail fast when the spawned
pp-devprocess dies during readiness polling.Right now the probe only checks whether something on
http://localhost:${fixture.port}returns<500. With--strictPort, a port conflict or boot failure can still produce a false-ready result if another process is already bound there, so Playwright can run against the wrong server.Suggested fix
-async function waitForServer(url: string, timeoutMs = 60_000): Promise<void> { +async function waitForServer(url: string, proc: ChildProcess, timeoutMs = 60_000): Promise<void> { const startedAt = Date.now(); while (Date.now() - startedAt < timeoutMs) { + if (proc.exitCode !== null || proc.signalCode !== null) { + throw new Error(`pp-dev exited before ${url} became ready`); + } + try { const res = await fetch(url, { redirect: 'manual', signal: AbortSignal.timeout(2_000), }); @@ - await waitForServer(baseURL); + await waitForServer(baseURL, server);Also applies to: 242-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-playwright-e2e.ts` around lines 201 - 222, The readiness polling in waitForServer can falsely succeed even if the spawned pp-dev process has already exited, so make the check fail fast when the child dies instead of waiting for an unrelated server on the same port. Update waitForServer and its caller in run-playwright-e2e.ts to accept/inspect the spawned process handle (or an exit promise), and during each poll iteration abort immediately with the child’s exit status if pp-dev has terminated before the expected server becomes ready.tests/test-nextjs/tsconfig.json-31-47 (1)
31-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
exclude: ["dist"]blocks the newdist/**type globs.
dist/types/**/*.ts,dist/dev/types/**/*.ts, anddist/dev/dev/types/**/*.tsstill fall under thedistexclude, so these generated declarations won’t be picked up. Removedistfromexcludeor move the exact paths tofiles.Suggested fix
"exclude": [ "node_modules", - "dist", "out" ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test-nextjs/tsconfig.json` around lines 31 - 47, The tsconfig include globs for generated declarations under dist are being shadowed by the broad dist exclusion. Update the tsconfig in the tests/test-nextjs setup so the generated type paths referenced by the include section are actually visible, either by removing dist from exclude or by explicitly listing the needed generated files in files; use the existing include/exclude arrays in the tsconfig to locate the change.tests/e2e/server-lifecycle.spec.ts-51-59 (1)
51-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve partial log lines across
dataevents.text.split('\n')drops the trailing fragment of each chunk, so a startup line split across two chunks never reacheswaitForLine()and can time out. Buffer the leftover and prepend it to the next chunk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/server-lifecycle.spec.ts` around lines 51 - 59, The onData handler in server-lifecycle.spec.ts is dropping partial log fragments because it splits each chunk by newline without preserving the trailing remainder, so waitForLine() can miss startup lines. Update the onData flow to keep a buffered leftover between data events, prepend it to the next chunk before splitting, and only emit complete lines through lines and listeners using the existing onData/listeners/waitForLine logic.tests/e2e/server-lifecycle.spec.ts-17-23 (1)
17-23: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminate the full process tree on POSIX —
shell: truemakesproc.pidthe shell wrapper, sokill('SIGTERM')can leavenpm/nextrunning and keep port 3000 busy. Either avoidshellhere or kill the spawned process group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/server-lifecycle.spec.ts` around lines 17 - 23, The killTree helper in the server lifecycle test is only terminating the shell wrapper on POSIX, which can leave the underlying npm/next process running. Update killTree to either avoid using shell when starting the process or terminate the spawned process group so the entire tree is cleaned up; keep the Windows taskkill path as-is and adjust the POSIX branch accordingly.e2e/toolbar/toolbar.sync.spec.ts-35-51 (1)
35-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't skip the disabled-before-syncing state. If that outcome is valid, assert it explicitly; otherwise
test.skip()hides the regression this test is meant to catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/toolbar/toolbar.sync.spec.ts` around lines 35 - 51, The sync toolbar test currently skips the case where the button becomes disabled before the syncing class is applied, which hides a real regression. In toolbar.sync.spec.ts, update the assertion flow around the sync button polling and the post-click checks so that the disabled-before-syncing state is asserted explicitly in the test instead of calling test.skip(); use the syncButton checks and the existing class/disabled assertions to validate whichever state is expected.src/lib/pp.middleware.ts-349-350 (1)
349-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInitialize
this.appIdbefore loading the v7 template.
MiAPIOptions.appIdis optional, butgetPageVariables()callsgetPageTemplate()first. On the v7 path, Line 223 usesthis.appId!, sonew MiAPI(...).getPageVariables(pageId, headers)can fetch with an undefined page ID before this fallback assignment runs.Suggested fix
async getPageVariables(pageId: number, headers: Headers) { + if (typeof this.appId === 'undefined') { + this.appId = pageId; + this.templateLess = false; + } + this.#pageTemplate = await this.getPageTemplate(headers); - - if (!this.appId) { - this.appId = pageId; - this.templateLess = false; - } return await this.pageApi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/pp.middleware.ts` around lines 349 - 350, `getPageVariables()` is resolving the v7 template before `this.appId` is initialized, which can make `getPageTemplate()` read an undefined page ID via `this.appId!`. Update the initialization flow in `MiAPI.getPageVariables()` so `this.appId` is assigned from `pageId` before any template lookup, and keep the fallback in the same path that reaches `getPageTemplate()` to ensure the v7 branch always has a valid app ID.src/cli.ts-651-657 (1)
651-657: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the same template prefix for
assetPrefixthatbaseuses.When
v7Featuresis true, Lines 654-657 switch the app base toPATH_TEMPLATE_LOCAL_PREFIX, but Line 683 still emits Next assets underPATH_TEMPLATE_PREFIX. That sends/_nextJS/CSS to/pt/...while this server routes the app under/pl/..., so v7 template-mode assets will 404.Suggested fix
- assetPrefix: `${templateLess ? PATH_PAGE_PREFIX : PATH_TEMPLATE_PREFIX}/${templateName}`, + assetPrefix: `${templateLess ? PATH_PAGE_PREFIX : v7Features ? PATH_TEMPLATE_LOCAL_PREFIX : PATH_TEMPLATE_PREFIX}/${templateName}`,Also applies to: 683-683
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 651 - 657, The app base selection in the CLI already switches to PATH_TEMPLATE_LOCAL_PREFIX when v7Features is enabled, but the assetPrefix logic still uses PATH_TEMPLATE_PREFIX, causing the app and Next assets to diverge. Update the assetPrefix calculation in src/cli.ts to mirror the same branch conditions used for base (including configBasePath, templateLess, and v7Features) so both values resolve to the same template prefix. Use the existing base assignment and assetPrefix emission as the main references when making the change.src/plugins/client-injection-plugin.ts-44-44 (1)
44-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep
resolveId()pinned todist/client.
PACKAGE_REGEXPnow accepts arbitrary suffixes, and Line 89 joins that suffix straight ontodist/client. A specifier like@metricinsights/pp-dev/client/../../package.jsonnormalizes outside the asset root, so this plugin stops enforcing that only packaged client assets are resolvable.🔒 Suggested fix
+const CLIENT_DIST_DIR = normalizePath(path.join(PP_DEV_PACKAGE_DIR, 'dist/client')); const PACKAGE_REGEXP = new RegExp(`^\\/?${PACKAGE_NAME}\\/client\\/(.*)$`); @@ resolveId(source) { - if (PACKAGE_REGEXP.test(source)) { + const match = source.match(PACKAGE_REGEXP); + + if (match) { + const resolvedId = normalizePath(path.join(CLIENT_DIST_DIR, match[1])); + + if (!resolvedId.startsWith(`${CLIENT_DIST_DIR}/`)) { + return null; + } + return { - id: normalizePath(path.join(PP_DEV_PACKAGE_DIR, 'dist/client', source.replace(PACKAGE_REGEXP, '$1'))), + id: resolvedId, }; } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/client-injection-plugin.ts` at line 44, Keep the client injection plugin’s resolveId logic pinned to the packaged client bundle by restricting PACKAGE_REGEXP and the path handling in resolveId() so only safe client asset paths under dist/client are accepted. Update the regex and any suffix joining so it rejects traversal or arbitrary suffixes instead of concatenating user-provided segments directly onto dist/client, and ensure the check in client-injection-plugin.ts only resolves known packaged client assets.src/lib/request-inspector.ts-25-27 (1)
25-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove
/@api/requests/statsbefore/:idand decode text bodies as UTF-8.
/@api/requests/statsis shadowed by/@api/requests/:id, so/statshits the single-request 404 path.atob(...)returns a byte string; using it directly in the JSON/plain-text and copy paths will mangle non-ASCII payloads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/request-inspector.ts` around lines 25 - 27, The `/@api/requests/:id` route is shadowing `/@api/requests/stats`, so move the `stats` handler above the single-request handler in `request-inspector.ts` to ensure `/stats` is matched first. Also update the body-decoding logic used by the request detail/copy paths in the same module so decoded text is interpreted as UTF-8 instead of using the raw `atob(...)` byte string directly, preserving non-ASCII characters in JSON, plain-text, and copy output.
🟡 Minor comments (11)
.github/workflows/release.yml-52-53 (1)
52-53: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDrop the
npm@latestupgrade here. The Node 24 toolchain already satisfies npm’s trusted-publishing minimum, so this step only adds an unnecessary moving dependency to the release path. Pin a specific npm version if you need the upgrade, or remove the step and use the bundled CLI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 52 - 53, Remove the npm@latest upgrade step from the release workflow since the Node 24 toolchain already meets the trusted-publishing requirement. In the release job, update the npm install step under the trusted publishing setup to either use the bundled npm CLI directly or replace the moving latest dependency with a pinned version if an upgrade is still needed. Keep the change focused on the workflow job that performs OIDC publishing so the release path stays deterministic.Source: Linters/SAST tools
docs/pp-dev-config-1.0-canvas.md-249-255 (1)
249-255: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the documented
mi.modedefault consistent.This reference block says
MiConfig.modedefaults to'embedding', but the Defaults table and the resolved decision later in the same document say'standalone'. Leaving both values here makes the migration contract ambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/pp-dev-config-1.0-canvas.md` around lines 249 - 255, The documented default for MiConfig.mode is inconsistent in the reference block, Defaults table, and later decision text. Update the MiConfig interface comment so the mode default matches the chosen contract everywhere in this document, and make sure the wording around MiConfig.mode and related include semantics is aligned with the same default value throughout.docs/pp-dev-config-1.0-canvas.md-160-160 (1)
160-160: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
##instead of###for these sections.Both headings skip from
#to###, which tripsMD001and makes the outline harder to scan.Also applies to: 285-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/pp-dev-config-1.0-canvas.md` at line 160, Update the affected markdown section headings under the canvas docs from third-level to second-level headings so the document outline stays consistent and avoids MD001. Adjust the `build.zip` object heading and the other matching section referenced in the review to use `##` instead of `###`, keeping the section titles unchanged.Source: Linters/SAST tools
CHANGELOG.md-9-10 (1)
9-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
inspectoras part of the 1.0 config surface.This entry says the new schema has five groups and the features list mirrors that, but this PR also introduces the
inspectorsection. Omitting it makes the migration notes incomplete for users updating their config.Also applies to: 92-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 9 - 10, Update the changelog entry for the config schema change to include the new inspector section alongside the other grouped config sections, since PPDevConfig now exposes inspector as part of the 1.0 surface. Locate the section describing the five grouped areas and adjust the feature list/migration notes so inspector is explicitly mentioned and the migration guidance is complete.README.md-419-421 (1)
419-421: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to this fenced block.
This trips markdownlint (
MD040) as written.Suggested fix
-``` +```text pp-dev 🔍 Request Inspector → http://localhost:3000/@pp-dev/inspector</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdaround lines 419 - 421, The fenced block in README.md is missing a
language identifier, which triggers markdownlint MD040. Update the markdown
fence around the Request Inspector snippet to use an explicit language such as
text, keeping the existing content unchanged and matching the style of nearby
fenced examples if any.</details> <!-- cr-comment:v1:0813bb6705abc45d4b6edf60 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>README.md-442-450 (1)</summary><blockquote> `442-450`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Add the missing `defineConfig` import in the example.** This snippet won’t paste into `pp-dev.config.ts` as shown because `defineConfig` is undefined. <details> <summary>Suggested fix</summary> ```diff // pp-dev.config.ts +import { defineConfig } from '`@metricinsights/pp-dev`'; + export default defineConfig({ inspector: { enabled: true, // set to false to disable entirely maxMemory: 100 * 1024 * 1024, // evict oldest entries above 100 MB captureLimit: 10 * 1024 * 1024, // capture at most 10 MB per body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 442 - 450, The README example for pp-dev.config.ts is missing the import for defineConfig, so the snippet will not work as pasted. Update the configuration example to include the required defineConfig import at the top of the snippet, and keep the rest of the inspector configuration unchanged so the example is directly copy-pastable.tests/e2e/server-lifecycle.spec.ts-4-6 (1)
4-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the suite header comment.
This comment says the suite does not use mock MI, but
beforeAll()now bootsstartMockMiServer()and rewrites the fixture config to use it. Leaving this stale makes the setup much harder to follow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/server-lifecycle.spec.ts` around lines 4 - 6, Update the suite header comment in the server lifecycle spec so it matches the current setup: the tests now start the mock MI server in beforeAll() and rewrite the fixture config to point at it. Edit the top-of-file comment near the suite description to remove the stale “does NOT use the mock MI server” wording and describe the actual mock-backed initialization used by startMockMiServer() and the fixture config rewrite.src/lib/migrate.ts-61-64 (1)
61-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
inspectorin the migrated-config detector.
normalizePPDevConfig()already accepts a top-levelinspectorblock, so a config like{ inspector: { enabled: false } }is valid 1.0 input. With the current set,pp-dev migratefalls through to “Could not detect config format” instead of exiting cleanly.Suggested fix
export function isAlreadyMigrated(config: Record<string, unknown>): boolean { - const NEW_KEYS = new Set(['mi', 'app', 'proxy', 'build', 'sync']); + const NEW_KEYS = new Set(['mi', 'app', 'proxy', 'build', 'sync', 'inspector']); return Object.keys(config).some((k) => NEW_KEYS.has(k)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/migrate.ts` around lines 61 - 64, The migrated-config detector in isAlreadyMigrated currently misses the top-level inspector block, so valid 1.0 configs like { inspector: {...} } are not recognized. Update the NEW_KEYS set inside isAlreadyMigrated to include inspector, keeping the existing detection logic unchanged so normalizePPDevConfig() inputs are treated as already migrated and pp-dev migrate exits cleanly.src/lib/request-store.ts-51-67 (1)
51-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHonor
maxMemoryfor single oversized entries.If
size > maxMemoryand the store is already empty, the eviction loop never runs and the entry is still inserted. That leavesmemoryUsageabove the configured cap permanently, so the inspector’s storage limit is no longer enforced. Drop oversized entries or strip their captured bodies before inserting them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/request-store.ts` around lines 51 - 67, The RequestStore.add method currently inserts a RequestEntry even when its computed size exceeds maxMemory and the store is empty, which bypasses the storage cap. Update add(entry: RequestEntry) to detect oversized entries before calling entries.set/insertOrder.push, and either skip storing that entry entirely or clear its captured requestBody/responseBody so the stored size stays within the limit; keep the eviction logic in add aligned with totalSize accounting.src/lib/request-capture.middleware.ts-54-60 (1)
54-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve chunk bytes when capturing
res.write()/res.end()
captureChunk()stringifies non-Buffer values, soUint8Arraychunks become comma-separated text and string writes with an explicit encoding are captured with the wrong bytes. Pass the write/end encoding through and treatArrayBufferViewinputs as raw bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/request-capture.middleware.ts` around lines 54 - 60, The captureChunk() logic in request-capture.middleware currently converts non-Buffer chunks to strings, which corrupts Uint8Array data and ignores explicit write/end encodings. Update the res.write()/res.end() capture path so the encoding argument is forwarded into captureChunk, and make captureChunk treat ArrayBufferView inputs as raw bytes rather than stringifying them; use the existing captureChunk helper and surrounding response-capture flow to locate the fix.src/lib/request-inspector.ts-518-531 (1)
518-531: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDecode text bodies as UTF-8 before rendering/copying
atob(b64)returns a binary string, so non-ASCII JSON/plain-text bodies will render as mojibake andcopyBody()will copy the wrong text. Decode the bytes withTextDecoderonce and reuse that string for both the preview and the clipboard action.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/request-inspector.ts` around lines 518 - 531, The text-body handling in request-inspector.ts is decoding with atob(b64), which treats the payload as a binary string and can corrupt non-ASCII content in both the preview and copy path. Update the body rendering logic in the affected branch to decode the base64 bytes with TextDecoder once, then reuse that decoded UTF-8 string for the JSON/plain-text preview and for copyBody() so both display and clipboard contents match the original text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/config.ts`:
- Around line 237-239: The fallback in getConfig() assigns pkg['pp-dev']
whenever typeof is "object", which incorrectly accepts null and arrays. Update
the package.json check in getConfig() to only use pkg['pp-dev'] when it is a
non-null plain object, so config never becomes null or an array before
normalizeConfig() runs. Keep the logic anchored around getConfig() and the
pkg['pp-dev'] fallback so the returned PPDevConfig stays valid.
---
Major comments:
In @.github/workflows/ci.yml:
- Around line 14-20: Disable persisted Git credentials on both
`actions/checkout` steps in `.github/workflows/ci.yml`: add
`persist-credentials: false` to the build job checkout at
`.github/workflows/ci.yml#L14-L20` and the test job checkout at
`.github/workflows/ci.yml#L44-L51`, matching the existing hardening used in the
beta release workflow.
- Around line 65-69: The E2E fixture install step is bypassing committed
lockfiles via npm install --package-lock=false, which makes the CI dependency
graph non-reproducible. Update the fixture install commands in the CI workflow
to use npm ci for the test-nextjs, test-commonjs, and test-nextjs-cjs fixtures
so they install exactly from their checked-in lockfiles. If any fixture needs
patched dependencies, update the corresponding tests/**/package.json overrides
and regenerate the matching package-lock.json files so audit:all stays clean.
In @.mcp.json:
- Around line 3-5: The MCP entry for the Framelink Figma server uses a
Windows-only launcher and an unpinned runtime package fetch, which should be
made cross-platform and reproducible. Update the MCP config entry for “Framelink
MCP for Figma” to use a platform-agnostic launch approach instead of cmd /c, and
ensure figma-developer-mcp is pinned to a specific version or installed as a
repository dependency so the command is deterministic. Keep the change localized
to the MCP config block so the launch behavior is consistent across
environments.
In `@e2e/toolbar/toolbar.sync.spec.ts`:
- Around line 35-51: The sync toolbar test currently skips the case where the
button becomes disabled before the syncing class is applied, which hides a real
regression. In toolbar.sync.spec.ts, update the assertion flow around the sync
button polling and the post-click checks so that the disabled-before-syncing
state is asserted explicitly in the test instead of calling test.skip(); use the
syncButton checks and the existing class/disabled assertions to validate
whichever state is expected.
In `@scripts/build-types.mjs`:
- Around line 16-31: The DTS child process started in build-types.mjs is not
placed in its own process group, so killTree() cannot reliably terminate the
full shell/Rollup subtree on POSIX. Update the child spawn logic in the code
that launches the DTS build to run detached on non-Windows platforms, and ensure
the existing killTree() path that uses process.kill(-pid, 'SIGKILL') is
targeting that detached group. Keep the current Windows taskkill behavior
unchanged.
In `@scripts/run-playwright-e2e.ts`:
- Around line 201-222: The readiness polling in waitForServer can falsely
succeed even if the spawned pp-dev process has already exited, so make the check
fail fast when the child dies instead of waiting for an unrelated server on the
same port. Update waitForServer and its caller in run-playwright-e2e.ts to
accept/inspect the spawned process handle (or an exit promise), and during each
poll iteration abort immediately with the child’s exit status if pp-dev has
terminated before the expected server becomes ready.
In `@src/cli.ts`:
- Around line 651-657: The app base selection in the CLI already switches to
PATH_TEMPLATE_LOCAL_PREFIX when v7Features is enabled, but the assetPrefix logic
still uses PATH_TEMPLATE_PREFIX, causing the app and Next assets to diverge.
Update the assetPrefix calculation in src/cli.ts to mirror the same branch
conditions used for base (including configBasePath, templateLess, and
v7Features) so both values resolve to the same template prefix. Use the existing
base assignment and assetPrefix emission as the main references when making the
change.
In `@src/index.ts`:
- Around line 291-314: The config handling in withPPDev is shallow-merging
grouped objects, so sibling fields inside app and mi can be lost, and basePath
is derived from templateName instead of the normalized app.name used by
normalizePPDevConfig(). Fix this by deep-merging the grouped config before
reading app/mi values, then derive the effective template name from the
normalized config and pass that into createBasePath so Next.js paths match the
CLI/Vite config.
In `@src/lib/pp.middleware.ts`:
- Around line 349-350: `getPageVariables()` is resolving the v7 template before
`this.appId` is initialized, which can make `getPageTemplate()` read an
undefined page ID via `this.appId!`. Update the initialization flow in
`MiAPI.getPageVariables()` so `this.appId` is assigned from `pageId` before any
template lookup, and keep the fallback in the same path that reaches
`getPageTemplate()` to ensure the v7 branch always has a valid app ID.
In `@src/lib/request-inspector.ts`:
- Around line 25-27: The `/@api/requests/:id` route is shadowing
`/@api/requests/stats`, so move the `stats` handler above the single-request
handler in `request-inspector.ts` to ensure `/stats` is matched first. Also
update the body-decoding logic used by the request detail/copy paths in the same
module so decoded text is interpreted as UTF-8 instead of using the raw
`atob(...)` byte string directly, preserving non-ASCII characters in JSON,
plain-text, and copy output.
In `@src/plugins/client-injection-plugin.ts`:
- Line 44: Keep the client injection plugin’s resolveId logic pinned to the
packaged client bundle by restricting PACKAGE_REGEXP and the path handling in
resolveId() so only safe client asset paths under dist/client are accepted.
Update the regex and any suffix joining so it rejects traversal or arbitrary
suffixes instead of concatenating user-provided segments directly onto
dist/client, and ensure the check in client-injection-plugin.ts only resolves
known packaged client assets.
In `@tests/e2e/server-lifecycle.spec.ts`:
- Around line 51-59: The onData handler in server-lifecycle.spec.ts is dropping
partial log fragments because it splits each chunk by newline without preserving
the trailing remainder, so waitForLine() can miss startup lines. Update the
onData flow to keep a buffered leftover between data events, prepend it to the
next chunk before splitting, and only emit complete lines through lines and
listeners using the existing onData/listeners/waitForLine logic.
- Around line 17-23: The killTree helper in the server lifecycle test is only
terminating the shell wrapper on POSIX, which can leave the underlying npm/next
process running. Update killTree to either avoid using shell when starting the
process or terminate the spawned process group so the entire tree is cleaned up;
keep the Windows taskkill path as-is and adjust the POSIX branch accordingly.
In `@tests/mock-mi/cassettes/startup.json`:
- Around line 21-34: The mock cassette entry for GET:/api/page/id/733 has
mismatched page data because the response body still contains the 937 page
metadata. Update the response payload in the startup cassette so the body
returned by the /api/page/id/733 fixture consistently reflects page 733,
including the page id and all associated fields, keeping the request path and
response content aligned.
In `@tests/mock-mi/record-auto.ts`:
- Around line 59-69: Replace the global uncaughtException-based teardown in
record-auto.ts with structured cleanup around the top-level async flow. Wrap the
config patch and the awaited record/startup steps in a try/finally so cleanup()
always runs after any rejection, and remove the
process.once('uncaughtException') handler; use the existing cleanup and
CONFIG_PATH logic to restore the config and stop leaving mock-mi/pp-dev running.
- Around line 95-99: The `record-auto.ts` Next.js launcher is assuming port 3000
via `PP_BASE`, but `spawn(..., 'next')` does not pin the port, so the recorder
can target the wrong server if 3000 is busy. Update the `ppdev` startup to pass
an explicit port (and ideally strict-port behavior) to the `next` command, then
align the `PP_BASE`/probe target in the recorder logic with that fixed port so
`record-auto` always records against the intended instance.
In `@tests/mock-mi/record.ts`:
- Around line 62-73: Make the shutdown path in cleanup idempotent because it is
invoked from both the SIGINT/SIGTERM handlers and ppdev.once('exit'), which can
cause re-entrant cleanup when ppdev.kill() triggers exit while shutdown is
already running. Add a single-run guard in cleanup and ensure the cassette is
persisted by calling mockMi.save?.() before mockMi.close(), so the record flow
in tests/mock-mi/record.ts reliably writes data even on unexpected pp-dev
termination.
In `@tests/mock-mi/server.ts`:
- Around line 298-326: The response-capture path in tests/mock-mi/server.ts only
handles gzip before storing cassette bodies, so Brotli and deflate payloads can
be saved as corrupted UTF-8 text. Update the decompression branch in the proxy
response handling logic around the `encoding`, `storedHeaders`, and
`interactionMap.set` flow to also decode `br` and `deflate` responses before
converting non-binary bodies to text, and make sure the corresponding
`content-encoding`/`content-length` headers are adjusted consistently after
successful decompression.
- Around line 71-137: The cassette sanitizers in sanitizeJsonValue and
sanitizeTextBody are missing filesystem-path redaction, so internal deployment
paths can still be saved into recorded fixtures. Update the sanitization logic
to detect and replace path-like values in JSON fields such as path/link and in
free-text bodies before cassettes are written. Use the existing sanitizeBody
flow in tests/mock-mi/server.ts as the place to centralize this redaction, then
re-record the affected cassette so it no longer contains real filesystem paths.
In `@tests/test-nextjs/tsconfig.json`:
- Around line 31-47: The tsconfig include globs for generated declarations under
dist are being shadowed by the broad dist exclusion. Update the tsconfig in the
tests/test-nextjs setup so the generated type paths referenced by the include
section are actually visible, either by removing dist from exclude or by
explicitly listing the needed generated files in files; use the existing
include/exclude arrays in the tsconfig to locate the change.
In `@tests/unit/plugin/plugin.normalize.spec.ts`:
- Around line 264-300: Update the `validatePPDevConfig` tests in
`plugin.normalize.spec.ts` so they do not rely on ambient
`process.env.MI_BACKEND_URL`; the missing-URL cases should explicitly clear or
override that env value in the test setup/teardown, similar to the normalization
suite, to keep `validatePPDevConfig()` deterministic across runners. Use the
existing `validatePPDevConfig` describe block and its related
`beforeEach`/`afterEach` hooks to isolate env state for all assertions that
expect `mi.url is required`.
---
Minor comments:
In @.github/workflows/release.yml:
- Around line 52-53: Remove the npm@latest upgrade step from the release
workflow since the Node 24 toolchain already meets the trusted-publishing
requirement. In the release job, update the npm install step under the trusted
publishing setup to either use the bundled npm CLI directly or replace the
moving latest dependency with a pinned version if an upgrade is still needed.
Keep the change focused on the workflow job that performs OIDC publishing so the
release path stays deterministic.
In `@CHANGELOG.md`:
- Around line 9-10: Update the changelog entry for the config schema change to
include the new inspector section alongside the other grouped config sections,
since PPDevConfig now exposes inspector as part of the 1.0 surface. Locate the
section describing the five grouped areas and adjust the feature list/migration
notes so inspector is explicitly mentioned and the migration guidance is
complete.
In `@docs/pp-dev-config-1.0-canvas.md`:
- Around line 249-255: The documented default for MiConfig.mode is inconsistent
in the reference block, Defaults table, and later decision text. Update the
MiConfig interface comment so the mode default matches the chosen contract
everywhere in this document, and make sure the wording around MiConfig.mode and
related include semantics is aligned with the same default value throughout.
- Line 160: Update the affected markdown section headings under the canvas docs
from third-level to second-level headings so the document outline stays
consistent and avoids MD001. Adjust the `build.zip` object heading and the other
matching section referenced in the review to use `##` instead of `###`, keeping
the section titles unchanged.
In `@README.md`:
- Around line 419-421: The fenced block in README.md is missing a language
identifier, which triggers markdownlint MD040. Update the markdown fence around
the Request Inspector snippet to use an explicit language such as text, keeping
the existing content unchanged and matching the style of nearby fenced examples
if any.
- Around line 442-450: The README example for pp-dev.config.ts is missing the
import for defineConfig, so the snippet will not work as pasted. Update the
configuration example to include the required defineConfig import at the top of
the snippet, and keep the rest of the inspector configuration unchanged so the
example is directly copy-pastable.
In `@src/lib/migrate.ts`:
- Around line 61-64: The migrated-config detector in isAlreadyMigrated currently
misses the top-level inspector block, so valid 1.0 configs like { inspector:
{...} } are not recognized. Update the NEW_KEYS set inside isAlreadyMigrated to
include inspector, keeping the existing detection logic unchanged so
normalizePPDevConfig() inputs are treated as already migrated and pp-dev migrate
exits cleanly.
In `@src/lib/request-capture.middleware.ts`:
- Around line 54-60: The captureChunk() logic in request-capture.middleware
currently converts non-Buffer chunks to strings, which corrupts Uint8Array data
and ignores explicit write/end encodings. Update the res.write()/res.end()
capture path so the encoding argument is forwarded into captureChunk, and make
captureChunk treat ArrayBufferView inputs as raw bytes rather than stringifying
them; use the existing captureChunk helper and surrounding response-capture flow
to locate the fix.
In `@src/lib/request-inspector.ts`:
- Around line 518-531: The text-body handling in request-inspector.ts is
decoding with atob(b64), which treats the payload as a binary string and can
corrupt non-ASCII content in both the preview and copy path. Update the body
rendering logic in the affected branch to decode the base64 bytes with
TextDecoder once, then reuse that decoded UTF-8 string for the JSON/plain-text
preview and for copyBody() so both display and clipboard contents match the
original text.
In `@src/lib/request-store.ts`:
- Around line 51-67: The RequestStore.add method currently inserts a
RequestEntry even when its computed size exceeds maxMemory and the store is
empty, which bypasses the storage cap. Update add(entry: RequestEntry) to detect
oversized entries before calling entries.set/insertOrder.push, and either skip
storing that entry entirely or clear its captured requestBody/responseBody so
the stored size stays within the limit; keep the eviction logic in add aligned
with totalSize accounting.
In `@tests/e2e/server-lifecycle.spec.ts`:
- Around line 4-6: Update the suite header comment in the server lifecycle spec
so it matches the current setup: the tests now start the mock MI server in
beforeAll() and rewrite the fixture config to point at it. Edit the top-of-file
comment near the suite description to remove the stale “does NOT use the mock MI
server” wording and describe the actual mock-backed initialization used by
startMockMiServer() and the fixture config rewrite.
Fix real PR review findings around config merging, inspector request handling, CI determinism, and mock MI cassette fidelity.
Avoid npm ci integrity failures for rebuilt local pp-dev tarballs in fixture apps.
- proxy-cache: fix infinite loop in EnhancedCache.cleanup() when stale key snapshot from phase 1 is reused in phase 2 size loop - proxy-cache: invoke res.write callback to preserve Node.js stream backpressure contract - request-capture: add once-guard to finalize() to prevent double res.end() corrupting RequestStore - index: use PATH_TEMPLATE_PREFIX for assetPrefix in production builds (was incorrectly using /pl/ for v7) - cli: add packages:'external' and target:'node24' to migrate command esbuild config - migrate: fix integrateMiTopBar object-is-truthy pitfall; only emit mi.include when at least one sub-flag is true - pp.middleware: align getPageInfo appId guard with getPageVariables (typeof === 'undefined') - request-store/inspector: guard NaN from parseInt leaking through ?? operator - plugin: wire validatePPDevConfig before normalizePPDevConfig in all entry points; fix contradiction in standalone+page+no-url validation - request-inspector: remove duplicate display:none from detail panel inline style
3 tasks
This was referenced Aug 7, 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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
🚀 Adds the pp-dev 1.0 developer tooling update for PP-3449: the Request Inspector UI/API, refreshed MI-style dev panel, grouped config schema with migration support, and stronger local/CI test coverage.
🧪 The PR also reworks CI so GitHub Actions runs the full expected test surface without Docker: repo-wide audit,
npm run test, Vitest E2E, and browser E2E against Vite, Next.js ESM, and Next.js CJS fixtures throughmock-mireplay.🚀 Product and Developer Features
/@pp-dev/inspectorwith captured request metadata, headers/bodies, copy/save actions, clearing, source badges, and storage limits./@api/requestsfor paginated request lists, full request detail, stats, and clearing captured entries.defineConfig, migration helpers, and tests for config normalization/validation.🔧 CI, Security, and Compatibility
npm ciuse--no-audit, followed by explicitnpm run audit:all.undiciaudit issue through package overrides and simplified the postinstall patch script.pp-devbinaries are available.🧪 Testing and Mock MI
mock-mirecord/replay infrastructure with cassette sanitization for tokens, cookies, emails, names, response headers, large binary assets, and MI top-bar assets.startupcassette that covers common startup, top-bar, Vite, Next.js ESM, and Next.js CJS fixture requests.scripts/run-playwright-e2e.tsto run browser E2E sequentially acrosscommonjs,nextjs, andnextjs-cjsfixtures.✅ Test Plan
npm run audit:allis part of CI.npm run testis part of CI.npm run test:e2eis part of CI.npm run test:e2e:browseris part of CI and runs all three browser fixtures.📊 Change Size
origin/pp-3449intoorigin/develop.Included commits
Compatibility: No Docker runner is required for CI browser tests. The PR includes pp-dev 1.0 config migration support for projects moving to the grouped config shape.
Merge Request:
origin/pp-3449→origin/developSummary by CodeRabbit
New Features
Bug Fixes
Documentation