feat: react router v6#1013
Conversation
arunjaindev
requested review from
vikramdevtron and
vivek-devtron
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request migrates the codebase from React Router v5 to v6, a major version upgrade with significant breaking changes. The migration introduces a new centralized routing structure (ROUTER_URLS), updates all routing-related hooks and components, and includes related changes like migrating from ReactDOM.render to ReactDOM.createRoot.
Changes:
- Upgraded react-router-dom from v5.3.0 to v6.30.3
- Created new ROUTER_URLS constants structure in Pages-Devtron-2.0/Shared/Routes/
- Migrated from Switch to Routes, Redirect to Navigate, useHistory to useNavigate, and useRouteMatch to useParams/generatePath
- Updated NavLink className to use function signature with isActive parameter
- Replaced Prompt component with useBlocker hook
- Migrated ReactDOM.render to ReactDOM.createRoot
- Updated breadcrumb components to work with new routing patterns
Reviewed changes
Copilot reviewed 90 out of 91 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Upgraded react-router-dom dependency from v5 to v6 |
| src/Pages-Devtron-2.0/Shared/Routes/routes.ts | New centralized routing constants structure (ROUTER_URLS) |
| src/Shared/Hooks/UsePrompt/UsePrompt.tsx | Replaced Prompt with useBlocker hook for route blocking |
| src/Shared/Components/CICDHistory/TriggerOutput.tsx | Migrated Switch to Routes, updated Route syntax with element prop |
| src/Shared/Components/CICDHistory/Sidebar.tsx | Replaced useHistory with useNavigate, useRouteMatch with useParams |
| src/Common/BreadCrumb/BreadCrumb.tsx | Updated useBreadcrumb hook to use generatePath and accept pathPattern parameter |
| src/Shared/Components/CodeEditor/utils.tsx | Migrated ReactDOM.render to createRoot |
| src/Shared/Components/TreeView/TreeView.component.tsx | Updated NavLink className to function signature |
| src/Common/Constants.ts | Deprecated URLS constant in favor of ROUTER_URLS |
| Multiple type files | Changed interface to type for parameter types (formatting changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }, [handlePageLeave]) | ||
|
|
||
| const blocker = useBlocker(shouldPrompt) |
There was a problem hiding this comment.
The useBlocker hook should receive a function that returns a boolean, not a boolean value directly. In React Router v6, useBlocker expects a function like useBlocker(() => shouldPrompt) or a function that evaluates the blocking condition. Passing a boolean directly may not work as expected.
| useEffect(() => { | ||
| if (blocker.state !== 'blocked') { | ||
| return | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-alert | ||
| const proceed = window.confirm(message) | ||
|
|
||
| if (proceed) { | ||
| blocker.proceed() | ||
| } else { | ||
| blocker.reset() | ||
| } | ||
| }, [blocker, message]) |
There was a problem hiding this comment.
The blocker state check and window.confirm logic should be wrapped in a check to ensure blocker is defined. There may be edge cases where blocker.state could throw an error if blocker is not properly initialized. Consider adding a null check: if (!blocker || blocker.state !== 'blocked') or verify that useBlocker always returns a valid object.
|
|
||
| export function useBreadcrumb(props?: UseBreadcrumbOptionalProps, deps?: any[]): UseBreadcrumbState { | ||
| export function useBreadcrumb(pathPattern: string, props?: UseBreadcrumbOptionalProps, deps?: any[]): UseBreadcrumbState { | ||
| const sep = props?.sep || '/' |
There was a problem hiding this comment.
Should we make params obj?
| export function useBreadcrumb(props?: UseBreadcrumbOptionalProps, deps?: any[]): UseBreadcrumbState { | ||
| export function useBreadcrumb(pathPattern: string, props?: UseBreadcrumbOptionalProps, deps?: any[]): UseBreadcrumbState { | ||
| const sep = props?.sep || '/' | ||
| deps = deps || [] |
There was a problem hiding this comment.
Can you check if deps need to be memoized?
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist