diff --git a/packages/react/src/ui/dialog/tests/dialog-root.test.tsx b/packages/react/src/ui/dialog/tests/dialog-root.test.tsx index 6e04a6585c..35119511f7 100644 --- a/packages/react/src/ui/dialog/tests/dialog-root.test.tsx +++ b/packages/react/src/ui/dialog/tests/dialog-root.test.tsx @@ -1,15 +1,25 @@ import { cleanup, render, waitFor } from '@testing-library/react'; -import { StrictMode } from 'react'; +import type { DialogApi } from '@videojs/core/dom'; +import { StrictMode, useLayoutEffect } from 'react'; import { renderToString } from 'react-dom/server'; import { afterEach, describe, expect, it, vi } from 'vite-plus/test'; import { Dialog } from '..'; import { MockErrorBoundary } from '../../../testing/mocks'; +import { useDialogContext } from '../context'; function Throw(): null { throw new Error('abandon render'); } +function CaptureDialog({ onCapture }: { onCapture: (dialog: DialogApi) => void }): null { + const { dialog } = useDialogContext(); + + useLayoutEffect(() => onCapture(dialog), [onCapture, dialog]); + + return null; +} + afterEach(() => { cleanup(); vi.restoreAllMocks(); @@ -66,4 +76,38 @@ describe('DialogRoot', () => { await waitFor(() => expect(getByRole('dialog')).toBeDefined()); expect(onOpenChange).toHaveBeenCalledExactlyOnceWith(true); }); + + it('keeps the committed onOpenChange when a re-render is abandoned', () => { + const committed = vi.fn(); + const abandoned = vi.fn(); + let dialog: DialogApi | null = null; + const capture = (instance: DialogApi) => { + dialog = instance; + }; + + vi.spyOn(console, 'error').mockImplementation(() => {}); + + const { rerender } = render( + + + + + + ); + + rerender( + + + + + + + ); + + // The retained dialog outlives the abandoned render and must still call the committed callback. + dialog!.open(); + + expect(committed).toHaveBeenCalledExactlyOnceWith(true); + expect(abandoned).not.toHaveBeenCalled(); + }); }); diff --git a/packages/react/src/ui/dialog/use-dialog-root.ts b/packages/react/src/ui/dialog/use-dialog-root.ts index e3aa5821fd..3bb87c3cbf 100644 --- a/packages/react/src/ui/dialog/use-dialog-root.ts +++ b/packages/react/src/ui/dialog/use-dialog-root.ts @@ -3,9 +3,9 @@ import { createDialog, createTransition } from '@videojs/core/dom'; import { useSnapshot } from '@videojs/store/react'; import { useLayoutEffect, useRef, useState } from 'react'; +import { useCommittedRef } from '../../utils/use-committed-ref'; import { useDestroy } from '../../utils/use-destroy'; import { useIsomorphicLayoutEffect } from '../../utils/use-isomorphic-layout-effect'; -import { useLatestRef } from '../../utils/use-latest-ref'; import { useSafeId } from '../../utils/use-safe-id'; import type { DialogContextValue } from './context'; @@ -31,9 +31,9 @@ export function useDialogRoot({ }: UseDialogRootOptions): DialogContextValue { const isControlled = controlledOpen !== undefined; const initialOpenRef = useRef(!isControlled && defaultOpen); - const onOpenChangeRef = useLatestRef(onOpenChangeProp); - const onOpenChangeCompleteRef = useLatestRef(onOpenChangeCompleteProp); - const closeOnEscapeRef = useLatestRef(closeOnEscape); + const onOpenChangeRef = useCommittedRef(onOpenChangeProp); + const onOpenChangeCompleteRef = useCommittedRef(onOpenChangeCompleteProp); + const closeOnEscapeRef = useCommittedRef(closeOnEscape); const [dialog] = useState(() => createDialog({ diff --git a/packages/react/src/ui/error-dialog/error-dialog-root.tsx b/packages/react/src/ui/error-dialog/error-dialog-root.tsx index c7a24e60b1..50a92e8e3a 100644 --- a/packages/react/src/ui/error-dialog/error-dialog-root.tsx +++ b/packages/react/src/ui/error-dialog/error-dialog-root.tsx @@ -4,7 +4,7 @@ import type { ReactNode } from 'react'; import { useRef } from 'react'; import { useContainer, usePlayer } from '../../player/context'; -import { useLatestRef } from '../../utils/use-latest-ref'; +import { useCommittedRef } from '../../utils/use-committed-ref'; import { DialogContextProvider } from '../dialog/context'; import { useDialogRoot } from '../dialog/use-dialog-root'; import { ErrorDialogContextProvider } from './context'; @@ -21,7 +21,7 @@ export function ErrorDialogRoot({ children }: ErrorDialogRootProps): ReactNode { if (errorState?.error) lastError.current = errorState.error; - const errorStateRef = useLatestRef(errorState); + const errorStateRef = useCommittedRef(errorState); const dialogContext = useDialogRoot({ open: Boolean(errorState?.error), onOpenChange(nextOpen) { diff --git a/packages/react/src/ui/gesture/use-doubletap-gesture.ts b/packages/react/src/ui/gesture/use-doubletap-gesture.ts index 786a2cd366..99b47fd547 100644 --- a/packages/react/src/ui/gesture/use-doubletap-gesture.ts +++ b/packages/react/src/ui/gesture/use-doubletap-gesture.ts @@ -4,7 +4,7 @@ import type { RefObject } from 'react'; import { useEffect } from 'react'; import { useContainer } from '../../player/context'; -import { useLatestRef } from '../../utils/use-latest-ref'; +import { useCommittedRef } from '../../utils/use-committed-ref'; export interface UseDoubleTapGestureOptions extends Pick { target?: RefObject; @@ -23,7 +23,7 @@ export function useDoubleTapGesture( const { pointer, region, disabled = false, target } = options ?? {}; const contextContainer = useContainer(); const container = target?.current ?? contextContainer; - const onActivateRef = useLatestRef(onActivate); + const onActivateRef = useCommittedRef(onActivate); useEffect(() => { if (!container || disabled) return; diff --git a/packages/react/src/ui/gesture/use-tap-gesture.ts b/packages/react/src/ui/gesture/use-tap-gesture.ts index fbfb79d5a6..789588d73e 100644 --- a/packages/react/src/ui/gesture/use-tap-gesture.ts +++ b/packages/react/src/ui/gesture/use-tap-gesture.ts @@ -4,7 +4,7 @@ import type { RefObject } from 'react'; import { useEffect } from 'react'; import { useContainer } from '../../player/context'; -import { useLatestRef } from '../../utils/use-latest-ref'; +import { useCommittedRef } from '../../utils/use-committed-ref'; export interface UseTapGestureOptions extends Pick { target?: RefObject; @@ -20,7 +20,7 @@ export function useTapGesture(onActivate: (event: PointerEvent) => void, options const { pointer, region, disabled = false, target } = options ?? {}; const contextContainer = useContainer(); const container = target?.current ?? contextContainer; - const onActivateRef = useLatestRef(onActivate); + const onActivateRef = useCommittedRef(onActivate); useEffect(() => { if (!container || disabled) return; diff --git a/packages/react/src/ui/hotkey/tests/use-hotkey.test.tsx b/packages/react/src/ui/hotkey/tests/use-hotkey.test.tsx new file mode 100644 index 0000000000..b1d7af57eb --- /dev/null +++ b/packages/react/src/ui/hotkey/tests/use-hotkey.test.tsx @@ -0,0 +1,89 @@ +import { cleanup, render, waitFor } from '@testing-library/react'; +import { findHotkeyCoordinator } from '@videojs/core/dom'; +import { type ReactNode, StrictMode } from 'react'; +import { afterEach, describe, expect, it, vi } from 'vite-plus/test'; + +import { PlayerContextProvider, type PlayerContextValue } from '../../../player/context'; +import { createMockStore } from '../../../testing/mocks'; +import { useHotkey } from '../use-hotkey'; + +function createContextValue(container: HTMLElement): PlayerContextValue { + return { + store: createMockStore() as any, + media: null, + setMedia: vi.fn(), + container, + setContainer: vi.fn(), + }; +} + +function Wrapper({ children, value }: { children: ReactNode; value: PlayerContextValue }) { + return {children}; +} + +function Shortcut({ onActivate }: { onActivate: (event: KeyboardEvent, key: string) => void }): null { + useHotkey({ keys: 'k', onActivate }); + return null; +} + +afterEach(() => { + cleanup(); +}); + +describe('useHotkey', () => { + it('calls the latest committed onActivate without re-registering the hotkey', async () => { + const container = document.createElement('div'); + const value = createContextValue(container); + const first = vi.fn(); + const second = vi.fn(); + + const { rerender } = render( + + + + ); + + await waitFor(() => expect(findHotkeyCoordinator(container)).toBeDefined()); + + rerender( + + + + ); + + container.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', bubbles: true })); + + expect(second).toHaveBeenCalledExactlyOnceWith(expect.any(KeyboardEvent), 'k'); + expect(first).not.toHaveBeenCalled(); + }); + + it('calls the latest committed onActivate once under StrictMode', async () => { + const container = document.createElement('div'); + const value = createContextValue(container); + const first = vi.fn(); + const second = vi.fn(); + + const { rerender } = render( + + + + + + ); + + await waitFor(() => expect(findHotkeyCoordinator(container)).toBeDefined()); + + rerender( + + + + + + ); + + container.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', bubbles: true })); + + expect(second).toHaveBeenCalledExactlyOnceWith(expect.any(KeyboardEvent), 'k'); + expect(first).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/react/src/ui/hotkey/use-hotkey.ts b/packages/react/src/ui/hotkey/use-hotkey.ts index a2bf7834bc..127499817a 100644 --- a/packages/react/src/ui/hotkey/use-hotkey.ts +++ b/packages/react/src/ui/hotkey/use-hotkey.ts @@ -2,7 +2,7 @@ import { createHotkey } from '@videojs/core/dom'; import { useEffect } from 'react'; import { useContainer } from '../../player/context'; -import { useLatestRef } from '../../utils/use-latest-ref'; +import { useCommittedRef } from '../../utils/use-committed-ref'; export interface UseHotkeyOptions { keys: string; @@ -20,7 +20,7 @@ export interface UseHotkeyOptions { export function useHotkey(options: UseHotkeyOptions): void { const { keys, target = 'player', repeatable = true, disabled = false } = options; const container = useContainer(); - const onActivateRef = useLatestRef(options.onActivate); + const onActivateRef = useCommittedRef(options.onActivate); useEffect(() => { if (!container || !keys || disabled) return; diff --git a/packages/react/src/ui/menu/menu-root.tsx b/packages/react/src/ui/menu/menu-root.tsx index ba76b2d655..501992ebc1 100644 --- a/packages/react/src/ui/menu/menu-root.tsx +++ b/packages/react/src/ui/menu/menu-root.tsx @@ -12,8 +12,8 @@ import { useEffect, useLayoutEffect, useMemo, useState } from 'react'; import { useOptionalContainer, useOptionalPlayer } from '../../player/context'; import { useOptionalPopupGroup } from '../../player/popup-group-context'; +import { useCommittedRef } from '../../utils/use-committed-ref'; import { useDestroy } from '../../utils/use-destroy'; -import { useLatestRef } from '../../utils/use-latest-ref'; import { useSafeId } from '../../utils/use-safe-id'; import { useOptionalControlsContext } from '../controls/context'; import { usePositionedState } from '../hooks/use-positioned-state'; @@ -52,14 +52,14 @@ export function MenuRoot({ const [uncontrolledOpen, setUncontrolledOpen] = useState(defaultOpen); const resolvedOpen = controlledOpen ?? uncontrolledOpen; - const onOpenChangeRef = useLatestRef(onOpenChangeProp); - const onOpenChangeCompleteRef = useLatestRef(onOpenChangeCompleteProp); + const onOpenChangeRef = useCommittedRef(onOpenChangeProp); + const onOpenChangeCompleteRef = useCommittedRef(onOpenChangeCompleteProp); - const closeOnEscapeRef = useLatestRef(closeOnEscape); - const closeOnOutsideClickRef = useLatestRef(closeOnOutsideClick); + const closeOnEscapeRef = useCommittedRef(closeOnEscape); + const closeOnOutsideClickRef = useCommittedRef(closeOnOutsideClick); - const popupGroupRef = useLatestRef(popupGroup); - const isSubmenuRef = useLatestRef(isSubmenu); + const popupGroupRef = useCommittedRef(popupGroup); + const isSubmenuRef = useCommittedRef(isSubmenu); const [menu] = useState(() => { const instance = createMenu({ diff --git a/packages/react/src/ui/popover/popover-root.tsx b/packages/react/src/ui/popover/popover-root.tsx index eb619e8fb9..b807bcadbf 100644 --- a/packages/react/src/ui/popover/popover-root.tsx +++ b/packages/react/src/ui/popover/popover-root.tsx @@ -12,9 +12,9 @@ import { useEffect, useRef, useState } from 'react'; import { useOptionalContainer } from '../../player/context'; import { useOptionalPopupGroup } from '../../player/popup-group-context'; +import { useCommittedRef } from '../../utils/use-committed-ref'; import { useDestroy } from '../../utils/use-destroy'; import { useIsomorphicLayoutEffect } from '../../utils/use-isomorphic-layout-effect'; -import { useLatestRef } from '../../utils/use-latest-ref'; import { useSafeId } from '../../utils/use-safe-id'; import { useOptionalControlsContext } from '../controls/context'; import { usePositionedState } from '../hooks/use-positioned-state'; @@ -49,18 +49,18 @@ export function PopoverRoot({ const isControlled = !isUndefined(controlledOpen); const initialOpenRef = useRef(!isControlled && defaultOpen); - // Keep refs that always point to the latest values so the - // createPopover closure never reads stale props. - const onOpenChangeRef = useLatestRef(onOpenChangeProp); - const onOpenChangeCompleteRef = useLatestRef(onOpenChangeCompleteProp); + // Publish props once their render commits so the retained createPopover closure + // never adopts values from an abandoned render. + const onOpenChangeRef = useCommittedRef(onOpenChangeProp); + const onOpenChangeCompleteRef = useCommittedRef(onOpenChangeCompleteProp); - const closeOnEscapeRef = useLatestRef(coreProps.closeOnEscape); - const closeOnOutsideClickRef = useLatestRef(coreProps.closeOnOutsideClick); - const openOnHoverRef = useLatestRef(openOnHover); - const delayRef = useLatestRef(delay); - const closeDelayRef = useLatestRef(closeDelay); + const closeOnEscapeRef = useCommittedRef(coreProps.closeOnEscape); + const closeOnOutsideClickRef = useCommittedRef(coreProps.closeOnOutsideClick); + const openOnHoverRef = useCommittedRef(openOnHover); + const delayRef = useCommittedRef(delay); + const closeDelayRef = useCommittedRef(closeDelay); - const popupGroupRef = useLatestRef(popupGroup); + const popupGroupRef = useCommittedRef(popupGroup); const [popover] = useState(() => { const instance = createPopover({ diff --git a/packages/react/src/ui/popover/tests/popover-root.test.tsx b/packages/react/src/ui/popover/tests/popover-root.test.tsx index c75150f5f7..023dc40e30 100644 --- a/packages/react/src/ui/popover/tests/popover-root.test.tsx +++ b/packages/react/src/ui/popover/tests/popover-root.test.tsx @@ -1,15 +1,25 @@ -import { cleanup, render, waitFor } from '@testing-library/react'; -import { StrictMode } from 'react'; +import { cleanup, fireEvent, render, waitFor } from '@testing-library/react'; +import type { PopoverApi } from '@videojs/core/dom'; +import { StrictMode, useLayoutEffect } from 'react'; import { renderToString } from 'react-dom/server'; import { afterEach, describe, expect, it, vi } from 'vite-plus/test'; import { MockErrorBoundary } from '../../../testing/mocks'; +import { usePopoverContext } from '../context'; import * as Popover from '../index.parts'; function Throw(): null { throw new Error('abandon render'); } +function CapturePopover({ onCapture }: { onCapture: (popover: PopoverApi) => void }): null { + const { popover } = usePopoverContext(); + + useLayoutEffect(() => onCapture(popover), [onCapture, popover]); + + return null; +} + afterEach(() => { cleanup(); vi.restoreAllMocks(); @@ -66,4 +76,64 @@ describe('PopoverRoot', () => { await waitFor(() => expect(getByTestId('popup')).toBeDefined()); expect(onOpenChange).toHaveBeenCalledExactlyOnceWith(true, expect.any(Object)); }); + + it('keeps the committed onOpenChange when a re-render is abandoned', () => { + const committed = vi.fn(); + const abandoned = vi.fn(); + let popover: PopoverApi | null = null; + const capture = (instance: PopoverApi) => { + popover = instance; + }; + + vi.spyOn(console, 'error').mockImplementation(() => {}); + + const { rerender } = render( + + + + + + ); + + rerender( + + + + + + + ); + + // The retained popover outlives the abandoned render and must still call the committed callback. + popover!.open('click'); + + expect(committed).toHaveBeenCalledExactlyOnceWith(true, { reason: 'click' }); + expect(abandoned).not.toHaveBeenCalled(); + }); + + it('calls the latest committed onOpenChange under StrictMode', () => { + const first = vi.fn(); + const second = vi.fn(); + + const { rerender, getByTestId } = render( + + + Open + + + ); + + rerender( + + + Open + + + ); + + fireEvent.click(getByTestId('trigger')); + + expect(second).toHaveBeenCalledExactlyOnceWith(true, expect.objectContaining({ reason: 'click' })); + expect(first).not.toHaveBeenCalled(); + }); }); diff --git a/packages/react/src/ui/tooltip/tests/tooltip-root.test.tsx b/packages/react/src/ui/tooltip/tests/tooltip-root.test.tsx index 9c795bc541..0799b622c7 100644 --- a/packages/react/src/ui/tooltip/tests/tooltip-root.test.tsx +++ b/packages/react/src/ui/tooltip/tests/tooltip-root.test.tsx @@ -1,15 +1,25 @@ import { cleanup, render, waitFor } from '@testing-library/react'; -import { StrictMode } from 'react'; +import type { TooltipApi } from '@videojs/core/dom'; +import { StrictMode, useLayoutEffect } from 'react'; import { renderToString } from 'react-dom/server'; import { afterEach, describe, expect, it, vi } from 'vite-plus/test'; import { Tooltip } from '..'; import { MockErrorBoundary } from '../../../testing/mocks'; +import { useTooltipContext } from '../context'; function Throw(): null { throw new Error('abandon render'); } +function CaptureTooltip({ onCapture }: { onCapture: (tooltip: TooltipApi) => void }): null { + const { tooltip } = useTooltipContext(); + + useLayoutEffect(() => onCapture(tooltip), [onCapture, tooltip]); + + return null; +} + afterEach(() => { cleanup(); vi.restoreAllMocks(); @@ -66,4 +76,38 @@ describe('TooltipRoot', () => { await waitFor(() => expect(getByTestId('popup')).toBeDefined()); expect(onOpenChange).toHaveBeenCalledExactlyOnceWith(true, expect.any(Object)); }); + + it('keeps the committed onOpenChange when a re-render is abandoned', () => { + const committed = vi.fn(); + const abandoned = vi.fn(); + let tooltip: TooltipApi | null = null; + const capture = (instance: TooltipApi) => { + tooltip = instance; + }; + + vi.spyOn(console, 'error').mockImplementation(() => {}); + + const { rerender } = render( + + + + + + ); + + rerender( + + + + + + + ); + + // The retained tooltip outlives the abandoned render and must still call the committed callback. + tooltip!.open(); + + expect(committed).toHaveBeenCalledExactlyOnceWith(true, { reason: 'hover' }); + expect(abandoned).not.toHaveBeenCalled(); + }); }); diff --git a/packages/react/src/ui/tooltip/tooltip-root.tsx b/packages/react/src/ui/tooltip/tooltip-root.tsx index 3018b90472..ded1c01da6 100644 --- a/packages/react/src/ui/tooltip/tooltip-root.tsx +++ b/packages/react/src/ui/tooltip/tooltip-root.tsx @@ -12,9 +12,9 @@ import { useEffect, useRef, useState } from 'react'; import { useOptionalContainer } from '../../player/context'; import { useOptionalPopupGroup } from '../../player/popup-group-context'; +import { useCommittedRef } from '../../utils/use-committed-ref'; import { useDestroy } from '../../utils/use-destroy'; import { useIsomorphicLayoutEffect } from '../../utils/use-isomorphic-layout-effect'; -import { useLatestRef } from '../../utils/use-latest-ref'; import { useSafeId } from '../../utils/use-safe-id'; import { useOptionalControlsContext } from '../controls/context'; import { usePositionedState } from '../hooks/use-positioned-state'; @@ -54,17 +54,17 @@ export function TooltipRoot({ const groupFromContext = useTooltipGroup(); - // Keep refs that always point to the latest values so the - // createTooltip closure never reads stale props. - const onOpenChangeRef = useLatestRef(onOpenChangeProp); - const onOpenChangeCompleteRef = useLatestRef(onOpenChangeCompleteProp); - const delayRef = useLatestRef(delay); - const closeDelayRef = useLatestRef(closeDelay); - const disableHoverablePopupRef = useLatestRef(disableHoverablePopup); - const disabledRef = useLatestRef(disabled); - const stickyRef = useLatestRef(sticky); - const groupRef = useLatestRef(groupFromContext); - const popupGroupRef = useLatestRef(popupGroup); + // Publish props once their render commits so the retained createTooltip closure + // never adopts values from an abandoned render. + const onOpenChangeRef = useCommittedRef(onOpenChangeProp); + const onOpenChangeCompleteRef = useCommittedRef(onOpenChangeCompleteProp); + const delayRef = useCommittedRef(delay); + const closeDelayRef = useCommittedRef(closeDelay); + const disableHoverablePopupRef = useCommittedRef(disableHoverablePopup); + const disabledRef = useCommittedRef(disabled); + const stickyRef = useCommittedRef(sticky); + const groupRef = useCommittedRef(groupFromContext); + const popupGroupRef = useCommittedRef(popupGroup); const [tooltip] = useState(() => { const instance = createTooltip({