Skip to content

build(frontend): migrate react-ystemandchess from CRA to Vite - #252

Draft
Deepesh-Katudia wants to merge 8 commits into
mainfrom
chore/frontend-cra-to-vite
Draft

build(frontend): migrate react-ystemandchess from CRA to Vite#252
Deepesh-Katudia wants to merge 8 commits into
mainfrom
chore/frontend-cra-to-vite

Conversation

@Deepesh-Katudia

@Deepesh-Katudia Deepesh-Katudia commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Replaces Create React App with Vite for react-ystemandchess. CRA is unmaintained, and its pinned toolchain was the source of most frontend audit findings: 47 findings go down to 5, none high or critical, with full test parity.

This branch was cut before #208 merged. It is now updated against current main, including a fix for a bug that would have blanked the production site after merging.

Draft until: a real docker build is run and checked in a browser, and the Phase 0 prerequisites below merge.

  • Migration write-up: documentation/frontend-cra-to-vite-migration.md
  • Merge plan (post-Fix environment separation #208 findings, sequencing, pipeline coordination): documentation/frontend-vite-merge-plan.md

Type of Change

  • Bug fix
  • Refactor
  • Documentation update
  • Other (please specify): build tooling migration (CRA → Vite), Node 24

Key Changes

  • Build: react-scripts removed. Vite 8 with @vitejs/plugin-react and vite-plugin-svgr replaces it, and Jest now runs standalone. Output stays in build/ on port 3000, so the runtime stage, serve -s build, and deploy build args are unchanged.
  • Env vars (the critical fix): Fix environment separation #208's environment.prod.js read variables with process.env[name]. Vite can't replace a dynamic key, and Vite 8 rewrites the leftover bare process.env to {}. The lookup compiled to {}[name], so the required-URL guard threw on load and the page went blank. Build, Jest, the Docker healthcheck, and a curl smoke test all still passed.
    • environment.prod.js now references each variable by its full literal name.
    • vite.config.mts defines every REACT_APP_* variable, read with loadEnv so .env files work as they did under CRA.
  • CI guard: scripts/verify-build-env.mjs runs in the build step with the same env as the build. It fails if process.env survives in shipped JS/HTML, or if a required URL wasn't inlined. A plain process.env grep would not have caught this bug, because the broken bundle contains no process.env text. The URL check does catch it: the script fails against main's old environment.prod.js and passes on this branch.
  • Password reset (separate commit, baa766d7): reset-password.tsx and set-password.tsx read REACT_APP_API_URL, which nothing sets, so production fell back to http://localhost:8000. Both now use environment.urls.middlewareURL. This bug predates Vite.
  • Docker: Node 20.19.0, which is end-of-life, becomes 24.21.0 LTS (the Volta pin too). The build stage uses npm ci instead of npm install.
  • Merge with main: Conflicts in Lessons.tsx, ActivitiesModal.tsx, and LeaderboardModal.tsx were in import lines only. Main's side is kept (no SCSS modules after Tailwind standardization #247, and environment imported from ../environments), with SVGs switched to ?react imports.
  • Second merge with main (29d8df73, Oct 8): StreakModal.tsx (week2: Wire StreakModal to live streak data; record completedDates on… #251) conflicted on the same import pattern. It's resolved the same way, with ?react SVGs and environment from ../environments. activitiesApi.ts (feat(middlewareNode): currency ledger + leaderboard swap (Karthik + Jimmy's Rev. 2 lanes) #250) merged cleanly but imported the dev-only environments/environment, which would have pointed production at the dev URLs, so it's fixed here too. package.json keeps main's engines.node from the Node 24 pin. After the merge, Jest passes 30/30 suites and 155/155 tests, tsc is clean, and verify-build-env.mjs passes.

Testing

  • Unit tests: 30/30 suites and 155/155 tests pass after the merge and all fixes
  • tsc --noEmit is clean
  • vite build with the production URLs passes verify-build-env.mjs
  • Manual: served the production build with vite preview and loaded it in Chrome. The home page and /reset-password render with no console errors, the bundle contains the production middleware URL, and localhost:8000 and process.env are absent
  • docker build with all build args, then open the container in a browser (Docker was not available locally)
  • After deploy: the live site renders in a browser, and a real password-reset email arrives with a working link

Bugs Fixed (if applicable)

  • The production bundle blanked on load under Vite because of the process.env[name] lookup from Fix environment separation #208.
  • Password reset and set-password called http://localhost:8000 in production.

TODO (Follow-up Work)

  • Phase 0 (gates the merge): chore/npm-audit-fix-frontend merges, and this branch then re-applies its dependency changes (keep socket.io-client ^4.8.3). The Node 24 pin has merged ✅. Then the CD pipeline (sahana/208-azure-cicd) merge.
  • Bundle smoke test in deploy.yml, in the same release as this PR: it checks that the deployed JS contains the middleware URL. It has to ship together with Vite because the asset path changes from /static/js/ to /assets/. See Phase 6 of the plan.
  • CI setup-node is still 20.19.0 for every service. Bump it to 24 separately.
  • chessclient: remove CRA entirely and serve public/chessclient/ with nginx. That goes in a separate PR, after checking the live board.

Additional Notes

Needs pipeline-owner review on the Dockerfile and on the smoke-test coordination. No changes to deploy.yml build args, contexts, or tags are needed.

@Deepesh-Katudia
build(frontend): migrate react-ystemandchess from CRA to Vite
8c6a523 
Resolves the 33 audit findings that remained after `npm audit fix`, all of
which were locked behind react-scripts@5.0.1 (unmaintained upstream). npm
reported the only "fix" for react-scripts as version 0.0.0, i.e. none exists,
and an npm override could not close the gap either: CRA's own dev-server
config calls onBeforeSetupMiddleware/onAfterSetupMiddleware/https, all removed
in webpack-dev-server 5, while the advisory fix requires >= 5.2.1.

Audit: 47 findings -> 5. Zero critical, high, or low remaining. The 5 moderate
are express (+body-parser/qs), which exist only for dead server-side code under
src/, and react-router/react-router-dom, deferred to their own task.

Build tooling:
- Replace react-scripts with vite, @vitejs/plugin-react, vite-plugin-svgr.
- Move public/index.html to the project root; %PUBLIC_URL% -> /.
- build.outDir 'build' so the Dockerfile's `serve -s build` is unchanged.
- Define process.env.REACT_APP_API_URL / NODE_ENV so the two components and
  the environments module that read them keep working under Vite and Jest.

Things CRA had been doing implicitly, now declared explicitly:
- postcss.config.js: CRA auto-detected tailwind.config.js and injected the
  tailwindcss PostCSS plugin. Vite does not, so Tailwind silently stopped
  compiling until this was added.
- uuid: promoted to a real dependency. It is imported by Puzzles.tsx and
  Student.tsx but resolved only via react-scripts > webpack-dev-server >
  sockjs, so removing CRA would have broken the build.
- jest.config.js: setupFiles (whatwg-fetch, previously supplied by
  react-app-polyfill/jsdom) and setupFilesAfterEnv (src/setupTests.ts), both
  of which react-scripts wired up on its own.

SVG imports move to Vite's `?react` convention; bare .svg imports stay asset
URLs. Both paths verified in a browser.

Test suite is now standalone Jest and reaches parity with the CRA baseline
(27 suites / 149 tests). Fixing it surfaced two real defects:
- babel.config.js lacked the automatic JSX runtime, so `npx jest` failed 14
  suites with "React is not defined" while `react-scripts test` passed.
- Two ParentSignUp specs submitted a form with empty `required` inputs and
  passed only because jsdom 16 (Jest 27) did not implement interactive
  constraint validation. jsdom 20 and every real browser block that, so they
  now submit the form directly to exercise the component's own validate().

Also: pin the Dockerfile to node:20.19.0-alpine (was node:18.20.8-alpine, EOL
April 2025), which Vite requires; drop CI=false from the frontend build; and
remove the dead @svgr/webpack devDependency and eslintConfig block that
referenced react-scripts' now-absent eslint-config-react-app.
@Deepesh-Katudia
docs: record the CRA to Vite migration rationale, findings, and follo…
7538865 
…w-ups

Covers why npm overrides could not close the react-scripts-locked findings,
the 47 -> 5 audit result, the four behaviours CRA had been supplying
implicitly, the two pre-existing test defects the migration surfaced, and the
verification that was and was not completed.
@Deepesh-Katudia
Merge origin/main into chore/frontend-cra-to-vite
49a4c66 
Brings in #208 (environment separation), #247 (Tailwind standardization)
and the other changes merged since the branch was cut on 2026-08-23.

Conflicts in Lessons.tsx, ActivitiesModal.tsx and LeaderboardModal.tsx were
import-only: main's side is kept (SCSS modules dropped by #247, environment
imported from ../environments), with SVG components switched from CRA's
`{ ReactComponent as X }` to Vite's `X from './x.svg?react'`.
@Deepesh-Katudia
fix(frontend): inline all REACT_APP_* vars under Vite and verify in CI
3277589 
environment.prod.js (from #208) read vars via process.env[name]. Vite's
define is a literal text replacement and cannot resolve a dynamic key.
Vite 8 rewrites the leftover bare `process.env` to `{}`, so the lookup
compiled to `{}[name]`, which is always undefined. The required-URL guard
then threw on load and blanked the whole site. Jest runs in Node, so the
test suite could not catch it.

- environment.prod.js: reference every variable by its full literal name
- vite.config.mts: define all six REACT_APP_* vars the app reads, loaded
  via loadEnv so .env files work as they did under CRA; NODE_ENV derives
  from the Vite mode
- scripts/verify-build-env.mjs + CI: fail if process.env survives in
  shipped JS/HTML, or if a required URL was not inlined into the bundle
  (the second check is what actually catches the {}[name] case; verified
  it fails against main's old environment.prod.js and passes on this one)
- Dockerfile and volta pin: Node 20.19.0 (EOL) -> 24.21.0 LTS
@Deepesh-Katudia
fix(frontend): point password reset at the configured middleware URL
baa766d 
reset-password.tsx and set-password.tsx read process.env.REACT_APP_API_URL,
which nothing sets (not CI, the Dockerfile, deploy.yml or
tag_build_containers.sh). Production builds therefore called
http://localhost:8000/user/... for both requests. This predates the Vite
migration; CRA behaved the same.

Both now use environment.urls.middlewareURL, the same base every other
/user/ call uses. REACT_APP_API_URL is dropped from vite.config's define.

Kept as its own commit so it can be reverted independently if the endpoint
behaves differently than expected. Verify a real reset email end to end
after deploy.
@Deepesh-Katudia
build(frontend): use npm ci in the Docker build stage
fde80cf 
npm install can resolve versions that differ from package-lock.json, so
image builds could drift from what CI tested. npm ci installs exactly the
lockfile and fails if it is out of sync with package.json.
@Deepesh-Katudia
docs: add the Vite merge plan alongside the migration write-up
bc3b16d 
The plan covers what changed after #208 merged (the env var blank-page bug,
merge conflicts, Node 24, password-reset URL), the prerequisite branches
that gate the merge, and coordination with the CD pipeline. The original
migration write-up stays as the reference for the migration itself.
@ToldYO

ToldYO commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Please fix the conflicts so i cna review

@Deepesh-Katudia
Merge origin/main into chore/frontend-cra-to-vite
29d8df7 
Conflicts:
- package.json: keep main's engines.node ">=24 <25" from the Node 24 pin.
- StreakModal.tsx: keep the branch's `?react` SVG imports, take main's
  removal of the calendar placeholder, and import environment from
  `environments` rather than `environments/environment` (which hardcodes
  the dev URLs in production).

Also fix the same environments/environment import that #250's
activitiesApi.ts brought in without a conflict.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Deepesh-Katudia @ToldYO