Convert the App.js root route tree to react-router v6 Routes - #425
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the UI’s root routing (awx/ui/src/App.js) from react-router v5’s <Switch>/<Route>/<Redirect> API to v6’s <Routes>/<Route>/<Navigate> API using react-router-dom-v5-compat, while keeping the existing HashRouter + CompatRouter bridge in place.
Changes:
- Converted the root
App.jsroute tree andAuthorizedRoutesfrom v5Switch/Redirectto v6Routes/Navigate. - Refactored
ProtectedRouteinto a v6 element guard that conditionally returns children or navigates to/login. - Updated tests in
App.test.jsto cover the authenticated-children render case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
awx/ui/src/App.js |
Migrates root and authorized routing to v6-style Routes and refactors ProtectedRoute and redirect logic. |
awx/ui/src/App.test.js |
Adds coverage for ProtectedRoute rendering children when authenticated. |
|
Thanks for the review. Fixed in |
|
Smoke-tested this in a browser against a running instance (login + 14 top-level routes). The root conversion works: login and the post-login redirect resolve, and the App.js routeConfig dispatcher routes correctly to Dashboard, Jobs, Templates, Projects, Inventories, Credentials, Organizations, Users, Teams, Schedules, Settings, Management Jobs, Notification Templates and Instance Groups. No errors, no not-found, no changes needed here. |
|
On the The rest of the v6 conversion checks out: |
|
Now that all the other page changes are in, and I rebase this one to main, most of those other pages fail to load completely. Things like dashboard and labels load, but the others are blank pages, no errors. Also FYI, in main, the Templates > Schedules no longer loads, was gonna create an issue for it, but figured I could drop it here while you are working on this one. |
Migrate the application root from the react-router v5 <Switch>/<Route>/<Redirect> API to v6 <Routes>/<Route>/<Navigate> via react-router-dom-v5-compat. The HashRouter + CompatRouter bridge stays in place (the package is not flipped to v6-proper yet, since a few screens still use <Switch>). - App root <Switch> -> <Routes>: /login, / -> Navigate /home, and a catch-all path="*" that renders the ProtectedRoute > ConfigProvider > app container. The legacy /*/ trailing-slash redirect is dropped (v6 matches trailing slashes leniently). - ProtectedRoute no longer renders a <Route>; it is a v6 element guard that returns its children (when authenticated) or <Navigate to="/login"> (the loginRedirectOverride locationReplace path is unchanged). - AuthorizedRoutes: <Switch> -> <Routes>; each routeConfig screen mounts at path/* (so the screen's own nested <Routes> resolve); metrics and the not-found fallback likewise; the unauthorized branch redirects to /subscription_management via <Navigate>. The unused match prop passed to screens is dropped (no screen reads it). - App uses useNavigate instead of useHistory. Screens that are still on v5 <Switch> keep working under the v6 root because their <Switch> matches the absolute location; already-migrated screens use absolute v6 paths. Tests: App.test.js (incl. a new authenticated-children case) and index.test.js pass; lint clean.
`redirectURL !== '/' || redirectURL !== '/home'` is always true, so the post-login redirect ran even for '/' and '/home'. Use && so it only navigates when the stored URL is neither of those defaults.
…t routes) This PR converts App.js to v6 <Routes>, mounting each routeConfig screen as a descendant at path="/<x>/*". The merged route-tree PRs (ctrliq#407-ctrliq#427) wrote each list-entry screen's own <Routes> with ABSOLUTE child paths, which only matched while App.js was still v5 (the screens were effectively top-level). As v6 descendants those absolute paths no longer match the relative remainder, so most pages rendered blank / not-found after rebasing onto current main (reported by cigamit). Convert the list-entry screens to RELATIVE paths (add, :id/*, <Route index>): Credentials, Jobs, Projects, Inventories, Hosts, Organizations, Users, Teams, CredentialTypes, WorkflowApprovals, NotificationTemplates, Applications, ExecutionEnvironments, AllSchedules, InstanceGroups, Instances, Settings, ManagementJobs (the last via a basePath template-literal path). Also convert the Template / WorkflowJobTemplate detail <Routes> from absolute /templates/:templateType/:id/... to relative so their tabs resolve under the v6 root. Update each screen's tests to mount the screen as a descendant under a real <Routes><Route path="/<x>/*" element={<Screen/>}/></Routes>, matching production (the old top-level mounts only resolved with absolute paths). Browser-smoke-tested every top-level route plus representative detail tabs against the dev server: all render with no /api/.../undefined/ calls. host_metrics and subscription_usage are license-gated out of routeConfig and are unchanged.
1727013 to
398ba9d
Compare
|
@cigamit good catch, reproduced it. The blank pages come from making I have just pushed a fix here: the 18 entry screens plus the Template/WorkflowJobTemplate detail routes now use relative paths, and their tests remount the screen as a descendant. And the Templates > Schedules one is solved. All load for me now. Shout if you still see it on your end. |
…#484) * Convert the Templates route tree to react-router v6 (last <Switch>) App.js is now v6 (#425) and mounts Templates as a descendant at /templates/*, so Templates.js becomes v6 <Routes> with relative children (job_template/add, workflow_job_template/add, job_template/:id/*, workflow_job_template/:id/*, index list). Template.js and WorkflowJobTemplate.js move their internal <Routes> to relative paths and read useParams from react-router-dom-v5-compat (v5 useParams returns {} as a v6 descendant). Tests remount these under the real v6 parent route. This removes the last <Switch> from main, unblocking the eventual react-router-dom-v5-compat bridge removal. * Convert the 3 Template entry/detail test suites to RTL (v6-mounted) These three files were the only overlap between this PR and the screens/Template RTL conversion (#483). Make this PR own them outright: convert them from enzyme to React Testing Library, mounted under the real v6 parent route so useParams resolves :id. #483 drops these three so the two PRs become independently mergeable in either order, with all three landing as RTL.
SUMMARY
Converts the App.js root route tree to react-router v6
<Routes>. This is the last piece of the v5 to v6 migration (#400 through #427). App.js now dispatches eachrouteConfigscreen as a v6 descendant atpath="/<x>/*", and the outer login,/to/home, andProtectedRoutetree is plain v6.Once App.js is v6, every screen's own
<Routes>becomes a descendant, which v6 matches relative to the parent. The merged route-tree PRs had written each list-entry screen with absolute child paths (path="/credentials",/credentials/add,/credentials/:id/*), and those only matched while App.js was still v5 and the screens were effectively top-level. As descendants they stop matching, so most pages went blank or not-found after rebasing onto currentmain. This PR switches those screens to relative paths so they resolve under the v6 root:add,:id/*, and<Route index>: Credentials, Jobs, Projects, Inventories, Hosts, Organizations, Users, Teams, CredentialTypes, WorkflowApprovals, NotificationTemplates, Applications, ExecutionEnvironments, AllSchedules, InstanceGroups, Instances, Settings, ManagementJobs.<Routes>use relative:templateType/:id/...(Templates.js itself stays a v5-compat<Switch>, which already prefix-matches the full path).<Routes><Route path="/<x>/*" element={<Screen/>}/></Routes>. The old top-level mounts only resolved with absolute paths.ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
I browser-smoke-tested this against the dev server (hash routing) after the change. Every top-level route and a representative set of detail tabs render, with no
/api/v2/.../undefined/...calls:host_metricsandsubscription_usageare license-gated out ofrouteConfig(SUBSCRIPTION_USAGE_MODEL), so they 404 by design. That's unchanged frommain.All 20 affected screen test suites pass (69 tests), and the changed screens lint clean.