Upgrade react-router-dom v6 to react-router v7 - #504
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the UI routing layer from react-router-dom v6 to react-router v7 (where DOM exports are merged into react-router) and updates application/test imports accordingly. It also includes several React 18/test-harness adjustments that appear related to the prerequisite React upgrade referenced in #501.
Changes:
- Replace
react-router-domwithreact-routeracross UI source and test files, including updated Jest mocks. - Update Jest environment setup to polyfill
TextEncoder/TextDecoderand adjust console error handling. - Update UI entrypoint/tests for React 18
createRoot, and add guards/try-catches in workflow graph components to avoid jsdom/SVG-related failures.
Reviewed changes
Copilot reviewed 283 out of 284 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| awx/ui/package.json | Switch routing dependency to react-router v7; includes React/testing-library version bumps and Jest TextEncoder setup file. |
| awx/ui/package-lock.json | Lockfile updates reflecting react-router v7 and dependency upgrades. |
| awx/ui/src/routerCompat.js | Re-export router symbols from react-router as the app’s canonical import site. |
| awx/ui/testUtils/rtlContexts.js | Test harness router wrapper now imports <Router> from react-router. |
| awx/ui/config/jest/textEncoderPolyfill.js | Adds TextEncoder/TextDecoder polyfill for the Jest/jsdom environment. |
| awx/ui/src/setupTests.js | Console error filtering updated (now suppresses additional broad error markers). |
| awx/ui/src/index.js | Uses React 18 createRoot and renders via root.render. |
| awx/ui/src/index.test.js | Updates entrypoint test to mock/assert createRoot().render(...). |
| awx/ui/src/screens/User/UserTokenList/UserTokenList.test.js | Updates router mocks (currently duplicates jest.mock('react-router', ...) in the same file). |
| awx/ui/src/screens/Setting/AzureAD/AzureADDetail/AzureADDetail.test.js | Router mock updated but still mocks useRouteMatch while component uses useMatch. |
| awx/ui/src/screens/Job/WorkflowOutput/WorkflowOutputGraph.js | Adds guards and wraps d3 zoom/fit initialization in try/catch to avoid environment-specific failures. |
| awx/ui/src/screens/Template/WorkflowJobTemplateVisualizer/VisualizerGraph.js | Similar d3 zoom/fit try/catch changes and zoom event guard. |
Files not reviewed (1)
- awx/ui/package-lock.json: Generated file
Comments suppressed due to low confidence (2)
awx/ui/src/screens/User/UserTokenList/UserTokenList.test.js:25
- There are two
jest.mock('react-router', ...)declarations for the same module in this file. Jest will use the last mock factory, so the earlier mock (foruseLocation) is effectively overwritten, which makes the test setup brittle and can lead to realuseLocationleaking into the test unexpectedly.
Combine these into a single jest.mock that overrides both useLocation and useParams in one place.
awx/ui/src/screens/Setting/AzureAD/AzureADDetail/AzureADDetail.test.js:19
- This mock overrides
useRouteMatch, but the component now usesuseMatch(seeAzureADDetail.js). As written, the mock is dead code and can mislead future maintenance (and it won’t control thecategoryparam the component reads).
Mock useMatch (or set the router location via renderWithContexts) so the test setup reflects the actual routing API being used.
| if ( | ||
| !networkRequestUrl && | ||
| !msg.includes('findDOMNode is deprecated') && | ||
| !msg.includes('Uncaught [TypeError:') && | ||
| !msg.includes('The above error occurred in') | ||
| ) { |
| try { | ||
| handleFitGraph(); | ||
| } catch { | ||
| // d3 zoom/layout can fail in environments without full SVG support | ||
| } |
0b20052 to
289c657
Compare
|
All Copilot review comments addressed: Inherited from #501 (fixed in commit c0af29f, picked up via rebase):
Unique to this PR (fixed in commit 289c657):
The pnpm and PR-scope comments do not apply: this project uses npm, and this PR is intentionally stacked on #501. |
289c657 to
218715e
Compare
|
Rebased onto main (includes PR #501 React 18 merge). Clean two commits, no conflicts. Ready to merge. |
|
Just a quick merge conflict after merging the others, but otherwise this is good to go. |
react-router v7 merges react-router-dom into the react-router package. All imports are repointed from 'react-router-dom' to 'react-router' (271 files), and jest.mock calls in 22 test files are updated to match. A TextEncoder polyfill is added to the jest setup because react-router v7's module initialization references TextEncoder, which jsdom does not provide by default. The routerCompat indirection module and the history package (used by the test harness's custom Router) are retained unchanged.
- UserTokenList.test.js: merge duplicate jest.mock('react-router') into
one declaration so both useLocation and useParams are mocked
- AzureADDetail.test.js: mock useMatch instead of useRouteMatch to match
the component's actual import
218715e to
e8b91f6
Compare
SUMMARY
Upgrade from
react-router-dom6.30.4 toreact-router7.18.0. In v7 the separatereact-router-dompackage was merged intoreact-router, so this is primarily a mechanical import rewrite.What changed:
react-router-domwithreact-routerinpackage.json'react-router-dom'to'react-router'jest.mock('react-router-dom', ...)calls in 22 test filesTextEncoderpolyfill to the jest setup (react-router v7 references it at module load; jsdom doesn't provide it)routerCompatmodule comment; retained the module itself as the canonical re-export siteNo API changes — all hooks (
useNavigate,useLocation,useParams,useMatch), components (Routes,Route,Link,Navigate,HashRouter,Router), and utilities (matchPath) are re-exported fromreact-routerunder the same names.The
historypackage (devDependency, used by the test harness's customRouter) is retained unchanged.Depends on: #501
ISSUE TYPE
COMPONENT NAME
UI
ASCENDER VERSION
ADDITIONAL INFORMATION
All 552 test suites pass (2908 tests). Lint clean.