Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion packages/react/src/ui/dialog/tests/dialog-root.test.tsx
Original file line number Diff line number Diff line change
@@ -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();
Expand Down Expand Up @@ -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(
<MockErrorBoundary>
<Dialog.Root onOpenChange={committed}>
<CaptureDialog onCapture={capture} />
</Dialog.Root>
</MockErrorBoundary>
);

rerender(
<MockErrorBoundary>
<Dialog.Root onOpenChange={abandoned}>
<CaptureDialog onCapture={capture} />
</Dialog.Root>
<Throw />
</MockErrorBoundary>
);

// The retained dialog outlives the abandoned render and must still call the committed callback.
dialog!.open();

expect(committed).toHaveBeenCalledExactlyOnceWith(true);
expect(abandoned).not.toHaveBeenCalled();
});
});
8 changes: 4 additions & 4 deletions packages/react/src/ui/dialog/use-dialog-root.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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({
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/ui/error-dialog/error-dialog-root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/ui/gesture/use-doubletap-gesture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<GestureProps, 'pointer' | 'region' | 'disabled'> {
target?: RefObject<HTMLElement | null>;
Expand All @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/ui/gesture/use-tap-gesture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<GestureProps, 'pointer' | 'region' | 'disabled'> {
target?: RefObject<HTMLElement | null>;
Expand All @@ -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;
Expand Down
89 changes: 89 additions & 0 deletions packages/react/src/ui/hotkey/tests/use-hotkey.test.tsx
Original file line number Diff line number Diff line change
@@ -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 <PlayerContextProvider value={value}>{children}</PlayerContextProvider>;
}

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(
<Wrapper value={value}>
<Shortcut onActivate={first} />
</Wrapper>
);

await waitFor(() => expect(findHotkeyCoordinator(container)).toBeDefined());

rerender(
<Wrapper value={value}>
<Shortcut onActivate={second} />
</Wrapper>
);

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(
<StrictMode>
<Wrapper value={value}>
<Shortcut onActivate={first} />
</Wrapper>
</StrictMode>
);

await waitFor(() => expect(findHotkeyCoordinator(container)).toBeDefined());

rerender(
<StrictMode>
<Wrapper value={value}>
<Shortcut onActivate={second} />
</Wrapper>
</StrictMode>
);

container.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', bubbles: true }));

expect(second).toHaveBeenCalledExactlyOnceWith(expect.any(KeyboardEvent), 'k');
expect(first).not.toHaveBeenCalled();
});
});
4 changes: 2 additions & 2 deletions packages/react/src/ui/hotkey/use-hotkey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
14 changes: 7 additions & 7 deletions packages/react/src/ui/menu/menu-root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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({
Expand Down
22 changes: 11 additions & 11 deletions packages/react/src/ui/popover/popover-root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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({
Expand Down
Loading
Loading