fix(e24): the review's low findings — dead constants, honest labels, a 500 that should be a 404

`add_pin`'s IntegrityError handler assumed the unique constraint was the only way in and
re-raised otherwise, so a target row pruned between `resolve_ref` confirming it and the
INSERT (a Plex sync dropping a removed item) tripped the FK and handed the user a 500.
Semantically that is the same "no such thing" the pre-check already returns. Untested: the
race needs two concurrent transactions.

The five per-kind unique constraints had no test at all — the route's own dedupe returns
before reaching the DB, and the migration drift check filters constraint diffs out as noise,
so they could go missing with every lane green.

Frontend, all from the same review: the shelf's unpin row is now `danger` like every other
destructive row in the app; the hero's artwork button says what it will actually do and
names the item instead of always announcing "Play"; the empty-budget message offers a reroll
rather than `StateMessage`'s "Retry", which promised a re-run of something that had not
failed; `setState` and the watch-later toggle catch their rejections instead of leaving an
unhandled promise and a click that silently does nothing.

`StateMessage` gained a `compact` rung, which retires the frontend's only two `!important`
overrides — they existed to beat the primitive's own padding from the call site, and were
the copyable idiom for the next surface that wanted a compact empty state.

Dead vocabulary removed or made load-bearing: `DISCOVER_BUDGETS` was exported, re-exported
and consumed by nothing while `Home.tsx` re-typed the same four bands (a `readonly
DiscoverBudget[]` accepts any subset, so a fifth band would have typechecked green and
rendered no chip); `PIN_KINDS`' value export had no consumer at all; `useComfortPin` no
longer publishes `pinIdOf`/`toggle`, which nothing outside the module calls.

Three comments corrected against the code they describe: a pointer to a `resolve.py` that
was never written, the file header's claim that Home defines no card component of its own,
and the `seen` window still quoting the 8 that was measured and abandoned for 32.

The rest of the review's survivors are logged on ROADMAP.md under "Deferred — E24 epic-end
deep-review (2026-09-08)" — the ones whose dissent was substantive, and the ones whose fix
is wider than the finding.
This commit is contained in:
2026-09-08 03:23:59 +02:00
parent cedebe0ffb
commit 3709b71a94
9 changed files with 81 additions and 30 deletions
+1 -1
View File
@@ -112,7 +112,7 @@ def from_plex(row: dict) -> dict:
def from_channel(ch: Channel, unwatched: int) -> dict:
"""A pinned channel. It is not itself playable — the shelf resolves it to its newest unwatched
video on click (see `resolve.py`) — so the tile advertises what is waiting there instead of a
video on click (see `pins._shelf_card`) — so the tile advertises what is waiting there instead of a
runtime it does not have."""
card = _base(
source="youtube",
+5 -1
View File
@@ -148,7 +148,11 @@ def add_pin(
)
).scalar_one_or_none()
if winner is None:
raise
# Not the unique constraint, then: the only other way in is the target row being pruned
# between `resolve_ref` confirming it and this INSERT (a Plex sync dropping a removed
# item), which trips the FK. Semantically that is the same "no such thing" the pre-check
# returns above — re-raising handed the user a 500 they could do nothing with.
raise HTTPException(status_code=404, detail="Unknown item")
return {"ok": True, "id": winner.id, "created": False}
return {"ok": True, "id": pin.id, "created": True}
+14
View File
@@ -488,6 +488,20 @@ def test_a_pin_must_reference_exactly_one_thing(db):
db.rollback()
def test_the_same_thing_cannot_be_pinned_twice(db):
"""The per-kind unique constraints are what make the route's concurrent-tab path safe (it catches
`IntegrityError` precisely because of them) — and the migration drift check filters constraint
diffs out as noise, so nothing else would notice them going missing. The route's own dedupe
returns early without ever reaching the DB, so only this test exercises them."""
user, _ = _seed_yt(db)
db.add(ComfortPin(user_id=user.id, video_id="vmed", position=0))
db.commit()
db.add(ComfortPin(user_id=user.id, video_id="vmed", position=1))
with pytest.raises(IntegrityError):
db.flush()
db.rollback()
def test_deleting_the_pinned_row_removes_the_pin(db):
"""A film dropped from the library must not leave a dead tile on the one surface whose whole job
is to be reliable — which is why the shelf holds foreign keys, not strings."""
@@ -50,6 +50,7 @@ vi.mock("../lib/api", () => ({
watchLaterAdd: () => Promise.resolve(),
watchLaterRemove: () => Promise.resolve(),
},
DISCOVER_BUDGETS: ["short", "medium", "long", "any"],
discoverCardKey: (c: { source: string; id: string }) => `${c.source}:${c.id}`,
}));
@@ -59,6 +59,7 @@ vi.mock("../lib/api", () => ({
reorderComfortPins: (...a: unknown[]) => reorderComfortPins(...a),
},
HttpError: class extends Error {},
DISCOVER_BUDGETS: ["short", "medium", "long", "any"],
discoverCardKey: (c: { source: string; id: string }) => `${c.source}:${c.id}`,
}));
+46 -20
View File
@@ -13,6 +13,7 @@ import {
import { qk } from "../lib/queryKeys";
import {
api,
DISCOVER_BUDGETS,
discoverCardKey,
type DiscoverBudget,
type DiscoverCard,
@@ -50,12 +51,16 @@ import { usePlexActions } from "./PlexProvider";
//
// Everything domain-shaped is already built: the cards are the feed's `VideoCard` and Plex's
// `PlexPosterCard`, rendered from the native payload each unified card carries, and the rails are
// the platform `Carousel`. Nothing here is a third card component.
// the platform `Carousel`. The two PLAYABLE card faces are reused rather than re-drawn; the only
// card this file defines is `ShelfTile`, because a pinned channel or playlist has no card anywhere
// in the app and `PosterTile`'s art is hardcoded to a Plex image URL.
// Lazy, exactly as the feed does it: the YouTube IFrame player loads only once something is played.
const PlayerModal = lazy(() => import("./PlayerModal"));
const BUDGETS: readonly DiscoverBudget[] = ["short", "medium", "long", "any"];
// From the api slice's own list, not a second copy: `readonly DiscoverBudget[]` accepts any
// SUBSET, so a band added server-side and to `DISCOVER_BUDGETS` typechecked green while this row
// silently kept offering four.
// A rail tile's slot width. `cardsSmall` is the feed's own dense card (its column bottoms out at
// 180px), so a 200px slot is the same object the feed shows — not a Home-only variant that would
@@ -139,6 +144,9 @@ function Rail({
label: t("home.pin.remove"),
icon: <PinOff className="w-4 h-4" />,
separatorBefore: true,
// The only row here that removes a curated entry, and it sat in the same colour as
// the four harmless reorders. Every other destructive row in the app is `danger`.
danger: true,
// The pin id travels on the shelf card, so this needs no lookup — and unlike every
// other surface it can never be in the "not known yet" state.
onSelect: () => pin.remove(target.pin_id as number, target.title),
@@ -153,7 +161,7 @@ function Rail({
return (
<section className="mb-6">
<h2 className="text-sm font-semibold mb-2">{title}</h2>
<StateMessage className="!py-6">{emptyLabel}</StateMessage>
<StateMessage compact>{emptyLabel}</StateMessage>
</section>
);
}
@@ -288,7 +296,7 @@ function Hero({
className={`relative block w-full overflow-hidden rounded-lg border border-border bg-card ${
card.aspect === "poster" ? "aspect-[2/3] max-w-[12rem]" : "aspect-video"
}`}
aria-label={t("home.hero.play")}
aria-label={`${started ? t("home.hero.continue") : t("home.hero.play")}: ${card.title}`}
>
{card.thumb ? <img src={card.thumb} alt="" className="w-full h-full object-cover" /> : null}
{card.duration_seconds != null && (
@@ -347,8 +355,9 @@ export default function Home() {
const [press, setPress] = useState(0);
// The cards the reroll has already offered this session, newest last. A ROLLING window rather than
// the whole history: an unbounded list would eventually exclude everything and leave the button
// with nothing to give, whereas a window of 8 can never exhaust a library and only repeats after
// eight presses. Kept here (not in the URL or a pref) because it is a property of this sitting.
// with nothing to give, whereas a bounded one cannot. Its size is `SEEN_WINDOW` — see there for
// why it is 32 and not the 8 this comment used to quote. Kept here (not in the URL or a pref)
// because it is a property of this sitting.
const [seen, setSeen] = useState<string[]>([]);
const [player, setPlayer] = useState<{ video: Video; startAt: number | null } | null>(null);
@@ -415,24 +424,31 @@ export default function Home() {
// A full refetch, unlike the feed's stale-mark: these rails are two dozen rows, and marking
// something watched genuinely changes what the page should answer (it can retire the hero).
// The feed avoids this because it would refetch ten infinite-query pages per click.
void api.setState(id, status).then(() => qc.invalidateQueries({ queryKey: qk.discover() }));
void api
.setState(id, status)
.then(() => qc.invalidateQueries({ queryKey: qk.discover() }))
// Without this the rejection escapes unhandled and the click just does nothing — the
// feed's equivalents both catch, and the reorder on this same page already does.
.catch((e: unknown) => notifyActionError(e, t("home.error")));
},
[qc],
[qc, t],
);
const onToggleSave = useCallback(
(id: string, saved: boolean) => {
void (saved ? api.watchLaterAdd(id) : api.watchLaterRemove(id)).then(() => {
qc.invalidateQueries({ queryKey: qk.playlists() });
qc.invalidateQueries({ queryKey: qk.playlist() });
// And the payload the star is READ from. `VideoCard` derives the filled/empty bookmark
// straight from `video.saved` and keeps no optimistic state of its own (the feed instead
// holds a `savedOverrides` map), so without this the icon never changed and a second click
// re-sent the same call — the control looked dead.
qc.invalidateQueries({ queryKey: qk.discover() });
});
void (saved ? api.watchLaterAdd(id) : api.watchLaterRemove(id))
.then(() => {
qc.invalidateQueries({ queryKey: qk.playlists() });
qc.invalidateQueries({ queryKey: qk.playlist() });
// And the payload the star is READ from. `VideoCard` derives the filled/empty bookmark
// straight from `video.saved` and keeps no optimistic state of its own (the feed instead
// holds a `savedOverrides` map), so without this the icon never changed and a second click
// re-sent the same call — the control looked dead.
qc.invalidateQueries({ queryKey: qk.discover() });
})
.catch((e: unknown) => notifyActionError(e, t("home.error")));
},
[qc],
[qc, t],
);
// One click plays. A shelf tile resolves through its `next` (a show → the episode that would
@@ -581,7 +597,7 @@ export default function Home() {
setSeen([]); // a new question deserves the whole library again
setPress(0); // …and the tier rotation starts from the page's own priority order
}}
options={BUDGETS.map((b) => ({ value: b, label: t(`home.budget.${b}`) }))}
options={DISCOVER_BUDGETS.map((b) => ({ value: b, label: t(`home.budget.${b}`) }))}
/>
</div>
@@ -597,9 +613,19 @@ export default function Home() {
// Honest: nothing in the library fits what was asked. Offering something that does not fit
// would make the one control on this page meaningless.
<section className="mb-6 rounded-xl border border-border bg-surface p-4">
<StateMessage className="!py-6" onRetry={budget === "any" ? undefined : reroll}>
{/* Not `StateMessage`'s `onRetry` slot: that button is labelled "Retry", which promises
a re-run of something that failed. Nothing failed — the library simply has no answer
this short, and the honest offer is to ask a different question. */}
<StateMessage compact>
{budget === "any" ? t("home.hero.emptyAny") : t("home.hero.emptyBudget")}
</StateMessage>
{budget !== "any" && (
<div className="flex justify-center pb-4">
<Button variant="outline" size="xs" type="button" onClick={reroll}>
{t("home.hero.reroll")}
</Button>
</div>
)}
</section>
)}
+4 -6
View File
@@ -75,11 +75,9 @@ export function shelfMoveTarget(from: number, to: ShelfMove, length: number): nu
const targetKey = (target: PinTarget): string => `${target.kind}:${target.ref}`;
export interface ComfortPin {
/** The pin's id when the target is on the shelf, `null` when it is not, `undefined` while the
* shelf is still being fetched — three states, because "not pinned" and "not known yet" must not
* render as the same row. */
pinIdOf: (target: PinTarget) => number | null | undefined;
toggle: (target: PinTarget, title: string) => void;
/** `pinIdOf` and `toggle` stay module-local until a second consumer needs them: publishing them
* invites the next surface to re-derive the three-state loading dance (pinned / not pinned /
* not known yet) that `item` already encapsulates, which is what this file exists to prevent. */
remove: (pinId: number, title: string) => void;
/** The pin/unpin row, ready to spread into any `ActionMenuItem[]`. Empty for a null target. */
item: (target: PinTarget | null, title: string) => ActionMenuItem[];
@@ -201,5 +199,5 @@ export function useComfortPin(open: boolean): ComfortPin {
[pinIdOf, t, toggle],
);
return { pinIdOf, toggle, remove, item };
return { remove, item };
}
+8 -1
View File
@@ -29,17 +29,24 @@ export function StateMessage({
children,
onRetry,
tone = "muted",
compact = false,
className = "",
}: {
children: ReactNode;
onRetry?: () => void;
tone?: "muted" | "error";
/** A rung down in vertical rhythm, for a message that sits INSIDE a section that already has its
* own heading and padding (a rail's empty state) rather than standing alone on a page. Here
* rather than at the call site because the only two consumers so far had to reach for `!py-6` to
* beat this component's own `py-10` — the first `!important` overrides in the frontend, and the
* copyable idiom for the next surface that wants a compact empty state. */
compact?: boolean;
className?: string;
}) {
const { t } = useTranslation();
return (
<div
className={`flex flex-col items-center gap-3 py-10 text-sm text-muted ${className}`}
className={`flex flex-col items-center gap-3 ${compact ? "py-6" : "py-10"} text-sm text-muted ${className}`}
// An error is announced promptly (assertive); a plain loading line is polite.
role={tone === "error" ? "alert" : "status"}
aria-live={tone === "error" ? "assertive" : "polite"}
+1 -1
View File
@@ -34,7 +34,7 @@ import { feedApi } from "./api/feed";
export type * from "./api/feed";
import { discoverApi } from "./api/discover";
export type * from "./api/discover";
export { DISCOVER_BUDGETS, PIN_KINDS, discoverCardKey } from "./api/discover";
export { DISCOVER_BUDGETS, discoverCardKey } from "./api/discover";
export type * from "./api/tagModes";
export {
DEFAULT_TAG_MODES,