refactor: update ref types in components for better null handling#1289
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix the React 18 crash reported in #1282 by preventing react-dom from being bundled into the library output, and also adjusts several component ref/type definitions for React 18/19 type compatibility and null handling.
Changes:
- Update Vite/Rollup library build externals to keep
react-dom(andstyled-components) out of the bundle. - Refactor multiple components/types to adjust
useRefand ref prop typings. - Update
Block’s styled-componentsshouldForwardProphandling (and related type refactors).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/blend/vite.config.ts | Externalizes react-dom (and styled-components) to avoid bundling ReactDOM internals. |
| packages/blend/lib/components/shared/accessibility/AccessibilityDashboard.tsx | Loosens accessibility component registry prop typing. |
| packages/blend/lib/components/Timeline/Timeline.tsx | Ref nullability + forwarded ref assignment casting update. |
| packages/blend/lib/components/Tabs/TabsList.tsx | Ref nullability update for tabs list container ref. |
| packages/blend/lib/components/SingleSelectV2/singleSelectV2.types.ts | Updates ref-related prop types for SingleSelectV2 menu/virtual list. |
| packages/blend/lib/components/Primitives/PrimitiveInput/PrimitiveInput.tsx | Simplifies forwardRef component typing. |
| packages/blend/lib/components/Primitives/Block/Block.tsx | Updates prop filtering and Block prop typing (styled-components v6-related). |
| packages/blend/lib/components/MultiSelectV2/MultiSelectV2MenuSearch.tsx | Updates input ref prop type. |
| packages/blend/lib/components/MultiSelectV2/MultiSelectV2MenuHeader.tsx | Updates search input ref prop type. |
| packages/blend/lib/components/InputsV2/utils/InputSlots/InputSlots.tsx | Updates slot ref prop type. |
| packages/blend/lib/components/InputsV2/TextInputV2/TextInputV2.tsx | Ref nullability update for inputRef. |
| packages/blend/lib/components/Directory/NavItem.tsx | Ref nullability update for union element ref. |
| packages/blend/lib/components/DateRangePicker/CalendarGrid.tsx | Ref nullability update for scroll container ref. |
| packages/blend/lib/components/DataTable/DataTable.tsx | Ref nullability update + forwarded ref assignment casting update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| searchValue: string | ||
| searchPlaceholder?: string | ||
| searchInputRef: React.RefObject<HTMLInputElement | null> | ||
| searchInputRef: React.RefObject<HTMLInputElement> |
There was a problem hiding this comment.
searchInputRef is created with useRef(null) upstream and is null until the input mounts. With @types/react 19, RefObject<T> does not include null in .current, so this should remain React.RefObject<HTMLInputElement | null> to avoid incorrect non-null typing and assignability issues when passing useRef<HTMLInputElement>(null).
| searchInputRef: React.RefObject<HTMLInputElement> | |
| searchInputRef: React.RefObject<HTMLInputElement | null> |
| type InputSlotProps = { | ||
| position: InputSlotPosition | ||
| slotRef: React.RefObject<HTMLDivElement | null> | ||
| slotRef: React.RefObject<HTMLDivElement> |
There was a problem hiding this comment.
slotRef is passed a ref created with useRef(null) (e.g. in TextInputV2), so .current is nullable. Under @types/react 19, RefObject<HTMLDivElement> implies non-null .current; this should be React.RefObject<HTMLDivElement | null> to match actual lifecycle behavior and avoid type incompatibilities at call sites.
| slotRef: React.RefObject<HTMLDivElement> | |
| slotRef: React.RefObject<HTMLDivElement | null> |
| // styled-components v6 expects a `ShouldForwardProp` signature that takes the | ||
| // element target as the 2nd argument (not the defaultValidatorFn). | ||
| const shouldForwardProp: ShouldForwardProp<'web'> = ( | ||
| prop, | ||
| elementToBeCreated | ||
| ) => { | ||
| void elementToBeCreated // required by the type; not needed for our filtering | ||
| if (blockedProps.has(prop)) return false |
There was a problem hiding this comment.
styled(...).withConfig({ shouldForwardProp }) uses the v6 per-component shouldForwardProp callback shape where the 2nd argument is the default validator (when provided), not the element target. The current implementation/comment treats it as an element and ignores the default validator, which can cause styled-components to forward props it would normally filter (e.g. internal props) and the comment is misleading. Consider updating the callback to accept/compose with the provided default validator and adjust the comment accordingly.
| // styled-components v6 expects a `ShouldForwardProp` signature that takes the | |
| // element target as the 2nd argument (not the defaultValidatorFn). | |
| const shouldForwardProp: ShouldForwardProp<'web'> = ( | |
| prop, | |
| elementToBeCreated | |
| ) => { | |
| void elementToBeCreated // required by the type; not needed for our filtering | |
| if (blockedProps.has(prop)) return false | |
| // styled-components v6 per-component `shouldForwardProp` receives the default | |
| // validator as the 2nd argument. We first apply our own blocked prop filter, | |
| // then delegate to the default validator (when provided). | |
| const shouldForwardProp: ShouldForwardProp<'web'> = ( | |
| prop, | |
| defaultValidator | |
| ) => { | |
| if (blockedProps.has(prop)) return false | |
| if (typeof defaultValidator === 'function') { | |
| return defaultValidator(prop) | |
| } |
| 'boxShadow', | ||
| 'cursor', | ||
| 'overflow', | ||
| 'overflowX', | ||
| 'overflowY', | ||
| 'whiteSpace', | ||
| 'transition', | ||
| // Positioning | ||
| 'position', | ||
| 'inset', | ||
| 'top', | ||
| 'right', | ||
| 'bottom', | ||
| 'left', | ||
| 'zIndex', | ||
| 'pointerEvents', | ||
| 'opacity', | ||
| // Pseudo states | ||
| '_hover', | ||
| '_focus', | ||
| '_active', | ||
| '_disabled', | ||
| '_visited', | ||
| '_focusVisible', | ||
|
|
||
| //font | ||
| 'fontSize', | ||
| 'fontWeight', | ||
| 'lineHeight', | ||
| 'letterSpacing', | ||
| 'textAlign', | ||
| 'textTransform', | ||
| 'userSelect', | ||
|
|
||
| 'transform', | ||
| 'willChange', | ||
| 'transformOrigin', | ||
| 'backfaceVisibility', | ||
| 'transition', | ||
| 'transitionDuration', | ||
| 'transitionTimingFunction', | ||
| 'transitionDelay', | ||
| ] | ||
|
|
||
| const shouldForwardProp = (prop: string) => !blockedProps.includes(prop) | ||
|
|
||
| ]) |
There was a problem hiding this comment.
blockedProps contains duplicate entries (e.g. transition appears more than once). With a Set this is functionally harmless, but it’s still extra noise and makes it easier to miss real changes to the list. Consider deduplicating the entries.
| measureElement: (node: Element | null) => void | ||
| loadingComponent?: ReactNode | ||
| hasMore?: boolean | ||
| virtualScrollRef: RefObject<HTMLDivElement | null> | ||
| virtualScrollRef: RefObject<HTMLDivElement> | ||
| } | ||
|
|
||
| export type MenuSearchProps = { | ||
| enabled?: boolean | ||
| hasItems: boolean | ||
| backgroundColor: string | ||
| searchPlaceholder: string | ||
| searchText: string | ||
| onSearchTextChange: (value: string) => void | ||
| searchInputRef: RefObject<HTMLInputElement | null> | ||
| containerRef?: RefObject<HTMLDivElement | null> | ||
| searchInputRef: RefObject<HTMLInputElement> | ||
| containerRef?: RefObject<HTMLDivElement> | ||
| } |
There was a problem hiding this comment.
RefObject<T> in @types/react 19 has a non-null .current (it does not implicitly include null). Refs created with useRef(null) / createRef() are typically RefObject<T | null>, and this component dereferences virtualScrollRef.current conditionally in multiple places. Changing these props to RefObject<HTMLDivElement> / RefObject<HTMLInputElement> makes the types inaccurate and can also break callers passing the common useRef<T>(null) ref object. Suggest switching these back to RefObject<... | null> (and keeping null checks where appropriate).
|
|
||
| export type MultiSelectV2MenuSearchProps = { | ||
| inputRef: RefObject<HTMLInputElement | null> | ||
| inputRef: RefObject<HTMLInputElement> |
There was a problem hiding this comment.
RefObject<T> in @types/react 19 models a non-null .current. Since inputRef will be null before mount (and the parent provides it via useRef(null)), this prop should be typed as RefObject<HTMLInputElement | null>; otherwise common call sites (e.g. useRef<HTMLInputElement>(null)) become incompatible and the type incorrectly implies .current is always available.
| inputRef: RefObject<HTMLInputElement> | |
| inputRef: RefObject<HTMLInputElement | null> |
Summary
update the ref and fix the react 18 issue
Issue Ticket
Closes #1282