From 4d1cf66d96d413a331ba77d2b4989c2ae284488e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 6 Nov 2026 02:20:52 +0100 Subject: [PATCH] fix: tag counts, chip focus, upload naming, share links and unsized clips hold after round 2 - A tag counts the followed channels the Subscriptions tab shows: hidden ones are left out, and a subscribe, unsubscribe, hide or tag toggle refreshes the count. The Tag manager's row and its hover list count the same channels as the chip; /api/tags sends both counts under their names. - A chip's count keeps the chip's colour after a dot (a dimmed count fell under 4.5:1). A pick in the open "+N" fold closes it; a chip that leaves the list hands the focus to the one in its place. The fold is a dialog of toggle buttons, not an empty menu. - Upload: a folder name left by a press is made once the press is over, if the dialog is still open, instead of guessing a dismissal from Modal's markup. While the folder is made, its place holds the focus and a live region says so. - Share: an expired link has no Copy and a warn badge; the link made on open goes into the cached list, so a reopen makes no second one; the New link form survives the link made on open. - Watch: a repeated Enter sends one unlock while one is on its way. See all buttons are named with their rail. - An unsized clip of a rotated file gets the upright size its crop cut. --- backend/app/downloads/clip.py | 10 +++ backend/app/downloads/edit.py | 7 +- backend/app/downloads/pipeline.py | 19 ++++- backend/app/reader/queries.py | 8 ++- backend/tests/test_edit_crop_frame.py | 21 ++++++ backend/tests/test_reader_service.py | 26 +++++-- docs/DESIGN.md | 3 + .../src/components/ui/Carousel.dom.test.tsx | 13 +++- frontend/src/components/ui/Carousel.tsx | 16 ++++- frontend/src/components/ui/Modal.tsx | 1 - .../channel/TagManager.count.dom.test.tsx | 69 +++++++++++++++++++ frontend/src/features/channel/TagManager.tsx | 4 +- frontend/src/i18n/locales/en/common.json | 1 + frontend/src/i18n/locales/hu/common.json | 1 + frontend/src/lib/api/tags.ts | 3 +- .../src/lib/channelWriteFailures.dom.test.tsx | 18 +++++ frontend/src/lib/subscriptionCache.test.ts | 14 ++++ frontend/src/lib/subscriptionCache.ts | 5 +- frontend/src/lib/useChannelTagActions.ts | 6 +- .../channels/TagFilterChips.dom.test.tsx | 56 ++++++++++----- .../src/modules/channels/TagFilterChips.tsx | 61 +++++++++++----- .../modules/channels/useChannelActions.tsx | 2 + frontend/src/modules/feed/ChannelPage.tsx | 2 + .../modules/files/ShareDialog.dom.test.tsx | 53 ++++++++++++++ frontend/src/modules/files/ShareDialog.tsx | 66 +++++++++++++----- .../modules/files/UploadDialog.dom.test.tsx | 67 ++++++++++++------ frontend/src/modules/files/UploadDialog.tsx | 69 ++++++++++++------- .../modules/plex/PlexDiscover.dom.test.tsx | 13 ++-- frontend/src/watch/WatchPage.dom.test.tsx | 16 +++++ frontend/src/watch/WatchPage.tsx | 4 +- 30 files changed, 526 insertions(+), 128 deletions(-) create mode 100644 frontend/src/features/channel/TagManager.count.dom.test.tsx diff --git a/backend/app/downloads/clip.py b/backend/app/downloads/clip.py index 4b4b1021..686e8506 100644 --- a/backend/app/downloads/clip.py +++ b/backend/app/downloads/clip.py @@ -201,6 +201,16 @@ def _ratio(v: object) -> float | None: return n / d if n > 0 and d > 0 else None +def quarter_turned(probe: dict) -> bool | None: + """Whether the first video stream of an ffprobe `-show_streams` JSON is stored turned a quarter + (ffmpeg stands it upright before any filter, so its filters see width and height swapped). None + when there is no video stream.""" + for s in probe.get("streams") or []: + if s.get("codec_type") == "video": + return _quarter_turned(s) + return None + + def _quarter_turned(stream: dict) -> bool: rotation: object = (stream.get("tags") or {}).get("rotate") for sd in stream.get("side_data_list") or []: diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 0686769e..7e9a6242 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -89,9 +89,10 @@ def crop_filter(crop: dict) -> str: def crop_stored_size(crop: dict, src_w: int | None, src_h: int | None) -> tuple[int, int] | None: - """The stored size `crop_filter` cuts out of a `src_w` x `src_h` file, evened as `EVEN_FRAME` - evens it — the clip's size when ffprobe cannot tell. A crop counted in the shown picture is a - fraction of the file's frame, not its own pixel count; None when the source size is unknown.""" + """The size `crop_filter` cuts out of a `src_w` x `src_h` frame, evened as `EVEN_FRAME` evens it — + the clip's size when ffprobe cannot tell. The frame is the one ffmpeg's filters see: a rotated + file's stood upright, its stored size turned (the caller's to turn). A crop counted in the shown + picture is a fraction of that frame, not its own pixel count; None when the size is unknown.""" if crop.get("frame_w") and crop.get("frame_h"): if not src_w or not src_h: return None diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 2b78c204..236b162d 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -837,6 +837,18 @@ def _fail_edit( ) +def _filter_frame( + path: Path, width: int | None, height: int | None +) -> tuple[int | None, int | None]: + """A file's frame as ffmpeg's filters see it: the stored size, turned when the stream is stored + turned a quarter (ffmpeg stands it upright first). Unknown when the file cannot be read.""" + data = probe.probe_json(path) + turned = clipmod.quarter_turned(data) if data else None + if turned is None: + return None, None + return (height, width) if turned else (width, height) + + def _process_edit(job_id: int, asset_id: int) -> None: """Produce a per-user trim/crop clip from the source asset's downloaded file via ffmpeg.""" with SessionLocal() as db: @@ -905,6 +917,12 @@ def _process_edit(job_id: int, asset_id: int) -> None: size = final.stat().st_size if final.exists() else None # The clip's real frame: a re-encode evens an odd crop or source (`editmod.EVEN_FRAME`). _, _, out_w, out_h, _ = probe.probe_media(final) + # Unsized, a crop's own size stands in, cut from the frame the filters saw. + cut = ( + editmod.crop_stored_size(edit_spec["crop"], *_filter_frame(src_path, src_w, src_h)) + if crop and not (out_w and out_h) + else None + ) with SessionLocal() as db: asset = db.get(MediaAsset, asset_id) @@ -914,7 +932,6 @@ def _process_edit(job_id: int, asset_id: int) -> None: asset.size_bytes = size asset.container = out_ext asset.duration_s = total_s or asset.duration_s - cut = editmod.crop_stored_size(edit_spec["crop"], src_w, src_h) if crop else None if out_w and out_h: asset.width, asset.height = out_w, out_h elif cut: diff --git a/backend/app/reader/queries.py b/backend/app/reader/queries.py index 4c938414..b5e9c026 100644 --- a/backend/app/reader/queries.py +++ b/backend/app/reader/queries.py @@ -44,9 +44,11 @@ def video(db: Session, video_id: str) -> Video | None: def tags_with_counts(db: Session, user: User) -> list[tuple[Tag, int, int]]: """The tags this user sees (the shared system ones and their own), by category then name, each with how many channel links it has and how many of the user's own links sit on a channel they - subscribe to — the rows the Channels page's tag filter shows. A link outlives an unsubscribe, - so the two differ.""" - subscribed = select(Subscription.channel_id).where(owned_by(Subscription.user_id, user)) + subscribe to and have not hidden — the rows the Channels page's tag filter shows (a hidden + channel has its own tab). A link outlives an unsubscribe and a hide, so the two differ.""" + subscribed = select(Subscription.channel_id).where( + owned_by(Subscription.user_id, user), Subscription.hidden.is_(False) + ) on_subscribed = dict( db.execute( select(ChannelTag.tag_id, func.count(ChannelTag.id)) diff --git a/backend/tests/test_edit_crop_frame.py b/backend/tests/test_edit_crop_frame.py index 70d3bbcb..c0242171 100644 --- a/backend/tests/test_edit_crop_frame.py +++ b/backend/tests/test_edit_crop_frame.py @@ -178,6 +178,27 @@ def test_a_clip_ffprobe_cannot_size_gets_the_size_its_crop_cut( assert (asset.width, asset.height) == (160, 120) +def test_a_rotated_clip_ffprobe_cannot_size_gets_the_upright_size_its_crop_cut( + db, make_user, tmp_path, monkeypatch +): + """The frame was drawn on the upright picture, which is the stored one turned: so is its cut.""" + monkeypatch.setattr(settings, "files_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _source(src, "320x240", "x=0:y=0:w=160:h=240", sar=1, rotate=90) + + def probe_fails(): + monkeypatch.setattr(pipeline.probe, "probe_media", lambda _p: (None,) * 5) + + asset = _cut_asset( + db, make_user, src, + {"crop": {"x": 0, "y": 160, "w": 240, "h": 160, "frame_w": 240, "frame_h": 320}}, + then=probe_fails, + ) # fmt: skip + + # What ffmpeg cut (the rotated test above): 240 x 160, not the stored 320 x 120. + assert (asset.width, asset.height) == (240, 160) + + @pytest.mark.parametrize("key", ["frame_w", "x"]) def test_an_infinite_number_in_a_crop_is_dropped_not_a_crash(key): crop = {"x": 4, "y": 6, "w": 100, "h": 50, "frame_w": 640, "frame_h": 240} diff --git a/backend/tests/test_reader_service.py b/backend/tests/test_reader_service.py index 81d551d4..647c6825 100644 --- a/backend/tests/test_reader_service.py +++ b/backend/tests/test_reader_service.py @@ -66,22 +66,36 @@ def test_a_system_tag_is_nobodys_to_edit(db, make_user): # --- tags ------------------------------------------------------------------------------------ -def test_a_tag_counts_apart_the_links_on_channels_the_user_still_follows(db, make_user): - """An unsubscribe keeps the channel's tag links; the Channels page's filter lists only followed - channels, so its count must not hold the others.""" +def _music_on_four(db, make_user): + """A tag on four channels: two followed, one followed but hidden, one no longer followed.""" user, other = make_user(), make_user() tag = service.create_tag(db, user, "Music", "other", None) - for cid in ("UCa", "UCb", "UCgone"): + for cid in ("UCa", "UCb", "UChid", "UCgone"): db.add(Channel(id=cid, title=cid)) db.flush() for cid in ("UCa", "UCb"): db.add(Subscription(user_id=user.id, channel_id=cid)) + db.add(Subscription(user_id=user.id, channel_id="UChid", hidden=True)) db.add(Subscription(user_id=other.id, channel_id="UCgone")) - for cid in ("UCa", "UCb", "UCgone"): + for cid in ("UCa", "UCb", "UChid", "UCgone"): db.add(ChannelTag(channel_id=cid, tag_id=tag.id, user_id=user.id, source="user")) db.commit() + return user, tag + + +def test_a_tag_counts_apart_the_links_on_channels_the_user_still_follows(db, make_user): + """An unsubscribe and a hide keep the channel's tag links; the Channels page's filter lists only + the followed channels not hidden, so its count must not hold the others.""" + user, tag = _music_on_four(db, make_user) [(_, count, subscribed)] = [r for r in queries.tags_with_counts(db, user) if r[0].id == tag.id] - assert (count, subscribed) == (3, 2) + assert (count, subscribed) == (4, 2) + + +def test_the_tag_list_sends_each_count_under_its_own_name(db, make_user, as_user): + """Both counts are ints, so a swap would type-check: the answer itself pins which is which.""" + user, tag = _music_on_four(db, make_user) + [row] = [t for t in as_user(user).get("/api/tags").json() if t["id"] == tag.id] + assert (row["channel_count"], row["subscribed_count"]) == (4, 2) def test_a_tag_is_made_once_per_name_and_an_unknown_category_is_other(db, make_user): diff --git a/docs/DESIGN.md b/docs/DESIGN.md index c8ddabfd..83c5b347 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -866,3 +866,6 @@ of a full-width panel and nothing on the right. | 2026-11-04 | Tag chip counts (owner D5a) | A Channels tag chip shows how many followed channels it holds: a muted number after the name, no badge | The number in brackets after the name | The owner's pick; the number is a detail of the name, so it takes the name's own text, muted, rather than a `CountBadge` | | 2026-11-05 | Picked tag chips lead the row (owner c) | On the Channels tag row the picked chips come first, in the tags' order, then the rest | Keep the order and fold the unpicked first; mark the "+N" when it holds a pick | The row folds from its end, so a picked tag late in the list sat only inside "+N" with nothing on the row saying a filter was on | | 2026-11-05 | A toggled tag chip keeps the focus (owner a) | A chip that moves when toggled takes the keyboard focus with it: to its new place, or to "+N" when it went into the closed fold | Keep a picked chip in the "+N" popover until it closes | The row always shows the filters as they are; the focus, not the order, was what got lost | +| 2026-11-06 | Tag chip count after a dot (owner a) | The count keeps the chip's own colour, set off from the name by a "·" | The number dimmed (it fell under the 4.5:1 text floor, resting and pressed); the number in a tinted inner badge | Contrast holds in every scheme without a new token, and the dot still reads the number as a detail of the name | +| 2026-11-06 | A pick in the open fold closes it (owner a) | Toggling a chip inside the "+N" popover closes the popover; the focus goes with the chip | Keep the popover open and focus the chip now in the picked one's place | The focus never sits outside a popover left open, and a picked chip leads the row anyway | +| 2026-11-06 | An expired share link offers no Copy (owner a) | An expired row in the share dialog has a warn badge "expired " and its address struck through; Open and Revoke stay, Copy goes | The warn badge alone, Copy kept | An expired link opens nothing, so it is no link to hand out; Open still lets the owner check it | diff --git a/frontend/src/components/ui/Carousel.dom.test.tsx b/frontend/src/components/ui/Carousel.dom.test.tsx index 14a55438..aa2c7f5b 100644 --- a/frontend/src/components/ui/Carousel.dom.test.tsx +++ b/frontend/src/components/ui/Carousel.dom.test.tsx @@ -9,7 +9,13 @@ import { act, render, screen, fireEvent, cleanup } from "@testing-library/react" vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); - return { ...actual, useTranslation: () => ({ t: (k: string) => k }) }; + return { + ...actual, + useTranslation: () => ({ + t: (k: string, o?: { action?: string; title?: string }) => + o?.title ? `${o.action}: ${o.title}` : k, + }), + }; }); import { Carousel } from "./Carousel"; @@ -54,7 +60,10 @@ describe("Carousel", () => { ); expect(screen.getByText("card-a")).toBeTruthy(); expect(screen.getByRole("heading", { name: "Recently added" })).toBeTruthy(); - fireEvent.click(screen.getByRole("button", { name: "common.seeAll" })); + // Named with its rail, the visible words first: a page of rails has a See all on each. + const seeAll = screen.getByRole("button", { name: "common.seeAll: Recently added" }); + expect(seeAll.textContent).toBe("common.seeAll"); + fireEvent.click(seeAll); expect(onSeeAll).toHaveBeenCalledOnce(); }); diff --git a/frontend/src/components/ui/Carousel.tsx b/frontend/src/components/ui/Carousel.tsx index d638ccc2..a0f4e3fc 100644 --- a/frontend/src/components/ui/Carousel.tsx +++ b/frontend/src/components/ui/Carousel.tsx @@ -109,7 +109,21 @@ export function Carousel({

{title}

{onSeeAll && ( - diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index 762dc53e..86964f22 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -231,7 +231,6 @@ export default function Modal({ size="sm" variant="ghost" onClick={onClose} - data-dismiss="" > diff --git a/frontend/src/features/channel/TagManager.count.dom.test.tsx b/frontend/src/features/channel/TagManager.count.dom.test.tsx new file mode 100644 index 00000000..31fb0d91 --- /dev/null +++ b/frontend/src/features/channel/TagManager.count.dom.test.tsx @@ -0,0 +1,69 @@ +// @vitest-environment jsdom +// +// A tag's row in the manager says how many channels it is on, beside a hover list of them, over the +// Channels page whose chip names the same tag with a number: all three count the same channels — +// the followed ones the Subscriptions tab shows, hidden ones left out. Every link (the number a +// delete removes) is only the delete's confirm. +import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; +import { render, screen, cleanup, fireEvent } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ + t: (key: string, o?: { count?: number }) => (o?.count != null ? `${key}:${o.count}` : key), + i18n: { language: "en" }, + }), +})); + +import TagManager from "./TagManager"; +import { api, type ManagedChannel, type Tag } from "../../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const tag = { + id: 5, + name: "Music", + color: null, + category: "other", + system: false, + channel_count: 4, + subscribed_count: 2, +} as Tag; +const ch = (id: string, hidden: boolean) => + ({ id, title: id, hidden, tag_ids: [5] }) as unknown as ManagedChannel; + +describe("TagManager · a tag's count", () => { + it("counts the followed channels the Subscriptions tab shows, not every link", async () => { + vi.spyOn(api, "tags").mockResolvedValue([tag]); + vi.spyOn(api, "channels").mockResolvedValue([ + ch("UCa", false), + ch("UCb", false), + ch("UChid", true), + ]); + render( + + {}} onFocusChannel={() => {}} /> + , + ); + expect(await screen.findByText("tagManager.channels:2")).toBeTruthy(); + expect(screen.queryByText("tagManager.channels:4")).toBeNull(); + fireEvent.mouseEnter(screen.getByText("tagManager.channels:2")); + expect(await screen.findByText("UCa")).toBeTruthy(); + expect(screen.queryByText("UChid")).toBeNull(); + }); +}); diff --git a/frontend/src/features/channel/TagManager.tsx b/frontend/src/features/channel/TagManager.tsx index c0aad6ed..752856f8 100644 --- a/frontend/src/features/channel/TagManager.tsx +++ b/frontend/src/features/channel/TagManager.tsx @@ -71,7 +71,7 @@ function TagRow({ : "" }`} > - {t("tagManager.channels", { count: tag.channel_count })} + {t("tagManager.channels", { count: tag.subscribed_count })} @@ -119,7 +119,7 @@ export default function TagManager({ const userTags = (tagsQuery.data ?? []).filter((tg) => !tg.system); const channelsForTag = (tagId: number) => (channelsQuery.data ?? []) - .filter((c) => c.tag_ids.includes(tagId)) + .filter((c) => !c.hidden && c.tag_ids.includes(tagId)) .map((c) => ({ id: c.id, title: c.title ?? c.id })); const invalidate = () => { diff --git a/frontend/src/i18n/locales/en/common.json b/frontend/src/i18n/locales/en/common.json index c91b8720..d192bad3 100644 --- a/frontend/src/i18n/locales/en/common.json +++ b/frontend/src/i18n/locales/en/common.json @@ -28,6 +28,7 @@ "loadError": "Couldn't load this — check your connection and try again", "deletedTag": "Deleted tag", "seeAll": "See all", + "seeAllOf": "{{action}}: {{title}}", "scrollLeft": "Scroll left", "scrollRight": "Scroll right", "search": "Search", diff --git a/frontend/src/i18n/locales/hu/common.json b/frontend/src/i18n/locales/hu/common.json index cc20e9db..a4f05cfe 100644 --- a/frontend/src/i18n/locales/hu/common.json +++ b/frontend/src/i18n/locales/hu/common.json @@ -28,6 +28,7 @@ "loadError": "Nem sikerült betölteni — ellenőrizd a kapcsolatot, és próbáld újra", "deletedTag": "Törölt címke", "seeAll": "Összes", + "seeAllOf": "{{action}}: {{title}}", "scrollLeft": "Görgetés balra", "scrollRight": "Görgetés jobbra", "search": "Keresés", diff --git a/frontend/src/lib/api/tags.ts b/frontend/src/lib/api/tags.ts index b1e0fb96..568206b3 100644 --- a/frontend/src/lib/api/tags.ts +++ b/frontend/src/lib/api/tags.ts @@ -10,7 +10,8 @@ export interface Tag { /** Every channel link it has, a channel the user no longer follows included (an unsubscribe keeps * the link) — what deleting the tag removes. */ channel_count: number; - /** The user's own links on channels they follow: the rows the Channels page's filter shows. */ + /** The user's own links on channels they follow and have not hidden: the rows the Channels page's + * filter shows (a hidden channel has its own tab). */ subscribed_count: number; } diff --git a/frontend/src/lib/channelWriteFailures.dom.test.tsx b/frontend/src/lib/channelWriteFailures.dom.test.tsx index 7f705701..f3bbad75 100644 --- a/frontend/src/lib/channelWriteFailures.dom.test.tsx +++ b/frontend/src/lib/channelWriteFailures.dom.test.tsx @@ -18,6 +18,7 @@ import { api, HttpError } from "./api"; import { msg, notify } from "./notifications"; import { useChannelTagActions } from "./useChannelTagActions"; import { useChannelPriority } from "./useChannelPriority"; +import { qk } from "./queryKeys"; const notifyMock = vi.mocked(notify); beforeEach(() => notifyMock.mockClear()); @@ -44,6 +45,23 @@ describe("useChannelTagActions", () => { }); }); + // A tag counts the followed channels it is on: a toggle that lands changes that number. + it("refreshes the tags once a toggle lands", async () => { + vi.spyOn(api, "attachChannelTag").mockResolvedValue({ ok: true } as never); + const qc = new QueryClient(); + const spy = vi.spyOn(qc, "invalidateQueries"); + const { result } = renderHook( + () => useChannelTagActions({ optimisticToggle: () => {}, onError: () => {} }), + { + wrapper: ({ children }) => ( + {children} + ), + }, + ); + act(() => result.current.toggleTag("UC1", 7, false)); + await waitFor(() => expect(spy.mock.calls.map(([f]) => f?.queryKey)).toContainEqual(qk.tags())); + }); + it("names a refused create-and-attach", async () => { vi.spyOn(api, "createTag").mockRejectedValue(new HttpError(429, "slow")); const { result } = renderHook( diff --git a/frontend/src/lib/subscriptionCache.test.ts b/frontend/src/lib/subscriptionCache.test.ts index bc5efd25..a541dbbf 100644 --- a/frontend/src/lib/subscriptionCache.test.ts +++ b/frontend/src/lib/subscriptionCache.test.ts @@ -21,9 +21,23 @@ describe("a subscription change", () => { qk.feedSummary(), qk.discoveredChannels(), qk.watchedChannels(), + qk.tags(), ]); }); + // A tag counts the followed channels it is on, not hidden: a hide changes the number as an + // unsubscribe does, and both places that hide a channel must refresh it. + it("a hide refreshes the tags wherever it is made", () => { + const root = new URL("../", import.meta.url); + const sites = readdirSync(root, { recursive: true, encoding: "utf8" }) + .map((f) => f.split("\\").join("/")) + .filter((f) => f.endsWith(".tsx") && !f.includes(".test.") && !f.startsWith("design-lab/")) + .map((f) => [f, readFileSync(new URL(f, root), "utf8")] as const) + .filter(([, src]) => /api\.updateChannel\([^)]*hidden/.test(src)); + expect(sites.length).toBeGreaterThanOrEqual(2); + for (const [f, src] of sites) expect(src, f).toContain("queryKey: qk.tags()"); + }); + it("is refreshed through that one function wherever it is made", () => { const root = new URL("../", import.meta.url); const sites = readdirSync(root, { recursive: true, encoding: "utf8" }) diff --git a/frontend/src/lib/subscriptionCache.ts b/frontend/src/lib/subscriptionCache.ts index dff6d221..12b344a5 100644 --- a/frontend/src/lib/subscriptionCache.ts +++ b/frontend/src/lib/subscriptionCache.ts @@ -3,8 +3,8 @@ import type { QueryClient } from "@tanstack/react-query"; import { qk } from "./queryKeys"; /** Everything a subscribe or an unsubscribe changes: the channel itself, the manager's list and its - * counts, the feed and its facets (the channel's videos come or go), and the two discovery lists (a - * subscribed channel leaves them). ONE set for every site that makes the change: each used to keep + * counts, the feed and its facets (the channel's videos come or go), the two discovery lists (a + * subscribed channel leaves them), and the tags (a tag counts the followed channels it is on). ONE set for every site that makes the change: each used to keep * its own list, and each list missed something. */ export function invalidateSubscription(qc: QueryClient, channelId: string): void { for (const queryKey of [ @@ -15,6 +15,7 @@ export function invalidateSubscription(qc: QueryClient, channelId: string): void qk.feedSummary(), qk.discoveredChannels(), qk.watchedChannels(), + qk.tags(), ]) void qc.invalidateQueries({ queryKey }); } diff --git a/frontend/src/lib/useChannelTagActions.ts b/frontend/src/lib/useChannelTagActions.ts index a85e152c..079517a1 100644 --- a/frontend/src/lib/useChannelTagActions.ts +++ b/frontend/src/lib/useChannelTagActions.ts @@ -28,7 +28,11 @@ export function useChannelTagActions(opts: { ? api.detachChannelTag(channelId, tagId) : api.attachChannelTag(channelId, tagId); call - .then(() => opts.onSuccess?.(channelId)) + .then(() => { + // A tag counts the followed channels it is on: the toggle changed that number. + void qc.invalidateQueries({ queryKey: qk.tags() }); + opts.onSuccess?.(channelId); + }) .catch((err: unknown) => { notifyActionError(err, msg("channels.tags.updateFailed")); opts.onError(channelId); diff --git a/frontend/src/modules/channels/TagFilterChips.dom.test.tsx b/frontend/src/modules/channels/TagFilterChips.dom.test.tsx index 43538a97..cd0c08ee 100644 --- a/frontend/src/modules/channels/TagFilterChips.dom.test.tsx +++ b/frontend/src/modules/channels/TagFilterChips.dom.test.tsx @@ -38,7 +38,7 @@ 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 menu = () => screen.queryByRole("dialog", { hidden: true }); const moreBtn = () => screen.queryByRole("button", { name: /channels.tags.moreCount/ }); afterEach(() => { @@ -112,8 +112,10 @@ describe("TagFilterChips counts", () => { ); const name = (label: string) => screen.getAllByRole("button").find((b) => b.textContent?.startsWith(label)); - expect(name("alpha")?.textContent).toBe("alpha42"); - expect(name("beta")?.textContent).toBe("beta0"); + expect(name("alpha")?.textContent).toBe("alpha · 42"); + expect(name("beta")?.textContent).toBe("beta · 0"); + // In the chip's own colour: a dimmed count falls under the text contrast floor. + expect(name("alpha")?.querySelector('[class*="opacity"]')).toBeNull(); }); it("draws nothing after the name of a chip that has no count", () => { @@ -125,8 +127,8 @@ describe("TagFilterChips counts", () => { }); }); -// The channel manager puts the picked chips first, so a toggle moves its chip: along the row, out -// of the "+N" fold, or into it. The keyboard's focus goes with the chip, not to . +// The channel manager puts the picked chips first, so a toggle moves its chip: out of the "+N" fold, +// into it, or out of the list. The keyboard's focus goes with the chip, not to . describe("TagFilterChips focus on a toggle that moves the chip", () => { function Row({ start }: { start: string[] }) { const [picked, setPicked] = useState(start); @@ -146,17 +148,9 @@ describe("TagFilterChips focus on a toggle that moves the chip", () => { const named = (label: string) => screen.getAllByRole("button", { hidden: true }).find((b) => b.textContent === label)!; - it("along the row: the chip keeps the focus where it now stands", () => { - fit.visible = 3; - render(); - press(named("gamma")); - expect(screen.getAllByRole("button").map((b) => b.textContent)).toEqual([ - "gamma", - "alpha", - "beta", - ]); - expect(document.activeElement).toBe(named("gamma")); - }); + // Along the row needs no test of its own: a keyed move within one parent keeps its node, and + // React gives the focus back to it after the commit. Only a remount (row ↔ fold) or a chip that + // leaves the list loses it. it("out of the fold: picked in the popover, it is focused on the row", () => { fit.visible = 1; @@ -167,13 +161,41 @@ describe("TagFilterChips focus on a toggle that moves the chip", () => { const onRow = named("beta"); expect(onRow).not.toBe(inFold); expect(document.activeElement).toBe(onRow); + // …and the fold is shut: the focus is not left outside a popover still open. + expect(menu()).toBeNull(); + expect(moreBtn()).toHaveProperty("ariaExpanded", "false"); + }); + + // A stored pick of a tag that no longer exists is an active chip that an un-pick removes for good. + function WithGhost() { + const [ghost, setGhost] = useState(true); + const rest = ["alpha", "beta"].map(chip); + return ( + setGhost(false) }, ...rest] + : rest + } + /> + ); + } + + it.each([ + ["with room for all", 3], + ["with a fold", 1], + ])("a chip that leaves the list hands the focus to the one now in its place (%s)", (_, n) => { + fit.visible = n; + render(); + press(named("ghost")); + expect(document.activeElement).toBe(named("alpha")); }); it("into a closed fold: the focus goes to the +N that now holds it", () => { fit.visible = 1; render(); press(named("gamma")); - expect(named("alpha").closest('[role="menu"]')).toBeNull(); // alpha leads the row now + expect(named("alpha").closest('[role="dialog"]')).toBeNull(); // alpha leads the row now expect(document.activeElement).toBe(moreBtn()); }); diff --git a/frontend/src/modules/channels/TagFilterChips.tsx b/frontend/src/modules/channels/TagFilterChips.tsx index 64151afe..c9a0c0eb 100644 --- a/frontend/src/modules/channels/TagFilterChips.tsx +++ b/frontend/src/modules/channels/TagFilterChips.tsx @@ -34,19 +34,32 @@ function ChipText({ chip }: { chip: FilterChip }) { return ( <> {chip.label} - {chip.count != null && {chip.count}} + {/* The count in the chip's own colour, set off by a dot rather than dimmed: a dimmed count + falls under the text contrast floor, on a resting chip and on a pressed one. */} + {chip.count != null && ( + <> + {" "} + {chip.count} + + )} ); } -function TagChip({ chip, onToggled }: { chip: FilterChip; onToggled: (key: string) => void }) { +function TagChip({ + chip, + onToggled, +}: { + chip: FilterChip; + onToggled: (key: string, from: HTMLElement) => void; +}) { return ( { // Before the toggle: a pressed chip may move (the caller's order can put picks first), and // the focus must go where it went. - if (e.currentTarget === document.activeElement) onToggled(chip.key); + if (e.currentTarget === document.activeElement) onToggled(chip.key, e.currentTarget); chip.onToggle(); }} pressed={chip.active} @@ -107,31 +120,43 @@ export default function TagFilterChips({ const pop = useAnchoredPopover({ open, onOpenChange: setOpen, - mode: "menu", + // A fold of toggle buttons, not a menu of items: each chip is a pressed/unpressed button in the + // Tab order, which a role=menu with no menu items would hide from a screen reader. + mode: "dialog", + role: "dialog", placement: "bottom-end", }); const { setReference } = pop.refs; const moreLabel = t("channels.tags.moreCount", { count: hidden }); - // A toggle may move its chip — between the row and the "+N" fold (two parents: the button is - // remounted), or along the row (a keyed move that drops the focus to ). The focus follows - // it: onto the chip where it now stands, or onto "+N" when it went into a closed fold. Held for - // the frame, since the fit re-decides in layout effects after the toggle's render. - const follow = useRef(null); + // A toggle may move its chip between the row and the "+N" fold (two parents: the button is + // remounted and the focus falls to ), or take it out of the list altogether (a stored pick of + // a tag that no longer exists). A move along the row keeps its node, and React gives it the focus + // back. The focus follows the chip: onto it where it now stands, onto "+N" when it went into a + // closed fold, or onto the chip now in its old place when it is gone. A toggle in the open fold + // closes it, so the focus never sits outside a popover left open. Held for the frame, since the fit + // re-decides in layout effects after the toggle's render. + const follow = useRef<{ key: string; at: number } | null>(null); const foldRef = useRef(null); const moreRef = useRef(null); - const onToggled = (chipKey: string) => { - follow.current = chipKey; + const onToggled = (chipKey: string, from: HTMLElement) => { + follow.current = { key: chipKey, at: chips.findIndex((c) => c.key === chipKey) }; requestAnimationFrame(() => (follow.current = null)); + if (foldRef.current?.contains(from)) setOpen(false); }; useLayoutEffect(() => { - const chipKey = follow.current; - if (!chipKey) return; - const find = (box: HTMLElement | null) => - [...(box?.querySelectorAll("[data-chip-key]") ?? [])].find( - (b) => b.dataset.chipKey === chipKey, - ); - const el = find(rowRef.current) ?? find(foldRef.current) ?? moreRef.current; + const target = follow.current; + if (!target) return; + const inBox = (box: HTMLElement | null) => [ + ...(box?.querySelectorAll("[data-chip-key]") ?? []), + ]; + const row = inBox(rowRef.current); + const find = (box: HTMLElement[]) => box.find((b) => b.dataset.chipKey === target.key); + const gone = !chips.some((c) => c.key === target.key); + const el = + find(row) ?? + find(inBox(foldRef.current)) ?? + (gone ? (row[Math.min(target.at, row.length - 1)] ?? moreRef.current) : moreRef.current); if (el && document.activeElement !== el) el.focus(); }); const setMore = (el: HTMLButtonElement | null) => { diff --git a/frontend/src/modules/channels/useChannelActions.tsx b/frontend/src/modules/channels/useChannelActions.tsx index b9e24365..bfb2fc61 100644 --- a/frontend/src/modules/channels/useChannelActions.tsx +++ b/frontend/src/modules/channels/useChannelActions.tsx @@ -50,8 +50,10 @@ export function useChannelActions({ const nameOf = (c: ManagedChannel) => c.title == null ? msg("channels.row.thisChannel") : raw(c.title); + // A hide also changes the tags' counts: a tag counts only the channels the Subscriptions tab shows. const refresh = () => { qc.invalidateQueries({ queryKey: qk.channels() }); + qc.invalidateQueries({ queryKey: qk.tags() }); qc.invalidateQueries({ queryKey: qk.myStatus() }); qc.invalidateQueries({ queryKey: qk.feed() }); qc.invalidateQueries({ queryKey: qk.feedSummary() }); diff --git a/frontend/src/modules/feed/ChannelPage.tsx b/frontend/src/modules/feed/ChannelPage.tsx index 57e4895d..6eb1f0e6 100644 --- a/frontend/src/modules/feed/ChannelPage.tsx +++ b/frontend/src/modules/feed/ChannelPage.tsx @@ -194,6 +194,8 @@ export default function ChannelPage({ qc.invalidateQueries({ queryKey: qk.channel(channelId) }); qc.invalidateQueries({ queryKey: qk.feed() }); qc.invalidateQueries({ queryKey: qk.channels() }); + // A tag counts only the channels the Subscriptions tab shows, which a hide leaves out. + qc.invalidateQueries({ queryKey: qk.tags() }); }, }); diff --git a/frontend/src/modules/files/ShareDialog.dom.test.tsx b/frontend/src/modules/files/ShareDialog.dom.test.tsx index 5e8feb4c..d13531eb 100644 --- a/frontend/src/modules/files/ShareDialog.dom.test.tsx +++ b/frontend/src/modules/files/ShareDialog.dom.test.tsx @@ -152,6 +152,59 @@ describe("ShareDialog · opened to share", () => { expect(h.copyText).toHaveBeenCalledTimes(1); }); + // An expired link opens nothing: it is no link to hand out, and reads as dead at a glance. + it("offers no Copy on an expired link, and marks it with a warning", async () => { + h.links = [row(1, "/file/old", lastWeek), row(2, "/file/b", nextWeek)]; + open(); + await waitFor(() => expect(h.copyText).toHaveBeenCalledWith(`${origin}/file/b`)); + expect(screen.getAllByRole("button", { name: "downloads.share.copy" })).toHaveLength(1); + expect(screen.getByText("downloads.share.expiredOn", { exact: false }).className).toContain( + "text-warn", + ); + expect(screen.getAllByRole("link", { name: "downloads.share.openNewTab" })).toHaveLength(2); + }); + + // Closed before the link it made came back, the dialog must not open next time on the list it + // had and make another. + it("makes one link, closed before it came back and opened again", async () => { + h.links = []; + let made: (l: unknown) => void = () => {}; + h.createDownloadLink.mockReturnValue(new Promise((res) => (made = res))); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const dialog = () => ( + + {}} /> + + ); + const first = render(dialog()); + await waitFor(() => expect(h.createDownloadLink).toHaveBeenCalledTimes(1)); + first.unmount(); + made(row(4, "/file/new")); + await new Promise((r) => setTimeout(r, 20)); + h.links = [row(4, "/file/new")]; + render(dialog()); + await screen.findByDisplayValue(`${origin}/file/new`); + await new Promise((r) => setTimeout(r, 20)); + expect(h.createDownloadLink).toHaveBeenCalledTimes(1); + }); + + it("leaves a New link form the user opened while the first link was on its way", async () => { + h.links = []; + let made: (l: unknown) => void = () => {}; + h.createDownloadLink.mockReturnValue(new Promise((res) => (made = res))); + open(); + fireEvent.click(await screen.findByText("downloads.share.newLink")); + fireEvent.change(screen.getByPlaceholderText("downloads.share.password"), { + target: { value: "s3cret" }, + }); + h.links = [row(4, "/file/new")]; + made(row(4, "/file/new")); + await screen.findByDisplayValue(`${origin}/file/new`); + expect(screen.getByPlaceholderText("downloads.share.password").value).toBe( + "s3cret", + ); + }); + // The clipboard holds what was copied last: the link copied on opening stops saying so. it("stops claiming the opening copy once another link is copied", async () => { h.links = [row(2, "/file/b"), row(3, "/file/c")]; diff --git a/frontend/src/modules/files/ShareDialog.tsx b/frontend/src/modules/files/ShareDialog.tsx index cf65128b..b350a5a9 100644 --- a/frontend/src/modules/files/ShareDialog.tsx +++ b/frontend/src/modules/files/ShareDialog.tsx @@ -25,6 +25,7 @@ import { copyText } from "../../lib/clipboard"; import { Input, InnerInput } from "../../components/ui/Input"; import { CheckRow } from "../../components/ui/NativeControls"; import { SURFACE } from "../../components/ui/surfaces"; +import { Badge } from "../../components/ui/Badge"; import UserPicker from "./UserPicker"; import { useCopyFeedback } from "../../components/ui/useCopyFeedback"; import { LoadingState, StateMessage } from "../../components/ui/QueryState"; @@ -126,14 +127,15 @@ function LinkRow({ }, }); + // An expired link opens nothing, so it is no link to hand out: no Copy, and it reads as dead at a + // glance (a warn badge, its address struck through). Open stays, to check it; Revoke, to clear it. + const expired = linkExpired(link); const badges: string[] = []; if (link.has_password) badges.push(t("downloads.share.hasPassword")); if (link.allow_download) badges.push(t("downloads.share.downloadable")); - if (link.expires_at) + if (link.expires_at && !expired) badges.push( - t(linkExpired(link) ? "downloads.share.expiredOn" : "downloads.share.expiresOn", { - date: formatDate(link.expires_at, i18n.language), - }), + t("downloads.share.expiresOn", { date: formatDate(link.expires_at, i18n.language) }), ); return ( @@ -142,14 +144,25 @@ function LinkRow({ {fullUrl ? ( <> - - - {copied || copiedOnOpen ? ( - - ) : ( - - )} - + + {!expired && ( + + {copied || copiedOnOpen ? ( + + ) : ( + + )} + + )} {t("downloads.share.views", { count: link.view_count })} + {expired && link.expires_at && ( + + {t("downloads.share.expiredOn", { date: formatDate(link.expires_at, i18n.language) })} + + )} {badges.map((b) => ( { + // In the list at once, as the one-click Share puts it: closed before the refetch, the dialog + // would open next time on the cached list without it and make another. + onSuccess: (made) => { + qc.setQueryData(linksKey(target), (old) => [made, ...(old ?? [])]); invalidate(); - setCreating(false); - // Back to the form's own default for the next link: downloadable. - setAllowDownload(true); - setExpiryDays(null); - setPassword(""); }, meta: { failTitle: "downloads.share.linkFailed" }, }); + // The form's own success, not the mutation's: the link made on open goes through the same + // mutation, and must not close and clear a form the user opened while it was on its way. + const createFromForm = () => + create.mutate(undefined, { + onSuccess: () => { + setCreating(false); + // Back to the form's own default for the next link: downloadable. + setAllowDownload(true); + setExpiryDays(null); + setPassword(""); + }, + }); // Open with a link already generated: sharing is almost always the goal, so mint a default // (no password, no expiry, downloadable) link the moment the dialog finds none that opens — as if the @@ -419,7 +447,7 @@ function LinkShare({ target }: { target: LinkTarget }) { diff --git a/frontend/src/modules/plex/PlexDiscover.dom.test.tsx b/frontend/src/modules/plex/PlexDiscover.dom.test.tsx index b173b5f0..525f7a5c 100644 --- a/frontend/src/modules/plex/PlexDiscover.dom.test.tsx +++ b/frontend/src/modules/plex/PlexDiscover.dom.test.tsx @@ -204,7 +204,11 @@ describe("PlexDiscover", () => { onSeeAllContinue={onSeeAllContinue} />, ); - fireEvent.click(await screen.findByRole("button", { name: "common.seeAll" })); + fireEvent.click( + await screen.findByRole("button", { + name: "common.seeAllOf:plex.discover.continueWatching", + }), + ); expect(onSeeAllContinue).toHaveBeenCalledOnce(); expect(onSeeAllRecent).not.toHaveBeenCalled(); }); @@ -224,9 +228,10 @@ describe("PlexDiscover", () => { onSeeAllContinue={noop} />, ); - // The last of them: Continue watching, above it, has its own. - const seeAll = await screen.findAllByRole("button", { name: "common.seeAll" }); - fireEvent.click(seeAll[seeAll.length - 1]!); + // Each See all is named with its rail: Continue watching, above it, has its own. + fireEvent.click( + await screen.findByRole("button", { name: "common.seeAllOf:plex.discover.recentlyAdded" }), + ); expect(onSeeAllRecent).toHaveBeenCalledOnce(); }); diff --git a/frontend/src/watch/WatchPage.dom.test.tsx b/frontend/src/watch/WatchPage.dom.test.tsx index 8f2b4e95..32d444f9 100644 --- a/frontend/src/watch/WatchPage.dom.test.tsx +++ b/frontend/src/watch/WatchPage.dom.test.tsx @@ -294,6 +294,22 @@ describe("WatchPage honest failures", () => { expect(page.getByRole("button", { name: button })).toBeTruthy(); }); + it("a repeated Enter sends one try while it is on its way", async () => { + window.history.replaceState({}, "", "/watch/tok-enter"); + const fetchMock = vi.fn().mockResolvedValueOnce(res(200, { needs_password: true })); + fetchMock.mockImplementation(() => new Promise(() => {})); + vi.stubGlobal("fetch", fetchMock); + const page = render(); + const field = await page.findByPlaceholderText("Enter password"); + fireEvent.change(field, { target: { value: "guess" } }); + fireEvent.keyDown(field, { key: "Enter" }); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + fireEvent.keyDown(field, { key: "Enter" }); + fireEvent.keyDown(field, { key: "Enter" }); + await new Promise((r) => setTimeout(r, 20)); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + it("too many password tries says so, and keeps the form", async () => { window.history.replaceState({}, "", "/watch/tok-429"); const page = await tryPassword(async () => res(429)); diff --git a/frontend/src/watch/WatchPage.tsx b/frontend/src/watch/WatchPage.tsx index 6f00b478..848a516f 100644 --- a/frontend/src/watch/WatchPage.tsx +++ b/frontend/src/watch/WatchPage.tsx @@ -259,7 +259,9 @@ export default function WatchPage() { value={password} autoFocus onChange={(e) => setPassword(e.target.value)} - onKeyDown={(e) => e.key === "Enter" && password && unlock()} + // Not again while one is on its way, as the button: a held or repeated Enter would + // spend the link's tries (and its view count) once per press. + onKeyDown={(e) => e.key === "Enter" && password && !unlocking && unlock()} placeholder={T.passwordPlaceholder} aria-describedby={error ? errorId : undefined} />