Update React Router to v6 #8739
Conversation
📝 Walkthrough""" WalkthroughThis change migrates the frontend routing system from React Router v5 to v6, replacing deprecated APIs ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hotzenklotz
added
enhancement
frontend
dependencies
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
297-324: Implement complete navigation-blocking migration for React Router v6The current class component only uses a
window.onbeforeunloadhandler (with@ts-ignoreand a lingering TODO) but no internal navigation blocker. To safely guard both browser unloads and in-app route changes when an upload is in progress, please:
- Remove the TODO and all
@ts-ignoredirectives aroundbeforeUnload.
• Change its signature to(event: BeforeUnloadEvent) => string | void.
• IncomponentDidMount, setwindow.onbeforeunload = this.beforeUnload, and clear it incomponentWillUnmount.- Introduce two HOCs at the bottom of
frontend/javascripts/admin/dataset/dataset_upload_view.tsx:import { UNSAFE_NavigationContext as NavigationContext, useNavigate } from 'react-router-dom'; import { useContext, useEffect } from 'react'; // HOC to inject `navigate` function withNavigation(Component) { return function Wrapped(props) { const navigate = useNavigate(); return <Component {...props} navigate={navigate} />; }; } // Hook to block navigations function useNavBlocker(shouldBlock, message) { const { navigator } = useContext(NavigationContext); useEffect(() => { if (!shouldBlock) return; const unblock = navigator.block(tx => { if (window.confirm(message)) { unblock(); tx.retry(); } }); return unblock; }, [navigator, shouldBlock, message]); } // HOC to apply the blocker function withNavBlocker(Component, getShouldBlock, message) { return function Wrapper(props) { useNavBlocker(getShouldBlock(), message); return <Component {...props} />; }; } export default withNavigation( withNavBlocker( DatasetUploadView, () => this.state.isUploading, messages['dataset.leave_during_upload'] ) );
- Ensure internal navigation is blocked when
isUploadingistrue, and the browser’s unload prompt also fires viabeforeUnload.This completes the migration from
history.blockin v5 to React Router v6’s blocking API for class components.
🧹 Nitpick comments (5)
frontend/javascripts/viewer/controller.tsx (1)
354-354: Consider removing unnecessary generic parameter.The
withRouter<PropsWithRouter>call includes an explicit generic parameter that may be unnecessary. The TypeScript compiler should be able to infer the type automatically. Consider simplifying towithRouter(Controller)for consistency with other components in the migration.-export default connector(withRouter<PropsWithRouter>(Controller)); +export default connector(withRouter(Controller));frontend/javascripts/router/router.tsx (1)
90-107: Well-structured root layout with pending TODO.The
RootLayoutcomponent correctly usesuseWkSelectorhooks and provides a clean structure withOutletfor nested routes. However, the TODO comment on lines 96-97 indicates the command palette logic needs refinement.Consider addressing the TODO to always show the command palette rather than conditionally rendering it based on view state.
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
373-373: Remove redundant type parameter.The type parameter for
withRouteris unnecessary as TypeScript can infer it from the component.-export default connector(withRouter<PropsWithFormAndRouter>(DatasetUploadView)); +export default connector(withRouter(DatasetUploadView));frontend/javascripts/components/secured_route.tsx (1)
17-17: Align callback type with its usage.The
serverAuthenticationCallbackis typed to accept arguments but is called without any. Update the type to reflect actual usage.- serverAuthenticationCallback?: (...args: Array<any>) => any; + serverAuthenticationCallback?: () => boolean | Promise<boolean>;Also applies to: 30-30
frontend/javascripts/router/route_wrappers.tsx (1)
315-317: Consider early return pattern for clarity.export function OnboardingRouteWrapper() { - return !features()?.isWkorgInstance ? <Onboarding /> : <PageNotFoundView />; + if (features()?.isWkorgInstance) { + return <PageNotFoundView />; + } + return <Onboarding />; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (49)
frontend/javascripts/admin/account/account_password_view.tsx(3 hunks)frontend/javascripts/admin/account/account_settings_view.tsx(3 hunks)frontend/javascripts/admin/auth/accept_invite_view.tsx(2 hunks)frontend/javascripts/admin/auth/finish_reset_password_view.tsx(1 hunks)frontend/javascripts/admin/auth/login_view.tsx(1 hunks)frontend/javascripts/admin/auth/registration_view.tsx(3 hunks)frontend/javascripts/admin/auth/start_reset_password_view.tsx(1 hunks)frontend/javascripts/admin/auth/verify_email_view.tsx(4 hunks)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(6 hunks)frontend/javascripts/admin/dataset/dataset_add_view.tsx(6 hunks)frontend/javascripts/admin/dataset/dataset_upload_view.tsx(4 hunks)frontend/javascripts/admin/dataset/dataset_url_import.tsx(1 hunks)frontend/javascripts/admin/onboarding.tsx(3 hunks)frontend/javascripts/admin/organization/organization_danger_zone_view.tsx(1 hunks)frontend/javascripts/admin/organization/organization_notifications_view.tsx(2 hunks)frontend/javascripts/admin/organization/organization_overview_view.tsx(2 hunks)frontend/javascripts/admin/organization/organization_view.tsx(3 hunks)frontend/javascripts/admin/project/project_create_view.tsx(2 hunks)frontend/javascripts/admin/project/project_list_view.tsx(2 hunks)frontend/javascripts/admin/scripts/script_create_view.tsx(3 hunks)frontend/javascripts/admin/task/task_create_form_view.tsx(3 hunks)frontend/javascripts/admin/task/task_create_view.tsx(1 hunks)frontend/javascripts/admin/task/task_list_view.tsx(2 hunks)frontend/javascripts/admin/tasktype/task_type_create_view.tsx(3 hunks)frontend/javascripts/admin/tasktype/task_type_list_view.tsx(1 hunks)frontend/javascripts/admin/user/user_list_view.tsx(2 hunks)frontend/javascripts/admin/voxelytics/task_list_view.tsx(4 hunks)frontend/javascripts/admin/voxelytics/workflow_view.tsx(1 hunks)frontend/javascripts/components/redirect.tsx(2 hunks)frontend/javascripts/components/secured_route.tsx(1 hunks)frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx(3 hunks)frontend/javascripts/dashboard/dashboard_view.tsx(5 hunks)frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx(3 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(3 hunks)frontend/javascripts/dashboard/publication_details_view.tsx(1 hunks)frontend/javascripts/libs/persistence.ts(1 hunks)frontend/javascripts/libs/with_router_hoc.tsx(1 hunks)frontend/javascripts/main.tsx(2 hunks)frontend/javascripts/navbar.tsx(3 hunks)frontend/javascripts/router.tsx(0 hunks)frontend/javascripts/router/page_not_found_view.tsx(1 hunks)frontend/javascripts/router/route_wrappers.tsx(1 hunks)frontend/javascripts/router/router.tsx(1 hunks)frontend/javascripts/viewer/controller.tsx(3 hunks)frontend/javascripts/viewer/view/action_bar_view.tsx(3 hunks)frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx(4 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/router.tsx
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/libs/persistence.ts (3)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
frontend/javascripts/components/redirect.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/auth/start_reset_password_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx (3)
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/dashboard/publication_details_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/tasktype/task_type_list_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/tasktype/task_type_create_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/voxelytics/task_list_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/main.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/account/account_settings_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/onboarding.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/dashboard/dashboard_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/dataset/dataset_add_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/router/router.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/project/project_list_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/admin/auth/verify_email_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/components/secured_route.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/admin/scripts/script_create_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/admin/auth/login_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/navbar.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
🧬 Code Graph Analysis (15)
frontend/javascripts/admin/dataset/dataset_url_import.tsx (2)
frontend/javascripts/libs/react_helpers.tsx (1)
useFetch(35-52)frontend/javascripts/admin/rest_api.ts (1)
getDatastores(1527-1531)
frontend/javascripts/viewer/view/action_bar_view.tsx (1)
frontend/javascripts/libs/window.ts (1)
location(60-60)
frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx (1)
frontend/javascripts/libs/with_router_hoc.tsx (2)
withRouter(20-30)RouteComponentProps(4-8)
frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)
frontend/javascripts/admin/user/user_list_view.tsx (1)
frontend/javascripts/libs/with_router_hoc.tsx (1)
withRouter(20-30)
frontend/javascripts/admin/tasktype/task_type_list_view.tsx (2)
frontend/javascripts/types/prop-types.d.ts (1)
PropTypes(126-147)frontend/javascripts/libs/window.ts (1)
location(60-60)
frontend/javascripts/admin/voxelytics/task_list_view.tsx (1)
frontend/javascripts/libs/window.ts (1)
location(60-60)
frontend/javascripts/main.tsx (3)
frontend/javascripts/components/disable_generic_dnd.ts (1)
DisableGenericDnd(4-22)frontend/javascripts/components/check_certificate_modal.tsx (1)
CheckCertificateModal(8-66)frontend/javascripts/components/terms_of_services_check.tsx (1)
CheckTermsOfServices(25-89)
frontend/javascripts/admin/project/project_create_view.tsx (1)
frontend/javascripts/types/api_types.ts (2)
APITeam(160-164)APIUser(330-345)
frontend/javascripts/admin/auth/accept_invite_view.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)
frontend/javascripts/viewer/controller.tsx (1)
frontend/javascripts/libs/with_router_hoc.tsx (1)
withRouter(20-30)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
frontend/javascripts/libs/with_router_hoc.tsx (2)
RouteComponentProps(4-8)withRouter(20-30)
frontend/javascripts/admin/auth/finish_reset_password_view.tsx (1)
frontend/javascripts/libs/utils.ts (1)
getUrlParamsObjectFromString(508-528)
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (1)
frontend/javascripts/libs/with_router_hoc.tsx (2)
RouteComponentProps(4-8)withRouter(20-30)
frontend/javascripts/admin/scripts/script_create_view.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (97)
frontend/javascripts/admin/task/task_create_view.tsx (1)
12-12: LGTM - Consistent with React Router v6 migration pattern.The removal of explicit
taskIdprop passing aligns with the migration approach where components now obtain route parameters internally usinguseParamshook instead of prop drilling.frontend/javascripts/admin/account/account_password_view.tsx (1)
7-7: LGTM - Correct React Router v6 migration.The migration from
useHistorytouseNavigateis implemented correctly with proper import updates and consistent navigation API usage.Also applies to: 18-18, 28-28
frontend/javascripts/libs/persistence.ts (1)
66-67: LGTM - Updated comments to reflect React Router v6 terminology.The comment updates accurately reflect the new React Router v6 API while maintaining the important explanation about focus-loss bugs.
frontend/javascripts/admin/dataset/dataset_url_import.tsx (1)
6-6: LGTM - Correct React Router v6 migration.The migration from
useHistorytouseNavigateis implemented correctly with proper import updates and navigation API usage.Also applies to: 9-9, 14-14
frontend/javascripts/admin/auth/start_reset_password_view.tsx (1)
6-6: LGTM - Correct React Router v6 migration.The migration from
useHistorytouseNavigateis implemented correctly with proper import updates and navigation API usage.Also applies to: 11-11, 18-18
frontend/javascripts/admin/task/task_list_view.tsx (2)
41-41: LGTM: Proper React Router v6 migrationThe import change correctly adopts React Router v6's
useParamshook.
59-66: LGTM: Clean parameter extraction and mergingThe implementation correctly extracts route parameters using
useParamsand merges them intoinitialFieldValues. This maintains the component's existing interface while adopting React Router v6 patterns.frontend/javascripts/viewer/view/action_bar_view.tsx (2)
11-11: LGTM: Correct React Router v6 hook importThe migration from
useHistorytouseNavigatealigns with React Router v6 API changes.
142-142: LGTM: Proper navigation implementationThe navigation correctly uses
useNavigateand preserves the hash fragment fromlocation.hash, maintaining the original behavior while adopting React Router v6 patterns.Also applies to: 177-177
frontend/javascripts/components/redirect.tsx (2)
3-3: LGTM: Correct React Router v6 hook importThe migration from
useHistorytouseNavigatefollows React Router v6 patterns.
11-11: LGTM: Proper navigation implementation with optionsThe navigation calls correctly use
useNavigatewith appropriate options:
navigate(newPath)for push behaviornavigate(newPath, { replace: true })for replace behaviorThe absolute URL handling via
location.assign/replaceis correctly preserved.Also applies to: 28-31
frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx (2)
12-12: LGTM: Correct migration to custom withRouter HOCThe import change from
react-router-domto the locallibs/with_router_hoccorrectly adopts the custom HOC that provides React Router v6 compatibility for class components.
170-170: LGTM: Simplified generic type signatureThe export statement correctly uses the simplified generic type signature for the custom
withRouterHOC, removing the unnecessary second type argument.frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx (1)
339-340: LGTM: Updated comments reflect React Router v6 migrationThe comment updates correctly reference
useNavigateinstead of the deprecateduseHistory, accurately explaining why the hook is avoided to prevent search input focus loss.Also applies to: 367-368
frontend/javascripts/admin/auth/login_view.tsx (1)
4-4: LGTM! Clean migration to React Router v6.The migration from
useHistorytouseNavigateis implemented correctly, preserving all navigation logic while adopting the new React Router v6 API.Also applies to: 12-12, 17-17, 19-19
frontend/javascripts/admin/tasktype/task_type_list_view.tsx (1)
26-26: LGTM! Good migration to hook-based parameter extraction.The transition from prop-based to hook-based parameter extraction using
useLocationis well-implemented. The hash parsing withlocation.hash.slice(1)is appropriate for this use case.Also applies to: 39-41
frontend/javascripts/router/page_not_found_view.tsx (1)
4-24: LGTM! Well-structured 404 page component.The component follows React best practices with proper use of Ant Design components and React Router's Link for navigation. The styling and layout are clean and accessible.
frontend/javascripts/admin/auth/registration_view.tsx (1)
9-9: LGTM! Consistent migration to React Router v6.The migration from
useHistorytouseNavigateis correctly implemented across both component variants. All navigation calls have been properly updated:
history.goBack()→navigate(-1)history.push()→navigate()Also applies to: 13-13, 58-58, 61-61, 98-98, 106-106
frontend/javascripts/admin/voxelytics/workflow_view.tsx (1)
321-321: LGTM! Good defensive programming practice.Adding a default empty string value for
workflowNameprevents potentialundefinedissues downstream and makes the code more robust.frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
6-13: LGTM! Good refactoring to use hooks-based state selection.The migration from prop-based organization data to internal state selection using
useWkSelectorandenforceActiveOrganizationaligns well with the React Router v6 migration pattern. This change eliminates prop drilling and makes the component more self-contained while maintaining the same functionality.frontend/javascripts/admin/account/account_settings_view.tsx (3)
4-4: LGTM! Correct React Router v6 migration.The import changes properly replace React Router v5 components with their v6 equivalents (
Outlet,useLocation,useNavigate).
46-47: Excellent URL parsing implementation.The migration from
useHistorytouseNavigateis correct, and the URL parsing logic usingpathname.split("/").filter(Boolean).pop()follows the recommended approach to handle trailing slashes properly, as noted in the retrieved learnings.
68-73: Perfect nested routing implementation.The replacement of explicit route definitions with
<Outlet />and the navigation update fromhistory.pushtonavigatecorrectly implements React Router v6's nested routing pattern.frontend/javascripts/admin/organization/organization_overview_view.tsx (2)
6-10: LGTM! Consistent migration to hooks-based state selection.The import changes correctly add the necessary hooks and selectors for internal state management.
25-28: Good refactoring to eliminate prop drilling.The migration from prop-based organization data to
useWkSelectorwithenforceActiveOrganizationis consistent with the pattern established in other organization components and aligns well with the React Router v6 migration objectives.frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx (3)
21-21: LGTM! Simplified imports align with v6 migration.The import change removes dependency on React Router v5 types, which is consistent with the broader migration goals.
103-104: Good refactoring to reduce coupling.Adding the explicit
titleprop instead of relying onLinkPropsmakes the component interface clearer and reduces coupling to React Router v5 types.
117-117: Improved safety with optional chaining.The use of optional chaining (
onClick?.()) prevents potential errors if the onClick handler is undefined, which is a good defensive programming practice.frontend/javascripts/main.tsx (2)
30-36: LGTM! Good additions for enhanced UI functionality.The new component imports (
CheckCertificateModal,DisableGenericDnd,CheckTermsOfServices,HelpButton) and the React Router v6RouterProviderimport are well-organized and represent appropriate enhancements to the application.
140-144: Perfect React Router v6 migration implementation.The replacement of the old router setup with
<RouterProvider router={router} />is the correct approach for React Router v6. The new UI components are properly positioned in the component tree to ensure they render appropriately alongside the router.frontend/javascripts/admin/tasktype/task_type_create_view.tsx (3)
12-12: LGTM: Correct React Router v6 importsThe migration from
useHistorytouseNavigateanduseParamsis properly implemented according to React Router v6 patterns.
72-75: LGTM: Proper hook usage and parameter extractionThe component correctly removes the
taskTypeIdprop and extracts it directly from URL parameters usinguseParams. TheuseNavigatehook is properly initialized for navigation.
166-166: LGTM: Correct navigation implementationThe migration from
history.push("/taskTypes")tonavigate("/taskTypes")is correct for React Router v6.frontend/javascripts/admin/auth/verify_email_view.tsx (4)
7-7: LGTM: Correct React Router v6 importsThe imports are properly updated to use React Router v6 hooks.
47-49: LGTM: Proper parameter extraction and navigation setupThe component correctly extracts the
tokenparameter usinguseParamswith a safe default value and initializes theuseNavigatehook.
66-66: LGTM: Updated comment reflects new hook usageThe comment correctly reflects that
navigate(nothistory.push) doesn't need to be included in the dependency array.
84-84: LGTM: Correct navigation callThe navigation implementation using
navigate("/")is correct for React Router v6.frontend/javascripts/admin/user/user_list_view.tsx (2)
27-27: LGTM: Correct import for React Router v6 compatibilityThe import is correctly updated to use the custom
withRouterHOC fromlibs/with_router_hoc, which provides React Router v6 compatibility for class components that cannot use hooks directly.
659-659: LGTM: Proper HOC wrapping for React Router v6The export correctly wraps the component with the custom
withRouterHOC to inject routing props (navigate,location,params) for React Router v6 compatibility.frontend/javascripts/admin/organization/organization_notifications_view.tsx (3)
7-7: LGTM: Correct hook importThe
useWkSelectorimport is properly added to access global state.
10-10: LGTM: Correct selector importThe
enforceActiveOrganizationimport is properly added for type-safe organization access.
21-24: LGTM: Proper state management transitionThe component correctly transitions from receiving
organizationas a prop to extracting it from global state usinguseWkSelector. This aligns with the nested routing pattern where parent components use<Outlet />instead of direct prop passing.frontend/javascripts/dashboard/publication_details_view.tsx (2)
7-7: LGTM: Correct React Router v6 importThe
useParamsimport is properly added for parameter extraction.
11-12: LGTM: Proper parameter extractionThe component correctly removes the
publicationIdprop and extracts theidparameter from the URL usinguseParamswith a safe default value. The parameter name mapping (id: publicationId) maintains consistency with the existing code.frontend/javascripts/admin/task/task_create_form_view.tsx (3)
45-45: LGTM - Correct React Router v6 hook importsProper migration from React Router v5 to v6 by importing the new hooks.
306-308: Excellent migration to React Router v6 patternsThe component signature is properly simplified by:
- Removing the
taskIdprop dependency- Using
useParams()to extract route parameters directly- Using
useNavigate()for navigationThis makes the component more self-contained and follows React Router v6 best practices.
381-381: Correct navigation API updateProperly migrated from
history.pushtonavigatewhile maintaining the same navigation behavior.frontend/javascripts/admin/voxelytics/task_list_view.tsx (4)
41-41: Proper React Router v6 hook importsCorrectly added the necessary React Router v6 hooks to replace the deprecated v5 APIs.
267-267: Correct hook migrationProperly replaced
useHistorywithuseNavigatefollowing React Router v6 patterns.
436-436: Correct navigation updateProperly migrated from
history.pushtonavigatemaintaining the same functionality.
681-685: Well-implemented conditional navigationThe migration correctly uses
navigatewith location-based URL construction. The conditional logic properly handles both URL parameter addition and removal using the helper functions.frontend/javascripts/admin/auth/finish_reset_password_view.tsx (4)
5-5: Appropriate imports for URL parameter extractionGood addition of the utility function for parsing URL parameters and proper React Router v6 hook imports.
Also applies to: 7-7
12-14: Excellent refactor to extract token from URLGood migration from prop-based token handling to URL parameter extraction. This makes the component more self-contained and follows better patterns for authentication flows where tokens are typically passed via URL parameters.
17-17: Correct navigation hook migrationProperly replaced
useHistorywithuseNavigatefollowing React Router v6 patterns.
22-22: Consistent token usage and navigation updateThe token validation and API request properly use the URL-derived token, and navigation is correctly migrated to the new React Router v6 API.
Also applies to: 27-27, 32-32
frontend/javascripts/admin/onboarding.tsx (3)
25-25: Proper HOC import for class component compatibilityGood use of the custom
withRouterHOC to enable React Router v6 compatibility for class components. The import structure is correctly updated.Also applies to: 28-28
412-412: Correct navigation API usage with HOCProperly migrated from
history.pushtonavigatewhile maintaining the same navigation behavior. The HOC correctly provides the navigate function.
737-737: Proper HOC compositionCorrectly combines the Redux connector with the custom
withRouterHOC, maintaining the component's access to both Redux state and routing functionality.frontend/javascripts/admin/project/project_create_view.tsx (3)
11-11: Clean migration to React Router v6 hooksExcellent refactoring that:
- Properly imports React Router v6 hooks
- Removes prop dependencies by using
useParams()directly- Makes the component more self-contained
This follows the same successful pattern used throughout the codebase migration.
Also applies to: 18-19
25-25: Correct navigation hook migrationProperly replaced
useHistorywithuseNavigatefollowing React Router v6 best practices.
61-61: Proper navigation API updateCorrectly migrated from
history.pushtonavigatewhile maintaining the same navigation behavior to "/projects".package.json (1)
186-186: React Router DOM v6 version is up-to-dateConfirmed via npm that
react-router-dom@6.30.1is the latest v6 release. No further changes needed.frontend/javascripts/admin/dataset/dataset_add_view.tsx (2)
10-11: Migration to React Router v6 looks goodThe replacement of
useHistorywithuseNavigateand removal of Redux connect aligns with the v6 migration and modern React patterns.Also applies to: 32-33, 261-261
269-269: Navigation updates in modal are correctThe modal function correctly uses the
navigatefunction with proper TypeScript typing.Also applies to: 309-335
frontend/javascripts/admin/organization/organization_view.tsx (1)
4-4: React Router v6 migration with nested routing implemented correctlyThe use of
<Outlet />for nested routing anduseNavigatefor navigation follows v6 patterns correctly. Good use offilter(Boolean)for path parsing to handle empty segments.Also applies to: 41-42, 66-66, 71-71
frontend/javascripts/admin/auth/accept_invite_view.tsx (2)
7-18: Clean migration to hooks-based componentThe refactor from props to hooks (
useParams,useWkSelector) simplifies the component interface and aligns with React Router v6 patterns.
46-46: Navigation update is correctThe change from
history.pushtonavigateis properly implemented.frontend/javascripts/admin/scripts/script_create_view.tsx (2)
3-15: Successful migration to hooks and React Router v6The component correctly uses
useParamsfor route parameters anduseWkSelectorfor state access, eliminating the need for Redux connect.Also applies to: 125-125
49-49: Navigation updated correctlyThe
navigatefunction usage follows React Router v6 patterns.frontend/javascripts/libs/with_router_hoc.tsx (1)
1-31: Excellent HOC implementation for React Router v6 migration!This custom
withRouterHOC is a well-designed solution that bridges the gap between React Router v6's hook-based API and class components that cannot use hooks directly. The implementation correctly:
- Uses React Router v6 hooks (
useLocation,useNavigate,useParams) internally- Provides type-safe prop injection with proper generic constraints
- Maintains all original component props while adding router props
- Follows React HOC patterns correctly
This approach enables a smooth migration path for existing class components.
frontend/javascripts/dashboard/dashboard_view.tsx (4)
15-15: Correct import migration to custom HOC.The import has been properly updated to use the new custom
withRouterHOC instead of the deprecated React Router v5 exports.
44-44: Good type simplification.The
PropsWithRoutertype has been correctly simplified to extend the entireRouteComponentPropstype instead of manually including specific properties.
138-138: Navigation API correctly updated.The navigation calls have been properly migrated from React Router v5's
history.push()to v6'snavigate()function, maintaining the same routing behavior.Also applies to: 244-244
311-311: Clean export simplification.The
withRoutercall has been appropriately simplified by removing unnecessary generic parameters, relying on TypeScript's type inference.frontend/javascripts/viewer/controller.tsx (2)
8-8: Correct import migration to custom HOC.The import has been properly updated to use the new custom
withRouterHOC.
132-133: Verify the @ts-ignore usage is necessary.The
@ts-ignorecomments are suppressing TypeScript errors related to the removal of React Router v5'shistory.blockAPI. While this is part of the intentional migration strategy mentioned in the PR objectives, ensure these suppressions are truly necessary and consider if the function parameters can be properly typed.frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (4)
7-7: Correct import migration.The import has been properly updated to use the custom
withRouterHOC.
68-68: Appropriate type simplification.The
PropsWithRoutertype has been correctly simplified to extendRouteComponentPropsdirectly.
329-329: Navigation API correctly migrated.The navigation call has been properly updated from
history.push()tonavigate(), maintaining the same routing behavior for creating new tracings.
450-450: Clean withRouter usage.The export statement has been appropriately simplified without explicit generic parameters.
frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx (4)
5-5: Correct import migration.The import has been properly updated to use the custom
withRouterHOC.
13-15: Good props integration with HOC.The props type correctly extends
RouteComponentProps, and the component properly destructures thenavigatefunction from the injected router props. This demonstrates the HOC working well with functional components.
62-62: Navigation API correctly updated.The navigation call has been properly migrated from
history.push("/dashboard")tonavigate("/dashboard"), maintaining the redirect behavior after dataset deletion.
77-77: Proper HOC wrapping.The functional component is correctly wrapped with the
withRouterHOC to inject router props.frontend/javascripts/navbar.tsx (2)
756-772: LGTM! Clean migration to hooks-based state access.The refactor successfully replaces Redux
connectHOC with directuseWkSelectorhooks, simplifying the component interface and modernizing the state access pattern. This aligns well with the React Router v6 migration objectives.
960-960: Correct removal of Redux connect wrapper.The direct export properly completes the migration from Redux
connectHOC to hooks-based state management.frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (2)
19-19: Proper migration to custom withRouter HOC.The import change to the custom
withRouterHOC fromlibs/with_router_hoccorrectly addresses React Router v6's removal of the originalwithRouterHOC. The type simplification also looks appropriate.Also applies to: 55-55
663-663: Appropriate use of custom withRouter HOC for class component.The export correctly uses the custom
withRouterHOC to provide React Router v6 functionality to this class-based component, which is the recommended approach mentioned in the PR objectives.frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (3)
28-28: Correct modernization of imports.The imports properly reflect the migration to React Router v6 (
useNavigate) and hooks-based Redux state access (useWkSelector), replacing the deprecateduseHistoryhook and Reduxconnectpattern.Also applies to: 36-37
50-50: Clean migration to hooks-based architecture.The component correctly migrates from props-based Redux state to internal
useWkSelectorusage and adoptsuseNavigatefor routing. The simplified Props interface reflects the removal of Redux connect dependencies.Also applies to: 177-179, 192-192
213-215: Proper adoption of React Router v6 navigation API.The replacement of
history.push()withnavigate()and the direct component export correctly complete the migration to React Router v6 and removal of Redux connect dependency.Also applies to: 729-729
frontend/javascripts/router/router.tsx (3)
1-47: Comprehensive router configuration setup.The imports correctly include all necessary view components, route wrappers, and React Router v6 APIs for the new routing architecture. The organization looks logical and complete.
79-88: Appropriate server-side authentication callback.The
serverAuthenticationCallbackfunction correctly implements server-side authentication by checking annotation visibility, which is essential for protecting private annotations while allowing public access.
109-442: Comprehensive React Router v6 route configuration.The route definitions properly utilize React Router v6 patterns including nested routes,
SecuredRoutecomponents for authentication, and appropriate redirects for legacy URLs. The router creation withcreateBrowserRouterfollows the recommended v6 API.frontend/javascripts/admin/project/project_list_view.tsx (3)
31-31: Clean migration to hooks-based component interface.The imports correctly adopt React Router v6 hooks (
useParams,useLocation) and Redux hooks (useWkSelector). The simplified component signature with no props is appropriate since routing and state data are now accessed internally via hooks.Also applies to: 37-37, 56-57
57-57: Proper adoption of React Router v6 and Redux hooks.The component correctly uses
useParamsfor route parameters,useLocationfor URL state access, anduseWkSelectorwithenforceActiveUserfor type-safe Redux state access. This replaces the previous props-based approach cleanly.Also applies to: 60-62
467-467: Correct completion of Redux connect removal.The direct export properly completes the migration from Redux
connectHOC to hooks-based state management, maintaining the same component interface while modernizing the implementation.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
frontend/javascripts/router/route_wrappers.tsx (1)
50-50: Replacewindow.location.hrefwith React Router'sNavigatecomponent.This was flagged in previous reviews. Use the
Navigatecomponent for consistency with React Router v6 patterns:- window.location.href = "/"; - return null; + return <Navigate to="/" replace />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/router/route_wrappers.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/router/route_wrappers.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
🪛 GitHub Check: frontend-tests
frontend/javascripts/router/route_wrappers.tsx
[failure] 293-293:
Property 'location' does not exist on type 'Location'.
[failure] 274-274:
Property 'location' does not exist on type 'Location'.
[failure] 234-234:
Property 'location' does not exist on type 'Location'.
[failure] 211-211:
Property 'location' does not exist on type 'Location'.
[failure] 134-134:
Property 'location' does not exist on type 'Location'.
[failure] 98-98:
Property 'location' does not exist on type 'Location'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/router/route_wrappers.tsx (1)
25-328: Well-structured React Router v6 migration with consistent patterns.The wrapper functions demonstrate good practices for React Router v6 migration:
- Proper use of
useParamsanduseLocationhooks- Consistent error handling with
coalescefor type safety- Appropriate use of
SecuredRoutefor authentication- Good separation of concerns with async redirects for legacy URLs
Once the
useLocation()destructuring issues are fixed, this provides a solid foundation for the routing system.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… into react-router-v6
| const location = useLocation(); | ||
| const { token } = getUrlParamsObjectFromString(location.search); |
There was a problem hiding this comment.
I'd unify how to extract the token from the url. in accept_invite_view.tsx you use
const { token = "" } = useParams();
| // TODO doublecheck this | ||
| // @ts-ignore newLocation, action are implicit any | ||
| const beforeUnload = (newLocation, action): string | false | void => { |
| const activeOrganization = useWkSelector((state) => state.activeOrganization); | ||
| const activeUser = useWkSelector((state) => state.activeUser); | ||
| const isAuthenticated = activeUser !== null; | ||
| const [isAdditionallyAuthenticated, setIsAdditionallyAuthenticated] = React.useState(false); |
There was a problem hiding this comment.
I'd also prefer to use a "direct import" for use state here to make the usage it like this:
| const [isAdditionallyAuthenticated, setIsAdditionallyAuthenticated] = React.useState(false); | |
| const [isAdditionallyAuthenticated, setIsAdditionallyAuthenticated] = useState(false); |
| children: React.ReactNode; | ||
| requiredPricingPlan?: PricingPlanEnum; | ||
| requiresAdminOrManagerRole?: boolean; | ||
| serverAuthenticationCallback?: (...args: Array<any>) => any; |
There was a problem hiding this comment.
To my understanding serverAuthenticationCallback should fetch whether an annotation given by an id is public or not. So I would rename this to something like checkIfPubliclyAccessible or checkIfPublicResource or isPublicResource or isPubliclyAvailable or so. And then isAdditionallyAuthenticated should be named isSecuredResourcePubliclyAvailableor so. That way the code makes more sense IMO
There was a problem hiding this comment.
Yes, you are right. That variable/callback has been called like this for ages but is not very clear/description. I like your suggestions.
| () => import("admin/voxelytics/workflow_list_view"), | ||
| ); | ||
|
|
||
| const serverAuthenticationCallback = async (id: string) => { |
There was a problem hiding this comment.
Why is this function called serverAuthenticationCallback? It just fetches tbe annotation of a given id and returns whether it is public?
For more, please see my comment in secured_route.tsx
|
|
||
| return ( | ||
| <Layout> | ||
| {/* TODO: always show command pallete; remove logic from router |
There was a problem hiding this comment.
I think this should be a follow up. I need to talk to @knollengewaechs or @philippotto why there is two code paths for initializing the Command Palette. Once in the router (here) and once in the annotation view.
| path="/reports/availableTasks" | ||
| component={AvailableTasksReportView} | ||
| requiresAdminOrManagerRole | ||
| exact |
There was a problem hiding this comment.
The exact props of some routes were lost. Just checking in whether this intended / not necessary anymore or so
| <Route path="/signup" element={<Navigate to="/auth/signup" />} /> | ||
| <Route path="/register" element={<Navigate to="/auth/signup" />} /> | ||
| <Route path="/auth/register" element={<Navigate to="/auth/signup" />} /> |
There was a problem hiding this comment.
Maybe also add a hint that these are legacy routes like above with
{/* Backwards compatibility for old password change URLs */}
| /> | ||
| <Route path="/imprint" element={<Imprint />} /> | ||
| <Route path="/privacy" element={<Privacy />} /> | ||
| <Route path="/links/:key" element={<ShortLinksRouteWrapper />} /> |
There was a problem hiding this comment.
The RouteWithErrorBoundary is no longer needed as the global route now has a global error boundary, correct?
| const beforeUnload = ( | ||
| newLocation: HistoryLocation<unknown>, | ||
| action: HistoryAction, | ||
| ): string | false | void => { | ||
| // @ts-ignore newLocation, action are implicit any | ||
| const beforeUnload = (newLocation, action): string | false | void => { | ||
| // Only show the prompt if this is a proper beforeUnload event from the browser |
There was a problem hiding this comment.
Isn't there an equivalent type in the new lib version for this variables? 😞 ?
There was a problem hiding this comment.
Oh nvm. This is the unintended / unsupported page redirect interruption way anyway and will be patched in another PR, correct?
|
Oh and I jsut pushed a mini fix of a typo which made the code syntax invalid. With this fix I was able to get the frontend running again :) |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/javascripts/router/router.tsx (1)
84-84: Acknowledge existing TODO comment.The TODO comment correctly identifies that command palette logic should be centralized. The current conditional rendering based on authentication and admin view state is a temporary solution.
frontend/javascripts/components/secured_route.tsx (1)
22-47: Review async authentication callback naming.The callback function and variable names could be more descriptive to improve code clarity.
The naming suggestions from previous reviews are valid -
getIsResourcePubliccould be renamed tocheckIfResourceIsPublicfor better clarity.frontend/javascripts/router/route_wrappers.tsx (1)
39-51: Replace hard navigation with React Router navigation.The component uses
window.location.hreffor navigation, which bypasses React Router and causes a full page reload. This is inconsistent with the React Router v6 migration.Replace the imperative navigation with React Router's declarative approach:
export function DashboardRouteRootWrapper() { const { activeUser } = Store.getState(); if (activeUser) { return <DashboardView userId={null} isAdminView={false} initialTabKey={null} />; } - // Hard navigate so that webknossos.org is shown for the wkorg instance. - window.location.href = "/"; - return null; + // Navigate to root so that webknossos.org is shown for the wkorg instance. + return <Navigate to="/" replace />; }
🧹 Nitpick comments (2)
frontend/javascripts/router/router.tsx (1)
78-95: Review authentication state access pattern.The
RootLayoutcomponent usesuseWkSelectorto access authentication state. Consider if this creates unnecessary re-renders when other Redux state changes, since this component is at the root of the routing tree.Consider memoizing the authentication checks if performance becomes an issue:
+import { useMemo } from 'react'; function RootLayout() { - const isAuthenticated = useWkSelector((state) => state.activeUser != null); - const isAdminView = useWkSelector((state) => !state.uiInformation.isInAnnotationView); + const authState = useWkSelector((state) => ({ + isAuthenticated: state.activeUser != null, + isAdminView: !state.uiInformation.isInAnnotationView, + })); + const { isAuthenticated, isAdminView } = useMemo(() => authState, [authState]);frontend/javascripts/router/route_wrappers.tsx (1)
111-157: Review error handling in async redirect.The
CreateExplorativeRouteWrapperhas good validation for URL parameters and mag restrictions, but consider enhancing error handling for the API call.Consider adding more specific error handling for the
createExplorationalAPI call:try { const annotation = await createExplorational( datasetId, tracingType, !!autoFallbackLayer, fallbackLayerName, null, magRestrictions, ); return `/annotations/${annotation.id}`; +} catch (error) { + console.error("Failed to create explorational annotation:", error); + throw new Error("Unable to create annotation. Please try again."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx(4 hunks)frontend/javascripts/components/secured_route.tsx(1 hunks)frontend/javascripts/dashboard/dataset_view.tsx(8 hunks)frontend/javascripts/router/route_wrappers.tsx(1 hunks)frontend/javascripts/router/router.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/dashboard/dataset_view.tsx
- frontend/javascripts/admin/dataset/dataset_upload_view.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/components/secured_route.tsx (2)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
frontend/javascripts/router/router.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/router/route_wrappers.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using `pathname.split("/").pop()` as it fails with trailing slashes. Instead, use `pathname.split("/").filter(Boolean)` to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (9)
frontend/javascripts/router/router.tsx (2)
1-77: LGTM - Clean imports and setup.The imports are well-organized, separating admin views, components, and route wrappers. The async loading of workflow components is appropriate for code splitting.
463-464: LGTM - Proper router creation.The router is correctly created using
createBrowserRouterwith the route configuration. This follows React Router v6 best practices.frontend/javascripts/components/secured_route.tsx (4)
1-21: LGTM - Clean imports and type definitions.The imports correctly use React Router v6 hooks and the type definitions are clear. The migration from class-based to functional component patterns is well-executed.
49-57: LGTM - Proper async effect pattern.The
useEffectcorrectly handles the async authentication check with proper dependencies. The async function is defined inside the effect to avoid stale closures.
59-79: Verify authentication logic completeness.The authentication and authorization flow looks correct. The component properly handles:
- Public resource access when not authenticated
- Pricing plan requirements
- Role-based access control
The logic correctly returns
LoginViewwith redirect, pricing enforcement views, or the protected children based on the authentication state.
81-81: LGTM - Appropriate memoization.Using
memois appropriate here sinceSecuredRouteis used extensively throughout the router and may receive the same props frequently.frontend/javascripts/router/route_wrappers.tsx (3)
24-37: LGTM - Clean authentication and onboarding logic.The
RootRouteWrappercorrectly handles the routing logic for unauthenticated users, onboarding flow, and authenticated dashboard access.
79-109: LGTM - Proper legacy URL handling.The
DatasetSettingsRouteWrappercorrectly handles legacy URLs by usingAsyncRedirectto resolve dataset information asynchronously before redirecting to the modern URL format.
159-169: LGTM - Well-structured route wrappers.The remaining route wrapper components follow consistent patterns:
- Proper use of React Router v6 hooks
- Appropriate error handling for invalid URLs
- Clean async redirect logic for legacy URL resolution
- Good separation of concerns
Also applies to: 209-247, 249-266, 268-301, 303-305
| /> | ||
| {/* Backwards compatibility for old auth token URLs */} | ||
| <Route path="/auth/token" element={<Navigate to="/account/token" />} /> | ||
| {/* Backwards compatibility for old password change URLs */} | ||
| <Route path="/auth/changePassword" element={<Navigate to="/account/password" />} /> | ||
| <Route path="/login" element={<Navigate to="/auth/login" />} /> | ||
|
|
||
| <Route path="/invite/:token" element={<AcceptInviteView />} /> | ||
|
|
||
| <Route path="/verifyEmail/:token" element={<VerifyEmailView />} /> | ||
| {/* Backwards compatibility for signup URLs */} | ||
| <Route path="/signup" element={<Navigate to="/auth/signup" />} /> | ||
| {/* Backwards compatibility for register URLs */} | ||
| <Route path="/register" element={<Navigate to="/auth/signup" />} /> | ||
| {/* Backwards compatibility for register URLs */} | ||
| <Route path="/auth/register" element={<Navigate to="/auth/signup" />} /> | ||
| <Route path="/auth/login" element={<LoginView />} /> | ||
| <Route path="/auth/signup" element={<RegistrationView />} /> | ||
|
|
||
| <Route path="/auth/resetPassword" element={<StartResetPasswordView />} /> | ||
| <Route path="/auth/finishResetPassword" element={<FinishResetPasswordView />} /> | ||
| {/* legacy view mode route */} | ||
| <Route | ||
| path="/datasets/:organizationId/:datasetName/view" | ||
| element={<TracingViewModeLegacyWrapper />} | ||
| /> | ||
| <Route path="/datasets/:datasetNameAndId/view" element={<TracingViewModeRouteWrapper />} /> | ||
| <Route | ||
| path="/datasets/:datasetNameAndId/sandbox/:type" | ||
| element={<TracingSandboxRouteWrapper />} | ||
| /> | ||
| {/* legacy sandbox route */} | ||
| <Route | ||
| path="/datasets/:organizationId/:datasetName/sandbox/:type" | ||
| element={<TracingSandboxLegacyRouteWrapper />} | ||
| /> | ||
| <Route | ||
| path="/datasets/:datasetId/createExplorative/:type" | ||
| element={ | ||
| <SecuredRoute> | ||
| <CreateExplorativeRouteWrapper /> | ||
| </SecuredRoute> | ||
| } | ||
| /> | ||
| { | ||
| // Note that this route has to be beneath all others sharing the same prefix, | ||
| // to avoid url mismatching | ||
| } | ||
| {/*legacy view mode route */} | ||
| <Route | ||
| path="/datasets/:organizationId/:datasetName" | ||
| element={<TracingViewModeLegacyWrapper />} | ||
| /> | ||
| <Route path="/datasets/:datasetNameAndId" element={<TracingViewModeRouteWrapper />} /> | ||
| <Route path="/publications/:id" element={<PublicationDetailView />} /> | ||
| <Route path="/publication/:id" element={<Navigate to="/publications/:id" />} /> | ||
| <Route | ||
| path="/workflows" | ||
| element={ | ||
| <SecuredRoute> | ||
| <AsyncWorkflowListView /> | ||
| </SecuredRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/aiModels" | ||
| element={ | ||
| <SecuredRoute> | ||
| <AiModelListView /> | ||
| </SecuredRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/workflows/:workflowName" | ||
| element={ | ||
| <SecuredRoute> | ||
| <AsyncWorkflowView /> | ||
| </SecuredRoute> | ||
| } | ||
| /> | ||
| <Route path="/imprint" element={<Imprint />} /> | ||
| <Route path="/privacy" element={<Privacy />} /> | ||
| <Route path="/links/:key" element={<ShortLinksRouteWrapper />} /> | ||
| <Route path="/onboarding" element={<OnboardingRouteWrapper />} /> | ||
| <Route | ||
| path="/account" | ||
| element={ | ||
| <SecuredRoute> | ||
| <AccountSettingsView /> | ||
| </SecuredRoute> | ||
| } | ||
| > | ||
| <Route index element={<Navigate to="profile" />} /> | ||
| <Route path="profile" element={<AccountProfileView />} /> | ||
| <Route path="password" element={<AccountPasswordView />} /> | ||
| <Route path="token" element={<AccountAuthTokenView />} /> | ||
| </Route> | ||
| <Route path="*" element={<PageNotFoundView />} /> | ||
| </Route>, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify consistent security enforcement across routes.
The route definitions show good use of SecuredRoute with appropriate role and pricing plan requirements. However, there are some inconsistencies in the security configuration patterns.
Standardize the security prop ordering for better readability:
// Example for consistency - always put role requirements before pricing requirements
<SecuredRoute
- requiredPricingPlan={PricingPlanEnum.Team} requiresAdminOrManagerRole
+ requiresAdminOrManagerRole
+ requiredPricingPlan={PricingPlanEnum.Team}
>Also verify that all sensitive routes have appropriate protection. Routes like /datasets/upload and /taskTypes/create correctly require admin/manager roles.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/router/router.tsx between lines 97 and 461, the security
props on SecuredRoute components are inconsistently ordered and applied. To fix
this, standardize the order of security-related props such as
requiresAdminOrManagerRole, requiredPricingPlan, and checkIfResourceIsPublic
across all SecuredRoute usages for better readability. Additionally, review all
sensitive routes to ensure they have appropriate SecuredRoute protection with
the correct role and pricing plan requirements, adding missing protections where
necessary.
| export function TracingSandboxLegacyRouteWrapper() { | ||
| const { type, datasetName = "", organizationId = "" } = useParams(); | ||
| const location = useLocation(); | ||
|
|
||
| const tracingType = coalesce(TracingTypeEnum, type); | ||
| if (tracingType == null) { | ||
| return <h3>Invalid annotation URL.</h3>; | ||
| } | ||
| const getParams = Utils.getUrlParamsObjectFromString(location.search); | ||
| return ( | ||
| <AsyncRedirect | ||
| redirectTo={async () => { | ||
| const datasetId = await getDatasetIdFromNameAndOrganization( | ||
| datasetName, | ||
| organizationId, | ||
| getParams.token, | ||
| ); | ||
| return `/datasets/${datasetName}-${datasetId}/sandbox/:${tracingType}${location.search}${location.hash}`; | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Fix template string syntax error.
There's a syntax error in the redirect URL construction.
- return `/datasets/${datasetName}-${datasetId}/sandbox/:${tracingType}${location.search}${location.hash}`;
+ return `/datasets/${datasetName}-${datasetId}/sandbox/${tracingType}${location.search}${location.hash}`;The extra colon (:) before ${tracingType} will result in an invalid URL.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function TracingSandboxLegacyRouteWrapper() { | |
| const { type, datasetName = "", organizationId = "" } = useParams(); | |
| const location = useLocation(); | |
| const tracingType = coalesce(TracingTypeEnum, type); | |
| if (tracingType == null) { | |
| return <h3>Invalid annotation URL.</h3>; | |
| } | |
| const getParams = Utils.getUrlParamsObjectFromString(location.search); | |
| return ( | |
| <AsyncRedirect | |
| redirectTo={async () => { | |
| const datasetId = await getDatasetIdFromNameAndOrganization( | |
| datasetName, | |
| organizationId, | |
| getParams.token, | |
| ); | |
| return `/datasets/${datasetName}-${datasetId}/sandbox/:${tracingType}${location.search}${location.hash}`; | |
| }} | |
| /> | |
| ); | |
| export function TracingSandboxLegacyRouteWrapper() { | |
| const { type, datasetName = "", organizationId = "" } = useParams(); | |
| const location = useLocation(); | |
| const tracingType = coalesce(TracingTypeEnum, type); | |
| if (tracingType == null) { | |
| return <h3>Invalid annotation URL.</h3>; | |
| } | |
| const getParams = Utils.getUrlParamsObjectFromString(location.search); | |
| return ( | |
| <AsyncRedirect | |
| redirectTo={async () => { | |
| const datasetId = await getDatasetIdFromNameAndOrganization( | |
| datasetName, | |
| organizationId, | |
| getParams.token, | |
| ); | |
| return `/datasets/${datasetName}-${datasetId}/sandbox/${tracingType}${location.search}${location.hash}`; | |
| }} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/router/route_wrappers.tsx between lines 186 and 206, the
redirect URL string incorrectly includes an extra colon before the template
variable tracingType, causing a syntax error and invalid URL. Remove the colon
before ${tracingType} in the template string to fix the URL construction.
This PR updates React Router (RR) from v5 to v6 following their upgrade guide. There is already react-router v7 but let's go step by step. v7 also seems to have the same API as v6 so we might update soon in a follow up PR (and double check compatibility with React v18/19).
Noteworthy changes and high level concepts:
<Route> elements like we did so farcreateRoutesFromElements, that translates JSX<Route>objects into code. I used that to keep the diff and changes somewhat manageable. We can switch to a purely code based definition in the future.<Routes>do not have arender={() => ...}prop anymore that would run any function. That makes it impossible to do any sort of conditional logic inside the router. I moved all these methods into new React wrapper components.useNavigation,useParams(for accessing route parameters, e.g./tasks/:taskId)<Outlet />component. I used it for the orga & account pages for rendering the various tabs into a comment parent. Also, the router's root element uses an<Outlet>.withRouterHOC was removed. I build a newwithRouterHOC as a workaround for our remaining React class components.useBlocker. Unfortunately, we mostly use the blocker in class based components. There is not easy workaround - so I had to remove some of the blocking stuff. (Dataset Setting are currently being refactored as functional components and will be able to use that once more. Rework: Dataset Settings #8732 )URL of deployed dev instance (used for testing):
Steps to test:
My testing process:
TODOs:
SecuredRoutecomponent? It is not a<Route>any more... maybeAuthRequiredorRequireAuthor something.featurestype definition. It needs too load earlier.Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)