Skip to content

⬆️ chore(frontend): upgrade React Router to 8.3.0 - #425

Open
quentinlebourles-packmind wants to merge 13 commits into
mainfrom
chore/react-router-8-upgrade
Open

⬆️ chore(frontend): upgrade React Router to 8.3.0#425
quentinlebourles-packmind wants to merge 13 commits into
mainfrom
chore/react-router-8-upgrade

Conversation

@quentinlebourles-packmind

Copy link
Copy Markdown
Contributor

What

Upgrades react-router to 8.3.0, which required first migrating apps/frontend from Jest to Vitest 4.1.10.

pnpm audit --prod now reports no known vulnerabilities (GHSA-qwww-vcr4-c8h2 cleared).

Why the runner migration came first

react-router 8 ships ESM only"type": "module", no .cjs anywhere in dist/. apps/frontend ran Jest through ts-jest, which emits CommonJS, so it died on import.meta.hot in dist/production/lib/dom/ssr/routeModules.js: 27 of 82 suites failed as collection errors, zero assertion failures.

@swc/jest does not help — verified by direct probe that SWC's commonjs transform leaves import.meta intact.

Vitest was chosen over Jest's ESM mode because vi.mock hoists exactly like jest.mock. Jest ESM has no mock hoisting, so all 113 jest.mock() calls across 45 files would have needed restructuring into jest.unstable_mockModule plus dynamic await import(). Vitest also resolves the ESM-only dependency natively and reuses the vite.config.ts aliases, letting the pathsToModuleNameMapper duplication in jest.config.ts be deleted outright.

Scope: apps/frontend only. Every packages/* project and every other app stays on Jest 30.

The v8 code changes

Only two were needed:

  • AppLoadContextRouterContextProvider in app/entry.server.tsx (v8 removes the type; under the middleware API the load context is a RouterContextProvider)
  • dropped v8_middleware from react-router.config.ts (became default behaviour; no longer exists as an option)

Verification

Check Result
Full monorepo (26 projects) exit 0
apps/frontend (Vitest) 83 files / 1070 tests
packages/ui (still Jest) 9 suites / 164 tests
apps/api 25 suites / 504 tests
CLI e2e 236 passed / 3 skipped / 0 failed
Frontend build + typecheck exit 0, clean
pnpm audit --prod no known vulnerabilities

The Jest baseline before migration was 82 files / 1066 tests. Nothing was dropped, skipped, or weakened — the delta is new tests added by the bundled fixes below.

Also verified by hand in a browser against the dockerised stack: app boots under v8, deep-linked routes resolve directly, zero console errors.

Bundled fixes

Two related fixes were folded in rather than left dangling:

  • PMLink nested anchors (packages/ui) — to was a dead prop: typed, accepted, silently discarded. Callers passed it believing it navigated, then wrapped a router Link inside to compensate, producing invalid <a> inside <a> and a React hydration error on /sign-in. Removing the prop forces asChild. Confirmed fixed in the live DOM (document.querySelectorAll('a a').length === 0) with the sign-up link still resolving correctly. Note: this is technically a breaking change to @packmind/ui, but no other caller passed to — frontend typecheck, ui build, and all 164 ui tests pass.
  • Mock shapes in the GitHub App specs — Vitest's precise Mock<Procedure> correctly rejects partial mutation stubs that jest's loose Mock<any, any> accepted. Fixed by constructing real members of the UseMutationResult union rather than casting the mismatch away.

Gotchas worth knowing (documented in the plugin knowledge banks)

  • vi is global under globals: true, but the types are not — import type { Mock, Mocked, MockedFunction } from 'vitest'.
  • vi.importActual is async; a synchronous factory is a parse error and fails the whole file.
  • reactRouter() is gated behind !process.env.VITEST — its Fast Refresh preamble needs a real browser document and killed 54 component suites under jsdom.
  • src/test-setup.ts shims globalThis.jest = { advanceTimersByTime }. @testing-library/dom gates fake-timer support on a jest global existing, so without it waitFor polls on real timers and times out. Deliberately one method, not an alias of vi, so a stray jest.fn() still fails loudly.
  • The test glob is src/** + app/**. tests/ and the gitignored .react-router/ are deliberately excluded.

Known gaps (pre-existing, not introduced here)

  • 4 Playwright tests fail locally — invitation activation, two revoked-PAT probes (need real GitHub credentials), and the browser-driven CLI install. Proven environmental by A/B: the identical tests fail at the same file:line on react-router 7 and 8 with everything else held constant. They have no passing e2e locally, so v8's behaviour on those specific flows is unverified — CI with credentials would close that.
  • apps/frontend/tsconfig.spec.json is compiled by no nx target (nx typecheck frontend only runs tsc -p tsconfig.app.json). It reports 91 errors nothing checks. Down from 699 before this migration, but still an unguarded surface.
  • Playwright runs chromium only.

Reviewer notes

The ✅ test(frontend): commits are a mechanical codemod and are best reviewed as a sequence rather than as a squashed diff. Two of them fix defects found mid-migration and are worth a closer look:

  • await the import batch before counting skill uploads — fixes a ~50% flake where two tests sampled a mock while the batch was still in flight (files are read through FileReader before the request goes out). Root-caused by instrumentation, not guessed.
  • shim advanceTimersByTime… — see the @testing-library/dom note above.

If you pull this branch and apps/api fails with Cannot find module '@packmind/marketplaces', that is stale local state, not this PR — run node scripts/select-tsconfig.mjs to regenerate tsconfig.base.effective.json.

🤖 Generated with Claude Code

quentinlebourles-packmind and others added 13 commits August 4, 2026 15:52
…e timers

@testing-library/dom gates fake-timer support on a `jest` global existing,
so under Vitest waitFor polled on real timers while the fake clock was
installed and 8 tests died at the 5s timeout. Shim only the one method it
calls, so a stray jest.fn() reintroduced later still fails loudly.
…onfig

Nx infers `test` from vite.config.ts via the @nx/vitest plugin already
registered in nx.json, so removing the explicit project.json targets is what
flips the runner. Carries testTimeout 15000 over from the retired jest config;
Vitest defaults to 5s, which the heavier component suites exceed under load.
The two upload-count assertions sampled the mock immediately after the
click, while the batch was still in flight: each skill's files are read
through FileReader before its request goes out, so the call landed an
event-loop turn or more later. Under full-suite parallelism that turn
often came too late and the count read 0.

Both now wait for the summary the panel renders once the batch has
settled, which also makes them stricter — a spurious extra upload can no
longer slip past an early sample.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
v8 removes AppLoadContext — the load context is a RouterContextProvider under
the middleware API — and v8_middleware became default behaviour, so the future
flag no longer exists as an option.

Clears GHSA-qwww-vcr4-c8h2; pnpm audit --prod now reports no known
vulnerabilities. The upgrade was blocked until now because v8 ships ESM only
and ts-jest emitted CJS, which failed on import.meta.hot in 27 of 82 suites.
Vitest's precise Mock<Procedure> rejected the partial mutation stubs that
jest's loose jest.Mock had accepted, surfacing 20 TS2352 errors in the
GitHub App specs. Construct real members of the UseMutationResult
discriminated union so the casts hiding the mismatch are unnecessary.

- add createIdleMutationResult/createFailedMutationResult test helpers
- drop the 20 `as ReturnType<typeof useX>` assertions they make redundant

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`to` was declared on IPMLinkProps but destructured away and never
forwarded, so `<PMLink to="/x">` rendered an anchor with no href. The
prop invited callers to nest a router link inside PMLink instead, which
emits invalid `<a>` inside `<a>` markup. Removing it makes the compiler
point callers at `asChild`, the supported way to render as a router link.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "Sign up" link wrapped a react-router `Link` in a plain `PMLink`, so
Chakra's own `<a>` and the router's `<a>` both rendered, tripping React's
"<a> cannot be a descendant of <a>" hydration warning. Nested anchors
also make screen readers and keyboard navigation behave unpredictably.
`asChild` renders the Chakra link *as* the router link instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
React rejects camelCase custom props on DOM elements, so every render of
the sign-up form logged "React does not recognize the `data-testId` prop"
twice. The ids themselves were fine — setAttribute lowercases attribute
names on HTML elements, so the DOM got `data-testid` either way and the
e2e page object kept working — but the warning was pure noise.

Renamed the five props in SignUpWithOrganizationForm and the latent one
in PMPageSection, whose `headerDataTestId` has no caller yet and would
have warned the moment one arrived.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The nested-anchor fix was branched from main, which predates the Vitest
migration, so its new spec arrived using jest.mock. Rename it and drop the
__esModule marker, which is a CJS interop artifact with no meaning under
Vitest.
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR upgrades the frontend to React Router 8 and migrates its Jest suite to Vitest so the ESM-only router can be tested natively.

  • Updates React Router packages and the server-entry context type for the v8 middleware API.
  • Adds Vitest discovery, jsdom setup, coverage, timer compatibility, and Nx target inference while converting the frontend test suite.
  • Corrects PMLink composition to prevent nested anchors and fixes affected test IDs and mutation mocks.

Confidence Score: 5/5

The PR appears safe to merge; no concrete changed-code defect remains after reviewing the router upgrade, test-runner integration, and shared-link API change.

The frontend test target is discoverable from the Vitest-enabled Vite configuration, active test locations are included, React Router 8 compatibility changes match the new API, and all current PMLink callers preserve valid navigation semantics.

Important Files Changed

Filename Overview
package.json Upgrades the React Router packages to 8.3.0 and adds the Vitest 4.1.10 runner and coverage provider.
apps/frontend/vite.config.ts Configures frontend Vitest discovery, setup, jsdom, timeout, and coverage while excluding the browser-only React Router plugin during tests.
apps/frontend/src/test-setup.ts Migrates global setup to Vitest and adds the narrow fake-timer compatibility shim required by Testing Library.
apps/frontend/project.json Removes the explicit Jest target so Nx can infer the frontend Vitest target from its Vite configuration.
apps/frontend/app/entry.server.tsx Replaces the removed AppLoadContext type with React Router 8's RouterContextProvider.
packages/ui/src/lib/components/typography/PMLink.tsx Removes the nonfunctional router-style to prop and documents single-anchor composition through asChild.
apps/frontend/app/routes/_public.sign-in.tsx Uses PMLink asChild around the router link to preserve navigation without nested anchors.
apps/frontend/src/test/mutationResultMocks.ts Adds type-correct TanStack mutation-result factories for migrated Vitest mocks.
pnpm-lock.yaml Locks the React Router and Vitest dependency changes to concrete resolved versions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Nx["Nx test target"] --> Vitest["Vitest 4"]
  Vitest --> Vite["Frontend Vite config"]
  Vite --> Router["React Router 8 ESM modules"]
  Vitest --> Setup["jsdom test setup"]
  Setup --> Tests["src/** and app/** suites"]
  App["Frontend application"] --> Router
  App --> UI["@packmind/ui"]
  UI --> PMLink["PMLink asChild composition"]
Loading

Reviews (1): Last reviewed commit: "✅ test(frontend): run the new sign-in ro..." | Re-trigger Greptile

@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant