Conversation
Made-with: Cursor # Conflicts: # .eslintrc.js # package.json # yarn.lock
|
View your CI Pipeline Execution ↗ for commit a0a3ad0 ☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3328 +/- ##
===========================================
- Coverage 89.98% 32.84% -57.15%
===========================================
Files 380 347 -33
Lines 5674 4856 -818
Branches 1845 1664 -181
===========================================
- Hits 5106 1595 -3511
- Misses 560 3261 +2701
+ Partials 8 0 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dreamwasp
requested review from
BandanaKM,
LinKCoding,
aresnik11,
jakemhiller and
sh0ji
There was a problem hiding this comment.
Pull request overview
Upgrades the Gamut monorepo to run on React 19 internally while widening published package peer dependency ranges to continue supporting React 18.3, and applies follow-on compatibility fixes across refs, tests, and tooling.
Changes:
- Bumps workspace React/tooling deps (Testing Library, TypeScript-ESLint, framer-motion, react-hook-form) and updates peer ranges to include React 19.
- Refactors several components/utilities for React 19 ref/type behavior (Popover/PopoverContainer, MenuItem, ButtonBase/Anchor, Tip, nullish helpers).
- Updates tests/docs/stories for new render behavior, stricter typing, and React key/ref expectations.
Reviewed changes
Copilot reviewed 87 out of 88 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/gamut-release/src/main.ts | Reorders node imports. |
| packages/variance/src/utils/responsive.ts | Simplifies optional param type. |
| packages/variance/src/types/config.ts | Removes duplicate union type. |
| packages/variance/integration/tests/component.test.tsx | Switches from test-renderer to RTL. |
| packages/styleguide/src/lib/Organisms/Lists & Tables/List/List.stories.tsx | Adds missing React keys. |
| packages/styleguide/src/lib/Organisms/ConnectedForm/ConnectedFormInputs/ConnectedFormInputsTable.tsx | Adds missing React key. |
| packages/styleguide/src/lib/Molecules/Tips/ToolTip/ToolTip.stories.tsx | Adds missing React key. |
| packages/styleguide/src/lib/Molecules/Tips/TipsTable.tsx | Adds missing React keys. |
| packages/styleguide/src/lib/Meta/Best practices.mdx | Doc formatting/quotes cleanup. |
| packages/styleguide/src/lib/Layouts/LayoutGrid/examples.tsx | Safer display of size value. |
| packages/styleguide/src/lib/Foundations/shared/elements.tsx | Adds eslint disable for namespace access. |
| packages/styleguide/src/lib/Foundations/Layout.mdx | Adds missing keys in examples. |
| packages/styleguide/src/lib/Atoms/UtilityComponents/DelayedRenderWrapper/DelayedRenderWrapper.mdx | Clarifies delay wording. |
| packages/styleguide/src/lib/Atoms/PopoverContainer/PopoverContainer.stories.tsx | Adds missing key in map. |
| packages/styleguide/src/lib/Atoms/PopoverContainer/PopoverContainer.mdx | Formats inline code snippet. |
| packages/styleguide/src/lib/Atoms/FormInputs/SelectDropdown/SelectDropdown.mdx | Flattens Callout formatting. |
| packages/gamut/src/utils/react.ts | Uses isNullish; tighter prop typing. |
| packages/gamut/src/utils/nullish.ts | Adds isNullish/isDefined helpers. |
| packages/gamut/src/utils/tests/nullish.test.ts | Unit tests for nullish helpers. |
| packages/gamut/src/Tip/shared/utils.tsx | Simplifies aria-expanded check. |
| packages/gamut/src/Tip/shared/types.tsx | Broadens ref prop types for React 19. |
| packages/gamut/src/Tip/shared/InlineTip.tsx | Ref casting updates for new types. |
| packages/gamut/src/Tip/shared/FloatingTip.tsx | Ref casting + explicit timeout refs. |
| packages/gamut/src/Tip/tests/helpers.tsx | Ref type update + keyboard open helper refactor. |
| packages/gamut/src/Tag/index.tsx | Safer aria-label generation. |
| packages/gamut/src/PopoverContainer/types.ts | Changes targetRef to Ref type. |
| packages/gamut/src/PopoverContainer/hooks.ts | Adds ref resolver helpers + shared target type. |
| packages/gamut/src/PopoverContainer/tests/PopoverContainer.test.tsx | Adds useRef-based targetRef test. |
| packages/gamut/src/PopoverContainer/PopoverContainer.tsx | Uses new ref helpers throughout. |
| packages/gamut/src/Popover/types.tsx | Broadens targetRef/container ref types. |
| packages/gamut/src/Popover/tests/Popover.test.tsx | Adds useRef-based targetRef test. |
| packages/gamut/src/Popover/Popover.tsx | Uses new ref helpers + ref casts. |
| packages/gamut/src/Pagination/utils.tsx | Centralizes animation timing constants. |
| packages/gamut/src/Modals/Modal.tsx | Avoids stringifying non-primitive title for aria props. |
| packages/gamut/src/Modals/Dialog.tsx | Avoids stringifying non-primitive title for aria props. |
| packages/gamut/src/Menu/tests/Menu.test.tsx | Adds ref-forwarding tests. |
| packages/gamut/src/Menu/MenuItem.tsx | Ref narrowing helper for render branches. |
| packages/gamut/src/GridForm/GridForm.tsx | JSX generic removal for ConnectedForm. |
| packages/gamut/src/Form/tests/snapshots/utils.test.tsx.snap | Updates snapshots for React 19 element shape. |
| packages/gamut/src/Form/SelectDropdown/utils.tsx | Replaces != null with isDefined. |
| packages/gamut/src/Form/SelectDropdown/types/internal.ts | Ref type update for internal focus refs. |
| packages/gamut/src/Form/SelectDropdown/elements/multi-value.tsx | Ref casts + focus call tweaks. |
| packages/gamut/src/Form/SelectDropdown/elements/controls.tsx | Ref casts + focus call tweaks. |
| packages/gamut/src/Form/SelectDropdown/elements/containers.tsx | Optional chaining for role check. |
| packages/gamut/src/Form/SelectDropdown/SelectDropdown.tsx | Tightens types for callbacks/options. |
| packages/gamut/src/FeatureShimmer/index.tsx | Adds as const easing typing. |
| packages/gamut/src/FeatureShimmer/tests/FeatureShimmer.test.tsx | Updates IntersectionObserver mock shape. |
| packages/gamut/src/ConnectedForm/utils.tsx | Simplifies validation rules access. |
| packages/gamut/src/ConnectedForm/ConnectedFormGroup.tsx | Stabilizes field id usage. |
| packages/gamut/src/ConnectedForm/ConnectedForm.tsx | Updates generic forwardRef typing. |
| packages/gamut/src/Coachmark/index.tsx | Uses React.JSX.Element return type. |
| packages/gamut/src/Card/styles.tsx | Adds as const easing typing. |
| packages/gamut/src/ButtonBase/tests/ButtonBase.test.tsx | Adds ref-forwarding tests. |
| packages/gamut/src/ButtonBase/ButtonBase.tsx | Ref type changes + narrowing helper. |
| packages/gamut/src/Button/shared/types.ts | React.JSX IntrinsicElements typing + eslint disable. |
| packages/gamut/src/Breadcrumbs/index.tsx | Replaces Object with object bound. |
| packages/gamut/src/Box/GridBox.tsx | Re-exports GridBoxProps type. |
| packages/gamut/src/BarChart/utils/hooks.ts | RefObject nullability updates + width measure tweak. |
| packages/gamut/src/Anchor/index.tsx | Ref narrowing + AnchorBase typing changes. |
| packages/gamut/src/Anchor/tests/Anchor.test.tsx | Adds ref-forwarding tests. |
| packages/gamut/src/AccordionButtonDeprecated/ButtonDeprecated/index.tsx | Renames themes const to _themes. |
| packages/gamut/package.json | Updates deps + React 19 peer range. |
| packages/gamut-tests/src/index.tsx | React.JSX IntrinsicAttributes typing update. |
| packages/gamut-tests/package.json | Pins component-test-setup + React 19 peer. |
| packages/gamut-styles/tsconfig.lib.json | Excludes tests from lib build. |
| packages/gamut-styles/src/variance/utils.ts | Uses React.JSX IntrinsicElements typing. |
| packages/gamut-styles/src/tests/fontUtilsMock.ts | Adds reusable getFonts mock export. |
| packages/gamut-styles/src/tests/fontLoading.test.tsx | Stabilizes font mocks + preload link lookup. |
| packages/gamut-styles/src/tests/AssetProvider.test.tsx | Stabilizes font mocks + preload link lookup. |
| packages/gamut-styles/package.json | React 19 peer range + framer-motion bump. |
| packages/gamut-patterns/package.json | React 19 peer range. |
| packages/gamut-illustrations/package.json | React 19 peer range. |
| packages/gamut-icons/package.json | React 19 peer range. |
| packages/eslint-plugin-gamut/src/prefer-themed.ts | Updates AST node type checks. |
| packages/eslint-plugin-gamut/src/prefer-themed.test.ts | Migrates RuleTester import/usage. |
| packages/eslint-plugin-gamut/src/no-kbd-element.ts | Uses AST_NODE_TYPES constants. |
| packages/eslint-plugin-gamut/src/no-kbd-element.test.ts | Migrates RuleTester import/usage. |
| packages/eslint-plugin-gamut/src/no-inline-style.ts | Uses AST_NODE_TYPES constants. |
| packages/eslint-plugin-gamut/src/no-inline-style.test.ts | Migrates RuleTester import/usage. |
| packages/eslint-plugin-gamut/src/no-css-standalone.ts | Meta docs tweak (recommended removed). |
| packages/eslint-plugin-gamut/src/no-css-standalone.test.ts | Migrates RuleTester import/usage. |
| packages/eslint-plugin-gamut/src/gamut-import-paths.ts | Meta docs tweak (recommended removed). |
| packages/eslint-plugin-gamut/package.json | Bumps @typescript-eslint/utils to v8. |
| package.json | React 19 workspace deps + tooling bumps/resolutions. |
| jest.config.base.ts | Import ordering cleanup. |
| .nx/version-plans/version-plan-1775236316343.md | Adds version plan for majors. |
| .eslintrc.js | Adds import parser settings + enables rule config. |
| export const useScrollingParents = ( | ||
| targetRef: React.RefObject<HTMLElement | null> | ||
| targetRef: React.Ref<PopoverTargetElement | null> | ||
| ): HTMLElement[] => { | ||
| return useMemo(() => { | ||
| if (!targetRef.current) { | ||
| return []; | ||
| } | ||
| return findAllAdditionalScrollingParents(targetRef.current); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [targetRef.current]); | ||
| const target = getRefElement(targetRef); | ||
| if (!target) return []; | ||
| return findAllAdditionalScrollingParents(getTargetAsElement(target)!); | ||
| }, [targetRef]); |
There was a problem hiding this comment.
useScrollingParents memoizes based on [targetRef], but ref object identity typically doesn't change when .current is set. This can permanently cache an empty scrollingParents array from the initial render (when ref.current is still null), which breaks closeOnViewportExit behavior. Derive a stable element value (e.g. const target = getRefElement(targetRef) outside the memo) and include that in the dependency list (or otherwise re-compute when the resolved element becomes available).
| * The target element around which the popover will be positioned. | ||
| * Only ref objects (e.g. from useRef) are supported at runtime; RefCallback is not. | ||
| */ | ||
| targetRef: RefObject<TargetRef>; | ||
| targetRef: Ref<TargetRef | null>; | ||
| /** |
There was a problem hiding this comment.
The public prop type targetRef: Ref<...> allows callback refs, but the implementation explicitly treats function refs as unsupported (getRefElement returns null) and the docs comment says RefCallback isn't supported. To avoid a type/runtime mismatch for consumers, consider changing the type to React.RefObject<...> (or implement callback-ref support by storing the node in state).
| /** | ||
| * The target element around which the popover will be positioned. | ||
| * Only ref objects (e.g. from useRef) are supported at runtime; RefCallback is not. | ||
| */ | ||
| targetRef: React.RefObject< | ||
| Pick<HTMLDivElement, 'getBoundingClientRect' | 'contains'> | ||
| >; | ||
| targetRef: React.Ref<HTMLElement | null>; | ||
|
|
||
| /** | ||
| * The PopoverContainer which contents will be rendered into. | ||
| */ | ||
| popoverContainerRef?: | ||
| | React.RefObject<HTMLDivElement> | ||
| | React.RefCallback<HTMLDivElement>; | ||
| popoverContainerRef?: React.Ref<HTMLDivElement | null>; | ||
|
|
There was a problem hiding this comment.
targetRef is typed as React.Ref<HTMLElement | null>, which includes callback refs, but the underlying positioning hooks treat callback refs as unsupported (and the docstring states RefCallback isn't supported). Consider narrowing this prop type to React.RefObject<HTMLElement | null> (or add runtime support for callback refs) to prevent consumers from passing a ref shape that won't work.
| <ModalContainer | ||
| aria-hidden="false" | ||
| aria-label={ariaLabel} | ||
| aria-labelledby={titleText ? String(titleText) : undefined} | ||
| aria-labelledby={ | ||
| !isNullish(titleText) && | ||
| (typeof titleText === 'string' || typeof titleText === 'number') | ||
| ? String(titleText) | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
aria-labelledby must reference the id of an element that labels the dialog (e.g. the heading’s id), but here it’s being set to the title text itself. This likely results in an invalid reference and the dialog not being properly labelled for screen readers. Consider generating a stable heading id (or using an existing id prop) and applying it to the title element, then set aria-labelledby to that id (or omit aria-labelledby and rely on aria-label).
| <ModalContainer | ||
| aria-hidden="false" | ||
| aria-label="dialog" | ||
| aria-labelledby={String(title)} | ||
| aria-labelledby={ | ||
| !isNullish(title) && | ||
| (typeof title === 'string' || typeof title === 'number') | ||
| ? String(title) | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
aria-labelledby should be an element id reference, but it’s currently set to the title text value. This likely points to a non-existent id and prevents the dialog from being labelled correctly for assistive tech. Consider giving the <Text as="h2"> a stable id and using that id for aria-labelledby (or remove aria-labelledby if aria-label is sufficient).
| /** | ||
| * Ref type for programmatic focus management. | ||
| * Ref type for programmatic focus management (internal refs from useRef). | ||
| * Used for managing focus on select input and remove all button. | ||
| */ | ||
| export type ProgramaticFocusRef = | ||
| | React.MutableRefObject<HTMLDivElement> | ||
| | React.MutableRefObject<null>; | ||
| export type ProgramaticFocusRef = React.RefObject<HTMLDivElement | null>; | ||
|
|
There was a problem hiding this comment.
Type name ProgramaticFocusRef appears misspelled ("Programmatic"). Since this type is only referenced within this file, consider renaming it to ProgrammaticFocusRef for clarity and consistency.
| mockIntersectionObserver.mockReturnValue({ | ||
| observe: jest.fn(), | ||
| unobserve: jest.fn(), | ||
| disconnect: jest.fn(), | ||
| unobserve: jest.fn(), | ||
| }); |
There was a problem hiding this comment.
The mocked IntersectionObserver object literal defines unobserve twice, which TypeScript flags as a duplicate property name and can fail compilation. Remove the duplicate entry (keep a single unobserve mock).
|
📬 Published Alpha Packages:
|
|
🚀 Styleguide deploy preview ready! Preview URL: https://69f2310ea380763887fcefab--gamut-preview.netlify.app |
Overview
Upgrades the Gamut monorepo for React 19 while keeping React 18 in the supported peer range for consumers. This includes:
react,react-dom,react-test-renderer) and aligned Yarn resolutions.@codecademy/gamut) so apps can use React 18.3 or 19.isNullish/isDefined) where strict equality is required.PR Checklist
Testing Instructions
Accessibility: On a subset of the above (PopoverContainer, Modal, Menu, SelectDropdown, ConnectedForm), run VoiceOver : focus order, labels, and escape to dismiss.
PR Previews
mono on 18
mono on 19
monolith on 18