ENG-3461: migrate admin-ui to RouterLink and remove legacyBehavior#7942
ENG-3461: migrate admin-ui to RouterLink and remove legacyBehavior#7942gilluminate merged 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: ENG-3461 — RouterLink / legacyBehavior migration
This is a well-scoped, well-executed cleanup PR. The motivation is clear, the new RouterLink component is documented, and the 10-test suite covers all meaningful branches. The changelog entry and manual testing steps are thorough. Overall the change is in good shape.
Summary of findings
- Private Next.js internal import (
next/dist/shared/lib/router/utils/format-url) — TheformatUrlfunction is pulled from Next.js's internal dist path, which is not part of the public API and can break on any Next.js update. A small inlineformatHrefhelper usingURLSearchParamswould remove this fragility entirely. See inline comment onRouterLink.tsx:2.
ℹ️ Informational (no action required, just noting):
-
Silent prop drop for
replace/scroll/prefetchin text mode — These props are only forwarded on the Button path; they silently no-op in text mode. The JSDoc partially covers this but "no-op in text mode" could be called out more explicitly. No current call site passes these in text mode, so there's no active bug. -
Button detection via reference equality —
only.type === Buttoncorrectly identifies directButtonusages but won't catch HOC-wrapped or memoized variants. The JSDoc already documents the limitation. -
MonitorResult.tsx—NextLinkimport retained intentionally — Confirmed: there is a second<NextLink>usage in that file (the monitor name link) that uses the modern non-legacyBehaviorform, so the import is not orphaned. -
Cypress test uses
.parent("a")— Functionally correct but slightly fragile compared to.closest("a"). Minor nit.
What's done well
- All 20
legacyBehavior/passHrefcall sites migrated;git grep 'passHref\|legacyBehavior'returns zero hits after this PR. - Both render paths (Button-wrap via
next/link, text-mode viaTypography.Link+router.push) are correctly implemented with proper modifier-click pass-through. target="_blank"correctly bypasses SPA interception on both paths, andrel="noopener noreferrer"is added where needed (AssessmentSettingsModal).- The
AssessmentSettingsModal.test.tsxnext/routermock addition prevents a previously-latent test breakage. - The
keyprop is preserved on migrated list-rendering usages (e.g.,ConfidenceCard,MonitorResult).
The only item worth addressing before merge is the next/dist internal import on line 2 of RouterLink.tsx.
🔬 Codegraph: unavailable
💡 Write /code-review in a comment to re-run this review.
rayharnett
left a comment
There was a problem hiding this comment.
Code Review: PR 7942
Branch: gill/ENG-2987/link-legacy-behavior
Status: ✅ LGTM (Looks Good To Me)
Summary of Changes
The PR introduces a new shared RouterLink component in the Admin UI to replace usage of next/link with legacyBehavior and passHref. This change aims to unify internal navigation and simplify how components like Ant Design's Button are wrapped for client-side routing.
Key Improvements
1. Unified Navigation Logic
The new RouterLink handles both plain text (using Typography.Link) and component wrapping (specifically detecting single Ant Design Button children) to ensure correct HTML structure without nested anchors. This solves the issue where Next.js and custom components would produce invalid nested <a> tags.
2. Improved Developer Experience
Developers no longer need to remember to use legacyBehavior or passHref when wrapping custom components. The component intelligently detects its children to apply the correct rendering strategy.
3. Enhanced Functionality
- URL Serialization: Built-in support for
UrlObjectserialization via a robustformatHrefutility that handles complex query objects, including arrays and null/undefined values. - Intelligent Prefetching: Supports prefetching on mount (in production) and provides an optimized
onMouseEnterprefetch strategy for non-production environments to improve perceived performance. - Native Browser Compatibility: Correctly handles modifier keys (meta, ctrl, shift, alt) and middle clicks, allowing standard browser features like "Open in new tab" or "Copy link" to function as expected.
4. Comprehensive Testing
A robust test suite (RouterLink.test.tsx) covers:
formatHrefedge cases (query serialization, hash handling, encoding).RouterLinkinteraction patterns (Button wrapping, text mode, prefetching, and modifier key interception).
Detailed Review Findings
🟢 Pass: Technical Correctness & Architecture
- Structural Detection: The use of
isAntButtonChildto decide between wrapping withNextLinkor using a styled anchor is an effective way to avoid the "nested anchor" issue in modern Next.js versions. - Event Handling: The implementation of
onClickwithinRouterLinkcorrectly intercepts only standard left-clicks while allowing native browser navigation for modifier keys. This is crucial for accessibility and UX. - Refactoring Scope: The replacement of
NextLinkacross multiple features (access-policies,consent-settings,data-discovery, etc.) is thorough and consistent.
🟡 Minor Observations / Suggestions
- Complexity in Detection: The
isAntButtonChildlogic relies on a strict structural check (a single child that is exactly theButtoncomponent). While effective, this is "magic" behavior. It's recommended to ensure any future custom components intended for use here are either wrapped in a fragment or handled via an explicit prop if they don't meet this specific criteria. - Prefetching Granularity: The
onMouseEnterprefetching is a great optimization. As the application grows, we should monitor that large tables with many links do not cause excessive network noise during rapid mouse movement.
🔴 Security & Compliance (Zero Tolerance Check)
- Sensitive Data: Verified that no real customer data, internal URLs (
*.ethyca.com), or private credentials were introduced in the code or test data. - Compliance: The PR adheres to all project security guidelines regarding placeholder usage and generic error messages.
Final Verdict
The refactor is well-architected, thoroughly tested, and provides a significant improvement to the codebase's maintainability and consistency by abstracting away the complexities of Next.js link behavior.
rayharnett
left a comment
There was a problem hiding this comment.
Approved, see prior review.
Next 16 has dropped passHref/legacyBehavior from the public Link API. Introduces a shared RouterLink (admin-ui/features/common/nav) that auto-detects antd Button children vs Typography.Link-styled text, so callers no longer need to hand-wire next/link + passHref + legacyBehavior. Migrates all 12 legacyBehavior usages to RouterLink and adds tests for both rendering modes and modifier-click fall-through. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Completes the Next 16 Link migration by sweeping the 8 sites that still used `<NextLink passHref>` on the merge base. Adds `target` and `rel` forwarding to RouterLink so the Slack-config link in AssessmentSettingsModal can keep its `target="_blank"` behaviour, and bails out of SPA interception in text mode when the caller opts out with `target="_blank"`. Migrated: pages/access-policies/index.tsx (New policy button) pages/privacy-assessments/[id].tsx (Back to list button) pages/settings/rbac/index.tsx (Create role button) pages/settings/rbac/roles/new.tsx (Cancel button) home/SystemCoverageCard.tsx (Connect more systems text link) features/privacy-assessments/AssessmentSettingsModal.tsx (Configure Slack, target=_blank) features/user-management/RolesForm.tsx (Cancel button) features/data-discovery-and-detection/action-center/ConfidenceCard.tsx (Review button) After this commit `git grep passHref\|legacyBehavior` in clients/admin-ui returns zero hits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Drop next/dist private import; add inline formatHref helper with 12 unit tests
- Honour replace/scroll/prefetch in text mode (router.replace, scroll option, mount/hover prefetch matching next/link semantics)
- Switch Cypress assertions from .parent("a") to .closest("a") for resilience
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fa318c5 to
9fc6d67
Compare
ControlForm.tsx and pages/access-policies/ gained passHref usages via PR #7918 merging into main. Migrated to RouterLink for consistency. git grep 'passHref|legacyBehavior' in admin-ui/src now returns zero hits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ticket ENG-3461
Description Of Changes
Next.js 16 dropped
passHrefandlegacyBehaviorfrom the public<Link>API, and Next.js 15 emits a runtime deprecation warning forlegacyBehavior. The Admin UI still relied on that pattern in 20 places to wrap aTypography.LinkorButtoninside anext/link.This PR introduces a shared
RouterLinkcomponent (clients/admin-ui/src/features/common/nav/RouterLink.tsx) that replaces every<Link passHref legacyBehavior>call site in admin-ui. It detects whether its only child is an antdButtonand either:next/linkso Next renders its own<a>around the button (preserves prefetch + modifier-click handling), orTypography.Link-styled anchor that intercepts plain left-clicks and callsrouter.pushfor SPA navigation, while letting modifier / middle / right clicks fall through to the browser for new-tab / copy-link behaviour.Also forwards
targetandrelto both paths so callers can still open a link in a new tab (bypassing SPA interception whentarget="_blank").The result is a single anchor per link (no more nested
<a>hack), no deprecation warnings, and consistent SPA navigation across the app. Unit tests cover both branches of the component.After this PR,
git grep 'passHref\|legacyBehavior'inclients/admin-uireturns zero hits. Privacy Center, fides-js, and fidesui already had zero deprecated Link usages and did not need changes.Code Changes
clients/admin-ui/src/features/common/nav/RouterLink.tsx+ jest coverage inRouterLink.test.tsx(10 tests: Button-wrap path, text path, URL-object serialisation, modifier-click pass-through,target="_blank"pass-through,target/relforwarding, customonClick+preventDefault, fragment fall-through).<Link passHref legacyBehavior>:features/access-policies/PolicyCard.tsxfeatures/common/table/cells/LinkCell.tsxfeatures/consent-settings/tcf/PublisherRestrictionsTable.tsxfeatures/data-discovery-and-detection/action-center/ConfidenceCard.tsxfeatures/data-discovery-and-detection/action-center/EmptyMonitorsResult.tsxfeatures/data-discovery-and-detection/action-center/MonitorResult.tsxfeatures/data-discovery-and-detection/action-center/fields/page.tsxfeatures/datamap/datamap-drawer/SystemInfo.tsxfeatures/integrations/IntegrationLinkedSystems.tsxfeatures/privacy-assessments/AssessmentSettingsModal.tsx(usestarget="_blank")features/privacy-requests/dashboard/DuplicateRequestsButton.tsxfeatures/user-management/RolesForm.tsxfeatures/user-management/UserManagementTable.tsxhome/SystemCoverageCard.tsx(text-mode link)pages/access-policies/index.tsxpages/add-systems/multiple.tsxpages/dataset/index.tsxpages/privacy-assessments/[id].tsxpages/settings/rbac/index.tsxpages/settings/rbac/roles/new.tsxSteps to Confirm
All of the below were verified manually against a local
npm run devadmin-ui. After each, confirm the URL changes to the expected destination and novalidateDOMNesting/ nested-anchor /legacyBehavior/passHrefwarnings appear in the console./dataset→ click the + Add dataset button (top right) → lands on/dataset/new./add-systems/multiple→ click the inline Add a system link in the description → lands on/add-systems/manual./data-discovery/action-center→ on any monitor card click Review → lands on/data-discovery/action-center/{website|datastore|infrastructure}/{monitorId}./user-management→ click Add new user (top right) → lands on/user-management/new./integrations→ on any row, click the integration name in the first column (should be a single blue link, ellipsis-truncated, and the row itself should NOT also navigate) → lands on/integrations/{id}./systems/configure/{systemKey}./reporting/datamap→ click a system cell in the table to open the drawer → click View more in the drawer header → lands on/systems/configure/{systemKey}./settings/consenton a TCF config with restrictions → click any Edit button in the restrictions table → lands on/settings/consent/{configId}/{restrictionId}./(home dashboard) → on the System Coverage card, click Connect more systems in the card header (text link, Typography.Link style) → lands on/add-systems/manual.Modifier-click check (optional but worth it): on any of the above links, Cmd/Ctrl-click should open the target in a new tab (not SPA-navigate in place).
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works