fix(frontend): four state defects the deep-review found in the E27 S2 sweep
Every one of them is the same shape: a value derived correctly, and a WRITE to the thing it derives from left unattended. Deriving hides a stale value; it does not consume it. PlayerModal — the watch toggle inherited the previous video's auto-watch. Moving the queue-advance sync into render was right for `currentVideoId`, but the same block also synced `status`, and that one had an ordering constraint of its own: useYtProgress's cleanup for the OUTGOING item can still call setWatched(true) near the end of a clip, and cleanups run after the commit. As an effect the sync landed last and corrected it; from render it lands first and the stray write wins. Press Next in the final ten seconds and the NEXT video showed as watched — one click on the toggle then sent onState(next, "new") for an item already new. Fixed by keying the mirror to the id it describes, so a write for another item is ignored rather than displayed, and no ordering has to be argued about at all. ChannelsProvider — a focus-channel intent was masked by the page rather than consumed, and nothing ever cleared it. The manager is a page chunk: it unmounts on every navigation, and its seed effect runs again on each mount, so returning to it re-applied a channel name from earlier in the session over whatever the user had typed since. The consumed marker now lives in the provider, keyed on the token — which also closes the case the old clear-on-leave effect never handled, opening a channel page (that does not change `page` at all). TagFilterChips + ChannelTags x2 — `open = openRaw && hidden > 0` hides the overflow popover when the lane stops overflowing, but floating-ui only reports a close for a real event on a mounted surface, so the raw flag stayed true and the menu came back by itself on the next re-fit — a window resize or the text-size slider is enough, with nobody near it. The flag is now reset in render against the same condition that hides it. Avatar + downloadRow — remembering a failed image BY URL fixed the recycled-row frame but never forgot it, so a transient 429 pinned a channel to its initial for the life of the instance, never re-requesting. The memory is now dropped once `src` moves off it, which keeps both properties. And playerModalHooks' last exhaustive-deps suppression claimed "nothing in the body reads a render value" while reading five. The invariant that is actually load-bearing is that each of them is a per-item snapshot — written down instead. All four fixes are mutation-checked: each new test was seen red, against the fixed line only, naming the real defect.
This commit is contained in:
@@ -121,8 +121,12 @@ function SuggestedChipsLane({
|
|||||||
|
|
||||||
// Open only while there is a "+N" trigger to anchor it — derived, not closed by an effect, so the
|
// Open only while there is a "+N" trigger to anchor it — derived, not closed by an effect, so the
|
||||||
// frame in which the last collapsed chip fits again cannot also hold a menu pointing at a
|
// frame in which the last collapsed chip fits again cannot also hold a menu pointing at a
|
||||||
// reference that has gone. Same shape as TagFilterChips and the second row below.
|
// reference that has gone. Same shape as TagFilterChips and the second row below, INCLUDING the
|
||||||
|
// render-phase reset: deriving hides the card but never clears the flag (a hover card whose pill
|
||||||
|
// unmounts under the cursor gets no mouseleave), so without this it springs back the next time
|
||||||
|
// the lane overflows. Convergent — the write falsifies its own condition.
|
||||||
const [overflowOpenRaw, setOverflowOpen] = useState(false);
|
const [overflowOpenRaw, setOverflowOpen] = useState(false);
|
||||||
|
if (overflowOpenRaw && hidden === 0) setOverflowOpen(false);
|
||||||
const overflowOpen = overflowOpenRaw && hidden > 0;
|
const overflowOpen = overflowOpenRaw && hidden > 0;
|
||||||
const overflowPop = useAnchoredPopover({
|
const overflowPop = useAnchoredPopover({
|
||||||
open: overflowOpen,
|
open: overflowOpen,
|
||||||
@@ -355,8 +359,10 @@ export function ChannelTagsControl({
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Derived for the same reason as the suggestion row above: if tags change so nothing is collapsed
|
// Derived for the same reason as the suggestion row above: if tags change so nothing is collapsed
|
||||||
// anymore, the "+N" trigger unmounts, and the popover must already be shut in that same render.
|
// anymore, the "+N" trigger unmounts, and the popover must already be shut in that same render —
|
||||||
|
// and the flag reset with it, or the card returns unasked on the next overflow.
|
||||||
const [overflowOpenRaw, setOverflowOpen] = useState(false);
|
const [overflowOpenRaw, setOverflowOpen] = useState(false);
|
||||||
|
if (overflowOpenRaw && hidden === 0) setOverflowOpen(false);
|
||||||
const overflowOpen = overflowOpenRaw && hidden > 0;
|
const overflowOpen = overflowOpenRaw && hidden > 0;
|
||||||
const overflowPop = useAnchoredPopover({
|
const overflowPop = useAnchoredPopover({
|
||||||
open: overflowOpen,
|
open: overflowOpen,
|
||||||
|
|||||||
@@ -86,6 +86,7 @@ export default function Channels() {
|
|||||||
setLayout,
|
setLayout,
|
||||||
setSort,
|
setSort,
|
||||||
focusChannel: onFocusChannel,
|
focusChannel: onFocusChannel,
|
||||||
|
consumeFocus,
|
||||||
} = useChannelsActions();
|
} = useChannelsActions();
|
||||||
const { openChannel: onViewChannel, setPage } = useNavigationActions();
|
const { openChannel: onViewChannel, setPage } = useNavigationActions();
|
||||||
const feedFilters = useFeedFilters();
|
const feedFilters = useFeedFilters();
|
||||||
@@ -284,12 +285,19 @@ export default function Channels() {
|
|||||||
// The TOKEN is the intent (it advances even when the same channel is re-focused); the name is the
|
// The TOKEN is the intent (it advances even when the same channel is re-focused); the name is the
|
||||||
// payload, so it rides a ref. Naming it would re-seed the box on any render that changed it,
|
// payload, so it rides a ref. Naming it would re-seed the box on any render that changed it,
|
||||||
// overwriting whatever the user had since typed.
|
// overwriting whatever the user had since typed.
|
||||||
|
//
|
||||||
|
// `consumeFocus` is what stops this page's OWN remounts from re-applying it: this effect runs on
|
||||||
|
// every mount, and this page unmounts on every navigation away — and on opening a channel page,
|
||||||
|
// which does not even change `page`. The provider then reports `focusName` as null, so a later
|
||||||
|
// visit leaves the box exactly as the user left it.
|
||||||
const focusChannelNameRef = useRef(focusChannelName);
|
const focusChannelNameRef = useRef(focusChannelName);
|
||||||
focusChannelNameRef.current = focusChannelName;
|
focusChannelNameRef.current = focusChannelName;
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const name = focusChannelNameRef.current;
|
const name = focusChannelNameRef.current;
|
||||||
if (name) setSearch(name);
|
if (!name) return;
|
||||||
}, [focusChannelToken, setSearch]);
|
setSearch(name);
|
||||||
|
consumeFocus();
|
||||||
|
}, [focusChannelToken, setSearch, consumeFocus]);
|
||||||
// The header's reset intent clears the search box (skip the initial mount); the tag chips ride in
|
// The header's reset intent clears the search box (skip the initial mount); the tag chips ride in
|
||||||
// `filters.tags`, which goToFullHistory already clears by setting a fresh map.
|
// `filters.tags`, which goToFullHistory already clears by setting a fresh map.
|
||||||
const resetRef = useRef(filtersResetToken);
|
const resetRef = useRef(filtersResetToken);
|
||||||
|
|||||||
@@ -8,8 +8,8 @@
|
|||||||
// useAccountPersistedState / useSortState / useFilterState all resolve their account-scoped keys; only
|
// useAccountPersistedState / useSortState / useFilterState all resolve their account-scoped keys; only
|
||||||
// navigationContext (the sole runtime dependency) is mocked, pinned to the channels page.
|
// navigationContext (the sole runtime dependency) is mocked, pinned to the channels page.
|
||||||
import { describe, it, expect, beforeEach, vi } from "vitest";
|
import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||||
import { renderHook, act } from "@testing-library/react";
|
import { renderHook, act, render as renderRoot } from "@testing-library/react";
|
||||||
import type { ReactNode } from "react";
|
import { useEffect, useRef, type ReactNode } from "react";
|
||||||
|
|
||||||
vi.mock("./navigationContext", () => ({
|
vi.mock("./navigationContext", () => ({
|
||||||
useNavigation: () => ({ page: "channels" }),
|
useNavigation: () => ({ page: "channels" }),
|
||||||
@@ -127,3 +127,85 @@ describe("ChannelsProvider — S8 shareable URL", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// --- the focus-a-channel intent is consumed, not masked -------------------------------------------
|
||||||
|
// `focusChannel` sends the user to the manager with its name filter seeded. The manager is a page
|
||||||
|
// chunk: it unmounts on every navigation away, and on opening a channel page (which does not even
|
||||||
|
// change `page`), and its seeding effect runs again on each mount. So the "already applied" marker
|
||||||
|
// has to live in the PROVIDER — inside the page it would reset with the page and re-apply an intent
|
||||||
|
// from earlier in the session over whatever the user has typed into the search box since.
|
||||||
|
describe("ChannelsProvider — the focus-a-channel intent", () => {
|
||||||
|
/** Stands in for Channels.tsx:287-297 — the same effect, the same consume call. */
|
||||||
|
function Manager() {
|
||||||
|
const { focusName, focusToken } = useChannels();
|
||||||
|
const { setSearch, consumeFocus } = useChannelsActions();
|
||||||
|
const nameRef = useRef(focusName);
|
||||||
|
nameRef.current = focusName;
|
||||||
|
useEffect(() => {
|
||||||
|
const name = nameRef.current;
|
||||||
|
if (!name) return;
|
||||||
|
setSearch(name);
|
||||||
|
consumeFocus();
|
||||||
|
}, [focusToken, setSearch, consumeFocus]);
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
function Harness({ mounted }: { mounted: boolean }) {
|
||||||
|
return <>{mounted && <Manager />}</>;
|
||||||
|
}
|
||||||
|
|
||||||
|
it("seeds the search box once, and a later visit does not re-seed it", () => {
|
||||||
|
let api!: ReturnType<typeof useChannels> & ReturnType<typeof useChannelsActions>;
|
||||||
|
function Probe() {
|
||||||
|
api = { ...useChannels(), ...useChannelsActions() };
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
const { rerender } = renderRoot(
|
||||||
|
<ChannelsProvider>
|
||||||
|
<Probe />
|
||||||
|
<Harness mounted />
|
||||||
|
</ChannelsProvider>,
|
||||||
|
);
|
||||||
|
|
||||||
|
act(() => api.focusChannel("Kurzgesagt"));
|
||||||
|
expect(api.search).toBe("Kurzgesagt");
|
||||||
|
|
||||||
|
// The user looks for something else instead.
|
||||||
|
act(() => api.setSearch("veritasium"));
|
||||||
|
|
||||||
|
// Leave the manager (the page chunk unmounts) and come back.
|
||||||
|
rerender(
|
||||||
|
<ChannelsProvider>
|
||||||
|
<Probe />
|
||||||
|
<Harness mounted={false} />
|
||||||
|
</ChannelsProvider>,
|
||||||
|
);
|
||||||
|
rerender(
|
||||||
|
<ChannelsProvider>
|
||||||
|
<Probe />
|
||||||
|
<Harness mounted />
|
||||||
|
</ChannelsProvider>,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(api.search).toBe("veritasium");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("re-seeds when the channel is focused again — the token is the intent", () => {
|
||||||
|
let api!: ReturnType<typeof useChannels> & ReturnType<typeof useChannelsActions>;
|
||||||
|
function Probe() {
|
||||||
|
api = { ...useChannels(), ...useChannelsActions() };
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
renderRoot(
|
||||||
|
<ChannelsProvider>
|
||||||
|
<Probe />
|
||||||
|
<Harness mounted />
|
||||||
|
</ChannelsProvider>,
|
||||||
|
);
|
||||||
|
|
||||||
|
act(() => api.focusChannel("Kurzgesagt"));
|
||||||
|
act(() => api.setSearch("veritasium"));
|
||||||
|
act(() => api.focusChannel("Kurzgesagt")); // the same channel, a new intent
|
||||||
|
expect(api.search).toBe("Kurzgesagt");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -183,7 +183,16 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
|
|||||||
// rather than cleared by an effect when you leave. focusChannel sets the name and navigates in the
|
// rather than cleared by an effect when you leave. focusChannel sets the name and navigates in the
|
||||||
// same handler, so the value is live by the time channels renders; everywhere else it reads null,
|
// same handler, so the value is live by the time channels renders; everywhere else it reads null,
|
||||||
// one render earlier than the effect used to make it.
|
// one render earlier than the effect used to make it.
|
||||||
const focusName = page === "channels" ? focusNameRaw : null;
|
//
|
||||||
|
// But an intent has to be CONSUMED, not merely masked, and the consumed marker has to live HERE.
|
||||||
|
// Channels is a page chunk that unmounts on every navigation (and on opening a channel page, which
|
||||||
|
// does not even change `page`), so its seeding effect runs again on each remount. A marker inside
|
||||||
|
// it would reset with it and re-apply an intent from earlier in the session over whatever the user
|
||||||
|
// has since typed into the search box. Keyed on the token rather than the name, because
|
||||||
|
// re-focusing the same channel is a new intent.
|
||||||
|
const [focusApplied, setFocusApplied] = useState(0);
|
||||||
|
const consumeFocus = useCallback(() => setFocusApplied(focusToken), [focusToken]);
|
||||||
|
const focusName = page === "channels" && focusApplied !== focusToken ? focusNameRaw : null;
|
||||||
|
|
||||||
// Label the entry the manager was REACHED on (setPage pushes a bare { sfPage }), so stepping back
|
// Label the entry the manager was REACHED on (setPage pushes a bare { sfPage }), so stepping back
|
||||||
// onto it later restores the tab it belonged to rather than leaving whatever tab is current.
|
// onto it later restores the tab it belonged to rather than leaving whatever tab is current.
|
||||||
@@ -216,9 +225,19 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
|
|||||||
setLayout,
|
setLayout,
|
||||||
setSort,
|
setSort,
|
||||||
focusChannel,
|
focusChannel,
|
||||||
|
consumeFocus,
|
||||||
goToFullHistory,
|
goToFullHistory,
|
||||||
}),
|
}),
|
||||||
[setSearch, setFilters, setView, setLayout, setSort, focusChannel, goToFullHistory],
|
[
|
||||||
|
setSearch,
|
||||||
|
setFilters,
|
||||||
|
setView,
|
||||||
|
setLayout,
|
||||||
|
setSort,
|
||||||
|
focusChannel,
|
||||||
|
consumeFocus,
|
||||||
|
goToFullHistory,
|
||||||
|
],
|
||||||
);
|
);
|
||||||
|
|
||||||
// S8: mirror the content state (tab + sort + column filters) into the address bar under the namespaced
|
// S8: mirror the content state (tab + sort + column filters) into the address bar under the namespaced
|
||||||
|
|||||||
@@ -351,3 +351,53 @@ describe("PlayerModal — the dialog contract", () => {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// --- the watch toggle belongs to the item it describes -------------------------------------------
|
||||||
|
// A late writer exists by construction: useYtProgress's cleanup for the OUTGOING item runs after the
|
||||||
|
// commit that already advanced `active`, and within the finish margin it auto-marks that item
|
||||||
|
// watched. While the mirror was a bare boolean that write landed last and painted the INCOMING item
|
||||||
|
// as watched. Keying the mirror by id makes the stray write not this item's, so it is ignored.
|
||||||
|
describe("PlayerModal — the watch toggle after a queue advance", () => {
|
||||||
|
const A = vid("A", 0);
|
||||||
|
const B = vid("B", 0);
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {}));
|
||||||
|
vi.spyOn(api, "saveProgress").mockImplementation(async (id: string) => ({
|
||||||
|
video_id: id,
|
||||||
|
position_seconds: 0,
|
||||||
|
}));
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not inherit the outgoing item's auto-watch when stepping away near the end", async () => {
|
||||||
|
const onState = vi.fn();
|
||||||
|
clock.current = 2995; // inside min(FINISH_MARGIN, duration/2) = 10s of 3000
|
||||||
|
renderModal(
|
||||||
|
<PlayerModal video={A} queue={[A, B]} startIndex={0} onClose={() => {}} onState={onState} />,
|
||||||
|
);
|
||||||
|
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||||
|
|
||||||
|
next();
|
||||||
|
await waitFor(() => expect(lastStart()?.videoId).toBe("B"));
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
// A really is marked watched — that part is correct and must keep working.
|
||||||
|
expect(onState.mock.calls).toContainEqual(["A", "watched"]);
|
||||||
|
// B is not, and its toggle says so. Before the fix both toggles read "watchedUnmark" here.
|
||||||
|
expect(screen.queryAllByTitle("player.watchedUnmark")).toHaveLength(0);
|
||||||
|
expect(screen.queryAllByTitle("player.markWatched").length).toBeGreaterThan(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still reflects a watch toggled on the item you are actually on", async () => {
|
||||||
|
const onState = vi.fn();
|
||||||
|
clock.current = 0;
|
||||||
|
renderModal(
|
||||||
|
<PlayerModal video={A} queue={[A, B]} startIndex={0} onClose={() => {}} onState={onState} />,
|
||||||
|
);
|
||||||
|
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||||
|
|
||||||
|
fireEvent.click(screen.getAllByTitle("player.markWatched")[0]!);
|
||||||
|
expect(onState.mock.calls).toContainEqual(["A", "watched"]);
|
||||||
|
expect(screen.queryAllByTitle("player.watchedUnmark").length).toBeGreaterThan(0);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -234,25 +234,36 @@ export default function PlayerModal({
|
|||||||
setLiveData(null);
|
setLiveData(null);
|
||||||
};
|
};
|
||||||
|
|
||||||
// Local mirror of watch status so the toggle reacts instantly; changes are
|
// Local mirror of watch status so the toggle reacts instantly; changes are propagated to the feed
|
||||||
// propagated to the feed (and server) via onState.
|
// (and server) via onState. The mirror carries the id it DESCRIBES, and a write for any other id
|
||||||
const [status, setStatus] = useState(active.status);
|
// is ignored rather than displayed — that is the enforcement, and it has to be in code because
|
||||||
const watched = status === "watched";
|
// one writer arrives late by construction: useYtProgress's cleanup for the OUTGOING item runs
|
||||||
|
// after the commit that already advanced `active`, and near the end of a clip it auto-marks that
|
||||||
|
// item watched (playerModalHooks.ts `maybeAutoWatch`). Keyed, that stray write is simply not this
|
||||||
|
// item's, so the toggle falls back to `active.status` and no ordering has to be argued about.
|
||||||
|
// Unkeyed it landed last and painted the NEXT video as watched; a corrective click then sent
|
||||||
|
// onState(next, "new") for an item that was already new.
|
||||||
|
const [status, setStatus] = useState<{ id: string; value: VideoStatus }>({
|
||||||
|
id: active.id,
|
||||||
|
value: active.status,
|
||||||
|
});
|
||||||
|
const watched = (status.id === active.id ? status.value : active.status) === "watched";
|
||||||
const setWatched = (on: boolean) => {
|
const setWatched = (on: boolean) => {
|
||||||
const next = on ? "watched" : "new";
|
const next = on ? "watched" : "new";
|
||||||
setStatus(next);
|
setStatus({ id: active.id, value: next });
|
||||||
onState(active.id, next);
|
onState(active.id, next);
|
||||||
};
|
};
|
||||||
|
|
||||||
// On queue advance (active changes), sync local state to the new item — DURING that render, not
|
// On queue advance (active changes), point `currentVideoId` at the new item — DURING that render,
|
||||||
// one commit later. As an effect, the frame between showed the new episode's chrome over the
|
// not one commit later. As an effect, the frame between still had it on the previous id, so the
|
||||||
// previous one's watch state and `navigated` flag (currentVideoId still pointed at the old id, so
|
// "you have navigated away" affordance flashed on every advance. React discards this pass instead
|
||||||
// the "you have navigated away" affordance flashed on every advance). React discards this pass
|
// of committing it, so there is no such frame; the guard falsifies itself immediately.
|
||||||
// instead of committing it, so there is no such frame; the guard falsifies itself immediately.
|
//
|
||||||
|
// The watch status is NOT synced here: it is keyed by id above, which is both stale-proof and
|
||||||
|
// immune to the late writer this block cannot see (see that comment).
|
||||||
const [syncedTo, setSyncedTo] = useState(active.id);
|
const [syncedTo, setSyncedTo] = useState(active.id);
|
||||||
if (syncedTo !== active.id) {
|
if (syncedTo !== active.id) {
|
||||||
setSyncedTo(active.id);
|
setSyncedTo(active.id);
|
||||||
setStatus(active.status);
|
|
||||||
setCurrentVideoId(active.id);
|
setCurrentVideoId(active.id);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,76 @@
|
|||||||
|
// @vitest-environment jsdom
|
||||||
|
//
|
||||||
|
// The chip-overflow popovers are open/closed by a DERIVED flag (`openRaw && hidden > 0`), which is
|
||||||
|
// what keeps a menu from outliving the "+N" trigger it is anchored to for a frame. The trap the
|
||||||
|
// derivation sets is that it hides the surface without closing it: floating-ui only reports a close
|
||||||
|
// for a real event on a mounted surface, so when the lane stops overflowing, the trigger and the
|
||||||
|
// menu disappear while the raw flag stays true — and the next re-fit that overflows again brings
|
||||||
|
// back a menu nobody asked for. `useChipOverflowFit` re-decides from a ResizeObserver, so that
|
||||||
|
// second re-fit needs no user action at all.
|
||||||
|
//
|
||||||
|
// The fit hook is mocked so `visible` is drivable: in jsdom every box measures 0, so the real
|
||||||
|
// measurement can only ever produce one answer.
|
||||||
|
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||||
|
import { render, screen, fireEvent, cleanup } from "@testing-library/react";
|
||||||
|
|
||||||
|
vi.mock("react-i18next", async (importOriginal) => {
|
||||||
|
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||||
|
return {
|
||||||
|
...actual,
|
||||||
|
useTranslation: () => ({
|
||||||
|
t: (k: string, o?: Record<string, unknown>) => (o?.count != null ? `${k}:${o.count}` : k),
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
const fit = { visible: 1 };
|
||||||
|
vi.mock("../lib/useChipOverflowFit", async (importOriginal) => {
|
||||||
|
const actual = await importOriginal<typeof import("../lib/useChipOverflowFit")>();
|
||||||
|
return { ...actual, useChipOverflowFit: () => fit.visible };
|
||||||
|
});
|
||||||
|
|
||||||
|
import TagFilterChips, { type FilterChip } from "./TagFilterChips";
|
||||||
|
|
||||||
|
const chip = (key: string): FilterChip => ({ key, label: key, active: false, onToggle: () => {} });
|
||||||
|
const chips = [chip("alpha"), chip("beta"), chip("gamma")];
|
||||||
|
|
||||||
|
// The surface mounts with `visibility: hidden` until floating-ui has positioned it, which never
|
||||||
|
// happens in jsdom (every box measures 0) — so it is out of the accessibility tree and only a
|
||||||
|
// `hidden: true` query can see it. Presence is the whole question here, not visibility.
|
||||||
|
const menu = () => screen.queryByRole("menu", { hidden: true });
|
||||||
|
const moreBtn = () => screen.queryByRole("button", { name: /channels.tags.moreCount/ });
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
cleanup();
|
||||||
|
fit.visible = 1;
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("TagFilterChips — the overflow menu", () => {
|
||||||
|
it("does not come back on its own once the lane stops overflowing", () => {
|
||||||
|
const { rerender } = render(<TagFilterChips chips={chips} />);
|
||||||
|
fireEvent.click(moreBtn()!);
|
||||||
|
expect(menu()).toBeTruthy();
|
||||||
|
|
||||||
|
// The lane widens (a resize, the text-size slider, the trailing Clear button unmounting) and
|
||||||
|
// nothing is collapsed any more: both trigger and menu go.
|
||||||
|
fit.visible = 3;
|
||||||
|
rerender(<TagFilterChips chips={chips} />);
|
||||||
|
expect(menu()).toBeNull();
|
||||||
|
expect(moreBtn()).toBeNull();
|
||||||
|
|
||||||
|
// …and it narrows again, with no user interaction in between. The menu must stay shut, and the
|
||||||
|
// returning trigger must not claim to be expanded.
|
||||||
|
fit.visible = 1;
|
||||||
|
rerender(<TagFilterChips chips={chips} />);
|
||||||
|
expect(menu()).toBeNull();
|
||||||
|
expect(moreBtn()).toHaveProperty("ariaExpanded", "false");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still opens and closes on the trigger", () => {
|
||||||
|
render(<TagFilterChips chips={chips} />);
|
||||||
|
fireEvent.click(moreBtn()!);
|
||||||
|
expect(menu()).toBeTruthy();
|
||||||
|
fireEvent.click(moreBtn()!);
|
||||||
|
expect(menu()).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -62,7 +62,17 @@ export default function TagFilterChips({
|
|||||||
// The popover can only be open while there is a "+N" trigger to anchor it. Derived rather than
|
// The popover can only be open while there is a "+N" trigger to anchor it. Derived rather than
|
||||||
// closed by an effect, so the frame in which the last overflowing chip fits again cannot also
|
// closed by an effect, so the frame in which the last overflowing chip fits again cannot also
|
||||||
// contain a menu hanging off a reference that is already gone.
|
// contain a menu hanging off a reference that is already gone.
|
||||||
|
//
|
||||||
|
// Deriving alone is NOT enough, and the missing half is the flag itself: floating-ui only reports
|
||||||
|
// a close for a real event on a mounted surface, so when the lane stops overflowing the trigger
|
||||||
|
// and the menu just disappear and `openRaw` stays true. The next re-fit that overflows again —
|
||||||
|
// a window resize, the text-size slider, or the trailing Clear button mounting as a tag is
|
||||||
|
// toggled; `useChipOverflowFit` re-decides from a ResizeObserver, with no user near this menu —
|
||||||
|
// would then re-open a menu nobody asked for, with its trigger already `aria-expanded`. So the
|
||||||
|
// flag is reset in RENDER against the same condition that hides it: convergent (the write
|
||||||
|
// falsifies it) and, unlike an effect, gone before the frame is painted.
|
||||||
const [openRaw, setOpen] = useState(false);
|
const [openRaw, setOpen] = useState(false);
|
||||||
|
if (openRaw && hidden === 0) setOpen(false);
|
||||||
const open = openRaw && hidden > 0;
|
const open = openRaw && hidden > 0;
|
||||||
const pop = useAnchoredPopover({
|
const pop = useAnchoredPopover({
|
||||||
open,
|
open,
|
||||||
|
|||||||
@@ -33,6 +33,10 @@ export interface ChannelsActions {
|
|||||||
// "Focus this channel in the manager": jump to Channels (subscriptions tab) and seed its name
|
// "Focus this channel in the manager": jump to Channels (subscriptions tab) and seed its name
|
||||||
// filter so the channel is isolated. Called from the feed sidebar and the notifications inbox.
|
// filter so the channel is isolated. Called from the feed sidebar and the notifications inbox.
|
||||||
focusChannel: (name: string) => void;
|
focusChannel: (name: string) => void;
|
||||||
|
// Mark the current focus intent as applied. The manager calls this the moment it seeds its name
|
||||||
|
// filter, so the intent cannot be re-applied when the page chunk remounts on a later visit and
|
||||||
|
// overwrite a search the user has typed since.
|
||||||
|
consumeFocus: () => void;
|
||||||
// Jump to the manager filtered to channels still missing full history (the header sync chip).
|
// Jump to the manager filtered to channels still missing full history (the header sync chip).
|
||||||
goToFullHistory: () => void;
|
goToFullHistory: () => void;
|
||||||
}
|
}
|
||||||
@@ -57,6 +61,7 @@ export const ChannelsActionsContext = createContext<ChannelsActions>({
|
|||||||
setLayout: () => {},
|
setLayout: () => {},
|
||||||
setSort: () => {},
|
setSort: () => {},
|
||||||
focusChannel: () => {},
|
focusChannel: () => {},
|
||||||
|
consumeFocus: () => {},
|
||||||
goToFullHistory: () => {},
|
goToFullHistory: () => {},
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -80,7 +80,11 @@ function Thumb({ job }: { job: DownloadJob }) {
|
|||||||
// Remembered BY the URL that failed, not as a value an effect re-syncs: these rows are recycled
|
// Remembered BY the URL that failed, not as a value an effect re-syncs: these rows are recycled
|
||||||
// down a long virtualized list, so a boolean-style mirror painted the previous job's fallback
|
// down a long virtualized list, so a boolean-style mirror painted the previous job's fallback
|
||||||
// over the next one's poster for a frame. See ui/Avatar, which had the same defect.
|
// over the next one's poster for a frame. See ui/Avatar, which had the same defect.
|
||||||
|
// …and forgotten once `primary` has moved off it, for the same reason ui/Avatar does: a poster
|
||||||
|
// that 404'd because its file was still being written should be re-requested when the row comes
|
||||||
|
// back, not written off for the life of the recycled slot. Convergent, so it belongs in render.
|
||||||
const [failedPrimary, setFailedPrimary] = useState<string | null>(null);
|
const [failedPrimary, setFailedPrimary] = useState<string | null>(null);
|
||||||
|
if (failedPrimary !== null && failedPrimary !== primary) setFailedPrimary(null);
|
||||||
const src = failedPrimary && failedPrimary === primary ? remote : primary;
|
const src = failedPrimary && failedPrimary === primary ? remote : primary;
|
||||||
// A generic uploaded file (media_kind='file') has no video frame to poster — it's a document/
|
// A generic uploaded file (media_kind='file') has no video frame to poster — it's a document/
|
||||||
// archive, not playable media. Show a neutral file glyph instead of an (always-empty) thumbnail.
|
// archive, not playable media. Show a neutral file glyph instead of an (always-empty) thumbnail.
|
||||||
|
|||||||
@@ -43,6 +43,9 @@ describe("Avatar · a failed image must not follow the component to the next ite
|
|||||||
expect(painted[0]).not.toContain(">Z<");
|
expect(painted[0]).not.toContain(">Z<");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// CHARACTERIZATION: the old boolean-plus-effect passes this too (the rerender is wrapped in act(),
|
||||||
|
// so its reset effect has run by the time the src is read). The frame-level guard is the test
|
||||||
|
// above, which the old shape fails; the retry guard is the test below, which it also fails.
|
||||||
it("keeps the fallback for the URL that actually failed, not for whatever is current", () => {
|
it("keeps the fallback for the URL that actually failed, not for whatever is current", () => {
|
||||||
const { rerender } = render(
|
const { rerender } = render(
|
||||||
<Avatar src="https://cdn.test/a.jpg" alt="avatar" fallback="Zed" />,
|
<Avatar src="https://cdn.test/a.jpg" alt="avatar" fallback="Zed" />,
|
||||||
@@ -56,6 +59,23 @@ describe("Avatar · a failed image must not follow the component to the next ite
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("retries a URL it comes back to — a 429 is transient, and so is the memory of it", () => {
|
||||||
|
// The instance is not remounted here on purpose: the player's channel avatar and the Messages
|
||||||
|
// thread header both swap `src` on a live instance. A memory that outlived the reason for it
|
||||||
|
// would pin a throttled channel to its initial for the rest of the session.
|
||||||
|
const { rerender } = render(
|
||||||
|
<Avatar src="https://cdn.test/a.jpg" alt="avatar" fallback="Zed" />,
|
||||||
|
);
|
||||||
|
fireEvent.error(screen.getByRole("img", { name: "avatar" }));
|
||||||
|
expect(screen.getByText("Z")).toBeTruthy();
|
||||||
|
|
||||||
|
rerender(<Avatar src="https://cdn.test/b.jpg" alt="avatar" fallback="Zed" />);
|
||||||
|
rerender(<Avatar src="https://cdn.test/a.jpg" alt="avatar" fallback="Zed" />);
|
||||||
|
expect(screen.getByRole("img", { name: "avatar" }).getAttribute("src")).toBe(
|
||||||
|
"https://cdn.test/a.jpg",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("falls back when there is no src at all", () => {
|
it("falls back when there is no src at all", () => {
|
||||||
render(<Avatar src={null} alt="avatar" fallback="Zed" />);
|
render(<Avatar src={null} alt="avatar" fallback="Zed" />);
|
||||||
expect(screen.queryByRole("img")).toBeNull();
|
expect(screen.queryByRole("img")).toBeNull();
|
||||||
|
|||||||
@@ -25,7 +25,16 @@ export default function Avatar({
|
|||||||
// one avatar's 429 used to paint the fallback initial over the NEXT item's perfectly good image
|
// one avatar's 429 used to paint the fallback initial over the NEXT item's perfectly good image
|
||||||
// for a frame — React committed the render with the new `src` and the stale `failed`, and only the
|
// for a frame — React committed the render with the new `src` and the stale `failed`, and only the
|
||||||
// post-paint effect cleared it. Derived state cannot be stale, so there is nothing to reset.
|
// post-paint effect cleared it. Derived state cannot be stale, so there is nothing to reset.
|
||||||
|
//
|
||||||
|
// Forgetting the URL once `src` has moved off it is the other half, and it is what keeps the 429
|
||||||
|
// TRANSIENT: without it the memory outlives the reason for it, and a mounted instance whose `src`
|
||||||
|
// swaps — the player's channel avatar stepping through a queue, the Messages thread header — would
|
||||||
|
// show the initial for a throttled channel forever, never re-requesting even after the CDN
|
||||||
|
// relented. Written in render because it is convergent (the write falsifies its own condition) and
|
||||||
|
// because an effect would leave one committed frame in which the memory is stale, which is the
|
||||||
|
// frame this whole component is about.
|
||||||
const [failedSrc, setFailedSrc] = useState<string | null>(null);
|
const [failedSrc, setFailedSrc] = useState<string | null>(null);
|
||||||
|
if (failedSrc !== null && failedSrc !== src) setFailedSrc(null);
|
||||||
const failed = !!src && failedSrc === src;
|
const failed = !!src && failedSrc === src;
|
||||||
|
|
||||||
if (src && !failed) {
|
if (src && !failed) {
|
||||||
|
|||||||
@@ -486,9 +486,17 @@ export function useYtProgress({
|
|||||||
};
|
};
|
||||||
// ONE PLAYER PER VIDEO is the invariant, and it is why this dep list is deliberately just the
|
// ONE PLAYER PER VIDEO is the invariant, and it is why this dep list is deliberately just the
|
||||||
// id: re-running would destroy and rebuild the YouTube iframe, losing the playing position and
|
// id: re-running would destroy and rebuild the YouTube iframe, losing the playing position and
|
||||||
// every listener attached to it. Nothing in the body reads a render value — the mutable state it
|
// every listener attached to it.
|
||||||
// needs mid-session (currentIdRef, modeRef, navRef, sessionPosRef, autoMarkedRef,
|
//
|
||||||
// errorStreakRef, playerRef) is all behind refs, which is the enforcement, not this comment.
|
// This body DOES capture render values — `resumeAt`, `setWatched`, `advanceOnEnd`,
|
||||||
|
// `reapplyStored`, `focusModal`, `qc` — and it must, because each of them is a PER-ITEM
|
||||||
|
// SNAPSHOT: they describe the video this player was built for, and re-reading them mid-session
|
||||||
|
// would be the bug, not the fix. `resumeAt` is the clearest case, since persist() writes
|
||||||
|
// sessionPosRef and so changes it while `activeId` has not moved; the player is already playing
|
||||||
|
// from it and must not be told again. What genuinely has to stay live mid-session is behind refs
|
||||||
|
// (currentIdRef, modeRef, navRef, sessionPosRef, autoMarkedRef, errorStreakRef, playerRef) — and
|
||||||
|
// THAT split is the enforcement: a value that is not a snapshot does not belong in this closure.
|
||||||
|
//
|
||||||
// useEffectEvent cannot help here the way it does above: YT.Player takes its callbacks as an
|
// useEffectEvent cannot help here the way it does above: YT.Player takes its callbacks as an
|
||||||
// object literal, and an effect event may not be passed around as a value.
|
// object literal, and an effect event may not be passed around as a value.
|
||||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||||
|
|||||||
Reference in New Issue
Block a user