rbrooks/Quest-Board

Watch

Fork 0

[Frontend] Migrate react-router-dom → react-router v8 (removes the GHSA-qwww-vcr4-c8h2 allowlist entry) #284

New issue

Closed

opened 2026-08-05 20:24:20 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-05 20:24:20 +00:00

Contributor

Split out of #282. That issue allowlisted GHSA-qwww-vcr4-c8h2 because there is no fixed version on our line; this is the work that makes the allowlist entry deletable.

Why this is a migration and not a bump

react-router-dom's latest dist-tag is 7.18.2 — the version we're on — and no 8.x was ever published. In v8 the package was folded into react-router itself, which is at 8.3.0. So:

the advisory's fixed version (8.3.0) lives in a package we don't currently depend on directly
npm audit fix --force proposes a downgrade to 7.11.0, not an upgrade
Renovate cannot resolve this on its own — there is nothing on the 7.x line to move to

Scope

47 files under webapp/frontend/src import from react-router-dom. The imported surface is small and entirely mainstream:

BrowserRouter  Link  MemoryRouter  Navigate  Route  Routes
useNavigate    useParams          useSearchParams

No loaders, no actions, no data-router APIs, no RSC. That is about as favourable as a router migration gets — most of the work is likely a mechanical import-specifier change plus whatever v8 renamed or removed.

Also note 10 test files call vi.mock("react-router-dom", …); those mock targets have to move in lockstep or the mocks silently stop applying and the tests pass against the real router.

Not urgent, and worth saying why

The advisory does not apply to us: its own wording is "This only affects your application if you are using the unstable RSC APIs", and this frontend is a plain Vite SPA. So this is hygiene and staying on a supported line, not incident response. It should not jump the queue ahead of roadmap work.

The real cost of staying put is that 7.x is now a legacy line — it will stop getting fixes, and the next react-router advisory may well be one that does apply, with the same "no fixed version available" problem and more urgency.

Approach

Read the official v7 → v8 upgrade guide first; do not infer the delta from the import list.
Change the dependency, then the 47 import sites, then the 10 vi.mock targets.
Frontend suite (431 tests) must stay green, and eslint src clean apart from the one known pre-existing warning in CampaignDetail.jsx:521.
Exercise the app for real — routing regressions (a nested route that silently stops matching, a Navigate redirect that loops) are exactly the class of bug a green unit suite can miss.
Delete the GHSA-qwww-vcr4-c8h2 entry from scripts/npm-audit-allowlist.json in the same PR and confirm node scripts/check_npm_audit.mjs still exits 0 — with the entry gone, that is now a real check rather than a formality.

Acceptance

react-router-dom removed; react-router@^8.3.0 in its place
All 47 import sites and 10 test mocks migrated
431 frontend tests green, lint clean
Manual pass over the main routes (dashboard, campaign detail, session detail, wiki, admin) including a browser refresh on a deep link
Allowlist entry deleted and the audit still exits 0

Labels: frontend

Split out of #282. That issue allowlisted `GHSA-qwww-vcr4-c8h2` because there is no fixed version on our line; this is the work that makes the allowlist entry deletable. ## Why this is a migration and not a bump `react-router-dom`'s `latest` dist-tag **is** `7.18.2` — the version we're on — and **no 8.x was ever published**. In v8 the package was folded into `react-router` itself, which is at 8.3.0. So: - the advisory's fixed version (8.3.0) lives in a package we don't currently depend on directly - `npm audit fix --force` proposes a *downgrade* to 7.11.0, not an upgrade - Renovate cannot resolve this on its own — there is nothing on the 7.x line to move to ## Scope **47 files** under `webapp/frontend/src` import from `react-router-dom`. The imported surface is small and entirely mainstream: ``` BrowserRouter Link MemoryRouter Navigate Route Routes useNavigate useParams useSearchParams ``` No loaders, no actions, no data-router APIs, no RSC. That is about as favourable as a router migration gets — most of the work is likely a mechanical import-specifier change plus whatever v8 renamed or removed. Also note **10 test files** call `vi.mock("react-router-dom", …)`; those mock targets have to move in lockstep or the mocks silently stop applying and the tests pass against the real router. ## Not urgent, and worth saying why The advisory does not apply to us: its own wording is *"This only affects your application if you are using the unstable RSC APIs"*, and this frontend is a plain Vite SPA. So this is **hygiene and staying on a supported line**, not incident response. It should not jump the queue ahead of roadmap work. The real cost of staying put is that 7.x is now a legacy line — it will stop getting fixes, and the next react-router advisory may well be one that *does* apply, with the same "no fixed version available" problem and more urgency. ## Approach 1. Read the official v7 → v8 upgrade guide first; do not infer the delta from the import list. 2. Change the dependency, then the 47 import sites, then the 10 `vi.mock` targets. 3. Frontend suite (431 tests) must stay green, and `eslint src` clean apart from the one known pre-existing warning in `CampaignDetail.jsx:521`. 4. Exercise the app for real — routing regressions (a nested route that silently stops matching, a `Navigate` redirect that loops) are exactly the class of bug a green unit suite can miss. 5. Delete the `GHSA-qwww-vcr4-c8h2` entry from `scripts/npm-audit-allowlist.json` in the same PR and confirm `node scripts/check_npm_audit.mjs` still exits 0 — with the entry gone, that is now a real check rather than a formality. ## Acceptance - [ ] `react-router-dom` removed; `react-router@^8.3.0` in its place - [ ] All 47 import sites and 10 test mocks migrated - [ ] 431 frontend tests green, lint clean - [ ] Manual pass over the main routes (dashboard, campaign detail, session detail, wiki, admin) including a browser refresh on a deep link - [ ] Allowlist entry deleted and the audit still exits 0 Labels: frontend

claude-bot added the

frontend

label

2026-08-05 20:24:24 +00:00

rbrooks referenced this issue from a commit

2026-08-07 04:07:36 +00:00

build: move the frontend toolchain from Node 20 to Node 22 (#284)

rbrooks referenced this issue from a commit

2026-08-07 04:35:36 +00:00

feat(frontend): migrate react-router-dom 7 → react-router 8 (#284)

claude-bot commented

2026-08-07 04:40:48 +00:00

Author

Contributor

Migration landed in 26d63a5, on top of the Node 22 bump in 850fe54. CI green on both. Leaving this open — the manual acceptance item is not something I can verify.

A prerequisite this issue didn't anticipate

react-router@8.3.0 declares engines.node ">=22.22.0" and is ESM-only. Every Node pin in the repo was on 20: ci.yml, dependency-audit.yml, and both Dockerfile frontend stages. So this was never a pure import change — it was gated on moving the whole frontend toolchain, including the production image builder.

That turned out to matter more than the router. Node 20 reached end-of-life on 2026-04-30 — the production frontend image was being built on an unsupported runtime, independent of react-router. 850fe54 moves all four pins together, deliberately: testing on one major while building the image with another is how a runtime-only break ships green.

The rest of v8's floor was already satisfied — vite 8.1.5 (needs ≥7) and react/react-dom 19.2.8 (needs ≥19.2.7).

Also fixed en route: the repo had no .dockerignore, so every build sent ~226 MB of node_modules and the full .git history to the daemon. On a Windows host it failed outright (invalid file request .../node_modules/.bin/acorn — npm's symlinks don't survive the bind mount), which made building the production image locally impossible. CI never saw it, because a fresh checkout has no node_modules.

Scope was smaller than estimated, once verified

I checked the export surface against the installed package rather than inferring it from the upgrade guide. All nine APIs in use — BrowserRouter, Link, MemoryRouter, Navigate, Route, Routes, useNavigate, useParams, useSearchParams — are exported from react-router unchanged. Only RouterProvider and HydratedRouter moved to react-router/dom, and neither is used here.

So it reduced to one uniform specifier change: 68 occurrences across 48 files, including all 10 vi.mock targets. No API renames, no shims.

The lockfile lost ~2,650 net lines — react-router-dom pulled a considerably larger tree than react-router alone.

Acceptance

react-router-dom removed; react-router@^8.3.0 in its place
All import sites and all 10 test mocks migrated
431 frontend tests green, lint clean (1 known CampaignDetail.jsx:521 warning, #21)
Manual pass over the main routes — not done
Allowlist entry deleted and the audit still exits 0

On the last one: with GHSA-qwww-vcr4-c8h2 gone the allowlist is [], and both check_npm_audit.mjs invocations now report "No advisories" rather than "1 allowlisted". The gate is a real check again.

Remaining work

A manual pass, ideally with a browser refresh on a deep link (that is what separates a working router from one that only survives client-side navigation):

/dashboard
/campaigns/:id — deepest nesting
/sessions/:id
/campaigns/:id/wiki and a /wiki/:entryId article
/admin
a logged-out hit on any deep link — exercises AuthGuard's Navigate redirect, the likeliest spot for a redirect loop

A green unit suite cannot catch a Navigate that loops or a nested route that silently stops matching, which is exactly the class this migration could break. Close once that pass is clean.

Migration landed in `26d63a5`, on top of the Node 22 bump in `850fe54`. CI green on both. **Leaving this open** — the manual acceptance item is not something I can verify. ## A prerequisite this issue didn't anticipate `react-router@8.3.0` declares `engines.node ">=22.22.0"` and is ESM-only. Every Node pin in the repo was on **20**: `ci.yml`, `dependency-audit.yml`, and both Dockerfile frontend stages. So this was never a pure import change — it was gated on moving the whole frontend toolchain, including the production image builder. That turned out to matter more than the router. [Node 20 reached end-of-life on 2026-04-30](https://eolradar.com/node-js-20-end-of-life-2026/) — the production frontend image was being built on an unsupported runtime, independent of react-router. `850fe54` moves all four pins together, deliberately: testing on one major while building the image with another is how a runtime-only break ships green. The rest of v8's floor was already satisfied — vite 8.1.5 (needs ≥7) and react/react-dom 19.2.8 (needs ≥19.2.7). Also fixed en route: the repo had **no `.dockerignore`**, so every build sent ~226 MB of `node_modules` and the full `.git` history to the daemon. On a Windows host it failed outright (`invalid file request .../node_modules/.bin/acorn` — npm's symlinks don't survive the bind mount), which made building the production image locally impossible. CI never saw it, because a fresh checkout has no `node_modules`. ## Scope was smaller than estimated, once verified I checked the export surface against the installed package rather than inferring it from the upgrade guide. **All nine** APIs in use — `BrowserRouter`, `Link`, `MemoryRouter`, `Navigate`, `Route`, `Routes`, `useNavigate`, `useParams`, `useSearchParams` — are exported from `react-router` unchanged. Only `RouterProvider` and `HydratedRouter` moved to `react-router/dom`, and neither is used here. So it reduced to one uniform specifier change: 68 occurrences across 48 files, including all 10 `vi.mock` targets. No API renames, no shims. The lockfile lost ~2,650 net lines — `react-router-dom` pulled a considerably larger tree than `react-router` alone. ## Acceptance - [x] `react-router-dom` removed; `react-router@^8.3.0` in its place - [x] All import sites and all 10 test mocks migrated - [x] 431 frontend tests green, lint clean (1 known `CampaignDetail.jsx:521` warning, #21) - [ ] **Manual pass over the main routes** — not done - [x] Allowlist entry deleted and the audit still exits 0 On the last one: with `GHSA-qwww-vcr4-c8h2` gone the allowlist is `[]`, and both `check_npm_audit.mjs` invocations now report **"No advisories"** rather than "1 allowlisted". The gate is a real check again. ## Remaining work A manual pass, ideally with a browser refresh on a deep link (that is what separates a working router from one that only survives client-side navigation): - `/dashboard` - `/campaigns/:id` — deepest nesting - `/sessions/:id` - `/campaigns/:id/wiki` and a `/wiki/:entryId` article - `/admin` - a **logged-out** hit on any deep link — exercises `AuthGuard`'s `Navigate` redirect, the likeliest spot for a redirect loop A green unit suite cannot catch a `Navigate` that loops or a nested route that silently stops matching, which is exactly the class this migration could break. Close once that pass is clean.

claude-bot commented

2026-08-07 05:38:37 +00:00

Author

Contributor

Manual route pass completed on dev by @rbrooks — all routes working. Closing.

Final state: 26d63a5 (migration) on top of 850fe54 (Node 22 floor), both green in CI and verified on the dev stack.

Deploy verification beyond container health:

check	result
SPA root + `/dashboard` `/campaigns/:uuid` `/sessions/:id` `/admin` `/profile`	200, index.html — deep-link refresh works
built bundle references to `react-router-dom`	0
manual browser pass incl. logged-out deep link	✅ clean
audit gates with empty allowlist	"No advisories"

react-router-dom removed; react-router@^8.3.0 in its place
All import sites and all 10 test mocks migrated
431 frontend tests green, lint clean
Manual pass over the main routes
Allowlist entry deleted and the audit still exits 0

GHSA-qwww-vcr4-c8h2 is gone and scripts/npm-audit-allowlist.json now has an empty allow list, so the audit gate is a real check rather than a formality — the next advisory that lands will actually block.

Worth recording for whoever picks up the next router bump: the migration itself was trivial once verified (all nine routing APIs export unchanged from react-router; only RouterProvider/HydratedRouter moved to the react-router/dom subpath). The substantive work was the Node 20 → 22 prerequisite, which turned out to matter on its own — Node 20 had been EOL since 2026-04-30 and the production frontend image was being built on it.

Manual route pass completed on dev by @rbrooks — all routes working. Closing. **Final state:** `26d63a5` (migration) on top of `850fe54` (Node 22 floor), both green in CI and verified on the dev stack. Deploy verification beyond container health: | check | result | |---|---| | SPA root + `/dashboard` `/campaigns/:uuid` `/sessions/:id` `/admin` `/profile` | 200, index.html — deep-link refresh works | | built bundle references to `react-router-dom` | **0** | | manual browser pass incl. logged-out deep link | ✅ clean | | audit gates with empty allowlist | "No advisories" | - [x] `react-router-dom` removed; `react-router@^8.3.0` in its place - [x] All import sites and all 10 test mocks migrated - [x] 431 frontend tests green, lint clean - [x] Manual pass over the main routes - [x] Allowlist entry deleted and the audit still exits 0 `GHSA-qwww-vcr4-c8h2` is gone and `scripts/npm-audit-allowlist.json` now has an empty `allow` list, so the audit gate is a real check rather than a formality — the next advisory that lands will actually block. Worth recording for whoever picks up the next router bump: the migration itself was trivial once verified (all nine routing APIs export unchanged from `react-router`; only `RouterProvider`/`HydratedRouter` moved to the `react-router/dom` subpath). The substantive work was the Node 20 → 22 prerequisite, which turned out to matter on its own — Node 20 had been EOL since 2026-04-30 and the production frontend image was being built on it.

claude-bot closed this issue

2026-08-07 05:38:41 +00:00

claude-bot referenced this issue

2026-08-07 06:03:06 +00:00

chore(release): v3.11.4 #295