ENG-2987: Upgrade Admin UI Next.js from 14 to 16#7907
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned Files
|
gilluminate
force-pushed
the
gill/ENG-2987/upgrade-nextjs-14-to-16
branch
from
5a73dde to
c2541d6
Compare
gilluminate
force-pushed
the
gill/ENG-2987/upgrade-nextjs-14-to-16
branch
from
c2541d6 to
68283c5
Compare
|
gilluminate
force-pushed
the
gill/ENG-2987/upgrade-nextjs-14-to-16
branch
from
e620dc7 to
29f3061
Compare
`form.validateFields({ validateOnly: true })` was unreliable on mount in
edit mode under antd v6 / React 19 — the Promise didn't resolve in time,
so `submittable` stayed false and the Next button remained disabled.
Since `name` is the only required field, gate `submittable` on its
presence directly, falling back to `monitor?.name` for the initial
render before `useWatch` emits.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gilluminate
force-pushed
the
gill/ENG-2987/upgrade-nextjs-14-to-16
branch
from
3228de5 to
50b1730
Compare
| "analyze:server": "cross-env BUNDLE_ANALYZE=server next build --webpack", | ||
| "build": "next build --webpack", | ||
| "build:test": "NODE_ENV=test next build --webpack", | ||
| "build:vercel": "(cd ../fides-js && npm run build) & (cd ../admin-ui && next build --webpack && node -e \"require('fs').writeFileSync('.next/routes-manifest-deterministic.json', '{}')\" && node -e \"if(process.env.VERCEL){const fs=require('fs'),path=require('path');const src='/vercel/path0/clients',dst='/vercel/path0';for(const e of fs.readdirSync(src)){try{fs.symlinkSync(path.join(src,e),path.join(dst,e),'dir')}catch(err){if(err.code!=='EEXIST')throw err}}}\") & wait", |
There was a problem hiding this comment.
This big ugly command is due to a bug in Vercel CLI where the post-build validation drops intermediate path segments from multi-segment Root Directory. I have a bug report submitted here: vercel/vercel#15937
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: ENG-2987 — Next.js 14 → 16 / React 18 → 19 Upgrade
This is a well-scoped and carefully executed major dependency upgrade. The PR description is detailed, the migration rationale is clear, and the author has methodically addressed each React 19 type-strictness breakage across the codebase. Overall the change is in good shape.
Summary of findings
One item to verify before merge:
gcm.tsgtag pattern (inline comment): ReplacingdataLayer.push(arguments)withdatalay.push(args)pushes anArrayinstead of anIArgumentsobject. The canonical GTM snippet relies onargumentsspecifically. This may work fine in practice, but should be validated against a live GTM/gtag setup — or use// eslint-disable-next-line prefer-rest-paramsto keep the original pattern.
Suggestions / low-risk items:
next-env.d.ts: Theimport "./.next/dev/types/routes.d.ts"line references a build artifact that won't exist on a fresh checkout. Investigate whether TypeScript errors occur before the firstnext devrun, and whether this file should be in.gitignore(Next.js typically owns and regenerates it).build:vercelscript: The inline Node.js for Vercel symlinks is hard to maintain and has limited error handling. Worth extracting to a dedicated script file soon.ConfigureMonitorForm.tsx: Thesubmittablesimplification is pragmatic and the comment explains the reasoning well — just consider adding a note warning future contributors to update the guard if new required fields are added.app-env.d.ts: The global JSX shim is correct;ElementChildrenAttributeis technically deprecated in React 19 but poses no immediate risk.next.config.js: EnablingreactStrictMode: trueis the right call. Watch for double-invocation side-effects in remaining Chakra components during dev-mode testing post-merge.
Positives
- Removing
next-transpile-modulesin favour of built-intranspilePackagesis the correct upgrade path. - The
--webpackflag rationale (Turbopack + Lightning CSS breaking fidesui's:exportpseudo-class) is well-documented inpackage.jsonwith a reference to ENG-3410. - All
@reduxjs/toolkit/dist/query→@reduxjs/toolkit/queryimport fixes are correct —dist/subpaths are non-public API. useCountUp.tsuseRefinitialisation with explicitundefinedanduseRef<number | undefined>is the correct React 19 pattern.- The local
FidesGlobalinterface inPreview.tsx(with TODO ENG-3409) is a clean short-term fix for themoduleResolution: bundlerconstraint. fidesuipeer dep ranges accepting^18 || ^19are correctly set for backward compatibility.
🔬 Codegraph: unavailable
💡 Write /code-review in a comment to re-run this review.
rayharnett
left a comment
rayharnett
left a comment
There was a problem hiding this comment.
Code Review for PR 7907
Branch: gill/ENG-2987/upgrade-nextjs-14-to-16
Focus: Upgrade Next.js from 14 to 16 and React from 18 to 19, including related UI component migrations from Formik to Ant Design (via fidesui).
🚨 Critical Issues (Must Fix)
No critical security or functional breaking issues were identified in the provided diff.
⚠️ Warnings (Should Fix)
1. Potential mismatch in AuthenticateOktaForm error handling
File: clients/admin-ui/src/features/config-wizard/AuthenticateOktaForm.tsx
Description:
The transition from Formik's submission pattern to Ant Design's (via fidesui) uses a new onFinish handler and manually manages the submittable state using an useEffect with form.validateFields({ validateOnly: true }). While this works, it adds complexity.
Rationale:
The current implementation of checkValidity is called on every change via allValues. While functional, if the form becomes very large, frequent validation calls might impact performance slightly, though unlikely for this specific small form. More importantly, ensure that any asynchronous custom validators in rules are properly handled by the Ant Design form instance.
Suggested Solution:
Ensure that all custom validators (like the JSON parse check for the private key) are robust and don't cause infinite loops or unnecessary re-renders. The current implementation looks okay, but keep an eye on performance if more fields are added.
💡 Suggestions (Nitpicks/Improvements)
1. Cleanup of commented out code in [...nextauth].ts
File: clients/admin-ui/src/pages/api/auth/[...nextauth].ts
Description:
The file was deleted, but it contains a large block of commented-out code.
Suggestion:
Since this is a public repository (as per project guidelines), even commented-out code that might contain business logic or old patterns should be cleaned up to keep the codebase professional and avoid cluttering history if not strictly necessary for future reference.
2. Explicitly handle console.error in login.tsx
File: clients/admin-ui/src/pages/login.tsx
Description:
A lint suppression (// eslint-disable-next-line no-console) was added to handle an error log.
Suggestion:
Instead of just suppressing the lint warning, consider using a proper logging utility if one exists in the project (e.g., a service that sends errors to a monitoring tool like Sentry), which would be more consistent with production-grade applications.
3. Webpack flag documentation in package.json
File: clients/admin-ui/package.json
Description:
A comment was added explaining the need for the --webpack flag to avoid Turbopack issues with Lightning CSS.
Suggestion:
This is excellent practice. Documenting "why" a workaround exists (especially one that limits performance like pinning to Webpack) is very helpful for future maintainers.
✅ Good Practices
- Dependency Upgrades: The upgrade from Next.js 14 -> 16 and React 18 -> 19 is handled cleanly, including the necessary
@typesupdates. - UI Component Migration: Moving from Formik to Ant Design/fidesui simplifies the component tree by removing the nested function children pattern required by Formik, making the JSX much flatter and easier to read.
- Testability Improvements: Adding
data-testidattributes (e.g.,input-orgUrl,submit-btn) during the migration is a great move for maintaining/improving E2E and unit test stability. - Documentation of Workarounds: The comment in
package.jsonregarding the Webpack flag is a perfect example of good technical documentation within code.
|
@rayharnett Thanks for the thorough review! A few notes:
|
Restores `dataLayer.push(arguments)` instead of rest-params to maintain the IArguments object that GTM's dataLayer processor expects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jpople
left a comment
jpople
left a comment
There was a problem hiding this comment.
Did a bunch of smoke testing, everything's looking good.
3 participants
Ticket ENG-2987
Description Of Changes
Upgrades the Admin UI from Next.js 14.2.35 to Next.js 16.x (and React 18 to React 19, which Next 15+ requires). Next.js upgrades had been suppressed in Dependabot because earlier attempts broke the
output: "export"static build and conflicted with Chakra v2's portal/focus-trap system under React 19. With the Chakra → Ant Design migration far enough along, this upgrade is now unblocked.Code Changes
next14 → 16,react/react-dom18 → 19, andeslint-config-next/@next/bundle-analyzerto match acrossadmin-ui,fidesui, andsample-appnext-transpile-moduleswith Next's built-intranspilePackagesinnext.config.js--webpackflag tonext dev/next buildto keep webpack as the bundler (Turbopack is now Next 16's default)next lint(removed in Next 16) with directeslintCLI; added.eslintignorereactStrictMode: falseworkaround that was there for Chakra modal bugspages/api/auth/[...nextauth].ts(static export cannot host API routes; this was dead on the exported build anyway)src/features/common/table/hooks/types.d.ts→types.tsand tightened a few call sites where React 19's stricter types flagged issues (useCountUp,CustomList,SystemDataGroup, a few modal/form components,Popup, etc.)app-env.d.tsand updatednext-env.d.ts/tsconfig.jsonfor the new Next 16 type surfaceSteps to Confirm
cd clients/admin-ui && npm iat the repo root, then run the dev server — verify the app boots and the dashboard rendersnpm run prod-export— verifyoutput: "export"still produces a working static bundle (see https://ethyca.atlassian.net/wiki/spaces/EN/pages/3986849842/Testing+Next.js+Static+Export+Locally)Pre-Merge Checklist
CHANGELOG.mdupdated (viachangelog/7907-upgrade-nextjs-14-to-16.yaml)maindowngrade()migration is correct and works