From e6ac5b535fa604311ce917371f9c179ecc3372eb Mon Sep 17 00:00:00 2001 From: Rahim Date: Wed, 2 Sep 2026 13:56:36 -0700 Subject: [PATCH] fix(react): project core state per render instead of mutating retained cores Media buttons, LiveButton, Controls.Root, Title, Time.Value, Poster, the popover, tooltip, menu, and dialog roots, VolumePopover.Root, SliderSegments, and the radio option hooks kept a component-lifetime core in `useState` and wrote this render's props, media, and input into it during render, so an abandoned render could leave the committed tree reading another render's values. Build those projections from render-local cores (or a shared instance when the core holds no state) and drop the `'use no memo'` directives that only guarded the retained mutation. Refs #1679 Co-Authored-By: Claude Fable 5.1 --- .../ui/audio-track/use-audio-track-options.ts | 2 - .../use-captions-options.ts | 2 - .../react/src/ui/controls/controls-root.tsx | 4 +- packages/react/src/ui/create-media-button.tsx | 8 ++- .../react/src/ui/dialog/use-dialog-root.ts | 11 ++-- .../src/ui/hooks/create-radio-options-hook.ts | 10 ++-- .../react/src/ui/live-button/live-button.tsx | 6 +- packages/react/src/ui/menu/menu-root.tsx | 12 ++-- .../ui/play-button/tests/play-button.test.tsx | 57 ++++++++++++++++++- .../react/src/ui/popover/popover-root.tsx | 4 +- packages/react/src/ui/poster/poster.tsx | 2 +- .../src/ui/quality/use-quality-options.ts | 2 - .../time-slider-chapters/slider-segments.tsx | 15 ++--- packages/react/src/ui/time/time-value.tsx | 4 +- packages/react/src/ui/title/title.tsx | 9 +-- .../react/src/ui/tooltip/tooltip-root.tsx | 4 +- .../ui/volume-popover/volume-popover-root.tsx | 5 +- 17 files changed, 99 insertions(+), 58 deletions(-) diff --git a/packages/react/src/ui/audio-track/use-audio-track-options.ts b/packages/react/src/ui/audio-track/use-audio-track-options.ts index 8e71dc59a0..dd5db9788b 100644 --- a/packages/react/src/ui/audio-track/use-audio-track-options.ts +++ b/packages/react/src/ui/audio-track/use-audio-track-options.ts @@ -32,8 +32,6 @@ const useAudioTrackRadioOptions = createRadioOptionsHook({ * @param props - Optional `label`, `formatTrack`, and `disabled` overrides. */ export function useAudioTrackOptions(props?: AudioTrackOptionsProps): AudioTrackOptionsResult | null { - 'use no memo'; - return useAudioTrackRadioOptions(props); } diff --git a/packages/react/src/ui/captions-radio-group/use-captions-options.ts b/packages/react/src/ui/captions-radio-group/use-captions-options.ts index 9332861f2a..625d2dc480 100644 --- a/packages/react/src/ui/captions-radio-group/use-captions-options.ts +++ b/packages/react/src/ui/captions-radio-group/use-captions-options.ts @@ -33,8 +33,6 @@ const useCaptionsRadioOptions = createRadioOptionsHook({ * @param props - Optional `label`, `formatTrack`, and `disabled` overrides. */ export function useCaptionsOptions(props?: CaptionsOptionsProps): CaptionsOptionsResult | null { - 'use no memo'; - const result = useCaptionsRadioOptions(props); if (!result) return null; diff --git a/packages/react/src/ui/controls/controls-root.tsx b/packages/react/src/ui/controls/controls-root.tsx index 7d4392026a..5dbf82ab53 100644 --- a/packages/react/src/ui/controls/controls-root.tsx +++ b/packages/react/src/ui/controls/controls-root.tsx @@ -1,7 +1,6 @@ import { ControlsCore, ControlsDataAttrs } from '@videojs/core'; import { selectControls } from '@videojs/core/dom'; import type { ReactNode } from 'react'; -import { useState } from 'react'; import { usePlayer } from '../../player/context'; import { useLogMissingFeature } from '../hooks/use-log-missing-feature'; @@ -14,12 +13,13 @@ export interface ControlsRootProps { /** Manages controls state and provides it to the compound parts. Does not render an element. */ export function ControlsRoot({ children }: ControlsRootProps): ReactNode { const controls = usePlayer(selectControls); - const [core] = useState(() => new ControlsCore()); useLogMissingFeature(!controls, 'Controls.Root', 'controls'); if (!controls) return null; + const core = new ControlsCore(); + core.setMedia(controls); const state = core.getState(); diff --git a/packages/react/src/ui/create-media-button.tsx b/packages/react/src/ui/create-media-button.tsx index f9d7adcb48..7c1dc1ce5e 100644 --- a/packages/react/src/ui/create-media-button.tsx +++ b/packages/react/src/ui/create-media-button.tsx @@ -3,7 +3,7 @@ import { isText, translateText } from '@videojs/core/i18n'; import type { Selector } from '@videojs/store'; import { isUndefined } from '@videojs/utils/predicate'; import type { ForwardedRef, ForwardRefExoticComponent, RefAttributes } from 'react'; -import { forwardRef, useLayoutEffect, useState } from 'react'; +import { forwardRef, useLayoutEffect } from 'react'; import { useTranslator } from '../i18n/context'; import { useContainer, usePlayer } from '../player/context'; @@ -88,12 +88,14 @@ export function createMediaButton, P const shortcut = useHotkeyShortcut(hotkeyAction, hotkeyValue?.(coreProps)); const translator = useTranslator(); - const [core] = useState(() => new CoreClass()); - if (corePropKeys.has('menuTrigger') && isUndefined(coreProps.menuTrigger)) { coreProps.menuTrigger = menuTriggerChild; } + // Project this render's props and media onto a render-local core so an abandoned render never mutates the + // committed one. + const core = new CoreClass(); + core.setProps(coreProps); const { getButtonProps, buttonRef } = useButton({ diff --git a/packages/react/src/ui/dialog/use-dialog-root.ts b/packages/react/src/ui/dialog/use-dialog-root.ts index db6db38c07..e3aa5821fd 100644 --- a/packages/react/src/ui/dialog/use-dialog-root.ts +++ b/packages/react/src/ui/dialog/use-dialog-root.ts @@ -29,10 +29,6 @@ export function useDialogRoot({ idPrefix = 'dialog', interactionRoot, }: UseDialogRootOptions): DialogContextValue { - const [core] = useState(coreFactory); - - core.setProps({ open: controlledOpen, defaultOpen, closeOnEscape }); - const isControlled = controlledOpen !== undefined; const initialOpenRef = useRef(!isControlled && defaultOpen); const onOpenChangeRef = useLatestRef(onOpenChangeProp); @@ -52,9 +48,6 @@ export function useDialogRoot({ const titleId = useSafeId(`${idPrefix}-title`); const descriptionId = useSafeId(`${idPrefix}-desc`); - core.setTitleId(titleId); - core.setDescriptionId(descriptionId); - useLayoutEffect(() => { dialog.setInteractionRoot(interactionRoot ?? null); }, [dialog, interactionRoot]); @@ -83,8 +76,12 @@ export function useDialogRoot({ const input = useSnapshot(dialog.input); const modality = useSnapshot(dialog.modality); + const core = coreFactory(); + core.setProps({ open: controlledOpen, defaultOpen, closeOnEscape }); core.setInput(input); + core.setTitleId(titleId); + core.setDescriptionId(descriptionId); core.setDocumentModal(modality.documentModal); return { diff --git a/packages/react/src/ui/hooks/create-radio-options-hook.ts b/packages/react/src/ui/hooks/create-radio-options-hook.ts index cb185ab4fe..a4dadb070c 100644 --- a/packages/react/src/ui/hooks/create-radio-options-hook.ts +++ b/packages/react/src/ui/hooks/create-radio-options-hook.ts @@ -1,7 +1,6 @@ import type { RadioOption, RadioOptionsState } from '@videojs/core'; import { type Text, type TextParams, translateText } from '@videojs/core/i18n'; import type { UnknownState } from '@videojs/store'; -import { useCallback, useState } from 'react'; import { useTranslator } from '../../i18n/context'; import { usePlayer } from '../../player/context'; @@ -51,15 +50,16 @@ export function createRadioOptionsHook RadioOptionsHookResult, State> | null { return function useRadioOptions(props?: Props): RadioOptionsHookResult, State> | null { - 'use no memo'; - const media = usePlayer(selector); const t = useTranslator(); - const [core] = useState(createCore); + + // A render-local core keeps the projection pure: it derives only from this render's props and media, so nothing + // leaks from abandoned renders and memoizing compilers may cache it safely. + const core = createCore(); core.setProps(props ?? ({} as Props)); - const setValue = useCallback((value: string) => core.selectValue(media!, value), [core, media]); + const setValue = (value: string) => core.selectValue(media!, value); useLogMissingFeature(!media, name, selector.displayName ?? feature); diff --git a/packages/react/src/ui/live-button/live-button.tsx b/packages/react/src/ui/live-button/live-button.tsx index 46e1d0eaf9..d636a0f217 100644 --- a/packages/react/src/ui/live-button/live-button.tsx +++ b/packages/react/src/ui/live-button/live-button.tsx @@ -1,7 +1,7 @@ import { LiveButtonCore, LiveButtonDataAttrs, type LiveButtonMediaState } from '@videojs/core'; import { selectBuffer, selectLive, selectTime } from '@videojs/core/dom'; import { translateText } from '@videojs/core/i18n'; -import { forwardRef, type ReactNode, useLayoutEffect, useState } from 'react'; +import { forwardRef, type ReactNode, useLayoutEffect } from 'react'; import { useTranslator } from '../../i18n/context'; import { usePlayer } from '../../player/context'; @@ -52,9 +52,7 @@ export const LiveButton = forwardRef( const tooltipCtx = useOptionalTooltipContext(); const translator = useTranslator(); - const [core] = useState(() => new LiveButtonCore()); - - core.setProps({ label, disabled }); + const core = new LiveButtonCore({ label, disabled }); const { getButtonProps, buttonRef } = useButton({ displayName: DISPLAY_NAME, diff --git a/packages/react/src/ui/menu/menu-root.tsx b/packages/react/src/ui/menu/menu-root.tsx index ec77b06681..ba76b2d655 100644 --- a/packages/react/src/ui/menu/menu-root.tsx +++ b/packages/react/src/ui/menu/menu-root.tsx @@ -48,8 +48,6 @@ export function MenuRoot({ const isSubmenu = parentMenu !== null; const { side, align, closeOnEscape, closeOnOutsideClick } = coreProps; - const [core] = useState(() => new MenuCore(coreProps)); - const isControlled = controlledOpen !== undefined; const [uncontrolledOpen, setUncontrolledOpen] = useState(defaultOpen); const resolvedOpen = controlledOpen ?? uncontrolledOpen; @@ -108,11 +106,13 @@ export function MenuRoot({ return controlsState?.requestControlsLock(); }, [controlsState?.requestControlsLock, input.active, isSubmenu]); - const preferredState = useMemo(() => { - core.setProps({ side, align, closeOnEscape, closeOnOutsideClick }); + const projection = useMemo(() => { + const core = new MenuCore({ side, align, closeOnEscape, closeOnOutsideClick }); + core.setInput({ ...input, isSubmenu }); - return core.getState(); - }, [core, input, side, align, closeOnEscape, closeOnOutsideClick, isSubmenu]); + return { core, state: core.getState() }; + }, [input, side, align, closeOnEscape, closeOnOutsideClick, isSubmenu]); + const { core, state: preferredState } = projection; const { state, preferredSide, setPositionedSide } = usePositionedState(preferredState); const contextValue = useMemo( diff --git a/packages/react/src/ui/play-button/tests/play-button.test.tsx b/packages/react/src/ui/play-button/tests/play-button.test.tsx index f371607dbc..634707d652 100644 --- a/packages/react/src/ui/play-button/tests/play-button.test.tsx +++ b/packages/react/src/ui/play-button/tests/play-button.test.tsx @@ -1,9 +1,12 @@ import { cleanup, render, screen } from '@testing-library/react'; +import { PlayButtonCore } from '@videojs/core'; import { registerI18n, resetI18nRegistry } from '@videojs/core/i18n'; +import { isString } from '@videojs/utils/predicate'; +import type { ReactNode } from 'react'; import { afterEach, describe, expect, it, vi } from 'vite-plus/test'; import { createI18n, I18nProvider } from '../../../i18n'; -import { createPlayerWrapper } from '../../../testing/mocks'; +import { createPlayerWrapper, MockErrorBoundary } from '../../../testing/mocks'; import { PlayButton } from '../play-button'; afterEach(() => { @@ -70,4 +73,56 @@ describe('PlayButton', () => { expect(screen.getByTestId('play').getAttribute('aria-label')).toBe('Custom play'); }); + + it('projects each render onto its own core so an abandoned render cannot mutate the committed one', () => { + const cores = new Map(); + const originalSetProps = PlayButtonCore.prototype.setProps; + const setProps = vi + .spyOn(PlayButtonCore.prototype, 'setProps') + .mockImplementation(function (this: PlayButtonCore, props) { + if (isString(props.label)) cores.set(props.label, this); + + originalSetProps.call(this, props); + }); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + const { Wrapper } = createPlayerWrapper({ + paused: true, + ended: false, + started: false, + waiting: false, + play: vi.fn(), + pause: vi.fn(), + togglePaused: vi.fn(), + }); + + function Thrower({ abandon }: { abandon: boolean }): ReactNode { + if (abandon) throw new Error('abandon render'); + + return null; + } + + const { rerender } = render( + + + + + + + ); + + rerender( + + + + + + + ); + + expect(cores.get('committed')).toBeInstanceOf(PlayButtonCore); + expect(cores.get('abandoned')).toBeInstanceOf(PlayButtonCore); + expect(cores.get('abandoned')).not.toBe(cores.get('committed')); + setProps.mockRestore(); + consoleError.mockRestore(); + }); }); diff --git a/packages/react/src/ui/popover/popover-root.tsx b/packages/react/src/ui/popover/popover-root.tsx index 5f8345335d..eb619e8fb9 100644 --- a/packages/react/src/ui/popover/popover-root.tsx +++ b/packages/react/src/ui/popover/popover-root.tsx @@ -45,9 +45,6 @@ export function PopoverRoot({ const container = useOptionalContainer(); const popupGroup = useOptionalPopupGroup(); const controls = useOptionalControlsContext(); - const [core] = useState(() => new PopoverCore(coreProps)); - - core.setProps(coreProps); const isControlled = !isUndefined(controlledOpen); const initialOpenRef = useRef(!isControlled && defaultOpen); @@ -120,6 +117,7 @@ export function PopoverRoot({ useDestroy(popover); const input = useSnapshot(popover.input); + const core = new PopoverCore(coreProps); core.setInput(input); const { state, preferredSide, setPositionedSide } = usePositionedState(core.getState()); diff --git a/packages/react/src/ui/poster/poster.tsx b/packages/react/src/ui/poster/poster.tsx index 661eb4d026..afde49ba98 100644 --- a/packages/react/src/ui/poster/poster.tsx +++ b/packages/react/src/ui/poster/poster.tsx @@ -49,7 +49,7 @@ export const Poster = forwardRef(function Poster( const playback = usePlayer(selectPlayback); const metadata = usePlayer(selectMetadata); - const [core] = useState(() => new PosterCore()); + const core = new PosterCore(); // The metadata feature is optional: without it nothing resolves a URL, and // this stays a visibility wrapper around whatever `src` was passed. diff --git a/packages/react/src/ui/quality/use-quality-options.ts b/packages/react/src/ui/quality/use-quality-options.ts index 7182884cdb..c42d08a0e0 100644 --- a/packages/react/src/ui/quality/use-quality-options.ts +++ b/packages/react/src/ui/quality/use-quality-options.ts @@ -32,8 +32,6 @@ const useQualityRadioOptions = createRadioOptionsHook({ * @param props - Optional `label`, `formatRendition`, and `disabled` overrides. */ export function useQualityOptions(props?: QualityOptionsProps): QualityOptionsResult | null { - 'use no memo'; - return useQualityRadioOptions(props); } diff --git a/packages/react/src/ui/time-slider/time-slider-chapters/slider-segments.tsx b/packages/react/src/ui/time-slider/time-slider-chapters/slider-segments.tsx index c0e7f96ff9..8eb0bbbc49 100644 --- a/packages/react/src/ui/time-slider/time-slider-chapters/slider-segments.tsx +++ b/packages/react/src/ui/time-slider/time-slider-chapters/slider-segments.tsx @@ -6,12 +6,15 @@ import { } from '@videojs/core'; import { getStateDataAttrs } from '@videojs/core/dom'; import type { CSSProperties, ReactElement } from 'react'; -import { Fragment, forwardRef, useMemo, useState } from 'react'; +import { Fragment, forwardRef, useMemo } from 'react'; import type { HTMLProps, UIComponentProps } from '../../../utils/types'; import { renderElement } from '../../../utils/use-render'; import { useSliderContext, useSliderPointerValue } from '../../slider/context'; +// `SliderSegmentsCore` holds no state, so one shared instance projects every render. +const segmentsCore = new SliderSegmentsCore(); + type SegmentProps = Omit, 'ref'>; interface SliderSegmentsProps extends Omit, 'children'> { @@ -29,16 +32,14 @@ export const SliderSegments = forwardRef( const slider = useSliderContext(); const pointerValue = useSliderPointerValue(); - const [core] = useState(() => new SliderSegmentsCore()); - const geometry = useMemo( - () => core.getGeometry({ ranges, min, max, orientation: slider.state.orientation }), - [core, ranges, min, max, slider.state.orientation] + () => segmentsCore.getGeometry({ ranges, min, max, orientation: slider.state.orientation }), + [ranges, min, max, slider.state.orientation] ); const sliderAttrs = getStateDataAttrs(slider.state, slider.stateAttrMap); const segments = geometry.map((segment) => { - const state = core.getState(segment, slider.state, pointerValue); + const state = segmentsCore.getState(segment, slider.state, pointerValue); const segmentStyle = { [TimeSliderChapterCSSVars.start]: state.startPercent, [TimeSliderChapterCSSVars.end]: state.endPercent, @@ -59,7 +60,7 @@ export const SliderSegments = forwardRef( ); }); - const state = geometry.length > 0 ? core.getState(geometry[0]!, slider.state, pointerValue) : null; + const state = geometry.length > 0 ? segmentsCore.getState(geometry[0]!, slider.state, pointerValue) : null; if (!state) return null; return renderElement( diff --git a/packages/react/src/ui/time/time-value.tsx b/packages/react/src/ui/time/time-value.tsx index 20697b69ac..f89adc8f64 100644 --- a/packages/react/src/ui/time/time-value.tsx +++ b/packages/react/src/ui/time/time-value.tsx @@ -35,8 +35,6 @@ export const Value = forwardRef(function Value( const translator = useTranslator(); const locale = useLocale(); - const [core] = useState(() => new TimeCore()); - const defaultType = type ?? TimeCore.defaultProps.type; const [activeType, setActiveType] = useState(defaultType); @@ -46,7 +44,7 @@ export const Value = forwardRef(function Value( setActiveType(defaultType); }, [defaultType, toggle]); - core.setProps({ + const core = new TimeCore({ type: activeType, negativeSign, label, diff --git a/packages/react/src/ui/title/title.tsx b/packages/react/src/ui/title/title.tsx index 7dd2e3ec0b..c678986576 100644 --- a/packages/react/src/ui/title/title.tsx +++ b/packages/react/src/ui/title/title.tsx @@ -3,13 +3,16 @@ import { TitleCore, TitleDataAttrs } from '@videojs/core'; import { selectMetadata } from '@videojs/core/dom'; import type { ForwardedRef } from 'react'; -import { forwardRef, useState } from 'react'; +import { forwardRef } from 'react'; import { usePlayer } from '../../player/context'; import type { UIComponentProps } from '../../utils/types'; import { renderElement } from '../../utils/use-render'; import { useLogMissingFeature } from '../hooks/use-log-missing-feature'; +// `TitleCore` holds no state, so one shared instance projects every render. +const titleCore = new TitleCore(); + export interface TitleProps extends Omit, 'children'> {} /** @@ -34,13 +37,11 @@ export const Title = forwardRef(function Title( const metadata = usePlayer(selectMetadata); - const [core] = useState(() => new TitleCore()); - useLogMissingFeature(!metadata, 'Title', 'metadata'); if (!metadata) return null; - const state = core.getState(metadata); + const state = titleCore.getState(metadata); if (state.hidden) return null; return renderElement( diff --git a/packages/react/src/ui/tooltip/tooltip-root.tsx b/packages/react/src/ui/tooltip/tooltip-root.tsx index 07766bb903..3018b90472 100644 --- a/packages/react/src/ui/tooltip/tooltip-root.tsx +++ b/packages/react/src/ui/tooltip/tooltip-root.tsx @@ -48,9 +48,6 @@ export function TooltipRoot({ const container = useOptionalContainer(); const popupGroup = useOptionalPopupGroup(); const controls = useOptionalControlsContext(); - const [core] = useState(() => new TooltipCore(coreProps)); - - core.setProps(coreProps); const isControlled = !isUndefined(controlledOpen); const initialOpenRef = useRef(!isControlled && defaultOpen); @@ -127,6 +124,7 @@ export function TooltipRoot({ useDestroy(tooltip); const input = useSnapshot(tooltip.input); + const core = new TooltipCore(coreProps); core.setInput(input); const { state, preferredSide, setPositionedSide } = usePositionedState(core.getState()); diff --git a/packages/react/src/ui/volume-popover/volume-popover-root.tsx b/packages/react/src/ui/volume-popover/volume-popover-root.tsx index f2107bb6d1..742d9e8ff5 100644 --- a/packages/react/src/ui/volume-popover/volume-popover-root.tsx +++ b/packages/react/src/ui/volume-popover/volume-popover-root.tsx @@ -2,7 +2,7 @@ import { VolumePopoverCore } from '@videojs/core'; import { selectVolume } from '@videojs/core/dom'; import type { MediaVolumeState } from '@videojs/media'; import type { ReactNode } from 'react'; -import { useEffect, useState } from 'react'; +import { useEffect } from 'react'; import { usePlayer } from '../../player/context'; import { Popover } from '../popover'; @@ -24,9 +24,8 @@ export interface VolumePopoverRootProps extends PopoverRootProps {} /** Owns volume availability and the popover interaction lifecycle. */ export function VolumePopoverRoot({ children, ...props }: VolumePopoverRootProps): ReactNode { const volume = usePlayer(selectVolume); - const [core] = useState(() => new VolumePopoverCore(props)); + const core = new VolumePopoverCore(props); - core.setProps(props); core.setMedia(volume ?? unavailableVolume); return (