react 19 draft #5277
react 19 draft #5277
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Viewers
|
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
react-compiler
|
| Run status |
|
| Run duration | 02m 26s |
| Commit |
|
| Committer | Ibrahim |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
37
|
| View all changes introduced in this branch ↗︎ | |
IbrahimCSAE
requested review from
Copilot and
IbrahimCSAE
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the OHIF codebase from PropTypes to TypeScript interfaces, removing prop-types dependencies in preparation for React 19 compatibility. The migration replaces PropTypes validation with TypeScript type definitions for better type safety and modern React development practices.
- Removed PropTypes imports and validation from React components
- Added TypeScript interfaces to replace PropTypes definitions
- Updated React DOM rendering methods from legacy
ReactDOM.rendertocreateRootAPI - Fixed TypeScript typing issues with ref objects and React element types
Reviewed Changes
Copilot reviewed 117 out of 118 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| platform/ui/src/components/ | Migrated all UI components from PropTypes to TypeScript interfaces |
| platform/ui-next/src/components/ | Updated UI-next components to use TypeScript interfaces instead of PropTypes |
| platform/app/src/ | Converted app-level components and route handlers to TypeScript interfaces |
| extensions/ | Updated extension components to replace PropTypes with TypeScript types |
| package.json | Updated react-shepherd dependency version |
| @@ -1,3 +1,4 @@ | |||
| import { createRoot, createRoot } from "react-dom/client"; | |||
There was a problem hiding this comment.
Duplicate import of createRoot - only one import is needed.
| import { createRoot, createRoot } from "react-dom/client"; | |
| import { createRoot } from "react-dom/client"; |
Copilot uses AI. Check for mistakes.
| if (portalNodes[this.props.group]) { | ||
| // Todo: move this to root.unmount | ||
| ReactDOM.unmountComponentAtNode(portalNodes[this.props.group].node); | ||
| const root = createRoot(portalNodes[this.props.group].node); |
There was a problem hiding this comment.
Creating a new root on every unmount will cause memory leaks. The root should be created once and stored, then reused for render calls and properly unmounted.
Copilot uses AI. Check for mistakes.
| interface ViewportGridProviderProps { | ||
| children?: any; | ||
| service: ViewportGridService; | ||
| } |
There was a problem hiding this comment.
Duplicate interface definition - ViewportGridProviderProps is defined twice. Remove the duplicate definition.
| interface ViewportGridProviderProps { | |
| children?: any; | |
| service: ViewportGridService; | |
| } | |
| // Removed duplicate definition of ViewportGridProviderProps |
Copilot uses AI. Check for mistakes.
| deduplicationInterval?: number; | ||
| } | ||
|
|
||
| interface NotificationProviderProps { |
There was a problem hiding this comment.
Duplicate interface definition - NotificationProviderProps is defined twice with different properties. Merge into a single interface definition.
Copilot uses AI. Check for mistakes.
| children, | ||
| label = 'More', | ||
| icon = 'tool-more-menu', | ||
| isActive |
There was a problem hiding this comment.
Parameter isActive is used in the function but not defined in the NestedMenuProps interface.
Copilot uses AI. Check for mistakes.
| interface LayoutSelectorProps { | ||
| onSelectionChange?(...args: unknown[]): unknown; | ||
| onSelection?(...args: unknown[]): unknown; | ||
| onSelectionPreset?(...args: unknown[]): unknown; | ||
| children: React.ReactNode; | ||
| open?: boolean; | ||
| onOpenChange?(...args: unknown[]): unknown; | ||
| tooltipDisabled?: boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicate interface definition - LayoutSelectorProps is defined twice. Remove the duplicate definition and ensure all required properties are in the single interface.
| interface LayoutSelectorProps { | |
| onSelectionChange?(...args: unknown[]): unknown; | |
| onSelection?(...args: unknown[]): unknown; | |
| onSelectionPreset?(...args: unknown[]): unknown; | |
| children: React.ReactNode; | |
| open?: boolean; | |
| onOpenChange?(...args: unknown[]): unknown; | |
| tooltipDisabled?: boolean; | |
| } |
Copilot uses AI. Check for mistakes.
| import { ShepherdJourneyProvider } from 'react-shepherd'; | ||
|
|
There was a problem hiding this comment.
[nitpick] Import should be grouped with other imports at the top of the file rather than separated by blank lines and other code.
| import { ShepherdJourneyProvider } from 'react-shepherd'; |
Copilot uses AI. Check for mistakes.
|
|
||
| function CallbackPage({ | ||
| userManager, | ||
| onRedirectSuccess |
There was a problem hiding this comment.
Parameter onRedirectSuccess is used in the function but not defined in the CallbackPageProps interface.
Copilot uses AI. Check for mistakes.
|
I think you are going to hit an issue with react shephred shipshapecode/shepherd#3102 |
Thanks for the heads up. I just removed it in the last commit, but still in progress |
to get ready for react compiler