From 482dd2800650a4a4e86b5398cef2ce14bc42fd8d Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 20 Aug 2026 05:40:59 +0200 Subject: [PATCH] fix(e4): address code-review findings (a11y sweep + dedup guards) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the 5 /code-review high findings on this branch: - a11y sweep: extend aria-label to the remaining icon-only Plex card buttons the first pass missed — PlexPosterCard Info, SeasonCard watched-toggle + add-season (EpisodeCard already had them). "Fix a principle everywhere, in one sweep." - new shared lib/parseId.ts `parsePositiveIntId`, used by BOTH readSelectedId (now guarding the stored ?pl fallback the same as the URL — was unguarded) and plexSubUrl's ?v=playlist decode; removes the hand-duplicated guard. - PAGE_KEY exported from navigationInit and imported by NavigationProvider (was defined in both). - hoist a `watchedLabel` const per card so title + aria-label can't drift. +8 tests (parseId unit + guarded-stored ?pl); full FE gate green (tsc/eslint/ prettier/knip, 317 vitest). --- .../src/components/NavigationProvider.tsx | 6 ++--- frontend/src/components/navigationInit.ts | 2 +- frontend/src/components/playlistsInit.test.ts | 8 +++++++ frontend/src/components/playlistsInit.ts | 16 ++++++------- frontend/src/components/plexCards.tsx | 20 ++++++++++------ frontend/src/components/plexSubUrl.ts | 7 +++--- frontend/src/lib/parseId.test.ts | 23 +++++++++++++++++++ frontend/src/lib/parseId.ts | 9 ++++++++ 8 files changed, 68 insertions(+), 23 deletions(-) create mode 100644 frontend/src/lib/parseId.test.ts create mode 100644 frontend/src/lib/parseId.ts diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index 869d293d..802b8a1d 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -11,10 +11,10 @@ import { import { useTranslation } from "react-i18next"; import { urlWithParams, type Page } from "../lib/urlState"; import { pageTitleKey } from "../lib/pageMeta"; -import { LS, setAccountRaw } from "../lib/storage"; +import { setAccountRaw } from "../lib/storage"; import { useAccountRehydrate } from "../lib/useAccountRehydrate"; import { setNavigator } from "../lib/nav"; -import { channelViewFrom, loadInitialPage, type ChannelView } from "./navigationInit"; +import { channelViewFrom, loadInitialPage, PAGE_KEY, type ChannelView } from "./navigationInit"; // Owns the app's navigation + browser-history state, lifted out of the App god component so pages // consume it via a hook instead of prop-drilling `page`/`setPage`/`onOpenChannel` through the tree. @@ -27,8 +27,6 @@ import { channelViewFrom, loadInitialPage, type ChannelView } from "./navigation // re-render on a page/channel switch — preserving the memo that keeps feed appends from re-rendering // every existing card. -const PAGE_KEY = LS.page; - export interface NavigationState { page: Page; /** The open channel page (null = none). Overlays `page`; rides in history.state._chan. */ diff --git a/frontend/src/components/navigationInit.ts b/frontend/src/components/navigationInit.ts index a57b0c03..da5640bb 100644 --- a/frontend/src/components/navigationInit.ts +++ b/frontend/src/components/navigationInit.ts @@ -5,7 +5,7 @@ import { hasFilterParams, isPage, readPage, type Page } from "../lib/urlState"; import { getAccountRaw, LS } from "../lib/storage"; -const PAGE_KEY = LS.page; +export const PAGE_KEY = LS.page; export interface ChannelView { id: string; diff --git a/frontend/src/components/playlistsInit.test.ts b/frontend/src/components/playlistsInit.test.ts index bdaf7aaa..5e67c970 100644 --- a/frontend/src/components/playlistsInit.test.ts +++ b/frontend/src/components/playlistsInit.test.ts @@ -48,6 +48,14 @@ describe("readSelectedId ?pl guard", () => { expect(readSelectedId()).toBe(9); }); + it.each(["", "0", "-2", "1.5", "abc"])( + "rejects a non-positive-integer STORED value %s (guarded like the URL)", + (raw) => { + stored.v = raw; + expect(readSelectedId()).toBeNull(); + }, + ); + it("returns null when neither ?pl nor a stored selection is present", () => { expect(readSelectedId()).toBeNull(); }); diff --git a/frontend/src/components/playlistsInit.ts b/frontend/src/components/playlistsInit.ts index bc3c256b..9ff88506 100644 --- a/frontend/src/components/playlistsInit.ts +++ b/frontend/src/components/playlistsInit.ts @@ -1,14 +1,14 @@ import { getAccountRaw, LS } from "../lib/storage"; +import { parsePositiveIntId } from "../lib/parseId"; // The Playlists module's selected-id seed, split out of PlaylistsProvider so its ?pl guard is // unit-testable and the provider stays react-refresh-clean (component-only exports). export function readSelectedId(): number | null { - // A ?pl bookmark wins over the stored selection so a pasted link opens that playlist. Accept only a - // positive integer id — a bare "?pl=" gives Number("")===0 (finite!) and "?pl=1.5" a fractional id, - // both of which would select a non-existent playlist. - const fromUrl = new URLSearchParams(window.location.search).get("pl"); - const n = fromUrl ? Number(fromUrl) : NaN; - if (Number.isInteger(n) && n > 0) return n; - const s = getAccountRaw(LS.playlist); - return s ? Number(s) : null; + // A ?pl bookmark wins over the stored selection so a pasted link opens that playlist. BOTH must be a + // positive-integer id (a bare "?pl=" gives Number("")===0, "?pl=1.5" a fractional id) — either would + // select a non-existent playlist, so the stored fallback is guarded exactly like the URL. + return ( + parsePositiveIntId(new URLSearchParams(window.location.search).get("pl")) ?? + parsePositiveIntId(getAccountRaw(LS.playlist)) + ); } diff --git a/frontend/src/components/plexCards.tsx b/frontend/src/components/plexCards.tsx index ffc58495..18595777 100644 --- a/frontend/src/components/plexCards.tsx +++ b/frontend/src/components/plexCards.tsx @@ -50,6 +50,7 @@ export function PlexPosterCard({ const { t } = useTranslation(); const inProgress = (card.position_seconds ?? 0) > 0 && card.status !== "watched"; const isPlayable = card.type !== "show"; // movies/episodes play; shows open the drill-down + const watchedLabel = card.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched"); const pct = inProgress && card.duration_seconds ? Math.min(100, Math.round(((card.position_seconds ?? 0) / card.duration_seconds) * 100)) @@ -99,8 +100,8 @@ export function PlexPosterCard({ e.stopPropagation(); onToggleWatched(card); }} - title={card.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")} - aria-label={card.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")} + title={watchedLabel} + aria-label={watchedLabel} className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition" > @@ -114,6 +115,7 @@ export function PlexPosterCard({ onInfo(); }} title={t("plex.info.title")} + aria-label={t("plex.info.title")} className="absolute bottom-1.5 left-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition" > @@ -192,6 +194,9 @@ export function SeasonCard({ }) { const { t } = useTranslation(); const watched = se.status === "watched"; + const watchedLabel = watched + ? t("plex.series.markSeasonUnwatched") + : t("plex.series.markSeasonWatched"); return (
@@ -241,6 +245,7 @@ export function SeasonCard({ onAdd(); }} title={t("plex.playlist.addSeason")} + aria-label={t("plex.playlist.addSeason")} className="absolute bottom-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition" > @@ -404,6 +409,7 @@ export function EpisodeCard({ }) { const { t } = useTranslation(); const inProgress = (ep.position_seconds ?? 0) > 0 && ep.status !== "watched"; + const watchedLabel = ep.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched"); const pct = inProgress && ep.duration_seconds ? Math.min(100, Math.round(((ep.position_seconds ?? 0) / ep.duration_seconds) * 100)) @@ -438,8 +444,8 @@ export function EpisodeCard({ e.stopPropagation(); onToggleWatched(); }} - title={ep.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")} - aria-label={ep.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")} + title={watchedLabel} + aria-label={watchedLabel} className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition" > diff --git a/frontend/src/components/plexSubUrl.ts b/frontend/src/components/plexSubUrl.ts index d3fc2816..a63e633a 100644 --- a/frontend/src/components/plexSubUrl.ts +++ b/frontend/src/components/plexSubUrl.ts @@ -1,4 +1,5 @@ import type { SubviewUrl } from "../lib/history"; +import { parsePositiveIntId } from "../lib/parseId"; // The Plex module's sub-view union and its address-bar mirror, extracted from PlexBrowse so the // encode/decode round-trip (and its guards) can be unit-tested without pulling the whole component @@ -46,9 +47,9 @@ export const PLEX_SUB_URL: SubviewUrl = { return arg ? { kind: "person", name: arg } : null; case "playlist": { // Positive-integer id only — a fractional/zero/negative ?v=playlist: would open a - // sub-view for a non-existent id (matches the ?pl guard in PlaylistsProvider.readSelectedId). - const id = arg ? Number(arg) : NaN; - return Number.isInteger(id) && id > 0 ? { kind: "playlist", id } : null; + // sub-view for a non-existent id (shares parsePositiveIntId with readSelectedId's ?pl guard). + const id = parsePositiveIntId(arg); + return id != null ? { kind: "playlist", id } : null; } case "season": { const [showId, seasonId] = arg.split(":"); diff --git a/frontend/src/lib/parseId.test.ts b/frontend/src/lib/parseId.test.ts new file mode 100644 index 00000000..fa2d98f1 --- /dev/null +++ b/frontend/src/lib/parseId.test.ts @@ -0,0 +1,23 @@ +import { describe, it, expect } from "vitest"; +import { parsePositiveIntId } from "./parseId"; + +describe("parsePositiveIntId", () => { + it("accepts a positive integer", () => { + expect(parsePositiveIntId("1")).toBe(1); + expect(parsePositiveIntId("42")).toBe(42); + }); + + it("rejects empty / nullish", () => { + expect(parsePositiveIntId("")).toBeNull(); + expect(parsePositiveIntId(null)).toBeNull(); + expect(parsePositiveIntId(undefined)).toBeNull(); + }); + + it("rejects zero, negative, fractional, and non-numeric", () => { + expect(parsePositiveIntId("0")).toBeNull(); + expect(parsePositiveIntId("-1")).toBeNull(); + expect(parsePositiveIntId("1.5")).toBeNull(); + expect(parsePositiveIntId("abc")).toBeNull(); + expect(parsePositiveIntId(" ")).toBeNull(); // Number(" ")===0 + }); +}); diff --git a/frontend/src/lib/parseId.ts b/frontend/src/lib/parseId.ts new file mode 100644 index 00000000..dd46fdb2 --- /dev/null +++ b/frontend/src/lib/parseId.ts @@ -0,0 +1,9 @@ +/** Parse a raw value to a positive-integer id, or null if it isn't one. A bare "" gives + * Number("")===0 (finite but not positive), "1.5" is fractional, "-1"/"abc" invalid — all rejected. + * The single guard behind the ?pl / ?v=playlist id params AND the stored playlist selection, so a bad + * value (a malformed link, a legacy/corrupted storage entry) never selects a non-existent playlist. */ +export function parsePositiveIntId(raw: string | null | undefined): number | null { + if (raw == null || raw === "") return null; + const n = Number(raw); + return Number.isInteger(n) && n > 0 ? n : null; +}