fix: review round 2 — Clear watch progress clears only the resume point, big view focus by the way it arrived, dialog name, reason tooltip, backfill locks
This commit is contained in:
+31
-19
@@ -18,16 +18,16 @@ vi.mock("../../features/wizard/wizardContext", () => ({
|
||||
vi.mock("./VirtualFeed", () => ({
|
||||
default: ({
|
||||
items,
|
||||
onResetState,
|
||||
onClearProgress,
|
||||
}: {
|
||||
items: { id: string; position_seconds: number }[];
|
||||
onResetState: (id: string) => void;
|
||||
onClearProgress: (id: string) => void;
|
||||
}) => (
|
||||
<ol aria-label="results">
|
||||
{items.map((v) => (
|
||||
<li key={v.id}>
|
||||
{v.id}@{v.position_seconds}
|
||||
<button onClick={() => onResetState(v.id)}>reset {v.id}</button>
|
||||
<button onClick={() => onClearProgress(v.id)}>clear {v.id}</button>
|
||||
</li>
|
||||
))}
|
||||
</ol>
|
||||
@@ -81,16 +81,16 @@ const halfWatched = {
|
||||
watch_url: "https://www.youtube.com/watch?v=v1",
|
||||
} as unknown as Video;
|
||||
|
||||
function mount(clear: () => Promise<unknown>) {
|
||||
vi.spyOn(api, "feed").mockResolvedValue({ items: [halfWatched], next_cursor: null } as never);
|
||||
function mount(clear: () => Promise<unknown>, video = halfWatched, filters = FILTERS) {
|
||||
vi.spyOn(api, "feed").mockResolvedValue({ items: [video], next_cursor: null } as never);
|
||||
vi.spyOn(api, "feedSummary").mockResolvedValue({ count: 1, facets: {} } as never);
|
||||
vi.spyOn(api, "clearState").mockImplementation(clear as never);
|
||||
vi.spyOn(api, "clearProgress").mockImplementation(clear as never);
|
||||
const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} });
|
||||
render(
|
||||
<QueryClientProvider client={qc}>
|
||||
<Feed
|
||||
filters={FILTERS}
|
||||
filters={filters}
|
||||
setFilters={() => {}}
|
||||
view="cards"
|
||||
setView={() => {}}
|
||||
@@ -104,29 +104,41 @@ function mount(clear: () => Promise<unknown>) {
|
||||
return qc;
|
||||
}
|
||||
|
||||
describe("Feed — resetting a half-watched video", () => {
|
||||
it("clears its resume position at once, not only its status", async () => {
|
||||
describe("Feed — clearing a half-watched video's progress", () => {
|
||||
it("leaves its status alone: a hidden video stays in the Hidden view", async () => {
|
||||
mount(
|
||||
() => Promise.resolve({ ok: true }),
|
||||
{ ...halfWatched, status: "hidden" },
|
||||
{ ...FILTERS, show: "hidden" },
|
||||
);
|
||||
fireEvent.click(await screen.findByText("clear v1"));
|
||||
await screen.findByText("v1@0");
|
||||
await act(() => new Promise((r) => setTimeout(r, 20)));
|
||||
expect(screen.getByText("v1@0")).toBeTruthy();
|
||||
});
|
||||
|
||||
it("clears its resume position at once", async () => {
|
||||
// Never settles: what the card shows before the server answers is the optimistic layer alone.
|
||||
mount(() => new Promise(() => {}));
|
||||
await waitFor(() => expect(screen.getByText("v1@95")).toBeTruthy());
|
||||
fireEvent.click(screen.getByText("reset v1"));
|
||||
fireEvent.click(screen.getByText("clear v1"));
|
||||
await waitFor(() => expect(screen.getByText("v1@0")).toBeTruthy());
|
||||
});
|
||||
|
||||
it("shows the position a replay after it saved, so a reopen resumes there", async () => {
|
||||
// Reset, played to 40 s and closed: the player writes what the server stored into the cache.
|
||||
// A reset mask over the cache kept showing 0, a reopen resumed at 0, and its first checkpoint
|
||||
// Cleared, played to 40 s and closed: the player writes what the server stored into the cache.
|
||||
// A mask over the cache kept showing 0, a reopen resumed at 0, and its first checkpoint
|
||||
// wrote over the 40.
|
||||
const qc = mount(() => Promise.resolve({ ok: true }));
|
||||
fireEvent.click(await screen.findByText("reset v1"));
|
||||
fireEvent.click(await screen.findByText("clear v1"));
|
||||
await screen.findByText("v1@0");
|
||||
act(() => patchCachedProgress(qc, new Map([["v1", 40]])));
|
||||
await waitFor(() => expect(screen.getByText("v1@40")).toBeTruthy());
|
||||
});
|
||||
|
||||
it("keeps the position at 0 through a refetch that set out before the reset", async () => {
|
||||
// That refetch read the row the reset deletes: its answer put the old position back under a
|
||||
// card the reset had made new, and a reopen resumed there.
|
||||
it("keeps the position at 0 through a refetch that set out before the clear", async () => {
|
||||
// That refetch read the row before the clear: its answer put the old position back under a
|
||||
// card the clear had reset, and a reopen resumed there.
|
||||
let answer: (v: unknown) => void = () => {};
|
||||
const qc = mount(() => new Promise(() => {}));
|
||||
await screen.findByText("v1@95");
|
||||
@@ -134,16 +146,16 @@ describe("Feed — resetting a half-watched video", () => {
|
||||
act(() => void qc.invalidateQueries({ queryKey: qk.feed() }));
|
||||
await waitFor(() => expect(api.feed).toHaveBeenCalledTimes(2));
|
||||
await act(() => new Promise((r) => setTimeout(r, 20)));
|
||||
fireEvent.click(screen.getByText("reset v1"));
|
||||
fireEvent.click(screen.getByText("clear v1"));
|
||||
await screen.findByText("v1@0");
|
||||
await act(async () => answer({ items: [halfWatched], next_cursor: null }));
|
||||
await act(() => new Promise((r) => setTimeout(r, 20)));
|
||||
expect(screen.getByText("v1@0")).toBeTruthy();
|
||||
});
|
||||
|
||||
it("puts the position back when the server refuses the reset", async () => {
|
||||
it("puts the position back when the server refuses the clear", async () => {
|
||||
mount(() => Promise.reject(new Error("down")));
|
||||
fireEvent.click(await screen.findByText("reset v1"));
|
||||
fireEvent.click(await screen.findByText("clear v1"));
|
||||
await waitFor(() => expect(screen.getByText("v1@95")).toBeTruthy());
|
||||
});
|
||||
});
|
||||
@@ -37,7 +37,7 @@ import {
|
||||
import { notifyActionError } from "../../lib/actionErrors";
|
||||
import { announceVideoState, type NoticeVideo } from "../../features/video/stateNotice";
|
||||
import {
|
||||
resetVideoState,
|
||||
clearVideoProgress,
|
||||
writeVideoState,
|
||||
writeWatchLater,
|
||||
} from "../../features/video/videoWrites";
|
||||
@@ -210,9 +210,9 @@ export default function Feed({
|
||||
const writeSeq = useRef(0);
|
||||
const answered = useRef(new Set<number>());
|
||||
const answeredAtRefetch = useRef<ReadonlySet<number>>(new Set());
|
||||
// A reset's position 0 lives in the cached lists (see onResetState), where a refetch that set out
|
||||
// before the server took the reset puts the old position back: written again on its landing.
|
||||
const resets = useRef(new Map<string, number>());
|
||||
// A cleared position's 0 lives in the cached lists (see onClearProgress), where a refetch that set
|
||||
// out before the server took the clear puts the old position back: written again on its landing.
|
||||
const clears = useRef(new Map<string, number>());
|
||||
// The open player: which video and where to start (null = resume from saved position). `alone`:
|
||||
// opened from an address naming a video this list has not loaded, so it plays without the queue.
|
||||
const [activeVideo, setActiveVideo] = useState<{
|
||||
@@ -237,8 +237,8 @@ export default function Feed({
|
||||
});
|
||||
const openVideo = useCallback((video: Video, startAt: number | null = null) => {
|
||||
playerSession.current.open = true;
|
||||
// Its player writes the next position: a reset no longer speaks for it.
|
||||
resets.current.delete(video.id);
|
||||
// Its player writes the next position: a clear no longer speaks for it.
|
||||
clears.current.delete(video.id);
|
||||
setActiveVideo({ video, startAt });
|
||||
}, []);
|
||||
// Stable identity, like Playlists' `closePlayer`: PlayerModal's keydown effect
|
||||
@@ -356,8 +356,8 @@ export default function Feed({
|
||||
setSavedOverrides((o) => unconfirmed(o, known));
|
||||
for (const seq of known) answered.current.delete(seq);
|
||||
const zero = new Map<string, number>();
|
||||
for (const [id, seq] of resets.current) {
|
||||
if (known.has(seq)) resets.current.delete(id);
|
||||
for (const [id, seq] of clears.current) {
|
||||
if (known.has(seq)) clears.current.delete(id);
|
||||
else zero.set(id, 0);
|
||||
}
|
||||
patchCachedProgress(qc, zero);
|
||||
@@ -429,30 +429,26 @@ export default function Feed({
|
||||
[qc, trackSessionWrite],
|
||||
);
|
||||
|
||||
const onResetState = useCallback(
|
||||
const onClearProgress = useCallback(
|
||||
(id: string) => {
|
||||
// Reset to pristine (drop the whole VideoState row, incl. resume position) — the
|
||||
// un-watch toggle can't clear an in-progress position, this can.
|
||||
// The resume point alone: the status stays, so no status override.
|
||||
const seq = ++writeSeq.current;
|
||||
setOverrides((o) => ({ ...o, [id]: { value: "new", seq } }));
|
||||
// The position goes into the cached lists, where the player's own closing patch writes the
|
||||
// next one: as a mask over them it outlived a replay, so a reopen resumed at 0 and its first
|
||||
// checkpoint wrote over the progress just made.
|
||||
const before = loadedRef.current.find((v) => v.id === id)?.position_seconds;
|
||||
patchCachedProgress(qc, new Map([[id, 0]]));
|
||||
resets.current.set(id, seq);
|
||||
resetVideoState(qc, id)
|
||||
// Stale-mark only (see setState above): the optimistic override already reset the card.
|
||||
clears.current.set(id, seq);
|
||||
clearVideoProgress(qc, id)
|
||||
// Stale-mark only (see setState above): the patch above already cleared the card.
|
||||
.then(() => {
|
||||
answered.current.add(seq);
|
||||
setOverrides((o) => withAnswer(o, id, seq));
|
||||
qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" });
|
||||
})
|
||||
.catch((e: unknown) => {
|
||||
notifyActionError(e, msg("errors.writeFailed.videoReset"));
|
||||
setOverrides((o) => withoutWrite(o, id, seq));
|
||||
if (resets.current.get(id) !== seq) return;
|
||||
resets.current.delete(id);
|
||||
notifyActionError(e, msg("errors.writeFailed.videoProgress"));
|
||||
if (clears.current.get(id) !== seq) return;
|
||||
clears.current.delete(id);
|
||||
if (before !== undefined) patchCachedProgress(qc, new Map([[id, before]]));
|
||||
});
|
||||
},
|
||||
@@ -666,7 +662,7 @@ export default function Feed({
|
||||
view={view}
|
||||
onState={onState}
|
||||
onToggleSave={onToggleSave}
|
||||
onResetState={onResetState}
|
||||
onClearProgress={onClearProgress}
|
||||
onOpen={openVideo}
|
||||
hasNextPage={false}
|
||||
isFetching={false}
|
||||
@@ -1004,7 +1000,7 @@ export default function Feed({
|
||||
view={view}
|
||||
onState={onState}
|
||||
onToggleSave={onToggleSave}
|
||||
onResetState={onResetState}
|
||||
onClearProgress={onClearProgress}
|
||||
onOpen={openVideo}
|
||||
// A channel page's view is the channel already: no row there to narrow it to one.
|
||||
channelFilter={
|
||||
|
||||
@@ -27,7 +27,7 @@ export interface VirtualFeedProps {
|
||||
view: FeedView;
|
||||
onState: (id: string, status: VideoStatus) => void;
|
||||
onToggleSave: (id: string, saved: boolean) => void;
|
||||
onResetState: (id: string) => void;
|
||||
onClearProgress: (id: string) => void;
|
||||
onOpen: (v: Video, startAt?: number | null) => void;
|
||||
/** The card menu's channel-filter row (see `useVideoActions`); omitted where it has no place. */
|
||||
channelFilter?: VideoActionHandlers["channelFilter"];
|
||||
@@ -44,7 +44,7 @@ export default function VirtualFeed({
|
||||
view,
|
||||
onState,
|
||||
onToggleSave,
|
||||
onResetState,
|
||||
onClearProgress,
|
||||
onOpen,
|
||||
channelFilter,
|
||||
hasNextPage,
|
||||
@@ -92,7 +92,7 @@ export default function VirtualFeed({
|
||||
/>
|
||||
<VideoMenu
|
||||
menu={menu}
|
||||
handlers={{ onState, onToggleSave, onResetState, onOpen, channelFilter }}
|
||||
handlers={{ onState, onToggleSave, onClearProgress, onOpen, channelFilter }}
|
||||
/>
|
||||
</>
|
||||
);
|
||||
|
||||
@@ -46,14 +46,14 @@ vi.mock("react-i18next", async (importOriginal) => {
|
||||
const discover = vi.fn();
|
||||
const discoverPick = vi.fn();
|
||||
const videoRow = vi.fn();
|
||||
const clearState = vi.fn((_id: string) => Promise.resolve());
|
||||
const clearProgress = vi.fn((_id: string) => Promise.resolve());
|
||||
vi.mock("../../lib/api", () => ({
|
||||
api: {
|
||||
discover: (...a: unknown[]) => discover(...a),
|
||||
videoRow: (...a: unknown[]) => videoRow(...a),
|
||||
discoverPick: (...a: unknown[]) => discoverPick(...a),
|
||||
setState: () => Promise.resolve(),
|
||||
clearState: (id: string) => clearState(id),
|
||||
clearProgress: (id: string) => clearProgress(id),
|
||||
watchLaterAdd: () => Promise.resolve(),
|
||||
watchLaterRemove: () => Promise.resolve(),
|
||||
downloadIndex: () => Promise.resolve({}), // the cards' download buttons read it
|
||||
@@ -816,11 +816,11 @@ describe("Home", () => {
|
||||
fireEvent.click((await screen.findAllByRole("button", { name: "card.menu.more" }))[0]!);
|
||||
const reset = screen
|
||||
.getAllByRole("menuitem", { hidden: true })
|
||||
.find((b) => b.textContent === "card.resetState");
|
||||
.find((b) => b.textContent === "card.clearProgress");
|
||||
expect(reset).toBeTruthy();
|
||||
const asked = discover.mock.calls.length;
|
||||
fireEvent.click(reset!);
|
||||
await waitFor(() => expect(clearState).toHaveBeenCalledWith("v1"));
|
||||
await waitFor(() => expect(clearProgress).toHaveBeenCalledWith("v1"));
|
||||
await waitFor(() => expect(discover.mock.calls.length).toBeGreaterThan(asked));
|
||||
});
|
||||
|
||||
|
||||
@@ -53,7 +53,7 @@ import { LoadingState, StateMessage } from "../../components/ui/QueryState";
|
||||
import VideoCard, { VideoHoverActions } from "../../features/video/VideoCard";
|
||||
import { useVideoActions } from "../../features/video/useVideoActions";
|
||||
import {
|
||||
resetVideoState,
|
||||
clearVideoProgress,
|
||||
writeVideoState,
|
||||
writeWatchLater,
|
||||
} from "../../features/video/videoWrites";
|
||||
@@ -161,14 +161,14 @@ function useHomeCardMenu({
|
||||
// A video tile's own action list, the same one the feed's menu shows; the rail keeps its
|
||||
// Play row (it resolves `next`) and its shelf rows around it.
|
||||
const qc = useQueryClient();
|
||||
// Back to fresh: what the rails answer changes with it (a reset video leaves Continue watching),
|
||||
// What the rails answer changes with it (a video with no resume point leaves Continue watching),
|
||||
// so Home asks again, as its marks do.
|
||||
const onResetState = (id: string) => {
|
||||
void resetVideoState(qc, id)
|
||||
const onClearProgress = (id: string) => {
|
||||
void clearVideoProgress(qc, id)
|
||||
.then(() => qc.invalidateQueries({ queryKey: qk.discover() }))
|
||||
.catch((e: unknown) => notifyActionError(e, msg("errors.writeFailed.videoReset")));
|
||||
.catch((e: unknown) => notifyActionError(e, msg("errors.writeFailed.videoProgress")));
|
||||
};
|
||||
const videoActions = useVideoActions({ onState, onToggleSave, onResetState }, menu.open);
|
||||
const videoActions = useVideoActions({ onState, onToggleSave, onClearProgress }, menu.open);
|
||||
// A Plex tile's, likewise (its "Media info" row goes where the tile's `onInfo` does).
|
||||
const plexActions = usePlexCardActions(
|
||||
{
|
||||
|
||||
@@ -570,14 +570,14 @@ async function rowMenu(title: string, item: string) {
|
||||
}
|
||||
|
||||
describe("Playlists — a row's watch state", () => {
|
||||
it("resets a started video's view from its menu", async () => {
|
||||
vi.spyOn(api, "clearState").mockResolvedValue(undefined as never);
|
||||
it("clears a started video's progress from its menu", async () => {
|
||||
vi.spyOn(api, "clearProgress").mockResolvedValue(undefined as never);
|
||||
mount({ 1: [vid("A", 0), vid("B", 300)] });
|
||||
await rowMenu("Video B", "card.resetState");
|
||||
await waitFor(() => expect(api.clearState).toHaveBeenCalledWith("B"));
|
||||
await rowMenu("Video B", "card.clearProgress");
|
||||
await waitFor(() => expect(api.clearProgress).toHaveBeenCalledWith("B"));
|
||||
});
|
||||
|
||||
it("offers no reset on a video never started", async () => {
|
||||
it("offers no clearing on a video never started", async () => {
|
||||
mount({ 1: [vid("A", 0)] });
|
||||
const row = (await screen.findByRole("button", { name: "card.play — Video A" })).closest("tr")!;
|
||||
fireEvent.contextMenu(row);
|
||||
@@ -587,7 +587,7 @@ describe("Playlists — a row's watch state", () => {
|
||||
expect(
|
||||
screen
|
||||
.getAllByRole("menuitem", { hidden: true })
|
||||
.some((b) => b.textContent === "card.resetState"),
|
||||
.some((b) => b.textContent === "card.clearProgress"),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -3,12 +3,12 @@ import { useLatest, useStableCallback } from "../../lib/useLatest";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { useQuery, useQueryClient } from "@tanstack/react-query";
|
||||
import { qk } from "../../lib/queryKeys";
|
||||
import { isResettable } from "../../lib/api/feed";
|
||||
import { hasResumePoint } from "../../lib/api/feed";
|
||||
import {
|
||||
addPlaylistItem,
|
||||
movePlaylistItem,
|
||||
removePlaylistItem,
|
||||
resetVideoState,
|
||||
clearVideoProgress,
|
||||
writeVideoState,
|
||||
} from "../../features/video/videoWrites";
|
||||
import {
|
||||
@@ -685,14 +685,14 @@ function PlaylistDetail() {
|
||||
return write;
|
||||
}
|
||||
|
||||
// Back to fresh (the video's state row and resume point both go), from the row's menu.
|
||||
function resetRow(id: string) {
|
||||
resetVideoState(qc, id)
|
||||
// The video's resume point goes (its status stays), from the row's menu.
|
||||
function clearRowProgress(id: string) {
|
||||
clearVideoProgress(qc, id)
|
||||
.then(() => {
|
||||
qc.invalidateQueries({ queryKey: qk.playlist() });
|
||||
qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" });
|
||||
})
|
||||
.catch((e: unknown) => notifyActionError(e, msg("errors.writeFailed.videoReset")));
|
||||
.catch((e: unknown) => notifyActionError(e, msg("errors.writeFailed.videoProgress")));
|
||||
}
|
||||
|
||||
// Stable identity: PlayerModal's keydown effect depends on it, so a fresh closure
|
||||
@@ -744,13 +744,13 @@ function PlaylistDetail() {
|
||||
onSelect: () => openPlayer(ctxRow.id),
|
||||
},
|
||||
...comfortPin.item(pinVideo(ctxRow.id), raw(ctxRow.title ?? "")),
|
||||
...(isResettable(ctxRow)
|
||||
...(hasResumePoint(ctxRow)
|
||||
? [
|
||||
{
|
||||
id: "reset",
|
||||
label: t("card.resetState"),
|
||||
id: "clearProgress",
|
||||
label: t("card.clearProgress"),
|
||||
icon: <Eraser className="w-4 h-4" />,
|
||||
onSelect: () => resetRow(ctxRow.id),
|
||||
onSelect: () => clearRowProgress(ctxRow.id),
|
||||
},
|
||||
]
|
||||
: []),
|
||||
|
||||
Reference in New Issue
Block a user