Review — PR #430 chore(demo): zero CVE advisories — replace EOL demo toolchains and fix all seven demos

Repo convertcom/javascript-sdk · HEAD 581a2d801a7820b2468a3b5e217d05e0d0ab7424 · base main @ fe46798d592b6633190451de4ae2f123aa04fd13 (PR is exactly 4 commits on top, no divergence)

1. Verdict

APPROVED WITH NON-BLOCKING FINDINGS

The two migrations are done correctly: packages/** is byte-untouched, the removal of the path-to-regexp resolution fixes a live breakage rather than merely being safe, every SVG/alias/JSX edge case of the CRA→Vite move is actually covered, and the React Router 7 config reproduces what the deleted Remix entry files did. Nothing here breaks a demo or reintroduces a vulnerability. What remains is documentation the PR invalidated but did not update, a resolution upper bound that over-reaches, and two engines fields that are now wrong.

2. Non-blocking findings

1. [IMPORTANT] Both Remix READMEs still document a build command that no longer exists and an output path that is never produced.
demo/remixjs-client-side/README.md:1,3,51,54 and demo/remixjs-server-side/README.md:1,3,51,54

Failure scenario: a user follows the "DIY" deployment section, runs remix build — after this PR there is no remix binary anywhere in the tree (@remix-run/dev has 0 entries in yarn.lock) — then deploys build/ and public/build/. react-router build emits build/client + build/server; public/build/ is never created. The start script this PR wrote (react-router-serve ./build/server/index.js) confirms the new layout. These are the only two stale references to the removed toolchain left in the repo (verified by grepping all *.md/json/js/mjs/ts/tsx for react-scripts|remix build|remix dev|remix-serve|entry.server|remix.config).

Fix: in both files replace remix build → react-router build, replace the build/ + public/build/ bullets with build/client + build/server, and retitle away from "Welcome to Remix!" / the Remix docs link.

2. [IMPORTANT] demo/reactjs/README.md still presents the demo as Create React App.
demo/reactjs/README.md:3,43,47,53,57,61,65,69,73

yarn start, yarn build and port 3002 are all still accurate, so nothing hard-breaks — but the README tells the reader the project is CRA and points at CRA docs for deployment, advanced configuration, code splitting and a "yarn build fails to minify" troubleshooting page that no longer applies. It also promises "You will also see any lint errors in the console", which the Vite dev server does not do. The new preview script is undocumented.

Fix: drop the CRA framing/links, document yarn preview.

3. [IMPORTANT] The undici resolution's <8 upper bound forces a major downgrade on a consumer that was already patched.
package.json:85 — "undici": ">=7.29.0 <8"

Evidence: in the HEAD lock exactly two packages depend on undici — miniflare@5.20260801.0-alpha (declares the exact pin 7.28.0) and node-gyp@latest (declares ^8.4.1, pulled in by fsevents). The lock now contains a single undici entry, "undici@npm:>=7.29.0 <8" → 7.29.0, so both are rewritten. Per the GitHub advisory DB (gh api /advisories?ecosystem=npm&affects=undici), the relevant advisories — including the high-severity GHSA-4cwx-7wf7-3272 — are patched at 7.29.0 and 8.9.0. The base lock resolved node-gyp's copy to 8.10.0, i.e. already unaffected. So the <8 cap buys zero security and costs a major-version downgrade (8.10.0 → 7.29.0) for the one consumer that didn't need touching. Practical impact today is small (node-gyp only runs if fsevents is rebuilt from source), but the cap will also silently downgrade any future legitimate undici@8 consumer.

Fix: scope it to the actually-vulnerable consumer — "miniflare/undici": ">=7.29.0 <8" — leaving node-gyp on 8.x.

4. [IMPORTANT] Both Remix demos still declare engines.node: ">=18.0.0", which their new dependencies cannot satisfy.
demo/remixjs-client-side/package.json:42-44 and demo/remixjs-server-side/package.json:42-44

Verified engines of the deps this PR added: react-router@7.18.2 → >=20.0.0; @react-router/dev@7.18.2 → >=20.0.0; @react-router/node@7.18.2 / @react-router/serve@7.18.2 → >=20.0.0; vite@7.3.5 → ^20.19.0 || >=22.12.0. Failure scenario: a contributor on Node 18 trusts the field, installs, and gets engine warnings plus a toolchain that is not supported at runtime.

Fix: bump both to >=20.19.0.

5. [IMPORTANT] Port wiring: two demos don't actually read the .env that demo:prepare seeds, and no demo pins its port.
demo/reactjs/vite.config.js:50-51; demo/nestjs/src/main.ts:9; scripts/ensure-sdk-built.mjs:5-9

Per the Vite docs (Config → Using Environment Variables in Config), Vite deliberately does not load .env into process.env when evaluating the config file — loadEnv is required. So Number(process.env.PORT) || 3002 in vite.config.js always takes the 3002 fallback, and the seeded demo/reactjs/.env has no effect: a user who edits PORT there sees nothing change. demo/nestjs has no dotenv/ConfigModule at all and likewise falls back to its hardcoded 3004. The four other demos genuinely do read .env (demo/nodejs/app.js:8-9 runs dotenv.config() before bin/www:15 reads PORT; nextjs and both remix demos go through dotenv-cli, declared at package.json:30). The observable outcome — documented ports, no :3000 collision — is achieved everywhere, so this is a configurability and claim-accuracy issue, not a break. Secondary: no config sets strictPort, so if 3006 is busy the remix client demo silently slides onto 3007, the remix server demo's documented port.

Fix: use loadEnv in demo/reactjs/vite.config.js (or run vite under dotenv-cli like the other demos) and add strictPort: true to the three Vite configs; either that, or narrow the comment in scripts/ensure-sdk-built.mjs:5-9 so it doesn't claim every demo reads its port from .env.

6. [minor] Six resolutions are now dead. package.json:60-85 — pbkdf2, cipher-base, sha.js, elliptic, http-proxy-middleware, nth-check have zero matching entries in the HEAD lock; they were all react-scripts-tree pins. Harmless (Yarn ignores unmatched resolutions), but this PR is the dependency-hygiene PR and removed only path-to-regexp.

7. [minor] The PR description's justification for the path-to-regexp removal is wrong on the facts, though right on the conclusion. @nestjs/core@11 and @nestjs/platform-express@11 declare path-to-regexp: 8.4.2, not ^0.1.12. More importantly the removal is stronger than "safe": the base lock had a single entry, "path-to-regexp@npm:^0.1.12" → 0.1.13, meaning the old blanket resolution collapsed every consumer — nestjs (8.4.2), express 5's router (^8.0.0), wrangler (6.3.0), serve-handler (3.3.0) — onto the API-incompatible 0.1.x. Worth correcting in the description so the next reader doesn't inherit the wrong reason for a load-bearing change.

4. Claims verified

5. What this review did not cover

• The manual browser verification. "All seven demos launched on documented ports, zero console/stdout/network errors" rests on a run that cannot be reproduced here. No yarn install, yarn build, vite build, react-router build, wrangler dev, or demo start was run. Everything above is static analysis plus first-party package sources and the GitHub advisory API.
• twin.macro's css={…} prop at runtime. 7 usages across 4 files. The babel ordering that makes it possible is verified; whether the emitted styles are visually identical to the CRA build needs a browser.
• wrangler dev under undici@7.29.0 instead of the 7.28.0 miniflare pins exactly. Low risk (patch-level within 7.x), but it is a deliberate upstream pin being overridden and only a real run proves it.
• yarn npm audit — Yarn 4 requires a restored install state, so the repo-wide "zero advisories" number is unconfirmed. Checks were targeted at the packages the description makes claims about plus the newly added build tooling.
• yarn typecheck in either Remix demo (react-router typegen && tsc) — not wired into CI and not run here; the hand-written ConvertSDKInstance interface in Convert.tsx and useLoaderData<ConvertLoaderData>() are plausible but untypechecked.
• Lint. The repo's lint scripts cover packages/* only; no lint gate applies to any file in this diff, so there was nothing meaningful to run.
• Staleness, as distinct from partial builds. scripts/ensure-sdk-built.mjs detects a missing or partial build. A complete-but-stale lib/ (older than src/) is not detected and the demo will run against it. Minor for a demo bootstrap, and out of the PR's stated scope.

Provenance

• Review pass: convert-code-reviewer agent (Opus), independent of the main session, reading a detached worktree at 581a2d80 — 76 tool calls.
• Verification pass: the main session independently spot-checked the reviewer's load-bearing claims against disk before this report was accepted, per .claude/rules/verify-subagent-claims.md. Checked and confirmed: packages/** empty diff; .gitignore:3 is .env (not .env*); all six tracked .env.example files and their exact ports; demo/reactjs/vite.config.js:50-51; demo/nestjs/src/main.ts:9 plus zero dotenv in its manifest; both remix engines blocks at :42-44; package.json:85 undici; HEAD-vs-BASE path-to-regexp lock entries (1 collapsed entry → 4 correct ones) and undici entries (3 → 1, incl. the 8.10.0 → 7.29.0 downgrade); all six dead resolutions at zero lock entries; vite 7.3.6 / esbuild 0.28.1; @nestjs/core@11.1.28 declaring path-to-regexp: npm:8.4.2; both remix READMEs' stale remix build / public/build/ lines; the reactjs README CRA lines; zero process.env / REACT_APP_ / %PUBLIC_URL% in the reactjs demo; one route file per remix demo; zero strictPort; and a full read of scripts/ensure-sdk-built.mjs. One imprecision found: the reviewer stated all @remix-run/* entries were gone; one remains, and it is a dependency of the new toolchain with no advisories — the conclusion is unaffected. Corrected inline above.
• CI at this sha: 6/6 green — pass-qa (22), SonarCloud, CodeQL, Analyze (actions), Analyze (javascript), auto-approve.

Approved via /convert:approve. An independent code review ran through /convert:review, and this issues the B-G4 human marker at 581a2d8.