refactor(feed): address /code-review high findings on the E2 stack
Review round on the E2 stack (S1-S5). No correctness bugs; the surviving low/medium findings addressed: - altitude (#1): extract the shared `tags_by_category` helper used by BOTH filtered_query (SQL tag WHERE) and feed_summary (Python facet/count), removing the category-grouping duplication, and add bidirectional "keep in lockstep — equivalence-tested by test_feed_summary_matches_count_plus_facets" comments so the SQL<->Python tag rule can't silently drift. - test-coverage (#2): the /feed/summary equivalence test seeded only system tags, leaving the `ChannelTag.user_id == user.id` visibility branch untested. Add a user-owned Tag + link + two combos; the suite is now 346 (was 344) and the new cases pass (feed_summary agrees with /feed/count + /facets on user-visible tags). - dead-code (#3): annotate get_feed_count, get_facets, and the FE api.feedCount/ facets as "SUPERSEDED by /feed/summary — kept only as the equivalence oracle; retire together" so the intentionally-retained surface reads as deliberate. Not changed (confirmed no-op): the search channel-name resolve (#4) is once-per-request on a 633-row table and a deliberate pre-resolve for PERF-12's BitmapOr plan; the facet loop (#5) already computes passes() once per (channel, category) with a distinct exclude each, so there is no recomputation. Gate green: ruff + 346 pytest; frontend tsc/knip/prettier (FE change is a comment).
This commit is contained in:
+23
-15
@@ -100,6 +100,16 @@ def serialize(row) -> dict:
|
||||
}
|
||||
|
||||
|
||||
def tags_by_category(db: Session, tag_ids: list[int]) -> dict[str, list[int]]:
|
||||
"""Group tag ids by their category (language/topic/other). The single source for the category split
|
||||
shared by the feed's tag WHERE (filtered_query) and the facet/count summary (feed_summary), so the
|
||||
two can't disagree on which selected tag belongs to which category."""
|
||||
out: dict[str, list[int]] = {}
|
||||
for tid, cat in db.execute(select(Tag.id, Tag.category).where(Tag.id.in_(tag_ids))).all():
|
||||
out.setdefault(cat, []).append(tid)
|
||||
return out
|
||||
|
||||
|
||||
def filtered_query(
|
||||
db: Session,
|
||||
user: User,
|
||||
@@ -281,12 +291,11 @@ def filtered_query(
|
||||
query = query.where(or_(func.unaccent(Video.title).ilike(like), channel_match))
|
||||
|
||||
if tags:
|
||||
# AND across tag categories (e.g. language AND topic narrows), OR within a
|
||||
# category; the any/all toggle controls multiple topic tags.
|
||||
cat_rows = db.execute(select(Tag.id, Tag.category).where(Tag.id.in_(tags))).all()
|
||||
by_category: dict[str, list[int]] = {}
|
||||
for tag_id, category in cat_rows:
|
||||
by_category.setdefault(category, []).append(tag_id)
|
||||
# AND across tag categories (e.g. language AND topic narrows), OR within a category; the any/all
|
||||
# toggle controls multiple topic tags. ⚠️ feed_summary.passes() reimplements this exact rule in
|
||||
# Python for the sidebar count/facets — keep the two in lockstep (equivalence-tested by
|
||||
# test_feed_summary_matches_count_plus_facets).
|
||||
by_category = tags_by_category(db, tags)
|
||||
# Facet counting drops the category being counted so its own chips don't zero each
|
||||
# other out (standard drill-down faceting: a category's count ignores its own
|
||||
# selections but still honours the other categories' filters).
|
||||
@@ -377,17 +386,16 @@ def feed_summary(db: Session, user: User, **params) -> dict:
|
||||
channel_tags.setdefault(cid, set()).add(tid)
|
||||
tag_category[tid] = cat
|
||||
|
||||
# Selected tags grouped by category (read from Tag, so a selected tag that no base channel carries
|
||||
# still filters everything out for its category — matching the SQL `IN (empty)` behaviour).
|
||||
selected_by_cat: dict[str, set[int]] = {}
|
||||
selected = params.get("tags") or []
|
||||
if selected:
|
||||
for tid, cat in db.execute(
|
||||
select(Tag.id, Tag.category).where(Tag.id.in_(selected))
|
||||
).all():
|
||||
selected_by_cat.setdefault(cat, set()).add(tid)
|
||||
# Selected tags grouped by category (read from Tag via the shared helper, so a selected tag that no
|
||||
# base channel carries still filters everything out for its category — matching the SQL `IN (empty)`
|
||||
# behaviour).
|
||||
selected_by_cat: dict[str, set[int]] = {
|
||||
cat: set(ids) for cat, ids in tags_by_category(db, params.get("tags") or []).items()
|
||||
}
|
||||
|
||||
def passes(ch_tags: set[int], exclude_cat: str | None) -> bool:
|
||||
# The Python mirror of filtered_query's SQL tag block (AND across categories, OR within,
|
||||
# topic-AND requires all selected) — keep in lockstep; see the ⚠️ note there.
|
||||
for cat, sel in selected_by_cat.items():
|
||||
if cat == exclude_cat:
|
||||
continue
|
||||
|
||||
@@ -128,6 +128,9 @@ def get_feed_count(
|
||||
user: User = Depends(current_user),
|
||||
db: Session = Depends(get_db),
|
||||
) -> dict:
|
||||
# SUPERSEDED by /feed/summary (PERF-13) — the app no longer calls this. Kept as the reference
|
||||
# oracle that test_feed_summary_matches_count_plus_facets asserts /feed/summary's `count` against;
|
||||
# remove both together with that test if this endpoint is ever retired.
|
||||
query, _rank = filtered_query(db, user, **params)
|
||||
total = db.scalar(select(func.count()).select_from(query.subquery()))
|
||||
return {"count": total or 0}
|
||||
@@ -147,7 +150,11 @@ def get_facets(
|
||||
|
||||
The count is the number of distinct channels that have at least one video in the current
|
||||
view, matching the existing channel-count chip semantics. JSON object keys are strings
|
||||
(tag ids)."""
|
||||
(tag ids).
|
||||
|
||||
SUPERSEDED by /feed/summary (PERF-13) — the app no longer calls this. Kept as the reference
|
||||
oracle for test_feed_summary_matches_count_plus_facets (it asserts /feed/summary's `facets`
|
||||
against these `counts`); retire the two together."""
|
||||
visible = or_(ChannelTag.user_id.is_(None), ChannelTag.user_id == user.id)
|
||||
counts: dict[int, int] = {}
|
||||
for category in ("language", "topic", "other"):
|
||||
|
||||
@@ -96,17 +96,22 @@ def _seed_tagged(db, make_user):
|
||||
"T1": Tag(user_id=None, name="Tech", category="topic"),
|
||||
"T2": Tag(user_id=None, name="Gaming", category="topic"),
|
||||
"O1": Tag(user_id=None, name="Favourite", category="other"),
|
||||
# A USER-owned tag (+ a user-owned ChannelTag link below) so the visibility branch
|
||||
# `ChannelTag.user_id == user.id` is exercised, not only the system `user_id IS NULL` one.
|
||||
"U1": Tag(user_id=user.id, name="MyPin", category="other"),
|
||||
}
|
||||
db.add_all(tags.values())
|
||||
chans = {c: Channel(id=c, title=c, subscriber_count=10) for c in ("chA", "chB", "chC", "chD")}
|
||||
db.add_all(chans.values())
|
||||
db.flush()
|
||||
# channel -> tag keys
|
||||
# channel -> (system) tag keys
|
||||
links = {"chA": ["L1", "T1"], "chB": ["L1", "T2"], "chC": ["L2", "T1", "O1"], "chD": []}
|
||||
for cid, tkeys in links.items():
|
||||
db.add(Subscription(user_id=user.id, channel_id=cid, priority=0, hidden=False))
|
||||
for tk in tkeys:
|
||||
db.add(ChannelTag(channel_id=cid, tag_id=tags[tk].id, user_id=None))
|
||||
# The user's own private link (visible only to them): chB carries U1 for this user.
|
||||
db.add(ChannelTag(channel_id="chB", tag_id=tags["U1"].id, user_id=user.id))
|
||||
# differing video counts per channel (chA:3, chB:2, chC:1, chD:2)
|
||||
counts = {"chA": 3, "chB": 2, "chC": 1, "chD": 2}
|
||||
for cid, n in counts.items():
|
||||
@@ -124,6 +129,8 @@ _SUMMARY_CASES = [
|
||||
"show=all&tags={L1}&tags={T1}",
|
||||
"show=all&tags={T1}&tags={T2}&tag_mode=or",
|
||||
"show=all&tags={T1}&tags={T2}&tag_mode=and",
|
||||
"show=all&tags={U1}", # user-owned tag (visibility == user.id branch)
|
||||
"show=all&tags={U1}&tags={L1}", # user tag AND a system tag, cross-category
|
||||
"show=all&q=clip",
|
||||
"show=unwatched",
|
||||
]
|
||||
|
||||
@@ -113,6 +113,8 @@ function feedQuery(f: FeedFilters, cursor: string | null, limit: number): string
|
||||
export const feedApi = {
|
||||
feed: (f: FeedFilters, cursor: string | null, limit: number): Promise<FeedResponse> =>
|
||||
req(`/api/feed?${feedQuery(f, cursor, limit)}`),
|
||||
// SUPERSEDED by feedSummary (PERF-13) — no component calls this anymore; the backend /feed/count
|
||||
// endpoint it targets is kept only as the /feed/summary equivalence-test oracle. Retire together.
|
||||
feedCount: (f: FeedFilters): Promise<{ count: number }> =>
|
||||
req(`/api/feed/count?${filterParams(f).toString()}`),
|
||||
// Live YouTube search: results are materialised into the catalog and returned as feed cards.
|
||||
@@ -129,6 +131,8 @@ export const feedApi = {
|
||||
// the now-orphaned, un-kept videos). Returns how many videos were removed.
|
||||
clearSearch: (videoIds: string[]): Promise<{ removed: number }> =>
|
||||
req("/api/search/clear", { method: "POST", body: JSON.stringify({ video_ids: videoIds }) }),
|
||||
// SUPERSEDED by feedSummary (PERF-13) — no component calls this anymore; the backend /facets
|
||||
// endpoint it targets is kept only as the /feed/summary equivalence-test oracle. Retire together.
|
||||
facets: (f: FeedFilters): Promise<{ counts: Record<string, number> }> =>
|
||||
req(`/api/facets?${filterParams(f).toString()}`),
|
||||
// The sidebar's total count + per-tag channel facets in ONE request + ONE grouped query, replacing
|
||||
|
||||
Reference in New Issue
Block a user