Skip to content

Commit 7c679ee

Browse files
richiemcilroyrichiemcilroy
authored
Merge pull request #2797 from AksharP5/fix/web-comment-playback-cleanup
fix(web): resume playback when a video comment disappears
2 parents 7840d8f + 29673a6 commit 7c679ee

3 files changed

Lines changed: 258 additions & 5 deletions

File tree

‎apps/web/__tests__/unit/media-comment-playback-cleanup.test.ts‎

Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,249 @@
1+
// @vitest-environment jsdom
2+
3+
import {
4+
act,
5+
type ComponentProps,
6+
createElement,
7+
Fragment,
8+
forwardRef,
9+
type PropsWithChildren,
10+
} from "react";
11+
import { createRoot, type Root } from "react-dom/client";
12+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
13+
import { CommentMiniPlayer } from "@/app/s/[videoId]/_components/media-comment/CommentMiniPlayer";
14+
import type { MediaComment } from "@/app/s/[videoId]/_components/media-comment/media-comment-types";
15+
import {
16+
stopAll,
17+
useMainVideoInterlock,
18+
} from "@/app/s/[videoId]/_components/media-comment/media-playback-registry";
19+
import { VideoCommentCard } from "@/app/s/[videoId]/_components/media-comment/VideoCommentCard";
20+
import {
21+
createPlaybackStore,
22+
type PlaybackStore,
23+
} from "@/app/s/[videoId]/_components/playback/playback-store";
24+
25+
const mocks = vi.hoisted(() => ({
26+
store: null as PlaybackStore | null,
27+
miniPlayerReady: true,
28+
}));
29+
30+
vi.mock("@/app/s/[videoId]/_components/playback/PlaybackContext", () => ({
31+
useOptionalPlayback: () => mocks.store,
32+
}));
33+
34+
vi.mock("motion/react", () => ({
35+
motion: {
36+
button: ({
37+
whileHover: _whileHover,
38+
whileTap: _whileTap,
39+
...props
40+
}: ComponentProps<"button"> & {
41+
whileHover?: unknown;
42+
whileTap?: unknown;
43+
}) => createElement("button", { ...props, type: "button" }),
44+
},
45+
}));
46+
vi.mock("next/dynamic", () => ({
47+
default: () => (props: ComponentProps<typeof CommentMiniPlayer>) =>
48+
mocks.miniPlayerReady ? createElement(CommentMiniPlayer, props) : null,
49+
}));
50+
vi.mock("@cap/ui", () => ({
51+
Dialog: ({ children }: PropsWithChildren) => children,
52+
DialogContent: ({ children }: PropsWithChildren) => children,
53+
DialogTitle: ({ children }: PropsWithChildren) => children,
54+
}));
55+
vi.mock("@/app/s/[videoId]/_components/video/media-player", () => {
56+
const Container = ({ children }: PropsWithChildren) => children;
57+
const Empty = () => null;
58+
return {
59+
MediaPlayer: Container,
60+
MediaPlayerControls: Container,
61+
MediaPlayerVideo: forwardRef<HTMLVideoElement, ComponentProps<"video">>(
62+
(props, ref) => createElement("video", { ...props, ref }),
63+
),
64+
MediaPlayerControlsOverlay: Empty,
65+
MediaPlayerFullscreen: Empty,
66+
MediaPlayerPlay: Empty,
67+
MediaPlayerPlaybackSpeed: Empty,
68+
MediaPlayerSeek: Empty,
69+
MediaPlayerTime: Empty,
70+
MediaPlayerVolume: Empty,
71+
};
72+
});
73+
74+
const comment: MediaComment = {
75+
id: "fixture-comment" as MediaComment["id"],
76+
videoId: "fixture-video" as MediaComment["videoId"],
77+
type: "video",
78+
content: "",
79+
mediaKey: "fixture.mp4",
80+
mediaDuration: 10,
81+
mediaMeta: null,
82+
};
83+
84+
function Interlock() {
85+
useMainVideoInterlock();
86+
return null;
87+
}
88+
89+
let root: Root;
90+
let container: HTMLDivElement;
91+
let main: HTMLVideoElement;
92+
let mainPlaying: boolean;
93+
94+
beforeEach(() => {
95+
mocks.miniPlayerReady = true;
96+
vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true);
97+
vi.spyOn(HTMLMediaElement.prototype, "pause").mockImplementation(() => {});
98+
vi.spyOn(window, "requestAnimationFrame").mockReturnValue(1);
99+
vi.spyOn(window, "cancelAnimationFrame").mockImplementation(() => {});
100+
container = document.createElement("div");
101+
main = document.createElement("video");
102+
mainPlaying = false;
103+
Object.defineProperty(main, "paused", { get: () => !mainPlaying });
104+
vi.spyOn(main, "pause").mockImplementation(() => {
105+
mainPlaying = false;
106+
main.dispatchEvent(new Event("pause"));
107+
});
108+
vi.spyOn(main, "play").mockImplementation(async () => {
109+
mainPlaying = true;
110+
main.dispatchEvent(new Event("play"));
111+
});
112+
document.body.append(container, main);
113+
root = createRoot(container);
114+
});
115+
116+
afterEach(async () => {
117+
await act(async () => root.unmount());
118+
stopAll();
119+
mocks.store?.detach();
120+
mocks.store = null;
121+
container.remove();
122+
main.remove();
123+
vi.unstubAllGlobals();
124+
});
125+
126+
describe("video comment playback cleanup with fixture media state", () => {
127+
async function mount(kind: "inline" | "mini", initiallyPlaying: boolean) {
128+
mainPlaying = initiallyPlaying;
129+
mocks.store = createPlaybackStore();
130+
mocks.store.attach(main);
131+
const render = (visible: boolean, interlockMounted = true) =>
132+
act(async () => {
133+
root.render(
134+
createElement(
135+
Fragment,
136+
null,
137+
interlockMounted ? createElement(Interlock) : null,
138+
visible
139+
? kind === "inline"
140+
? createElement(VideoCommentCard, { comment })
141+
: createElement(CommentMiniPlayer, {
142+
comment,
143+
src: "fixture.mp4",
144+
onClose: () => undefined,
145+
})
146+
: null,
147+
),
148+
);
149+
});
150+
await render(false);
151+
await render(true);
152+
if (kind === "inline") {
153+
await act(async () => container.querySelector("button")?.click());
154+
}
155+
expect(container.querySelector("video")).not.toBeNull();
156+
expect(mainPlaying).toBe(false);
157+
return render;
158+
}
159+
160+
it.each([
161+
["inline", true],
162+
["inline", false],
163+
["mini", true],
164+
["mini", false],
165+
] as const)(
166+
"removing %s restores the main video's prior playing state of %s",
167+
async (kind, initiallyPlaying) => {
168+
const render = await mount(kind, initiallyPlaying);
169+
await render(false);
170+
expect(mainPlaying).toBe(initiallyPlaying);
171+
expect(main.play).toHaveBeenCalledTimes(initiallyPlaying ? 1 : 0);
172+
},
173+
);
174+
175+
it("keeps the main video paused through inline-to-dialog handoff", async () => {
176+
const render = await mount("inline", true);
177+
await act(async () => {
178+
container
179+
.querySelector<HTMLButtonElement>('[aria-label="Open in mini player"]')
180+
?.click();
181+
});
182+
expect(container.textContent).toContain("Video comment");
183+
expect(main.play).not.toHaveBeenCalled();
184+
await render(false);
185+
expect(main.play).toHaveBeenCalledTimes(1);
186+
expect(mainPlaying).toBe(true);
187+
});
188+
189+
it("returns playback when removed before the mini player loads", async () => {
190+
const render = await mount("inline", true);
191+
mocks.miniPlayerReady = false;
192+
await act(async () => {
193+
container
194+
.querySelector<HTMLButtonElement>('[aria-label="Open in mini player"]')
195+
?.click();
196+
});
197+
expect(container.querySelector("video")).toBeNull();
198+
expect(main.play).not.toHaveBeenCalled();
199+
await render(false);
200+
expect(main.play).toHaveBeenCalledTimes(1);
201+
expect(mainPlaying).toBe(true);
202+
});
203+
204+
it.each(["close", "ended"] as const)(
205+
"can play another inline comment after %s ends the interaction",
206+
async (ending) => {
207+
const render = await mount("inline", true);
208+
await act(async () => {
209+
if (ending === "close") {
210+
container
211+
.querySelector<HTMLButtonElement>(
212+
'[aria-label="Close video comment"]',
213+
)
214+
?.click();
215+
} else {
216+
container.querySelector("video")?.dispatchEvent(new Event("ended"));
217+
}
218+
});
219+
expect(mainPlaying).toBe(true);
220+
expect(main.play).toHaveBeenCalledTimes(1);
221+
await act(async () => container.querySelector("button")?.click());
222+
expect(mainPlaying).toBe(false);
223+
await render(false);
224+
expect(main.play).toHaveBeenCalledTimes(2);
225+
expect(mainPlaying).toBe(true);
226+
},
227+
);
228+
229+
it.each(["inline", "mini"] as const)(
230+
"returns playback before the last interlock unmounts with %s",
231+
async (kind) => {
232+
const render = await mount(kind, true);
233+
await render(false, false);
234+
expect(main.play).toHaveBeenCalledTimes(1);
235+
expect(mainPlaying).toBe(true);
236+
},
237+
);
238+
239+
it.each(["inline", "mini"] as const)(
240+
"does not resume twice after the main video takes over from %s",
241+
async (kind) => {
242+
const render = await mount(kind, true);
243+
await act(async () => main.play());
244+
await render(false);
245+
expect(main.play).toHaveBeenCalledTimes(1);
246+
expect(mainPlaying).toBe(true);
247+
},
248+
);
249+
});

‎apps/web/app/s/[videoId]/_components/media-comment/CommentMiniPlayer.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
"use client";
22

33
import { Dialog, DialogContent, DialogTitle } from "@cap/ui";
4-
import { useCallback, useEffect, useRef } from "react";
4+
import { useCallback, useLayoutEffect, useRef } from "react";
55
import {
66
MediaPlayer,
77
MediaPlayerControls,
@@ -70,7 +70,7 @@ export function CommentMiniPlayer({
7070

7171
// The host card can unmount the whole dialog (list re-order, tab switch);
7272
// the interaction is over either way, so settle up like a close would.
73-
useEffect(
73+
useLayoutEffect(
7474
() => () => {
7575
const el = videoRef.current;
7676
if (!el) return;

‎apps/web/app/s/[videoId]/_components/media-comment/VideoCommentCard.tsx‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import { motion } from "motion/react";
44
import dynamic from "next/dynamic";
5-
import { useCallback, useEffect, useRef, useState } from "react";
5+
import { useCallback, useLayoutEffect, useRef, useState } from "react";
66
import { commentMediaUrl } from "@/lib/comment-media";
77
import { formatMediaDuration } from "./format-media-duration";
88
import type { MediaComment } from "./media-comment-types";
@@ -98,6 +98,7 @@ export const VideoCommentCard = ({
9898
// losing the slot doesn't linger as a paused frame).
9999
const collapse = useCallback(() => {
100100
videoRef.current?.pause();
101+
videoRef.current = null;
101102
stopPainting();
102103
setPlaying(false);
103104
}, [stopPainting]);
@@ -113,16 +114,19 @@ export const VideoCommentCard = ({
113114
// The mini player's own claim takes the slot over when its video mounts,
114115
// and its close is what hands the main video back.
115116
const expand = useCallback(() => {
117+
const el = videoRef.current;
116118
collapse();
119+
videoRef.current = el;
117120
setExpanded(true);
118121
}, [collapse]);
119122

120123
// The <video> mounts only after the play click, so claiming happens via ref
121124
// callback rather than an effect racing autoPlay.
122125
const attachVideo = useCallback(
123126
(el: HTMLVideoElement | null) => {
124-
videoRef.current = el;
127+
// Keep the claim reachable while the lazy mini player loads.
125128
if (!el) return;
129+
videoRef.current = el;
126130
claimPlayback(el, collapse);
127131
suspendMainVideo();
128132
el.addEventListener("ended", () => {
@@ -134,7 +138,7 @@ export const VideoCommentCard = ({
134138
[collapse, paintLoop],
135139
);
136140

137-
useEffect(
141+
useLayoutEffect(
138142
() => () => {
139143
stopPainting();
140144
const el = videoRef.current;

0 commit comments

Comments
 (0)